Conversation
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Adds OpenAPI generation support for the HTTP QUERY operation (introduced as a first-class PathItem operation in OpenAPI 3.2), and updates integration snapshots/tests to validate correct serialization across OpenAPI versions.
Changes:
- Map
ApiDescription.HttpMethod == "QUERY"to an OpenAPI operation and ensure it’s treated like other body-less methods for inferred body behavior. - Include
QUERYin the set of supported OpenAPI HTTP methods for path generation. - Update OpenAPI 3.0/3.1/3.2 snapshot baselines to reflect
QUERYserialization (nativequeryin 3.2; extension-based in earlier versions).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OpenApi/src/Extensions/ApiDescriptionExtensions.cs | Adds QUERY to the ApiDescription→HttpMethod mapping and updates the spec reference link. |
| src/OpenApi/src/Services/OpenApiConstants.cs | Adds HttpMethod.Query to the preallocated HTTP method list used during path generation. |
| src/OpenApi/src/Services/OpenApiGenerator.cs | Treats QUERY as a method that normally does not infer a request body. |
| src/OpenApi/src/Services/OpenApiDocumentService.cs | Contains unrelated formatting-only changes in several edited lines. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/ApiDescriptionExtensionsTests.cs | Extends method mapping test data to include QUERY and adds an unsupported-method test. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApiDocumentLocalizationTests.VerifyOpenApiDocumentIsInvariant.verified.txt | Snapshot updated to include /query output using additional-operations extension format. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_2/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt | Snapshot updated to serialize /query using the OpenAPI 3.2 query operation field. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_1/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt | Snapshot updated to serialize /query via x-oai-additionalOperations. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_0/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt | Snapshot updated to serialize /query via x-oai-additionalOperations. |
|
There's lots of whitespace changes in this PR that shouldn't be here, it also incorrectly emits QUERY operations in OpenAPI documents that aren't targeting version 3.2 of the specification (see #64368 (comment))). |
Why is this an issue? it's being emitted as an extension when looking at the unit tests. Which means the document in earlier versions is valid (albeit not very useful). |
IIRC, they caused the documents to fail schema validation: #63092. Does the Microsoft.OpenApi schema validation now allow QUERY for all document versions? |
|
@martincostello I'm not sure what you mean by "schema"? If you're referring to the OpenAPI schemas that can be found here since query was added for 3.2.0 only, it's valid only for that version. Earlier versions need to use an extension instead. If you're referring to the Microsoft.OpenApi object model, yes, that property (and serialization/deserialization for it) was added in v3 of the library, which brings supports for OpenAPI 3.2.0, and which we have upgraded for asp.net 11 preview2. |
|
By schema I meant the schema documents that say what a valid OpenAPI document looks like (the schema for the schemas). If the library has evolved since the version where I made the test changes then it's not a concern as the tests won't break, but at the time adding QUERY to a 3.0 and 3.1 document would have caused the tests to fail when Microsoft.OpenApi was used to validate the object model, as it would contain a validation diagnostic in the result here: |
|
@martincostello thanks for sharing this. Since that validation is implemented in Microsoft.OpenAPI, if that's still a challenge (it should not), I can always put a bugfix together there. Bottom line being:
In both cases, it should not lead to any validation issue from Microsoft.OpenApi (v3) |
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/ApiDescriptionExtensionsTests.cs
Outdated
Show resolved
Hide resolved
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/ApiDescriptionExtensionsTests.cs
Outdated
Show resolved
Hide resolved
mikekistler
left a comment
There was a problem hiding this comment.
QUERY operations must be able to accept a body, and we should have tests that verify this.
…om/kilifu/aspnetcore into dotnetgh-65489-add-query-operation
|
@martincostello @mikekistler I believe all the feedback as been addressed by @kilifu would you mind giving it another review please? |
mikekistler
left a comment
There was a problem hiding this comment.
Thanks for addressing my prior comments, but now I think there is another issue. I do not see any support for minimal APIs, and all the tests are using a controller-based sample. I think at minimum there should be a MapQuery method that produces a query endpoint and corresponding "query" operation in the OpenAPI.
|
There is already a Minimal API QUERY endpoint in the tests - I added it last year. It's why the snapshots have two query endpoints in them even though only one endpoint was added in this PR. It was added to validate that query wasn't in the OpenAPI document previously. |
There was some previous discussion about this when we added My opinion, and the opinion of those in the meeting at the time, is that it's a little premature to add a |
|
While I understand we wouldn't want to add query async to the dotnet client based on a draft RFC. I'm not sure why the map query support should be contingent to that? It would make sense that the open API description at least supports "documenting" the operation. Wouldn't it? |
If I was in this meeting it has vanished from my memory, but in any event I defer to @halter73 on this. I'll switch my review to approve. |
mikekistler
left a comment
There was a problem hiding this comment.
Switching my review to approve as the team has decided to defer support for MapQuery.
[OpenAPI] Support HTTP QUERY
Summary of the changes (Less than 80 chars)
Description
add support for query operations
Fixes #65489