Skip to content

Support returning OpenAPI document in YAML from MapOpenApi #58616

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 9, 2024
+43 −11
Merged

Support returning OpenAPI document in YAML from MapOpenApi #58616

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2f0c89
Support returning OpenAPI document in YAML from MapOpenApi
captainsafia Oct 24, 2024
a9b0993
Update tests and YAML content type
captainsafia Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/OpenApi/sample/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
var app = builder.Build();

app.MapOpenApi();
app.MapOpenApi("/openapi/{documentName}.yaml");
if (app.Environment.IsDevelopment())
{
app.MapSwaggerUi();
Expand Down
16 changes: 14 additions & 2 deletions src/OpenApi/src/Extensions/OpenApiEndpointRouteBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,16 @@ public static IEndpointConventionBuilder MapOpenApi(this IEndpointRouteBuilder e
using var writer = Utf8BufferTextWriter.Get(output);
try
{
document.Serialize(new OpenApiJsonWriter(writer), documentOptions.OpenApiVersion);
context.Response.ContentType = "application/json;charset=utf-8";
if (UseYaml(pattern))
{
document.Serialize(new OpenApiYamlWriter(writer), documentOptions.OpenApiVersion);
Copy link
Member

Choose a reason for hiding this comment

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

I assume OpenApiYamlWriter does smart things to guard against, e.g., the Norway Problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

OpenAPI recommends YAML 1.2 of the spec, which prohibits non-boolean string literals from being interpreted as booleans, AFAIK.

The serialization itself seems to emit it as a string correctly. For example:

[JsonConverter(typeof(JsonStringEnumConverter<Choices>))]
public enum Choices
{
    Yes,
    No
}

becomes

schemas:
    Choices:
      enum:
        - Yes
        - No

context.Response.ContentType = "application/yaml;charset=utf-8";
Copy link
Member Author

Choose a reason for hiding this comment

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

RFC 9512 declares application/yaml as the official content-type for YAML-based types. One thing I observed is that this MIME type is fairly new and most browsers still don't know how to render it inline and will fallback to downloading the YAML file to disk.

To get around this, we could use the text/plain+yaml content type to force inline rendering while still being in compliance with the MIME type spec for YAML.

Side note: This RFC is in draft which defines an official application/openapi content type that we might consider using in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Based on my limited knowledge of MIME types, application/yaml seems more correct since it is primarily for programmatic consumption, but I can definitely imagine people asking for text+yaml as a convenience. I see one of the RFC authors was from Mozilla, so hopefully built-in support isn't too far off?

Copy link

Choose a reason for hiding this comment

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

No matter the mime type (which is a non-sense to me), it should not be hardcoded and be part of the MediaTypeNames which would help people to avoid using the (more logic) text/yaml.

}
else
{
document.Serialize(new OpenApiJsonWriter(writer), documentOptions.OpenApiVersion);
Copy link
Member

Choose a reason for hiding this comment

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

Are people going to be surprised when openapi.toml returns JSON, rather than failing?

Copy link

Choose a reason for hiding this comment

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

Maybe give more control for the user, passing all values in params, and give some magic extension .json or .yaml in the end of path if exists or not or wrong will be removed and set the right extension, for content type / mime type pass in the last param like mimeType: | default value like pattern: "openapi/v1" format: JSON mimeType: "application/json" ( 0-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.

Are people going to be surprised when openapi.toml returns JSON, rather than failing?

Depends on user expectation and the type of experience that we want to provide. TOML isn't a supported OpenAPI document format so we could decide to throw if we encounter an extension and it's not associated with the supported set of formats.

  • Route with extension ending in .yaml or yml produces an OpenAPI document in YAML format.
  • Route with extension ending in .json produces an OpenAPI document in JSON format.
  • Route with extension not ending in any of the above produces an error.
  • Route with no extension produces an OpenAPI document in JSON format.

With this behavior, #3 would be a breaking change between .NET 9 and .NET 10 but I think we can live with it.

Maybe give more control for the user, passing all values in params, and give some magic extension .json or .yaml in the end of path if exists or not or wrong will be removed and set the right extension, for content type / mime type pass in the last param like mimeType: | default value like pattern: "openapi/v1" format: JSON mimeType: "application/json" ( 0-0)/

Yeah, I considered this option as well but opted to be more conservative in the amount of new API that was introduced. It also feels that given there are only two formats supported currently, we might want to support a simpler API for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opted to avoid the breaking change for now. We've got runaway to revisit this if needed.

context.Response.ContentType = "application/json;charset=utf-8";
}
await context.Response.BodyWriter.WriteAsync(output.ToArray(), context.RequestAborted);
await context.Response.BodyWriter.FlushAsync(context.RequestAborted);
}
Expand All @@ -63,4 +71,8 @@ public static IEndpointConventionBuilder MapOpenApi(this IEndpointRouteBuilder e
}
}).ExcludeFromDescription();
}

private static bool UseYaml(string pattern) =>
pattern.EndsWith(".yaml", StringComparison.OrdinalIgnoreCase) ||
pattern.EndsWith(".yml", StringComparison.OrdinalIgnoreCase);
}
28 changes: 19 additions & 9 deletions ...crosoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiEndpointRouteBuilderExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,13 @@ public void MapOpenApi_ReturnsEndpointConventionBuilder()
Assert.IsAssignableFrom<IEndpointConventionBuilder>(returnedBuilder);
}

[Fact]
public void MapOpenApi_SupportsCustomizingPath()
[Theory]
Copy link
Member

@martincostello martincostello Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do any of these tests validate it is actually YAML? They check the content type and that a OpenAPI parser can parse the content, but if you replaced the writer implementation in the YAML block with the JSON one would they still pass?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it would still pass if the writer used was a JSON one. Since JSON is valid YAML, if we wanted to verify that the emitted content was actually YAML we'd have to perhaps do some string-based check to see if it contained the correct starting characters compared to a JSON output?

Copy link
Member

Choose a reason for hiding this comment

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

TIL JSON is YML, but that still feels wrong to me that the tests wouldn't detect the content not being the requested format.

[InlineData("/custom/{documentName}/openapi.json")]
[InlineData("/custom/{documentName}/openapi.yaml")]
[InlineData("/custom/{documentName}/openapi.yml")]
public void MapOpenApi_SupportsCustomizingPath(string expectedPath)
{
// Arrange
var expectedPath = "/custom/{documentName}/openapi.json";
var serviceProvider = CreateServiceProvider();
var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider));

Expand Down Expand Up @@ -72,13 +74,16 @@ public async Task MapOpenApi_ReturnsRenderedDocument()
});
}

[Fact]
public async Task MapOpenApi_ReturnsDefaultDocumentIfNoNameProvided()
[Theory]
[InlineData("/openapi.json", "application/json;charset=utf-8")]
[InlineData("/openapi.yaml", "application/yaml;charset=utf-8")]
[InlineData("/openapi.yml", "application/yaml;charset=utf-8")]
public async Task MapOpenApi_ReturnsDefaultDocumentIfNoNameProvided(string expectedPath, string expectedContentType)
{
// Arrange
var serviceProvider = CreateServiceProvider();
var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider));
builder.MapOpenApi("/openapi.json");
builder.MapOpenApi(expectedPath);
var context = new DefaultHttpContext();
var responseBodyStream = new MemoryStream();
context.Response.Body = responseBodyStream;
Expand All @@ -91,6 +96,7 @@ public async Task MapOpenApi_ReturnsDefaultDocumentIfNoNameProvided()

// Assert
Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode);
Assert.Equal(expectedContentType, context.Response.ContentType);
ValidateOpenApiDocument(responseBodyStream, document =>
{
Assert.Equal("OpenApiEndpointRouteBuilderExtensionsTests | v1", document.Info.Title);
Expand Down Expand Up @@ -121,16 +127,19 @@ public async Task MapOpenApi_Returns404ForUnresolvedDocument()
Assert.Equal("No OpenAPI document with the name 'v2' was found.", Encoding.UTF8.GetString(responseBodyStream.ToArray()));
}

[Fact]
public async Task MapOpenApi_ReturnsDocumentIfNameProvidedInQuery()
[Theory]
[InlineData("/openapi.json", "application/json;charset=utf-8")]
[InlineData("/openapi.yaml", "application/yaml;charset=utf-8")]
[InlineData("/openapi.yml", "application/yaml;charset=utf-8")]
public async Task MapOpenApi_ReturnsDocumentIfNameProvidedInQuery(string expectedPath, string expectedContentType)
{
// Arrange
var documentName = "v2";
var hostEnvironment = new HostEnvironment() { ApplicationName = nameof(OpenApiEndpointRouteBuilderExtensionsTests) };
var serviceProviderIsService = new ServiceProviderIsService();
var serviceProvider = CreateServiceProvider(documentName);
var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider));
builder.MapOpenApi("/openapi.json");
builder.MapOpenApi(expectedPath);
var context = new DefaultHttpContext();
var responseBodyStream = new MemoryStream();
context.Response.Body = responseBodyStream;
Expand All @@ -144,6 +153,7 @@ public async Task MapOpenApi_ReturnsDocumentIfNameProvidedInQuery()

// Assert
Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode);
Assert.Equal(expectedContentType, context.Response.ContentType);
ValidateOpenApiDocument(responseBodyStream, document =>
{
Assert.Equal($"OpenApiEndpointRouteBuilderExtensionsTests | {documentName}", document.Info.Title);
Expand Down
Loading