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 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
This commit is contained in:
Ryan Nowak 2020-04-21 22:48:27 -07:00 committed by GitHub
parent 2e970f8e4f
commit 80861f1da9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 394 additions and 21 deletions

View File

@ -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.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
@ -52,6 +53,9 @@ namespace Microsoft.AspNetCore.Routing.Matching
_constraints = new List<KeyValuePair<string, IRouteConstraint>>();
}
// Used in tests
internal EndpointComparer Comparer => _comparer;
public override void AddEndpoint(RouteEndpoint endpoint)
{
_endpoints.Add(endpoint);
@ -59,17 +63,11 @@ 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);
// 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 +77,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<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 +116,12 @@ namespace Microsoft.AspNetCore.Routing.Matching
nextWork = previousWork;
}
// See comments on precedenceDigitComparer
work.Sort(0, workCount, precedenceDigitComparer);
for (var i = 0; i < workCount; i++)
{
var (endpoint, parents) = work[i];
var (endpoint, _, parents) = work[i];
if (!HasAdditionalRequiredSegments(endpoint, depth))
{
@ -122,7 +139,8 @@ 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 +148,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 +300,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 +321,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
@ -670,6 +701,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<DfaNode>() { node, };
List<DfaNode> previousWork = null;

View File

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

View File

@ -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 @@ namespace Microsoft.AspNetCore.Routing.Matching
Assert.Same(catchAll, catchAll.CatchAll);
}
// Regression test for https://github.com/dotnet/aspnetcore/issues/16579
[Fact]
public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order1()
{
// Arrange
var builder = CreateDfaMatcherBuilder();
var endpoint1 = CreateEndpoint("a/{b}", order: 0);
builder.AddEndpoint(endpoint1);
var endpoint2 = CreateEndpoint("a/{*b}", order: 1);
builder.AddEndpoint(endpoint2);
// Act
var root = builder.BuildDfaTree();
// Assert
Assert.Null(root.Matches);
Assert.Null(root.Parameters);
var next = Assert.Single(root.Literals);
Assert.Equal("a", next.Key);
var a = next.Value;
Assert.Same(endpoint2, Assert.Single(a.Matches));
Assert.Null(a.Literals);
var b = a.Parameters;
Assert.Collection(
b.Matches,
e => Assert.Same(endpoint1, e),
e => Assert.Same(endpoint2, e));
Assert.Null(b.Literals);
Assert.Null(b.Parameters);
Assert.NotNull(b.CatchAll);
var catchAll = b.CatchAll;
Assert.Same(endpoint2, Assert.Single(catchAll.Matches));
Assert.Null(catchAll.Literals);
Assert.Same(catchAll, catchAll.Parameters);
Assert.Same(catchAll, catchAll.CatchAll);
}
// Regression test for https://github.com/dotnet/aspnetcore/issues/16579
[Fact]
public void BuildDfaTree_MultipleEndpoint_ParameterAndCatchAll_OnSameNode_Order2()
{
// Arrange
var builder = CreateDfaMatcherBuilder();
var endpoint1 = CreateEndpoint("a/{*b}", order: 0);
builder.AddEndpoint(endpoint1);
var endpoint2 = CreateEndpoint("a/{b}", order: 1);
builder.AddEndpoint(endpoint2);
// Act
var root = builder.BuildDfaTree();
// Assert
Assert.Null(root.Matches);
Assert.Null(root.Parameters);
var next = Assert.Single(root.Literals);
Assert.Equal("a", next.Key);
var a = next.Value;
Assert.Same(endpoint1, Assert.Single(a.Matches));
Assert.Null(a.Literals);
var b = a.Parameters;
Assert.Collection(
b.Matches,
e => Assert.Same(endpoint1, e),
e => Assert.Same(endpoint2, e));
Assert.Null(b.Literals);
Assert.Null(b.Parameters);
Assert.NotNull(b.CatchAll);
var catchAll = b.CatchAll;
Assert.Same(endpoint1, Assert.Single(catchAll.Matches));
Assert.Null(catchAll.Literals);
Assert.Same(catchAll, catchAll.Parameters);
Assert.Same(catchAll, catchAll.CatchAll);
}
// Regression test for https://github.com/dotnet/aspnetcore/issues/18677
[Fact]
public void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order1()
{
// Arrange
var builder = CreateDfaMatcherBuilder();
var endpoint1 = CreateEndpoint("{a}/{b}", order: 0);
builder.AddEndpoint(endpoint1);
var endpoint2 = CreateEndpoint("a/{*b}", order: 1);
builder.AddEndpoint(endpoint2);
// Act
var root = builder.BuildDfaTree();
// Assert
Assert.Null(root.Matches);
var next = Assert.Single(root.Literals);
Assert.Equal("a", next.Key);
var a1 = next.Value;
Assert.Same(endpoint2, Assert.Single(a1.Matches));
Assert.Null(a1.Literals);
var b1 = a1.Parameters;
Assert.Collection(
b1.Matches,
e => Assert.Same(endpoint1, e),
e => Assert.Same(endpoint2, e));
Assert.Null(b1.Literals);
Assert.Null(b1.Parameters);
Assert.NotNull(b1.CatchAll);
var catchAll1 = b1.CatchAll;
Assert.Same(endpoint2, Assert.Single(catchAll1.Matches));
Assert.Null(catchAll1.Literals);
Assert.Same(catchAll1, catchAll1.Parameters);
Assert.Same(catchAll1, catchAll1.CatchAll);
var a2 = root.Parameters;
Assert.Null(a2.Matches);
Assert.Null(a2.Literals);
var b2 = a2.Parameters;
Assert.Collection(
b2.Matches,
e => Assert.Same(endpoint1, e));
Assert.Null(b2.Literals);
Assert.Null(b2.Parameters);
Assert.Null(b2.CatchAll);
}
// Regression test for https://github.com/dotnet/aspnetcore/issues/18677
[Fact]
public void BuildDfaTree_MultipleEndpoint_CatchAllWithHigherPrecedenceThanParameter_Order2()
{
// Arrange
var builder = CreateDfaMatcherBuilder();
var endpoint1 = CreateEndpoint("a/{*b}", order: 0);
builder.AddEndpoint(endpoint1);
var endpoint2 = CreateEndpoint("{a}/{b}", order: 1);
builder.AddEndpoint(endpoint2);
// Act
var root = builder.BuildDfaTree();
// Assert
Assert.Null(root.Matches);
var next = Assert.Single(root.Literals);
Assert.Equal("a", next.Key);
var a1 = next.Value;
Assert.Same(endpoint1, Assert.Single(a1.Matches));
Assert.Null(a1.Literals);
var b1 = a1.Parameters;
Assert.Collection(
b1.Matches,
e => Assert.Same(endpoint1, e),
e => Assert.Same(endpoint2, e));
Assert.Null(b1.Literals);
Assert.Null(b1.Parameters);
Assert.NotNull(b1.CatchAll);
var catchAll1 = b1.CatchAll;
Assert.Same(endpoint1, Assert.Single(catchAll1.Matches));
Assert.Null(catchAll1.Literals);
Assert.Same(catchAll1, catchAll1.Parameters);
Assert.Same(catchAll1, catchAll1.CatchAll);
var a2 = root.Parameters;
Assert.Null(a2.Matches);
Assert.Null(a2.Literals);
var b2 = a2.Parameters;
Assert.Collection(
b2.Matches,
e => Assert.Same(endpoint2, e));
Assert.Null(b2.Literals);
Assert.Null(b2.Parameters);
Assert.Null(b2.CatchAll);
}
[Fact]
public void BuildDfaTree_WithPolicies()
{
@ -729,6 +923,48 @@ 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 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<Endpoint> endpoints)
{
// The list should already be in sorted order, every time build is called.
Assert.Equal(endpoints, endpoints.OrderBy(e => e, builder.Comparer));
}
}
[Fact]
public void BuildDfaTree_RequiredValues()
{
@ -1281,9 +1517,10 @@ 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 +1543,8 @@ namespace Microsoft.AspNetCore.Routing.Matching
public IComparer<Endpoint> Comparer => EndpointMetadataComparer<TestMetadata1>.Default;
public Action<IReadOnlyList<Endpoint>> OnGetEdges { get; set; }
public bool AppliesToEndpoints(IReadOnlyList<Endpoint> endpoints)
{
return endpoints.Any(e => e.Metadata.GetMetadata<TestMetadata1>() != null);
@ -1318,6 +1557,7 @@ namespace Microsoft.AspNetCore.Routing.Matching
public IReadOnlyList<PolicyNodeEdge> GetEdges(IReadOnlyList<Endpoint> endpoints)
{
OnGetEdges?.Invoke(endpoints);
return endpoints
.GroupBy(e => e.Metadata.GetMetadata<TestMetadata1>().State)
.Select(g => new PolicyNodeEdge(g.Key, g.ToArray()))
@ -1345,6 +1585,9 @@ namespace Microsoft.AspNetCore.Routing.Matching
public IComparer<Endpoint> Comparer => EndpointMetadataComparer<TestMetadata2>.Default;
public Action<IReadOnlyList<Endpoint>> OnGetEdges { get; set; }
public bool AppliesToEndpoints(IReadOnlyList<Endpoint> endpoints)
{
return endpoints.Any(e => e.Metadata.GetMetadata<TestMetadata2>() != null);
@ -1357,6 +1600,7 @@ namespace Microsoft.AspNetCore.Routing.Matching
public IReadOnlyList<PolicyNodeEdge> GetEdges(IReadOnlyList<Endpoint> endpoints)
{
OnGetEdges?.Invoke(endpoints);
return endpoints
.GroupBy(e => e.Metadata.GetMetadata<TestMetadata2>().State)
.Select(g => new PolicyNodeEdge(g.Key, g.ToArray()))

View File

@ -4,6 +4,7 @@
using System;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Xunit;
namespace Microsoft.AspNetCore.Routing.Matching
@ -442,5 +443,98 @@ 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);
}
// https://github.com/dotnet/aspnetcore/issues/18677
[Theory]
[InlineData("/middleware", 1)]
[InlineData("/middleware/test", 1)]
[InlineData("/middleware/test1/test2", 1)]
[InlineData("/bill/boga", 0)]
public virtual async Task Match_Regression_18677(string path, int endpointIndex)
{
var endpoints = new RouteEndpoint[]
{
EndpointFactory.CreateRouteEndpoint(
"{firstName}/{lastName}",
order: 0,
defaults: new { controller = "TestRoute", action = "Index", }),
EndpointFactory.CreateRouteEndpoint(
"middleware/{**_}",
order: 0),
};
var expected = endpoints[endpointIndex];
var matcher = CreateMatcher(endpoints);
var httpContext = CreateContext(path);
// Act
await matcher.MatchAsync(httpContext);
// Assert
MatcherAssert.AssertMatch(httpContext, expected, ignoreValues: true);
}
}
}