Skip to content

Remove RequestDelegateFactory call from RouteEndpointBuilder #42703

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

Closed
halter73 opened this issue Jul 13, 2022 · 0 comments · Fixed by #42827
Closed

Remove RequestDelegateFactory call from RouteEndpointBuilder #42703

halter73 opened this issue Jul 13, 2022 · 0 comments · Fixed by #42827
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc design-proposal This issue represents a design proposal for a different issue, linked in the description 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
Milestone

Comments

@halter73
Copy link
Member

The proposal is to remove the call to RequestDelegateFactory.Create() I added to RouteEndpointBuilder.Build() in #42195. The discussion surrounding its inclusion is here. The main impact of this change is that filters run on all endpoints, not just minimal route handlers.

Currently, if an endpoint is not a minimal route handler, the method being filtered is just a RequestDelegate even if that RequestDelegate ultimately called an MVC action or SignalR Hub method with a different method signature.

If we only stopped calling RDF.Create() in REB.Build() and made no other changes, this would mean filters would go back to only working with minimal route handlers despite now being applicable to any IEndpointConventionBuilder. We could conceivably come up with analyzers that give errors when applied to IEndpointConventionBuilders we know won't work, but that still doesn't help that filters applied to groups would only run for minimal route handlers and no other endpoints.

Fortunately, in addition to removing RDF.Create() from REB.Build(), we're planning to add better support for what are now "EndpointFilters" instead of "RouteHandlerFilters" in MVC with #42557. With this PR, the actual MVC action ends the filter pipeline instead of a RequestDelegate that indirectly invokes the action.

Furthermore, we should update our internal ModelEndpointDataSource to run filters on the RequestDelegates passed to MapGet and its associated methods. It make sense to use RequestDelegate as the final method in the filter pipeline in this case, because that's what the developer defined the endpoint with.

Some things like MapHub just call the RequestDelegate Map method overloads. For things like SignalR Hubs that might have another plausible way of filtering the endpoint other than just by filtering the RequestDelegate it defines, we should probably start using a custom internal EndpointDataSource that does not run filters on the RequestDelegates.

Based on early discussions, we don't think we'll be running these filters for Razor Pages or SignalR Hub invocations in .NET 7. If we decide to run filters on more things in .NET 8, we'll need to figure out how to do it in a way that is sufficiently non-breaking way, but we'll cross that bridge when we get there. Any change to filter behavior without an opt-in switch is potentially breaking.

In general, the change here is to make the EndpointDataSource implementations solely responsible for running filters. This means that anyone using a custom EndpointDataSource won't have filters run automatically when building their RouteEndpoints, but at least they can run them manually. I think custom EndpointDataSource implementations are relatively rare in any case.

@halter73 halter73 added design-proposal This issue represents a design proposal for a different issue, linked in the description old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jul 13, 2022
@halter73 halter73 changed the title Remove call to RequestDelegateFactory from RouteEndpointBuilder.Build() Remove RequestDelegateFactory call from RouteEndpointBuilder Jul 13, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-rc1 milestone Jul 13, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 5, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc design-proposal This issue represents a design proposal for a different issue, linked in the description 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
Development

Successfully merging a pull request may close this issue.

3 participants