Hibernate Validator does not support constraints on static methods

[pilhuhn]

Describe the bug

We have code

   public static Pager extractPager(@NotNull UriInfo uriInfo) {

Which fails with "HV000116: The object to be validated must not be null." The passed uriInfo is not null, as one can see in the debugger or via printout.

This used to work in Quarkus 1.5.1 and earlier (in fact that line of code was last touched in January).
When I remove the @NotNull annotation, the code works expected.

Expected behavior

Code execution continues with a non-null uriInfo parameter

Actual behavior

Hibernate Validator throws an exception that the passed object must not be null, even if it is not null

To Reproduce

Calling code: https://github.com/RedHatInsights/policies-ui-backend/blob/master/src/main/java/com/redhat/cloud/policies/app/rest/PolicyCrudService.java#L237

Problematic code:
https://github.com/RedHatInsights/policies-ui-backend/blob/master/src/main/java/com/redhat/cloud/policies/app/rest/utils/PagingUtils.java#L45

[quarkusbot]

/cc @gsmet

[gsmet]

Any chance you could come up with a simple reproducer?

I wonder if it's a consequence of some work that has been done to support interceptors in more cases. @geoand does it ring a bell?

Until now, I don't think HV was supposed to work for static methods especially for a non bean instance.

[geoand]

@gsmet absolutely right. I think that interceptors for static methods were added recently, so that would explain it (cc @mkouba)

[gsmet]

I'll try to understand what's going on tomorrow. Not fair to break my extension that didn't ask anything!

[pilhuhn]

Interceptor sounds right, as this is the top frame I end up when I step into extractPager():

1cb101e6936e2eb232d0960a042364b57af8d809:116, PagingUtils_InterceptorInitializer (com.redhat.cloud.policies.app.rest.utils)
extractPager:-1, PagingUtils (com.redhat.cloud.policies.app.rest.utils)
getPoliciesForCustomer:237, PolicyCrudService (com.redhat.cloud.policies.app.rest)
getPoliciesForCustomer$$superaccessor6:1497, PolicyCrudService_Subclass (com.redhat.cloud.policies.app.rest)

[mkouba]

So if I understand it correctly the @NotNull annotation was previously ignored and nobody compained ;-). Now it's intercepted but fails incorrectly. We should probably (1) find out why it fails incorrectly, (2) ignore static methods in MethodValidatedAnnotationsTransformer if not supported and log a big warning for any javax.validation annotation declared on a static method.

[pilhuhn]

Indeed, @NotNull is ignored in 1.5.1.

[pilhuhn]

I don't think ignoring static methods is a good idea, as a lot of helpers are static and will/may/could use those validation annotations as a way to enforce pre-conditions without writing a lot of code.

[mkouba]

I don't think ignoring static methods is a good idea...

Ok, but it never worked before because static method interception is not supported by Weld (and CDI in general).

I will create a simple test to see what's happening...

[mkouba]

So the problem is that AbstractMethodValidationInterceptor.validateMethodInvocation() does not expect the result of ctx.getTarget() to be null. Unfortunately, the contract of javax.validation.executable.ExecutableValidator.validateParameters(T, Method, Object[], Class<?>...) is very clear - throw IllegalArgumentException if null is passed for any of the parameters... @gsmet Any idea how to workaround this limitation?

[gsmet]

So, I had a look this morning. The spec thing is not a big issue as we could have an HV-specific interface extending it and taking a class.

What concerns me is that the executable validation feature has clearly been designed with the limitations of CDI/Weld in mind which were in line with the validation of beans. So a lot of things in HV expect an object, some SPI included (for instance DefaultGroupSequenceProvider).

I have absolutely no idea if we could shoe in support for that either by not supporting some features in this case or by redesigning things.

For now, I think I would go back to ignoring these annotations as they might be in area of the code developers can't touch.

Question: how should I do it? Should I simply ignore the static methods in the annotation transformer? What happened previously if the user called a static method on an instance object, it could be intercepted or not?

I suppose the other option would be to ignore things in the interceptor when ctx.getTarget() is null.

WDYT?

[mkouba]

Question: how should I do it? Should I simply ignore the static methods in the annotation transformer?

I'd rather log a warning.

What happened previously if the user called a static method on an instance object, it could be intercepted or not?

I believe that it's called as a regular static method. So it was not intercepted.

[pilhuhn]

On 9 Jul 2020, at 13:41, Martin Kouba wrote: > Question: how should I do it? Should I simply ignore the static > methods in the annotation transformer? I'd rather log a warning.

So BV 5.6.1 [1] says that static methods should be ignored. So the previous behaviour seems correct (even if unexpected if one does not read the BV spec). I think the best would be to warn the user during the augmentation phase if such a (bad) annotation is found (and then ignore it at runtime). [1] https://beanvalidation.org/2.0/spec/#constraintdeclarationvalidationprocess-methodlevelconstraints-requirements