-
Notifications
You must be signed in to change notification settings - Fork 10.4k
OpenAPI: merge multiple body content types into one operation #61401
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Comment left inline.
@@ -295,6 +294,24 @@ private async Task<Dictionary<OperationType, OpenApiOperation>> GetOperationsAsy | |||
{ | |||
await endpointOperationTransformer.TransformAsync(operation, operationContext, cancellationToken); | |||
} | |||
|
|||
var operationType = description.GetOperationType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic looks great overall! However, I believe we want to apply this before operation transformers are applied so that transformers registered by the user get the correct view.
I'm reviewing this on mobile so if this is already the case and I missed it let me know.
Also, we might want to add a test case to verify that transformers see the correct data.
@captainsafia Thank you for the review. I made some adjustments to better support operation transformers. Your comments made me realize that with my original changes, the transformer would run multiple times for a given operation, once per ApiDescription. I've adjusted it to perform grouping and merging before the transformers runs for the operation. I added a property This should eliminate any possible breakage caused by this change - I'd imagine there are users whose operation transformers are not expecting to be run multiple times on one operation. I had to make a slight adjustment to My gut is that this change from last-wins to first-wins isn't a major problem in terms of breaking changes, but if it is a problem, I can change the merging logic to preserve the old behavior where the last declared endpoint is the primary source of information for a give path + method combination, instead of the first declared endpoint. |
I thought that having multiple endpoints with the same route and verb triggered analyzer warnings. Is that not the case or are these being suppressed for this use case? Also, I want to note ... in the related issue there is this statement:
I'm curious how this change will affect the generated OpenAPI if the app specifies that it should be OpenAPI v2. |
I'll investigate the impact on v2 - thanks for identifying that. The analyzer doesn't trigger in this case because aspnetcore/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/DetectAmbiguousActionRoutes.cs Line 168 in 5d0e3b0
|
It looks like for v2, the following happens:
So, it at least seems that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables merging multiple body content types into a single OpenAPI operation when multiple endpoints share the same path and HTTP method but have different [Consumes]
attributes. Previously, the last defined operation would overwrite earlier ones, resulting in lost content type definitions.
Key changes:
- Modified OpenAPI document generation to group endpoints by HTTP method and merge their request body content types
- Enhanced the operation transformer context to include all merged endpoint descriptions
- Updated test cases to verify the merging behavior and transformer execution
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
OpenApiDocumentService.cs | Core logic to group endpoints by HTTP method and merge request body content types |
OpenApiOperationTransformerContext.cs | Added AllDescriptions property to expose all merged endpoint descriptions |
OperationTransformerTests.cs | Added test verifying transformers execute once per merged operation set |
OpenApiDocumentServiceTests.RequestBody.cs | Added test for multiple content type merging behavior |
XmlController.cs | Updated route template from query parameter to path parameter |
Snapshot files | Updated test snapshots reflecting the route template change |
PublicAPI.Unshipped.txt | Added new public API surface for AllDescriptions property |
Co-authored-by: Copilot <[email protected]>
OpenAPI: merge multiple body content types for the same path/method
Description
This change merges multiple body content types into one operation definition in the OpenAPI document when there are multiple endpoints with the same path and HTTP method but different Consumes attributes.
The previous behavior would clobber existing operations in this scenario such that the last defined operation wins.
Fixes #58329