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

ValidationException should not result in HTTP 400 status #254

Closed
jburkal opened this issue Jan 11, 2023 · 4 comments · Fixed by #260
Closed

ValidationException should not result in HTTP 400 status #254

jburkal opened this issue Jan 11, 2023 · 4 comments · Fixed by #260
Labels
bug Something isn't working

Comments

@jburkal
Copy link
Contributor

jburkal commented Jan 11, 2023

An exception of class javax/jakarta.validation.ValidationException is handled by the ValidationExceptionMapper, which maps it to an HTTP 400 status and doesn't log the exception.

However, ValidationException and its subclasses (except ConstraintViolationException) are not thrown due to failed validation, but are instead thrown on invalid use of the framework (e.g. unexpected exceptions thrown from validators).

This can clearly be seen from the names of the sub-classes:

  • ConstraintDeclarationException
  • ConstraintDefinitionException
  • NoProviderFoundException
  • ValueExtractorDefinitionException

The exception mapper in Quarkus is also quite clear about this exception not being thrown as a result of failed validation:

// Not a violation in a REST endpoint call, but rather in an internal component.
// This is an internal error: handle through the QuarkusErrorHandler,
// which will return HTTP status 500 and log the exception.

I believe it would be more correct for these exceptions to result in an HTTP 500 status being returned, and the exception logged.
I'll be happy to create a pull request that fixes this.

Tested with com.tietoevry.quarkus:quarkus-resteasy-problem:2.1.0 using Quarkus 2.14.3.

@lwitkowski
Copy link
Collaborator

@jburkal great catch! You're most likely correct that it should be handled differently. Let me do a little research before we plan the next steps.

@lwitkowski
Copy link
Collaborator

lwitkowski commented Jan 19, 2023

@jburkal sorry it took me so long to investigate.

I agree with everything you wrote. The fix should be pretty easy, because ConstraintViolationExceptionMapper handles also ResteasyViolationExceptions, and I believe it does that correctly (nice 400 with all the details), so there's no need to complicate ValidationExceptionMapper as Quarkus guys did in ResteasyViolationExceptionMapper.

So the only thing we need to change is ValidationExceptionMapper + one test (feel free to add more for some of the exceptions you mentioned, not sure how to reproduce them easily).

I can fix that myself, but feel more than welcome to contribute and create a PR from your own fork. Some guidelines are described here.

If you don't have time or energy for that - that's cool, just let me know and I'll handle it myself.

ps. There's a typo in ValdationExceptionsResource, and throwViolationException should be called a bit differently.

@lwitkowski
Copy link
Collaborator

@jburkal PR merged, much appreciated!

One test suite is failing on master, but this is expected (it checks latest released to maven central version, v2.1.0, with tests from master). I'll release v2.2.0 with your fix later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants