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

JacksonMessageBodyProvider throws exception when post body is null #625

Closed
gedl opened this issue Jun 23, 2014 · 13 comments · Fixed by #842
Closed

JacksonMessageBodyProvider throws exception when post body is null #625

gedl opened this issue Jun 23, 2014 · 13 comments · Fixed by #842

Comments

@gedl
Copy link

gedl commented Jun 23, 2014

JacksonMessageBodyProvider in Dropwizard 0.6 allowed null post bodies. As a result, the resource methods would get a "default" instance (calling the default constructor) of the parameter type.

However in 0.7, JacksonMessageBodyProvider throws an exception and the resource method never gets called. Posting {} reverts to the old behaviour.

Because our application is now expecting the old behaviour (i.e. accepted null post bodies reverting to default constructors on resource methods) in multiple places (namely many client applications) changing all the calls is likely to be a nightmare.

Is there any way we can workaround this? Expected behaviour is to get null or an instance of the parameter created with the default constructor.

Thanks.

@nicktelford
Copy link
Contributor

Can you post an example of the Resource method, a request being made (i.e. via curl -v) and the full exception you're seeing, including stack-trace please? Ideally, as a linked gist

@gedl
Copy link
Author

gedl commented Jun 24, 2014

Of course: https://gist.github.com/gedl/5e359a7cdf8d6f83b099

Dropwizard version 0.71

Thanks for helping.

@gedl
Copy link
Author

gedl commented Jun 25, 2014

Added https://gist.github.com/gedl/5e359a7cdf8d6f83b099#file-workaround which is a workaround. Needs to be hooked to Jersey

Hope it helps somebody else.

@joschi
Copy link
Member

joschi commented Jun 25, 2014

This behaviour has been added (incidentally by me) in #433 (also see #232, #233, and #431) and is intentional.

I think we should discuss whether this behaviour should be configurable (either throw a ConstraintViolationException or skip validation for null references and return it). Personally I'm strongly in favor of throwing a ConstraintViolationException.

Maybe someone else wants to weigh in on this issue.

@gedl
Copy link
Author

gedl commented Jun 25, 2014

I clearly have a clumsy fix, but I'm more worried about breaking the
previous contract.
I'm of the opinion that such break needs to be thoroughly documented
including how to "keeps things running".
I'm neutral on how an empty body should be seen: whether a default request
(ie default constructors for all parameters) or a faulty one, albeit HTTP
spec does not deem an entity mandatory in the case of POST or PUT.

What do you think?
On 25 Jun 2014 21:43, "Jochen Schalanda" notifications@github.com wrote:

This behaviour has been added (incidentally by me) in #431
#431 and #433
#433 (also see #232
#232 and #233
#233) and is intentional.

I think we should discuss whether this behaviour should be configurable
(either throw a ConstraintViolationException or skip validation for null
references and return it). Personally I'm in favor of throwing a
ConstraintViolationException but YMMV.


Reply to this email directly or view it on GitHub
#625 (comment)
.

@joschi
Copy link
Member

joschi commented Jun 25, 2014

I think that the null check should only occur if the request method parameter is annotated with @Valid or @Validated and that we can simply resolve this issue by moving the check a little down JacksonMessageBodyProvider#validate().

I've created #633 to address this issue.

@gedl
Copy link
Author

gedl commented Jun 25, 2014

Good point. I vote for @NotNull as @Valid does not necessarly mean the
instance is not null (null is a non-value and such is not invalid by
definition).

G.
On 25 Jun 2014 23:12, "Jochen Schalanda" notifications@github.com wrote:

I think that the null check should only occur if the request method
parameter is annotated with @Valid or @validated and that we can simply
resolve this issue by moving the check a little down
JacksonMessageBodyProvider#validate().


Reply to this email directly or view it on GitHub
#625 (comment)
.

@joschi
Copy link
Member

joschi commented Jun 30, 2014

@Valid implies a valid object reference which means that a null reference should (and actually does) throw a ConstraintViolationException.

If you want to use parameters which may or may not be set, I suggest wrapping them in an Optional<T> which by the way works as intended thanks to Dropwizard's OptionalQueryParamInjectableProvider.

@saadmufti
Copy link

I haven't carefully followed the whole thread, but on my experience at least with Jersey, @Valid on a resource method parameter does NOT throw any exception if the parameter is not provided. To insist that the parameter be provided, you have to add @NotNull . Doesn't that indicate that @Valid only applies when the object in question is non-null? I looked up the spec on the Hibernate site, but that is silent on the question of null objects when @Valid is used.

@joschi
Copy link
Member

joschi commented Jun 30, 2014

So this doesn't seem to be simple at all…

  • Hibernate Validator will (as of version 5.1.1.Final which Dropwizard is currently using) throw an IllegalArgumentException if the method parameter annotated with @Valid is null (see org.hibernate.validator.internal.engine.ValidatorImpl.
  • The Bean Validation spec says "Null references are ignored.", but that only refers to references inside a Java Bean (which is not the case with our OptionalQueryParamInjectableProvider)
  • The JavaDoc on @Valid says "The associated object will be validated by cascade." which I would interpret as the reference must not be null.

Anything else to consider?

@joschi
Copy link
Member

joschi commented Oct 5, 2014

I think this functionality is now provided by the Jersey upstream with Bean Validation Support.

@jelgh
Copy link

jelgh commented Dec 5, 2014

I did now run in to this problem as well. I'm migrating our system from Dropwizard 0.6.1 to 0.7.1 and now our log in doesn't work because our clients sends an empty json POST. Did you guys come up with a way around this by keeping legacy client support (that will continuously send empty json POSTs)?

@joschi you mention Jersey's upstream, but it's not available in Dropwizard 0.7.1 is it?

@chrislbs
Copy link

chrislbs commented Feb 2, 2016

In case someone came here like me trying to resolve a similar issue and can't upgrade the dropwizard version to the latest immediately. I tried using @gedl workaround but it made the assumption that .available() would always return non-zero. I made a modification to just go ahead and copy the entity stream to a byte array regardless to ensure we truly know if anything is on the input stream.

https://gist.github.com/chrislbs/b64a9346db07840076b8

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

Successfully merging a pull request may close this issue.

6 participants