search

quarkusio / quarkus

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

Discuss and possibly adjust Hibernate Validator + Spring Web integration #11532

Open famod opened 4 years ago

famod commented 4 years ago

Description See https://github.com/quarkusio/quarkus/pull/11506/files#r474658572.

It should be checked whether the Hibernate Validator extension needs to be adjusted to consider class level "JAX-RS" annotations.

quarkusbot commented 4 years ago

/cc @geoand /cc @gsmet

geoand commented 4 years ago

Can someone please provide me with the context of this one?

famod commented 4 years ago

@geoand In #11506 I first did not check the annotation target kind which resulted in the following error for spring-web: https://github.com/quarkusio/quarkus/pull/11506#issuecomment-678022827

So it seems the spring-web extension is addind non-method-level jaxrs-annotations. Since hibernate validator is only checking for jaxrs method-level annotations, the question is whether we are missing something in hibernate-validator for spring-web.

PS: Welcome back, btw.

geoand commented 4 years ago

@geoand In #11506 I first did not check the annotation target kind which resulted in the following error for spring-web: #11506 (comment)

So it seems the spring-web extension is addind non-method-level jaxrs-annotations. Since hibernate validator is only checking for jaxrs method-level annotations, the question is whether we are missing something in hibernate-validator for spring-web.

Do you recall which annotation was causing the error?

PS: Welcome back, btw.

Thanks :)

famod commented 4 years ago

Do you recall which annotation was causing the error?

No, but should be easy to reproduce. I'll add another comment later.

famod commented 4 years ago

Printout of all non-method annotations in ExceptionHandlingTest:

[INFO] Running io.quarkus.it.spring.web.ExceptionHandlingTest
@RequestMapping(value = ["/cache"])
@RequestMapping(value = ["/api"])
@RequestMapping(value = ["/exception"])
@RequestMapping(value = ["/greeting"])
@RequestMapping(value = ["/cache"])
@RequestMapping(value = ["/api"])
@RequestMapping(value = ["/exception"])
@RequestMapping(value = ["/greeting"])
geoand commented 4 years ago

Right, I saw that, so I guess it's my bad that I didn't ask the proper question:

What I don't understand is how come the test started failing now? What changed in the PR regarding annotation lookup?

famod commented 4 years ago

The test started failing when I addedthe gathering of all methods with JAX-RS annotations in the entire Quarkus app. This is required to cover JAX-RS annotations in interfaces and superclasses. At that early stage of the PR, a sanity check for the method kind was missing (asMethod() was called without a pre-check).

geoand commented 4 years ago

OK :) Then it seems to me that there is nothing weird really going with Spring web, no?

famod commented 4 years ago

Then it seems to me that there is nothing weird really going with Spring web, no?

As long as it's correct that @RequestMapping is added to the Hibernate Validator processor as a "JAX-RS" annotation, the only question is whether the processor needs to cover class level as well (AFAICS: yes).

geoand commented 4 years ago

Yes to both :)