quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.36k stars 2.56k forks source link

ExceptionMapper not being called when using `@Valid` with hibernate validator in REST endpoints #41102

Open tmulle opened 3 weeks ago

tmulle commented 3 weeks ago

Describe the bug

This used to work and not sure when it stopped working, but I have code that uses the @Valid annotation to validate incoming JSON objects in my rest endpoints.

I also have a custom exception mapper which handles the ConstrainValidation exception and formats the message nicely.

In Quarkus 3.11.1 this no longer gets triggered when I send in a bad value for my json object.

I am getting a generic Constraint validation message now.

I noticed this in my real code so I added a test case to my demo code for this bug.

In this test code, I simply have a ExceptionMapper with a log statement, it never gets triggered.

My real mapper would then process an instance of ConstraintValidationException and proceed.

Test: https://github.com/tmulle/quarkus-exception-mapper-test.git

Try to POST to http://localhost:8080/hello/validator with a JSON payload of {"someItem":""} and you'll see the exception mapper not print out its log statement but you are returned a constraint message.

Did something change? was there a migration note I missed?

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

MacOS/Linux

Output of java -version

JDK17

Quarkus version or git rev

3.11.1

Build tool (ie. output of mvnw --version or gradlew --version)

maven

Additional information

No response

quarkus-bot[bot] commented 3 weeks ago

/cc @gsmet (hibernate-validator), @yrodiere (hibernate-validator)

yrodiere commented 3 weeks ago

Hey,

Can you tell us in which version of Quarkus this used to work?

gsmet commented 3 weeks ago

I went back to 3.8.5 and it's consistent with the 3.11.1 behavior and with what I think it should do.

We handle HibernateValidator exceptions with an ExceptionMapper:

public class ResteasyReactiveViolationExceptionMapper implements ExceptionMapper<ValidationException>

It is more specific than yours so it takes precedence.

If you want to override it, you will need to define a ExceptionMapper<ValidationException> with a higher priority.

That being said, I wonder if we should define the priority of these exception mappers to Priorities#HEADER_DECORATOR (I didn't find anything better) so that definining one without priority would take precedence. @yrodiere WDYT?

yrodiere commented 3 weeks ago

That being said, I wonder if we should define the priority of these exception mappers to Priorities#HEADER_DECORATOR (I didn't find anything better) so that definining one without priority would take precedence. @yrodiere WDYT?

If you're suggesting to give built-in mappers lower priority than user mappers, I'm not sure about that. I suspect most people are likely to write a ExceptionMapper<Exception> and handle their app-specific stuff there... while still wanting to have "built-in" mappers (such as the HV one) do their work.

Wouldn't it make more sense for people wanting to override built-in mappers to do so explicitly with @Priority(...)?

gsmet commented 3 weeks ago

My understanding is that ExceptionMapper<Exception> won't override the ExceptionMapper<ValidationException>. Only a ExceptionMapper<ValidationException> will override it.

yrodiere commented 3 weeks ago

If it indeed works that way, then sure, your suggestion makes sense. I have no idea if that's really how it works though.

geoand commented 3 weeks ago

My understanding is that ExceptionMapper<Exception> won't override the ExceptionMapper<ValidationException>. Only a ExceptionMapper<ValidationException> will override it.

Correct.

Also FWIW, a user provided ExceptionMapper<ValidationException> would take precedence over a built in (that is, one provided by Quarkus) ExceptionMapper<ValidationException> if they have the same priority Actually this does not seem to be the case for ExceptionMapper which is weird...

tmulle commented 3 weeks ago

Ok, so I went back to 3.2 and it still works the same way.

So, I'm not sure when I saw it working, or if I mis-remebered and was actually doing manual validation and throwing the ValidationException.

However, I tried two things based on your suggestions:

  1. Created a specific exception mapper for ExceptionMapper<ValidationException> with default priority

This caught the exception as expected.

  1. Then I tried just adding a @Priority(1) and then @Priority(Priorities.USER) and even something like @Priority(10000000) to my existing ExceptionMapper<Throwable> and commenting out my ExceptionMapper<ValidationException>.

This didn't work and I still got the default Validation error message.

I'd like to be able to use ONE exception mapper for ALL exceptions but if I have to break it up into different specific classes then I can do that too. I just wanted to make sure something wasn't broken.

I thought I read you could in fact override the default exception mappers with the @Priority but I can't figure out which value to use to do that.