Fix Action Selection for Routes with Overlapping Templates

src/Microsoft.AspNetCore.Mvc.Versioning/Microsoft.AspNetCore.Mvc.Versioning.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
  <PropertyGroup>
-  <VersionPrefix>1.1.0</VersionPrefix>
+  <VersionPrefix>1.1.1</VersionPrefix>
   <AssemblyVersion>1.1.0.0</AssemblyVersion>
   <TargetFrameworks>net451;netstandard1.6</TargetFrameworks>
   <NetStandardImplicitPackageVersion>1.6.1</NetStandardImplicitPackageVersion>
@@ -11,7 +11,7 @@
   <Description>A service API versioning library for Microsoft ASP.NET Core.</Description>
   <RootNamespace>Microsoft.AspNetCore.Mvc</RootNamespace>
   <PackageTags>Microsoft;AspNet;AspNetCore;Versioning</PackageTags>
-  <PackageReleaseNotes>https://github.com/Microsoft/aspnet-api-versioning/releases/tag/v1.1.0</PackageReleaseNotes>
+  <PackageReleaseNotes>• Fixed overlapped route action selection (Issue #148)</PackageReleaseNotes>
  </PropertyGroup>
 
  <ItemGroup>

src/Microsoft.AspNetCore.Mvc.Versioning/Routing/DefaultApiVersionRoutePolicy.cs

@@ -3,7 +3,6 @@
     using Microsoft.AspNetCore.Http;
     using Microsoft.AspNetCore.Http.Extensions;
     using Microsoft.AspNetCore.Mvc.Abstractions;
-    using Microsoft.AspNetCore.Mvc.ActionConstraints;
     using Microsoft.AspNetCore.Mvc.Infrastructure;
     using Microsoft.AspNetCore.Mvc.Internal;
     using Microsoft.AspNetCore.Routing;
@@ -16,6 +15,7 @@
     using Versioning;
     using static ApiVersion;
     using static System.Environment;
+    using static System.Linq.Enumerable;
     using static System.String;
     using static Versioning.ErrorCodes;
 
@@ -193,10 +193,7 @@ RequestHandler ClientError( RouteContext context, ActionSelectionResult selectio
             var requestedVersion = default( string );
             var parsedVersion = properties.ApiVersion;
             var actionNames = new Lazy<string>( () => Join( NewLine, candidates.Select( a => a.DisplayName ) ) );
-            var allowedMethods = new Lazy<HashSet<string>>(
-                () => new HashSet<string>( candidates.SelectMany( c => c.ActionConstraints.OfType<HttpMethodActionConstraint>() )
-                                                     .SelectMany( ac => ac.HttpMethods ),
-                                           StringComparer.OrdinalIgnoreCase ) );
+            var allowedMethods = new Lazy<HashSet<string>>( () => AllowedMethodsFromCandidates( candidates ) );
             var newRequestHandler = default( Func<IErrorResponseProvider, string, string, RequestHandler> );
 
             if ( parsedVersion == null )
@@ -250,6 +247,29 @@ RequestHandler ClientError( RouteContext context, ActionSelectionResult selectio
             return newRequestHandler( ErrorResponseProvider, code, message );
         }
 
+        static HashSet<string> AllowedMethodsFromCandidates( IEnumerable<ActionDescriptor> candidates )
+        {
+            Contract.Requires( candidates != null );
+            Contract.Ensures( Contract.Result<HashSet<string>>() != null );
+
+            var httpMethods = new HashSet<string>( StringComparer.OrdinalIgnoreCase );
+
+            foreach ( var candidate in candidates )
+            {
+                if ( candidate.ActionConstraints == null )
+                {
+                    continue;
+                }
+
+                foreach ( var constraint in candidate.ActionConstraints.OfType<HttpMethodActionConstraint>() )
+                {
+                    httpMethods.AddRange( constraint.HttpMethods );
+                }
+            }
+
+            return httpMethods;
+        }
+
         sealed class DefaultActionHandler
         {
             readonly IActionContextAccessor actionContextAccessor;

src/Microsoft.AspNetCore.Mvc.Versioning/Versioning/ApiVersionActionSelector.cs

@@ -120,7 +120,8 @@ public virtual ActionDescriptor SelectBestCandidate( RouteContext context, IRead
                 var selectedAction = finalMatches[0];
 
                 // note: short-circuit if the api version policy has already been applied to the match
-                if ( selectedAction.VersionPolicyIsApplied() )
+                // and no other better match has already been selected
+                if ( selectedAction.VersionPolicyIsApplied() && selectionResult.BestMatch == null )
                 {
                     httpContext.ApiVersionProperties().ApiVersion = selectionContext.RequestedVersion;
                     return selectedAction;
@@ -137,7 +138,7 @@ public virtual ActionDescriptor SelectBestCandidate( RouteContext context, IRead
             }
 
             // note: even though we may have had a successful match, this method could be called multiple times. the final decision
-            // is made by the IApiVersionRoutePolicy. we return here to make sure all candidates have been considered.
+            // is made by the IApiVersionRoutePolicy. we return here to make sure all candidates have been considered at least once.
             selectionResult.EndIteration();
             return null;
         }

test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Basic/Controllers/OverlappingRouteTemplateController.cs

@@ -0,0 +1,16 @@
+namespace Microsoft.AspNetCore.Mvc.Basic.Controllers
+{
+    using Microsoft.AspNetCore.Mvc;
+    using System;
+
+    [ApiVersion( "1.0" )]
+    [Route( "api/v{version:apiVersion}/values" )]
+    public class OverlappingRouteTemplateController : Controller
+    {
+        [HttpGet( "{id:int}/{childId}" )]
+        public IActionResult Get( int id, string childId ) => Ok( new { id, childId } );
+
+        [HttpGet( "{id:int}/children" )]
+        public IActionResult Get( int id ) => Ok( new { id } );
+    }
+}

test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Basic/given a versioned Controller/when two route templates overlap.cs

@@ -0,0 +1,51 @@
+namespace given_a_versioned_Controller
+{
+    using FluentAssertions;
+    using Microsoft.AspNetCore.Mvc;
+    using Microsoft.AspNetCore.Mvc.Basic;
+    using Microsoft.AspNetCore.Mvc.Basic.Controllers;
+    using System.Reflection;
+    using System.Threading.Tasks;
+    using Xunit;
+
+    public class when_two_route_templates_overlap : BasicAcceptanceTest
+    {
+        public when_two_route_templates_overlap()
+        {
+            FilteredControllerTypes.Clear();
+            FilteredControllerTypes.Add( typeof( OverlappingRouteTemplateController ).GetTypeInfo() );
+        }
+
+        [Fact]
+        public async Task then_the_higher_precedence_route_should_be_selected_during_the_first_request()
+        {
+            // arrange
+            var response = await Client.GetAsync( "api/v1/values/42/children" );
+            var result1 = await response.EnsureSuccessStatusCode().Content.ReadAsStringAsync();
+
+            // act
+            response = await Client.GetAsync( "api/v1/values/42/abc" );
+            var result2 = await response.EnsureSuccessStatusCode().Content.ReadAsStringAsync();
+
+            // assert
+            result1.Should().Be( "{\"id\":42}" );
+            result2.Should().Be( "{\"id\":42,\"childId\":\"abc\"}" );
+        }
+
+        [Fact]
+        public async Task then_the_higher_precedence_route_should_be_selected_during_the_second_request()
+        {
+            // arrange
+            var response = await Client.GetAsync( "api/v1/values/42/abc" );
+            var result1 = await response.EnsureSuccessStatusCode().Content.ReadAsStringAsync();
+
+            // act
+            response = await Client.GetAsync( "api/v1/values/42/children" );
+            var result2 = await response.EnsureSuccessStatusCode().Content.ReadAsStringAsync();
+
+            // assert
+            result1.Should().Be( "{\"id\":42,\"childId\":\"abc\"}" );
+            result2.Should().Be( "{\"id\":42}" );
+        }
+    }
+}