-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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.
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
Odd results mixing fully-tokenized attribute-route and wildcard-map within endpoint-routing #18677
Labels
Milestone
Comments
Maybe related to #16579? |
Thanks for contacting us, @billbogaiv. |
rynowak
added a commit
that referenced
this issue
Apr 14, 2020
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
added a commit
that referenced
this issue
Apr 22, 2020
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
added a commit
that referenced
this issue
Apr 22, 2020
* 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
rynowak
added a commit
that referenced
this issue
Apr 23, 2020
* 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
rynowak
added a commit
that referenced
this issue
Apr 24, 2020
* 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
rynowak
added a commit
that referenced
this issue
Apr 29, 2020
* 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
added a commit
that referenced
this issue
Apr 29, 2020
* 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
added a commit
that referenced
this issue
Apr 29, 2020
* 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)
wtgodbe
pushed a commit
that referenced
this issue
May 13, 2020
* 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
Describe the bug
The presence of the attribute-route seems to clobber requests that should go through the separate wildcard-mapped middleware. This only seems to affect fully tokenized and path-separated attribute-routes (i.e.
{firstName}/{lastName}
, etc.). Something likeapi/{firstName}/{lastName}
is not affected.My expectation using the sample below is that any request starting with
/middleware
would go through the middleware due to having a more-exact match based on available endpoints.To Reproduce
Results
Results removing
endpoints.MapControllers();
Further technical details
The text was updated successfully, but these errors were encountered: