From b194b6c90af6fbc8e6667f6f70b244162928d005 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 13 May 2020 11:12:14 -0700 Subject: [PATCH] Fix use of precedence in endpoint routing DFA (#20801) (#21200) * Fix use of precedence in endpoint routing DFA Fixes: #18677 Fixes: #16579 This is a change to how sorting is use when building endpoint routing's graph of nodes that is eventually transformed into the route table. There were bugs in how this was done that made it incompatible in some niche scenarios both with previous implementations and how we describe the features in the abstract. There are a wide array of cases that might have been impacted by this bug because routing is a pattern language. Generally the bugs will involve a catch-all, and some something that changes ordering of templates. Issue #18677 has the simplest repro for this, the following templates would not behave as expected: ``` a/{*b} {a}/{b} ``` One would expect any URL Path starting with `/a` to match the first route, but that's not what happens. --- The change supports an opt-in via the following AppContext switch: ``` Microsoft.AspNetCore.Routing.UseCorrectCatchAllBehavior ``` Set to true to enable the correct behavior. --- The root cause of this bug was an issue in how the algorithm used to be build the DFA was designed. Specifically that it uses a BFS to build the graph, and it uses an up-front one-time sort of endpoints in order to drive that BFS. The building of the graph has the expectation that at each level, we will process **all** literal segments (`/a`) and then **all** parameter segments (`/{a}`) and then **all** catch-all segments (`/{*a}`). Routing defines a concept called *precedence* that defines the *conceptual* order in while segments types are ordered. So there are two problems: - We sort based on criteria other than precedence (#16579) - We can't rely on a one-time sort, it needs to be done at each level (#18677) --- The fix is to repeat the sort operation at each level and use precedence as the only key for sorting (as dictated by the graph building algo). We do a sort of the matches of each node *after* building the precedence-based part of the DFA, based on the full sorting criteria, to maintain compatibility. * Add test --- .../Microsoft.AspNetCore.Routing.Manual.cs | 2 + .../Routing/src/Matching/DfaMatcherBuilder.cs | 91 +++- .../Routing/src/Template/RoutePrecedence.cs | 4 +- .../test/UnitTests/GlobalSuppressions.cs | 11 + .../Matching/DfaMatcherBuilderTest.cs | 453 +++++++++++++++++- .../Matching/DfaMatcherConformanceTest.cs | 131 +++++ .../FullFeaturedMatcherConformanceTest.cs | 62 +++ .../Matching/RouteMatcherConformanceTest.cs | 40 +- .../TreeRouterMatcherConformanceTest.cs | 35 +- 9 files changed, 807 insertions(+), 22 deletions(-) create mode 100644 src/Http/Routing/test/UnitTests/GlobalSuppressions.cs diff --git a/src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs b/src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs index 3ce07a7861..d2518a0f31 100644 --- a/src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs +++ b/src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs @@ -199,6 +199,8 @@ namespace Microsoft.AspNetCore.Routing.Matching internal partial class DfaMatcherBuilder : Microsoft.AspNetCore.Routing.Matching.MatcherBuilder { public DfaMatcherBuilder(Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.Routing.ParameterPolicyFactory parameterPolicyFactory, Microsoft.AspNetCore.Routing.Matching.EndpointSelector selector, System.Collections.Generic.IEnumerable policies) { } + internal EndpointComparer Comparer { get; } + internal bool UseCorrectCatchAllBehavior { get; set; } public override void AddEndpoint(Microsoft.AspNetCore.Routing.RouteEndpoint endpoint) { } public override Microsoft.AspNetCore.Routing.Matching.Matcher Build() { throw null; } public Microsoft.AspNetCore.Routing.Matching.DfaNode BuildDfaTree(bool includeLabel = false) { throw null; } diff --git a/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs b/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs index 180d91a177..628adf4b74 100644 --- a/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs +++ b/src/Http/Routing/src/Matching/DfaMatcherBuilder.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; 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 @@ -40,6 +41,15 @@ namespace Microsoft.AspNetCore.Routing.Matching _parameterPolicyFactory = parameterPolicyFactory; _selector = selector; + if (AppContext.TryGetSwitch("Microsoft.AspNetCore.Routing.UseCorrectCatchAllBehavior", out var enabled)) + { + UseCorrectCatchAllBehavior = enabled; + } + else + { + UseCorrectCatchAllBehavior = false; // default to bugged behavior + } + var (nodeBuilderPolicies, endpointComparerPolicies, endpointSelectorPolicies) = ExtractPolicies(policies.OrderBy(p => p.Order)); _endpointSelectorPolicies = endpointSelectorPolicies; _nodeBuilders = nodeBuilderPolicies; @@ -52,6 +62,12 @@ namespace Microsoft.AspNetCore.Routing.Matching _constraints = new List>(); } + // Used in tests + internal EndpointComparer Comparer => _comparer; + + // Used in tests + internal bool UseCorrectCatchAllBehavior { get; set; } + public override void AddEndpoint(RouteEndpoint endpoint) { _endpoints.Add(endpoint); @@ -59,17 +75,20 @@ namespace Microsoft.AspNetCore.Routing.Matching 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); + if (!UseCorrectCatchAllBehavior) + { + // In 3.0 we did a global sort of the endpoints up front. This was a bug, because we actually want + // do do the sort at each level of the tree based on precedence. + // + // _useLegacy30Behavior enables opt-out via an AppContext switch. + _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 +98,37 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 +137,17 @@ namespace Microsoft.AspNetCore.Routing.Matching nextWork = previousWork; } + if (UseCorrectCatchAllBehavior) + { + // The fix for the 3.0 sorting behavior bug. + + // 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 +165,8 @@ namespace Microsoft.AspNetCore.Routing.Matching nextParents = nextWork[nextWorkCount].parents; nextParents.Clear(); - nextWork[nextWorkCount] = (endpoint, nextParents); + var nextPrecedenceDigit = GetPrecedenceDigitAtDepth(endpoint, depth + 1); + nextWork[nextWorkCount] = (endpoint, nextPrecedenceDigit, nextParents); } else { @@ -130,7 +174,8 @@ namespace Microsoft.AspNetCore.Routing.Matching // 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 +326,7 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 +347,18 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 @@ -673,6 +730,10 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 d5b3ebc03f..3bad48614b 100644 --- a/src/Http/Routing/src/Template/RoutePrecedence.cs +++ b/src/Http/Routing/src/Template/RoutePrecedence.cs @@ -219,7 +219,7 @@ namespace Microsoft.AspNetCore.Routing.Template // 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 @@ namespace Microsoft.AspNetCore.Routing.Template } } } -} \ No newline at end of file +} diff --git a/src/Http/Routing/test/UnitTests/GlobalSuppressions.cs b/src/Http/Routing/test/UnitTests/GlobalSuppressions.cs new file mode 100644 index 0000000000..1fbf994418 --- /dev/null +++ b/src/Http/Routing/test/UnitTests/GlobalSuppressions.cs @@ -0,0 +1,11 @@ +// This file is used by Code Analysis to maintain SuppressMessage +// attributes that are applied to this project. +// Project-level suppressions either have no target or are given +// a specific target and scoped to a namespace, type, member, etc. + +[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage( + "Build", + "xUnit1013:Public method 'Quirks_CatchAllParameter' on test class 'FullFeaturedMatcherConformanceTest' should be marked as a Theory.", + Justification = "This is a bug in the xUnit analyzer. This method is already marked as a theory.", + Scope = "member", + Target = "~M:Microsoft.AspNetCore.Routing.Matching.FullFeaturedMatcherConformanceTest.Quirks_CatchAllParameter(System.String,System.String,System.String[],System.String[])~System.Threading.Tasks.Task")] diff --git a/src/Http/Routing/test/UnitTests/Matching/DfaMatcherBuilderTest.cs b/src/Http/Routing/test/UnitTests/Matching/DfaMatcherBuilderTest.cs index c031bdd4f9..6ac360a207 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,403 @@ namespace Microsoft.AspNetCore.Routing.Matching Assert.Same(catchAll, catchAll.CatchAll); } + // Regression test for https://github.com/dotnet/aspnetcore/issues/16579 + // + // This case behaves the same for all combinations. + [Fact] + public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1_CorrectBehavior() + { + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = true; + BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1_Core(builder); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/16579 + // + // This case behaves the same for all combinations. + [Fact] + public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1_DefaultBehavior() + { + var builder = CreateDfaMatcherBuilder(); + BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1_Core(builder); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/16579 + // + // This case behaves the same for all combinations. + [Fact] + public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1_LegacyBehavior() + { + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = false; + BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1_Core(builder); + } + + private void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1_Core(DfaMatcherBuilder builder) + { + // Arrange + 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_CorrectBehavior() + { + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = true; + BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2_CorrectBehavior_Core(builder); + } + + [Fact] + public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2_DefaultBehavior() + { + var builder = CreateDfaMatcherBuilder(); + BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2_LegacyBehavior_Core(builder); + } + + [Fact] + public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2_LegacyBehavior() + { + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = false; + BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2_LegacyBehavior_Core(builder); + } + + private void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2_CorrectBehavior_Core(DfaMatcherBuilder builder) + { + // Arrange + 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); + } + + private void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2_LegacyBehavior_Core(DfaMatcherBuilder builder) + { + // Arrange + 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.Same(endpoint1, Assert.Single(a.Matches)); + Assert.Null(b.Literals); + Assert.Null(b.Parameters); + Assert.Null(b.CatchAll); + + var catchAll = a.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_CorrectBehavior() + { + // Arrange + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = true; + + 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_CorrectBehavior() + { + // Arrange + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = true; + + 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); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/18677 + [Fact] + public void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order1_DefaultBehavior() + { + var builder = CreateDfaMatcherBuilder(); + BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order1_Legacy30Behavior_Core(builder); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/18677 + [Fact] + public void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order1_Legacy30Behavior() + { + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = false; + BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order1_Legacy30Behavior_Core(builder); + } + + private void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order1_Legacy30Behavior_Core(DfaMatcherBuilder builder) + { + // Arrange + 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); + Assert.Null(a1.Parameters); + + var catchAll1 = a1.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_DefaultBehavior() + { + var builder = CreateDfaMatcherBuilder(); + BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order2_Legacy30Behavior_Core(builder); + } + + // Regression test for https://github.com/dotnet/aspnetcore/issues/18677 + [Fact] + public void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order2_Legacy30Behavior() + { + var builder = CreateDfaMatcherBuilder(); + builder.UseCorrectCatchAllBehavior = false; + BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order2_Legacy30Behavior_Core(builder); + } + + private void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order2_Legacy30Behavior_Core(DfaMatcherBuilder builder) + { + // Arrange + 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.Same(endpoint2, Assert.Single(b1.Matches)); + Assert.Null(b1.Literals); + Assert.Null(b1.Parameters); + Assert.Null(b1.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 +1126,50 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 comparer = new EndpointComparer(policies.OrderBy(p => p.Order).OfType().ToArray()); + + 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, comparer)); + } + } + [Fact] public void BuildDfaTree_RequiredValues() { @@ -1281,9 +1722,10 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 +1748,8 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 +1762,7 @@ namespace Microsoft.AspNetCore.Routing.Matching public 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 +1790,9 @@ namespace Microsoft.AspNetCore.Routing.Matching 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 +1805,7 @@ namespace Microsoft.AspNetCore.Routing.Matching public 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/DfaMatcherConformanceTest.cs b/src/Http/Routing/test/UnitTests/Matching/DfaMatcherConformanceTest.cs index 46ca9fdb2c..66fb02c03a 100644 --- a/src/Http/Routing/test/UnitTests/Matching/DfaMatcherConformanceTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/DfaMatcherConformanceTest.cs @@ -3,6 +3,7 @@ using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; +using Xunit; namespace Microsoft.AspNetCore.Routing.Matching { @@ -23,7 +24,132 @@ namespace Microsoft.AspNetCore.Routing.Matching MatcherAssert.AssertMatch(httpContext, endpoint, keys, values); } + // 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 async Task Match_Regression_1867_CorrectBehavior(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(useCorrectCatchAllBehavior: true, endpoints); + var httpContext = CreateContext(path); + + // 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", 0)] + [InlineData("/middleware/test1/test2", -1)] + [InlineData("/bill/boga", 0)] + public async Task Match_Regression_1867_DefaultBehavior(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 = endpointIndex switch + { + -1 => null, + _ => endpoints[endpointIndex], + }; + + var matcher = CreateMatcher(useCorrectCatchAllBehavior: default, endpoints); + var httpContext = CreateContext(path); + + // Act + await matcher.MatchAsync(httpContext); + + // Assert + if (expected == null) + { + MatcherAssert.AssertNotMatch(httpContext); + } + else + { + MatcherAssert.AssertMatch(httpContext, expected, ignoreValues: true); + } + } + + // https://github.com/dotnet/aspnetcore/issues/18677 + // + [Theory] + [InlineData("/middleware", 1)] + [InlineData("/middleware/test", 0)] + [InlineData("/middleware/test1/test2", -1)] + [InlineData("/bill/boga", 0)] + public async Task Match_Regression_1867_LegacyBehavior(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 = endpointIndex switch + { + -1 => null, + _ => endpoints[endpointIndex], + }; + + var matcher = CreateMatcher(useCorrectCatchAllBehavior: false, endpoints); + var httpContext = CreateContext(path); + + // Act + await matcher.MatchAsync(httpContext); + + // Assert + if (expected == null) + { + MatcherAssert.AssertNotMatch(httpContext); + } + else + { + MatcherAssert.AssertMatch(httpContext, expected, ignoreValues: true); + } + } + internal override Matcher CreateMatcher(params RouteEndpoint[] endpoints) + { + return CreateMatcher(useCorrectCatchAllBehavior: default, endpoints); + } + + internal Matcher CreateMatcher(bool? useCorrectCatchAllBehavior, params RouteEndpoint[] endpoints) { var services = new ServiceCollection() .AddLogging() @@ -32,6 +158,11 @@ namespace Microsoft.AspNetCore.Routing.Matching .BuildServiceProvider(); var builder = services.GetRequiredService(); + if (useCorrectCatchAllBehavior.HasValue) + { + builder.UseCorrectCatchAllBehavior = useCorrectCatchAllBehavior.Value; + } + for (var i = 0; i < endpoints.Length; i++) { builder.AddEndpoint(endpoints[i]); diff --git a/src/Http/Routing/test/UnitTests/Matching/FullFeaturedMatcherConformanceTest.cs b/src/Http/Routing/test/UnitTests/Matching/FullFeaturedMatcherConformanceTest.cs index ca86fe3e1d..a3abad92fa 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,66 @@ namespace Microsoft.AspNetCore.Routing.Matching // 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); + } } } diff --git a/src/Http/Routing/test/UnitTests/Matching/RouteMatcherConformanceTest.cs b/src/Http/Routing/test/UnitTests/Matching/RouteMatcherConformanceTest.cs index 1690696951..fad408f5a1 100644 --- a/src/Http/Routing/test/UnitTests/Matching/RouteMatcherConformanceTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/RouteMatcherConformanceTest.cs @@ -1,16 +1,52 @@ -// 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.Threading.Tasks; +using Xunit; + namespace Microsoft.AspNetCore.Routing.Matching { public class RouteMatcherConformanceTest : FullFeaturedMatcherConformanceTest { + // 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 async Task Match_Regression_1867(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); + } + internal override Matcher CreateMatcher(params RouteEndpoint[] endpoints) { var builder = new RouteMatcherBuilder(); for (var i = 0; i < endpoints.Length; i++) { - builder.AddEndpoint(endpoints[i]); + builder.AddEndpoint(endpoints[i]); } return builder.Build(); } diff --git a/src/Http/Routing/test/UnitTests/Matching/TreeRouterMatcherConformanceTest.cs b/src/Http/Routing/test/UnitTests/Matching/TreeRouterMatcherConformanceTest.cs index e2c3a73108..dcf0d7e5ff 100644 --- a/src/Http/Routing/test/UnitTests/Matching/TreeRouterMatcherConformanceTest.cs +++ b/src/Http/Routing/test/UnitTests/Matching/TreeRouterMatcherConformanceTest.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.Threading.Tasks; @@ -22,6 +22,39 @@ namespace Microsoft.AspNetCore.Routing.Matching return Task.CompletedTask; } + // 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 async Task Match_Regression_1867(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); + } + internal override Matcher CreateMatcher(params RouteEndpoint[] endpoints) { var builder = new TreeRouterMatcherBuilder();