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

Allow user to override configured JsonProvider #1788

Merged
merged 1 commit into from Nov 22, 2016

Conversation

Projects
None yet
4 participants
@nickbabcock
Contributor

nickbabcock commented Oct 27, 2016

I wondered if I could create a custom JsonProvider where I could read and write a special payload at the top the body (don't ask why, it's just a thought experiment):

/* I'm a special header */
{
    "check": "mate"
}

This PR allows a user configured JacksonJaxbJsonProvider or another appropriate class to take priority over the dropwizard configured class (JacksonMessageBodyProvider).

I believe this PR closes #1005

@jplock I used App1 in dropwizard-e2e, not sure if it is too early to call it a theme, but so far it is about overriding defaults 馃構

@coveralls

This comment has been minimized.

coveralls commented Oct 27, 2016

Coverage Status

Coverage decreased (-0.08%) to 82.227% when pulling 8ebdb8d on nickbabcock:bind-json-provider into 00cc42f on dropwizard:master.

@nickbabcock nickbabcock force-pushed the nickbabcock:bind-json-provider branch from 8ebdb8d to 899c201 Oct 27, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 27, 2016

Coverage Status

Coverage decreased (-0.08%) to 82.227% when pulling 899c201 on nickbabcock:bind-json-provider into 00cc42f on dropwizard:master.

@nickbabcock nickbabcock force-pushed the nickbabcock:bind-json-provider branch from 899c201 to d0f7d35 Oct 27, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 27, 2016

Coverage Status

Coverage decreased (-0.08%) to 82.227% when pulling d0f7d35 on nickbabcock:bind-json-provider into 00cc42f on dropwizard:master.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 27, 2016

Pushed a commit to fix the style issues and added explicit test case that'll close #1005

@jplock

This comment has been minimized.

Member

jplock commented Nov 16, 2016

@nickbabcock sorry, I forgot about this one. Good to merge?

@jplock jplock added the improvement label Nov 16, 2016

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

@nickbabcock nickbabcock force-pushed the nickbabcock:bind-json-provider branch from d0f7d35 to 5ac14c6 Nov 18, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 18, 2016

Rebased PR on master, this should be good to go once Travis gives the green light!

@jplock

This comment has been minimized.

Member

jplock commented Nov 18, 2016

Travis failed :( Seems like its something unrelated

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 18, 2016

Yeah maven is having some problems with their certificates

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 18, 2016

Looks like there is now a test failure with the view integration, I'll have to look into and report back.

@nickbabcock nickbabcock force-pushed the nickbabcock:bind-json-provider branch from 5ac14c6 to 668f61d Nov 20, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 20, 2016

Ok fixed previous issue by specifying that the view produces html.

I've also changed the exception mapper that looks for the view render exception. cc @acwwat and @dren-dk to get their insight and see if they prefer this approach (and if so, it'll need to be documented).

@coveralls

This comment has been minimized.

coveralls commented Nov 20, 2016

Coverage Status

Changes Unknown when pulling 668f61d on nickbabcock:bind-json-provider into * on dropwizard:master*.

@acwwat

This comment has been minimized.

Contributor

acwwat commented Nov 20, 2016

@nickbabcock I have some questions/comments about the exception mapper change.

  1. I personally don't have a preference on ExtendedExceptionMapper over ExceptionMapper and it seems to fit the use case better anyway, so the change is OK. But just wondering - is the usage of ExtendedExceptionMapper common in JAX-RS development?
  2. For the example in App1, given that the custom exception mapper is specifically for handling MustacheNotFoundException to return a 404 error, it should be checked for instead of ViewRenderException.
@acwwat

This comment has been minimized.

Contributor

acwwat commented Nov 20, 2016

@nickbabcock Oh and also, if we decide to use ExtendedExceptionMapper, maybe we should also update ViewRenderExceptionMapper that I added earlier to be more consistent.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 20, 2016

I'm not sure how common ExtendedExceptionMapper is, but it's common enough to warrant a mention in the jersey docs (emphasis mine).

[ExtendedExceptionMapper] can for example check the exception parameters and based on them return false and let other provider to be chosen for the exception mapping.

By defining an WebApplicationException mapper in dropwizard-views, it would override the WebApplicationException logic found in LoggingExceptionMapper. Which I can see as undesirable for those who were previously relying on that behavior.

It should be preferential to have exception mappers that target specific exceptions (in App1 case, MustacheNotFoundException), but since the exception thrown is a MustacheNotFoundException wrapped in a WebApplicationException I used ExtendedExceptionMapper. I realize that it is not 100% necessary, but since others may look to dropwizard-e2e or dropwizard-example for code examples I'd rather it keep unexpected behavior (eg. overriding LoggingExceptionMapper web exception behavior) to a minimum.

So yes, I think ViewRenderExceptionMapper should be updated as well.

Thank you for the code review, I've updated the code to just remap MustacheNotFoundException to 404.

@nickbabcock nickbabcock force-pushed the nickbabcock:bind-json-provider branch from 668f61d to d3b6e6a Nov 20, 2016

@coveralls

This comment has been minimized.

coveralls commented Nov 20, 2016

Coverage Status

Changes Unknown when pulling d3b6e6a on nickbabcock:bind-json-provider into * on dropwizard:master*.

@jplock

This comment has been minimized.

Member

jplock commented Nov 21, 2016

This looks good to me. Any other comments before we merge?

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 21, 2016

I've moved the discussion/issue about ViewRenderExceptionMapper to #1837, as to not impede this PR.

@jplock jplock merged commit 921d928 into dropwizard:master Nov 22, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Nov 22, 2016

Updated release notes in 361218c

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