Fix endpoint policy for invalid candidate with matching API version. Fixes #423.

Author: Chris Martinez

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

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
  <PropertyGroup>
-  <VersionPrefix>3.1.0</VersionPrefix>
+  <VersionPrefix>3.1.1</VersionPrefix>
   <AssemblyVersion>3.1.0.0</AssemblyVersion>
   <TargetFramework>netstandard2.0</TargetFramework>
   <NETStandardImplicitPackageVersion>2.0.0-*</NETStandardImplicitPackageVersion>

src/Microsoft.AspNetCore.Mvc.Versioning/ReleaseNotes.txt

@@ -1,4 +1 @@
-Support Endpoint Routing (#413)
-ApiVersioningOptions.UseApiBehavior defaults to true
-Add IApiControllerSpecification for API controllers
-Add ApiBehaviorSpecification for [ApiController]
+Fix 404 and 405 endpoint matching (#423)

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

@@ -101,53 +101,51 @@ public Task ApplyAsync( HttpContext httpContext, EndpointSelectorContext context
             {
                 for ( var i = 0; i < finalMatches.Count; i++ )
                 {
-                    candidates.SetValidity( finalMatches[i].index, true );
+                    var (index, _, valid) = finalMatches[i];
+                    candidates.SetValidity( index, valid );
                 }
             }
 
             return CompletedTask;
         }
 
-        static IReadOnlyList<(int index, ActionDescriptor action)> EvaluateApiVersion(
+        static IReadOnlyList<(int index, ActionDescriptor action, bool valid)> EvaluateApiVersion(
             HttpContext httpContext,
             CandidateSet candidates,
             ApiVersion apiVersion )
         {
             Contract.Requires( httpContext != null );
             Contract.Requires( candidates != null );
-            Contract.Ensures( Contract.Result<IReadOnlyList<(int index, ActionDescriptor action)>>() != null );
+            Contract.Ensures( Contract.Result<IReadOnlyList<(int, ActionDescriptor, bool)>>() != null );
 
-            var bestMatches = new List<(int index, ActionDescriptor action)>();
-            var implicitMatches = new List<(int, ActionDescriptor)>();
+            var bestMatches = new List<(int index, ActionDescriptor action, bool)>();
+            var implicitMatches = new List<(int, ActionDescriptor, bool)>();
 
             for ( var i = 0; i < candidates.Count; i++ )
             {
-                if ( !candidates.IsValidCandidate( i ) )
-                {
-                    continue;
-                }
-
                 ref var candidate = ref candidates[i];
                 var action = candidate.Endpoint.Metadata?.GetMetadata<ActionDescriptor>();
 
                 if ( action == null )
                 {
-                    candidates.SetValidity( i, false );
                     continue;
                 }
 
+                // remember whether the candidate is currently valid. a matching api version will not
+                // make the candidate valid; however, we want to short-circuit with 400 if no candidates
+                // match the api version at all.
                 switch ( action.MappingTo( apiVersion ) )
                 {
                     case Explicit:
-                        bestMatches.Add( (i, action) );
+                        bestMatches.Add( (i, action, candidates.IsValidCandidate( i )) );
                         break;
                     case Implicit:
-                        implicitMatches.Add( (i, action) );
+                        implicitMatches.Add( (i, action, candidates.IsValidCandidate( i )) );
                         break;
                 }
 
                 // perf: always make the candidate invalid so we only need to loop through the
-                // final, best matches for any remaining, valid candidates
+                // final, best matches for any remaining candidates
                 candidates.SetValidity( i, false );
             }
 

test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Mvc/Basic/given a versioned Controller/when using a query string and split into two types.cs

@@ -60,6 +60,35 @@ public async Task then_get_with_integer_id_should_return_200()
             content.Should().BeEquivalentTo( new { controller = nameof( Values2Controller ), id = 42, version = "2.0" } );
         }
 
+        [Fact]
+        public async Task then_get_returns_400_or_405_with_invalid_id()
+        {
+            // arrange
+            var requestUrl = "api/values/abc?api-version=2.0";
+            var statusCode = UsingEndpointRouting ? NotFound : BadRequest;
+
+            // act
+            var response = await GetAsync( requestUrl );
+
+            // assert
+            response.StatusCode.Should().Be( statusCode );
+        }
+
+        [Theory]
+        [InlineData( "1.0" )]
+        [InlineData( "2.0" )]
+        public async Task then_delete_should_return_405( string apiVersion )
+        {
+            // arrange
+            var requestUrl = $"api/values/42?api-version={apiVersion}";
+
+            // act
+            var response = await DeleteAsync( requestUrl );
+
+            // assert
+            response.StatusCode.Should().Be( MethodNotAllowed );
+        }
+
         [Fact]
         public async Task then_get_should_return_400_for_an_unsupported_version()
         {

test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Mvc/Basic/given a versioned Controller/when using a url segment.cs

@@ -64,6 +64,34 @@ public async Task then_post_should_return_201( string version )
             response.Headers.Location.Should().Be( new Uri( $"http://localhost/api/{version}/HelloWorld/42" ) );
         }
 
+        [Fact]
+        public async Task then_get_returns_400_or_405_with_invalid_id()
+        {
+            // arrange
+            var requestUrl = "api/v2/helloworld/abc";
+            var statusCode = UsingEndpointRouting ? NotFound : BadRequest;
+
+            // act
+            var response = await GetAsync( requestUrl );
+
+            // assert
+            response.StatusCode.Should().Be( statusCode );
+        }
+
+        [Theory]
+        [InlineData( "api/v1/helloworld/42" )]
+        [InlineData( "api/v2/helloworld/42" )]
+        public async Task then_delete_should_return_405( string requestUrl )
+        {
+            // arrange
+
+            // act
+            var response = await DeleteAsync( requestUrl );
+
+            // assert
+            response.StatusCode.Should().Be( MethodNotAllowed );
+        }
+
         [Fact]
         public async Task then_get_should_return_400_for_an_unsupported_version()
         {

test/Microsoft.AspNetCore.Mvc.Versioning.Tests/Routing/ApiVersionMatcherPolicyTest.cs

@@ -156,46 +156,6 @@ public async Task apply_should_use_400_endpoint_for_unmatched_api_version()
             errorResponses.Verify( er => er.CreateResponse( It.Is<ErrorResponseContext>( c => c.StatusCode == 400 && c.ErrorCode == "UnsupportedApiVersion" ) ), Once() );
         }
 
-        [Fact]
-        public async Task apply_should_use_405_endpoint_for_unmatched_api_version()
-        {
-            // arrange
-            var feature = new Mock<IApiVersioningFeature>();
-            var errorResponses = new Mock<IErrorResponseProvider>();
-            var result = new Mock<IActionResult>();
-
-            feature.SetupProperty( f => f.RawRequestedApiVersion, "1.0" );
-            feature.SetupProperty( f => f.RequestedApiVersion, new ApiVersion( 1, 0 ) );
-            result.Setup( r => r.ExecuteResultAsync( It.IsAny<ActionContext>() ) ).Returns( CompletedTask );
-            errorResponses.Setup( er => er.CreateResponse( It.IsAny<ErrorResponseContext>() ) ).Returns( result.Object );
-
-            var options = Options.Create( new ApiVersioningOptions() { ErrorResponses = errorResponses.Object } );
-            var policy = new ApiVersionMatcherPolicy( options, NewReporter(), NewLoggerFactory() );
-            var context = new EndpointSelectorContext();
-            var items = new object[]
-            {
-                new ActionDescriptor()
-                {
-                    DisplayName = "Test",
-                    ActionConstraints = new IActionConstraintMetadata[]{ new HttpMethodActionConstraint(new[] { "POST" }) },
-                    Properties = { [typeof( ApiVersionModel )] = new ApiVersionModel( new ApiVersion( 1, 0 ) ) },
-                },
-            };
-            var endpoint = new Endpoint( c => CompletedTask, new EndpointMetadataCollection( items ), default );
-            var candidates = new CandidateSet( new[] { endpoint }, new[] { new RouteValueDictionary() }, new[] { 0 } );
-            var httpContext = NewHttpContext( feature );
-
-            candidates.SetValidity( 0, false );
-
-            // act
-            await policy.ApplyAsync( httpContext, context, candidates );
-            await context.Endpoint.RequestDelegate( httpContext );
-
-            // assert
-            result.Verify( r => r.ExecuteResultAsync( It.IsAny<ActionContext>() ), Once() );
-            errorResponses.Verify( er => er.CreateResponse( It.Is<ErrorResponseContext>( c => c.StatusCode == 405 && c.ErrorCode == "UnsupportedApiVersion" ) ), Once() );
-        }
-
         [Fact]
         public async Task apply_should_use_400_endpoint_for_invalid_api_version()
         {