Skip to content

Spruce up OpenApiSchema generation #41468

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

Merged
merged 3 commits into from
May 10, 2022

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented May 2, 2022

Part of #41246

We're pursuing more fully-fledged schema generation support via microsoft/OpenAPI.NET#836 but this fixes up a few things with the current logic:

  • Supports more of the primitive .NET types
  • Sets the Nullable attribute on types using NullabilityContext info
  • Sets the format for primitive types where it is supported

@mkArtakMSFT mkArtakMSFT added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label May 2, 2022
@captainsafia captainsafia requested a review from a team May 3, 2022 01:01
@captainsafia captainsafia marked this pull request as ready for review May 3, 2022 01:01
@captainsafia captainsafia requested a review from Pilchie as a code owner May 3, 2022 01:01
@captainsafia captainsafia removed the request for review from Pilchie May 3, 2022 01:02
@captainsafia captainsafia requested a review from JamesNK May 3, 2022 17:02
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Generally looks good. Do we need to add a couple test cases for Dictionary, Time* types, and IEnumerable?

The PR description says it closes the issue, the issue talks about handling TryParse and default values. Does that still need to be tracked?

@captainsafia
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@captainsafia
Copy link
Member Author

captainsafia commented May 10, 2022

The PR description says it closes the issue,

I changed the issue text here. There's actually potentially other things that will happen in this schema generation area.

The PR description says it closes the issue, the issue talks about handling TryParse and default values. Does that still need to be tracked?

Their omission is intentional here and mainly has to do with the fact that Stuff's Complicated ™️ .

  • Handling TryParse requires that we be able to create a schema for a potentially complex object, such as a TryParse that targets a record Location(int latitude, int longitude). In that case, we need to be able to construct a schema for an object containing latitude and longitude properties. There's a catch here, however, which is that we still want to be allow users to treat this types as string parameters to their request (so the interactive experience in Swagger UI) works.
  • The Default in the OpenApiSchema must implement the OpenApiAny type which means that any default value you pass to it has to either implement OpenApiAny or map to it, which requires being able to generate a schema for the type.

Edit: I'm also noodling on an alternative approach for this problem which involves "merging" with the Swashbuckle-generated schema instead of overriding it altogether (unless the user explicitly did so in their WithOpenApi invocation).

@captainsafia captainsafia merged commit 52c11c3 into dotnet:main May 10, 2022
@ghost ghost added this to the 7.0-preview5 milestone May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants