Fix `ProducesResponseType` on `IResult` handling in `OpenApiGenerator` and `ApiResponseTypeProvider`

[nicro950]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

It looks like minimal apis adds a ProducesResponseTypeMetadata with status 200 automatically to some Map*(Get/Put/Post/...) configurations. From my testing it looks like it happens as long as the Map* is given a delegate which returns some data, including Task or Task_T. In some cases this might be what you want, but if the endpoint, for instance, only returns a 201 Created, then the endpoint metadata is reporting that it both return a 200 and 201(As long as you also adds the Produces(201) metadata), which it does not do. This became a problem for us further down the line, when we tried to generate the corresponding OpenApi document for the metadata information, which added the extra response code, and set the response type to an empty object.

As seen in the image, both endpoint which returns a Task, have the extra metadata, but the last one returning only void does not. I also tried returning a string, and then it added the 200, but that would be the correct behavior in that context. The 200 response was removed when returning an Result.NoContent() directly without having it in a Task.

Expected Behavior

Either that ProducesResponseTypeMetadata is never added automatically or that it is only automatically added if the endpoint is guaranteed to have a scenario which results in a 200 status code. Looks like the intention was that it gets the 200 if the endpoint returns a type, but it should not get the 200 if it returns void, IResult, Task, or Task_IResult but the last two was not included.

Steps To Reproduce

1. Create a new asp.net core application with minimal apis
2. Add the following endpoints:

        app.MapPost("/test", () =>
            {
                return Task.CompletedTask;
            });

        app.MapPost("/test2", () =>
            {
                return Task.FromResult(Results.NoContent);
            })
            .Produces(201);

        app.MapPost("/test3", () =>
            {
            });

3. The first two will have the extra metadata added to them.

Exceptions (if any)

No response

.NET Version

9.0.100-rc.1.24452.12

Anything else?

No response

[captainsafia]

@nicro950 Thanks for filing this issue!

Some thoughts on the scenarios that you have shared here.

() => Task.CompletedTask returning a 200 description is expected behavior. By default, all void-returning methods will have 200 OK description associated with them because that matches the framework behavior here. You'll observe that your last test3 example works the same way.

() => Task.FromResult(Results.NoContent) is a bit of an interesting scenario. In this particularly case, you're not returning a NoContent object, you're returning the function that returns a NoContent object. I think what you're probably trying to do here is something like () => Task.FromResult(TypedResults.Created(...)).

Let me know if this response is helpful or if you're seeing other issues with this!

[nicro950]

@captainsafia I can agree with the Task automatically adding 200, but then, should not the 3. option also add the 200 ProducesResponseTypeMetadata, which returns void?

The code I wrote here was the simplest form of our problem, showing only the tings resulting in the extra ProducesResponseTypeMetadata being added to the metadata. Our problem is spesific for file upload which returns NoContent after the file is uploaded, and is more closely related to the following code:

        app.MapPost("/file", async (IFormFile file) =>
        {
            await FileUpload(file);
            return Results.NoContent;
        })
        .Produces(StatusCodes.Status204NoContent);

where it gets both the 200 and 204, even if the endpoint can never return 200.

[martincostello]

return Results.NoContent; is still returning the function itself, not its result.

Try:

- return Results.NoContent;
+ return Results.NoContent();

[nicro950]

@martincostello Sorry about that, but it does not look like it is related to the problem even if you return the object result instead of, it still adds both of the statuses, since our original code do work the proper way, but by trying to remove the most, resulted in in dissaparing:

[nicro950]

@martincostello @captainsafia here should be a proper example, showing all the types involved (so i can get them correct myself as well 😅). As we can see, both the 200 and 204 are added to the metadata list. Sorry about the bad examples.

[captainsafia]

As we can see, both the 200 and 204 are added to the metadata list. Sorry about the bad examples.

I think the problem that you are seeing here is related to the fact that you are using Results.NoContent instead of TypedResults.NoContent. The Results.NoContent type doesn't produce any metadata about what it produces to the API. In lieu of this, minimal APIs will make the same assumption that it makes about void-returning types: that they should return 200 OK.

Updating your API to use TypedResults.NoContent should get you the right behavior here.

[nicro950]

Using TypedResult, and it still adds the 200 return status:

That logic would also break the moment you return multiple statuses if you have some not found check, or conflict check or something like that, and the function needs to fallback to using IResult as the common return type.

[chrisoverzero]

That logic would also break the moment you return multiple statuses […]

This is what the composite Results type is meant for when used as a return type.

[nicro950]

@chrisoverzero Nice, did not now about that one, since we have mostly abstracted that layer away, but it still adds the 200 in that case 😅

Personally, I just feel like if a IResult is returned (Either IResult or Task_IResult), then it should not make any assumption about what HTTP status is returned from the result, and it should be up to the user to add it to the metadata manually.

[captainsafia]

@chrisoverzero Nice, did not now about that one, since we have mostly abstracted that layer away, but it still adds the 200 in that case 😅

Personally, I just feel like if a IResult is returned (Either IResult or Task_IResult), then it should not make any assumption about what HTTP status is returned from the result, and it should be up to the user to add it to the metadata manually.

Minor nit: the typical pattern is to use the Results<> type as the return for the method instead of casting the given case.

The ProducesResponseTypeMetadata that gets added here is a result of the recent work that we've done to support native AoT for minimal APIs + OpenAPI. Essentially, we use that metadata as a replacement for dynamic reflection for resolving the return type from methods that return awaitable types (like yours).

When this metadata is processed by the ApiExplorer layer, it gets filters out from the ApiResponse objects that are constructed. IMO, for realizing what response types are actually produced by an endpoint API explorer (and not endpoint metadata) should be the source of truth.

The original proposal for resolving this bug proposed using a new metadata type to capture await able unwrapping but we opted to use ProducesResponseType since ti already exists. We can consider adding a property to indicate that it is auto-generated.

Is there a particular library that is querying this metadata and running into issues?

[nicro950]

Minor nit: the typical pattern is to use the Results<> type as the return for the method instead of casting the given case.

Yeah, I know, it was just the easiest way to force the return type resolution to chose the correct type, instead of having to create a separate function.

The ProducesResponseTypeMetadata that gets added here is a result of the recent work that we've done to support native AoT for minimal APIs + OpenAPI. Essentially, we use that metadata as a replacement for dynamic reflection for resolving the return type from methods that return awaitable types (like yours).

Aha, that make sense, I went in to take a look at the code, and the code worked as i expected before it was changed in a pr which had something to do with Native AoT, which I have little knowledge about, so I trust that you do the right thing there.

The reason I made the bug in that way in the first place was that the metadata was the first place i looked when open api generator started generating 200 statuses with result type void, in the form of SwaggerResponse<void> which did not make sense at all. Then afterwards I manually removed the 200 from the endpoint metadata and the problem was resolved in our code.

So a little more context, we are using NSwag to generate both the OpenAPI document, as well as C# clients from the OpenAPI document afterwards. I went digging into the NSwag source code with stepthrough debugging and it looks like NSwag uses the Microsoft.OpenApi.Models.OpenApiOperation added from .WithOpenApi() with the Microsoft.AspNetCore.OpenApi package with version 9.0.0-rc.1.24452.1.

While inspecting the code it looks like it does not filter out the 200 status code:

Which like you mentioned that ApiExplorer does:

Not quite sure if this issue should be edited and renamed, or moved somewhere else, or created again in another repo, or if there is an intention to change status code resolution of the .WithOpenApi()?

The original proposal for resolving this bug proposed using a new metadata type to capture await able unwrapping but we opted to use ProducesResponseType since ti already exists. We can consider adding a property to indicate that it is auto-generated.

I was at least confused by the automatically added status code, which i never added, and probably some other code out there which might depend on the minimal api metadata. But I think that you have a larger knowledge context on the matter, so would be hard for me to know about all the nuances with choosing one option or another here.