Skip to content
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

Auto revolve open-api docs from JsonHttpResult<TValue> type #47630

Closed
mehdihadeli opened this issue Apr 10, 2023 · 10 comments
Closed

Auto revolve open-api docs from JsonHttpResult<TValue> type #47630

mehdihadeli opened this issue Apr 10, 2023 · 10 comments
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update.

Comments

@mehdihadeli
Copy link

mehdihadeli commented Apr 10, 2023

Hi,
Currently .net core 7 doesn't support, auto revolve open-api docs from JsonResult type because it doesn't implement IEndpointMetadataProvider. Could you consider this in the next version?

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 10, 2023
@captainsafia captainsafia added feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Apr 10, 2023
@captainsafia
Copy link
Member

The JsonResult type is an MVC-specific result type (as in: it's an implementation of IActionResult). Built-in IEndpointMetadataProvider implementations are applicable on the IResult types. The IResult analog for this type is the JsonHttpResult type which does contain an IEndpointMetadataProvider implementation. I'm inclined to not provide IEndpointMetadataProvider implementations for the ActionResult because it would be usless at the moment given #44988 the metadata wouldn't be processed anyways if you are in an MVC controller.

One thing to note here is that the JsonResult type is not generic so you'd either have to annotate it as an open-ended Dictionary type in Swagger or provide your own definitions if you know what the type is.

What kind of API are you using JsonResult in?

@captainsafia captainsafia added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 10, 2023
@ghost
Copy link

ghost commented Apr 10, 2023

Hi @mehdihadeli. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@mehdihadeli
Copy link
Author

mehdihadeli commented Apr 10, 2023

Thanks for your response. That was my mistake, I mean JsonHttpResult<TValue> and it implements IResult for using in minimal apis, but it doesn't implement IEndpointMetadataProvider so we don't have built-in open-api support. This maybe useful for someone like me. I use that in a generic endpoint like this, it will be great if support open-api with JsonHttpResult<TValue> response type automatically:

    public static RouteHandlerBuilder MapCommandEndpoint<TRequest, TResponse, TCommand, TCommandResult>(
        this IEndpointRouteBuilder builder,
        string pattern,
        int statusCode,
        Func<TRequest, TCommand>? mapRequestToCommand = null,
        Func<TCommandResult, TResponse>? mapCommandResultToResponse = null
    )
    where TRequest : class
    where TResponse : class
    where TCommandResult : class
    where TCommand : IRequest<TCommandResult>
    {
        return builder.MapPost(pattern, Handle);

        async Task<JsonHttpResult<TResponse>> Handle([AsParameters] HttpCommand<TRequest> requestParameters)
        {
            var (request, context, mediator, mapper, cancellationToken) = requestParameters;

            var command = mapRequestToCommand is not null ?
                              mapRequestToCommand(request) :
                              mapper.Map<TCommand>(request);
            var res = await mediator.Send(command, cancellationToken);

            var response = mapCommandResultToResponse is not null ?
                               mapCommandResultToResponse(res) :
                               mapper.Map<TResponse>(res);

            // https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/responses
            // https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/openapi?view=aspnetcore-7.0#multiple-response-types
            return TypedResults.Json(response, statusCode: statusCode);
        }
    }

@ghost ghost added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Apr 10, 2023
@mehdihadeli mehdihadeli changed the title Auto revolve open-api docs from JsonResult type Auto revolve open-api docs from JsonHttpResult<TValue> type Apr 10, 2023
@mitchdenny
Copy link
Member

mitchdenny commented Apr 11, 2023

@mehdihadeli what were you hoping for in the current situation. You are returning a JsonHttpResult from the method with the status code being dynamically set at runtime. The Swagger document produced includes the endpoint but can't provide more detail about the status codes because that information isn't available at that point.

You might want to look at mapping the range of possible status codes to a strong type like the following so that the field of possible results can be inferred:

app.MapGet("/", async Task<Results<Ok<string>, NotFound<string>>> (string response) => {
    return response switch
    {
        "200" => TypedResults.Ok<string>("foo"),
        _ => TypedResults.NotFound<string>("bar"),
    };
});

@mehdihadeli
Copy link
Author

mehdihadeli commented Apr 11, 2023

Just to generalize my response type, because in my endpoint there are multiple possibilities for the result, and putting all them in the multiple Results<,> is one approach, but I think JsonHttpResult also should work. When we want to write our response if JsonHttpResult has a StatusCode it could be translated to swagger metadata, but problem here is that JsonHttpResult doesn't implement IEndpointMetadataProvider

@mitchdenny
Copy link
Member

mitchdenny commented Apr 11, 2023

Not quite. The JsonHttpResult instance that you are returning has a status code - but this is runtime information that is present when the delegate executes. It isn't really possible to reliably determine what the value of statusCode is inside the handler.

To conceptualize this a bit better imagine if your code instead set the value of statusCode by reading a file. It isn't possible for implementor of IEndpointMetadataProvider to find out what that value is unless it actually executes the code to read the file.

By using Results<T ...> you are declaring statically in a way that open API can read what the expected range of results is.

Hope this makes sense.

@mitchdenny mitchdenny added Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. and removed Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. labels Apr 11, 2023
@ghost
Copy link

ghost commented Apr 11, 2023

Hi @mehdihadeli. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@mehdihadeli
Copy link
Author

Thanks for your explanation, so this is not possible because IEndpointMetadataProvider implemented a static PopulateMetadata and we don't have any control on the JsonHttpResult status code out there.

public static void PopulateMetadata(MethodInfo method, EndpointBuilder builder)
{
    builder.Metadata.Add(new ResponseMetadata(???, typeof(ProblemDetails)));
}

@ghost ghost added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Apr 11, 2023
@mitchdenny
Copy link
Member

Thats right. The purpose of IEndpointMetadataProvider is to look at the method that a type is returned from and based on that generate the metadata to add to the endpoint. I'll close this issue now since the current behavior is by design.

@mehdihadeli
Copy link
Author

Thanks

@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update.
Projects
None yet
Development

No branches or pull requests

3 participants