Skip to content

Do not generate Content for request parameters in OpenAPI #45404

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 1 commit into from
Dec 3, 2022

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Dec 1, 2022

Closes #45365.

OpenAPI parameters source from the route/query/header do not need to have the Content property defined as long as a Schema is generated. In this case, the schema is generated in Swashbuckle so we don't need to produce content attributes for them. This fix is a backport candidate.

cc: @marcominerva

@captainsafia captainsafia requested a review from a team as a code owner December 1, 2022 21:15
@ghost ghost 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 Dec 1, 2022
@@ -384,7 +384,6 @@ private List<OpenApiParameter> GetOpenApiParameters(MethodInfo methodInfo, Endpo
{
Name = name,
In = parameterLocation,
Content = GetOpenApiParameterContent(metadata),
Copy link
Member

@halter73 halter73 Dec 2, 2022

Choose a reason for hiding this comment

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

Is the idea that endpoint IAcceptsMetadata should only influence the RequestBody and never any Parameters?

Adding the same Content to all parameters seems bad, but it seems odd that no OpenApiParameter ever has any Content. It kind of makes sense because there's no ParameterLocation for the body and OpenApiRequestBody exists, but then what's the purpose of OpenApiParameter.Content?

Edit. I found a sample. It seems like it's if you actually want to do something like accept json in a query string. Weird.

A complex parameter using content to define serialization:

{
  "in": "query",
  "name": "coordinates",
  "content": {
    "application/json": {
      "schema": {
        "type": "object",
        "required": [
          "lat",
          "long"
        ],
        "properties": {
          "lat": {
            "type": "number"
          },
          "long": {
            "type": "number"
          }
        }
      }
    }
  }
}

https://github.com/OAI/OpenAPI-Specification/blob/157a4c81ae537ef793b2bee368bc00d88b461de8/versions/3.1.0.md#parameter-object-examples

Copy link
Member Author

Choose a reason for hiding this comment

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

Edit. I found a sample. It seems like it's if you actually want to do something like accept json in a query string. Weird.

Chatted with @baywet about this. So for 99% of scenarios, the formats that are available for parameters sourced from routes/headers/etc. are going to be described by the style and explode property (see this).

Content is really meant to describe inputs/outputs that are more suited to serialization (request body and response) but is reused here for the 1% of scenarios where people are defining some custom serialization, like the example you shared with queries that look like ?coordiantes={lat:45,long:46}. For those scenarios, it probably makes sense to have users describe that specification themselves.

Most importantly, it looks like producing this content for the scenarios where it is not needed causes a buggy path in the Swagger UI that causes hangs if you send certain types of inputs through. :/

@captainsafia captainsafia merged commit dc60cb8 into main Dec 3, 2022
@captainsafia captainsafia deleted the safia/openapi-content-fix branch December 3, 2022 18:24
@ghost ghost added this to the 8.0-preview1 milestone Dec 3, 2022
@captainsafia
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2022

Started backporting to release/7.0: https://github.com/dotnet/aspnetcore/actions/runs/3609714209

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.

[Minimal API] Incorrect OpenAPI definition of path, query and header parameters when there is a body and we're using WithOpenApi extension method
2 participants