Skip to content
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

Default Dropwizard-added ExceptionMappers to be configurable #710

Closed
kschulst opened this issue Sep 19, 2014 · 16 comments
Closed

Default Dropwizard-added ExceptionMappers to be configurable #710

kschulst opened this issue Sep 19, 2014 · 16 comments
Labels
Milestone

Comments

@kschulst
Copy link

Dropwizard by default adds jersey exception mappers. There have been various discussions related to how these can be disabled, like:
https://groups.google.com/forum/#!searchin/dropwizard-user/exception$20mappers/dropwizard-user/CgPjcY4SLFg/xSDzOCvyx0gJ

A workaround is described here:
http://thoughtspark.org/2013/02/25/dropwizard-and-jersey-exceptionmappers/

This all worked great with jersey 1. However, with jersey 2 there are no way (that I have found) to "deregister" exception mappers once they have been registered. The jersey provider singletons (in org.glassfish.jersey.server.ResourceConfig) are wrapped in an unmodifiable Set which results in an exception being thrown if attempting to use the approach referenced above.

The motivation for removing/overriding dropwizard exception mappers can of course be debated, but we have currently been doing so in order to have full control over what is being used. Since the underlying jersey implementation uses a Set to hold the exception mappers and the iteration order of a Set cannot be guaranteed, then it has been the safest bet to just remove all dropwizard exception mappers to guarantee that our custom ones are being used.

Therefore, I propose a feature request: Make it possible in dropwizard configuraton to override the way exception mappers are being added.

Would it be interesting if I created a pull request for something like this? And if so, would it be possible to get this into the upcoming 0.8.0 relase if I move quickly?

@kschulst kschulst changed the title Default Dropwizard-added ExceptionMappers to configurable Default Dropwizard-added ExceptionMappers to be configurable Sep 19, 2014
@wirde
Copy link
Contributor

wirde commented Sep 19, 2014

FWIW - I would really like to have this ability too.

@bjorntorewiken
Copy link

+1

@mcarrierastonish
Copy link
Contributor

+1, if you need a hand let me know.

@nottix
Copy link

nottix commented Sep 19, 2014

+1

1 similar comment
@wangyingang
Copy link

+1

@skjegg
Copy link

skjegg commented Oct 24, 2014

+1 . If Jersey 2 doesn't allow for unregistering exception mappers, yet retains the utterly broken un ordered Set implementation, it could make moving to 0.8 difficult for us.

@dbyron0
Copy link

dbyron0 commented Nov 18, 2014

Doesn't the code here: http://grepcode.com/file/repo1.maven.org/maven2/com.sun.jersey/jersey-server/1.17.1/com/sun/jersey/api/core/DefaultResourceConfig.java?av=f#62 mean that iterating mappers happens in a predictable/stable order? If so, then isn't the bug elsewhere? I still totally support being able to override the built-in mappers.

@joschi
Copy link
Member

joschi commented Nov 19, 2014

@dbyron0 The source code you've come up with is from Jersey 1.17.1. We're using Jersey 2.13 in Dropwizard 0.8.x.

@mikeycmccarthy
Copy link
Contributor

+1

@cmicali
Copy link
Contributor

cmicali commented Nov 21, 2014

+1 - This really needs to be configurable.

Here is my current hack solution for Jersey 2.x / dw 0.8:
https://gist.github.com/cmicali/2ee7b3b6941005dc5596

@joschi I'm happy to do this work but interested in your thoughts on how to make this configurable. There is no configuration going into Environment/JerseyEnvironment currently.

@GreyTeardrop
Copy link

The problem with exception mappers in Jersey 1.x is caused by ExceptionMapperFactory class, as it stores all exception mappers in HashSet and has unpredictable iteration order (even though complete set of singletons in DefaultResourceConfig has predictable ordering).

However, Jersey 2.x replaces that HashSet with LinkedHashSet. It might resolve exception mappers ordering issue for Dropwizard 0.8, but I haven't yet had a chance to check that.

@cmicali
Copy link
Contributor

cmicali commented Nov 24, 2014

I made this configurable with pull request #793

I made this a configuration property on AbstractServerFactory so you just add the following to your server.yml

server:
    registerDefaultExceptionMappers: false

FWIW this is a blocking issue for us to migrate to 0.8

@mikeycmccarthy
Copy link
Contributor

Great hack, cheers @cmicali . Will give that one a go today.

@cmicali
Copy link
Contributor

cmicali commented Nov 25, 2014

@mikeycmccarthy It worked well enough for us to get going with the 0.8 transition, but beware the hack is not 100%. I think the DW exceptionmappers are still being cached somewhere and thanks to the non-deterministic set they are stored in 1 in 10 times they take precedence over your versions.

So, good enough to unblock development but not suitable for production. #793 is the "right" way to handle this.

@fzakaria
Copy link

Honestly why even have LoggingExceptionMapper ?
It seems like it can be replaced with a Filter and achieve the same functionality ?
See : https://jax-rs-spec.java.net/nonav/2.0/apidocs/javax/ws/rs/container/ContainerResponseFilter.html

Why don't we just remove it...

@vaamyob
Copy link

vaamyob commented Dec 19, 2014

It's been 4 months since this issue was opened. I'm surprised @codahale hasn't responded, since overriding the default DW behavior seems like it would be one of the first things a new project would want to do. How else are you going to get ConstraintViolationExceptions to the client?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests