Skip to content

Add ability to modify UseRouting/UseEndpoint behavior for WebApplicationBuilder #35336

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

Merged
merged 11 commits into from
Aug 17, 2021

Conversation

BrennanConroy
Copy link
Member

Fixes #35147
Fixes #35281
Pre-req for #34146

UseRouting will not replace the EndpointRouteBuilder if a "global" one exists, this allows us to set a global one that will effectively ignore users calling UseRouting a second time (or multiple times themselves) so we can have all routes on the same route builder.

UseEndpoints also honors the global builder so it wont throw in some of it's checks that were specific to the default route builder.

Lastly, we use the new global route builder concept in WebApplicationBuilder so we can safely wrap users code in UseRouting/UseEndpoints and put all routes onto a single route builder.

@BrennanConroy BrennanConroy added feature-minimal-hosting old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Aug 13, 2021
@ghost ghost added the area-runtime label Aug 13, 2021
@davidfowl
Copy link
Member

davidfowl commented Aug 14, 2021

We also need to avoid copying the __GlobalEndpointRouteBuilder properties when we call ApplicationBuilder.New(). We only need to do this in the WebApplication implementation though.

// REVIEW: this makes startup filter with userouting/useendpoints fail unless we don't remove the EndpointRouteBuilder in UseEndpoints if a global one exists
// or if we stored the property in this method at the beginning and replaced it at the end.
app.Properties.Remove(WebApplication.GlobalEndpointRouteBuilderKey);
_builtApplication.Properties.Remove(WebApplication.GlobalEndpointRouteBuilderKey);
Copy link
Member

@davidfowl davidfowl Aug 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave this (possibly).

Suggested change
_builtApplication.Properties.Remove(WebApplication.GlobalEndpointRouteBuilderKey);

@davidfowl
Copy link
Member

davidfowl commented Aug 16, 2021

After playing with this, here's another scenario that's broken that we need to preserve. The scenario here is that I have a static file called /foo.txt in the wwwroot:

image

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.UseStaticFiles();

app.UseRouting();

app.MapGet("/foo.txt", () => "This is a route");

app.Run();

After this change, we'll get "This is a route", if I hit /foo.txt instead of "This is a static file". This is because we unconditionally add UseRouting. We need to not call UseRouting if it's already been called because that lets the user control when route matching happens if they want to.

@javiercn
Copy link
Member

After this change, we'll get "This is a route", if I hit /foo.txt instead of "This is a static file". This is because we unconditionally add UseRouting. We need to not call UseRouting if it's already been called because that lets the user control when route matching happens if they want to

That's a good catch we didn't thought of.

There are two questions:

  • Implicit routing and teaching middleware about it (should static files know that the route decision was made implicitly and to serve the file even though and endpoint was set?)
  • Should we cave in and avoid implicit routing? (Add the extra use routing call explicitly at the right time and do the UseEndpoints implicitly)

@davidfowl
Copy link
Member

davidfowl commented Aug 16, 2021

Implicit routing and teaching middleware about it (should static files know that the route decision was made implicitly and to serve the file even though and endpoint was set?)

I like this, but in .NET 7.

Should we cave in and avoid implicit routing? (Add the extra use routing call explicitly at the right time and do the UseEndpoints implicitly)

No I think we should do what we did before. Only add the implicit UseRouting if it's not defined by the application. This means we need a way to detect if it's been called.

@javiercn
Copy link
Member

No I think we should do what we did before. Only add the implicit UseRouting if it's not defined by the application. This means e need a way to detect if it's been called.

Can we do that? (Is that something we get to do with the "double pipeline" trick?

@davidfowl
Copy link
Member

Can we do that? (Is that something we get to do with the "double pipeline" trick?

I think we'd just update the call to UseRouting to set another property in the app builder so we can detect it. This is what we were doing previously.

if (context.HostingEnvironment.IsDevelopment())
{
// TODO: add test for this
Copy link
Member

@davidfowl davidfowl Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have coverage for this? File an issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrennanConroy BrennanConroy merged commit b88bc69 into main Aug 17, 2021
@BrennanConroy BrennanConroy deleted the brecon/routing branch August 17, 2021 20:07
@ghost ghost added this to the 6.0-rc1 milestone Aug 17, 2021
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-hosting old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
6 participants