@Retryable annotation silently ignored when 'open' keyword is forgotten in front of Kotlin method

[rpelissier]

I could not findout why a @Retry annotation was not taken into consideration.
After hours of reading the documentation and trying to improve the log verbosity, I realised the keyword 'open' was missing so the method could be overwritten in order to proxy.

It would be great if you could at least raise a warning when scanning the annotation.

More generally, I could not figure out how to follow the @EnableRetry and @Retry injection process to debug my configuration. It makes the API very hard to use if you are blind troubleshooting configuration issues.

Code to reproduce

import org.junit.jupiter.api.Test
import org.slf4j.Logger
import org.slf4j.LoggerFactory
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.boot.test.context.SpringBootTest
import org.springframework.retry.annotation.Backoff
import org.springframework.retry.annotation.EnableRetry
import org.springframework.retry.annotation.Retryable
import org.springframework.stereotype.Service

@SpringBootTest
@EnableRetry
open class RetryTest {

    @Autowired
    private lateinit var retryableBean: RetryableBean

    @Test
    fun testRetry() {
        retryableBean.trySomething()
    }
}

class CustomException(message: String) : RuntimeException(message)

@Service
open class RetryableBean {
    
    private val log: Logger = LoggerFactory.getLogger(javaClass),
    private var failureCount: Int = 0


    //Will ignore this method if no 'open' keyword, but no warning in the log
    @Retryable
    open fun trySomething() {
        failureCount++
        log.info("Attempt #$failureCount")
        throw CustomException("Failure #$failureCount.")
    }
}

[artembilan]

I think we can fix it, indeed. I mean add the warning. But I don't think we are going to say there open. More like non-final because it is just Java. We are not aware from this point perspective that you are up to Kotlin to point properly what is the problem.

Probably better to look into the org.jetbrains.kotlin:kotlin-allopen plugin when you compile your project: https://kotlinlang.org/docs/all-open-plugin.html

[rpelissier]

Hi @artembilan thank you for following up.
Indeed you are not aware that I am coding in Kotlin.
Kotlin open translates to being NOT final in Java.
But indeed public (or protected?) would be another requirement that deserve a Warning if not met ?

Thank you for pointing the allopen plugin, I am not sure why it would be an issue to be non-final by default.

[rpelissier]

I tried to write a pull request, but the proxying work seems to be delegated to the AOP layer and I could not find out where this warning could be inserted. If you can point me to it I would be pleased to submit a PR.

[artembilan]

why it would be an issue to be non-final by default.

That's more question to Kotlin, then here.
Or you probably just mean for your own project.

Yes, my bad: open is about non-final. The non-public problem is a bit different...

I found where is that.
See org.springframework.cglib.proxy.Enhancer.getMethods():

CollectionUtils.filter(methods, new RejectModifierPredicate(Constants.ACC_FINAL));

So, if the method is final it is rejected from being proxied. And yes: silently.

For this particular issue we just could say some WARN in the AnnotationMethodsResolver.hasAnnotatedMethods() that we found a @Retryable annotation on the method, but it is final therefor won't be proxied.

But again: does it worth to do that at all? I believe someone's going to face similar issue with many other Spring annotations, e.g. @Cachable, @EventListener etc. So, an apply plugin: 'kotlin-spring' for building your project is must have when you develop with Spring.

[rpelissier]

Even worse than expected.
Look at that. This case is not handled by all-open. They don't open the public properties in parents of annotated beans.
I mean this issue is not related to Spring-Retry, I am using your proxy to reveal the issue.

import org.junit.Assert
import org.junit.jupiter.api.Test
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.boot.test.context.SpringBootTest
import org.springframework.retry.annotation.EnableRetry
import org.springframework.retry.annotation.Retryable
import org.springframework.stereotype.Component


private const val COUNT=1000

@SpringBootTest
@EnableRetry
class SpringOpenIssueTest {

    @Autowired
    private lateinit var bean: ChildBean

    @Test
    fun `proxies should work in kotlin with all-open plugin`(){
        bean.assertOkayFromTheInside()
        Assert.assertEquals(COUNT, bean.openCount)
        Assert.assertEquals(COUNT, bean.closedCount) //java.lang.AssertionError: expected:<1000> but was:<0>
    }

}

@Component //This class is automatically opened by all-open plugin
class ChildBean : AbstractBean(){
    @Retryable
    fun assertOkayFromTheInside(){
        Assert.assertEquals(COUNT, openCount)
        Assert.assertEquals(COUNT, closedCount)
    }
}

//This class is open because abstract
abstract class AbstractBean{
    var closedCount = COUNT //This field is not automatically opened
    open var openCount = COUNT
}

[rpelissier]

But again: does it worth to do that at all? I believe someone's going to face similar issue with many other Spring annotations, e.g. @cachable, @eventlistener etc. So, an apply plugin: 'kotlin-spring' for building your project is must have when you develop with Spring.

You are right it looks like a global issue.

So, if the method is final it is rejected from being proxied. And yes: silently.

Precisely, shouldn't we warn someone who is implementing a proxy on top of a final class ? It is a contract break of making the proxy. It should throw a NotProxiableMethod("The method is final.") exception.
I am not sure where to open an issue for that. The ability to open new issues is not public for the allopen Github

[artembilan]

Yeah... Let's raise one in https://github.com/spring-projects/spring-framework/issues !

[rpelissier]

I am on it. Thanks for your support

[artembilan]

Better you - a community request.
With the link to this one and then you'll get notifications from everywhere.

[rpelissier]

Thank you @artembilan :-) Very happy to be able to humbly contribute to the Spring project.