Skip to content

ProblemHttpResult doesn't show in generated OpenApi document #52424

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

Open
1 task done
ilmax opened this issue Nov 28, 2023 · 4 comments
Open
1 task done

ProblemHttpResult doesn't show in generated OpenApi document #52424

ilmax opened this issue Nov 28, 2023 · 4 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi
Milestone

Comments

@ilmax
Copy link
Contributor

ilmax commented Nov 28, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

It seems that ProblemHttpResult doesn't implement the IEndpointMetadataProvider interface hence, a method like the following:

endpoints.MapGet("/", async Task<Results<Ok<GetClientsResponse>, ProblemHttpResult>>([FromServices] IRequiredActor<GetClientsActor> actor) =>
{
    var response = await /* Get clients from somewhere somehow.. Response is a poor man union type of IEnumerable<Client> | Error*/
      
    return response.Map<Results<Ok<GetClientsResponse>, ProblemHttpResult>>(
        clients => TypedResults.Ok(clients),
        error => TypedResults.Problem(detail: error.Reason, instance: error.Code, statusCode: StatusCodes.Status400BadRequest)
    );
});

Doesn't produce the expected OpenApi response types, only the 200 response ends up in the responses section of the document.

If I'm not reading the documentation wrong, I'd expect the code above to produce 2 responses, a 200 and a 400. am I wrong?

Expected Behavior

The problem details get's correctly added on the endpoint metadata when using Result<Something, ProblemHttpResult> and it shows up in the swagger UI

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

net8.0

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot 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 Nov 28, 2023
@captainsafia captainsafia added feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Nov 28, 2023
@captainsafia
Copy link
Member

@ilmax Thanks for reporting this issue! As I recall, at the moment this is an intentional pattern. Unlike ValidationProblemDetails, which can default to a 400 status code. ProblemDetails does not have the same sane default. It could be 500, but it could also be 401 or 505 or 429.

In the past, we rejected an API proposal that attempted to solve this by introducing specific overloads for each status code (#47705) which is not ideal. The proposal indicates that there is a class of users interested in non-500 status codes.

I'm open to changing this given we get strong feedback that 500 is the correct default and won't cause surprises. I'll put this is the backlog and see if more folks upvote this.

@captainsafia captainsafia added this to the Backlog milestone Nov 29, 2023
@ghost
Copy link

ghost commented Nov 29, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@marinasundstrom
Copy link

marinasundstrom commented Dec 6, 2023

I was surprised by this yesterday. Had to add .ProducesProblem(StatusCode…) for it to show up.

It makes sense since ASP.NET Core can’t know what status code it is. Hence why you annotate.

But at least there could be a generated default for 500 if you have enabled problem details for unhandled exceptions etc.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@linde12
Copy link

linde12 commented Jun 17, 2024

This is very confusing as a relatively green .NET developer. There are so many ways of doing the same thing. With builder pattern (.Produces), with annotations (ASP.NET MVC style?) and with generic return types (TypedResults).

I really liked the TypedResults concept where i could type whatever i return as generic arguments, e.g. Results<Ok<UserResponse>, BadRequest<ErrorResponse>> but when switching to ProblemHttpResult i now have to revert to the "old" and less cohesive (in my case, where each endpoint is a static method) way of annotating a response by imperatively adding .ProducesProblem(...) where i register the route (which is very easy to forget)

I expected the ProblemHttpResult to take a generic argument, just like e.g. BadRequest does for the body shape, but it does not. I mean like this:

public static Results<Ok<UserCreatedResponse>, ProblemHttpResult<BadRequest>> CreateUser(...)

Again, new to C# and .NET world so i'm not sure if it is possible to have const generics in C#, but being able to do ProblemHttpResult<400> (or using a const integer instead of hardcoded 400) would be really nice.

It is not the end of the world, having to add .ProducesProblem(400), but as i mentioned above some cohesion is lost and it is really easy to forget since it is not required/doesn't cause any errors if you forget to add it.

While we're at it, i'm also curious how we would type multiple kinds of errors (e.g. 400, 401, 500) using TypedResults - Results only takes 2 generic arguments (one for success and one for error) so how would i type a endpoint that returns 200, 400 and 402 with different bodies? Is the builder pattern or annotations the only escape hatch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi
Projects
None yet
Development

No branches or pull requests

6 participants
@marinasundstrom @captainsafia @linde12 @ilmax @wtgodbe and others