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

Move validation logic from Jackson to Jersey #1251

Merged
merged 4 commits into from Nov 21, 2015

Conversation

Projects
None yet
5 participants
@nickbabcock
Contributor

nickbabcock commented Sep 9, 2015

To discuss this pull request, see the related dropwizard-dev thread.

This pull request is not meant to be merged, as it is far from complete and wanted to hear comments on it before I sink more work into it (can now be merged).

assertThat(violations).hasSize(1);
assertThat(ConstraintViolations.determineStatus(violations)).isEqualTo(422);
}
// @Test

This comment has been minimized.

@joschi

joschi Sep 10, 2015

Member

Please don't just comment code but remove it completely. In the end that's what a version control system like git is for. 😉

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
import com.google.common.collect.*;

This comment has been minimized.

@joschi

joschi Sep 10, 2015

Member

Please expand that wildcard import.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 10, 2015

This pull request does need a lot of polish before it is even remotely merge-able. I just wanted to get confirmation from the team that this is a good direction to be heading.

import javax.ws.rs.ext.ExceptionMapper;
import java.util.Set;
public class JerseyViolationExceptionMapper implements ExceptionMapper<JerseyViolationException> {

This comment has been minimized.

@gtrog

gtrog Sep 10, 2015

I like the fact that now it'll be possible to intercept validation exceptions via an exception mapper and write a custom response for them. This is something that I've struggled with in the past where I've wanted custom formatting of validation errors

This comment has been minimized.

@nickbabcock

nickbabcock Sep 10, 2015

Contributor

Not to get off topic, but this should already be possible if you define your own ExceptionMapper<ConstraintViolationException> and register it. (You'll also need to set registerDefaultExceptionMappers to false). Feel free to post on the dropwizard-user channel for additional questions.

@nickbabcock nickbabcock force-pushed the nickbabcock:move-validation-logic branch from b81a070 to bb315c5 Sep 21, 2015

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 23, 2015

Ok, pull request has matured a little bit since inception. I've decided to put my thoughts together to let everyone know the current impact of this pull request.

@Validated/@Valid Distinction

The @Validated annotation is no longer self sufficient for constraint group validation -- @Validated must be used in conjunction with @Valid

Before

@POST
public PartialExample validatedPartialExample(
        @Validated({Partial1.class}) PartialExample obj) {
    return obj;
}

Now

@POST
public PartialExample validatedPartialExample(
        @Validated({Partial1.class}) @Valid PartialExample obj) {
    return obj;
}

Validated with no groups is same as specifying the default group.

The cons with this approach is that it is not backwards compatible. Everyone who previously specified @Validated({...}), they now must add the @Valid. Also another annotation is required to achieve previous behavior (and no one likes having more code)

Return Value Validation

This pull request adds return value validation on resources. This allows for your service to make guarantees about the response. If these guarantees aren't meant, the service will return an internal server error rather than a malformed response. To enable return value validation, mark the endpoint method with the wanted constraints

@POST
@NotNull
@Valid
public PartialExample validatedPartialExample() {
    // Will trigger an internal server error, because we aren't allowed to
    // return null!
    return null;
}

Null Entities Handled

Dropwizard currently doesn't validate null entities in a way that is consistent with Hibernate Validator. Right now, if a request entity is null, even if it is ok to be null, a 422 is returned with "The request entity was empty". There are a couple pull requests to only throw a 422 on a null entity when the entity is annotated with Validated or Valid, but this is still not to spec. I'll copy my thoughts from another thread (#633):

When using the validator and you call validateParameters with the parameter representing the request entity being null:

  • If no annotations on the parameter, no validations are performed
  • If the parameter has @Valid, no validations are performed
  • If the parameter has @NotNull, a constraint violation will be created because the parameter is null. If the parameter was not null then cascading validations wouldn't have applied
  • If the parameter has @Valid and @NotNull, a constraint violation will be created

The pull request brings Dropwizard in line with said behavior.

The other side of the argument is that if something can be null it should be wrapped in an Optional, which I completely agree with, but to assume null is erroneous regardless of annotations may be too against the Java mindset (unfortunately)

Unresolved Questions

  • How can someone extend validation? Solved, it is possible to pass in your own validator
  • How badly is this going to break for people who extended the jackson mapper?
  • I introduced a new exception JerseyViolationException and exception mapper JerseyViolationExceptionMapper to achieve some of the functionality. Ideally, I could have rolled the functionality into ConstraintViolationExceptionMapper, but I was at a loss as how to do so. Consequently, ConstraintViolationException is never instantiated and thrown, which leaves the usefulness of ConstraintViolationExceptionMapper in question (should it be deleted?, I'm not sure)

Conclusion

Lots of big changes here and backwards incompatibility, so this shouldn't be considered for 0.9.x

@arteam

This comment has been minimized.

Member

arteam commented Sep 24, 2015

I like the plan.

How can someone extend validation?

Could users write own HibernateValidationFeature and register it through JerseyEnvironment?
Will it replace already registered feature? I am not familiar enough with the JAX-RS spec on this.
If not, maybe we could to parametrize DropwizardResourceConfig with a feature class.

How badly is this going to break for people who extended the jackson mapper?

I don't think it's a very big concern. I beleiv, a little percent of users extends the mapper. 1.0 will be a big release anyway, so we are justified to break some stuff to bring advanced features.

I introduced a new exception JerseyViolationException and exception mapper JerseyViolationExceptionMapper to achieve some of the functionality. Ideally, I could have rolled the functionality into ConstraintViolationExceptionMapper, but I was at a loss as how to do so. Consequently, ConstraintViolationException is never instantiated and thrown, which leaves the usefulness of ConstraintViolationExceptionMapper in question (should it be deleted?, I'm not sure)

I think we should delete it. If we leave the class, we will only increase confusion. Again, it's a quesion
of compatability: we probably will break it anyway, so is not worth to preserve this exception.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 29, 2015

Could users write own HibernateValidationFeature and register it through JerseyEnvironment?
Will it replace already registered feature?

Unfortunately, it looks like re-registering is not possible. Quoting from the Jersey docs:

Any subsequent registration attempts for a component type, for which a class or instance-based registration already exists in the system MUST be rejected by the JAX-RS implementation and a warning SHOULD be raised to inform the user about the rejected registration.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 1, 2015

With my latest commit, I've reached the same level of validation extension as the functionality this pull replaced. Now a user configured validator can be used in validation. I'd still like to see the ability to extend worked on, but I'm somewhat satisfied for now.

@jplock

This comment has been minimized.

Member

jplock commented Oct 1, 2015

This looks great! Thanks for all the hard work

@jplock

This comment has been minimized.

Member

jplock commented Oct 1, 2015

Do we want this in 0.9 or 1.0? thoughts @dropwizard/committers ?

@arteam

This comment has been minimized.

Member

arteam commented Oct 1, 2015

I think that it's a too big change for 0.9. People extensively tested release candidates and it would be a big disappointment for them, if we release something not compatible.

@jplock

This comment has been minimized.

Member

jplock commented Oct 1, 2015

@arteam very true, and I agree. We should hold this until 1.0. Should we create a release/0.9.x branch to prep the final 0.9.0 release so we can merge this into master and start an rc process for 1.0?

@jplock jplock added this to the 1.0.0 milestone Oct 1, 2015

@nickbabcock nickbabcock force-pushed the nickbabcock:move-validation-logic branch from 2e1d59c to 80d03f8 Oct 28, 2015

@nickbabcock nickbabcock force-pushed the nickbabcock:move-validation-logic branch from 80d03f8 to b42cdbb Oct 29, 2015

@nickbabcock nickbabcock force-pushed the nickbabcock:move-validation-logic branch from e10fdae to 1e668ce Nov 13, 2015

@jplock

This comment has been minimized.

Member

jplock commented Nov 20, 2015

@dropwizard/committers should we merge this in for 1.0.0 now? I'm thinking so.

@jplock jplock added the improvement label Nov 20, 2015

arteam added a commit that referenced this pull request Nov 21, 2015

Merge pull request #1251 from nickbabcock/move-validation-logic
Move validation logic from Jackson to Jersey

@arteam arteam merged commit 2f7c954 into dropwizard:master Nov 21, 2015

@arteam

This comment has been minimized.

Member

arteam commented Nov 21, 2015

I took another look and this looks good to me. Merged to master.

Thanks to @nickbabcock for all the hardwork on validation.

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