Javalin: Reconsider exception-handlers

[tipsy]

Currently, Javalin will only run the closest matching exception handler. From the docs:

app.exception(NullPointerException.class, (e, ctx) -> {
    // handle nullpointers here
});

app.exception(Exception.class, (e, ctx) -> {
    // handle general exceptions here
    // will not trigger if more specific exception-mapper found
});

This could be a bit annoying to deal with, especially when developing plugins (as pointed out by @jkschneider in https://github.com/tipsy/javalin/pull/959)

Should multiple exception handlers be allowed to run per exception?

Should we have something like next()/done() ?

This would be a breaking change, but I guess we could also make it configurable.

[jkschneider]

One thing to add here... the default exception handler yields an HTTP 200. How should exception handlers be ordered such that different handlers affecting ctx in different ways yield predictable behavior?

[MrThreepwood]

For the projects we've done we've always handled multiple expected exceptions that parse the exception into a specific response and status, and one that is a fall back in case no better exception would be handled (which turns into a 500, sorry something went wrong).

It seems reasonable to think most of these handlers would be handled separately, I would have been very surprised if multiple handlers were called when a specific exception (like Unauthorized) was called. Especially if some of those were expensive. The way I've been looking at the exception handlers are: An exception ocurred, decide what response should be sent to the client. It doesn't really make sense to ask multiple functions what response should be provided, but maybe I'm thinking about them differently than others do?

Maybe it would be easier to have a separate function for registering handlers that should run on all exceptions? Something like app.notifyOnException(Exception.class, (e, ctx) ->{})?

[jkschneider]

Something like app.notifyOnException(Exception.class, (e, ctx) ->{})

This seems very reasonable!

[tipsy]

I like the idea of adding a separate method, but it would be nice if it was named something like exceptionXyz to group it with the exception method. Any ideas?

[jkschneider]

exceptionListener? Looking to other frameworks, I think onException, doOnException, etc. are popular choices as well.

[MrThreepwood]

I'm not sure how best to make it clear to a user that one will be called all the time, and the other will only be called if something more specific is not called.

I do like listener. I also think that ideally things bound with the current exception() should probably be called after anything bound with this new method?

Do after filters run after the exception handlers? I assume they do. Since the goal of this new functionality is primarily for metric tracking/some other thing you want to do on every exception that's not necessarily handling the response, would it maybe be simpler/more obvious intent to just add a flag on context that signals an error occurred (and possibly holds the original exception?) that Javalin sets (internal setter) before handing to the exception handlers?

This seems like it would then allow @jkschneider to do what he was hoping to do by adding an after filter and checking for the existence of an exception without adding a second function whose purpose/intent may be somewhat confusing with the existing function.

[jkschneider]

I also think that ideally things bound with the current exception() should probably be called after anything bound with this new method?

There are probably cases to suggest this should work both ways. From my perspective as a metrics instrumentation library author, I would prefer listeners to be called after anything that currently implements exception(..). Specifically, this is because my listener would be completing a timing operation on a REST endpoint, and I want to be able to tag this telemetry with the HTTP status code, which can be influenced by a traditional exception handler.

[MrThreepwood]

That makes sense, I was thinking I'd like things using exception() to have the final say in the output as having multiple listeners be handed the context and allowed to change things could make things difficult to track down, but I suppose people can choose to use it how they like and it does make sense to have a listener that's implemented largely for metric tracking/events to happen last.

That does make it sound even more like an after handler though.