Smells in exception handling: how to separate between public and internal exceptions?

Question

During the development of a small web API, we decided to separate internal exceptions from public exceptions. Public exceptions are HTTP exceptions, that translate into HTTP responses with proper status code (e.g: BadRequestHttpException, NotFoundHttpException, etc). Internal exceptions are either exceptions thrown from adapters in our system (cache layer, persistence layer, etc.) or the domain (InvalidUserException, BadPasswordException, etc.).

This has been great so far. However, our controllers are starting to smell really bad:

    try {
        securityService.login(credentials);
    } catch (InvalidOAuthProviderException e) {
        throw new BadRequestHttpException(e.getMessage(), e.getCode());
    } catch (UserNotFoundException e) {
        throw new NotFoundHttpException(e.getMessage(), e.getCode());
    } catch (InactiveUserException e) {
        throw new NotFoundHttpException(e.getMessage(), e.getCode());
    } catch (InvalidOAuthTokenException e) {
        throw new ForbiddenHttpException(e.getMessage(), e.getCode());
    }

The HttpException constructor signature is the error message and code. This will result in a JSON body: {"message":"","code":222}. The code is the unique error code for doc reference. The actual response HTTP code is defined by the type of the exception.

This is clearly not a good sign and i'm sure there's something wrong in the design here. Any help?

Answer 1

You are handling errors across the boundary between two systems, effectively, so its best to think of the http handler as a system and the internal logic that throws these internal exceptions as a library.

Given you want to catch these at the boundary point, and you do not want to map internal to external exceptions, then you're left with a simple catch-all (or a couple of catch-most) handlers. The client doesn't want to know about the internal exception, so catching everything from a login request and returning a forbidden or bad request exception is the way to go, considering you include the error message from the internal exception, it doesn't really matter what the internal exception was as far as the client interface is concerned - something went wrong, it was either a bad user or some server error.

In most cases, you should be catching the internal errors and performing some other form of handler to them - like logging them or raising an admin alarm, and then returning a more generic "it didn't work" exception to the client. The only difference here is for exceptions you know are 'expected' such as login failure due to a bad password.

Answer 2

You can create reusable utility class that will resolve correct HttpException.

try {
        securityService.login(credentials);
    } catch (InternalException e) {
        throw ExceptionUtil.resolveHttpException(e);
    }      

public class ExceptionUtil {
  public static HttpException resolveHttpException(InternalException exc) {
     if (exc instanceOf InvalidOAuthProviderException) 
        return new InvalidOAuthProviderException(e.getMessage(), e.getCode()
     else if (exc instanceOf AnotherInternalException)
        return new  AnotherInternalException;
      ...
    }

}

Then you can use it across all application without code redundancy.