Customize error handling for [ApiController]

[ryanelian]

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:

• In development, instead of Developer Exception Page, ideally the API Controller should return stack trace in plain text / JSON.

• In production, instead of a custom HTML error response, ideally the API Controller should return a plain text / JSON with like "An error occurred while processing your request."

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]

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]

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]

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

[ryanelian]

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]

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]

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

[mkArtakMSFT]

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

• Update the templates to return JSON response in case of an error when in an ApiController
• Expose information about whether the action is in a context of an API controller or not (will be handled as part of some other issue - dispatcher)

/cc @glennc, @danroth27

[Eneuman]

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]

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

[glennc]

@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]

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]

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]

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]

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]

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.