From 7da1baf9d844472a421133e18a7b0ee03cadefed Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Mon, 23 Jul 2018 11:06:41 -0700 Subject: [PATCH] Do not use decision tree for named routes in RouteValuesBasedEndpointFinder --- .../RouteValuesBasedEndpointFinder.cs | 57 ++++++++------- .../RouteValueBasedEndpointFinderTest.cs | 71 ++++++++++++++++--- 2 files changed, 94 insertions(+), 34 deletions(-) diff --git a/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs b/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs index eb51c49967..2c3afaf55d 100644 --- a/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs +++ b/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs @@ -18,7 +18,7 @@ namespace Microsoft.AspNetCore.Routing private readonly CompositeEndpointDataSource _endpointDataSource; private readonly ObjectPool _objectPool; private LinkGenerationDecisionTree _allMatchesLinkGenerationTree; - private IDictionary _namedMatches; + private IDictionary> _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( - context.ExplicitValues, - context.AmbientValues); + matchResults = namedMatchResults; } 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(); - _namedMatches = GetNamedMatches(namedMatches); + var (allMatches, namedMatchResults) = GetOutboundMatches(); + _namedMatchResults = namedMatchResults; _allMatchesLinkGenerationTree = new LinkGenerationDecisionTree(allMatches.ToArray()); } - protected virtual (IEnumerable, IDictionary>) 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, IDictionary>) GetOutboundMatches() { var allOutboundMatches = new List(); - var namedOutboundMatches = new Dictionary>(StringComparer.OrdinalIgnoreCase); + var namedOutboundMatchResults = new Dictionary>( + StringComparer.OrdinalIgnoreCase); var endpoints = _endpointDataSource.Endpoints.OfType(); foreach (var endpoint in endpoints) @@ -101,16 +117,16 @@ namespace Microsoft.AspNetCore.Routing continue; } - List matches; - if (!namedOutboundMatches.TryGetValue(entry.RouteName, out matches)) + List matchResults; + if (!namedOutboundMatchResults.TryGetValue(entry.RouteName, out matchResults)) { - matches = new List(); - namedOutboundMatches.Add(entry.RouteName, matches); + matchResults = new List(); + 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 GetNamedMatches( - IDictionary> namedOutboundMatches) - { - var result = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (var namedOutboundMatch in namedOutboundMatches) - { - result.Add(namedOutboundMatch.Key, new LinkGenerationDecisionTree(namedOutboundMatch.Value.ToArray())); - } - return result; - } } } diff --git a/test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs index 7ba5b92f90..6296604d2a 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs +++ b/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(namedMatch.Entry.Data); + var actual = Assert.IsType(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(namedMatches[0].Entry.Data)); - Assert.Same(endpoint3, Assert.IsType(namedMatches[1].Entry.Data)); + Assert.Same(endpoint2, Assert.IsType(namedMatches[0].Match.Entry.Data)); + Assert.Same(endpoint3, Assert.IsType(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(namedMatches[0].Entry.Data)); - Assert.Same(endpoint3, Assert.IsType(namedMatches[1].Entry.Data)); + Assert.Same(endpoint2, Assert.IsType(namedMatches[0].Match.Entry.Data)); + Assert.Same(endpoint3, Assert.IsType(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(namedMatch.Entry.Data); + var actual = Assert.IsType(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 AllMatches { get; private set; } - public IDictionary> NamedMatches { get; private set; } + public IDictionary> NamedMatches { get; private set; } - protected override (IEnumerable, IDictionary>) GetOutboundMatches() + protected override (IEnumerable, IDictionary>) GetOutboundMatches() { var matches = base.GetOutboundMatches(); AllMatches = matches.Item1;