spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.25k stars 37.98k forks source link

Fail explicitly if a final method is invoked on a CGLIB proxy #26729

Open rpelissier opened 3 years ago

rpelissier commented 3 years ago

Started from https://github.com/spring-projects/spring-retry/issues/238

In org.springframework.cglib.proxy.Enhancer.getMethods(): CollectionUtils.filter(methods, new RejectModifierPredicate(Constants.ACC_FINAL)); If the method is final it is SILENTLY rejected from being proxied.

Shouldn't we warn someone who is implementing a proxy on top of a final class ? I believe it is a contract break of making the proxy and should throw a NotProxiableElementException("The method or field is final.").

This issue is particularly severe in Kotlin where final is the default. This example is using Spring-Retry to generate the Proxy and demonstrate 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
}
mdeinum commented 3 years ago

Generating a more clear warning would be nice as it is also a cause for weird behaviour (see https://deinum.biz/2020-07-03-Autowired-Field-Null/). It doesn't only apply to final methods but also private and default modifier methods leading to this.

I have been answering too many questions on StackOverflow on this kind of issues, so a nice warning would probably trigger people to watch closer.