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

MapHost endpoint routing #19354

Open
Tratcher opened this issue Feb 25, 2020 · 7 comments
Open

MapHost endpoint routing #19354

Tratcher opened this issue Feb 25, 2020 · 7 comments
Labels
affected-few This issue impacts only small number of customers api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@Tratcher
Copy link
Member

Describe the bug

In endpoint routing today routes are defined around paths. There's an additional constraint you can add for a host, but only after already defining a path. Instead what I want to do is map all requests to a specific host as the primary pivot. This can be done today but it is inelegant (I have to define a catch-all route), and in-efficient (route values would still be extracted).

To Reproduce

Here's what I have to write today:

endpoints.Map("/{**path}", handler).RequireHost(host);

Proposal:

endpoints.MapHost(host, handler);
@Tratcher Tratcher added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 25, 2020
@javiercn
Copy link
Member

@Tratcher can you add a bit more info here?

What is the scenario you have in mind.

  • Would you map the host directly with an implicit catch-all route?
  • Would you offer the ability to pass in the route pattern explicitly as an option?
  • Do you mean for this to be literally equivalent to endpoints.Map("/{**path}", handler).RequireHost(host)?

@rynowak
Copy link
Member

rynowak commented Feb 26, 2020

We had some discussion offline before this issue was created. I think the key points are:

  • This can be done today but it is inelegant (I have to define a catch-all route)
  • in-efficient (route values would still be extracted)

The proxy wants this, because it's a fairly common thing for proxies to route traffic solely on host. The behavior would be the same as a catch-all route.

  • It's easy for us to make a first-class API for this, I just don't want it to cause confusion.
  • I have a cool idea about how to make this non-allocating.

We should add support for unnamed route parameters (or optionally _ as a prefix for non-capturing`). Unnamed route parameters would not be used during URL parsing, and would not support parameter transformers, nor constraints (since they never have a value).

I haven't totally made up my mind yet whether I think a route with unnamed parameters should support link generation, or whether unnamed route parameters can be optional.

The ability to control capturing and matching separately is really nice.

There's a big difference when a route has parameters vs when it don't be. https://github.com/dotnet/aspnetcore/blob/master/src/Http/Routing/src/Matching/DfaMatcher.cs#L115 - routes with parameters allocate a string (for each value), a KeyValuePair<string, object>[] for the backing store, and a RouteValueDictionary.

A route without parameters doesn't allocate anything during matching. An RVD is allocated only if someone tries to access it - and they likely won't for the proxy case.


The option of supporting _ as a prefix requires more ceremony overall to use, but it's much more flexible. These routes can be used with link generation if you want (since the parameter has a name), so it's more obvious that they should follow all of the normal rules except for capturing.

@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Feb 26, 2020
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Feb 26, 2020
@Tratcher
Copy link
Member Author

@mkArtakMSFT
Copy link
Member

@Tratcher do you want to tackle this?

@Tratcher
Copy link
Member Author

We're not blocked by this at the moment, but we'll see how the perf benchmarks work out.

@captainsafia captainsafia added affected-few This issue impacts only small number of customers severity-nice-to-have This label is used by an internal tool labels Mar 15, 2021 — with ASP.NET Core Issue Ranking
@rafikiassumani-msft rafikiassumani-msft added area-web-frameworks and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Oct 20, 2021
@ghost
Copy link

ghost commented Dec 28, 2021

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks labels Jun 20, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing severity-nice-to-have This label is used by an internal tool
Projects
No open projects
Status: No status
Development

No branches or pull requests

6 participants