Skip to content

Support all error's status codes ProblemDetail types for generating swagger docs in MinimalApi #47705

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 14, 2023 · 3 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc

Comments

@mehdihadeli
Copy link

mehdihadeli commented Apr 14, 2023

Background and Motivation

If we want to create an api with multiple result types with using TypedResults, .net core 7 only supports ValidationProblem for auto generating swagger metadata and ProblemHttpResult type doesn't generate any swagger metadata (because it doesn't implement IEndpointMetadataProvider). Maybe we need to declare dedicated problem types like ValidationProblem for others error status codes, like InternalProbelm, UnAuthorizedProblem, NotFoundProblem...

There is an issue #47623 already for this proposal.

Proposed API

This is an example for UnAuthorized problem, but it is the same for other error status codes:

namespace Microsoft.AspNetCore.Http.HttpResults;

+ public sealed class UnAuthorizedHttpProblemResult
+       : IResult,
+       IEndpointMetadataProvider,
+        IStatusCodeHttpResult,
+        IContentTypeHttpResult,
+        IValueHttpResult
+{
+    internal UnAuthorizedHttpProblemResult(ProblemDetails problemDetails)
+    {
+        ArgumentNullException.ThrowIfNull(problemDetails);
+        if (problemDetails is { Status: not null and not StatusCodes.Status401Unauthorized })
+        {
+            throw new ArgumentException(
+                $"{nameof(UnAuthorizedHttpProblemResult)} only supports a 401 UnAuthorize response status code.",
+               nameof(problemDetails)
+            );
+       }
+
+        ProblemDetails = problemDetails;
+        ProblemDetailsDefaults.Apply(ProblemDetails, statusCode: StatusCodes.Status401Unauthorized);
+    }
+
+   public ProblemDetails ProblemDetails { get; }
+
+   object? IValueHttpResult.Value => ProblemDetails;
+
+   public string ContentType => "application/problem+json";
+
+   public int StatusCode => StatusCodes.Status401Unauthorized;
+
+    int? IStatusCodeHttpResult.StatusCode => StatusCode;
+
+
+    public Task ExecuteAsync(HttpContext httpContext)
+    {
+        ArgumentNullException.ThrowIfNull(httpContext);
+
+        var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
+        var logger = loggerFactory.CreateLogger(typeof(UnAuthorizedHttpProblemResult));
+
+       HttpResultsHelper.Log.WritingResultAsStatusCode(logger, StatusCode);
+        httpContext.Response.StatusCode = StatusCode;
+
+        return HttpResultsHelper.WriteResultAsJsonAsync(httpContext, logger, value: ProblemDetails, ContentType);
+    }
+
+    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
+    {
+        ArgumentNullException.ThrowIfNull(method);
+        ArgumentNullException.ThrowIfNull(builder);
+
+        builder.Metadata.Add(
+            new ProducesResponseTypeMetadata(
+                typeof(ProblemDetails),
+                StatusCodes.Status401Unauthorized,
+                "application/problem+json"
+           )
+       );
+    }
+}
namespace Microsoft.AspNetCore.Http;

/// <summary>
/// A typed factory for <see cref="IResult"/> types in <see cref="Microsoft.AspNetCore.Http.HttpResults"/>.
/// </summary>
public static class TypedResults
{
+    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 UnAuthorizedHttpProblemResult(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;
+    }
}

Usage Examples

endpoints.MapPost("/products",
    Results<UnAuthorizedHttpProblemResult, Ok<ProductDto>>(ProductDto product) =>
    {
        if ( some condition )
           return TypedResults.UnauthorizedProblem();
         
           return TypedResults.Ok(product);
    });

Alternative Designs

N/A

Risks

None that I am aware of.

@mehdihadeli mehdihadeli added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 14, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 14, 2023
@javiercn javiercn added 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 14, 2023
@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 27, 2023
@ghost
Copy link

ghost commented Apr 27, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented May 11, 2023

API Review Notes:

  • We do not want to duplicate all of our non-200-returning IResult types and TypedResult methods to have a ProblemDetails variation.
    • We already have special types for many status codes like UnauthorizedHttpResult for non-ProblemDetails returning APIs that return a 401. Same goes for many other status codes.
    • We think 400 for ValidationProblem is a special case, but we don't want to extend this pattern for every status code since it requires a new type and method per status code.
  • Others error status codes, like InternalProbelm, UnAuthorizedProblem, NotFoundProblem... is not specific enough for approving the API, but even if they were all listed out, we'd be unlikely to accept the API.

We encourage using TypedResults.Problem with the ProducesProblem IEndpointConventionBuilder extension method either on the endpoint or a RouteGroup returned by MapGroup. For example:

var hasUnauthorizedProblemsGroup = endpoints.MapGroup("");

hasUnauthorizedProblemsGroup.MapPost("/products",
    Results<ProblemHttpResult, Ok<ProductDto>>(ProductDto product) =>
    {
        if ( some condition )
           return TypedResults.Problem(statusCode: 401);
         
         return TypedResults.Ok(product);
    });

hasUnauthorizedProblemGroup.ProducesProblem(401);

You can also use Produces directly on the MapPost call:

endpoints.MapPost("/products", ...).ProducesProblem(401);

Thanks for submitting the API proposal, but we have decided to reject it.

@mehdihadeli
Copy link
Author

mehdihadeli commented May 12, 2023

@halter73 Thanks for your review.
I respect your decision.
But is there any clean way to handle this in my code? I want to handle open-apis automatically with TypeResults, because this is one of the advantages of TypeResults. I mentioned my solution here (#47623).
But because of some internal and sealed classes in.net, it is not very clean.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2023
@halter73 halter73 removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 22, 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
Projects
None yet
Development

No branches or pull requests

4 participants