Add entry-point APIs for OpenAPI support #54789

captainsafia · 2024-03-26T22:23:51Z

This PR adds the set of entry-point APIs needed for build-in OpenAPI document generation to the framework.

The included APIs are tracked in the following issues:

Note: this PR intentionally lacks any functionality for generating the actual document. This will be included in follow-up PRs to the feature branch.

src/OpenApi/sample/EndpointRouteBuilderExtensions.cs

Rick-Anderson

@tdykstra please review my review

src/OpenApi/src/Services/IDocumentProvider.cs

src/OpenApi/src/Services/OpenApiDocumentProvider.cs

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

src/OpenApi/src/Extensions/OpenApiServiceCollectionExtensions.cs

src/OpenApi/src/Services/OpenApiComponentService.cs

src/OpenApi/src/Services/OpenApiDocumentProvider.cs

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/src/Services/OpenApiOptions.cs

Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]>

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/src/Services/OpenApiOptions.cs

eng/Versions.props

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

src/OpenApi/src/Services/NamedService.cs

src/OpenApi/src/Services/OpenApiDocumentProvider.cs

amcasey · 2024-03-28T01:40:09Z

src/OpenApi/test/Services/OpenApiDocumentProviderTests.cs

+        var documentService = new OpenApiDocumentService(hostEnvironment);
+        var serviceCollection = new ServiceCollection();
+        serviceCollection
+            .AddOpenApi("v2")


What happens if I call .AddOpenApi("foo").AddOpenApi("foo")? Is it worth testing?

We're used keyed DI under the hood so the last registration is used. For the exact scenario you presented, you'll get virtually the same behavior. If you did something like this though:

services .AddOpenApi("v1") .AddOpenApi("v1", options => options.OpenApiVersion = OpenApiSpecVersion.OpenApi2_0);

Then you'll get a document serialized using the OpenApi v2 since the last registration is favored.

I'm pretty sure you'll get the document serialized using OpenApio v2 in the given the opposite order too, right?

services .AddOpenApi("v1", options => options.OpenApiVersion = OpenApiSpecVersion.OpenApi2_0) .AddOpenApi("v1");

Sounds like we need a test. 😄

@halter73 Yep, you're right. The test I added here didn't cover this scenario since I was overriding the value of OpenApiVersion in the second call.

Digging deeper into the Options code, it looks like each registered IConfigureOptions instance is called for the same named option (ref), so what you effectively get is a union of all the configured options.

Perhaps this is "fine" behavior. Thoughts?

The reason you might want to add a document twice is to modify the configuration of a document added elsewhere in the code? i.e. there's a reason to not just throw when this happens (which I'm sure is unidiomatic in aspnetcore)?

The reason you might want to add a document twice is to modify the configuration of a document added elsewhere in the code?

That would be one reason yes. In which case, the fact that configurations get unioned would make sense. My suspicion is that users will likely only be calling AddOpenApi once per document in their code.

i.e. there's a reason to not just throw when this happens (which I'm sure is unidiomatic in aspnetcore)?

Throwing is an option, but feels a little aggressive, assuming the default behavior is well-documented and sane enough.

I jotted this as an implementation consideration and will solicit feedback on the API issue associated with this.

src/OpenApi/src/Extensions/OpenApiServiceCollectionExtensions.cs

halter73 · 2024-03-28T18:01:35Z

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

+                var documentService = context.RequestServices.GetRequiredKeyedService<OpenApiDocumentService>(documentName);
+                var document = await documentService.GetOpenApiDocumentAsync();
+                var documentOptions = options.Get(documentName);
+                using var stringWriter = new StringWriter(CultureInfo.InvariantCulture);


This seems allocatey. What about using SignalR's Utf8BufferTextWriter with the response PipeWriter instead? It will buffer indefinitely without flushing, so blocking shouldn't be a concern.

Good pointer to Utf8BufferTextWriter. I think I was able to wire this up correctly with the in-memory buffer writer.

halter73 · 2024-03-28T18:07:48Z

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

+                // It would be ideal to use the `HttpResponseStreamWriter` to
+                // asynchronously write to the response stream here but Microsoft.OpenApi
+                // does not yet support async APIs on their writers.
+                // See https://github.com/microsoft/OpenAPI.NET/issues/421 for more info.


Have we pushed to get this resolved?

Not aggressively -- it's on deck for their v2.1 release. I think if we're able to work around this with the buffered writer we should be fine.

halter73 · 2024-03-28T18:11:43Z

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

+                var documentOptions = options.Get(documentName);
+                using var stringWriter = new StringWriter(CultureInfo.InvariantCulture);
+                var jsonWriter = new OpenApiJsonWriter(stringWriter);
+                document.Serialize(jsonWriter, documentOptions.OpenApiVersion);


Another thing we could consider is caching the response for each document in a byte array. If we do that, the impact of this becomes smaller because all the concurrent requests for a given document would share a buffer. We could lean on output caching middleware for this, but I don't think that should be necessary.

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

halter73 · 2024-03-28T18:23:16Z

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

+                    context.Response.StatusCode = StatusCodes.Status200OK;
+                    context.Response.ContentType = "application/json;charset=utf-8";
+                    await context.Response.WriteAsync(stringWriter.ToString(), context.RequestAborted);


Would it be better to use TypedResults.Text() for both the 200 and 404 responses if Utf8BufferTextWriter doesn't work out?

Suggested change

context.Response.StatusCode = StatusCodes.Status200OK;

context.Response.ContentType = "application/json;charset=utf-8";

await context.Response.WriteAsync(stringWriter.ToString(), context.RequestAborted);

return TypedResults.Text(stringWriter.ToString(), "application/json;charset=utf-8");

That way you could remove the HttpContext parameter and inject the IServiceProvider instead.

src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs

halter73 · 2024-03-28T18:29:09Z

src/OpenApi/test/Services/OpenApiDocumentProviderTests.cs

+        var documentService = new OpenApiDocumentService(hostEnvironment);
+        var serviceCollection = new ServiceCollection();
+        serviceCollection
+            .AddOpenApi("v2")


I'm pretty sure you'll get the document serialized using OpenApio v2 in the given the opposite order too, right?

services .AddOpenApi("v1", options => options.OpenApiVersion = OpenApiSpecVersion.OpenApi2_0) .AddOpenApi("v1");

halter73 · 2024-03-28T18:32:51Z

src/OpenApi/test/Extensions/OpenApiEndpointRouteBuilderExtensionsTests.cs

+        context.Response.Body = responseBodyStream;
+        context.RequestServices = serviceProvider;
+        context.Request.RouteValues.Add("documentName", "v2");
+        var endpoint = builder.DataSources.First().Endpoints.First();


It might be easier in the long run to convert these to use TestServer.

I don't expect us to add too many additional tests to this particular codepath after this PR. I have an alternate strategy for validating the contents of the OpenAPI document that actually gets generated.

captainsafia · 2024-03-29T20:07:06Z

I'n going to go ahead and merge this for now. Follow-ups include:

Close on the best way to handle duplicate AddOpenApi calls
Verify implementation with Utf8BufferedTextWriter in endpoint

* Add entry-point APIs for OpenAPI support * Apply suggestions from code review Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]> * Update docs and pass cancellation token to WriteAsync * Address feedback * Remove trailing comma in slnf * Address more feedback * Seal OpenApiOptions --------- Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]>

Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]> This PR adds support for OpenAPI document generation, sans schema generation to Microsoft.AspNetCore.OpenApi. Relevant changes are available in individual PRs: - Add entry-point APIs for OpenAPI support (#54789) - Support resolving OpenApiPaths entries from document (#54847) - Support generating OpenAPI operation and associated fields (#54903) - Add APIs for OpenAPI document transformers (#54935) - Add support for generating OpenAPI parameters (#55041) - Add support for generating OpenAPI responses (#55020) - Add support for generating OpenAPI request bodies (#55040)

Add entry-point APIs for OpenAPI support

12d1ef0

captainsafia requested review from a team and wtgodbe as code owners March 26, 2024 22:23

dotnet-issue-labeler bot 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 Mar 26, 2024

captainsafia requested review from amcasey, Rick-Anderson and tdykstra March 26, 2024 22:24

ilmax reviewed Mar 26, 2024

View reviewed changes

src/OpenApi/sample/EndpointRouteBuilderExtensions.cs Show resolved Hide resolved

Rick-Anderson requested changes Mar 27, 2024

View reviewed changes

src/OpenApi/src/Services/IDocumentProvider.cs Outdated Show resolved Hide resolved

src/OpenApi/src/Services/OpenApiDocumentProvider.cs Outdated Show resolved Hide resolved

martincostello reviewed Mar 27, 2024

View reviewed changes

captainsafia and others added 2 commits March 27, 2024 10:15

Apply suggestions from code review

c4ee086

Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]>

Update docs and pass cancellation token to WriteAsync

8d05001

captainsafia requested a review from Rick-Anderson March 27, 2024 18:52

Rick-Anderson approved these changes Mar 27, 2024

View reviewed changes

amcasey approved these changes Mar 28, 2024

View reviewed changes

captainsafia added 2 commits March 28, 2024 10:54

Address feedback

f154712

Remove trailing comma in slnf

b2984a6

halter73 approved these changes Mar 28, 2024

View reviewed changes

halter73 reviewed Mar 28, 2024

View reviewed changes

captainsafia added 2 commits March 29, 2024 10:45

Address more feedback

cda4b37

Seal OpenApiOptions

ef6fcf7

captainsafia merged commit b082c5f into feature/openapi Mar 29, 2024
19 of 21 checks passed

captainsafia deleted the safia/api-scaffold branch March 29, 2024 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add entry-point APIs for OpenAPI support #54789

Add entry-point APIs for OpenAPI support #54789

captainsafia commented Mar 26, 2024 •

edited

Loading

Rick-Anderson left a comment

amcasey Mar 28, 2024

captainsafia Mar 28, 2024

halter73 Mar 28, 2024

amcasey Mar 28, 2024

captainsafia Mar 29, 2024

amcasey Mar 29, 2024 •

edited

Loading

captainsafia Mar 29, 2024

captainsafia Mar 29, 2024

halter73 Mar 28, 2024

captainsafia Mar 29, 2024

halter73 Mar 28, 2024

captainsafia Mar 29, 2024

halter73 Mar 28, 2024

halter73 Mar 28, 2024 •

edited

Loading

halter73 Mar 28, 2024

halter73 Mar 28, 2024

captainsafia Mar 29, 2024

captainsafia commented Mar 29, 2024

Add entry-point APIs for OpenAPI support #54789

Add entry-point APIs for OpenAPI support #54789

Conversation

captainsafia commented Mar 26, 2024 • edited Loading

Rick-Anderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainsafia commented Mar 29, 2024

captainsafia commented Mar 26, 2024 •

edited

Loading

amcasey Mar 29, 2024 •

edited

Loading

halter73 Mar 28, 2024 •

edited

Loading