Implicit endpoint with better score should have precedence over explicit endpoint with worse score. Fixes #884

Author: commonsensesoftware

Diff for: src/AspNetCore/Acceptance/Asp.Versioning.Mvc.Acceptance.Tests/Mvc/UsingAttributes/Controllers/OverlappingRouteTemplateController.cs

@@ -1,11 +1,14 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 
+#pragma warning disable CA1822 // Mark members as static
+
 namespace Asp.Versioning.Mvc.UsingAttributes.Controllers;
 
 using Microsoft.AspNetCore.Mvc;
 
 [ApiController]
 [ApiVersion( "1.0" )]
+[ApiVersion( "2.0" )]
 [Route( "api/v{version:apiVersion}/values" )]
 public class OverlappingRouteTemplateController : ControllerBase
 {
@@ -20,4 +23,11 @@ public class OverlappingRouteTemplateController : ControllerBase
 
     [HttpGet( "{id:int}/ambiguous" )]
     public IActionResult Ambiguous2( int id ) => Ok();
+
+    [HttpGet( "[action]" )]
+    public string Echo() => "Test";
+
+    [HttpGet( "[action]/{id}" )]
+    [MapToApiVersion( "1.0" )]
+    public string Echo( string id ) => id;
 }

Diff for: src/AspNetCore/Acceptance/Asp.Versioning.Mvc.Acceptance.Tests/Mvc/UsingAttributes/given a versioned Controller/when two route templates overlap.cs

@@ -4,6 +4,8 @@ namespace given_a_versioned_Controller;
 
 using Asp.Versioning;
 using Asp.Versioning.Mvc.UsingAttributes;
+using System.Net;
+using static System.Net.HttpStatusCode;
 
 public class when_two_route_templates_overlap : AcceptanceTest, IClassFixture<OverlappingRouteTemplateFixture>
 {
@@ -54,6 +56,40 @@ public async Task then_the_higher_precedence_route_should_result_in_ambiguous_ac
         ( await act.Should().ThrowAsync<Exception>() ).And.GetType().Name.Should().Be( "AmbiguousMatchException" );
     }
 
+    [Theory]
+    [InlineData( "api/v1/values/echo" )]
+    [InlineData( "api/v2/values/echo" )]
+    public async Task then_route_with_same_score_and_version_should_return_200( string requestUri )
+    {
+        // arrange
+
+
+        // act
+        var response = await GetAsync( requestUri );
+
+        // assert
+        response.StatusCode.Should().Be( OK );
+    }
+
+    [Theory]
+    [InlineData( "api/v1/values/echo/42", OK )]
+#if NETCOREAPP3_1
+    [InlineData( "api/v2/values/echo/42", BadRequest )]
+#else
+    [InlineData( "api/v2/values/echo/42", NotFound )]
+#endif
+    public async Task then_route_with_same_score_and_different_versions_should_return_expected_status( string requestUri, HttpStatusCode statusCode )
+    {
+        // arrange
+
+
+        // act
+        var response = await GetAsync( requestUri );
+
+        // assert
+        response.StatusCode.Should().Be( statusCode );
+    }
+
     public when_two_route_templates_overlap( OverlappingRouteTemplateFixture fixture, ITestOutputHelper console )
         : base( fixture ) => console.WriteLine( fixture.DirectedGraphVisualizationUrl );
 }

Diff for: src/AspNetCore/WebApi/src/Asp.Versioning.Http/Routing/ApiVersionMatcherPolicy.cs

@@ -8,6 +8,7 @@ namespace Asp.Versioning.Routing;
 using Microsoft.AspNetCore.Routing.Patterns;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
+using System.Buffers;
 using System.Runtime.CompilerServices;
 using System.Text.RegularExpressions;
 using static Asp.Versioning.ApiVersionMapping;
@@ -261,7 +262,7 @@ private static bool DifferByRouteConstraintsOnly( CandidateSet candidates )
 
         for ( var i = 0; i < candidates.Count; i++ )
         {
-            ref var candidate = ref candidates[i];
+            ref readonly var candidate = ref candidates[i];
 
             if ( candidate.Endpoint is not RouteEndpoint endpoint )
             {
@@ -291,81 +292,85 @@ private static bool DifferByRouteConstraintsOnly( CandidateSet candidates )
 
     private static (bool Matched, bool HasCandidates) MatchApiVersion( CandidateSet candidates, ApiVersion? apiVersion )
     {
-        List<int>? bestMatches = default;
-        List<int>? implicitMatches = default;
+        var total = candidates.Count;
+        var count = 0;
+        var array = default( Match[] );
+        var bestMatch = default( Match? );
         var hasCandidates = false;
+        Span<Match> matches =
+            total <= 16
+            ? stackalloc Match[total]
+            : ( array = ArrayPool<Match>.Shared.Rent( total ) ).AsSpan();
 
-        for ( var i = 0; i < candidates.Count; i++ )
+        for ( var i = 0; i < total; i++ )
         {
             if ( !candidates.IsValidCandidate( i ) )
             {
                 continue;
             }
 
             hasCandidates = true;
-            ref var candidate = ref candidates[i];
+            ref readonly var candidate = ref candidates[i];
             var metadata = candidate.Endpoint.Metadata.GetMetadata<ApiVersionMetadata>();
 
             if ( metadata == null )
             {
                 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.
+            var score = candidate.Score;
+            bool isExplicit;
+
+            // perf: always make the candidate invalid so we only need to loop through the
+            // final, best matches for any remaining candidates
+            candidates.SetValidity( i, false );
+
             switch ( metadata.MappingTo( apiVersion ) )
             {
                 case Explicit:
-                    bestMatches ??= new();
-                    bestMatches.Add( i );
+                    isExplicit = true;
                     break;
                 case Implicit:
-                    implicitMatches ??= new();
-                    implicitMatches.Add( i );
+                    isExplicit = metadata.IsApiVersionNeutral;
                     break;
+                default:
+                    continue;
             }
 
-            // perf: always make the candidate invalid so we only need to loop through the
-            // final, best matches for any remaining candidates
-            candidates.SetValidity( i, false );
-        }
+            var match = new Match( i, score, isExplicit );
 
-        if ( bestMatches is null )
-        {
-            if ( implicitMatches is null )
-            {
-                return (false, hasCandidates);
-            }
+            matches[count++] = match;
 
-            for ( var i = 0; i < implicitMatches.Count; i++ )
+            if ( !bestMatch.HasValue || match.CompareTo( bestMatch.Value ) > 0 )
             {
-                candidates.SetValidity( implicitMatches[i], true );
+                bestMatch = match;
             }
-
-            return (true, hasCandidates);
         }
 
-        if ( bestMatches.Count == 1 && implicitMatches is not null )
+        var matched = false;
+
+        if ( bestMatch.HasValue )
         {
-            ref var candidate = ref candidates[bestMatches[0]];
-            var metadata = candidate.Endpoint.Metadata.GetMetadata<ApiVersionMetadata>()!;
+            matched = true;
+            var match = bestMatch.Value;
 
-            if ( metadata.IsApiVersionNeutral )
+            for ( var i = 0; i < count; i++ )
             {
-                for ( var i = 0; i < implicitMatches.Count; i++ )
+                ref readonly var otherMatch = ref matches[i];
+
+                if ( match.CompareTo( otherMatch ) == 0 )
                 {
-                    candidates.SetValidity( implicitMatches[i], true );
+                    candidates.SetValidity( otherMatch.Index, true );
                 }
             }
         }
 
-        for ( var i = 0; i < bestMatches.Count; i++ )
+        if ( array is not null )
         {
-            candidates.SetValidity( bestMatches[i], true );
+            ArrayPool<Match>.Shared.Return( array );
         }
 
-        return (true, hasCandidates);
+        return (matched, hasCandidates);
     }
 
     private ApiVersion TrySelectApiVersion( HttpContext httpContext, CandidateSet candidates )
@@ -393,4 +398,24 @@ private ApiVersion TrySelectApiVersion( HttpContext httpContext, CandidateSet ca
 
     bool INodeBuilderPolicy.AppliesToEndpoints( IReadOnlyList<Endpoint> endpoints ) =>
         !ContainsDynamicEndpoints( endpoints ) && AppliesToEndpoints( endpoints );
+
+    private readonly struct Match
+    {
+        internal readonly int Index;
+        internal readonly int Score;
+        internal readonly bool IsExplicit;
+
+        internal Match( int index, int score, bool isExplicit )
+        {
+            Index = index;
+            Score = score;
+            IsExplicit = isExplicit;
+        }
+
+        internal int CompareTo( in Match other )
+        {
+            var result = -Score.CompareTo( other.Score );
+            return result == 0 ? IsExplicit.CompareTo( other.IsExplicit ) : result;
+        }
+    }
 }