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

Support registering OpenApiOperationTransformer via extension method for Minimal APIs #59180

Open
captainsafia opened this issue Nov 26, 2024 · 7 comments
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-openapi
Milestone

Comments

@captainsafia
Copy link
Member

captainsafia commented Nov 26, 2024

Background and Motivation

I'm opening this proposal after a conversation with @sander1095 in relation to #58723 and #58724.

Prior to .NET 9, we supported a WithOpenApi extension method on that when invoked would generate an OpenApiOperation, inject it into endpoint metadata, then rely on consuming OpenAPI implementations like Swashbuckle to pluck this OpenApiOperation and integrate it into the document that was being generated.

When we introduced built-in OpenAPI support in .NET 9, we opted not to bring in support for this strategy and instead steer people towards the new IOpenApiOperationTransformer abstraction for making modifications to their OpenAPI document.

However, one of the things that @sander1095 pointed out with this approach is that you lose the ability to have operation transformations colocated with the endpoint they affected. For example, let's say that I want to set the description for a response in a given endpoint. With the operation transformer model, I might have to write something like this:

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddOpenApi(options =>
{
    options.AddOperationTransformer((operation, context, ct) =>
    {
        if (context.Description.RelativePath == "weatherforecast")
        {
            operation.Responses["200"].Description = "Weather forecast for today";
        }
        return Task.CompletedTask;
    });
});

var app = builder.Build();

if (app.Environment.IsDevelopment())
{
    app.MapOpenApi();
}

app.MapGet("/weatherforecast", () =>
    new WeatherForecast(DateOnly.FromDateTime(DateTime.Now), 25, "Sunny"));

app.Run();

record WeatherForecast(DateOnly Date, int TemperatureC, string? Summary)
{
    public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
}

In addition to the transformer being far away from the associated endpoint, I also have to implement the associated path-based check myself.

Proposed API

This issue proposes introducing a WithOpenApiTransformer extension method that can be used to register an operation transformer for a given endpoint without having to use the global registration feature.

// Assembly: Microsoft.AspNetCore.OpenApi
namespace Microsoft.AspNetCore.Builder;

public static class OpenApiEndpointConventionBuilderExtensions
{
+    public static TBuilder AddOpenApiOperationTransformer<TBuilder>(this TBuilder builder, Func<OpenApiOperation, OpenApiOperationTransformerContext, CancellationToken, Task> transformer) where TBuilder : IEndpointConventionBuilder { }
}

Usage Examples

With this proposed API, the same example above could be re-implemented in the following way:

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddOpenApi();

var app = builder.Build();

if (app.Environment.IsDevelopment())
{
    app.MapOpenApi();
}

app.MapGet("/weatherforecast", () =>
    new WeatherForecast(DateOnly.FromDateTime(DateTime.Now), 25, "Sunny"))
    .AddOpenApiOperationTransformer((operation, context, ct) =>
    {
        operation.Responses["200"].Description = "Weather forecast for today";
        return Task.CompletedTask;
    });

app.Run();

record WeatherForecast(DateOnly Date, int TemperatureC, string? Summary)
{
    public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
}

There's also the option for 3rd party authors to provide their own extensions on top of this API to support further customizations. For example, the support for setting descriptions on responses as proposed in #58724 can be implemented in the following way:

namespace FooBar.OpenApiExtensions;

public static class ExtensionMethods
{
    public static RouteHandlerBuilder WithResponseDescription(this RouteHandlerBuilder builder, int statusCode, string description)
    {
        builder.AddOpenApiOperationTransformer((operation, context, cancellationToken)
        {
            operation.Responses[statusCode.ToString()].Description = description;
            return Task.CompletedTask;
        });
        return builder;
    }
}

With the consumption pattern in the invoked code being:

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddOpenApi();

var app = builder.Build();

if (app.Environment.IsDevelopment())
{
    app.MapOpenApi();
}

app.MapGet("/weatherforecast", () =>
    new WeatherForecast(DateOnly.FromDateTime(DateTime.Now), 25, "Sunny"))
    .WithResponseDescription(200, "Weather forecast for today");

app.Run();

record WeatherForecast(DateOnly Date, int TemperatureC, string? Summary)
{
    public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
}

Alternative Designs

  • We could instead update the implementation to support the pre-existing WithOpenApi overloads. However, this would prevent users from being able to access the OpenApiOperationTransformerContext to customize the behavior of the transformer.
  • The proposal only supports adding the transformer via the delegate pattern. We could consider supporting the other registration patterns as well although that might not be necessary.

Risks

  • This proposal does introduce additional complexity to the transformer registration APIs. Operation transformers become unique in the sense that they can be configured at the top-level via the options object and at the endpoint level. We'll also have to consider ordering semantics for when operation transformers are applied.

Addendum

In the first round of reviews on this API, there was feedback about whether it was feasible to reuse the existing WithOpenApi to fulfill this feature. After some investigation, a new API is required because:

@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi labels Nov 26, 2024
Copy link
Contributor

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

API review notes?

  • Can you do the same thing today with WithOpenApi?
    • That only works with Swashbuckle today in practice. This will be supported by AddOpenApi.
  • Given that we cannot make WithOpenApi to work with AddOpenApi does the name WithOpenApiTransformer create confusion?
  • WithOpenApiTransformer adds the context parameter that WithOpenApi doesn't offer.
  • Do WithOpenApiTransformer callbacks run before or after global transformers? After because it's the "nearest" so it can override.

We'd like to know if we can make the existing WithOpenApi method interoperate with AddOpenApi before agreeing to a new similar method.

@halter73 halter73 added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jan 23, 2025
@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Jan 30, 2025
@captainsafia
Copy link
Member Author

Bringing this back in for review after some more iteration. I've revised the API name to make it a little harder to associate this with the existing overloads.

Copy link
Contributor

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.

@bartonjs
Copy link
Member

  • Should the existing WithOpenApi methods be marked as Obsolete? Not at this time.
  • Looks good as proposed
// Assembly: Microsoft.AspNetCore.OpenApi
namespace Microsoft.AspNetCore.Builder;

public static class OpenApiEndpointConventionBuilderExtensions
{
    public static TBuilder AddOpenApiOperationTransformer<TBuilder>(this TBuilder builder, Func<OpenApiOperation, OpenApiOperationTransformerContext, CancellationToken, Task> transformer) where TBuilder : IEndpointConventionBuilder { }
}

@bartonjs bartonjs 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 Feb 24, 2025
@sander1095
Copy link
Contributor

Hi @bartonjs ! I created an issue to talk about the issues around the no-op behavior of WithOpenApi in modern projects in #59927 . Would you please provide some thoughts on that issue? :)

@crwsolutions
Copy link

Hi @captainsafia, I watched your ASP.NET API Review on youtube this morning where you discussed this issue. I personally think you guys do handle the situation WithOpenApi() a bit too easily (with the best of intentions). As I pointed out in my issue #59812. But also mentioned in #59927 of @sander1095 , we are currently being led astray by various forces. So what is the problem with making it obsolete? How long can that marker stay there? Is there no migration path for Swashbuckle then? So at least link some kind of change to this issue to naturally lead people in the right direction. Some remark down in the docs is not enough in my opinion.

To me, creating sample messages so they can be fired directly into the generated client is very obvious and part of creating an API. I assume more people do that. It's not enjoyable that you are now so easily led in the wrong direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-openapi
Projects
None yet
Development

No branches or pull requests

5 participants