Add new StandardExceptionMappers#register method that doesn't use reflection #426
Closed
sleberknight
started this conversation in
Ideas
Replies: 2 comments
-
Initially we might also want to annotate this with |
Beta Was this translation helpful? Give feedback.
0 replies
-
This is fixed by #479 |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
We obviously missed it, but Dropwizard has allowed you to override the default exception mappers for a long time (1.0.0 I'm pretty sure).
We therefore no longer need to use reflection on a
ServerFactory
to invokesetRegisterDefaultExceptionMappers
with a value offalse
. The original reason for doing this was to avoid having to put explicit configuration in every one of ourconfig.yml
files.Instead, we can simply register our own exception mapper classes "that implements the same
ExceptionMapper<T>
as one of the default".This means we don't need to require a
ServerFactory
argument, so the new method signature is simply:We need to verify this behavior, especially for
LoggingExceptionMapper
since it has (at the time of this writing) sub-classes (two are in Dropwizard Jersey and another two are in Dropwizard JDBI3, and there may be more in other modules). Dropwizard uses theExceptionMapperBinder
to register its default versions. It binds anew LoggingExceptionMapper<Throwable> {}
(anonymous class instance) and then later binds the sub-classes. The question is how the overrides work, e.g.,StandardExceptionMappers
registers justnew LoggingExceptionMapper<> {}
. This works currently since it disables all the default Dropwizard exception mappers. But to properly override, do we need to exactly match the one inExceptionMapperBinder
which usesLoggingExceptionMapper<Throwable>
?Assuming the overrides work as expected, then the question is whether to deprecate the existing StandardExceptionMappers#register method.
One argument for deprecation is that it is no longer necessary since you can override Dropwizard's default ones. An argument against is that certain applications may still want to ensure Dropwizard does not register any of its own exception mappers for complete control over which ones are registered. I originally thought we should deprecate it for the reason mentioned above, but after thinking more about it, I am leaning against that, again so that we can choose the best option.
Beta Was this translation helpful? Give feedback.
All reactions