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

[Route Groups] WithTags et al. should target IEndpointConventionBuilder #41428

Closed
Tracked by #41433
halter73 opened this issue Apr 28, 2022 · 6 comments · Fixed by #41830
Closed
Tracked by #41433

[Route Groups] WithTags et al. should target IEndpointConventionBuilder #41428

halter73 opened this issue Apr 28, 2022 · 6 comments · Fixed by #41830
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing 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

halter73 commented Apr 28, 2022

WithTags and other OpenApiRouteHandlerBuilderExtensions don't work with route groups today because they target a custom IEndpointConventionBuilder type (RouteHandlerBuilder) rather than any IEndpointConventionBuilder. While we do plan to support custom convention builder types on route groups (#41427), it shouldn't be necessary for many of the methods on OpenApiRouteHandlerBuilderExtensions.

var group = app.MapGroup("/todos");
group.WithTags("tag"); // This doesn't compile 
group.WithDescription("description"); // ditto
group.WithSummary("summary"); // ditto

Describe the solution you'd like

We should retarget WithTags and similar methods to IEndpointConventionBuilder. If we cannot do that without breaking, maybe it should target GroupRouteBuilder (or a future IRouteGroup interface).

@BrennanConroy BrennanConroy added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Apr 28, 2022
@captainsafia
Copy link
Member

The AddFilter extension methods also suffer from this problem as they similarly target RouteHandlerBuilder.

@halter73 halter73 mentioned this issue Apr 28, 2022
4 tasks
@halter73
Copy link
Member Author

Indeed. Route filters are one of the main things we want to be able to apply to an entire group. Unlike WithTags, I don't think it makes sense to be able to apply RouteFilters to just any IEndpointConventionBuilder (though that's certainly up for discussion).

I am hoping that #41427 which tracks supporting custom IEndpointConventionBuilder types will resolve that problem. I need to update the issue to show example usages with AddFilter though. There's still a lot of design work there.

@halter73 halter73 added this to the .NET 7 Planning milestone May 3, 2022
@ghost
Copy link

ghost commented May 3, 2022

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.

@halter73
Copy link
Member Author

halter73 commented May 25, 2022

Background and Motivation

WithTags and other OpenApiRouteHandlerBuilderExtensions don't work with route groups today because they target a custom IEndpointConventionBuilder type (RouteHandlerBuilder) rather than any IEndpointConventionBuilder.

Proposed API

namespace Microsoft.AspNetCore.Http;

public static class OpenApiRouteHandlerBuilderExtensions
{
     public static RouteHandlerBuilder ExcludeFromDescription(this RouteHandlerBuilder builder);
+    public static TBuilder ExcludeFromDescription<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;

    public static RouteHandlerBuilder Produces<TResponse>(
        this RouteHandlerBuilder builder,
        int statusCode = StatusCodes.Status200OK,
        string? contentType = null,
        params string[] additionalContentTypes);
    public static RouteHandlerBuilder Produces(
        this RouteHandlerBuilder builder,
        int statusCode,
        Type? responseType = null,
        string? contentType = null,
        params string[] additionalContentTypes);
+    public static TBuilder Produces<TBuilder>(
+        this TBuilder builder,
+        Type? responseType = null,
+        int statusCode = StatusCodes.Status200OK,
+        string? contentType = null,
+        params string[] additionalContentTypes) where TBuilder : IEndpointConventionBuilder;

    public static RouteHandlerBuilder ProducesProblem(
        this RouteHandlerBuilder builder,
        int statusCode,
        string? contentType = null);
+    public static TBuilder ProducesProblem<TBuilder>(
+        this TBuilder builder,
+        int statusCode,
+        string? contentType = null) where TBuilder : IEndpointConventionBuilder;

    public static RouteHandlerBuilder ProducesValidationProblem(
        this RouteHandlerBuilder builder,
        int statusCode = StatusCodes.Status400BadRequest,
        string? contentType = null);
+    public static TBuilder ProducesValidationProblem<TBuilder>(
+        this TBuilder builder,
+        int statusCode = StatusCodes.Status400BadRequest,
+        string? contentType = null) where TBuilder : IEndpointConventionBuilder;

    public static RouteHandlerBuilder Accepts<TRequest>(
        this RouteHandlerBuilder builder,
        bool isOptional,
        string contentType,
        params string[] additionalContentTypes) where TRequest : notnull;
    public static RouteHandlerBuilder Accepts(
        this RouteHandlerBuilder builder,
        Type requestType,
        bool isOptional,
        string contentType,
        params string[] additionalContentTypes);
    public static RouteHandlerBuilder Accepts(
        this RouteHandlerBuilder builder,
        Type requestType,
        string contentType,
        params string[] additionalContentTypes);
+    public static TBuilder Accepts<TBuilder>(
+        this TBuilder builder,
+        Type requestType,
+        bool isOptional,
+        string contentType,
+        params string[] additionalContentTypes) where TBuilder : IEndpointConventionBuilder;
+    public static TBuilder Accepts<TBuilder>(
+        this TBuilder builder,
+        Type requestType,
+        string contentType,
+        params string[] additionalContentTypes) where TBuilder : IEndpointConventionBuilder;

    public static RouteHandlerBuilder WithTags(this RouteHandlerBuilder builder, params string[] tags);
+    public static TBuilder WithTags<TBuilder>(this TBuilder builder, params string[] tags) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder WithDescription(this RouteHandlerBuilder builder, string description);
+    public static TBuilder WithDescription<TBuilder>(this TBuilder builder, string description) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder WithSummary(this RouteHandlerBuilder builder, string summary);
+    public static TBuilder WithSummary<TBuilder>(this TBuilder builder, string summary) where TBuilder : IEndpointConventionBuilder

Usage Examples

var todoGroup = app.MapGroup("/todos");
todoGroup.WithTags("todos");
todoGroup.WithSummary("An endpoint for managing todos.");

todoGroup.MapGet("/", (TodoDb db) => TypedResults.Ok(await db.Todos.ToArrayAsync()));
todoGroup.MapPost("/todos", (Todo todo, TodoDb db) =>
{
    // ....

Alternative Designs

  • Keep some of these targeting RouteHandlerBuilder only. This seems unnecessary.
  • Add TBuilder Produces<TResponse, TBuilder> and TBuilder Accepts<TRequest, TBuilder> overloads too. If we did this, the TBuilder type could not be inferred once the TResponse or TRequest types is specified.

Risks

Low. No overloads that exist in .NET 6 are removed. I also added tests calling all the new and old overloads to verify they do not cause ambiguity.

This does cause these extension methods to show up in a bunch of new places however like on the builders returned by MapControllers() and MapHub(). It could be argued that these extension methods don't belong there, but I don't think it hurts. I think it can make sense to apply metadata to all endpoints defined by these methods in some scenarios. And this has the added benefit of now also working with the RequestDelegate overloads of MapGet, MapPost, etc...

@halter73 halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label May 25, 2022
@ghost
Copy link

ghost commented May 25, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member Author

API Review Notes:

  • Do we need to make the this generic or could it be IEndpointConventionBuilder?
    • Returning TBuilder for better chaining is the reason for not just using IEndpointConventionBuilder directly.
  • app.MapGet("/", () => {}).Produces() now compiles because every argument has a default value (typeof(void) for the responseType and 200 for status). Previously, one of either the status code or response type was required. I did this so you wouldn't need to manually pass in null or typeof(void) just to add a produced status code to a group. Is this a problem?
    • Yes this is a problem. What are the alternatives?
      a. Leave defaults for all parameters. Produces(responseType = null, statusCode = 200, …
      b. Leave default for statusCode but not responseType. Produces(responseType, statusCode = 200”, …
      c. No default for responseType or statusCode. Produces(responseType, statusCode, …
      d. Remove the MapGroup-supporting TBuilder overloads for Produces/ProducesProblem/ProducesValidationProblem/Accepts because people don’t need them on groups.
    • Option "d" wins the vote over option "a" because:
      • A group of endpoints having the same produces or accepts metadata doesn't seem super common compared to tags for example.
      • Produces and accepts metadata can be added manually with WithMetadata if you really need to.
      • Something like a common set a ProblemDetails for a group of endpoints might be common enough, but we can always add the Produces/ProducesProblem/ProducesValidationProblem/Accepts overloads or some subset back if we want to. Removing overloads is harder.

Approved API:

namespace Microsoft.AspNetCore.Http;

public static class OpenApiRouteHandlerBuilderExtensions
{
     public static RouteHandlerBuilder ExcludeFromDescription(this RouteHandlerBuilder builder);
+    public static TBuilder ExcludeFromDescription<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;

    public static RouteHandlerBuilder WithTags(this RouteHandlerBuilder builder, params string[] tags);
+    public static TBuilder WithTags<TBuilder>(this TBuilder builder, params string[] tags) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder WithDescription(this RouteHandlerBuilder builder, string description);
+    public static TBuilder WithDescription<TBuilder>(this TBuilder builder, string description) where TBuilder : IEndpointConventionBuilder;

-    public static RouteHandlerBuilder WithSummary(this RouteHandlerBuilder builder, string summary);
+    public static TBuilder WithSummary<TBuilder>(this TBuilder builder, string summary) where TBuilder : IEndpointConventionBuilder

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 31, 2022
@rafikiassumani-msft rafikiassumani-msft added the feature-minimal-actions Controller-like actions for endpoint routing label May 31, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 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
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing 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.

5 participants