diff --git a/src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs b/src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs index 3ce07a786140..d2518a0f3154 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 @@ internal static partial class FastPathTokenizer 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 b5b3721b2324..9bff01e0023c 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 @@ -40,6 +41,15 @@ public DfaMatcherBuilder( _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 @@ public DfaMatcherBuilder( _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 @@ 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); + 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 @@ 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 +137,17 @@ public DfaNode BuildDfaTree(bool includeLabel = false) 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 @@ 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 +174,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 +326,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 +347,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 +727,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/GlobalSuppressions.cs b/src/Http/Routing/test/UnitTests/GlobalSuppressions.cs new file mode 100644 index 000000000000..1fbf99441867 --- /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 c031bdd4f9a8..6ac360a20713 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 @@ public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll() 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 @@ 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 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 @@ 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 +1748,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 +1762,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 +1790,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 +1805,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/DfaMatcherConformanceTest.cs b/src/Http/Routing/test/UnitTests/Matching/DfaMatcherConformanceTest.cs index 46ca9fdb2c15..66fb02c03a94 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 @@ public override async Task Quirks_CatchAllParameter(string template, string path 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 @@ internal override Matcher CreateMatcher(params RouteEndpoint[] endpoints) .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 ca86fe3e1def..a3abad92fa25 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 @@ 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); + } } } diff --git a/src/Http/Routing/test/UnitTests/Matching/RouteMatcherConformanceTest.cs b/src/Http/Routing/test/UnitTests/Matching/RouteMatcherConformanceTest.cs index 169069695142..fad408f5a1bc 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 e2c3a73108ba..dcf0d7e5ffdc 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 @@ public override Task Match_ExtraDefaultValues() 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();