Skip to content

Fix use of precedence in endpoint routing DFA #20801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 22, 2020
Merged

Fix use of precedence in endpoint routing DFA #20801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9b7335
Fix use of precedence in endpoint routing DFA
rynowak Apr 13, 2020
c5fe7c0
Add test
rynowak Apr 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 52 additions & 17 deletions src/Http/Routing/src/Matching/DfaMatcherBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -52,24 +53,21 @@ public DfaMatcherBuilder(
_constraints = new List<KeyValuePair<string, IRouteConstraint>>();
}

// Used in tests
internal EndpointComparer Comparer => _comparer;

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);

// 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 };

Expand All @@ -79,21 +77,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any significance to changing where this is computed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it just seemed to make sense to me to place after the other stuff.

}

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
{
Expand All @@ -102,9 +116,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))
{
Expand All @@ -122,15 +139,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);
Expand Down Expand Up @@ -281,7 +300,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)
{
Expand All @@ -302,6 +321,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
Expand Down Expand Up @@ -670,6 +701,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;
Expand Down
4 changes: 2 additions & 2 deletions src/Http/Routing/src/Template/RoutePrecedence.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -260,4 +260,4 @@ private static int ComputeInboundPrecedenceDigit(RoutePattern routePattern, Rout
}
}
}
}
}
Loading