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

.NET9: Microsoft.AspNetCore.OpenApi.OpenApiSchemaService should not be internal #57798

Closed
jornhd opened this issue Sep 11, 2024 · 7 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved

Comments

@jornhd
Copy link

jornhd commented Sep 11, 2024

Background and Motivation

As Swashbuckle is no longer supported in .NET 9, we've started migrating our operation filters to operation transformers to prepare for the .NET 9 release.

In some of our current Swashbuckle filters, we need to add types to the schema that isn't described in the ApiDescription, in order to reference these types in eg. operation.Responses.

We do this today with the following line:
var schema = context.SchemaGenerator.GenerateSchema(myType, context.SchemaRepository);

We tried to do the same in our IOpenApiOperationTransformer implementation:

var componentService = context.ApplicationServices.GetRequiredKeyedService<OpenApiSchemaService>(context.DocumentName);
var schema = await componentService.GetOrCreateSchemaAsync(myType, null, true, cancellationToken);

The problem is that OpenApiSchemaService is internal, not public, so it's impossible to add schema for additional types.

Proposed API

Make OpenApiSchemaService public, or provide GetOrCreateSchemaAsync in a public api, preferable the OpenApiOperationTransformerContext.

@jornhd jornhd added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 11, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Sep 11, 2024
@martincostello
Copy link
Member

As Swashbuckle is no longer supported in .NET 9

I assume you mean "not included in the ASP.NET Core 9 templates"? Swashbuckle.AspNetCore itself is still supported by its maintainers (of which I'm one), and there's support for .NET 9 in progress: domaindrivendev/Swashbuckle.AspNetCore#3007

it's impossible to add schema for additional types

You should be able to add additional schema components using IOpenApiOperationTransformer (via adding additional requests/responses) or via IOpenApiDocumentTransformer by adding components to the OpenApiDocument. You shouldn't need to have access to the internal implementation details to change the OpenAPI document like that.

@martincostello martincostello added feature-openapi 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 Sep 11, 2024
@jornhd
Copy link
Author

jornhd commented Sep 11, 2024

You should be able to add additional schema components using IOpenApiOperationTransformer (via adding additional requests/responses)

That's exactly what I'm trying to do! The problem is how do we create the schema for a type?

One example: The API is set up to handle and return exceptions as a custom type ErrorResponse.
Instead of setting this response type with attribute on every single method, we do this today in an OperationFilter.

Migrating this filter to an IOpenApiOperationTransformer implementation, we have this code:

public Task TransformAsync(OpenApiOperation operation, OpenApiOperationTransformerContext context, CancellationToken cancellationToken)
{
    if (operation.Responses.ContainsKey("500"))
    {
        return Task.CompletedTask;
    }
    
    // Next line is old Swashbuckle code - How can we do convert the type ErrorResponse to OpenApiSchema using IOpenApiOperationTransformer?
    // var schema = context.SchemaGenerator.GenerateSchema(typeof(ErrorResponse), context.SchemaRepository);

    var serverError = new OpenApiResponse
    {
        Description = "Server Error",
        Content =
        {
            { "application/json", new OpenApiMediaType { Schema = schema } }
        }
    };
    operation.Responses.Add("500", serverError);

    return Task.CompletedTask;
}

So the problem is, when we can't access the OpenApiSchemaService.GetOrCreateSchemaAsync, how are we supposed to get the schema for the custom type ErrorRespose?

@martincostello
Copy link
Member

My understanding is that, given a type, you can use the new JsonSchemaExporter.GetJsonSchemaAsNode() method, like the service does here to create a schema.

var schemaObject = JsonSchemaExporter.GetJsonSchemaAsNode(jsonSerializerOptions, typeof(ErrorResponse), jsonSchemaExporterOptions);
// Create an OpenApiSchema from the JsonNode returned above

That's not something I've done myself though. Rather than get into adding a schema directly, I've instead added metadata to the API endpoints to add additional produces metadata (e.g. ProducesProblem()), and then the internals add it to the schema automatically.

@jornhd
Copy link
Author

jornhd commented Sep 11, 2024

Ok, so we basically need to copy most of the service (the jsonSchemaExporterOptions is huge, and we still need to deserialize JsonNode). It would have been nice to have this in a public method like Swashbuckle has.

@captainsafia
Copy link
Member

@jornhd Thanks for filing this issue and trying out the new APIs.

The choice to make the OpenApiSchemaService internal as part of this release was intentional. As mentioned in this thread, the current OpenApiSchemaService implementation depends on the new JsonSchemaExporter APIs in STJ for generating schemas. There are some OpenAPI-specific mappings in our options object that are required when targeting Open API v3.0. We're hoping to add support for OpenAPI v3.1 in the future, which will change the shape and requirements of the schema service. So the choice to not make the APIs public at the moment is to give us the flexibility to design the right API in the future.

I do have one curious question about your scenario though. Where is the ErrorResponse type that you're returning manifesting in your API? Are you using middleware or filters to generate the response type?

I'd +1 @martincostello's recommendation above: if you can you should use ProducesProblem (there are ways to only call it once) to convey to the application that it returns that type. That way, that behavior is codified in the metadata of the APIs and isn't just superficially in the OpenAPI document. It also means you don't have to author transformers/filters for each OpenAPI package to configure this behavior.

@captainsafia captainsafia added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 11, 2024
@captainsafia captainsafia added this to the Discussions milestone Sep 11, 2024
@jornhd
Copy link
Author

jornhd commented Sep 13, 2024

I do have one curious question about your scenario though. Where is the ErrorResponse type that you're returning manifesting in your API? Are you using middleware or filters to generate the response type?

We do this using middleware.

I've learned now that we can add the response type to all methods by adding a custom convention with AddControllers option action. I guess we can do something similar in MinimalApi.

@martincostello @captainsafia Thanks for your inputs. You can close this issue.

@dotnet-policy-service dotnet-policy-service bot added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Sep 13, 2024
@martincostello martincostello added ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. and removed Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. labels Sep 13, 2024
Copy link
Contributor

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@dotnet-policy-service dotnet-policy-service bot removed this from the Discussions milestone Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved
Projects
None yet
Development

No branches or pull requests

3 participants