Fix UsePathBase with WAB #42876
Fix UsePathBase with WAB #42876
Conversation
BrennanConroy
requested review from
javiercn,
Tratcher and
halter73
as code owners
ghost
added
the
src/Http/Http.Abstractions/src/Extensions/UsePathBaseExtensions.cs
Outdated
Show resolved
Hide resolved
src/Http/Routing/src/Builder/EndpointRoutingApplicationBuilderExtensions.cs
Show resolved
Hide resolved
src/Shared/ReRoute.cs
Outdated
| // ((IApplicationBuilder)WebApplication).New() does not copy globalRouteBuilderKey automatically like it does for all other properties. | ||
| builder.Properties[globalRouteBuilderKey] = routeBuilder; | ||
| // UseRouting() | ||
| if (builder.Properties.TryGetValue(useRoutingKey, out var useRouting) && useRouting is Func<IApplicationBuilder, IApplicationBuilder> useRoutingFunc) |
There was a problem hiding this comment.
This if should be first and then all the logic happens inside. Else, return the original next.
There was a problem hiding this comment.
I still think we could get rid of the __EndpointRouteBuidler property and just use __UseRouting in its place, but it looks like people have already taken a dependency on routing adding the __EndpointRouteBuildeer key, so maybe it's best to leave the existing keys alone.
I do not like adding more keys just to fix this layering problem, but hopefully this is the end of adding more keys just for rerouting purposes.
| @@ -31,6 +32,17 @@ public static IApplicationBuilder UsePathBase(this IApplicationBuilder app, Path | |||
| return app; | |||
| } | |||
|
|
|||
| const string globalRouteBuilderKey = "__GlobalEndpointRouteBuilder"; | |||
There was a problem hiding this comment.
Can we put all these constants in Reroute.cs and share them between all these projects?
| @@ -31,6 +32,17 @@ public static IApplicationBuilder UsePathBase(this IApplicationBuilder app, Path | |||
| return app; | |||
| } | |||
|
|
|||
| const string globalRouteBuilderKey = "__GlobalEndpointRouteBuilder"; | |||
| // Only use this path if there's a global router (in the 'WebApplication' case). | |||
There was a problem hiding this comment.
I know this isn't new, but is there really a scenario for calling UsePathBase() after route matching with or without WebApplication? I'm not saying we change this now, just curious.
There was a problem hiding this comment.
Pretty sure we had the same question for some of the other middleware when we initially created this "global routing" concept. And the answer was, why not make it work if it's easy.
There was a problem hiding this comment.
And the answer was, why not make it work if it's easy.
Then why didn't we make it work in the non-'WebApplication' case? This isn't feedback on the PR. I think we should leave it how you have it since it's a bit late to change that kind of behavior.
There was a problem hiding this comment.
non WAB doesn't have the global routing concept, and we don't have a way to effectively inject middleware before UseRouting.
amcasey
added
area-middleware
Fixes #38448
UsePathBasecan't referenceUseRouting(circular reference) so had to employ a bit of a trick to make it work.