Do not use decision tree for named routes in RouteValuesBasedEndpointFinder

src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs

@@ -18,7 +18,7 @@ namespace Microsoft.AspNetCore.Routing
         private readonly CompositeEndpointDataSource _endpointDataSource;
         private readonly ObjectPool<UriBuildingContext> _objectPool;
         private LinkGenerationDecisionTree _allMatchesLinkGenerationTree;
-        private IDictionary<string, LinkGenerationDecisionTree> _namedMatches;
+        private IDictionary<string, List<OutboundMatchResult>> _namedMatchResults;
 
         public RouteValuesBasedEndpointFinder(
             CompositeEndpointDataSource endpointDataSource,
@@ -45,11 +45,9 @@ namespace Microsoft.AspNetCore.Routing
                     context.ExplicitValues,
                     context.AmbientValues);
             }
-            else if (_namedMatches.TryGetValue(context.RouteName, out var linkGenerationTree))
+            else if (_namedMatchResults.TryGetValue(context.RouteName, out var namedMatchResults))
             {
-                matchResults = linkGenerationTree.GetMatches(
+                matchResults = namedMatchResults;
-                    context.ExplicitValues,
-                    context.AmbientValues);
             }
 
             if (matchResults == null || !matchResults.Any())
@@ -78,15 +76,33 @@ namespace Microsoft.AspNetCore.Routing
         {
             // Refresh the matches in the case where a datasource's endpoints changes. The following is OK to do
             // as refresh of new endpoints happens within a lock and also these fields are not publicly accessible.
-            var (allMatches, namedMatches) = GetOutboundMatches();
+            var (allMatches, namedMatchResults) = GetOutboundMatches();
-            _namedMatches = GetNamedMatches(namedMatches);
+            _namedMatchResults = namedMatchResults;
             _allMatchesLinkGenerationTree = new LinkGenerationDecisionTree(allMatches.ToArray());
         }
 
-        protected virtual (IEnumerable<OutboundMatch>, IDictionary<string, List<OutboundMatch>>) GetOutboundMatches()
+        /// Decision tree is built using the 'required values' of actions.
+        /// - When generating a url using route values, decision tree checks the explicitly supplied route values +
+        ///   ambient values to see if they have a match for the required-values-based-tree.
+        /// - When generating a url using route name, route values for controller, action etc.might not be provided
+        ///   (this is expected because as a user I want to avoid writing all those and instead chose to use a
+        ///   routename which is quick). So since these values are not provided and might not be even in ambient
+        ///   values, decision tree would fail to find a match. So for this reason decision tree is not used for named
+        ///   matches. Instead all named matches are returned as is and the LinkGenerator uses a TemplateBinder to
+        ///   decide which of the matches can generate a url.
+        ///   For example, for a route defined like below with current ambient values like new { controller = "Home",
+        ///   action = "Index" }
+        ///     "api/orders/{id}",
+        ///     routeName: "OrdersApi",
+        ///     defaults: new { controller = "Orders", action = "GetById" },
+        ///     requiredValues: new { controller = "Orders", action = "GetById" },
+        ///   A call to GetLink("OrdersApi", new { id = "10" }) cannot generate url as neither the supplied values or
+        ///   current ambient values do not satisfy the decision tree that is built based on the required values.
+        protected virtual (IEnumerable<OutboundMatch>, IDictionary<string, List<OutboundMatchResult>>) GetOutboundMatches()
         {
             var allOutboundMatches = new List<OutboundMatch>();
-            var namedOutboundMatches = new Dictionary<string, List<OutboundMatch>>(StringComparer.OrdinalIgnoreCase);
+            var namedOutboundMatchResults = new Dictionary<string, List<OutboundMatchResult>>(
+                StringComparer.OrdinalIgnoreCase);
 
             var endpoints = _endpointDataSource.Endpoints.OfType<MatcherEndpoint>();
             foreach (var endpoint in endpoints)
@@ -101,16 +117,16 @@ namespace Microsoft.AspNetCore.Routing
                     continue;
                 }
 
-                List<OutboundMatch> matches;
+                List<OutboundMatchResult> matchResults;
-                if (!namedOutboundMatches.TryGetValue(entry.RouteName, out matches))
+                if (!namedOutboundMatchResults.TryGetValue(entry.RouteName, out matchResults))
                 {
-                    matches = new List<OutboundMatch>();
+                    matchResults = new List<OutboundMatchResult>();
-                    namedOutboundMatches.Add(entry.RouteName, matches);
+                    namedOutboundMatchResults.Add(entry.RouteName, matchResults);
                 }
-                matches.Add(outboundMatch);
+                matchResults.Add(new OutboundMatchResult(outboundMatch, isFallbackMatch: false));
             }
 
-            return (allOutboundMatches, namedOutboundMatches);
+            return (allOutboundMatches, namedOutboundMatchResults);
         }
 
         private OutboundRouteEntry CreateOutboundRouteEntry(MatcherEndpoint endpoint)
@@ -129,16 +145,5 @@ namespace Microsoft.AspNetCore.Routing
             entry.Defaults = new RouteValueDictionary(endpoint.RoutePattern.Defaults);
             return entry;
         }
-
-        private IDictionary<string, LinkGenerationDecisionTree> GetNamedMatches(
-            IDictionary<string, List<OutboundMatch>> namedOutboundMatches)
-        {
-            var result = new Dictionary<string, LinkGenerationDecisionTree>(StringComparer.OrdinalIgnoreCase);
-            foreach (var namedOutboundMatch in namedOutboundMatches)
-            {
-                result.Add(namedOutboundMatch.Key, new LinkGenerationDecisionTree(namedOutboundMatch.Value.ToArray()));
-            }
-            return result;
-        }
     }
 }

test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs

@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Routing.EndpointFinders;
 using Microsoft.AspNetCore.Routing.Internal;
 using Microsoft.AspNetCore.Routing.Matchers;
 using Microsoft.AspNetCore.Routing.Patterns;
@@ -33,7 +34,7 @@ namespace Microsoft.AspNetCore.Routing
             Assert.NotNull(finder.NamedMatches);
             Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches));
             var namedMatch = Assert.Single(namedMatches);
-            var actual = Assert.IsType<MatcherEndpoint>(namedMatch.Entry.Data);
+            var actual = Assert.IsType<MatcherEndpoint>(namedMatch.Match.Entry.Data);
             Assert.Same(endpoint2, actual);
         }
 
@@ -54,8 +55,8 @@ namespace Microsoft.AspNetCore.Routing
             Assert.NotNull(finder.NamedMatches);
             Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches));
             Assert.Equal(2, namedMatches.Count);
-            Assert.Same(endpoint2, Assert.IsType<MatcherEndpoint>(namedMatches[0].Entry.Data));
+            Assert.Same(endpoint2, Assert.IsType<MatcherEndpoint>(namedMatches[0].Match.Entry.Data));
-            Assert.Same(endpoint3, Assert.IsType<MatcherEndpoint>(namedMatches[1].Entry.Data));
+            Assert.Same(endpoint3, Assert.IsType<MatcherEndpoint>(namedMatches[1].Match.Entry.Data));
         }
 
         [Fact]
@@ -75,8 +76,8 @@ namespace Microsoft.AspNetCore.Routing
             Assert.NotNull(finder.NamedMatches);
             Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches));
             Assert.Equal(2, namedMatches.Count);
-            Assert.Same(endpoint2, Assert.IsType<MatcherEndpoint>(namedMatches[0].Entry.Data));
+            Assert.Same(endpoint2, Assert.IsType<MatcherEndpoint>(namedMatches[0].Match.Entry.Data));
-            Assert.Same(endpoint3, Assert.IsType<MatcherEndpoint>(namedMatches[1].Entry.Data));
+            Assert.Same(endpoint3, Assert.IsType<MatcherEndpoint>(namedMatches[1].Match.Entry.Data));
         }
 
         [Fact]
@@ -98,7 +99,7 @@ namespace Microsoft.AspNetCore.Routing
             Assert.NotNull(finder.NamedMatches);
             Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches));
             var namedMatch = Assert.Single(namedMatches);
-            var actual = Assert.IsType<MatcherEndpoint>(namedMatch.Entry.Data);
+            var actual = Assert.IsType<MatcherEndpoint>(namedMatch.Match.Entry.Data);
             Assert.Same(endpoint2, actual);
         }
 
@@ -169,6 +170,60 @@ namespace Microsoft.AspNetCore.Routing
                 });
         }
 
+        [Fact]
+        public void FindEndpoints_ReturnsEndpoint_WhenLookedUpByRouteName()
+        {
+            // Arrange
+            var expected = CreateEndpoint(
+                "api/orders/{id}",
+                defaults: new { controller = "Orders", action = "GetById" },
+                requiredValues: new { controller = "Orders", action = "GetById" },
+                routeName: "OrdersApi");
+            var finder = CreateEndpointFinder(expected);
+
+            // Act
+            var foundEndpoints = finder.FindEndpoints(
+                new RouteValuesBasedEndpointFinderContext
+                {
+                    ExplicitValues = new RouteValueDictionary(new { id = 10 }),
+                    AmbientValues = new RouteValueDictionary(new { controller = "Home", action = "Index" }),
+                    RouteName = "OrdersApi"
+                });
+
+            // Assert
+            var actual = Assert.Single(foundEndpoints);
+            Assert.Same(expected, actual);
+        }
+
+        [Fact]
+        public void FindEndpoints_AlwaysReturnsEndpointsByRouteName_IgnoringMissingRequiredParameterValues()
+        {
+            // Here 'id' is the required value. The endpoint finder would always return an endpoint by looking up
+            // name only. Its the link generator which uses these endpoints finally to generate a link or not
+            // based on the required parameter values being present or not.
+
+            // Arrange
+            var expected = CreateEndpoint(
+                "api/orders/{id}",
+                defaults: new { controller = "Orders", action = "GetById" },
+                requiredValues: new { controller = "Orders", action = "GetById" },
+                routeName: "OrdersApi");
+            var finder = CreateEndpointFinder(expected);
+
+            // Act
+            var foundEndpoints = finder.FindEndpoints(
+                new RouteValuesBasedEndpointFinderContext
+                {
+                    ExplicitValues = new RouteValueDictionary(),
+                    AmbientValues = new RouteValueDictionary(),
+                    RouteName = "OrdersApi"
+                });
+
+            // Assert
+            var actual = Assert.Single(foundEndpoints);
+            Assert.Same(expected, actual);
+        }
+
         private CustomRouteValuesBasedEndpointFinder CreateEndpointFinder(params Endpoint[] endpoints)
         {
             return CreateEndpointFinder(new DefaultEndpointDataSource(endpoints));
@@ -239,9 +294,9 @@ namespace Microsoft.AspNetCore.Routing
 
             public IEnumerable<OutboundMatch> AllMatches { get; private set; }
 
-            public IDictionary<string, List<OutboundMatch>> NamedMatches { get; private set; }
+            public IDictionary<string, List<OutboundMatchResult>> NamedMatches { get; private set; }
 
-            protected override (IEnumerable<OutboundMatch>, IDictionary<string, List<OutboundMatch>>) GetOutboundMatches()
+            protected override (IEnumerable<OutboundMatch>, IDictionary<string, List<OutboundMatchResult>>) GetOutboundMatches()
             {
                 var matches = base.GetOutboundMatches();
                 AllMatches = matches.Item1;