Fix use of precedence in endpoint routing DFA #20801

rynowak · 2020-04-14T00:07:56Z

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 (ASP.NET Core 3.0 catch-all route unexpected behavior #16579)
We can't rely on a one-time sort, it needs to be done at each level
(Odd results mixing fully-tokenized attribute-route and wildcard-map within endpoint-routing #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.

Summary of the changes (Less than 80 chars)

Detail 1
Detail 2

Addresses #bugnumber (in this specific format)

src/Http/Routing/src/Matching/DfaMatcherBuilder.cs

pranavkm · 2020-04-14T00:48:08Z

src/Http/Routing/src/Matching/DfaMatcherBuilder.cs


-                work.Add((endpoint, new List<DfaNode>() { root, }));
+                maxDepth = Math.Max(maxDepth, endpoint.RoutePattern.PathSegments.Count);


Is there any significance to changing where this is computed?

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

pranavkm

once @JamesNK is happy

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.

rynowak · 2020-04-22T01:35:29Z

Updated with a brand new test 🥂

* 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

* 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-out via the following AppContext switch: ``` Microsoft.AspNetCore.Routing.Use30CatchAllBehavior ``` Set to true to enable the buggy behavior for compatibility. --- 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

* 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

* 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 (cherry picked from commit e317089)

* 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

rynowak requested a review from JamesNK April 14, 2020 00:07

rynowak requested review from analogrelay, jkotalik and Tratcher as code owners April 14, 2020 00:07

ghost added the area-servers label Apr 14, 2020

JamesNK reviewed Apr 14, 2020

View reviewed changes

src/Http/Routing/src/Matching/DfaMatcherBuilder.cs Show resolved Hide resolved

pranavkm reviewed Apr 14, 2020

View reviewed changes

pranavkm approved these changes Apr 14, 2020

View reviewed changes

analogrelay added this to the 5.0.0-preview5 milestone Apr 20, 2020

rynowak added 2 commits April 21, 2020 16:57

Add test

c5fe7c0

rynowak force-pushed the rynowak/routing-fixes branch from 730e1ad to c5fe7c0 Compare April 22, 2020 01:35

JamesNK approved these changes Apr 22, 2020

View reviewed changes

rynowak merged commit 80861f1 into master Apr 22, 2020

rynowak deleted the rynowak/routing-fixes branch April 22, 2020 05:48

xuzhg mentioned this pull request Apr 22, 2020

odata 7.4.0 endpoints routing break when using route constraints OData/WebApi#2134

Open

rynowak mentioned this pull request Apr 22, 2020

Fix behavior of catch-all routes in 3.1 #21114

Closed

rynowak mentioned this pull request Apr 28, 2020

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

Merged

jtkech mentioned this pull request Aug 28, 2020

ASP.NET Core "UseCorrectCatchAllBehavior" - impacts AutoRouteTransformer OrchardCMS/OrchardCore#6926

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix use of precedence in endpoint routing DFA #20801

Fix use of precedence in endpoint routing DFA #20801

Uh oh!

rynowak commented Apr 14, 2020

Uh oh!

Uh oh!

pranavkm Apr 14, 2020

Uh oh!

rynowak Apr 21, 2020

Uh oh!

pranavkm left a comment

Uh oh!

rynowak commented Apr 22, 2020

Uh oh!

Uh oh!


		work.Add((endpoint, new List<DfaNode>() { root, }));
		maxDepth = Math.Max(maxDepth, endpoint.RoutePattern.PathSegments.Count);

Fix use of precedence in endpoint routing DFA #20801

Fix use of precedence in endpoint routing DFA #20801

Uh oh!

Conversation

rynowak commented Apr 14, 2020

Uh oh!

Uh oh!

pranavkm Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

rynowak Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

rynowak commented Apr 22, 2020

Uh oh!

Uh oh!