-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Add support for global conventions on WebApplication #59983
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
[Fact] | ||
public async Task ThrowsExceptionOnNonRouteEndpointsAtTopLevel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@halter73 I realized while working through test cases for this that route groups are only designed to work with RouteEndpoints
due to the requirement to be able to concatenate the route prefix to the route when constructing the endpoint. See the comment you left here below.
aspnetcore/src/Http/Routing/src/EndpointDataSource.cs
Lines 45 to 50 in 31d5a5f
// Endpoint does not provide a RoutePattern but RouteEndpoint does. So it's impossible to apply a prefix for custom Endpoints. | |
// Supporting arbitrary Endpoints just to add group metadata would require changing the Endpoint type breaking any real scenario. | |
if (endpoint is not RouteEndpoint routeEndpoint) | |
{ | |
throw new NotSupportedException(Resources.FormatMapGroup_CustomEndpointUnsupported(endpoint.GetType())); | |
} |
We have two options at the moment:
- Leave things as is. The WebApplication will still work with our primary scenarios (Blazor, MVC, SignalR, etc.) but users using custom endpoints will have to use a branched pipeline to register endpoints.
- Revise the implementation of grouped endpoints to support un-routed endpoints, perhaps by defaulting to an empty prefix + route for all? I haven't evaluated how expensive this would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can really make WebApplication
throw for any non-RouteEndpoints
if they worked before, but maybe we could only throw if you've added a global convention.
I don't think it's even feasible to support global conventions for non-RouteEndpoint
s, because I imagine most non-RouteEndpoint
implementations would be some other derived type which we wouldn't be able to instantiate with the additional metadata from the global conventions.
At best, we could create a plain Endpoint
with the additional metadata. We could restrict it to support just RouteEndpoint
and plain non-derived Endpoint
(but only for global conventions and maybe prefix-less route groups), but I'd want to know exactly what the use case is before we expand support.
This also makes me wonder if just adding a parameterless MapGroup
overload is sufficient for this scenario. I know it lacks discoverability though.
Implements #59755.
This pull request introduces changes to the
WebApplication
class and its associated components to support global endpoint conventions.Changes include:
_dataSources
list withGlobalEndpointRouteBuilder
andRouteGroupBuilder
to create an implicit global route group for all endpoints (src/DefaultBuilder/src/WebApplication.cs
).Conventions
property to exposeIEndpointConventionBuilder
for the application (src/DefaultBuilder/src/WebApplication.cs
) that maps to underlyingRouteGroupBuilder
WebApplicationGlobalConventionTests
to verify the application of global conventions on endpoints (src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationGlobalConventionTests.cs
).Wait to review + merge until attached API proposal is reviewed.