Skip to content

Support all ProblemDetail types in the multi response types for generating swagger docs MinimalApi #47623

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

Closed
mehdihadeli opened this issue Apr 10, 2023 · 15 comments
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-openapi ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. Status: Resolved

Comments

@mehdihadeli
Copy link

mehdihadeli commented Apr 10, 2023

Hi,
I want to create an api with multiple result types with using TypedResults, but seems, .net core 7 only supports ValidationProblem for generating swagger metadata automatically and ProblemHttpResult type doesn't generate any swagger metadata. Maybe we need to declare dedicated problem types like ValidationProblem for others, like InternalProbelm, UnAuthorizedProblem, ...

      async Task<Results<CreatedAtRoute<CreateProductResponse>, ProblemHttpResult>>
            Handle([AsParameters] CreateProductRequestParameters requestInput)
        {
            ///...
            return TypedResults.CreatedAtRoute(
                new CreateProductResponse(result.Id),
                "GetProductById",
                new {id = result.Id});
        }

I had to handle it manually for myself like this (we have some limitation because ProblemHttpResult is sealed class and I can't inherit from it directly, so I created a HttpProblemResultBase for replacement, also ProducesResponseTypeMetadata was internal, and I had to create a custom one):

public class HttpProblemResultBase :
    IResult,
    IStatusCodeHttpResult,
    IContentTypeHttpResult,
    IValueHttpResult,
    IEndpointMetadataProvider,
    IValueHttpResult<ProblemDetails>
{
    private readonly ProblemHttpResult _problem;

    public HttpProblemResultBase(
        int statusCode,
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null,
        IDictionary<string, object?>? extensions = null
    )
    {
        _problem = TypedResults.Problem(
            statusCode: statusCode,
            title: title,
            type: type,
            detail: detail,
            instance: instance,
            extensions: extensions);
    }

    public Task ExecuteAsync(HttpContext httpContext)
    {
        return _problem.ExecuteAsync(httpContext);
    }

    public ProblemDetails ProblemDetails => _problem.ProblemDetails;
    public virtual int? StatusCode => _problem.StatusCode;
    public string ContentType => _problem.ContentType;
    object IValueHttpResult.Value => ProblemDetails;
    public ProblemDetails Value => ProblemDetails;

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

public class InternalHttpProblemResult : HttpProblemResultBase
{
    public InternalHttpProblemResult(
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null,
        IDictionary<string, object?>? extensions = null
    ) : base(StatusCodes.Status500InternalServerError, title, type, detail, instance, extensions)
    {
    }
}

public class NotFoundHttpProblemResult : HttpProblemResultBase
{
    public NotFoundHttpProblemResult(
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null,
        IDictionary<string, object?>? extensions = null
    ) : base(StatusCodes.Status404NotFound, title, type, detail, instance, extensions)
    {
    }
}

And my route now should be for generating swagger metadata for problems automatically:

      async Task<Results<CreatedAtRoute<CreateProductResponse>, UnAuthorizedHttpProblemResult, ValidationProblem>>
            Handle([AsParameters] CreateProductRequestParameters requestInput)
        {
            ///...
            return TypedResults.CreatedAtRoute(
                new CreateProductResponse(result.Id),
                "GetProductById",
                new {id = result.Id});
        }
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 10, 2023
@mehdihadeli mehdihadeli changed the title Support all ProbelmDetail types in the multi response types for generating swagger docs Support all ProbelmDetail types in the multi response types for generating swagger docs MinimalApi Apr 10, 2023
@SteveSandersonMS SteveSandersonMS changed the title Support all ProbelmDetail types in the multi response types for generating swagger docs MinimalApi Support all ProblemDetail types in the multi response types for generating swagger docs MinimalApi Apr 10, 2023
@SteveSandersonMS SteveSandersonMS added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Apr 10, 2023
@SteveSandersonMS
Copy link
Member

Moving to area-web-frameworks. @adityamandaleeka does this look like it should go to your team?

@adityamandaleeka
Copy link
Member

Yup. cc @mitchdenny @captainsafia

@mitchdenny mitchdenny added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Apr 10, 2023
@mitchdenny
Copy link
Member

@captainsafia assigning this one to you since you are more familiar with the interplay with Swagger.

@mehdihadeli
Copy link
Author

Can I contribute for this?

@mitchdenny
Copy link
Member

@mehdihadeli is you would like to propose a change to the public API of ASP.NET Core consider submitting an issue using the following template so that we can run it through the review process.

https://github.com/dotnet/aspnetcore/issues/new?assignees=&labels=api-suggestion&template=30_api_proposal.md&title=

If you prefer you can update the description of the issue above with the template so that we don't lose the context of this conversation. Getting the API changes reviewed is the first step to make sure you don't waste any time working on API changes that may not be approved.

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

ghost commented Apr 12, 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

Ok, I will create a proposal for that and mention the proposal here

@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 14, 2023
@mehdihadeli
Copy link
Author

#47705

@captainsafia
Copy link
Member

captainsafia commented Apr 24, 2023

but seems, .net core 7 only supports ValidationProblem for generating swagger metadata automatically and ProblemHttpResult type doesn't generate any swagger metadata.

It seems intentional that the HttpProblem type doesn't implement the IEndpointMetadataProvider interface for generating OpenAPI schemas like ValidationProblem does for the problem that you identified of having to support a wider set of status codes (compared to ValidationProblem which is implicitly 400).

I'm not enthusiastic about the API introduced in #47705 because I think that's best suited to be in app code or an external dependency. Starting to maintain strongly-typed ProblemDetails APIs for a subset of issues (unauthorized, etc.) gets us into tricky area with regard to bookkeeping.

I had to handle it manually for myself like this (we have some limitation because ProblemHttpResult is sealed class and I can't inherit from it directly, so I created a HttpProblemResultBase for replacement, also ProducesResponseTypeMetadata was internal, and I had to create a custom one):

You might want to take advantage of the ProducesResponseTypeAttribute for this particular case since it's already public.

@captainsafia captainsafia 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 24, 2023
@ghost
Copy link

ghost commented Apr 24, 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 24, 2023

Thanks for your response.
ValidationProblem name shows it is just for validation problem that is 400 or badrequest, but we don't have other problem types, and it makes results in minimal apis less readable!
Adopting an app or an external dependency due to limitations in the access modifiers is not easy.

This is one of my custom problem types:

public class NotFoundHttpProblemResult
    : IResult,
      IStatusCodeHttpResult,
      IContentTypeHttpResult,
      IValueHttpResult,
      IEndpointMetadataProvider
{
    private readonly ProblemHttpResult _internalResult;

    internal NotFoundHttpProblemResult(ProblemDetails problemDetails)
    {
        ArgumentNullException.ThrowIfNull(problemDetails);
        if (problemDetails is
            {
                Status: not null and not StatusCodes.Status404NotFound
            })
        {
            throw new ArgumentException(
                $"{nameof(NotFoundHttpProblemResult)} only supports a 404 NotFound response status code.",
                nameof(problemDetails));
        }

        problemDetails.Status ??= StatusCodes.Status404NotFound;

        _internalResult = TypedResults.Problem(problemDetails);
    }

    public ProblemDetails ProblemDetails => _internalResult.ProblemDetails;

    public Task ExecuteAsync(HttpContext httpContext)
    {
        return _internalResult.ExecuteAsync(httpContext);
    }

    public int? StatusCode => _internalResult.StatusCode;
    public string? ContentType => _internalResult.ContentType;
    object? IValueHttpResult.Value => _internalResult.ProblemDetails;

    public static void PopulateMetadata(MethodInfo method, EndpointBuilder builder)
    {
        ArgumentNullException.ThrowIfNull(method);
        ArgumentNullException.ThrowIfNull(builder);
        builder.Metadata.Add(new ProducesResponseTypeAttribute(typeof(ProblemDetails), StatusCodes.Status404NotFound));
    }
}

Here in the constructor, I don't have access to something like ProblemDetailsDefaults.Apply(ProblemDetails, statusCode: StatusCodes.Status400BadRequest), that we have in ValidationProblem:

public sealed class ValidationProblem : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult, IContentTypeHttpResult, IValueHttpResult, IValueHttpResult<HttpValidationProblemDetails>
{
    internal ValidationProblem(HttpValidationProblemDetails problemDetails)
    {
        ArgumentNullException.ThrowIfNull(problemDetails);
        if (problemDetails is { Status: not null and not StatusCodes.Status400BadRequest })
        {
            throw new ArgumentException($"{nameof(ValidationProblem)} only supports a 400 Bad Request response status code.", nameof(problemDetails));
        }

        ProblemDetails = problemDetails;
        ProblemDetailsDefaults.Apply(ProblemDetails, statusCode: StatusCodes.Status400BadRequest);
    }
}

ProblemDetailsDefaults is an internal class, so I had to use _internalResult = TypedResults.Problem(problemDetails) that is not clean.

Also, because TypedResult is static class and its methods are not extension method and I can't extend it for my custom problem types, so I had to create my own static TypedResult:

public static class TypedResultsExtensions
{
    public static InternalHttpProblemResult InternalProblem(
        string? title = null,
        string? detail = null,
        string? instance = null,
        string? type = null,
        IDictionary<string, object?>? extensions = null
    )
    {
        var problemDetails = CreateProblem(title, detail, instance, type, extensions);

        return new(problemDetails);
    }

    public static UnAuthorizedHttpProblemResult UnAuthorizedProblem(
        string? title = null,
        string? detail = null,
        string? instance = null,
        string? type = null,
        IDictionary<string, object?>? extensions = null
    )
    {
        var problemDetails = CreateProblem(title, detail, instance, type, extensions);

        return new(problemDetails);
    }

    private static ProblemDetails CreateProblem(
        string? title,
        string? detail,
        string? instance,
        string? type,
        IDictionary<string, object?>? extensions
    )
    {
        var problemDetails = new ProblemDetails
        {
            Detail = detail,
            Instance = instance,
            Type = type
        };

        problemDetails.Title = title ?? problemDetails.Title;

        if (extensions is not null)
        {
            foreach (var extension in extensions)
            {
                problemDetails.Extensions.Add(extension);
            }
        }

        return problemDetails;
    }
}

How can I handle these limitations?

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

@mehdihadeli Is there a reason you are not using the ProducesValidationProblem extension method to add the metadata for each custom status code your response supports?

@mehdihadeli
Copy link
Author

mehdihadeli commented Apr 24, 2023

Because I want to make my minimal api compatible with TypedResults, for more explicit typing like this:

app.MapPost("/", Handle);

async Task<Results<CreatedAtRoute<CreateProductResponse>, UnAuthorizedHttpProblemResult, ValidationProblem>> Handle([AsParameters] CreateProductRequestParameters requestParameters)
{
}

And as document says it is better we handle open-api-docuemntation with typed results instead of writing app.MapPost("/", Handle).ProducesValidationProblem() for each endpoint explicitly

@captainsafia
Copy link
Member

As mentioned in the corresponding API issue, providing status code specific ProblemDetails-generating extension methods isn't something we'd like to pursue in the framework at the moment.

Although TypedResults is the recommended approach for built-in Result types, it's totally valid to use Produces or custom Results definitions to support this scenario.

@captainsafia captainsafia added ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. 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 May 18, 2023
@ghost ghost added the Status: Resolved label May 18, 2023
@ghost
Copy link

ghost commented May 19, 2023

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed May 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2023
This issue was closed.
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 ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. Status: Resolved
Projects
None yet
Development

No branches or pull requests

6 participants
@adityamandaleeka @mitchdenny @SteveSandersonMS @captainsafia @mehdihadeli and others