Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CGLIB proxies should still consider @Transactional annotations on interface methods [SPR-14322] #18894

Closed
spring-projects-issues opened this issue May 31, 2016 · 10 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented May 31, 2016

Oliver Drotbohm opened SPR-14322 and commented

If an application components implements an interface whose methods carry annotations that are triggering interceptors (e.g. for transactions), enabling target class proxying will result in the interceptors for those annotations not being triggered anymore. Here's a sample:

interface SomeComponent {

  @Transactional
  void init();
}

@Component
class SomeComponentImpl  implements SomeComponent {

  @Override
  public void init() {
    if (!TransactionSynchronizationManager.isActualTransactionActive()) {
      throw new IllegalStateException("Expected transaction to be active!");
    }
  }
}

@Component
class Invoker {

  public Invoker(List<SomeComponent> components) {
    components.forEach(SomeComponent::init);
  }
}

If the above is bootstrapped with standard @EnableTransactionManagement the instances handed to the constructor of Invoker are JDK proxies and the lookup of the advice chain results in the interceptor for transactions being returned and thus activated. If proxyTargetClass is set to true, the instances received by the constructor are CGLib proxies and the lookup of the advice chain results in an empty one and thus no transaction is created in the first place.


Affects: 4.2.6, 4.3 RC2

Attachments:

Issue Links:

  • DATAJPA-1222 Overriding type level @Transactional does not appear to work in custom repository implementation
  • #19084 Consider target-class proxy mode by default
  • #20072 Revisit storage of null attributes in AbstractFallbackTransaction/CacheAttributeSource
  • #18915 Caching annotation on interface are ignored when cglib proxies are used
  • #19836 Reliably detect @Cacheable declarations on interface methods
  • #19516 Cglib proxy not working with @Async if there is another interceptor

1 votes, 9 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 31, 2016

Oliver Drotbohm commented

Attached a sample using Spring Boot. On 1.3 (with target class proxying disabled by default) the application can be bootstrapped, on 1.4 M3 the bootstrap fails due to the situation described above. Manually enabling target class proxying on 1.3 results in the same problem.

Boot can even be moved out of the picture by using this class to bootstrap the app:

@Configuration
@ComponentScan
@EnableTransactionManagement(proxyTargetClass = true)
public class TxDifferencesApplication {

	@Bean
	public DataSourceTransactionManager transactionManager() {
		return new DataSourceTransactionManager(dataSource());
	}

	@Bean
	public DataSource dataSource() {
		return new EmbeddedDatabaseBuilder().setType(EmbeddedDatabaseType.HSQL).build();
	}

	public static void main(String[] args) {
		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(TxDifferencesApplication.class);
	}
}
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 31, 2016

Oliver Drotbohm commented

I just wondered: isn't that a problem with Spring MVC controllers using @RequestMapping on interfaces, too? If a CGLib proxy is created here, how would Spring MVC find those annotations then?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Stéphane Nicoll commented

We've enabled CGLIB proxies by default in Boot 1.4 and that broke things for our users, see #6693

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 13, 2016

Harald Radi commented

Oliver, it is. I just stumbled upon this issue because of that and due to spring-projects/spring-boot#5423. I don't see any better option than working around the Spring Boot autoconfiguration for now.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

Dave Syer commented

I think the tests in #19516 show that this is not a problem for all interceptors (there's an @Aspect there which is applied to the proxy), but only apparently some (@Async in that case).

@harald you can also work around the issue by not putting the annotations in the interface (kind of makes more sense to put them in the concrete class to me anyway).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

Oliver Drotbohm commented

Putting them on the interface makes sense in cases where the implementation doesn't even show up in kind of documentation (e.g. due to the implementation class being held in package scope) and the annotation functionality being a crucial part of the public contract. I'd argue there's value in knowing a method will be executed transactionally if I just study the interface.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2016

Harald Radi commented

In our specific case we dynamically generate client-proxies, hence i definitely need the @RequestMapping annotations on the interface. And for the @Transactional I'm with @oliver, as part of the contract it needs to go into the interface definition.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 27, 2017

Harald Radi commented

@juergen, it's not just @Transactional but essentially any annotation. In our particular case it is a custom @RestRequestMapping annotation that itself is annotated with @ResponseBody.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 27, 2017

Juergen Hoeller commented

We're tracking handler method annotations separately in #15682, and we got dedicated tickets for @Cacheable (#19836) and @Async (#19516) already. The latter two are interceptor-related like Ollie's original case here, which is why I've changed this ticket to focus on Ollie's @Transactional case. Those three need to be addressed in distinct places but share the basic characteristic of being triggered by an AOP pointcut. Handler methods such as @RequestMapping methods in MVC are actually quite a different case since they are being triggered by an external dispatcher which needs to be able to deal with existing proxies, and we also got the issue of retaining parameter-level annotations in an interface-declared method here (which is technically only possible on Java 8+).

So rest assured, we are revisiting the wider issue, with this ticket as one of four work items. The general target is 5.0 RC1; we're considering backports to the 4.3.x line.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 27, 2017

Harald Radi commented

Thanks for the update, and the effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.