New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finer-grain control of exception behaviour in view renderers #1820

Merged
merged 4 commits into from Nov 18, 2016

Conversation

Projects
None yet
6 participants
@acwwat
Contributor

acwwat commented Nov 15, 2016

This PR is one possible implementation of issue #1811. In short, I am standardizing the exception thrown by each ViewRenderer implementation as ViewRenderException, which is then caught and rethrown as WebApplicationException by ViewMessageBodyWriter.

To preserve previous behavior of returning an HTML error response on render exceptions, I have added a new ViewRenderExceptionMapper class which should be registered. Consequently I am still changing the original behavior by adding this registration requirement. Let me know if this is OK from backward-compatibility standpoint - if needed we can register the mapper in ViewBundle, although I don't know how I can register another exception mapper afterwards if I want custom behaviour. This also requires change to the documentation.

Otherwise, one can choose to provide their own ExceptionMapper to catch WebApplicationException and check that the cause is ViewRenderException, then do whatever is needed with its cause in the form of an exception thrown by the rendering code (Viewmarker, Mustache, etc.)

I tried updating the tests to verify both cases of with and without ViewRenderExceptionMapper, but I can't seem to get registration in each test case working like this:

target("/test/bad").regsiter(new ViewRenderExceptionMapper()).request().get(String.class);

I had to register the mapper in configure(). Please let me know if you have any solution to this so I can increase the test coverage.

Thanks.

@coveralls

This comment has been minimized.

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-0.1%) to 81.804% when pulling 46a0a14 on acwwat:view-exc into 867a101 on dropwizard:master.

@jplock jplock added this to the 1.1.0 milestone Nov 16, 2016

@jplock jplock added the improvement label Nov 16, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 16, 2016

I think it would be worthwhile to have a test in dropwizard-e2e/src/main/java/com/example/app1 showing that the behavior can be changed.

@acwwat

This comment has been minimized.

Contributor

acwwat commented Nov 17, 2016

@nickbabcock I added a test/example to e2e on overriding the standard 503 response on missing mustache view with a 404 response.

@coveralls

This comment has been minimized.

coveralls commented Nov 17, 2016

Coverage Status

Coverage decreased (-0.09%) to 81.814% when pulling 534205f on acwwat:view-exc into 867a101 on dropwizard:master.

@@ -82,12 +76,9 @@ public void writeTo(View t,
}
}
throw new ViewRenderException("Unable to find a renderer for " + t.getTemplateName());
} catch (Exception e) {
} catch (ViewRenderException e) {
LOGGER.error("Template Error", e);

This comment has been minimized.

@dren-dk

dren-dk Nov 17, 2016

Contributor

My preference would be to format the log message in my own ExceptionMapper, so could this go into the ExceptionMapper as well?

This comment has been minimized.

@acwwat

acwwat Nov 18, 2016

Contributor

That makes sense. I've removed the logging from ViewMessageBodyWriter and added the logging code to ViewRenderExceptionMapper to preserve the original exception logging behavior.

@coveralls

This comment has been minimized.

coveralls commented Nov 18, 2016

Coverage Status

Coverage decreased (-0.1%) to 81.794% when pulling 6d3a467 on acwwat:view-exc into 867a101 on dropwizard:master.

@arteam

This comment has been minimized.

Member

arteam commented Nov 18, 2016

The pull request looks good to me. I don't think the code coverage is so important, what matters you provided relevant tests. And thank you very much for a very detailed documentation for ViewRenderException!

@arteam arteam merged commit 7e1f4b0 into dropwizard:master Nov 18, 2016

arteam added a commit that referenced this pull request Nov 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment