WebAPI global exception handler

Question

There is a global exception handler in our WebAPI application that looks like the following:

public class ApiExceptionHandler : IExceptionHandler
{
    public async Task HandleAsync(ExceptionHandlerContext context, CancellationToken cancellationToken)
    {

        var exceptionType = context.Exception.GetType();

        if (exceptionType == typeof(ResourceNotFoundException))
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.NotFound, context.Exception.Message));
        }

        else if (exceptionType == typeof(UserNotFoundException))
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.Unauthorized, context.Exception.Message));
        }

        else if (exceptionType == typeof(UserAlreadyExistsExeption))
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.Conflict, context.Exception.Message));
        }

        ...

        else
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.InternalServerError, "An unexpected error occured"));
        }
    }
}

And each exception has the following form:

public class UserAlreadyExistsExeption: Exception
{
    public UserAlreadyExistsExeption()
    {
    }

    public UserAlreadyExistsExeption(string message) : base(message)
    {
    }

    public UserAlreadyExistsExeption(string message, Exception inner) : base(message, inner)
    {

    }
}

And can be thrown in the code like the following:

//If user exists
throw new UserAlreadyExistsExeption("User already exists");

I was thinking of making all our custom exceptions implement a IHasHttpErrorCode so each one has its own HttpErrorCode - usage like the following:

Interface:

public interface IHasHttpErrorCode
{
    HttpStatusCode GetHttpStatusCode();
}

Exception:

public class UserAlreadyExistsExeption: Exception, IHasHttpErrorCode
{
    public UserAlreadyExistsExeption()
    {
    }

    public UserAlreadyExistsExeption(string message) : base(message)
    {
    }

    public UserAlreadyExistsExeption(string message, Exception inner) : base(message, inner)
    {

    }

    public HttpStatusCode GetHttpStatusCode() {
        return HttpStatusCode.Conflict;
    }
}

Global Error Handler:

public class ApiExceptionHandler : IExceptionHandler
{
    public async Task HandleAsync(ExceptionHandlerContext context, CancellationToken cancellationToken)
    {

        var exceptionType = context.Exception as IHasHttpErrorCode;


        if (customException != null) { 
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(customException.GetHttpStatusCode(), context.Exception.Message));
        } 
        else
        {
            context.Result = new ResponseMessageResult(
                context.Request.CreateResponse(HttpStatusCode.InternalServerError, "An unexpected error occured"));
        }
    }
}

Is this a valid approach to improve the existing code? Is there anything else I should look to improve?

I would personally go even further and make common abstract base to force you to implement the status code. — user52292
– user52292, Commented Sep 19, 2018 at 16:09
@firda Is that much different from an interface? What would it look like? — TomSelleck
– TomSelleck, Commented Sep 19, 2018 at 16:30
@TomSelleck the only difference is that you can forget to add the interface, but cannot forget to implement the (abstract) status code if you derive all your exceptions from common abstract base... which you can of course forget as well, but having one common base exception for a library / group of objects has another advantages (catching base) and therefore solves two things at once (and maybe you already have the common base exception). — user52292
– user52292, Commented Sep 19, 2018 at 16:36

user52292 · Accepted Answer · 2018-09-19 16:58:21Z

Using the interface is definitely an improvement, but what I lack in your code is common base exception, something like ServerException that could be used to distinguish other exceptions (e.g. IndexOutOfRangeException or whatever) from your exceptions (useful for catching).

That common base could be made abstract to force you to define HttpStatusCode

public abstract class ServerException: Exception {
//  standard constructors...
    public abstract HttpStatusCode GetHttpStatusCode();
}

Now, if you derive any custom exception from this (as you should, maybe creating some other abstract derived exceptions for some groups of problems), then you have to override (define) the code (or it would have to remain abstract and you won't be able to throw it).

public class UserAlreadyExistsExeption: ServerException {
//  again standard .ctors
    override public HttpStatusCode GetHttpStatusCode() => HttpStatusCode.Conflict;
}

Alternative would be to use public get-only property and initialize it in the constructor, maybe even having one exception type (and use try-catch-when), but I am not sure that can be used in your code (would require perfect 1-to-1 match between exception types and status codes).

Make message default

public class UserAlreadyExistsExeption: Exception, IHasHttpErrorCode
{
    public UserAlreadyExistsExeption(): base("User already exists") {}
}

and

throw new UserAlreadyExistsExeption();

instead of

throw new UserAlreadyExistsExeption("User already exists");

Maybe even handling localization in the default constructor, if needed.

Stack Exchange Network

WebAPI global exception handler

1 Answer 1

Make message default

You must log in to answer this question.

Hot Network Questions

WebAPI global exception handler

1 Answer 1

Make message default

You must log in to answer this question.

Related

Hot Network Questions