Skip to content

[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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

martincostello
Copy link
Member

@martincostello martincostello commented May 31, 2025

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).

Ensure OpenAPI documents are written to a culture-invariant `TextWriter` implementation.

Contributes to dotnet#60628.
@github-actions github-actions bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label May 31, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 31, 2025
@martincostello martincostello added feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels May 31, 2025
@martincostello martincostello marked this pull request as ready for review May 31, 2025 16:21
@Copilot Copilot AI review requested due to automatic review settings May 31, 2025 16:21
Copy link
Contributor

@Copilot Copilot AI left a 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)

@desjoerd
Copy link

desjoerd commented Jun 2, 2025

I think this change should also be applied for GetDocument, but I am not sure.

@martincostello
Copy link
Member Author

GetDocument

Where do you mean exactly?

@desjoerd
Copy link

desjoerd commented Jun 2, 2025

It's not in the filtered solution, but the dotnet tool/executable which extracts the document during build.

@martincostello
Copy link
Member Author

Yep you're right.

The code here creates a TextWriter (StreamWriter):

using (var writer = new StreamWriter(stream, _utf8EncodingWithoutBOM, bufferSize: 1024, leaveOpen: true))
{
var targetMethod = generateWithVersionMethod ?? generateMethod;
object[] arguments = [documentName, writer];
if (generateWithVersionMethod != null)
{
_reporter.WriteInformation(Resources.VersionedGenerateMethod);
if (Enum.TryParse<OpenApiSpecVersion>(_context.OpenApiVersion, out var version))
{
arguments = [documentName, writer, version];
}
else
{
if (!string.IsNullOrWhiteSpace(_context.OpenApiVersion))
{
_reporter.WriteWarning(Resources.FormatInvalidOpenApiVersion(_context.OpenApiVersion));
}
arguments = [documentName, writer, OpenApiSpecVersion.OpenApi3_1];
}
}
using var resultTask = (Task)InvokeMethod(targetMethod, service, arguments);

Then that gets used here:

var document = await targetDocumentService.GetOpenApiDocumentAsync(scopedService.ServiceProvider);
var jsonWriter = new OpenApiJsonWriter(writer);
await document.SerializeAsync(jsonWriter, openApiSpecVersion);

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 <2460430+desjoerd@users.noreply.github.com>
@captainsafia
Copy link
Member

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).

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.

@martincostello
Copy link
Member Author

Yeah, they still pass without the fix being present.

My hunch is that they stopped using an overload like Write(double) or similar, which would have gone through a code path that would have used the current culture formatter.

I did try to test the theory by making an equivalent change to the release/9.0 branch, but I had issues running restore.cmd for some reason. I might try that again later this week.

martincostello and others added 2 commits June 4, 2025 14:43
Change the integration test to use HTTP client so that the middleware to write the OpenAPI document is invoked, which is where the bug is.

Going through `OpenApiDocument.SerializeAsJsonAsync()` uses a different code path that already uses a culture-invariant `TextWriter` so wasn't susceptible to the bug.
Update OpenAPI range formatting to format using the target culture and update tests to write in the invariant culture.

Co-Authored-By: Sjoerd van der Meer <2460430+desjoerd@users.noreply.github.com>
@martincostello
Copy link
Member Author

martincostello commented Jun 4, 2025

@captainsafia I worked out why the repro wasn't happening and fixed it.

TL;DR - because the snapshots directly got the document and self-serialized it, it didn't go through the code path for the endpoint that was causing the issue (the tests didn't have the same bug in them).

@@ -1,9 +1,14 @@
{
"openapi": "3.0.4",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like something spooky happened with the new test data setup. These should still target OpenApi v3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I see what I've done.

There's no mechanism to pass the OpenAPI version to use on to the endpoint now that it's doing it over HTTP instead of using the service directly.

Copy link
Member Author

@martincostello martincostello Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I've fixed this now, but I'm not particularly happy with it.

Unless I've missed something, it's not possible to dynamically determine the OpenAPI version to return without encoding it into the document's name, so I've had to expose the endpoints on multiple URLs each to cover 3.0 and 3.1, which then means I need to customise ShouldInclude so the paths still get returned.

Is it worth adding a capability somehow to allow the user to dynamically compute the OpenAPI version to use at runtime? I tried to do it dynamically with IPostConfigureOptions<T>, but as it uses IOptionsMonitor<T> it only runs once and keeps the first request's preference.

Maybe something like this?

public OpenApiOptions
{
+   public Func<HttpContext, OpenApiSpecVersion?>? OpenApiVersionSelector { get; set; }
}

If you think it's worth proposing I'll spin up an API Suggestion issue (and am happy to take a hand at implementing it if approved), assuming there's still time to get it into 10.0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make OpenAPI sample endpoints version-specific.
Use `Convert.ToString()` instead of a format string.
martincostello added a commit to martincostello/aspnetcore that referenced this pull request Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc community-contribution Indicates that the PR has been added by a community member feature-openapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants