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 Resource Bean Validation #1572

Merged
merged 1 commit into from May 30, 2016

Conversation

Projects
None yet
3 participants
@pkwarren
Contributor

pkwarren commented May 29, 2016

Update ConstraintMessage.isRequestEntity to be resilient against
exceptions by using the Iterables.get method which accepts a default
value. This is required in case the property path contains fewer than 2
items.

@coveralls

This comment has been minimized.

coveralls commented May 29, 2016

Coverage Status

Coverage increased (+0.006%) to 81.155% when pulling a673698 on pkwarren:constraintmessage_safe_iterable into ceff305 on dropwizard:master.

@nickbabcock nickbabcock self-assigned this May 29, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 29, 2016

In situations like these, I find a test much more powerful if the bug can be exposed by adding or modifying an endpoint in ValidatingResource and the corresponding test case in ConstraintViolationExceptionMapperTest because it adds a real world flair to bug.

@pkwarren pkwarren force-pushed the pkwarren:constraintmessage_safe_iterable branch from a673698 to 0fed4a7 May 30, 2016

@pkwarren

This comment has been minimized.

Contributor

pkwarren commented May 30, 2016

@nickbabcock - Thanks for the pointer - I didn't look around enough to find the right home for the test. I found this while porting some test code from 0.9.2 to 1.0.0-rc2 and so I'm not sure if it is reproducable in the wild - it is more a correctness fix similar to the existing lines found here: https://github.com/dropwizard/dropwizard/blob/master/dropwizard-jersey/src/main/java/io/dropwizard/jersey/validation/ConstraintMessage.java#L115-L118

I was able to come up with an example which failed, but it required enabling validation for the resource itself (instead of just the method parameters). I don't know if this is desired behavior (although it could be useful).

@coveralls

This comment has been minimized.

coveralls commented May 30, 2016

Coverage Status

Coverage increased (+0.09%) to 81.256% when pulling 0fed4a7 on pkwarren:constraintmessage_safe_iterable into d597d61 on dropwizard:master.

@nickbabcock nickbabcock added this to the 1.0.0 milestone May 30, 2016

.request()
.get();
assertThat(response.getStatus()).isEqualTo(422);

This comment has been minimized.

@nickbabcock

nickbabcock May 30, 2016

Contributor

Can you add an additional assertion about the response message

This comment has been minimized.

@pkwarren

pkwarren May 30, 2016

Contributor

Updated.

Philip K. Warren
Avoid exception in ConstraintMessage.
Update ConstraintMessage.isRequestEntity to be resilient against
exceptions by using the `Iterables.get` method which accepts a default
value. This is required in case the property path contains fewer than 2
items.

@pkwarren pkwarren force-pushed the pkwarren:constraintmessage_safe_iterable branch from 0fed4a7 to 867fb61 May 30, 2016

@coveralls

This comment has been minimized.

coveralls commented May 30, 2016

Coverage Status

Coverage increased (+0.09%) to 81.256% when pulling 867fb61 on pkwarren:constraintmessage_safe_iterable into d597d61 on dropwizard:master.

@nickbabcock nickbabcock changed the title from Avoid exception in ConstraintMessage. to Add Resource Bean Validation May 30, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 30, 2016

Looks good to me. I renamed the PR to represent the added feature 😄

Merging this now, yet some additional changes should be done in this area:

  • "sortParam must match \\\"^(asc|desc)$\\\" should be "query param sort must match \\\"^(asc|desc)$\\\"
  • May need additional checks for possible mismatch in validation groups between the Invocable and bean members

@nickbabcock nickbabcock merged commit d63c5c0 into dropwizard:master May 30, 2016

@pkwarren pkwarren deleted the pkwarren:constraintmessage_safe_iterable branch May 30, 2016

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