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.

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

Do not generate Content for request parameters in OpenAPI #45404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 2 additions & 18 deletions src/OpenApi/src/OpenApiGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ private OpenApiOperation GetOperation(string httpMethod, MethodInfo methodInfo,
Summary = metadata.GetMetadata<IEndpointSummaryMetadata>()?.Summary,
Description = metadata.GetMetadata<IEndpointDescriptionMetadata>()?.Description,
Tags = GetOperationTags(methodInfo, metadata),
Parameters = GetOpenApiParameters(methodInfo, metadata, pattern, disableInferredBody),
Parameters = GetOpenApiParameters(methodInfo, pattern, disableInferredBody),
RequestBody = GetOpenApiRequestBody(methodInfo, metadata, pattern),
Responses = GetOpenApiResponses(methodInfo, metadata)
};
Expand Down Expand Up @@ -356,7 +356,7 @@ private List<OpenApiTag> GetOperationTags(MethodInfo methodInfo, EndpointMetadat
return new List<OpenApiTag>() { new OpenApiTag() { Name = controllerName } };
}

private List<OpenApiParameter> GetOpenApiParameters(MethodInfo methodInfo, EndpointMetadataCollection metadata, RoutePattern pattern, bool disableInferredBody)
private List<OpenApiParameter> GetOpenApiParameters(MethodInfo methodInfo, RoutePattern pattern, bool disableInferredBody)
{
var parameters = PropertyAsParameterInfo.Flatten(methodInfo.GetParameters(), ParameterBindingMethodCache);
var openApiParameters = new List<OpenApiParameter>();
Expand Down Expand Up @@ -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 •
edited
Loading

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. :/

Required = !isOptional

};
Expand All @@ -394,21 +393,6 @@ private List<OpenApiParameter> GetOpenApiParameters(MethodInfo methodInfo, Endpo
return openApiParameters;
}

private static Dictionary<string, OpenApiMediaType> GetOpenApiParameterContent(EndpointMetadataCollection metadata)
{
var openApiParameterContent = new Dictionary<string, OpenApiMediaType>();
var acceptsMetadata = metadata.GetMetadata<IAcceptsMetadata>();
if (acceptsMetadata is not null)
{
foreach (var contentType in acceptsMetadata.ContentTypes)
{
openApiParameterContent.Add(contentType, new OpenApiMediaType());
}
}

return openApiParameterContent;
}

private (bool isBodyOrForm, ParameterLocation? locatedIn) GetOpenApiParameterLocation(ParameterInfo parameter, RoutePattern pattern, bool disableInferredBody)
{
var attributes = parameter.GetCustomAttributes();
Expand Down
12 changes: 12 additions & 0 deletions src/OpenApi/test/OpenApiGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ static void AssertPathParameter(OpenApiOperation operation)
{
var param = Assert.Single(operation.Parameters);
Assert.Equal(ParameterLocation.Path, param.In);
Assert.Empty(param.Content);
}

AssertPathParameter(GetOpenApiOperation((int foo) => { }, "/{foo}"));
Expand All @@ -335,6 +336,7 @@ static void AssertPathParameter(OpenApiOperation operation)
{
var param = Assert.Single(operation.Parameters);
Assert.Equal(ParameterLocation.Path, param.In);
Assert.Empty(param.Content);
}
AssertPathParameter(GetOpenApiOperation((TryParseStringRecord foo) => { }, pattern: "/{foo}"));
}
Expand All @@ -346,6 +348,7 @@ static void AssertPathParameter(OpenApiOperation operation)
{
var param = Assert.Single(operation.Parameters);
Assert.Equal(ParameterLocation.Path, param.In);
Assert.Empty(param.Content);
}

AssertPathParameter(GetOpenApiOperation((int? foo) => { }, "/{foo}"));
Expand All @@ -359,6 +362,7 @@ static void AssertPathParameter(OpenApiOperation operation)
{
var param = Assert.Single(operation.Parameters);
Assert.Equal(ParameterLocation.Path, param.In);
Assert.Empty(param.Content);
}
AssertPathParameter(GetOpenApiOperation((TryParseStringRecordStruct foo) => { }, pattern: "/{foo}"));
}
Expand All @@ -370,6 +374,7 @@ static void AssertQueryParameter(OpenApiOperation operation, string type)
{
var param = Assert.Single(operation.Parameters);
Assert.Equal(ParameterLocation.Query, param.In);
Assert.Empty(param.Content);
}

AssertQueryParameter(GetOpenApiOperation((int foo) => { }, "/"), "integer");
Expand All @@ -388,6 +393,7 @@ public void AddsFromHeaderParameterAsHeader()
var param = Assert.Single(operation.Parameters);

Assert.Equal(ParameterLocation.Header, param.In);
Assert.Empty(param.Content);
}

[Fact]
Expand Down Expand Up @@ -430,11 +436,13 @@ public void AddsMultipleParameters()
Assert.Equal("foo", fooParam.Name);
Assert.Equal(ParameterLocation.Path, fooParam.In);
Assert.True(fooParam.Required);
Assert.Empty(fooParam.Content);

var barParam = operation.Parameters[1];
Assert.Equal("bar", barParam.Name);
Assert.Equal(ParameterLocation.Query, barParam.In);
Assert.True(barParam.Required);
Assert.Empty(barParam.Content);

var fromBodyParam = operation.RequestBody;
var fromBodyContent = Assert.Single(fromBodyParam.Content);
Expand All @@ -455,12 +463,14 @@ static void AssertParameters(OpenApiOperation operation, string capturedName = "
Assert.Equal(capturedName, param.Name);
Assert.Equal(ParameterLocation.Path, param.In);
Assert.True(param.Required);
Assert.Empty(param.Content);
},
param =>
{
Assert.Equal("Bar", param.Name);
Assert.Equal(ParameterLocation.Query, param.In);
Assert.True(param.Required);
Assert.Empty(param.Content);
}
);
}
Expand All @@ -485,11 +495,13 @@ public void TestParameterIsRequired()
Assert.Equal("foo", fooParam.Name);
Assert.Equal(ParameterLocation.Path, fooParam.In);
Assert.True(fooParam.Required);
Assert.Empty(fooParam.Content);

var barParam = operation.Parameters[1];
Assert.Equal("bar", barParam.Name);
Assert.Equal(ParameterLocation.Query, barParam.In);
Assert.False(barParam.Required);
Assert.Empty(barParam.Content);
}

[Fact]
Expand Down