Hibernate Validator with Kotlin and RestEasy - Custom Validations executed twice

[cfinkelstein]

Describe the bug

Hello there,

I recognized that custom validators will be executed twice in Kotlin with reactive context.

Endpoint:

package com.grattis.controller

import javax.ws.rs.ApplicationPath
import javax.ws.rs.core.Application

@ApplicationPath("/api")
class GrattisApplication: Application()

package com.grattis.controller

import com.grattis.model.*
import com.grattis.model.validation.UUIDPathElement
import kotlinx.coroutines.*
import javax.enterprise.context.ApplicationScoped
import javax.ws.rs.GET
import javax.ws.rs.Path
import javax.ws.rs.PathParam
import javax.ws.rs.core.Response

@ApplicationScoped
@Path("/users")
class UserController {

    @GET
    @Path("/{userId}")
    suspend fun userById(
        @UUIDPathElement()
        @PathParam("userId") userId: String): Response  {
        return GlobalScope.async{
            Response.status(200).build()
        }.await()
    }
}

package com.grattis.controller.validation


import javax.validation.ConstraintViolationException
import javax.ws.rs.core.Response
import javax.ws.rs.ext.ExceptionMapper
import javax.ws.rs.ext.Provider

@Provider
class ViolationExceptionMapper: ExceptionMapper<ConstraintViolationException> {

    override fun toResponse(ex: ConstraintViolationException): Response {
        ex.printStackTrace()
        return Response.status(400).build()
    }

}

package com.grattis.model.validation


import javax.enterprise.context.ApplicationScoped
import javax.validation.ConstraintValidator
import javax.validation.ConstraintValidatorContext

@ApplicationScoped
class UUIDConstraintValidator: ConstraintValidator<UUIDPathElement, String> {
    override fun isValid(uuid: String?, context: ConstraintValidatorContext): Boolean {
       //can be actually not null as the endpoint would not be found in this example
       //but this method will be called a second time with a 'null' value
        return uuid != null
    }
}

package com.grattis.model.validation

import java.lang.annotation.Documented
import javax.validation.Constraint
import kotlin.reflect.KClass

@Target(AnnotationTarget.FIELD, AnnotationTarget.VALUE_PARAMETER)
@Retention(AnnotationRetention.RUNTIME)
@Constraint(validatedBy = [UUIDConstraintValidator::class])
@Documented
annotation class UUIDPathElement(val message: String = "must be a valid uuid",
                                 val groups: Array<KClass<out Any>> = [],
                                 val payload: Array<KClass<out Any>> = [])

 <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-kotlin</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-resteasy-reactive</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-resteasy-reactive-jackson</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-hibernate-validator</artifactId>
        </dependency>
        <dependency>
            <groupId>org.jetbrains.kotlin</groupId>
            <artifactId>kotlin-script-runtime</artifactId>
        </dependency>

Expected behavior

Following GET Request:

http://localhost:8080/api/users/2df56772-7464-428f-8b73-b54f5c6052d0

Should deliver HTTP 200

Actual behavior

Following GET Request:

http://localhost:8080/api/users/2df56772-7464-428f-8b73-b54f5c6052d0

Delivers HTTP 400 as the Validator will be called a second time with 'null' for uuid.

How to Reproduce?

See above.

Output of uname -a or ver

Linux cfn7655 5.19.0-35-generic #36~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Feb 17 15:17:25 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

openjdk version "19.0.1" 2022-10-18
OpenJDK Runtime Environment GraalVM CE 22.3.0 (build 19.0.1+10-jvmci-22.3-b08)
OpenJDK 64-Bit Server VM GraalVM CE 22.3.0 (build 19.0.1+10-jvmci-22.3-b08, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

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

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)

Additional information

No response

[quarkus-bot]

/cc @evanchooly (kotlin), @geoand (kotlin), @gsmet (hibernate-validator), @yrodiere (hibernate-validator)

[gsmet]

Could you please assemble a simple project reproducing the issue?

Also would you happen to know if the same issue happens in plain Java?

[cfinkelstein]

Could you please assemble a simple project reproducing the issue?

Hello @gsmet, thanks a lot for the quick response. I added a repo here => https://github.com/cfinkelstein/kotlin-reasy-validator-bug

[cfinkelstein]

Also would you happen to know if the same issue happens in plain Java?

Did not test with Java so far.

[cfinkelstein]

I added some tests "https://github.com/cfinkelstein/kotlin-reasy-validator-bug". The strange thing is that the bug occurs not for every run. It seems that it works if I execute multiple calls but not if I just trigger a single request. Tested it also with a http client. Where it sometimes worked and and sometimes not. Pls checkout the attached video.

screen_recording_kotlin_resteasy.mp4

[cfinkelstein]

Hey @gsmet, I changed now all my integrations to Uni instead of the default suspend handling:

suspend fun call(): ...

@OptIn(ExperimentalCoroutinesApi::class, DelicateCoroutinesApi::class)
fun sample(): Uni<Response> {
 return return GlobalScope.async { call() }.asUni().map { 
    Response.status(200).build()
 }
}

It seems to be no difference in terms of execution time/performance etc.
With this approach I could not see this bug anymore. There seems to be something wrong in the way how the coroutine will be managed. What I could also see in the debugger is that the ConstraintValidator isValid method will be called with two different ConstraintValidatorContext instances in the error case.

[geoand]

I tried your sample with the latest Quarkus version and the validation was only executed once as expected

[geoand]

Please ignore my previous comment as I was able to reproduce the problem

[geoand]

The reason this happens is that CDI interceptors get invoked every time the continuation is invoked.

@Ladicek IIRC, you had to deal with a similar problem in Fault Tolerance, no? Do we have a good way of disabling interceptors with some API call (I am pretty sure I can distinguish between the first and subsequent calls, so if I had an API to call to disable the interceptors, the issue would be solved)?
cc @mkouba for ^

[Ladicek]

I vaguely recall stumbling upon interceptors on suspending functions, but not in the FT context. If I remember correctly, the Kotlin compiler emits a synthetic method that is called repeatedly, and calls it from the original. I think (but not sure) that the Kotlin compiler also copies annotations from the original method to the synthetic method that implements the state machine, in which case the synthetic method is intercepted on each call. We have some code in place to not intercept certain synthetic methods, but I don't recall from the top of my head. I'd first look at the interceptor binding annotations.

[geoand]

Thanks for the input. Let's see what @mkouba has to say as well :)

[evanchooly]

I vaguely recall stumbling upon interceptors on suspending functions, but not in the FT context. If I remember correctly, the Kotlin compiler emits a synthetic method that is called repeatedly, and calls it from the original. I think (but not sure) that the Kotlin compiler also copies annotations from the original method to the synthetic method that implements the state machine, in which case the synthetic method is intercepted on each call. We have some code in place to not intercept certain synthetic methods, but I don't recall from the top of my head. I'd first look at the interceptor binding annotations.

From what I recall, that's true. All the annotations are copied over which is almost certainly confusing things. We could likely filter out any synthetic methods or, and my memory is foggy here, any methods with coroutine types in the signatures. I know the kotlin compiler does some fancy footwork with suspend functions so there might be some consistent artifact of that processing that we could flag one way or another if the synthetic filter isn't viable.

[mkouba]

We do not intercept non-bridge synthetic methods since quarkus 3.0.0.Alpha4; the change got introduced in this pull request. Maybe we should backport the specific commit in 2.16?

[geoand]

I think we got a little sidetracked here, so let me demonstrate with some code exactly what the issue is.

Here is a JAX-RS Resource:

@Path("/users")
class UserController {

    @GET
    @Path("/nb/{userId}")
    suspend fun userById(
        @UUIDPathElement
        @PathParam("userId") userId: String): Response  {
        println("test")
        delay(100)
        return Response.status(200).build()
    }


}

Note the use of @UUIDPathElement which makes engages an @ApplicarionScope javax.validation.ConstraintValidator.

Now, in RESTEasy Reactive we initiate the Coroutine call like this:

public class UserController$quarkuscoroutineinvoker$userById_0506e3a2a56eb32a9c3d1b74fd72a6e695e67551 implements CoroutineEndpointInvoker {

   public Object invokeCoroutine(Object var1, Object[] var2, Continuation var3) {
      Object var4 = var2[0];
      return ((UserController)var1).userById((String)var4, var3);
   }
}

Now every time the coroutine is resumed (so in the example above after delay), Kotlin calls this again which results in the interceptors being run again.

So my question is how can I invoke ((UserController)var1).userById((String)var4, **var3); without the interceptors bound to UserController#userById being run.

Is that more clear now @mkouba @Ladicek ?

[mkouba]

So my question is how can I invoke ((UserController)var1).userById((String)var4, **var3); without the interceptors bound to UserController#userById being run.

You can't.

[Ladicek]

You cannot "invoke a method without interceptors", that's not how it works.

BTW, I recalled that you're right, I did have the same problem in Fault Tolerance. This is how I solved it: https://github.com/smallrye/smallrye-fault-tolerance/blob/main/implementation/autoconfig/core/src/main/java/io/smallrye/faulttolerance/autoconfig/KotlinSupport.java

$suspendImpl marks the generated state machine method that gets invoked repeatedly. In Fault Tolerance, I make sure that it doesn't get intercepted. I guess something like that would be doable here too, or perhaps universally.

[geoand]

You can't.

Well, then we need to add something, because the current behavior is problematic for Kotlin

[geoand]

I guess something like that would be doable here too, or perhaps universally

+1

[mkouba]

You can't.

Well, then we need to add something, because the current behavior is very problematic

Well, for each intercepted bean we generate an intercepted subclass that represents the contextual instance of the bean. And for each intercepted method we add a synthetic method that forwards invocation to the superclass - the original bean class. For example, if you bean class declares a method String foo(String name) then we (1) override foo() (with the interception logic) and (2) generate a method like:

public String foo$$superforward1(String var1) {
   return super.foo(var1);
}

And this method is called when the last interceptor in the chain calls InvocationContext#proceed().

Now this is of course an implementation detail but if we standardize the name of the generated synthetic method...

[geoand]

@mkouba sorry, I am missing how that helps in this case.

[mkouba]

@mkouba sorry, I am missing how that helps in this case.

foo$$superforward1() does not trigger interception.

[geoand]

Ah okay, I now see that is public in the generated _Subclass :)

I propose we do standardize the name (or even have some kind of utility that will allow code that generates other code to know the name of the method.) I could certainly just call it myself assuming what the name will be, but that sounds pretty brittle :)

WDYT?

[Ladicek]

I don't think calling the $$superforward method from the outside is a good idea. It breaks in case of package-private method declared in a different package -- learned the hard way :-)

[geoand]

So what do you propose instead @Ladicek ?

[Ladicek]

A simple annotation transformation should work.

Additionally, if the generated method is synthetic (which I don't remember and didn't check now), the issue should fix itself in Quarkus 3 :-)

[geoand]

You mean the invokeCoroutine method I showed above?

[Ladicek]

No, I mean transform (remove) annotations on the userById$suspendImpl method. The coroutine invoker doesn't need to be changed.

We could also backport the change for not intercepting synthetic method to Quarkus 2.16, I guess.

[geoand]

Interesting, I don't see $suspendImpl anywhere in the compiled code nor do I ever remember working with that before...

[Ladicek]

Oh this is an interesting one! This class

@ApplicationScoped
@Path("/users")
class UserController {
    @GET
    @Path("/nb/{userId}")
    suspend fun userById(@UUIDPathElement @PathParam("userId") userId: String): Response {
        delay(100)
        return Response.status(200).build()
    }
}

is final and the userById method is also final. The Kotlin compiler generates a more streamlined bytecode in this case, because it assumes that there may be no subclass and the method may not be overridden.

Quarkus then removes the final modifiers through bytecode transformation (or maybe the kotlin-maven-allopen does that? Not sure), but at that point, the Kotlin-generated bytecode is simply not prepared for subclassing / overriding.

If you make the class open and the method as well, Kotlin generates a little more complex bytecode that works well with subclass-based interception. The code for userById is in fact emitted into a synthetic static method called userById$suspendImpl. The userById method does nothing but call the $suspendImpl method, and all continuations also call the $suspendImpl method.

Making the class and method open is in fact enough to solve the issue here. Even though the synthetic static method has all the annotations as the original, we don't intercept it for some reason (static method interception has some limits, I'm not sure which exactly comes to play in this case).

[geoand]

Wow, that's neat :).

Too bad we can't do much here then except propose the use of open :)

[Ladicek]

I think we should probably bail out on suspend functions that are final (or that are in final classes) instead of making them non-final, because that simply doesn't work. It is a special case, which is ugly, but it allows us to provide a tailored error message with guidance, which is nice.

[geoand]

Well, it works in some cases, so maybe we should just log a warning?

[Ladicek]

Yeah, it works when the suspend function actually never suspends :-)

I mean, I don't want to bail out on all suspend functions that are final (or in a final class) -- just when they are intercepted (or decorated).

[mkouba]

Even though the synthetic static method has all the annotations as the original, we don't intercept it for some reason (static method interception has some limits, I'm not sure which exactly comes to play in this case).

Only method-level bindings are considered and private static methods are ignored; see the docs.

[Ladicek]

Well, the static method is package-private and it has the validation annotation, so the reason is probably in the code that makes sure methods with validation annotations are intercepted.

This https://github.com/quarkusio/quarkus/blob/main/extensions/hibernate-validator/deployment/src/main/java/io/quarkus/hibernate/validator/deployment/MethodValidatedAnnotationsTransformer.java seems to be the annotation transformation that adds the interceptor binding to methods that have validator annotations, and it seems to skip static methods for now. So it might become a problem in the future :-)

[geoand]

Yeah, it works when the suspend function actually never suspends :-)

We have tests that prove(?) otherwise. From what I can tell for the RR case, the problem only occurs when there are interceptors

[Ladicek]

Well exactly, that's what I'm talking about :-) Sorry for not being clear -- I'm proposing to unconditionally disallow interception (and decoration) of suspend functions that are either final or in a final class, instead of removing the final flag through bytecode transformation.

[geoand]

+1 for that

[cfinkelstein]

One point I missed here. I could not see this issue while using the validations which are provided from the library like @NotNull. Is there any difference in the way how those are handled?

[Ladicek]

There's no difference. I'm pretty confident that the classes using the other validations were different in some other regard -- were open, didn't suspend, or something like that.

[Ladicek]

I have an initial implementation of noninterceptable suspend functions detection here: https://github.com/Ladicek/quarkus/commits/arc-kotlin-suspend-functions-interception I need to think about testing this, as we don't have any Kotlin in ArC and I'd guess we don't want to change that :-) There's probably a place somewhere in Quarkus where a test case can be added.

[geoand]

Yeah, the only tests I can think of are in RR, HR and RM