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

ConstraintViolations context determines HTTP status #993

Merged
merged 1 commit into from May 1, 2015

Conversation

Projects
None yet
5 participants
@nickbabcock
Contributor

nickbabcock commented Apr 15, 2015

Due to #842, validation opens up new opportunities and by happenstance, a new bug. Now validations can be done on resource query parameters, return values, etc, and this, in my opinion is a good thing.

The pitfall I ran into is that the current ConstraintViolationExceptionMapper with the new bean validation in place, does not follow the jersey documentation, which states that:

500 (Internal Server Error): If the exception was thrown while validating a method return type.

400 (Bad Request): Otherwise.

This pull request attempts to fix the current behavior, which will return 422 on return values and *Params. The violations in *Params now return a 400 and a return value violation is a 500.

There is an issue with this implementation: I'm uncomfortable that the only sane implementation for checking if constraint violation was fired on a parameter is by regex. Please advise.

EDIT: I found a better way than regex from the Jersey code

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Apr 16, 2015

Member

Thanks for catching this! I think this change looks fine to me. Any thoughts from your side @joschi ?

Member

jplock commented Apr 16, 2015

Thanks for catching this! I think this change looks fine to me. Any thoughts from your side @joschi ?

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

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Apr 16, 2015

Contributor

Just note, this isn't ready to merge until I comment here with the swapped out implementation, which'll be sometime tomorrow 😄

Contributor

nickbabcock commented Apr 16, 2015

Just note, this isn't ready to merge until I comment here with the swapped out implementation, which'll be sometime tomorrow 😄

ConstraintViolations HTTP status fix
The `ConstraintViolationExceptionMapper` determines what HTTP status code
to emit depending on where the violation occurred.
@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Apr 16, 2015

Contributor

Okay, code is ready for review 😎

/cc @joschi

Contributor

nickbabcock commented Apr 16, 2015

Okay, code is ready for review 😎

/cc @joschi

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock May 1, 2015

Member

I think this looks good. Thanks for the contribution!

Member

jplock commented May 1, 2015

I think this looks good. Thanks for the contribution!

jplock added a commit that referenced this pull request May 1, 2015

Merge pull request #993 from nickbabcock/constraint-violation-code
ConstraintViolations context determines HTTP status

@jplock jplock merged commit 70c3822 into dropwizard:master May 1, 2015

@arteam arteam added the bug label May 6, 2015

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