6

Imagine a program which exposes a REST service/gRPC service/whatever service, which uses several 3rd party libraries. These libraries can of course throw exceptions if something goes wrong, for example if the user tried to access something which he isn't allowed to.

The problem here is that it isn't possible to check beforehand if the request is correct or if the user has enough rights. So we send the request to the 3rd party library and get an exception.

Is it good practice to catch these exceptions at the top level and map them to status codes like this?

var statusCode = HttpStatusCode.InternalServerError;
if (ex is ArgumentException || ex is ResourceDoesntExistException)
    statusCode = HttpStatusCode.BadRequest;
else if (ex is UnauthorizedAccessException)
    statusCode = HttpStatusCode.Forbidden;
else if (ex is CustomerNotFoundException)
    statusCode = HttpStatusCode.NotFound;

and then return the status code with an error object:

return new ErrorResponse
{
    Error = new ErrorDescription
    {
        Message = ex.Message,
        Type = ex.GetType().Name
    }
};

Advantages I see by using this approach:

  • In the program, we don't have to care if we expose a REST service or a SOAP service or whatever. Simply throw an exception, it will be handled correctly later.
  • The caller gets enough and correct information if something goes wrong (as long as the exceptions have meaningful names and information).
  • Logging can also be centralised. All unhandled exceptions will be logged right where we convert them into error responses.

Disadvantages:

  • It feels a little "hacky".
  • ?

What is the correct way to do this?


Edit: Since it was requested, here is what I do for gRPC services, where the error mapping is quite similar:

private RpcException GenerateRpcException(Exception ex)
{
    var statusCode = StatusCode.Unknown;

    if (ex is ArgumentException || ex is ResourceDoesntExistException)
        statusCode = StatusCode.InvalidArgument;
    else if (ex is UnauthorizedAccessException)
        statusCode = StatusCode.PermissionDenied;
    else if (ex is CustomerNotFoundException)
        statusCode = StatusCode.NotFound;

    var status = new Status(statusCode, ex.Message);
    var exceptionName = ex.GetType().Name;

    var rpcMetadata = new Metadata
    {
        { "exception_name", exceptionName }
    };
    return new RpcException(status, rpcMetadata);
}
  • Does gRPC works over HTTP? – Laiv Feb 7 '18 at 19:50
  • @Laiv It works over HTTP/2. It has other error codes, but the mapping is similar. – CommonGuy Feb 7 '18 at 20:46
  • Why are you changing the protocol semantics? Will the gRPC client know what 404 means? Or 500? Doesn't gRPC has its own Rpcexceptions, so that you don't need to do this conversion? – Laiv Feb 7 '18 at 21:00
  • @Laiv I map our exceptions to RpcExceptions, where you can set a StatusCode. I don't use HttpCodes there. – CommonGuy Feb 7 '18 at 21:02
  • Ok. I think the question is clear but the code is confusing. It would be good if you could contextualize the code. Could you add an example of code causing the exception, the exception handler and the conversion to the Rpcexception? No need a whole class, just the key code of each element in play. Understaing what you want to achieve is important for us to say whether your mapping is improvable (or not) and why. – Laiv Feb 7 '18 at 21:33
5

There is nothing wrong with normalizing exceptions and handling them properly. Your class ErrorResponse seems to be the right approach for handling a call to your REST service.

If there is something here which could be improved, it's a static error handling approach. The list of possible exceptions/error codes could easily grow tomorrow and your if else is going to grow to be monstrous at this rate.

Consider creating a ErrorManager class which holds a list of ErrorHandler interfaces with two methods: bool CanHandle(Exception) and ErrorResponse Handle(Exception)

Your ErrorManager when given an Exception will stupidly check each registered ErrorHandler with a call to CanHandle with the exception. When it returns true, the check stops and you return your ErrorHandler from calling your handler's Handle method. You then create an ErrorHandler for each possible ErrorResponse (and not necessarily for each exception).

This dynamic approach gives you the flexibility to programmatically register the ErrorHandler classes, or load it from a database if you so choose. More importantly than this, if you decide to programmatically register the ErrorHandler classes now, should you decide to change how you load these handlers, you can do so with little to no impact on your program as is. In essence, you're future-proofing your error handling.

A word to the wise: have a plan for when you find no handler for an exception. This is probably best left to be a HttpStatusCode.InternalServerError. While you could create a handler which "always handles" exceptions, I would advise against it, since technically we're talking about managing known possible exceptions and any exception which isn't expected is an unknown exception and should most certainly be red flagged and not merely treated like any other error.

1

I like your approach, except that I would also translate what goes to the client in the body of the error response. As you say you don't have control over the 3rd party libraries, you also don't know if they are going to leak sensitive information to the client. Better safe than sorry: log the exception in your internal logs, and propagate just the minimum information that the client needs to know about the error.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.