@Transactional does not work on package protected methods of CGLib proxies

[odrotbohm]

AnnotationTransactionAttributeSource contains a flag whether to only consider public methods, set to true by default. I assume that stems from the times when JDK proxies where the primary way of applying proxies and with those only public methods can be intercepted anyway.

With CGLib proxies this is different. Package private methods can be invoked on the proxy and properly make their way through the AOP infrastructure. However, the lookup of transaction attributes is eagerly aborted due to the flag mentioned above. This creates confusing situations (assume @EnableGlobalMethodSecurity and @EnableTransactionManagement applied):

@Component
class MyClass {

  @Secured(…)
  @Transactional
  void someMethod() { … }
}

In this example, the security annotations are applied as the security infrastructure does not work with a flag like this and the advice is registered for the method invocation. The transactional annotations are not applied, as the method is not inspected for transactional annotations in the first place.

I wonder if it makes sense to flip the flag based on the proxyTargetClass attribute in @EnableTransactionManagement. If that is set to true, CGLib proxies are created and thus, transaction annotations should be regarded on package protected methods. This seems to be especially important in the context of Spring Boot setting this flag to true by default.

A current workaround is demonstrated in this commit, which uses a PriorityOrdered BeanPostProcessor to reflectively flip the flag, not considering any configuration as in that particular case we know we're always gonna run with CGLib proxies.

[sbrannen]

FWIW, the TestContext framework actually sets the flag to false in order to support package-private @Test methods in TestNG and JUnit Jupiter.

spring-framework/spring-test/src/main/java/org/springframework/test/context/transaction/TransactionalTestExecutionListener.java

Line 151 in 13183c8

protected final TransactionAttributeSource attributeSource = new AnnotationTransactionAttributeSource(false);

I'd be in favor of making production changes here (as an opt-in feature). In light of that, I've added the for: team-attention label.

[odrotbohm]

I'd be in favor of making production changes here (as an opt-in feature).

I really don't think it should be an opt-in fix as it currently creates an inconsistency in the applicability of annotations to methods as shown above. Also, you don't have to explicitly enable this for other annotations, why would you have to in this particular case?

I guess the reason that this has been overseen for so long is that folks are used to make everything and the world public in the first place even on code that doesn't need to be public (mostly due to misguidance by their IDEs).

[sbrannen]

I'd be in favor of making production changes here (as an opt-in feature).

I really don't think it should be an opt-in fix as it currently creates an inconsistency in the applicability of annotations to methods as shown above. Also, you don't have to explicitly enable this for other annotations, why would you have to in this particular case?

Then perhaps an opt-in feature for switching it back to the old way, in case the change causes issues for some projects.

I guess the reason that this has been overseen for so long is that folks are used to make everything and the world public in the first place even on code that doesn't need to be public (mostly due to misguided guidance by their IDEs).

Yes, I agree.

[odrotbohm]

Another user stumbling over this: https://stackoverflow.com/questions/63675153/transactional-annotation-doesnt-solve-org-hibernate-lazyinitializationexception

[odrotbohm]

Here's how I currently work around the issue in a project. I get hold of the AnnotationTransactionAttributeSource very early in the bean lifecycle and flip the publicMethodsOnly flag.