Javalin: Exception handler trumps 404 error handler since Javalin 2.5.0

[dwestheide]

Actual behavior (the bug)
Since version 2.5.0, when I define an exception handler as well as an error handler for status code 404, a 500 response as created by the exception handler is returned. The error handler does not seem to have any effect.

Expected behavior
When I register both an exception handler and an error handler for status code 404, 404 responses are not transformed into 500 responses. This is the behaviour on Javalin 2.4.0 and earlier versions.

To Reproduce
Here is a small demo application:

import io.javalin.Javalin;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class Main {

    private static final Logger log = LoggerFactory.getLogger(Main.class);

    public static void main(final String[] args) {
        final var server = Javalin.create()
                .port(8000)
                .error(404, ctx -> ctx.result("NOT FOUND!"))
                .exception(Exception.class, (e, ctx) -> {
                    log.error("Exception was thrown", e);
                    ctx.result("BOOM!");
                })
                .get("/hello", ctx -> ctx.result("WORLD!"))
                .start();
        Runtime.getRuntime().addShutdownHook(new Thread(server::stop));
    }

}

In 2.4.0, a request to http://localhost:8000/doesnotexist returns a 404 with the response body NOT FOUND!.

In 2.5.0, the same request returns a 500 with the response body BOOM!.

[tipsy]

Thanks @dwestheide, it's not supposed to behave like that. Would you like to create a PR?

[dwestheide]

At the moment, I haven't had time yet to dig into the sources and find out what exactly has changed, but I can give it a try. If you have any idea on which of the recent changes between 2.4.0 and 2.5.0 might have caused this, please let me know. Otherwise, I will just have to step through each of the commits.

[tipsy]

I suspect it's #436, I can check when I get home.

[dwestheide]

Thanks!

[dwestheide]

So, I wrote a few regression tests to clarify my expectations. You can find them here (not opening a PR yet): https://github.com/dwestheide/javalin/commit/846def9a2a2c33497d066346dcec4c75bc9166b7

Only the first two are failing, the third one, where I return an 404 explicitly from a route, is working fine. So I think the problem is that when a route doesn't exist at all, the router probably throws a NotFoundResponse exception, whereas a route calling ctx.status(404) wouldn't lead to an exception. With the changes from #436, an exception mapper for a generic type like Exception will also take effect in the case of a missing route. I wonder if it's possible to fix this without causing another breaking change for the behaviour intended by #436.

[tipsy]

I think the problem is that when a route doesn't exist at all, the router probably throws a NotFoundResponse exception, whereas a route calling ctx.status(404) wouldn't lead to an exception.

Yes, that's correct.

I wonder if it's possible to fix this without causing another breaking change for the behaviour intended by #436.

I think it should be enough to check if a user has defined a mapper for HttpResponseException? The default behavior should be that a general exception mapper doesn't interfere with the XyzResponse exceptions.

[tipsy]

@dwestheide so I re-introduced noUserMapperFound, but slightly rewritten:

private fun noUserMapperFound(exception: Exception): Boolean =
        this.exceptionMap[exception.javaClass] == null && this.exceptionMap[HttpResponseException::class.java] == null

I also removed a confusing performance improvement:

And added this as the first branch of the when:

HttpResponseExceptionMapper.canHandleException(exception) && noUserMapperFound(exception) -> HttpResponseExceptionMapper.handleException(exception, ctx)

All the tests pass, but it's a bit messy and hard to read.

[dwestheide]

Thanks a lot for already coming up with a solution, @tipsy. It's a bit difficult to read, but not too much in my opinion.

[tipsy]

Thank you for reporting the bug, this would have broken my applications too if I had upgraded 😅

I also refactored a bit to make things more readable. Could you do a quick post-merge review of https://github.com/tipsy/javalin/commit/3a99864a995b3403e25cef36c2f39f43de0f50fd ?

[dwestheide]

Thanks a lot for fixing this so quickly! I did a post-merge review of the linked commit, and to me, it looks fine. :+1:

[tipsy]

You're very welcome. This was a pretty serious bug, so I'll probably release a new version soon.

[tipsy]

This was released as part of 2.6.0 now: https://javalin.io/news/2019/01/17/javalin-2.6.0-released.html

[dwestheide]

Thank you very much for putting out a release with a fix so quickly. You rock! :clap: