micronaut-projects / micronaut-core

Micronaut Application Framework
http://micronaut.io
Apache License 2.0
6.09k stars 1.07k forks source link

KSP: Problem with making @Singleton allOpen and ksp #9764

Open Spikhalskiy opened 1 year ago

Spikhalskiy commented 1 year ago

Actual Behaviour

We have the following class and the project builds with kapt, allopen and micronaut 4.0.4

@Singleton
class SomeRequestValidator @Inject constructor(
    private val someService: SomeService
) : RequestValidator<SomeRequest> {
    ...
}
allOpen {
    annotations("jakarta.inject.Singleton")
    preset("micronaut")
}

If I try to migrate to ksp and use micronaut-core:4.0.6-SNAPSHOT and mictonaut-inject-kotlin:4.0.6-SNAPSHOT, I get

> Task :kspKotlin
e: [ksp] Originating element: SomeRequestValidator
e: [ksp] /<path>/SomeRequestValidator.kt:32: Cannot apply AOP advice to final class. Class must be made non-final to support proxying: <package>.SomeRequestValidator
e: Error occurred in KSP, check log for detail

Making the class explicitly "open" helps.

It looks like there is a hardcoded list of classes that are considered "shouldBeOpen": https://github.com/micronaut-projects/micronaut-core/pull/9456/files#diff-8f68af29dd990607ef86587858c4f4e22a496688049ab10f74029a93149206ebR136 It would be great if we can add to it through grade configuration the same way allOpen plugin can be configured.

Version

Micronaut 4.0.4 / 4.0.6-SNAPSHOT

dstepanov commented 1 year ago

Unfortunately, KSP doesn't see the open classes by the allOpen plugin.

What is your use case with SomeRequestValidator? Is there an intercepted method?

Spikhalskiy commented 1 year ago

@dstepanov This specific class is just a @Singleton that implements RequestValidator. There is no intercepted methods. I think this problem will be present for any Singleton with @Inject on its constructor. That’s why we have to add jakarta.inject.Singleton to the allOpen config section.

yawkat commented 1 year ago

AOP advice is usually added when there is an additional AOP annotation at the method level. You would then mark that annotation for the all open plugin, not the entirety of @Singleton.

dstepanov commented 1 year ago

A simple singleton doesn't need to have AOP advice. Maybe you are using Jakarta Validation?

yawkat commented 1 year ago

note that there is an open issue with method-level annotations for all-open support too: #9727

Spikhalskiy commented 1 year ago

Maybe you are using Jakarta Validation?

Yeah, you are right, the method's parameter is annotated with @Valid

dstepanov commented 1 year ago

@yawkat Is there a tracking issue for KSP/all open plugin?

yawkat commented 1 year ago

not that i know of, they said it's on their radar but nothing else.

dstepanov commented 1 year ago

I will take a look what can be done for the method level interceptors

Spikhalskiy commented 1 year ago

Thank you. I think it then can be considered a duplicate of #9727.

Spikhalskiy commented 1 year ago

Do I understand correctly that if I get an error like

package/ClassName.java:6: error: Cannot apply AOP advice to final class. Class must be made non-final to support proxying: ClassName

And the ClassName is a @Singleton with a method that, for example, has a parameter annotated with @NotBlank, I should be adding @NotBlank to allOpen section, not a @Singleton? Previously I had

allOpen {
    annotations("jakarta.inject.Inject", "jakarta.inject.Singleton")
    preset("micronaut")
}

and that was all that needed, it looks like now I will need to add all the validation annotations separately here one by one :)

dstepanov commented 1 year ago

Yes, but it might be enough to add @Constraint. Not sure how does it work. You can try it with KAPT first

Spikhalskiy commented 1 year ago

Ok, this is kinda interesting. Neither Constraint nor even listing the validation annotations in the allOpen configuration worked. I was trying to do something like this:

allOpen {
    annotations("jakarta.inject.Inject", "jakarta.validation.constraints.NotBlank", "jakarta.validation.constraints.NotEmpty", "jakarta.validation.Valid", ...)
    preset("micronaut")
}

and it did not solve the Cannot apply AOP advice to final class. Class must be made non-final to support proxying error. Only

allOpen {
    annotations("jakarta.inject.Singleton")
    preset("micronaut")
}

works for me.

So it feels like this

AOP advice is usually added when there is an additional AOP annotation at the method level. You would then mark that annotation for the all open plugin, not the entirety of @Singleton.

is not actually working @yawkat. The annotation on the class level has to be specified in the allOpen config instead of the annotations that are causing an AOP advice to be added to the class.

dstepanov commented 1 year ago

So you are saying the all open plugin only checks for the class annotations?

Spikhalskiy commented 1 year ago

So you are saying the all open plugin only checks for the class annotations?

I don't know. The only thing that I can state is that the advice earlier that the actual method-level annotations causing AOP advice to be added should be specified in allOpen config didn't work for me. Adding those annotations had no effect. While the nuclear allOpen {annotations("jakarta.inject.Inject", "jakarta.inject.Singleton")} solution does the trick for KAPT.

dstepanov commented 1 year ago

Hmm not sure how this can be fixed. We have only added the default rules for the all open plugin. The only way would be for our Gradle plugin to pass the possible rules to the annotation processor.

Spikhalskiy commented 1 year ago

The correspondent issue of Kotlin allOpen plugin not being supported in KSP repo: https://github.com/google/ksp/issues/1576

Spikhalskiy commented 10 months ago

Unfortunately, it doesn't look like compatibility with all-open is anywhere on the ksp team roadmap: https://kotlinlang.slack.com/archives/C013BA8EQSE/p1703125190192729 Screenshot from 2024-01-10 15-51-09

Spikhalskiy commented 10 months ago

Also, the same problem is present with KAPT in Kotlin 2.0.0-Beta2 with kapt.use.k2=true.

ondrejhrstkagendigital commented 1 month ago

Hi @dstepanov : Is there any update on this issue? I tried brand new project, with KSP and added following class:

@Singleton
class Test {
    @Transactional
    fun test() {
        println("Hello")
    }
}

I've tried:

allOpen {
    // Mark any classes with the following transactions as `open` automatically.
    annotations("jakarta.inject.Singleton", "jakarta.transaction.Transactional")
    preset("micronaut")
}

or kotlin.allopen.annotations="jakarta.inject.Singleton,jakarta.transaction.Transactional" nothing worked. Any idea how to proceed?