Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
captainsafia merged 2 commits intomainfromsafia/openapi-yml
Nov 9, 2024

Conversation

captainsafia
Copy link
Member

Addresses#58516.

This PR adds support for returning the OpenAPI document at runtime in the YAML format. It does this by checking the extension on the route pattern that is provided for resolving the OpenAPI document. If a user calls:

app.MapOpenApi("/openapi/{documentName}.yaml");

Then visiting the/openapi/v1.yaml route in their application will return the document in YAML format. The implementation treats routes ending in eitheryaml oryml as YAML-generating by default. All other routes will produce an OpenAPI document serialized to JSON.

As an alternative approach, we could follow the pattern proposed inthis issue and introduce an additionalOpenApiFormat argument to theMapOpenApi method. However, because the implementation assumes that the extension appears in the route pattern by default we run the risk of users inadvertently calling:

app.MapOpenApi(format:OpenApiFormat.Yaml);

Which would use the default route pattern for the document that uses a JSON file extension with the incorrect file format. I figured it was more straightforward if we define the format based on the route pattern and avoid adding another option. There is a risk that more formats get added in the future and the complexity of our checks increases but that isn't a likely eventuality at the moment.

@captainsafiacaptainsafia requested a review froma team as acode ownerOctober 24, 2024 21:49
@ghostghost added the old-area-web-frameworks-do-not-use*DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labelOct 24, 2024
if (UseYaml(pattern))
{
document.Serialize(new OpenApiYamlWriter(writer), documentOptions.OpenApiVersion);
context.Response.ContentType = "application/yaml;charset=utf-8";
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

RFC 9512 declaresapplication/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 thetext/plain+yaml content type to force inline rendering while still being in compliance with the MIME type spec for YAML.

Side note: ThisRFC is in draft which defines an officialapplication/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 fortext+yaml as a convenience. I see one of the RFC authors was from Mozilla, so hopefully built-in support isn't too far off?

@@ -29,11 +29,13 @@ public void MapOpenApi_ReturnsEndpointConventionBuilder()
Assert.IsAssignableFrom<IEndpointConventionBuilder>(returnedBuilder);
}

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

@martincostellomartincostelloOct 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
MemberAuthor

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?

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.

amcasey reacted with laugh emoji
@captainsafiacaptainsafia added area-mvcIncludes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi area-minimalIncludes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use*DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labelsOct 24, 2024
Copy link
Member

@amcaseyamcasey left a comment

Choose a reason for hiding this comment

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

Seems sensible, but might have a bug tail. Getting it in early makes sense.

if (UseYaml(pattern))
{
document.Serialize(new OpenApiYamlWriter(writer), documentOptions.OpenApiVersion);
context.Response.ContentType = "application/yaml;charset=utf-8";
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 fortext+yaml as a convenience. I see one of the RFC authors was from Mozilla, so hopefully built-in support isn't too far off?

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 assumeOpenApiYamlWriter does smart things to guard against, e.g., the Norway Problem?

Copy link
MemberAuthor

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

krishnaanaril reacted with thumbs up emoji
}
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 whenopenapi.toml returns JSON, rather than failing?

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
MemberAuthor

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 extensionand it's not associated with the supported set of formats.

  • Route with extension ending in.yaml oryml 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
MemberAuthor

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.

@dotnet-policy-servicedotnet-policy-servicebot added the pending-ci-rerunWhen assigned to a PR indicates that the CI checks should be rerun labelNov 6, 2024
@skfw-dev
Copy link

/azp run LGTM

@azure-pipelinesAzure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 58616 in repo dotnet/aspnetcore

@captainsafia
Copy link
MemberAuthor

/azp run LGTM

Thanks! I owe another commit to this branch that improves the tests based on the feedback in#58616 (comment).

I'm inclined to leave the implementation as is for now (anything that isn't YAML will produce JSON) and avoid the extra logic in around what extension is actually in the file. We're early enough in the release cycle that we can actually change this.

@captainsafiacaptainsafia removed the pending-ci-rerunWhen assigned to a PR indicates that the CI checks should be rerun labelNov 8, 2024
@captainsafiacaptainsafia merged commit3c9639f intomainNov 9, 2024
27 checks passed
@captainsafiacaptainsafia deleted the safia/openapi-yml branchNovember 9, 2024 00:47
@dotnet-policy-servicedotnet-policy-servicebot added this to the10.0-preview1 milestoneNov 9, 2024
captainsafia added a commit that referenced this pull requestFeb 11, 2025
* Support returning OpenAPI document in YAML from MapOpenApi* Update tests and YAML content type
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@martincostellomartincostellomartincostello left review comments

@skfw-devskfw-devskfw-dev left review comments

@BrennanConroyBrennanConroyBrennanConroy approved these changes

@amcaseyamcaseyamcasey approved these changes

Assignees
No one assigned
Labels
area-minimalIncludes minimal APIs, endpoint filters, parameter binding, request delegate generator etcarea-mvcIncludes: MVC, Actions and Controllers, Localization, CORS, most templatesfeature-openapi
Projects
None yet
Milestone
10.0-preview1
Development

Successfully merging this pull request may close these issues.

5 participants
@captainsafia@skfw-dev@martincostello@BrennanConroy@amcasey

[8]ページ先頭

©2009-2025 Movatter.jp