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

Minimal API: TypedResults, Problem, ValidationProblem #59560

Open
fdonnet opened this issue Dec 19, 2024 · 9 comments
Open

Minimal API: TypedResults, Problem, ValidationProblem #59560

fdonnet opened this issue Dec 19, 2024 · 9 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc design-proposal This issue represents a design proposal for a different issue, linked in the description

Comments

@fdonnet
Copy link

fdonnet commented Dec 19, 2024

Summary

Enhance the way we return TypedResult for minimal api and ProblemDetails when the system raise an error.

Motivation and goals

I believe the newly added features related to this topic are great, but there is room for improvement.

In scope

  • TypedResults
  • ProblemDetails options
  • Minimal Api
  • Exceptions versus "business" errors.

Out of scope

N/A

Risks / unknowns

None

Examples

The way we can now use the following code is great:

services.AddProblemDetails(options =>
{
    options.CustomizeProblemDetails = context =>
    {
        context.ProblemDetails.Instance =
            $"{context.HttpContext.Request.Method} {context.HttpContext.Request.Path}";

        context.ProblemDetails.Extensions.TryAdd("requestId", context.HttpContext.TraceIdentifier);

        Activity? activity = context.HttpContext.Features.Get<IHttpActivityFeature>()?.Activity;
        context.ProblemDetails.Extensions.TryAdd("traceId", activity?.Id);
    };
});

app.UseStatusCodePages();

Enriching our problem details in this manner is simply ❤

For exception handling in the case of a real exception, the ability to inject the IProblemDetailsService service is cool:

public class CustomExceptionHandler(IProblemDetailsService problemDetailsService, ILogger<CustomExceptionHandler> log) : IExceptionHandler
{
    private readonly ILogger<CustomExceptionHandler> _log = log;

    public async ValueTask<bool> TryHandleAsync(
        HttpContext httpContext,
        Exception exception,
        CancellationToken cancellationToken)
    {
        _log.LogError(exception, "An error occurred: {message}",exception.Message);

        var problemDetails = new ProblemDetails
        {
            Status = StatusCodes.Status500InternalServerError,
            Title = "An error occurred",
            Type = exception.GetType().Name,
            Detail = exception.Message,
        };

        return await problemDetailsService.TryWriteAsync(new ProblemDetailsContext
        {
            Exception = exception,
            HttpContext = httpContext,
            ProblemDetails = problemDetails
        });
    }
}

However, I believe there is a need for more clarity regarding when we are serious about error handling and do not want to raise an exception in the case of a normal error. An exception should indicate an unplanned error.

Now that we have the TypedResults extension and a way to enforce our Minimal API to return certain types, ValidationProblem seems unnecessary because ultimately, we want to return either our normal typed result or a typed ProblemDetails result.

Something like that:
Task<Results<Created<TenantStandardResult>, ProblemHttpResult >>

Not something like that
Task<Results<Created<TenantStandardResult>, ProblemHttpResult, ValidationProblem >>

On my end, to always be able to return ProblemDetails in the case of "business" errors, validation errors, or exceptions, I created a new extension called CustomTypedResults as follows:

    public static class CustomTypedResults
    {
        public static ProblemHttpResult Problem(IDictionary<string, string[]> validationErrors)
        {
            ArgumentNullException.ThrowIfNull(validationErrors);
            
            var problemDetails = new ProblemDetails()
            {
                Detail = "Validation error",
                Status = 400,
                Title = "Validation error",
                Extensions = { { "validationErrors", validationErrors } }
            };

            return TypedResults.Problem(problemDetails);
        }

        public static ProblemHttpResult Problem(IFeatureError featureError)
        {
            ArgumentNullException.ThrowIfNull(featureError);

            var problemDetails = new ProblemDetails()
            {
                Detail = featureError.Details,
                Status = (int)featureError.ErrorType,
                Title = "Service - feature error",
                Extensions = { { "errors", featureError.CustomErrors } }
            };

            return TypedResults.Problem(problemDetails);
        }

    }

With that, I m able to pass the result of a fluent validation (Dic) (like before with TypedResults.ValidationProblem():
CustomTypedResults.Problem()

Or pass my error interface (IFeatureError), the same way:
CustomTypedResults.Problem()

And maintain my typed API returns simple:
private async Task<Results<Created<TenantStandardResult>, ProblemHttpResult >>

Detailed design

I am unsure if this approach will be interesting for others, but I believe it simplifies the process and allows us to consistently return ProblemDetails without further consideration.

Mainly when you don't use exception handler to trap normal errors...
I think it's not good and I prefer very much the Either<IFeatureError,RightResult> pattern and be able to transform a IFeatureError to ProblemDetails.

I am curious if it would be beneficial to explore this topic further or if anyone has a better approach already.

@fdonnet fdonnet added the design-proposal This issue represents a design proposal for a different issue, linked in the description label Dec 19, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Dec 19, 2024
@mikekistler
Copy link
Contributor

I think this approach may not work well with the generation of OpenAPI documents that was added in .NET 9. In particular, I think it would require ProducesResponseType attributes to be specified in order to get the error responses to be included in the generated OpenAPI document. This is because TypedResults.Problem does not provide metadata for the OpenAPI document, unlike other TypedResults methods like TypedResults.BadRequest. Is that important to you?

@fdonnet
Copy link
Author

fdonnet commented Dec 24, 2024

@mikekistler right, thx I was convinced that TypedResults.Problem will provide the ProblemDetails model in OpenID doc. Other TypedResults. are working but not TypedResults.Problem... I just confirmed now and I m sad... ;)

@fdonnet
Copy link
Author

fdonnet commented Dec 24, 2024

@mikekistler I changed for something like that, but it's bad:

       public static BadRequest<ProblemDetails> Problem(IFeatureError featureError)
        {
            ArgumentNullException.ThrowIfNull(featureError);

            var problemDetails = new ProblemDetails()
            {
                Detail = featureError.Details,
                Status = (int)featureError.ErrorType,
                Title = "Service - feature error",
                Extensions = { { "errors", featureError.CustomErrors } }
            };

            return TypedResults.BadRequest(problemDetails);
        }

Not as cool as before... but at least I have the problemdetails model in OpenApi

EDIT: but now, I m loosing the correct StatusCode.... grrr, so not good. => worst.

@mikekistler
Copy link
Contributor

Right. BadRequest is wired to produce a 400 status (only). In fact, this is what makes it amenable to providing metadata for the OpenAPI doc. If the status code is unknown at build time, I don't think it's possible to produce OpenAPI metadata.

If there is some fixed set of status codes for featureError.ErrorType, it might be possible to create a TypedResults for just this set ...

@fdonnet
Copy link
Author

fdonnet commented Dec 24, 2024

I will try this way... because I have the current status code in my IfeatureError. I was thinking that the .Problem() will cover all my needs but no. My Api will return ProblemDetails for all possible err status code... sad I cannot document that easily.

@fdonnet
Copy link
Author

fdonnet commented Dec 24, 2024

linked to that #52424

@mikekistler
Copy link
Contributor

Perhaps what you want is to add a 4XX response. OpenAPI v3.0 added support for "response code ranges" and "4XX" basically means any response code in the range 400-499 that is not explicitly defined.

I added a ProblemResponseTransformer to my TransformerGallery project to demonstrate how to do this. It adds a 4XX response to every operation -- it was pretty straightforward to develop.

I hope this is helpful.

@erwinkramer
Copy link

Thanks @mikekistler.

Just a small detail: you missed a forward slash after TransformerGallery in your hyperlink in the readme :)

Pointing to
https://github.com/mikekistler/aspnet-transformer-gallery/blob/main/TransformerGalleryTransformers/ProblemResponseTransformer.cs

Should be
https://github.com/mikekistler/aspnet-transformer-gallery/blob/main/TransformerGallery/Transformers/ProblemResponseTransformer.cs

Speaking of transformers, this is also a cool one that adds a security requirement to each operation based on the security policy that has been set:
https://github.com/erwinkramer/bank-api/blob/c4d28c60118ada98b9a59ce6e9c312e7581096b6/BankApi.Core/Defaults/Transformer.Operation.cs#L52

@fdonnet
Copy link
Author

fdonnet commented Dec 27, 2024

@mikekistler, this looks great! Now, I'm unsure whether to use .ProducesProblem(4xx) to detail all possible problem returns for each endpoint or to go with your transformer. I appreciate having visibility into all potential status codes—for instance, an update might return a 409 due to an "optimistic concurrency" exception etc etc. However, I wonder if providing so much detail is necessary, or if a more general approach would suffice.

At the end, it's for a side project I have, and I believed that this TypedResuls.Problem() thing was the Holy Grail 😄 To enforce the result types for my endpoints and to have an OpenApi documentation that fits without any effort.

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 design-proposal This issue represents a design proposal for a different issue, linked in the description
Projects
None yet
Development

No branches or pull requests

3 participants