Skip to content
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

Closed
billbogaiv opened this issue Jan 30, 2020 · 2 comments · Fixed by #20801
Closed
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates investigate
Milestone

Comments

@billbogaiv
Copy link

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 like api/{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

using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Routing.Patterns;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using System.Threading.Tasks;

namespace SampleRouteConflict
{
    public class Program
    {
        public static void Main(string[] args)
        {
            CreateHostBuilder(args).Build().Run();
        }

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseStartup<Startup>();
                });
    }

    public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddControllers();
        }

        public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app
                .UseHttpsRedirection()
                .UseRouting()
                .UseEndpoints(endpoints =>
                {
                    endpoints.MapControllers();

                    var appBuilder = endpoints.CreateApplicationBuilder();
                    var pattern = RoutePatternFactory.Parse("/middleware");

                    var pipeline = appBuilder
                        .UsePathBase(pattern.RawText)
                        .UseMiddleware<TestRouteMiddleware>()
                        .Build();

                    endpoints.Map(pattern.RawText + "/{**_}", pipeline);
                });
        }
    }

    [ApiController]
    public class TestRouteController : ControllerBase
    {
        [HttpGet]
        [Route("{firstName}/{lastName}")]
        public IActionResult Index(string firstName, string lastName)
        {
            return Ok(new { firstName, lastName });
        }
    }

    public class TestRouteMiddleware
    {
        public TestRouteMiddleware(RequestDelegate next)
        { }

        public async Task Invoke(HttpContext context)
        {
            await context.Response.WriteAsync("got to the middleware");
        }
    }
}

Results

Request API Match Middleware Match 404?
/middleware X
/middleware/test X
/middleware/test1/test2 X
/bill/boga X

Results removing endpoints.MapControllers();

Request API Match Middleware Match 404?
/middleware X
/middleware/test X
/middleware/test1/test2 X
/bill/boga X

Further technical details

.NET Core SDK (reflecting any global.json):
 Version:   3.1.100
 Commit:    cd82f021f4

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.1.100\

Host (useful for support):
  Version: 3.1.0
  Commit:  65f04fb6db
@billbogaiv
Copy link
Author

Maybe related to #16579?

@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 30, 2020
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Jan 30, 2020
@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @billbogaiv.
@javiercn can you please look into this? Thanks!

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
@ghost ghost locked as resolved and limited conversation to collaborators May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates investigate
4 participants