diff --git a/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs b/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs index b5b3721b2324..a2597121097b 100644 --- a/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs +++ b/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -6,6 +6,7 @@ using System.Linq; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing.Patterns; +using Microsoft.AspNetCore.Routing.Template; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Routing.Matching @@ -52,6 +53,9 @@ public DfaMatcherBuilder( _constraints = new List>(); } + // Used in tests + internal EndpointComparer Comparer => _comparer; + public override void AddEndpoint(RouteEndpoint endpoint) { _endpoints.Add(endpoint); @@ -59,17 +63,11 @@ public override void AddEndpoint(RouteEndpoint endpoint) public DfaNode BuildDfaTree(bool includeLabel = false) { - // We build the tree by doing a BFS over the list of entries. This is important - // because a 'parameter' node can also traverse the same paths that literal nodes - // traverse. This means that we need to order the entries first, or else we will - // miss possible edges in the DFA. - _endpoints.Sort(_comparer); - // Since we're doing a BFS we will process each 'level' of the tree in stages // this list will hold the set of items we need to process at the current // stage. - var work = new List<(RouteEndpoint endpoint, List parents)>(_endpoints.Count); - List<(RouteEndpoint endpoint, List parents)> previousWork = null; + var work = new List<(RouteEndpoint endpoint, int precedenceDigit, List parents)>(_endpoints.Count); + List<(RouteEndpoint endpoint, int precedenceDigit, List parents)> previousWork = null; var root = new DfaNode() { PathDepth = 0, Label = includeLabel ? "/" : null }; @@ -79,21 +77,37 @@ public DfaNode BuildDfaTree(bool includeLabel = false) for (var i = 0; i < _endpoints.Count; i++) { var endpoint = _endpoints[i]; - maxDepth = Math.Max(maxDepth, endpoint.RoutePattern.PathSegments.Count); + var precedenceDigit = GetPrecedenceDigitAtDepth(endpoint, depth: 0); + work.Add((endpoint, precedenceDigit, new List() { root, })); - work.Add((endpoint, new List() { root, })); + maxDepth = Math.Max(maxDepth, endpoint.RoutePattern.PathSegments.Count); } + var workCount = work.Count; + // Sort work at each level by *PRECEDENCE OF THE CURRENT SEGMENT*. + // + // We build the tree by doing a BFS over the list of entries. This is important + // because a 'parameter' node can also traverse the same paths that literal nodes + // traverse. This means that we need to order the entries first, or else we will + // miss possible edges in the DFA. + // + // We'll sort the matches again later using the *real* comparer once building the + // precedence part of the DFA is over. + var precedenceDigitComparer = Comparer<(RouteEndpoint endpoint, int precedenceDigit, List parents)>.Create((x, y) => + { + return x.precedenceDigit.CompareTo(y.precedenceDigit); + }); + // Now we process the entries a level at a time. for (var depth = 0; depth <= maxDepth; depth++) { // As we process items, collect the next set of items. - List<(RouteEndpoint endpoint, List parents)> nextWork; + List<(RouteEndpoint endpoint, int precedenceDigit, List parents)> nextWork; var nextWorkCount = 0; if (previousWork == null) { - nextWork = new List<(RouteEndpoint endpoint, List parents)>(); + nextWork = new List<(RouteEndpoint endpoint, int precedenceDigit, List parents)>(); } else { @@ -102,9 +116,12 @@ public DfaNode BuildDfaTree(bool includeLabel = false) nextWork = previousWork; } + // See comments on precedenceDigitComparer + work.Sort(0, workCount, precedenceDigitComparer); + for (var i = 0; i < workCount; i++) { - var (endpoint, parents) = work[i]; + var (endpoint, _, parents) = work[i]; if (!HasAdditionalRequiredSegments(endpoint, depth)) { @@ -122,7 +139,8 @@ public DfaNode BuildDfaTree(bool includeLabel = false) nextParents = nextWork[nextWorkCount].parents; nextParents.Clear(); - nextWork[nextWorkCount] = (endpoint, nextParents); + var nextPrecedenceDigit = GetPrecedenceDigitAtDepth(endpoint, depth + 1); + nextWork[nextWorkCount] = (endpoint, nextPrecedenceDigit, nextParents); } else { @@ -130,7 +148,8 @@ public DfaNode BuildDfaTree(bool includeLabel = false) // Add to the next set of work now so the list will be reused // even if there are no parents - nextWork.Add((endpoint, nextParents)); + var nextPrecedenceDigit = GetPrecedenceDigitAtDepth(endpoint, depth + 1); + nextWork.Add((endpoint, nextPrecedenceDigit, nextParents)); } var segment = GetCurrentSegment(endpoint, depth); @@ -281,7 +300,7 @@ private static void AddLiteralNode(bool includeLabel, List nextParents, nextParents.Add(next); } - private RoutePatternPathSegment GetCurrentSegment(RouteEndpoint endpoint, int depth) + private static RoutePatternPathSegment GetCurrentSegment(RouteEndpoint endpoint, int depth) { if (depth < endpoint.RoutePattern.PathSegments.Count) { @@ -302,6 +321,18 @@ private RoutePatternPathSegment GetCurrentSegment(RouteEndpoint endpoint, int de return null; } + private static int GetPrecedenceDigitAtDepth(RouteEndpoint endpoint, int depth) + { + var segment = GetCurrentSegment(endpoint, depth); + if (segment is null) + { + // Treat "no segment" as high priority. it won't effect the algorithm, but we need to define a sort-order. + return 0; + } + + return RoutePrecedence.ComputeInboundPrecedenceDigit(endpoint.RoutePattern, segment); + } + public override Matcher Build() { #if DEBUG @@ -670,6 +701,10 @@ private void ApplyPolicies(DfaNode node) return; } + // We're done with the precedence based work. Sort the endpoints + // before applying policies for simplicity in policy-related code. + node.Matches.Sort(_comparer); + // Start with the current node as the root. var work = new List() { node, }; List previousWork = null; diff --git a/src/Http/Routing/src/Template/RoutePrecedence.cs b/src/Http/Routing/src/Template/RoutePrecedence.cs index d5b3ebc03fdd..3bad48614bfe 100644 --- a/src/Http/Routing/src/Template/RoutePrecedence.cs +++ b/src/Http/Routing/src/Template/RoutePrecedence.cs @@ -219,7 +219,7 @@ private static int ComputeInboundPrecedenceDigit(TemplateSegment segment) // see description on ComputeInboundPrecedenceDigit(TemplateSegment segment) // // With a RoutePattern, parameters with a required value are treated as a literal segment - private static int ComputeInboundPrecedenceDigit(RoutePattern routePattern, RoutePatternPathSegment pathSegment) + internal static int ComputeInboundPrecedenceDigit(RoutePattern routePattern, RoutePatternPathSegment pathSegment) { if (pathSegment.Parts.Count > 1) { @@ -260,4 +260,4 @@ private static int ComputeInboundPrecedenceDigit(RoutePattern routePattern, Rout } } } -} \ No newline at end of file +} diff --git a/src/Http/Routing/test/UnitTests/Matching/DfaMatcherBuilderTest.cs b/src/Http/Routing/test/UnitTests/Matching/DfaMatcherBuilderTest.cs index c031bdd4f9a8..71b7068f0051 100644 --- a/src/Http/Routing/test/UnitTests/Matching/DfaMatcherBuilderTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/DfaMatcherBuilderTest.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -459,6 +459,200 @@ public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll() Assert.Same(catchAll, catchAll.CatchAll); } + // Regression test for https://github.com/dotnet/aspnetcore/issues/16579 + [Fact] + public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1() + { + // Arrange + var builder = CreateDfaMatcherBuilder(); + + var endpoint1 = CreateEndpoint("a/{b}", order: 0); + builder.AddEndpoint(endpoint1); + + var endpoint2 = CreateEndpoint("a/{*b}", order: 1); + builder.AddEndpoint(endpoint2); + + // Act + var root = builder.BuildDfaTree(); + + // Assert + Assert.Null(root.Matches); + Assert.Null(root.Parameters); + + var next = Assert.Single(root.Literals); + Assert.Equal("a", next.Key); + + var a = next.Value; + Assert.Same(endpoint2, Assert.Single(a.Matches)); + Assert.Null(a.Literals); + + var b = a.Parameters; + Assert.Collection( + b.Matches, + e => Assert.Same(endpoint1, e), + e => Assert.Same(endpoint2, e)); + Assert.Null(b.Literals); + Assert.Null(b.Parameters); + Assert.NotNull(b.CatchAll); + + var catchAll = b.CatchAll; + Assert.Same(endpoint2, Assert.Single(catchAll.Matches)); + Assert.Null(catchAll.Literals); + Assert.Same(catchAll, catchAll.Parameters); + Assert.Same(catchAll, catchAll.CatchAll); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/16579 + [Fact] + public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2() + { + // Arrange + var builder = CreateDfaMatcherBuilder(); + + var endpoint1 = CreateEndpoint("a/{*b}", order: 0); + builder.AddEndpoint(endpoint1); + + var endpoint2 = CreateEndpoint("a/{b}", order: 1); + builder.AddEndpoint(endpoint2); + + // Act + var root = builder.BuildDfaTree(); + + // Assert + Assert.Null(root.Matches); + Assert.Null(root.Parameters); + + var next = Assert.Single(root.Literals); + Assert.Equal("a", next.Key); + + var a = next.Value; + Assert.Same(endpoint1, Assert.Single(a.Matches)); + Assert.Null(a.Literals); + + var b = a.Parameters; + Assert.Collection( + b.Matches, + e => Assert.Same(endpoint1, e), + e => Assert.Same(endpoint2, e)); + Assert.Null(b.Literals); + Assert.Null(b.Parameters); + Assert.NotNull(b.CatchAll); + + var catchAll = b.CatchAll; + Assert.Same(endpoint1, Assert.Single(catchAll.Matches)); + Assert.Null(catchAll.Literals); + Assert.Same(catchAll, catchAll.Parameters); + Assert.Same(catchAll, catchAll.CatchAll); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/18677 + [Fact] + public void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order1() + { + // Arrange + var builder = CreateDfaMatcherBuilder(); + + var endpoint1 = CreateEndpoint("{a}/{b}", order: 0); + builder.AddEndpoint(endpoint1); + + var endpoint2 = CreateEndpoint("a/{*b}", order: 1); + builder.AddEndpoint(endpoint2); + + // Act + var root = builder.BuildDfaTree(); + + // Assert + Assert.Null(root.Matches); + + var next = Assert.Single(root.Literals); + Assert.Equal("a", next.Key); + + var a1 = next.Value; + Assert.Same(endpoint2, Assert.Single(a1.Matches)); + Assert.Null(a1.Literals); + + var b1 = a1.Parameters; + Assert.Collection( + b1.Matches, + e => Assert.Same(endpoint1, e), + e => Assert.Same(endpoint2, e)); + Assert.Null(b1.Literals); + Assert.Null(b1.Parameters); + Assert.NotNull(b1.CatchAll); + + var catchAll1 = b1.CatchAll; + Assert.Same(endpoint2, Assert.Single(catchAll1.Matches)); + Assert.Null(catchAll1.Literals); + Assert.Same(catchAll1, catchAll1.Parameters); + Assert.Same(catchAll1, catchAll1.CatchAll); + + var a2 = root.Parameters; + Assert.Null(a2.Matches); + Assert.Null(a2.Literals); + + var b2 = a2.Parameters; + Assert.Collection( + b2.Matches, + e => Assert.Same(endpoint1, e)); + Assert.Null(b2.Literals); + Assert.Null(b2.Parameters); + Assert.Null(b2.CatchAll); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/18677 + [Fact] + public void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order2() + { + // Arrange + var builder = CreateDfaMatcherBuilder(); + + var endpoint1 = CreateEndpoint("a/{*b}", order: 0); + builder.AddEndpoint(endpoint1); + + var endpoint2 = CreateEndpoint("{a}/{b}", order: 1); + builder.AddEndpoint(endpoint2); + + // Act + var root = builder.BuildDfaTree(); + + // Assert + Assert.Null(root.Matches); + + var next = Assert.Single(root.Literals); + Assert.Equal("a", next.Key); + + var a1 = next.Value; + Assert.Same(endpoint1, Assert.Single(a1.Matches)); + Assert.Null(a1.Literals); + + var b1 = a1.Parameters; + Assert.Collection( + b1.Matches, + e => Assert.Same(endpoint1, e), + e => Assert.Same(endpoint2, e)); + Assert.Null(b1.Literals); + Assert.Null(b1.Parameters); + Assert.NotNull(b1.CatchAll); + + var catchAll1 = b1.CatchAll; + Assert.Same(endpoint1, Assert.Single(catchAll1.Matches)); + Assert.Null(catchAll1.Literals); + Assert.Same(catchAll1, catchAll1.Parameters); + Assert.Same(catchAll1, catchAll1.CatchAll); + + var a2 = root.Parameters; + Assert.Null(a2.Matches); + Assert.Null(a2.Literals); + + var b2 = a2.Parameters; + Assert.Collection( + b2.Matches, + e => Assert.Same(endpoint2, e)); + Assert.Null(b2.Literals); + Assert.Null(b2.Parameters); + Assert.Null(b2.CatchAll); + } + [Fact] public void BuildDfaTree_WithPolicies() { @@ -729,6 +923,48 @@ public void BuildDfaTree_WithPolicies_AndBranches_BothPoliciesSkipped() Assert.Null(a.PolicyEdges); } + // Verifies that we sort the endpoints before calling into policies. + // + // The builder uses a different sort order when building the tree, vs when building the policy nodes. Policy + // nodes should see an "absolute" order. + [Fact] + public void BuildDfaTree_WithPolicies_SortedAccordingToScore() + { + // Arrange + // + // These cases where chosen where the absolute order incontrolled explicitly by setting .Order, but + // the precedence of segments is different, so these will be sorted into different orders when building + // the tree. + var policies = new MatcherPolicy[] + { + new TestMetadata1MatcherPolicy(), + new TestMetadata2MatcherPolicy(), + }; + + var builder = CreateDfaMatcherBuilder(policies); + + ((TestMetadata1MatcherPolicy)policies[0]).OnGetEdges = VerifyOrder; + ((TestMetadata2MatcherPolicy)policies[1]).OnGetEdges = VerifyOrder; + + var endpoint1 = CreateEndpoint("/a/{**b}", order: -1, metadata: new object[] { new TestMetadata1(0), new TestMetadata2(true), }); + builder.AddEndpoint(endpoint1); + + var endpoint2 = CreateEndpoint("/a/{b}/{**c}", order: 0, metadata: new object[] { new TestMetadata1(1), new TestMetadata2(true), }); + builder.AddEndpoint(endpoint2); + + var endpoint3 = CreateEndpoint("/a/b/{c}", order: 1, metadata: new object[] { new TestMetadata1(1), new TestMetadata2(false), }); + builder.AddEndpoint(endpoint3); + + // Act & Assert + _ = builder.BuildDfaTree(); + + void VerifyOrder(IReadOnlyList endpoints) + { + // The list should already be in sorted order, every time build is called. + Assert.Equal(endpoints, endpoints.OrderBy(e => e, builder.Comparer)); + } + } + [Fact] public void BuildDfaTree_RequiredValues() { @@ -1281,9 +1517,10 @@ private static RouteEndpoint CreateEndpoint( object defaults = null, object constraints = null, object requiredValues = null, + int order = 0, params object[] metadata) { - return EndpointFactory.CreateRouteEndpoint(template, defaults, constraints, requiredValues, metadata: metadata); + return EndpointFactory.CreateRouteEndpoint(template, defaults, constraints, requiredValues, order: order, metadata: metadata); } private class TestMetadata1 @@ -1306,6 +1543,8 @@ private class TestMetadata1MatcherPolicy : MatcherPolicy, IEndpointComparerPolic public IComparer Comparer => EndpointMetadataComparer.Default; + public Action> OnGetEdges { get; set; } + public bool AppliesToEndpoints(IReadOnlyList endpoints) { return endpoints.Any(e => e.Metadata.GetMetadata() != null); @@ -1318,6 +1557,7 @@ public PolicyJumpTable BuildJumpTable(int exitDestination, IReadOnlyList GetEdges(IReadOnlyList endpoints) { + OnGetEdges?.Invoke(endpoints); return endpoints .GroupBy(e => e.Metadata.GetMetadata().State) .Select(g => new PolicyNodeEdge(g.Key, g.ToArray())) @@ -1345,6 +1585,9 @@ private class TestMetadata2MatcherPolicy : MatcherPolicy, IEndpointComparerPolic public IComparer Comparer => EndpointMetadataComparer.Default; + public Action> OnGetEdges { get; set; } + + public bool AppliesToEndpoints(IReadOnlyList endpoints) { return endpoints.Any(e => e.Metadata.GetMetadata() != null); @@ -1357,6 +1600,7 @@ public PolicyJumpTable BuildJumpTable(int exitDestination, IReadOnlyList GetEdges(IReadOnlyList endpoints) { + OnGetEdges?.Invoke(endpoints); return endpoints .GroupBy(e => e.Metadata.GetMetadata().State) .Select(g => new PolicyNodeEdge(g.Key, g.ToArray())) diff --git a/src/Http/Routing/test/UnitTests/Matching/FullFeaturedMatcherConformanceTest.cs b/src/Http/Routing/test/UnitTests/Matching/FullFeaturedMatcherConformanceTest.cs index a4daf33682d4..10cdbc7e3614 100644 --- a/src/Http/Routing/test/UnitTests/Matching/FullFeaturedMatcherConformanceTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/FullFeaturedMatcherConformanceTest.cs @@ -4,6 +4,7 @@ using System; using System.Linq; using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; using Xunit; namespace Microsoft.AspNetCore.Routing.Matching @@ -442,5 +443,98 @@ public virtual async Task Match_IntegrationTest_MultipleEndpoints(string path, s // Assert MatcherAssert.AssertMatch(httpContext, expected, ignoreValues: true); } + + // https://github.com/dotnet/aspnetcore/issues/16579 + [Fact] + public virtual async Task Match_Regression_16579_Order1() + { + var endpoints = new RouteEndpoint[] + { + EndpointFactory.CreateRouteEndpoint( + "{controller}/folder/{*path}", + order: 0, + defaults: new { controller = "File", action = "Folder", }, + requiredValues: new { controller = "File", }), + EndpointFactory.CreateRouteEndpoint( + "{controller}/{action}/{filename}", + order: 1, + defaults: new { controller = "File", action = "Index", }, + requiredValues: new { controller = "File", action = "Index", }), + }; + + var expected = endpoints[0]; + + var matcher = CreateMatcher(endpoints); + var httpContext = CreateContext("/file/folder/abc/abc"); + + // Act + await matcher.MatchAsync(httpContext); + + // Assert + MatcherAssert.AssertMatch(httpContext, expected, ignoreValues: true); + } + + // https://github.com/dotnet/aspnetcore/issues/16579 + [Fact] + public virtual async Task Match_Regression_16579_Order2() + { + var endpoints = new RouteEndpoint[] + { + EndpointFactory.CreateRouteEndpoint( + "{controller}/{action}/{filename}", + order: 0, + defaults: new { controller = "File", action = "Index", }, + requiredValues: new { controller = "File", action = "Index", }), + + EndpointFactory.CreateRouteEndpoint( + "{controller}/folder/{*path}", + order: 1, + defaults: new { controller = "File", action = "Folder", }, + requiredValues: new { controller = "File", }), + }; + + var expected = endpoints[1]; + + var matcher = CreateMatcher(endpoints); + var httpContext = CreateContext("/file/folder/abc/abc"); + + // Act + await matcher.MatchAsync(httpContext); + + // Assert + MatcherAssert.AssertMatch(httpContext, expected, ignoreValues: true); + } + + // https://github.com/dotnet/aspnetcore/issues/18677 + [Theory] + [InlineData("/middleware", 1)] + [InlineData("/middleware/test", 1)] + [InlineData("/middleware/test1/test2", 1)] + [InlineData("/bill/boga", 0)] + public virtual async Task Match_Regression_18677(string path, int endpointIndex) + { + var endpoints = new RouteEndpoint[] + { + EndpointFactory.CreateRouteEndpoint( + "{firstName}/{lastName}", + order: 0, + defaults: new { controller = "TestRoute", action = "Index", }), + + EndpointFactory.CreateRouteEndpoint( + "middleware/{**_}", + order: 0), + }; + + var expected = endpoints[endpointIndex]; + + var matcher = CreateMatcher(endpoints); + var httpContext = CreateContext(path); + + // Act + await matcher.MatchAsync(httpContext); + + // Assert + MatcherAssert.AssertMatch(httpContext, expected, ignoreValues: true); + } } }