Fix use of precedence in endpoint routing DFA (#20801)

* 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

Author: rynowak

Diff for: 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<Microsoft.AspNetCore.Routing.MatcherPolicy> 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 for: src/Http/Routing/src/Matching/DfaMatcherBuilder.cs

@@ -1,11 +1,12 @@
-// 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;
 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 @@ 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,24 +62,33 @@ public DfaMatcherBuilder(
             _constraints = new List<KeyValuePair<string, IRouteConstraint>>();
         }
 
+        // Used in tests
+        internal EndpointComparer Comparer => _comparer;
+
+        // Used in tests
+        internal bool UseCorrectCatchAllBehavior { get; set; }
+
         public override void AddEndpoint(RouteEndpoint endpoint)
         {
             _endpoints.Add(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<DfaNode> parents)>(_endpoints.Count);
-            List<(RouteEndpoint endpoint, List<DfaNode> parents)> previousWork = null;
+            var work = new List<(RouteEndpoint endpoint, int precedenceDigit, List<DfaNode> parents)>(_endpoints.Count);
+            List<(RouteEndpoint endpoint, int precedenceDigit, List<DfaNode> 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<DfaNode>() { root, }));
 
-                work.Add((endpoint, new List<DfaNode>() { 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<DfaNode> 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<DfaNode> parents)> nextWork;
+                List<(RouteEndpoint endpoint, int precedenceDigit, List<DfaNode> parents)> nextWork;
                 var nextWorkCount = 0;
                 if (previousWork == null)
                 {
-                    nextWork = new List<(RouteEndpoint endpoint, List<DfaNode> parents)>();
+                    nextWork = new List<(RouteEndpoint endpoint, int precedenceDigit, List<DfaNode> 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,15 +165,17 @@ 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
                     {
                         nextParents = new List<DfaNode>();
 
                         // 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<DfaNode> 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<DfaNode>() { node, };
             List<DfaNode> previousWork = null;

Diff for: 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
             }
         }
     }
-}
+}

Diff for: 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")]