dotnet / AspNetCore.Docs

Documentation for ASP.NET Core
https://docs.microsoft.com/aspnet/core
Creative Commons Attribution 4.0 International
12.63k stars 25.29k forks source link

Mention Web API (Core) error handling recommended practises #8118

Closed matteocontrini closed 5 years ago

matteocontrini commented 6 years ago

This article is focused on ASP.NET Core applications with views, so when developing Web APIs it doesn't really make clear what should be done and how.

I would suggest to add a section that explains recommended practises when handling errors in Web API ASP.NET Core projects.

You usually want responses from a Web API to be consistent (for example, to always be JSON with a common structure), so it would be nice to have an example of an error controller which returns a common response, and then an example code that shows how to "link" errors to the controller, like this:

app.UseExceptionHandler("/errors/500");
app.UseStatusCodePagesWithReExecute("/errors/{0}");

It's basically the information that you can find in this error handling article in the "Handling global errors" section, but I think it would be better to have some parts of it in the official documentation.

Thanks!


Document details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

guardrex commented 6 years ago

Thanks @matteocontrini ... great idea.

I'll take it on, but it's going to be a little while before I can get to it. 🏃🏃🏃🏃😅 If you or a member of the community would like to address it before I'm free, please let us know.

spottedmahn commented 6 years ago

I was looking for something similar to @matteocontrini.

I want API errors to return "raw" responses that come out of my API.

I want all other errors to go user-friendly error page.

Does that make sense?

NicolasDorier commented 6 years ago
app.UseStatusCodePages(async context =>
{
    context.HttpContext.Response.ContentType = "text/plain";

    await context.HttpContext.Response.WriteAsync(
        "Status code page, status code: " + 
        context.HttpContext.Response.StatusCode);
});

Copy pasted, does not build.

guardrex commented 6 years ago

@NicolasDorier It's in the Microsoft.AspNetCore.Diagnostics package. Confirm that you reference the package. After that, you should be able to use the UseStatusCodePages extension method with your using Microsoft.AspNetCore.Builder; statement.

It's called out for the Developer Exception Page but not the Status Code Pages. This is a quick fix compared to the overall issue here, so I'm going to submit a quick patch that will fix that now. I'll circle back around to this issue :point_up: at some point in the future.

NicolasDorier commented 6 years ago

wow this is confusing! I was not referencing Diagnostics. Both UseStatusCodePages from Diagnostic and builder use the same middleware under the hood?

guardrex commented 6 years ago

The IApplicationBuilder namespace (Microsoft.AspNetCore.Builder) exposes the extension method after the package is referenced by the app. All of the middlewares in the framework work the same way (the ones provided by Microsoft; 3rd party may require one to reach the middleware using a different namespace). You add the package, then you only need to have the namespace for Microsoft.AspNetCore.Builder to get the middlewares configured and added to the pipeline in your Startup code.

There's a brief ...... very brief .... remark about it in this section of the Middleware topic ...

https://docs.microsoft.com/aspnet/core/fundamentals/middleware#write-middleware

... where it says ...

The following extension method exposes the middleware through IApplicationBuilder:

... and then there's an example.

I do think it could be stated in a more general context earlier in that topic. Unfortunately, this one didn't hit the radar for recent updates to the Middleware topic on #8209. I'll open a new issue to address it. Sorry you hit this ... I think we can take this opportunity to make that point clear in the topic.

NicolasDorier commented 6 years ago

I am sorry actually my problem was not about the extension method not being found, but this.

image

NicolasDorier commented 6 years ago

But the strange thing is that my intellisense show HttpContext when I type context.. But the compiler is not resolving the same type.

NicolasDorier commented 6 years ago

Then I specify the type explicitely, seems to work, though no WriteAsync.

image

guardrex commented 6 years ago

Add ...

using Microsoft.AspNetCore.Http;

... and see if that does the trick.

guardrex commented 6 years ago

... and don't specify the type explicitly. Go back to the original code from the topic.

NicolasDorier commented 6 years ago

Ok it worked, so in summary.

+ using Microsoft.AspNetCore.Http;
- app.UseStatusCodePages(async context =>
+ app.UseStatusCodePages(async (StatusCodeContext context) =>
NicolasDorier commented 6 years ago

This is very curious that my intellisense correctly infer context to be from type StatusCodeContext, but not the compiler (C# 7.3)

NicolasDorier commented 6 years ago

OK sorry I tried again, with

+ app.UseStatusCodePages(async context =>

It pass correctly. without using Microsoft.AspNetCore.Http;, the error message was about context being infered HttpContext by the compiler, but not if I include Microsoft.AspNetCore.Http. Very strange.

guardrex commented 6 years ago

Yes, Microsoft.AspNetCore.Http exposes the property Microsoft.AspNetCore.Diagnostics.StatusCodeContext.HttpContext ...

https://docs.microsoft.com/dotnet/api/microsoft.aspnetcore.diagnostics.statuscodecontext.httpcontext

In order to clarify how to make the code sample work, I'll add comments to the topic for both the Diagnostics package and the Microsoft.AspNetCore.Http namespace.

I'll ping u on the PR, and you can let me know how the revised text looks.

guardrex commented 6 years ago

The first round of that is just the package .... I'll add the namespace language on the next commit.

guardrex commented 6 years ago

Sorry @matteocontrini for all of the chatter. I will circle back around to your requests for updates here as soon as I can (or a member of the community is welcome to address the points you raised).

HamedMousavi commented 6 years ago

In WebApi it was possible to write a global exception handler and replace the default exception handler with 1 line of code: config.Services.Replace(typeof(IExceptionHandler), new MyGlobalExceptionHandler());

This allowed me to throw exceptions in my code regardless of the client code (restful service or otherwise), then it was my global exception handler that would test exception type and generate proper response in web api. For example in MyGlobalExceptionHandler:

if (exception is RootObjectNotFoundException) { context.Result = new SimpleErrorResult(context.Request, HttpStatusCode.NotFound, exception.Message); return; }

This would remove all the ugly try/catch clutter from all controllers and decouple domain logic code from the client code (in this case web api) and centralize response generation in exception cases (higher cohesion).

I was expecting to see instructions for such a pattern here.

HamedMousavi commented 6 years ago

Sorry, this is the rest of the above message:

To achieve the same goal, I had to do this: in Startup.Configure:

        app.UseExceptionHandler(errorApp => {
            errorApp.Run(async context => {
                var error = context.Features.Get<IExceptionHandlerFeature>();
                if (error != null) {
                    var ex = error.Error;
                    await _exceptionHandler.Handle(ex, context.Response);
                }
            });
        });

where _exceptionHandler is an instance of MyGlobalExceptionHandler class:

public class GlobalExceptionHandler {
    internal async Task Handle(Exception exception, HttpResponse response) {
        if (exception is CallerNotFoundException) {
            response.StatusCode = 404;
            response.ContentType = "text/plain";
            await response.WriteAsync(exception.ToString(), Encoding.UTF8);
            return;
        }

        response.StatusCode = 500;
        response.ContentType = "text/plain";
        await response.WriteAsync(exception.ToString(), Encoding.UTF8);
    }

Obviously, sending exception details to client is not a good idea. So, the above code should not be used in production.

danacon01 commented 5 years ago

Hi guardrex, it's been a while since your last comment - i know you said you guys were rammed but i was wondering if you had any estimate for when the recommended practices for web api would be added to the help docs?

guardrex commented 5 years ago

I don't at this time. I'm just getting past my portion of the 2.2 RTM work, and I have some high priority IIS/Azure Apps work to do by year's end.

Rick-Anderson commented 5 years ago

@danroth27 please review

simeyla commented 5 years ago

This seems to be a very popular Stackoverflow answer on the topic of exception handling middleware.

https://stackoverflow.com/a/38935583/16940

Some official guidance would still be very welcome. The Core documentation on the whole is excellent, but sometimes there are notable holes like this for seasoned developers looking for 'the new way' to do something.

Rick-Anderson commented 5 years ago

@danroth27 please review and assign someone to provide guidance on Web API exception handling.

guardrex commented 5 years ago

There are updates headed to the topic now on https://github.com/aspnet/Docs/pull/11242. It's in review.

The middleware scenario described in the SO post can easily be handled by the framework's Exception Handling Middleware that takes the lambda. I also added content on getting some :heart: out of IExceptionHandlerFeature in a controller or page. The OOB Exception Handling Middleware is fairly powerful stuff 💪.

danroth27 commented 5 years ago

@glennc is the right person to handle this.

cremor commented 5 years ago

You usually want responses from a Web API to be consistent (for example, to always be JSON with a common structure)

I agree that the Web API exception handling documentation should be improved and this is the most important part.

Right now, there are a few pitfalls with exception handling for Web APIs:

  1. The project templates already contain code that returns HTTP from API calls. No REST client would want that. For the plain Web API template it's only the UseDeveloperExceptionPage in development environment, but I'd say even that is questionable. For the MVC template the UseExceptionHandler call for production environments gets problematic as soon as you add API controllers.
  2. If you want to have both view and API controllers in one project, you have to configure the exception handlers correctly for each part (view controllers should return an error page, but API controllers should either return no body or return an error object as JSON). One way to do this is shown here: https://github.com/christianacca/ProblemDetailsDemo/blob/master/src/ProblemDetailsDemo.Api/Startup.cs#L60

Maybe the documentation should mention the package Hellang.Middleware.ProblemDetails. It is very useful for API exception handling.

xcaptain commented 5 years ago

I'm new to dotnet core, currently I found 3 ways to do exception handler in asp dotnet core web api.

  1. Config UseExceptionHandler as the document says
  2. Use an exception handler middleware as stackoverflow question says
  3. Use controller helper functions like BadRequest()

I prefer to use controller helper function but these functions don't throw an exception. Would be help to built in some basic http exceptions and let these helper functions throw exception.

bugproof commented 5 years ago

I generally prefer to use IExceptionFilter (MVC filter) more than UseExceptionHandler (middleware) when using MVC. If you use content negotiation (MVC feature) then exception filters are better because they respect it. With UseExceptionHandler you set your Content-Type explicitly which completely ignores content negotiation. The documentation doesn't mention that. Most people only use JSON anyway for their Web APIs.

So my best practice would be:

If you're using MVC you can use both filters and middleware, if you're not using MVC (perhaps you're using https://github.com/CarterCommunity/Carter) - use middleware.

prince272 commented 5 years ago

Hello, I'm Prince. I would like to share alittle priece of code concerning error handling with or without the /api path.

app.UseWhen( context => !context.Request.Path.StartsWithSegments("/api"), a => { a.UseStatusCodePagesWithReExecute("/Home/Error{0}"); a.UseExceptionHandler("/Home/Error"); } );

            app.UseWhen(
                context => context.Request.Path.StartsWithSegments("/api"),
                a =>
                {
                    a.UseStatusCodePagesWithReExecute("/Home/Error{0}");
                    app.UseExceptionHandler(errorApp =>
                    {
                        errorApp.Run(async context =>
                        {
                            var feature = context.Features.Get<IExceptionHandlerPathFeature>();
                            var exception = feature.Error;

                            var result = JsonConvert.SerializeObject(new { error = "Internal Server Error" });
                            context.Response.ContentType = "application/json";
                            await context.Response.WriteAsync(result);
                        });
                    });
                }

Is there any built-in method for global error logging?

hades200082 commented 5 years ago

The problem I've seen with all implementations of exception handlers so far is that they completely ignore content negotiation.

If the client has stated that they only accept XML, and the rest of your API can provide XML responses, then the exception result returned should be no different.

paul-michalik commented 5 years ago

The easiest most robust and portable (with respect to content negotiation and such) method to handle errors in WEB API apps is the one mentioned by @matteocontrini in the very first comment. ASP.NET Core team, please add a section to the documentation which advises the readers to use it!

Rick-Anderson commented 5 years ago

This was fixed in #14366 and will go live today. cc @pranavkm @scottaddie