Incorrect OpenAPI schema generated for implicit services/special types/parseables

[martincostello]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

I'm working on a talk I'll be giving soon on new features in ASP.NET Core 7 and Minimal APIs, and in the process of updating my sample application for RC2 I've noticed a number of issues with the OpenAPI schema generated for various endpoints in the sample.

1. Using implicit service resolution for an MVC controller renders the service as an object in the OpenAPI schema and as a query string parameter.

2. Using TryParse() support for parameters for an MVC controller has a similar issue, and renders the query string parameter as an object mirroring the C# model, rather than as a string.

3. Using the support for Stream and PipeReader to consume the request body shows both Stream and PipeReader in the schema.

Expected Behavior

1. MyService is not shown as an endpoint parameter and is not included in the OpenAPI schemas.
2. The name parameter is shown as a simple string and Name is not included in the OpenAPI schemas.
3. Stream and PipeReader are not included in the OpenAPI schemas.

Steps To Reproduce

1. Clone the martincostello/aspnet-core-7-samples repository.
2. Build and run the application.
3. View the rendered OpenAPI schema with Swagger UI at https://localhost:5001/swagger-ui/index.html.

Exceptions (if any)

None.

.NET Version

7.0.100-rc.2.22477.23

Anything else?

> dotnet --info
.NET SDK:
 Version:   7.0.100-rc.2.22477.23
 Commit:    0a5360315a

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22000
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\7.0.100-rc.2.22477.23\

Host:
  Version:      7.0.0-rc.2.22472.3
  Architecture: x64
  Commit:       550605cc93

[captainsafia]

Great sample app! Thanks for putting it together.

Using implicit service resolution for an MVC controller renders the service as an object in the OpenAPI schema and as a query string parameter.

It looks like the ApiExplorer is producing an ApiParameterDescription for the service parameter and it ends up being treated as a query parameter by the fallback logic in Swashbuckle. I'm not totally familiar with MVC's model binding logic but my suspicion is that we'll have to update this to not generate a parameter description for parameters that might be implicit services. Paging @brunolins16 for model binding insights.

Using TryParse() support for parameters for an MVC controller has a similar issue, and renders the query string parameter as an object mirroring the C# model, rather than as a string.

I suspect the same thing happening for services is happening here. We need to update the pseudo-model binding logic in the ApiExplorer to match the new behavior in MVC.

Using the support for Stream and PipeReader to consume the request body shows both Stream and PipeReader in the schema.

We're using Swahsbuckle's schema generator here (see this PR). It looks like the schema generator does not support generating schemes for those two types yet...

[brunolins16]

1. Using implicit service resolution for an MVC controller renders the service as an object in the OpenAPI schema and as a query string parameter.

@martincostello implicit service resolution, as all other implicit source resolution, are enable by default for API Controllers only, so you need to add the ApiControllerAttribute and this should be fixed

[brunolins16]

I suspect the same thing happening for services is happening here. We need to update the pseudo-model binding logic in the ApiExplorer to match the new behavior in MVC.

A type with TryParse is consider Simple Type and Metadata.IsComplexType should be false and report it correctly. Let me take a further look at how this code works to see if I can understand what I happening.

[martincostello]

Confirmed that adding [ApiController] fixed item 1.

[martincostello]

@captainsafia Do you think this is the right fix for item 3, or have I got the best representation for the data here completely wrong and it just "fixes it" by accident?

https://github.com/domaindrivendev/Swashbuckle.AspNetCore/compare/master...martincostello:Swashbuckle.AspNetCore:handle-stream-and-pipereader

Testing locally, Stream and PipeReader stop being returned in the schema with these changes.

[martincostello]

@brunolins16 I had a quick look at the code. I didn't fully dig through and understand it all, but I wondered if this line needs changing to use string for the type if IsParseableType is true?

https://github.com/dotnet/aspnetcore/blob/8c19ce1bd7b91b72a4b8a5c96183b0258221d99e/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs#L658

- UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType;
+ UnderlyingOrModelType = IsParseableType ? typeof(string) : Nullable.GetUnderlyingType(ModelType) ?? ModelType;

[brunolins16]

@brunolins16 I had a quick look at the code. I didn't fully dig through and understand it all, but I wondered if this line needs changing to use string for the type if IsParseableType is true?

https://github.com/dotnet/aspnetcore/blob/8c19ce1bd7b91b72a4b8a5c96183b0258221d99e/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs#L658

- UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType;
+ UnderlyingOrModelType = IsParseableType ? typeof(string) : Nullable.GetUnderlyingType(ModelType) ?? ModelType;

I think is not possible to simply change it because it might affect the binding logic. However we might be able to apply the same idea in the API Explorer but we will need to think how can we differentiate from primitives (int, bool, etc.).

I need to check what we are doing for Minimal Endpoints.

BTW: Types with Type Converter have the same problem

[brunolins16]

@martincostello Maybe we can do the same as here IsPrimitive:

https://github.com/dotnet/aspnetcore/blob/dcbfb829777da26078b31bace83736e6f3bf3295/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs#L291-L292

I will do some experiments and create a PR if it works. Thanks for reporting this bug.

[brunolins16]

I give it a try but, if I got it correctly, IsPrimitive is based on the CorElementType that means some types will not be covered (that probably make sense for OpenAPI), eg. decimal or guid, and will be reported as string. That is probably the cause of https://github.com/dotnet/aspnetcore/issues/39886

[captainsafia]

@captainsafia Do you think this is the right fix for item 3, or have I got the best representation for the data here completely wrong and it just "fixes it" by accident?

domaindrivendev/Swashbuckle.AspNetCore@master...martincostello:Swashbuckle.AspNetCore:handle-stream-and-pipereader

Testing locally, Stream and PipeReader stop being returned in the schema with these changes.

I think that should be sufficient given the version of OpenAPI schema that Swashbuckle is currently targeting. I went down a bit of a rabbit hole trying to figure out what the best type/format to use is given the different OAS versions but this seems to be appropriate for v3.0.

[martincostello]

Great, thanks for looking @captainsafia - I'll open a PR against Swashbuckle with that change shortly 👍

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.