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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bind default exception mappers #1768

Merged
merged 3 commits into from Oct 11, 2016

Conversation

Projects
None yet
5 participants
@nickbabcock
Contributor

nickbabcock commented Oct 7, 2016

Currently when a user wants to override one of the default exception mappers they must specify registerDefaultExceptionMappers: false in their configuration, register their exception mapper, and then re-register all the other default exception mappers they want. This is not user friendly. A user should be able to register their own exception mapper, override the default exception mapper while preserving other default exception mappers.

It should be possible to achieve this behavior as alluded to in JERSEY-2722

your own ExceptionMapper should always have a higher priority than anything provided by Jersey

and alluded in Jersey's Bean Validation code

// Custom Exception Mapper and Writer - registering in binder to make possible for users register their own providers.
bind(ValidationExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
bind(ValidationErrorMessageBodyWriter.class).to(MessageBodyWriter.class).in(Singleton.class);

This pull request takes a crack at introducing this to Dropwizard, while remaining 100% backwards compatible. I've tried creating this behavior before in #1144, but it didn't work out, so I'd appreciate some help testing this 馃槃. I believe it is working as I have an app that has:

@Override
public void run(final DropwizardSampleConfiguration configuration,
                final Environment environment) {
    environment.jersey().register(new Resource());
    environment.jersey().register(new MyJerseyException());
}


public class MyJerseyException implements ExceptionMapper<JerseyViolationException> {
    @Override
    public Response toResponse(JerseyViolationException e) {
        return Response.ok("Oh yeah").build();
    }
}

and I receive the "Oh yeah" response with every restart and request (100 requests across 10 restarts).

We can even keep around the registerDefaultExceptionMappers for users who want no default exception mappers registered, but I'm assuming most users will not use this option and opt for overriding specific exception mappers.

The major benefit to this approach is the ability to add new exception mappers without having the relatively large cost of forcing the user to know them all 馃構

TODO:

  • Add tests
  • Update docs
  • To use the provider annotation or not?

@nickbabcock nickbabcock force-pushed the nickbabcock:rethink-exception-mappers branch from 16b5c81 to 2a755e9 Oct 7, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 7, 2016

Coverage Status

Coverage increased (+0.01%) to 82.276% when pulling 2a755e9 on nickbabcock:rethink-exception-mappers into 72fba53 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented Oct 7, 2016

Coverage Status

Coverage increased (+0.008%) to 82.274% when pulling 2a755e9 on nickbabcock:rethink-exception-mappers into 72fba53 on dropwizard:master.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 8, 2016

Added the test case. Writing it made me kinda wish that we had a dedicated spot for integration tests, and I didn't want to pile on additional tests in dropwizard-example 馃槙

@coveralls

This comment has been minimized.

coveralls commented Oct 8, 2016

Coverage Status

Coverage increased (+0.01%) to 82.276% when pulling 5a2c793 on nickbabcock:rethink-exception-mappers into 72fba53 on dropwizard:master.

@jplock jplock added the improvement label Oct 8, 2016

@jplock jplock added this to the 1.1.0 milestone Oct 8, 2016

@jplock

This comment has been minimized.

Member

jplock commented Oct 8, 2016

@jplock

jplock approved these changes Oct 8, 2016

This looks like a great simplification, thanks!

import javax.ws.rs.ext.ExceptionMapper;

public class ExceptionMapperBinder extends AbstractBinder {
private boolean showDetails;

This comment has been minimized.

@jplock

jplock Oct 8, 2016

Member

nitpick: this could be final

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 8, 2016

Yeah, imo I should update the docs in this PR too

@nickbabcock nickbabcock force-pushed the nickbabcock:rethink-exception-mappers branch from 5a2c793 to 20ddc24 Oct 8, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 8, 2016

Coverage Status

Coverage increased (+0.01%) to 82.276% when pulling 20ddc24 on nickbabcock:rethink-exception-mappers into 7e9927c on dropwizard:master.

- ``JsonProcessingExceptionMapper``
- ``EarlyEofExceptionMapper``
To override a specific exception mapper, register your own class that implements the same
``ExceptionMapper<T>`` as one of the default. For instance, we can customize responses caused by

This comment has been minimized.

@evnm

evnm Oct 8, 2016

Member

Are the defaults clearly documented anywhere?

This comment has been minimized.

@nickbabcock

nickbabcock Oct 10, 2016

Contributor

Yeah, couple sentences later

See the class ExceptionMapperBinder for a list of the default exception mappers.

Which is similar to the other snippets in the documentation.

This comment has been minimized.

@evnm

evnm Oct 10, 2016

Member

Whoops. Shoulda kept reading. :-X

@davidmankin

This comment has been minimized.

davidmankin commented Oct 10, 2016

Thank you. I've wanted this feature several times!

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 11, 2016

Looks like even Jersey code doesn't standardize on whether to annotate exception mappers with @Provider, so as far as I'm concerned this is ready for further review/merge.

@jplock

This comment has been minimized.

Member

jplock commented Oct 11, 2016

Looks good to me!

@jplock jplock merged commit e82f1a8 into dropwizard:master Oct 11, 2016

@jplock

This comment has been minimized.

Member

jplock commented Oct 11, 2016

@nickbabcock mind updating the release notes?

nickbabcock added a commit that referenced this pull request Oct 11, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 11, 2016

@jplock done, 89ab17b 馃槃

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