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

Add support for an interface for OpenAPI operation transformations #56022

Closed
martincostello opened this issue Jun 2, 2024 · 5 comments · Fixed by #56395
Closed

Add support for an interface for OpenAPI operation transformations #56022

martincostello opened this issue Jun 2, 2024 · 5 comments · Fixed by #56395
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 area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi

Comments

@martincostello
Copy link
Member

martincostello commented Jun 2, 2024

Background and Motivation

In the new OpenAPI functionality in .NET 9 preview.4 there is a first-class interface for document transformations, IOpenApiDocumentTransformer, but there isn't an analogous interface for operations.

It is possible to perform operation transformations by providing a lambda function, but this can quickly get complicated if you want to do things that are non-trivial, use dependencies from DI etc. compared to if you could encapsulate your logic in a class to perform a specific operation like is possible for document transforms.

Proposed API

namespace Microsoft.AspNetCore.OpenApi;

+/// <summary>
+/// Represents a transformer that can be used to modify an OpenAPI operation.
+/// </summary>
+public interface IOpenApiOperationTransformer
+{
+    Task TransformAsync(OpenApiOperation operation, OpenApiOperationTransformerContext context, CancellationToken cancellationToken);
+}

public partial sealed class OpenApiOptions
{
+    public OpenApiOptions UseOperationTransformer<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TTransformerType>()
+        where TTransformerType : IOpenApiOperationTransformer
+    public OpenApiOptions UseOperationTransformer(IOpenApiOperationTransformer transformer)
}

These would be implemented the same way the document support is, so that DI works as expected to activate the types, it's AoT-friendly etc.

Usage Examples

services.AddOpenApi("v1", (options) =>
{
    options.UseOperationTransformer<MyOperationTransformer>();
});

public sealed class MyOperationTransformer(MyService someService, IConfiguration configuration) : IOpenApiOperationTransformer
{
    public Task TransformAsync(
        OpenApiOperation operation,
        OpenApiOperationTransformerContext context,
        CancellationToken cancellationToken)
    {
        if (configuration["DoWhatever"] == "true")
        {
            await someService.DoStuff(operation, cancellationToken);
        }
    }
}

Alternative Designs

Make the code that enumerates the operations re-usable to public callers so that if an operation interface isn't desired, then the user can implement a document transformer to easily enumerate the operations to do a transform in their custom implementation of the interface. Maybe a base class that implements IOpenApiDocumentTransformer and applies the visitor pattern to allow the user to override methods to apply transforms:

namespace Microsoft.AspNetCore.OpenApi;

+public abstract class OpenApiOperationTransformer : IOpenApiDocumentTransformer
+{
+    protected OpenApiOperationTransformer();
+
+    public async Task TransformAsync(OpenApiDocument document, OpenApiDocumentTransformerContext context, CancellationToken cancellationToken); // Enumerates the operations and calls TransformOperationAsync() for each
+
+    protected abstract Task TransformOperationAsync(OpenApiOperation operation, OpenApiOperationTransformerContext context, CancellationToken cancellationToken);
}

Risks

Increased complexity of the OpenAPI generator implementation.

@martincostello martincostello added api-suggestion Early API idea and discussion, it is NOT ready for implementation feature-openapi labels Jun 2, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Jun 2, 2024
@martincostello martincostello added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Jun 2, 2024
@captainsafia
Copy link
Member

@martincostello Thanks for filing this issue!

We intentionally kept the surface area for the transformers API small to start, with the option to introduce an interface for operation transformers if the feedback arose. It appears the feedback has arisen.

The API that you've proposed is pretty close in shape to what was originally encapsulated in the API proposal for transformers. The only thing missing is a UseTransformer(IOpenApiOperationTransformer) registration overload. I'll add it to the proposal for completeness and we can discuss if it is necessary during API review.

@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 5, 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.

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 5, 2024
@halter73
Copy link
Member

  • Will this create too many overloads on OpenApiOptions around adding transformers?
    • Maybe, but trying to combine the all the transformers into a single interface type seems worse.
  • Would we prefer to use "Add" over "Use" for the extension method?
    • This is more consistent with endpoint filters.
    • "Add" wins, we'll update the other "Use" methods for consistency.
  • Let's also change UseTransformer to AddDocumentTransformer for consistency since it takes an IDocumentTransformer and it doesn't encompass schema transformations.
  • Let's add IOpenApiSchemaTransformer for more consistency between the three types of transformers.

API approved.

namespace Microsoft.AspNetCore.OpenApi;

+/// <summary>
+/// Represents a transformer that can be used to modify an OpenAPI operation.
+/// </summary>
+public interface IOpenApiOperationTransformer
+{
+    Task TransformAsync(OpenApiOperation operation, OpenApiOperationTransformerContext context, CancellationToken cancellationToken);
+}

+ public interface IOpenApiSchemaTransformer
+ {
+  Task TransformAsync(OpenApiSchema schema, OpenApiSchemaTransformerContext context, CancellationToken cancellationToken);
+ }

public partial sealed class OpenApiOptions
{
+    public OpenApiOptions AddOperationTransformer<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TTransformerType>()
+        where TTransformerType : IOpenApiOperationTransformer
+    public OpenApiOptions AddOperationTransformer(IOpenApiOperationTransformer transformer)

-  public OpenApiOptions UseTransformer<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TTransformerType>()
-        where TTransformerType : IOpenApiDocumentTransformer
+  public OpenApiOptions AddDocumentTransformer<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TTransformerType>()
+        where TTransformerType : IOpenApiDocumentTransformer
-  public OpenApiOptions UseTransformer(IOpenApiDocumentTransformer transformer)
+  public OpenApiOptions AddDocumentTransformer(IOpenApiDocumentTransformer transformer)
-  public OpenApiOptions UseTransformer(Func<OpenApiDocument, OpenApiDocumentTransformerContext, CancellationToken, Task> transformer);
+  public OpenApiOptions AddDocumentTransformer(Func<OpenApiDocument, OpenApiDocumentTransformerContext, CancellationToken, Task> transformer);
-  public OpenApiOptions UseOperationTransformer(Func<OpenApiOperation, OpenApiOperationTransformerContext, CancellationToken, Task> transformer);
+  public OpenApiOptions AddOperationTransformer(Func<OpenApiOperation, OpenApiOperationTransformerContext, CancellationToken, Task> transformer);
-  public OpenApiOptions UseSchemaTransformer(Func<OpenApiSchema, OpenApiSchemaTransformerContext, CancellationToken, Task> transformer);
+  public OpenApiOptions AddSchemaTransformer(Func<OpenApiSchema, OpenApiSchemaTransformerContext, CancellationToken, Task> transformer);


+    public OpenApiOptions AddSchemaTransformer<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TTransformerType>()
+        where TTransformerType : IOpenApiSchemaTransformer
+    public OpenApiOptions AddSchemaTransformer(IOpenApiSchemaTransformer transformer);
}

@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 Jun 20, 2024
@martincostello
Copy link
Member Author

@captainsafia I'm happy to pick this up, unless you're planning on tackling it yourself.

@captainsafia
Copy link
Member

@martincostello I'd be happy to review a PR if you're able to open one! 🙇🏽

martincostello added a commit to martincostello/aspnetcore that referenced this issue Jun 22, 2024
- Add `IOpenApiOperationTransformer`.
- Add `IOpenApiSchemaTransformer`.
- Rename `Use*Transfomer()` methods to `Add*Transformer()`.

Resolves dotnet#56022.
martincostello added a commit to martincostello/aspnetcore that referenced this issue Jun 26, 2024
- Add `IOpenApiOperationTransformer`.
- Add `IOpenApiSchemaTransformer`.
- Rename `Use*Transfomer()` methods to `Add*Transformer()`.

Resolves dotnet#56022.
martincostello added a commit to martincostello/aspnetcore that referenced this issue Jul 9, 2024
- Add `IOpenApiOperationTransformer`.
- Add `IOpenApiSchemaTransformer`.
- Rename `Use*Transfomer()` methods to `Add*Transformer()`.

Resolves dotnet#56022.
martincostello added a commit to martincostello/aspnetcore that referenced this issue Jul 13, 2024
- Add `IOpenApiOperationTransformer`.
- Add `IOpenApiSchemaTransformer`.
- Rename `Use*Transfomer()` methods to `Add*Transformer()`.

Resolves dotnet#56022.
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 area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants