-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[OpenAPI] Use invariant culture for TextWriter #62193
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
Ensure OpenAPI documents are written to a culture-invariant `TextWriter` implementation. Contributes to dotnet#60628.
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 updates the OpenAPI implementation to enforce the usage of an invariant culture when writing documents using TextWriter. Key changes include adding a new constructor overload to Utf8BufferTextWriter that accepts an IFormatProvider, updating OpenAPI endpoint routing to explicitly use CultureInfo.InvariantCulture, and adding tests and a sample endpoint ("/getcultureinvariant") to verify the culture invariance behavior.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/SignalR/common/Shared/Utf8BufferTextWriter.cs | Added a new constructor overload to inject an IFormatProvider. |
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_1/... | Updated snapshots to include a new "/getcultureinvariant" endpoint. |
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_0/... | Updated snapshots to include a new "/getcultureinvariant" endpoint. |
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentIntegrationTests.cs | Refactored test data setup using MemberData and updated the verification call. |
src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs | Modified the instantiation of Utf8BufferTextWriter to use CultureInfo.InvariantCulture. |
src/OpenApi/sample/Program.cs | Added middleware to change the current culture for testing invariant behavior. |
src/OpenApi/sample/Controllers/TestController.cs | Introduced a new GET endpoint and record type to support culture-invariant document generation. |
Comments suppressed due to low confidence (2)
src/OpenApi/sample/Controllers/TestController.cs:35
- [nitpick] The method name 'PostTypedResult' is potentially misleading for a GET endpoint. Consider renaming it to something like 'GetCultureInvariant' to reflect its purpose more clearly.
[HttpGet]
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentIntegrationTests.cs:49
- The change from 'Verifier.Verify(json)' to 'Verify(json)' should be confirmed as intentional, as it alters the API call pattern. If this change is designed to utilize a new verification mechanism, consider adding a brief comment for clarity.
await Verify(json)
I think this change should also be applied for GetDocument, but I am not sure. |
Where do you mean exactly? |
It's not in the filtered solution, but the dotnet tool/executable which extracts the document during build. |
Yep you're right. The code here creates a aspnetcore/src/Tools/GetDocumentInsider/src/Commands/GetDocumentCommandWorker.cs Lines 333 to 353 in faf2696
Then that gets used here: aspnetcore/src/OpenApi/src/Services/OpenApiDocumentProvider.cs Lines 58 to 60 in faf2696
I'll update that to use the invariant culture too. |
Apply the same fix to ensure invariant formatting to the OpenAPI document tool. Co-Authored-By: Sjoerd van der Meer <[email protected]>
Do you mean that the new tests pass without any of the code changes in this PR? It might be good to track the change on the Microsoft.OpenApi side that caused this. |
Yeah, they still pass without the fix being present. My hunch is that they stopped using an overload like I did try to test the theory by making an equivalent change to the |
Use invariant culture for TextWriter
Use culture-invariant
TextWriter
implementation for OpenAPI.Description
Ensure OpenAPI documents are written to a culture-invariant
TextWriter
implementation.I couldn't replicate this issue in .NET 10, I'm guessing Microsoft.OpenApi has had some internal refactoring that means that it's no longer using an overload on
TextWriter
that was hitting this (unless I messed up my test to repro it somehow).Contributes to #60628.