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 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.

Author: rynowak

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
@@ -59,17 +60,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<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 +74,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 +113,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,15 +136,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 +297,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 +318,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 +698,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;

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
             }
         }
     }
-}
+}

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()
         {
@@ -1281,9 +1475,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