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

Add Jersey Bean Validation Support #842

Merged
merged 1 commit into from Apr 15, 2015

Conversation

Projects
None yet
7 participants
@joschi
Member

joschi commented Jan 11, 2015

See https://jersey.java.net/documentation/latest/bean-validation.html for details
about the Bean Validation support in Jersey. It enables the use of Java validation annotations like @NotNull, @Min, or @Email in the context of JAX-RS and Jersey.

Merging this could probably fix #625 and #633 (as both, the issue and the PR, are now outdated anyway).

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 11, 2015

Coverage Status

Coverage increased (+0.01%) when pulling aaa240e on joschi:bean-validation into 8a1d9ca on dropwizard:master.

coveralls commented Jan 11, 2015

Coverage Status

Coverage increased (+0.01%) when pulling aaa240e on joschi:bean-validation into 8a1d9ca on dropwizard:master.

@skamille

This comment has been minimized.

Show comment
Hide comment
@skamille

skamille Jan 12, 2015

Member

I'm OK with this, are we comfortable with adding a new dependency? @carlo-rtr @ryankennedy if one of you 2 +1s let's put it in

Member

skamille commented Jan 12, 2015

I'm OK with this, are we comfortable with adding a new dependency? @carlo-rtr @ryankennedy if one of you 2 +1s let's put it in

@ryankennedy

This comment has been minimized.

Show comment
Hide comment
@ryankennedy

ryankennedy Jan 12, 2015

Member

I assume the exclusion of hibernate-validator is because we're pulling it in as part of dropwizard-validation and don't want Jersey's bean validation bits to stomp the version we're pulling? Is that going to be safe to do going forward? Is there any way to move these bits to dropwizard-validation instead so all the validation-related bits stay in one place? I assume removing hibernage-validator from dropwizard-validation would suddenly cause some awkward runtime and, maybe, test missing class failures way over in dropwizard-jersey. Maven exclusions make me nervous for long-term maintenance.

I'm a little confused as to why people are POSTing empty bodies, having their resources take an argument, and complaining when it says foo = null is not a valid foo. It's triggering my smell test. Like…why would you do that? Especially the one talking about an empty POST for a login request being a valid request. I'm not sure how opinionated to be about that one. But I'm not sure that's a problem I'd aim to solve.

Out of curiosity, why aren't these validations just working right now? Is this for validation annotations in the resource method parameter list (i.e. Foo getFoo(@NotNull FooRequest req))? I assume validation annotations in the representation class (i.e. class FooRequest { @NotNull String id; }) are already working since we advertise that in the docs.

I'm fine with the new dependency if we don't see a way we'd deeply regret adding this feature down the road.

Member

ryankennedy commented Jan 12, 2015

I assume the exclusion of hibernate-validator is because we're pulling it in as part of dropwizard-validation and don't want Jersey's bean validation bits to stomp the version we're pulling? Is that going to be safe to do going forward? Is there any way to move these bits to dropwizard-validation instead so all the validation-related bits stay in one place? I assume removing hibernage-validator from dropwizard-validation would suddenly cause some awkward runtime and, maybe, test missing class failures way over in dropwizard-jersey. Maven exclusions make me nervous for long-term maintenance.

I'm a little confused as to why people are POSTing empty bodies, having their resources take an argument, and complaining when it says foo = null is not a valid foo. It's triggering my smell test. Like…why would you do that? Especially the one talking about an empty POST for a login request being a valid request. I'm not sure how opinionated to be about that one. But I'm not sure that's a problem I'd aim to solve.

Out of curiosity, why aren't these validations just working right now? Is this for validation annotations in the resource method parameter list (i.e. Foo getFoo(@NotNull FooRequest req))? I assume validation annotations in the representation class (i.e. class FooRequest { @NotNull String id; }) are already working since we advertise that in the docs.

I'm fine with the new dependency if we don't see a way we'd deeply regret adding this feature down the road.

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 9, 2015

Member

@joschi did you want to respond to @ryankennedy before someone merges this in?

Member

jplock commented Mar 9, 2015

@joschi did you want to respond to @ryankennedy before someone merges this in?

@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@carlo-rtr

carlo-rtr Mar 10, 2015

Member

I think the validations are useful if you aren't using a bean. There are many annotations, that make more sense than NotNull. http://docs.oracle.com/javaee/7/api/javax/validation/constraints/package-summary.html

I suspect the exclusion in org.hibernate.hibernate-validator is to avoid dependency divergence. I doubt trying to use jersey module without providing org.hibernate.hibernate-validator would work. Perhaps we should also explicitly add a dependency on org.hibernate.hibernate-validator in dropwizard-jersey. This way both dropwizard-jersey & dropwizard-validation have the right deps. We can ensure version consistency by moving the version to a property field defined in the parent pom

Member

carlo-rtr commented Mar 10, 2015

I think the validations are useful if you aren't using a bean. There are many annotations, that make more sense than NotNull. http://docs.oracle.com/javaee/7/api/javax/validation/constraints/package-summary.html

I suspect the exclusion in org.hibernate.hibernate-validator is to avoid dependency divergence. I doubt trying to use jersey module without providing org.hibernate.hibernate-validator would work. Perhaps we should also explicitly add a dependency on org.hibernate.hibernate-validator in dropwizard-jersey. This way both dropwizard-jersey & dropwizard-validation have the right deps. We can ensure version consistency by moving the version to a property field defined in the parent pom

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 27, 2015

Member

@joschi did you want to address @carlo-rtr 's comment and we'll add this into 0.9.0?

Member

jplock commented Mar 27, 2015

@joschi did you want to address @carlo-rtr 's comment and we'll add this into 0.9.0?

@joschi

This comment has been minimized.

Show comment
Hide comment
@joschi

joschi Apr 15, 2015

Member

Ah, sorry for not responding. Completely overlooked the replies on this PR.

I assume the exclusion of hibernate-validator is because we're pulling it in as part of dropwizard-validation and don't want Jersey's bean validation bits to stomp the version we're pulling?

Yes, exactly. In the end it's excluded to make the Maven Enforcer plugin happy. The alternative would be to add Hibernate Validator to the dependencyManagement section of the parent POM to enforce a certain version of it. We should probably do that anyway.

This being said, it's rather unlikely that we remove Hibernate Validator in the foreseeable future because it's quite deeply embedded in dropwizard-validation.

Is there any way to move these bits to dropwizard-validation instead so all the validation-related bits stay in one place?

dropwizard-jersey pulls in dropwizard-validation (see pom.xml) and this feature is tightly coupled with Jersey. It's completely useless without dropwizard-jersey, so I assumed putting it in there is the right place™.

Out of curiosity, why aren't these validations just working right now? Is this for validation annotations in the resource method parameter list (i.e. Foo getFoo(@NotNull FooRequest req))?

Yes, exactly. Some annotations (namely @Valid and @Validated) can already be used in Resource classes, others are currently limited to value classes.

If the addition of this feature doesn't impose a large performance overhead (which we should test), I don't see a reason not to add it by default.

Member

joschi commented Apr 15, 2015

Ah, sorry for not responding. Completely overlooked the replies on this PR.

I assume the exclusion of hibernate-validator is because we're pulling it in as part of dropwizard-validation and don't want Jersey's bean validation bits to stomp the version we're pulling?

Yes, exactly. In the end it's excluded to make the Maven Enforcer plugin happy. The alternative would be to add Hibernate Validator to the dependencyManagement section of the parent POM to enforce a certain version of it. We should probably do that anyway.

This being said, it's rather unlikely that we remove Hibernate Validator in the foreseeable future because it's quite deeply embedded in dropwizard-validation.

Is there any way to move these bits to dropwizard-validation instead so all the validation-related bits stay in one place?

dropwizard-jersey pulls in dropwizard-validation (see pom.xml) and this feature is tightly coupled with Jersey. It's completely useless without dropwizard-jersey, so I assumed putting it in there is the right place™.

Out of curiosity, why aren't these validations just working right now? Is this for validation annotations in the resource method parameter list (i.e. Foo getFoo(@NotNull FooRequest req))?

Yes, exactly. Some annotations (namely @Valid and @Validated) can already be used in Resource classes, others are currently limited to value classes.

If the addition of this feature doesn't impose a large performance overhead (which we should test), I don't see a reason not to add it by default.

@ryankennedy

This comment has been minimized.

Show comment
Hide comment
@ryankennedy

ryankennedy Apr 15, 2015

Member

Cool, thanks for the explanation Jochen. I'm a +1.

Ryan

On Wed, Apr 15, 2015 at 6:45 AM, Jochen Schalanda notifications@github.com
wrote:

Ah, sorry for not responding. Completely overlooked the replies on this PR.

I assume the exclusion of hibernate-validator is because we're pulling it
in as part of dropwizard-validation and don't want Jersey's bean validation
bits to stomp the version we're pulling?

Yes, exactly. In the end it's excluded to make the Maven Enforcer plugin
happy. The alternative would be to add Hibernate Validator to the
dependencyManagement section of the parent POM to enforce a certain version
of it. We should probably do that anyway.

This being said, it's rather unlikely that we remove Hibernate Validator
in the foreseeable future because it's quite deeply embedded in
dropwizard-validation.

Is there any way to move these bits to dropwizard-validation instead so
all the validation-related bits stay in one place?

dropwizard-jersey pulls in dropwizard-validation (see pom.xml
https://github.com/dropwizard/dropwizard/blob/58c4d52ce212d05b691e80e5ff687c92a24d510f/dropwizard-jersey/pom.xml#L20-24)
and this feature is tightly coupled with Jersey. It's completely useless
without dropwizard-jersey, so I assumed putting it in there is the right
place™.

Out of curiosity, why aren't these validations just working right now? Is
this for validation annotations in the resource method parameter list (i.e.
Foo getFoo(@NotNull https://github.com/NotNull FooRequest req))?

Yes, exactly. Some annotations (namely @Valid and @Validated) can already
be used in Resource classes, others are currently limited to value classes.

If the addition of this feature doesn't impose a large performance
overhead (which we should test), I don't see a reason not to add it by
default.


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

Member

ryankennedy commented Apr 15, 2015

Cool, thanks for the explanation Jochen. I'm a +1.

Ryan

On Wed, Apr 15, 2015 at 6:45 AM, Jochen Schalanda notifications@github.com
wrote:

Ah, sorry for not responding. Completely overlooked the replies on this PR.

I assume the exclusion of hibernate-validator is because we're pulling it
in as part of dropwizard-validation and don't want Jersey's bean validation
bits to stomp the version we're pulling?

Yes, exactly. In the end it's excluded to make the Maven Enforcer plugin
happy. The alternative would be to add Hibernate Validator to the
dependencyManagement section of the parent POM to enforce a certain version
of it. We should probably do that anyway.

This being said, it's rather unlikely that we remove Hibernate Validator
in the foreseeable future because it's quite deeply embedded in
dropwizard-validation.

Is there any way to move these bits to dropwizard-validation instead so
all the validation-related bits stay in one place?

dropwizard-jersey pulls in dropwizard-validation (see pom.xml
https://github.com/dropwizard/dropwizard/blob/58c4d52ce212d05b691e80e5ff687c92a24d510f/dropwizard-jersey/pom.xml#L20-24)
and this feature is tightly coupled with Jersey. It's completely useless
without dropwizard-jersey, so I assumed putting it in there is the right
place™.

Out of curiosity, why aren't these validations just working right now? Is
this for validation annotations in the resource method parameter list (i.e.
Foo getFoo(@NotNull https://github.com/NotNull FooRequest req))?

Yes, exactly. Some annotations (namely @Valid and @Validated) can already
be used in Resource classes, others are currently limited to value classes.

If the addition of this feature doesn't impose a large performance
overhead (which we should test), I don't see a reason not to add it by
default.


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

jplock added a commit that referenced this pull request Apr 15, 2015

Merge pull request #842 from joschi/bean-validation
Add Jersey Bean Validation Support

@jplock jplock merged commit 3115fc4 into dropwizard:master Apr 15, 2015

@jplock jplock added this to the 0.9.0 milestone Apr 15, 2015

@joschi joschi deleted the joschi:bean-validation branch Apr 16, 2015

@arteam arteam added the feature label Apr 26, 2015

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