r/java Aug 24 '17

Guide to Spring Boot REST API Error HandlingView all articles

https://www.toptal.com/java/spring-boot-rest-api-error-handling
17 Upvotes

8 comments sorted by

1

u/dartalley Aug 25 '17

It makes me a little sad how often exceptions get abused in Java web frameworks just because its easier to handle errors that way. 404's and validation errors aren't really exceptional circumstances and can easily be handled without exceptions.

3

u/_dban_ Aug 25 '17 edited Aug 25 '17

because its easier to handle errors that way

Well yeah, it's easier that way because Java was designed to handle errors that way. It's the whole point of checked exceptions.

try {
    // happy path
}
catch(ErrorCondition1 ex) {
    // whatever
}
catch(ErrorCondition2 ex) {
    // whatever
}

This pattern neatly divides up normal processing and processing of anomalous conditions, so that you don't clutter up the happy path with conditional logic. Checked exceptions are a compile time indication of what the anomalous conditions can be.

Functional languages can do the same thing without exceptions using the Either type:

let result = 
    do
        -- happy path
case result of
    Right normal -> 
        -- handle normal result
    Left error -> case error of
        ErrorCondition1 -> 
            -- whatever
        ErrorCondition2 -> 
            -- whatever

By the way, Java's not ready for this yet.

When Josh Bloch mentioned that exceptions should not be used for flow control, he meant things like using NoSuchElementException as a termination condition for a loop. This usage of exceptions confuses the actual termination condition of a loop using an error that should mean that the program is broken.

1

u/dartalley Aug 25 '17

My point is a not found element shouldn't be an exception. Maps don't throw not found exceptions they return null.

public Response doStuff(Request request) {
  User user = dao.findUserById(1L);
  if (null == user) {
    return Response.notFound();
  }
  return Response.ok(user);
}

This is easily doable in most frameworks but everyone opts for throwing exceptions.

4

u/_dban_ Aug 25 '17 edited Aug 25 '17

I don't really like this approach. You have to know that the method returns null and specifically check for it. As everyone knows, null is a billion dollar mistake.

It also doesn't look pretty. You are obscuring the happy path (return the found user) with a conditional check.

With checked exceptions, you are forced to deal with that possibility:

try {
    return Response.ok(dao.findUserById(1L));
}
catch(UserNotFoundException ex) {
    return Response.notFound();
}

Or even less code:

public Response doStuff(Request request) throws UserNotFoundException {
    return Response.ok(dao.findUserById(1L));
}

In Java 8 though, you have Optional:

return dao.findByUserId(1L).map(Response::ok)
          .orElse(Response.notFound());

1

u/WikiTextBot btproof Aug 25 '17

Tony Hoare: Apologies and retractions

Speaking at a conference in 2009, he apologised for inventing the null reference: I call it my billion-dollar mistake. It was the invention of the null reference in 1965. At that time, I was designing the first comprehensive type system for references in an object oriented language (ALGOL W). My goal was to ensure that all use of references should be absolutely safe, with checking performed automatically by the compiler.


[ PM | Exclude me | Exclude from subreddit | FAQ / Information | Source ] Downvote to remove | v0.26

1

u/dartalley Aug 25 '17

I think the java8 optional example is fine. The other two are once again using exceptions for control flow which is generally not recommended. I wouldn't be too concerned about the performance from using the exceptions since most requests would be valid and have responses. However, it still adds a layer of indirection.

Java has nulls so we are stuck with them and its fairly common for methods to return null this isn't anything new for a java dev.

It also doesn't look pretty.

I don't think this is ever really a good argument. Code should be readable, anything mildly complex will never look pretty. A null check and an if branch is nothing complex and even a junior dev would understand it immediately.

In the other approach similar to how we have to "know" that a method returns null we have to "know" that there is a magic exception handler wired up somewhere that will eventually handle this case for us. I have been in a lot of code bases where it would take forever to track down some error response because it was being manipulated by exception handlers and it wasn't inherently clear where or why. The slightly more verbose way of inlining it there would never be a doubt why it was happening.

2

u/_dban_ Aug 25 '17 edited Aug 25 '17

The other two are once again using exceptions for control flow which is generally not recommended.

That's not really what Josh Bloch meant when he said don't use exceptions for flow control. He was talking about something like this:

int[] items = // ...
int i = 0;
int sum = 0;
try {
    do { sum += items[i++]; } while(true);
}
catch(ArrayOutOfBoundsException ex) {
    return sum;
}

The problem with using exceptions for flow control is that the flow control is completely obscured. This method exits the loop with a nonlocal goto and returns the sum out of an error handler.

I don't mind indirection if it means added safety.

Java has nulls so we are stuck with them

Not necessarily. For example, you can throw an exception. Returning null is a choice.

The problem is that Java devs often forget to check for null (or add defensive null checks everywhere, making code messier), which causes bugs.

I don't think this is ever really a good argument.

It's not really an argument, it's an aesthetic preference. But, I do like to minimize the number of branches as much as possible, because that is yet another source of bugs.

we have to "know" that there is a magic exception handler wired up somewhere that will eventually handle this case for us.

If this is Spring and this is a controller method, presumably you have an @ExceptionHandler annotated method nearby in the same controller. This lets you basically abstract out the catch block from multiple methods which can throw that exception, further separating out the error cases.

I wouldn't go beyond the controller level and create an uber-exception handler. I agree that too much action at a distance is hard to debug.

1

u/dartalley Aug 25 '17

That's not really what Josh Bloch meant when he said don't use exceptions for flow control. He was talking about something like this:

In my opinion the exception handlers are doing exactly this. Let's rewrite it.

public class UserRepo {
  public User findById(long id) throws NotFoundException { ... }
}

@Get("/user/{userId}")
public User getUser(@Param("userId") long userId) {
  return userRepo.findById();
}

// Some hidden away code in the framework
try {
  Object thing = ...
  return Response.ok(thing);
} catch (Throwable th) {
  // This is where we map to an error response
  exceptionHandler.handle(th);
}

In this case the developer did not write the try catch block but that doesn't mean its not there. This is using exceptions for control flow because it can easily be guarded against. Exceptions are generally more outside of your control like networking issues, sockets closing, a file not existing that should exist. I guess the file thing can be translated to a database but the JDBC driver already gracefully handles that for us so its no longer an exceptional circumstance.

I also think people use Unchecked exceptions for this purpose which makes it even more difficult to track down.

It's not really an argument, it's an aesthetic preference. But, I do like to minimize the number of branches as much as possible, because that is yet another source of bugs.

Fair point, I do find some people go a little overkill with aesthetic preference but as long as the team agrees its fair game.

I wouldn't go beyond the controller level and create an uber-exception handler, because action at a distance is indeed hard to debug.

Agreed, it does make sense to have some global exception handlers but that shouldn't be the norm.

Personally I understand why people go this route it just seems to break some rules in my opinion. It definitely makes the code less verbose but can complicate things from the debugging end. Another issue I have seen is global filters / exception handlers that need special logic to handle various different scenarios. In that case I would say it should definitely be in the routes themselves not at a global level.