Skip to content

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

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 1 commit into from
May 13, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
93 changes: 77 additions & 16 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 @@ -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;
Expand All @@ -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 };

Expand All @@ -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
{
Expand All @@ -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))
{
Expand All @@ -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);
Expand Down Expand Up @@ -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)
{
Expand All @@ -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
Expand Down Expand Up @@ -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;
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
}
}
}
}
}
11 changes: 11 additions & 0 deletions src/Http/Routing/test/UnitTests/GlobalSuppressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")]
Loading