dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.46k stars 10.03k forks source link

Customize error handling for [ApiController] #4888

Closed ryanelian closed 5 years ago

ryanelian commented 6 years ago

Is this a Bug or Feature request?:

Feature Request / Question

Steps to reproduce or link to a repro project:

[ApiController]
[Produces("application/json")]
[Route("api/v1/value")]
public class ValueApiController : Controller
{
    [HttpGet]
    public ActionResult<string[]> Get()
    {
        throw new Exception("bam");
    }
}

Startup.cs

if (env.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}
else
{
    app.UseExceptionHandler("/Error");
    app.UseStatusCodePagesWithReExecute("/Error");
}

Description of the problem:

When an error occurs in Controller classes marked with [ApiController], I don't want them to return HTML but JSON instead:

When using Core MVC < 2.1, I have been able to do this by using code like:

app.UseWhen(context => context.Request.Path.Value.StartsWith("/api", StringComparison.OrdinalIgnoreCase), builder =>
{
    builder.UseExceptionHandler(configure =>
    {
        configure.UseMiddleware<ApiExceptionHandlerMiddleware>(env.IsDevelopment());
    });
});

app.UseWhen(context => context.Request.Path.Value.StartsWith("/api", StringComparison.OrdinalIgnoreCase) == false, builder =>
{
    if (env.IsDevelopment())
    {
        builder.UseDeveloperExceptionPage();
    }
    else
    {
        builder.UseExceptionHandler("/error");
        builder.UseStatusCodePagesWithReExecute("/error");
    }
});

However, seeing that the purpose of the [ApiController] is: (written on the XML doc)

Indicates that a type and all derived types are used to serve HTTP API responses. The presence of this attribute can be used to target conventions, filters and other behaviors based on the purpose of the controller.

, I would like to know how to alter the error behavior when this attribute is encountered. (Instead of checking whether the request starts with /api)

Version of Microsoft.AspNetCore.Mvc or Microsoft.AspNetCore.App or Microsoft.AspNetCore.All:

<PackageReference Include="Microsoft.AspNetCore.App" Version="2.1.0-rc1-final" />
khalidabuhakmeh commented 6 years ago

It looks like the UseExceptionHandlerMiddleware has the ability to take a RequestDelegate that can alter the response coming out of the middleware. The ExceptionHandlerTests have some examples of how to do that.

https://github.com/aspnet/Diagnostics/blob/88db534e42a49eade81611b04c0adc04e4684b2c/test/Microsoft.AspNetCore.Diagnostics.Tests/ExceptionHandlerTest.cs

Looking at the implementation of ApiControllerAttribute it looks purely like a marker attribute with no real functionality. This leads me to believe that knowing whether your request is served by an API controller or an MVC controller from middleware may be difficult without rolling your own custom approach like you have above.

khellang commented 6 years ago

All behavior related to the ApiControllerAttribute is defined in the ApiBehaviorApplicationModelProvider:

https://github.com/aspnet/Mvc/blob/504da3c565a9a159365b4564dba5f2cd7d059e7f/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs#L57-L91

AFAIK, the ApiControllerAttribute doesn't affect exception handling. You might be referring to the ModelStateInvalidFilter, which lets you remove the usual MVC validation boilerplate:

https://github.com/aspnet/Mvc/blob/504da3c565a9a159365b4564dba5f2cd7d059e7f/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ModelStateInvalidFilter.cs#L51-L58

The default implementation of InvalidModelStateResponseFactory just creates a new BadRequestObjectResult with the ModelState dictionary:

https://github.com/aspnet/Mvc/blob/ec31ff0c28961be64ad0b8228eba3a084086f60b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs#L25-L33

If you want to customize this behavior, you can set InvalidModelStateResponseFactory:

services.Configure<ApiBehaviorOptions>(options =>
{
    options.InvalidModelStateResponseFactory = context =>
    {
        // Do whatever you want in here, i.e. return an IActionResult or throw an exception.
    };
});
mkArtakMSFT commented 6 years ago

@pranavkm, do we have any guidance here? Or is @khellang's suggestion inline with the approach we'd suggest?

ryanelian commented 6 years ago

AFAIK, the ApiControllerAttribute doesn't affect exception handling.

@khellang

Very interesting! But I really mean exception handling, not model-state validation...

I wish there is a way to check whether the request is an API Controller. Something like:

HttpContext.IsApiController
pranavkm commented 6 years ago

Like @ryanelian points out, the model state response factory addresses one aspect of the error handling viz model validation. Handling errors is just as interesting. Speaking to @rynowak, to support Api Controllers over routing would involve

a) Set a flag (maybe a Http feature) as part of executing an API controller b) Use this in the error handler (Template update)

Speaking to @rynowak, dispatcher should solve the first requirement. This would make for a pretty nice enhancement with the rest of the API work we'd do for 2.2

mkArtakMSFT commented 6 years ago

@pranavkm, can you please summarize the actions we want to take as part of this issue? Also please assign cost to this issue.

mkArtakMSFT commented 6 years ago

Talked to @pranavkm regarding this, and here are the actions we want to take here:

/cc @glennc, @danroth27

Eneuman commented 6 years ago

I like this. Please allow us to customize the response. Today we are using a custom Middleware that adds a global exception filter. The exception filter then creates a response that conforms with RFC 7807. It all gets setup with a builder like this: app.UseApiExceptionResponse(bool devEnv);

If we can do something like this nativly in MVC that will be great.

mkArtakMSFT commented 6 years ago

@glennc, moving this out to 3.0 because of the required changes from dispatcher work have been pushed to 3.0.

glennc commented 6 years ago

@Eneuman @ryanelian I definitely want to do something here, it's one of my pet annoyances that you end up with HTML by default when debugging an API from our template. We also want to do work around Problem Details. The Problem Details work will likely be in 2.2, but I don't know how that will affect the default exception page yet.

bugproof commented 6 years ago

The problem details should also be returned when the exception occurs and not only upon validation error or some easy way to set it up.

It is common for people to have thin controllers and people usually throw exceptions from their lower layers below controllers like service classes/MediatR handlers and that needs to be handled too.

There's also one library that you can use to avoid throwing exceptions everywhere: https://github.com/mcintyre321/OneOf/ but exceptions from other libraries etc. are inevitable so they need to be handled anyway.

I like the idea of having "exception policies" like in this library: https://github.com/IharYakimush/asp-net-core-exception-handling

khellang commented 6 years ago

There's also https://www.nuget.org/packages/Hellang.Middleware.ProblemDetails. See this sample; https://github.com/khellang/Middleware/blob/master/samples/ProblemDetails.Sample/Program.cs

bugproof commented 6 years ago

It's good but exception mapping functionality should be more reusable in my opinion and ideally should be a part of the official libraries so we don't have to look for the solution every time when starting a new project. Hopefully, that will change in 3.0

khellang commented 6 years ago

ideally should be a part of the official libraries

Those are official libraries. Not official from Microsoft, but official from the .NET community. Not good enough?

so we don't have to look for the solution every time when starting a new project.

You don't. Look no further. The functionality has been provided by the community. You're welcome 😉

rynowak commented 5 years ago

So we've done extensible handlers for DeveloperExceptionPage (#8536), and we've done a fallback to plaintext for non-html-clients (ie, not browsers).

That feels like enough for a default experience here. Since this is for dev-time, we don't think JSON is going to end up being more human-readable than plaintext.

Feel free to post your feedback if you think have a JSON version of the development exception page is vital. We have some other plans for improved error handling in general for APIs that relate to production that are tracked elsewhere.