From c950e018f50e516bd97dc7681d11d9f38405b188 Mon Sep 17 00:00:00 2001 From: Semyon Danilov Date: Tue, 17 Sep 2019 09:53:13 +0300 Subject: [PATCH] fix tx manager cache not cleared in aspect --- integration-tests/integration-tests.gradle | 10 + ...TransactionManagementIntegrationTests.java | 10 +- .../AnnotationTransactionAspectTest.java | 201 ++++++++++++++++++ ...JtaTransactionManagementConfiguration.java | 7 +- ...ctJTransactionManagementConfiguration.java | 7 +- .../interceptor/TransactionAspectSupport.java | 3 +- 6 files changed, 225 insertions(+), 13 deletions(-) create mode 100644 integration-tests/src/test/java/org/springframework/transaction/interceptor/AnnotationTransactionAspectTest.java diff --git a/integration-tests/integration-tests.gradle b/integration-tests/integration-tests.gradle index 6e285d8471ae..bdc92b14c56e 100644 --- a/integration-tests/integration-tests.gradle +++ b/integration-tests/integration-tests.gradle @@ -1,7 +1,16 @@ description = "Spring Integration Tests" +apply plugin: "io.freefair.aspectj" apply plugin: "org.springframework.build.test-sources" +sourceSets.main.aspectj.srcDir "src/main/java" +sourceSets.main.java.srcDirs = files() + +sourceSets.test.aspectj.srcDir "src/test/java" +sourceSets.test.java.srcDirs = files() + +aspectj.version = dependencyManagement.managedVersions['org.aspectj:aspectjweaver'] + dependencies { testCompile(project(":spring-aop")) testCompile(project(":spring-beans")) @@ -13,6 +22,7 @@ dependencies { testCompile(project(":spring-test")) testCompile(project(":spring-tx")) testCompile(project(":spring-web")) + aspect(project(":spring-aspects")) testCompile("javax.inject:javax.inject") testCompile("javax.resource:javax.resource-api") testCompile("javax.servlet:javax.servlet-api") diff --git a/integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java b/integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java index 20dfe81d2f86..589111277fef 100644 --- a/integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java +++ b/integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java @@ -42,6 +42,7 @@ import org.springframework.stereotype.Repository; import org.springframework.tests.transaction.CallCountingTransactionManager; import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.aspectj.AspectJJtaTransactionManagementConfiguration; import org.springframework.transaction.interceptor.BeanFactoryTransactionAttributeSourceAdvisor; import static org.assertj.core.api.Assertions.assertThat; @@ -95,12 +96,9 @@ void repositoryIsClassBasedTxProxy() { void repositoryUsesAspectJAdviceMode() { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); ctx.register(Config.class, AspectJTxConfig.class); - // this test is a bit fragile, but gets the job done, proving that an - // attempt was made to look up the AJ aspect. It's due to classpath issues - // in .integration-tests that it's not found. - assertThatExceptionOfType(Exception.class) - .isThrownBy(ctx::refresh) - .withMessageContaining("AspectJJtaTransactionManagementConfiguration"); + ctx.refresh(); + final AspectJJtaTransactionManagementConfiguration config = (AspectJJtaTransactionManagementConfiguration) ctx.getBean("org.springframework.transaction.aspectj.AspectJJtaTransactionManagementConfiguration"); + assertThat(config).isNotNull(); } @Test diff --git a/integration-tests/src/test/java/org/springframework/transaction/interceptor/AnnotationTransactionAspectTest.java b/integration-tests/src/test/java/org/springframework/transaction/interceptor/AnnotationTransactionAspectTest.java new file mode 100644 index 000000000000..4724ecf5ced1 --- /dev/null +++ b/integration-tests/src/test/java/org/springframework/transaction/interceptor/AnnotationTransactionAspectTest.java @@ -0,0 +1,201 @@ +/* + * Copyright 2002-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.transaction.interceptor; + +import org.junit.jupiter.api.Test; +import org.springframework.context.annotation.AdviceMode; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.jdbc.datasource.DataSourceTransactionManager; +import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder; +import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseType; +import org.springframework.stereotype.Repository; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionManager; +import org.springframework.transaction.annotation.EnableTransactionManagement; +import org.springframework.transaction.annotation.TransactionManagementConfigurer; + +import javax.sql.DataSource; +import java.util.Collections; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests proving that after the creation of new context, transaction aspects are cleared of + * transaction managers created in previous context. + * + * @author Semyon Danilov + */ +class AnnotationTransactionAspectTest { + + private static final String TRANSACTION_MANAGER_KEY = "org.springframework.transaction.config.internalTransactionAspect"; + private static final String JAVAX_TRANSACTION_MANAGER_KEY = "org.springframework.transaction.config.internalJtaTransactionAspect"; + + @Test + void testAspectTxManagerCacheCleanInNewContext() { + // stub txAttr to get txManager + TransactionAttribute txAttr = attr(); + + // create first context + AnnotationConfigApplicationContext ctx1 = new AnnotationConfigApplicationContext(); + ctx1.register(Config.class, AspectJTxConfig.class); + ctx1.refresh(); + + // invoke transactional method thus initializing txManagerCache in aspect + final JdbcFooRepository repository1 = ctx1.getBean(JdbcFooRepository.class); + repository1.findAll(); + repository1.findAllJta(); + + // get aspect bean (it's a real singleton, it's one per jvm) + TransactionAspectSupport supportBean = ctx1.getBeansOfType(TransactionAspectSupport.class).get(TRANSACTION_MANAGER_KEY); + TransactionAspectSupport supportBeanJavax = ctx1.getBeansOfType(TransactionAspectSupport.class).get(JAVAX_TRANSACTION_MANAGER_KEY); + + // get txManager + final PlatformTransactionManager firstManager = supportBean.determineTransactionManager(txAttr); + final PlatformTransactionManager firstManagerJavax = supportBeanJavax.determineTransactionManager(txAttr); + + // create second context, little different from first + AnnotationConfigApplicationContext ctx2 = new AnnotationConfigApplicationContext(); + ctx2.register(Config.class, AspectJTxConfig.class, SomeOtherConfig.class); + ctx2.refresh(); + + // invoke transactional method thus initializing txManagerCache in aspect + final JdbcFooRepository repository2 = ctx2.getBean(JdbcFooRepository.class); + repository2.findAll(); + repository2.findAllJta(); + + // get new txManager + final PlatformTransactionManager secondManager = supportBean.determineTransactionManager(txAttr); + final PlatformTransactionManager secondManagerJavax = supportBeanJavax.determineTransactionManager(txAttr); + + // check that txManager was NOT cached + assertThat(firstManager).isNotEqualTo(secondManager); + assertThat(firstManagerJavax).isNotEqualTo(secondManagerJavax); + } + + + @Test + void testAspectTxManagerFieldCleanInNewContext() { + // stub txAttr to get txManager + TransactionAttribute txAttr = attr(); + + // create first context + AnnotationConfigApplicationContext ctx1 = new AnnotationConfigApplicationContext(); + + // this config will create default transaction manager + ctx1.register(Config.class, AspectJTxConfig.class, ConfigWithTxManagementConfigurer.class); + ctx1.refresh(); + + // get aspect bean (it's a real singleton, it's one per jvm) + TransactionAspectSupport supportBean = ctx1.getBeansOfType(TransactionAspectSupport.class).get(TRANSACTION_MANAGER_KEY); + TransactionAspectSupport supportBeanJavax = ctx1.getBeansOfType(TransactionAspectSupport.class).get(JAVAX_TRANSACTION_MANAGER_KEY); + + // get txManager + final PlatformTransactionManager firstManager = supportBean.determineTransactionManager(txAttr); + final PlatformTransactionManager firstManagerJavax = supportBeanJavax.determineTransactionManager(txAttr); + + // create second context, little different from first + AnnotationConfigApplicationContext ctx2 = new AnnotationConfigApplicationContext(); + + // this config is without default transaction manager, so it shouldn't be in aspect + ctx2.register(Config.class, AspectJTxConfig.class, SomeOtherConfig.class); + ctx2.refresh(); + + // get new txManager + final PlatformTransactionManager secondManager = supportBean.determineTransactionManager(txAttr); + final PlatformTransactionManager secondManagerJavax = supportBeanJavax.determineTransactionManager(txAttr); + + // check that txManager is not the same + assertThat(firstManager).isNotEqualTo(secondManager); + assertThat(firstManagerJavax).isNotEqualTo(secondManagerJavax); + } + + @Configuration + @EnableTransactionManagement(mode = AdviceMode.ASPECTJ) + static class Config { + + @Bean + DataSource dataSource() { + return new EmbeddedDatabaseBuilder() + .setType(EmbeddedDatabaseType.HSQL) + .build(); + } + + @Bean + JdbcFooRepository repository() { + return new JdbcFooRepository(); + } + } + + @Configuration + static class AspectJTxConfig { + + @Bean + PlatformTransactionManager transactionManager(DataSource dataSource) { + return new DataSourceTransactionManager(dataSource); + } + } + + @Configuration + static class SomeOtherConfig { + + } + + static class ConfigWithTxManagementConfigurer { + @Bean + TransactionManagementConfigurer configurer(DataSource dataSource) { + return new TransactionManagementConfigurer() { + @Override + public TransactionManager annotationDrivenTransactionManager() { + return new DataSourceTransactionManager(dataSource); + } + }; + } + } + + @Repository + static class JdbcFooRepository { + + @org.springframework.transaction.annotation.Transactional + public List findAll() { + return Collections.emptyList(); + } + + @javax.transaction.Transactional + public List findAllJta() { + return Collections.emptyList(); + } + } + + private TransactionAttribute attr() { + return new TransactionAttribute() { + + @Override + public String getQualifier() { + return ""; + } + + @Override + public boolean rollbackOn(Throwable ex) { + return false; + } + }; + } + +} diff --git a/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJJtaTransactionManagementConfiguration.java b/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJJtaTransactionManagementConfiguration.java index caa7cc3c2f72..9667682c7624 100644 --- a/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJJtaTransactionManagementConfiguration.java +++ b/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJJtaTransactionManagementConfiguration.java @@ -31,6 +31,7 @@ * {@link org.springframework.transaction.annotation.Transactional} annotation. * * @author Juergen Hoeller + * @author Semyon Danilov * @since 5.1 * @see EnableTransactionManagement * @see TransactionManagementConfigurationSelector @@ -42,9 +43,9 @@ public class AspectJJtaTransactionManagementConfiguration extends AspectJTransac @Role(BeanDefinition.ROLE_INFRASTRUCTURE) public JtaAnnotationTransactionAspect jtaTransactionAspect() { JtaAnnotationTransactionAspect txAspect = JtaAnnotationTransactionAspect.aspectOf(); - if (this.txManager != null) { - txAspect.setTransactionManager(this.txManager); - } + // aspect is a singleton, so it must be cleared every time bean is created + txAspect.clearTransactionManagerCache(); + txAspect.setTransactionManager(this.txManager); return txAspect; } diff --git a/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJTransactionManagementConfiguration.java b/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJTransactionManagementConfiguration.java index 2784a733e7c2..83ae3b8bf5ee 100644 --- a/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJTransactionManagementConfiguration.java +++ b/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AspectJTransactionManagementConfiguration.java @@ -32,6 +32,7 @@ * * @author Chris Beams * @author Juergen Hoeller + * @author Semyon Danilov * @since 3.1 * @see EnableTransactionManagement * @see TransactionManagementConfigurationSelector @@ -44,9 +45,9 @@ public class AspectJTransactionManagementConfiguration extends AbstractTransacti @Role(BeanDefinition.ROLE_INFRASTRUCTURE) public AnnotationTransactionAspect transactionAspect() { AnnotationTransactionAspect txAspect = AnnotationTransactionAspect.aspectOf(); - if (this.txManager != null) { - txAspect.setTransactionManager(this.txManager); - } + // aspect is a singleton, so it must be cleared every time bean is created + txAspect.clearTransactionManagerCache(); + txAspect.setTransactionManager(this.txManager); return txAspect; } diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java index c8150bef556a..f8f19da6bcd3 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java @@ -78,6 +78,7 @@ * @author Stéphane Nicoll * @author Sam Brannen * @author Mark Paluch + * @author Semyon Danilov * @since 1.1 * @see PlatformTransactionManager * @see ReactiveTransactionManager @@ -437,7 +438,7 @@ protected Object invokeWithinTransaction(Method method, @Nullable Class targe /** * Clear the cache. */ - protected void clearTransactionManagerCache() { + public void clearTransactionManagerCache() { this.transactionManagerCache.clear(); this.beanFactory = null; }