Add JAX-RS exception mapper for IllegalArgumentException

[bernd]

The mapper converts IllegalArgumentExceptions to a 400 Bad Request response.

/nocl

[bernd]

@dennisoelkers @Graylog2/architecture Can you think of any situations where this wouldn't be a good idea?

[dennisoelkers]

@bernd: Right now it would just end up as a 500 through the AnyExceptionClassMapper, right? I am not aware of any place where it would break things when we return a 400 instead of a 500 (and if that is so, that should be fixed asap anyway).

[bernd]

@bernd: Right now it would just end up as a 500 through the AnyExceptionClassMapper, right?

Aye, the catch-all mapper would catch it and return a 500.

[bernd]

Looks straightforward, of course not every unhandled IAE is actually a bad request, but I think there's no universal answer anyway.

Yeah, good point. I am coming from a DB service context where we often throw IllegalArgumentException which are often Bad Request errors. Having a handler for those would save a lot of exception handling in REST resources. :thinking:

[kroepke]

I think mapping this is reasonable, ideally, we don't let exceptions bubble up all the way, but if they do 400 is no more wrong than 500 :)

[thll]

I think it makes sense, but we should document it in our upgrade notes. Client behavior might be affected, as they could have retried responses with a 5xx status, while they usually won't retry 400s. It shouldn't make a big difference in practice, though, because if the 500 was, in reality, a client error, then retrying wouldn't have helped.

[boosty]

Sounds reasonable, as long as practically all unhandled IllegalArgumentException cases are indeed caused by bad client requests? I assume that this is the case. Otherwise we would accidentally "punish" the client with a 400, when there is nothing to change on the client side. This could be misleading.

[bernd]

@boosty @dennisoelkers @kroepke @thll I added an alternative implementation with annotation-based explicit exception mapping per resource class.

This would avoid unintended behavior changes and still make it relatively easy to set up exception handling. The advantage is that the explicit approach supports mapping multiple exceptions to response statuses.

See the HelloWorldResource example change in the diff.

What do you think?

[kroepke]

What is the intended purpose of these? I can see that certain exceptions always mean a certain response code (e.g. downstream service failures, NPEs etc will be 50x), validation errors will always be bad requests etc. Annotation one resource (or resource class) with exception mapping sounds like it can quickly become inconsistent, so I'm feeling like I'm missing context for what we want to solve.