From 172c8b2c3527f5bb8e2a0cb48d0d35302a42766e Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 1 Dec 2024 15:32:03 +0100 Subject: [PATCH 1/2] Polish AOP tests --- .../AspectJAutoProxyCreatorTests.java | 61 +++++----- .../BeanFactoryTransactionTests.java | 111 +++++++++--------- 2 files changed, 88 insertions(+), 84 deletions(-) diff --git a/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java b/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java index 16da2ef070d..bc6f45bfc44 100644 --- a/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java +++ b/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java @@ -85,7 +85,7 @@ class AspectJAutoProxyCreatorTests { void aspectsAreApplied() { ClassPathXmlApplicationContext bf = newContext("aspects.xml"); - ITestBean tb = (ITestBean) bf.getBean("adrian"); + ITestBean tb = bf.getBean("adrian", ITestBean.class); assertThat(tb.getAge()).isEqualTo(68); MethodInvokingFactoryBean factoryBean = (MethodInvokingFactoryBean) bf.getBean("&factoryBean"); assertThat(AopUtils.isAopProxy(factoryBean.getTargetObject())).isTrue(); @@ -96,7 +96,7 @@ class AspectJAutoProxyCreatorTests { void multipleAspectsWithParameterApplied() { ClassPathXmlApplicationContext bf = newContext("aspects.xml"); - ITestBean tb = (ITestBean) bf.getBean("adrian"); + ITestBean tb = bf.getBean("adrian", ITestBean.class); tb.setAge(10); assertThat(tb.getAge()).isEqualTo(20); } @@ -105,7 +105,7 @@ class AspectJAutoProxyCreatorTests { void aspectsAreAppliedInDefinedOrder() { ClassPathXmlApplicationContext bf = newContext("aspectsWithOrdering.xml"); - ITestBean tb = (ITestBean) bf.getBean("adrian"); + ITestBean tb = bf.getBean("adrian", ITestBean.class); assertThat(tb.getAge()).isEqualTo(71); } @@ -113,8 +113,8 @@ class AspectJAutoProxyCreatorTests { void aspectsAndAdvisorAreApplied() { ClassPathXmlApplicationContext ac = newContext("aspectsPlusAdvisor.xml"); - ITestBean shouldBeWeaved = (ITestBean) ac.getBean("adrian"); - doTestAspectsAndAdvisorAreApplied(ac, shouldBeWeaved); + ITestBean shouldBeWoven = ac.getBean("adrian", ITestBean.class); + assertAspectsAndAdvisorAreApplied(ac, shouldBeWoven); } @Test @@ -124,20 +124,22 @@ class AspectJAutoProxyCreatorTests { GenericApplicationContext childAc = new GenericApplicationContext(ac); // Create a child factory with a bean that should be woven RootBeanDefinition bd = new RootBeanDefinition(TestBean.class); - bd.getPropertyValues().addPropertyValue(new PropertyValue("name", "Adrian")) + bd.getPropertyValues() + .addPropertyValue(new PropertyValue("name", "Adrian")) .addPropertyValue(new PropertyValue("age", 34)); childAc.registerBeanDefinition("adrian2", bd); // Register the advisor auto proxy creator with subclass - childAc.registerBeanDefinition(AnnotationAwareAspectJAutoProxyCreator.class.getName(), new RootBeanDefinition( - AnnotationAwareAspectJAutoProxyCreator.class)); + childAc.registerBeanDefinition(AnnotationAwareAspectJAutoProxyCreator.class.getName(), + new RootBeanDefinition(AnnotationAwareAspectJAutoProxyCreator.class)); childAc.refresh(); - ITestBean beanFromChildContextThatShouldBeWeaved = (ITestBean) childAc.getBean("adrian2"); - //testAspectsAndAdvisorAreApplied(childAc, (ITestBean) ac.getBean("adrian")); - doTestAspectsAndAdvisorAreApplied(childAc, beanFromChildContextThatShouldBeWeaved); + ITestBean beanFromParentContextThatShouldBeWoven = ac.getBean("adrian", ITestBean.class); + ITestBean beanFromChildContextThatShouldBeWoven = childAc.getBean("adrian2", ITestBean.class); + assertAspectsAndAdvisorAreApplied(childAc, beanFromParentContextThatShouldBeWoven); + assertAspectsAndAdvisorAreApplied(childAc, beanFromChildContextThatShouldBeWoven); } - protected void doTestAspectsAndAdvisorAreApplied(ApplicationContext ac, ITestBean shouldBeWeaved) { + protected void assertAspectsAndAdvisorAreApplied(ApplicationContext ac, ITestBean shouldBeWoven) { TestBeanAdvisor tba = (TestBeanAdvisor) ac.getBean("advisor"); MultiplyReturnValue mrv = (MultiplyReturnValue) ac.getBean("aspect"); @@ -146,10 +148,10 @@ class AspectJAutoProxyCreatorTests { tba.count = 0; mrv.invocations = 0; - assertThat(AopUtils.isAopProxy(shouldBeWeaved)).as("Autoproxying must apply from @AspectJ aspect").isTrue(); - assertThat(shouldBeWeaved.getName()).isEqualTo("Adrian"); + assertThat(AopUtils.isAopProxy(shouldBeWoven)).as("Autoproxying must apply from @AspectJ aspect").isTrue(); + assertThat(shouldBeWoven.getName()).isEqualTo("Adrian"); assertThat(mrv.invocations).isEqualTo(0); - assertThat(shouldBeWeaved.getAge()).isEqualTo((34 * mrv.getMultiple())); + assertThat(shouldBeWoven.getAge()).isEqualTo((34 * mrv.getMultiple())); assertThat(tba.count).as("Spring advisor must be invoked").isEqualTo(2); assertThat(mrv.invocations).as("Must be able to hold state in aspect").isEqualTo(1); } @@ -158,13 +160,13 @@ class AspectJAutoProxyCreatorTests { void perThisAspect() { ClassPathXmlApplicationContext bf = newContext("perthis.xml"); - ITestBean adrian1 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian1 = bf.getBean("adrian", ITestBean.class); assertThat(AopUtils.isAopProxy(adrian1)).isTrue(); assertThat(adrian1.getAge()).isEqualTo(0); assertThat(adrian1.getAge()).isEqualTo(1); - ITestBean adrian2 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian2 = bf.getBean("adrian", ITestBean.class); assertThat(adrian2).isNotSameAs(adrian1); assertThat(AopUtils.isAopProxy(adrian1)).isTrue(); assertThat(adrian2.getAge()).isEqualTo(0); @@ -178,7 +180,7 @@ class AspectJAutoProxyCreatorTests { void perTargetAspect() throws SecurityException, NoSuchMethodException { ClassPathXmlApplicationContext bf = newContext("pertarget.xml"); - ITestBean adrian1 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian1 = bf.getBean("adrian", ITestBean.class); assertThat(AopUtils.isAopProxy(adrian1)).isTrue(); // Does not trigger advice or count @@ -199,7 +201,7 @@ class AspectJAutoProxyCreatorTests { adrian1.setName("Adrian"); //assertEquals("Any other setter does not increment", 2, adrian1.getAge()); - ITestBean adrian2 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian2 = bf.getBean("adrian", ITestBean.class); assertThat(adrian2).isNotSameAs(adrian1); assertThat(AopUtils.isAopProxy(adrian1)).isTrue(); assertThat(adrian2.getAge()).isEqualTo(34); @@ -239,7 +241,7 @@ class AspectJAutoProxyCreatorTests { void twoAdviceAspect() { ClassPathXmlApplicationContext bf = newContext("twoAdviceAspect.xml"); - ITestBean adrian1 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian1 = bf.getBean("adrian", ITestBean.class); testAgeAspect(adrian1, 0, 2); } @@ -247,9 +249,9 @@ class AspectJAutoProxyCreatorTests { void twoAdviceAspectSingleton() { ClassPathXmlApplicationContext bf = newContext("twoAdviceAspectSingleton.xml"); - ITestBean adrian1 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian1 = bf.getBean("adrian", ITestBean.class); testAgeAspect(adrian1, 0, 1); - ITestBean adrian2 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian2 = bf.getBean("adrian", ITestBean.class); assertThat(adrian2).isNotSameAs(adrian1); testAgeAspect(adrian2, 2, 1); } @@ -258,9 +260,9 @@ class AspectJAutoProxyCreatorTests { void twoAdviceAspectPrototype() { ClassPathXmlApplicationContext bf = newContext("twoAdviceAspectPrototype.xml"); - ITestBean adrian1 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian1 = bf.getBean("adrian", ITestBean.class); testAgeAspect(adrian1, 0, 1); - ITestBean adrian2 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian2 = bf.getBean("adrian", ITestBean.class); assertThat(adrian2).isNotSameAs(adrian1); testAgeAspect(adrian2, 0, 1); } @@ -280,7 +282,7 @@ class AspectJAutoProxyCreatorTests { void adviceUsingJoinPoint() { ClassPathXmlApplicationContext bf = newContext("usesJoinPointAspect.xml"); - ITestBean adrian1 = (ITestBean) bf.getBean("adrian"); + ITestBean adrian1 = bf.getBean("adrian", ITestBean.class); adrian1.getAge(); AdviceUsingThisJoinPoint aspectInstance = (AdviceUsingThisJoinPoint) bf.getBean("aspect"); //(AdviceUsingThisJoinPoint) Aspects.aspectOf(AdviceUsingThisJoinPoint.class); @@ -292,7 +294,7 @@ class AspectJAutoProxyCreatorTests { void includeMechanism() { ClassPathXmlApplicationContext bf = newContext("usesInclude.xml"); - ITestBean adrian = (ITestBean) bf.getBean("adrian"); + ITestBean adrian = bf.getBean("adrian", ITestBean.class); assertThat(AopUtils.isAopProxy(adrian)).isTrue(); assertThat(adrian.getAge()).isEqualTo(68); } @@ -310,7 +312,7 @@ class AspectJAutoProxyCreatorTests { void withAbstractFactoryBeanAreApplied() { ClassPathXmlApplicationContext bf = newContext("aspectsWithAbstractBean.xml"); - ITestBean adrian = (ITestBean) bf.getBean("adrian"); + ITestBean adrian = bf.getBean("adrian", ITestBean.class); assertThat(AopUtils.isAopProxy(adrian)).isTrue(); assertThat(adrian.getAge()).isEqualTo(68); } @@ -321,8 +323,7 @@ class AspectJAutoProxyCreatorTests { UnreliableBean bean = (UnreliableBean) bf.getBean("unreliableBean"); RetryAspect aspect = (RetryAspect) bf.getBean("retryAspect"); - int attempts = bean.unreliable(); - assertThat(attempts).isEqualTo(2); + assertThat(bean.unreliable()).isEqualTo(2); assertThat(aspect.getBeginCalls()).isEqualTo(2); assertThat(aspect.getRollbackCalls()).isEqualTo(1); assertThat(aspect.getCommitCalls()).isEqualTo(1); @@ -332,7 +333,7 @@ class AspectJAutoProxyCreatorTests { void withBeanNameAutoProxyCreator() { ClassPathXmlApplicationContext bf = newContext("withBeanNameAutoProxyCreator.xml"); - ITestBean tb = (ITestBean) bf.getBean("adrian"); + ITestBean tb = bf.getBean("adrian", ITestBean.class); assertThat(tb.getAge()).isEqualTo(68); } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java index f8fd380f6f7..169ac138ed0 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java @@ -56,59 +56,54 @@ import static org.mockito.Mockito.verifyNoInteractions; */ class BeanFactoryTransactionTests { - private DefaultListableBeanFactory factory; + private final DefaultListableBeanFactory factory = new DefaultListableBeanFactory(); @BeforeEach - void setUp() { - this.factory = new DefaultListableBeanFactory(); + void loadBeanDefinitions() { new XmlBeanDefinitionReader(this.factory).loadBeanDefinitions( new ClassPathResource("transactionalBeanFactory.xml", getClass())); } @Test - void testGetsAreNotTransactionalWithProxyFactory1() { - ITestBean testBean = (ITestBean) factory.getBean("proxyFactory1"); + void getsAreNotTransactionalWithProxyFactory1() { + ITestBean testBean = factory.getBean("proxyFactory1", ITestBean.class); assertThat(Proxy.isProxyClass(testBean.getClass())).as("testBean is a dynamic proxy").isTrue(); - boolean condition = testBean instanceof TransactionalProxy; - assertThat(condition).isFalse(); - doTestGetsAreNotTransactional(testBean); + assertThat(testBean).isNotInstanceOf(TransactionalProxy.class); + assertGetsAreNotTransactional(testBean); } @Test - void testGetsAreNotTransactionalWithProxyFactory2DynamicProxy() { + void getsAreNotTransactionalWithProxyFactory2DynamicProxy() { this.factory.preInstantiateSingletons(); - ITestBean testBean = (ITestBean) factory.getBean("proxyFactory2DynamicProxy"); + ITestBean testBean = factory.getBean("proxyFactory2DynamicProxy", ITestBean.class); assertThat(Proxy.isProxyClass(testBean.getClass())).as("testBean is a dynamic proxy").isTrue(); - boolean condition = testBean instanceof TransactionalProxy; - assertThat(condition).isTrue(); - doTestGetsAreNotTransactional(testBean); + assertThat(testBean).isInstanceOf(TransactionalProxy.class); + assertGetsAreNotTransactional(testBean); } @Test - void testGetsAreNotTransactionalWithProxyFactory2Cglib() { - ITestBean testBean = (ITestBean) factory.getBean("proxyFactory2Cglib"); + void getsAreNotTransactionalWithProxyFactory2Cglib() { + ITestBean testBean = factory.getBean("proxyFactory2Cglib", ITestBean.class); assertThat(AopUtils.isCglibProxy(testBean)).as("testBean is CGLIB advised").isTrue(); - boolean condition = testBean instanceof TransactionalProxy; - assertThat(condition).isTrue(); - doTestGetsAreNotTransactional(testBean); + assertThat(testBean).isInstanceOf(TransactionalProxy.class); + assertGetsAreNotTransactional(testBean); } @Test - void testProxyFactory2Lazy() { - ITestBean testBean = (ITestBean) factory.getBean("proxyFactory2Lazy"); + void proxyFactory2Lazy() { + ITestBean testBean = factory.getBean("proxyFactory2Lazy", ITestBean.class); assertThat(factory.containsSingleton("target")).isFalse(); assertThat(testBean.getAge()).isEqualTo(666); assertThat(factory.containsSingleton("target")).isTrue(); } @Test - void testCglibTransactionProxyImplementsNoInterfaces() { - ImplementsNoInterfaces ini = (ImplementsNoInterfaces) factory.getBean("cglibNoInterfaces"); + void cglibTransactionProxyImplementsNoInterfaces() { + ImplementsNoInterfaces ini = factory.getBean("cglibNoInterfaces", ImplementsNoInterfaces.class); assertThat(AopUtils.isCglibProxy(ini)).as("testBean is CGLIB advised").isTrue(); - boolean condition = ini instanceof TransactionalProxy; - assertThat(condition).isTrue(); + assertThat(ini).isInstanceOf(TransactionalProxy.class); String newName = "Gordon"; // Install facade @@ -121,31 +116,41 @@ class BeanFactoryTransactionTests { } @Test - void testGetsAreNotTransactionalWithProxyFactory3() { - ITestBean testBean = (ITestBean) factory.getBean("proxyFactory3"); - boolean condition = testBean instanceof DerivedTestBean; - assertThat(condition).as("testBean is a full proxy").isTrue(); - boolean condition1 = testBean instanceof TransactionalProxy; - assertThat(condition1).isTrue(); - InvocationCounterPointcut txnCounter = (InvocationCounterPointcut) factory.getBean("txnInvocationCounterPointcut"); - InvocationCounterInterceptor preCounter = (InvocationCounterInterceptor) factory.getBean("preInvocationCounterInterceptor"); - InvocationCounterInterceptor postCounter = (InvocationCounterInterceptor) factory.getBean("postInvocationCounterInterceptor"); - txnCounter.counter = 0; - preCounter.counter = 0; - postCounter.counter = 0; - doTestGetsAreNotTransactional(testBean); - // Can't assert it's equal to 4 as the pointcut may be optimized and only invoked once - assertThat(0 < txnCounter.counter && txnCounter.counter <= 4).isTrue(); - assertThat(preCounter.counter).isEqualTo(4); - assertThat(postCounter.counter).isEqualTo(4); + void getsAreNotTransactionalWithProxyFactory3() { + ITestBean testBean = factory.getBean("proxyFactory3", ITestBean.class); + assertThat(testBean).as("testBean is a full proxy") + .isInstanceOf(DerivedTestBean.class) + .isInstanceOf(TransactionalProxy.class); + + InvocationCounterPointcut txnPointcut = factory.getBean("txnInvocationCounterPointcut", InvocationCounterPointcut.class); + InvocationCounterInterceptor preInterceptor = factory.getBean("preInvocationCounterInterceptor", InvocationCounterInterceptor.class); + InvocationCounterInterceptor postInterceptor = factory.getBean("postInvocationCounterInterceptor", InvocationCounterInterceptor.class); + assertThat(txnPointcut.counter).as("txnPointcut").isGreaterThan(0); + assertThat(preInterceptor.counter).as("preInterceptor").isZero(); + assertThat(postInterceptor.counter).as("postInterceptor").isZero(); + + // Reset counters + txnPointcut.counter = 0; + preInterceptor.counter = 0; + postInterceptor.counter = 0; + + // Invokes: getAge() * 2 and setAge() * 1 --> 2 + 1 = 3 method invocations. + assertGetsAreNotTransactional(testBean); + + // The transaction pointcut is currently asked if it matches() for all method + // invocations, but we cannot assert it's equal to 3 since the pointcut may be + // optimized and only invoked once. + assertThat(txnPointcut.counter).as("txnPointcut").isGreaterThanOrEqualTo(1).isLessThanOrEqualTo(3); + assertThat(preInterceptor.counter).as("preInterceptor").isEqualTo(3); + assertThat(postInterceptor.counter).as("postInterceptor").isEqualTo(3); } - private void doTestGetsAreNotTransactional(final ITestBean testBean) { + private void assertGetsAreNotTransactional(final ITestBean testBean) { // Install facade PlatformTransactionManager ptm = mock(); PlatformTransactionManagerFacade.delegate = ptm; - assertThat(testBean.getAge()).as("Age should not be " + testBean.getAge()).isEqualTo(666); + assertThat(testBean.getAge()).as("Age").isEqualTo(666); // Expect no methods verifyNoInteractions(ptm); @@ -177,14 +182,14 @@ class BeanFactoryTransactionTests { }; PlatformTransactionManagerFacade.delegate = ptm; - // TODO same as old age to avoid ordering effect for now + // same as old age to avoid ordering effect for now int age = 666; testBean.setAge(age); assertThat(testBean.getAge()).isEqualTo(age); } @Test - void testGetBeansOfTypeWithAbstract() { + void getBeansOfTypeWithAbstract() { Map beansOfType = factory.getBeansOfType(ITestBean.class, true, true); assertThat(beansOfType).isNotNull(); } @@ -193,24 +198,22 @@ class BeanFactoryTransactionTests { * Check that we fail gracefully if the user doesn't set any transaction attributes. */ @Test - void testNoTransactionAttributeSource() { - assertThatExceptionOfType(FatalBeanException.class).isThrownBy(() -> { - DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); - new XmlBeanDefinitionReader(bf).loadBeanDefinitions(new ClassPathResource("noTransactionAttributeSource.xml", getClass())); - bf.getBean("noTransactionAttributeSource"); - }); + void noTransactionAttributeSource() { + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + new XmlBeanDefinitionReader(bf).loadBeanDefinitions(new ClassPathResource("noTransactionAttributeSource.xml", getClass())); + assertThatExceptionOfType(FatalBeanException.class).isThrownBy(() -> bf.getBean("noTransactionAttributeSource")); } /** * Test that we can set the target to a dynamic TargetSource. */ @Test - void testDynamicTargetSource() { + void dynamicTargetSource() { // Install facade CallCountingTransactionManager txMan = new CallCountingTransactionManager(); PlatformTransactionManagerFacade.delegate = txMan; - TestBean tb = (TestBean) factory.getBean("hotSwapped"); + TestBean tb = factory.getBean("hotSwapped", TestBean.class); assertThat(tb.getAge()).isEqualTo(666); int newAge = 557; tb.setAge(newAge); @@ -218,7 +221,7 @@ class BeanFactoryTransactionTests { TestBean target2 = new TestBean(); target2.setAge(65); - HotSwappableTargetSource ts = (HotSwappableTargetSource) factory.getBean("swapper"); + HotSwappableTargetSource ts = factory.getBean("swapper", HotSwappableTargetSource.class); ts.swap(target2); assertThat(tb.getAge()).isEqualTo(target2.getAge()); tb.setAge(newAge); From 320831b18ad7e8f0d0f0e9072f287ba699e96a21 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 1 Dec 2024 15:38:15 +0100 Subject: [PATCH 2/2] Test status quo for StaticMethodMatcherPointcut#matches invocations This commit introduces a test which verifies how many times the matches() method of a StaticMethodMatcherPointcut is invoked during ApplicationContext startup as well as during actual method invocations via the advice chain, which also indirectly tests the behavior of the equals() implementation in AdvisedSupport.MethodCacheKey. In addition, this commit revises BeanFactoryTransactionTests to assert that a transaction is started for the setAge() method. See gh-33915 --- .../DefaultAdvisorAutoProxyCreatorTests.java | 95 +++++++++++++++++++ .../BeanFactoryTransactionTests.java | 29 +++--- 2 files changed, 108 insertions(+), 16 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java diff --git a/spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java b/spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java new file mode 100644 index 00000000000..a9c723a04fa --- /dev/null +++ b/spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java @@ -0,0 +1,95 @@ +/* + * Copyright 2002-2024 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.aop.framework.autoproxy; + +import java.lang.reflect.Method; + +import org.aopalliance.aop.Advice; +import org.aopalliance.intercept.MethodInterceptor; +import org.junit.jupiter.api.Test; + +import org.springframework.aop.Pointcut; +import org.springframework.aop.support.AbstractPointcutAdvisor; +import org.springframework.aop.support.RootClassFilter; +import org.springframework.aop.support.StaticMethodMatcherPointcut; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration tests for {@link DefaultAdvisorAutoProxyCreator}. + * + * @author Sam Brannen + * @since 6.2.1 + */ +class DefaultAdvisorAutoProxyCreatorTests { + + /** + * Indirectly tests behavior of {@link org.springframework.aop.framework.AdvisedSupport.MethodCacheKey}. + * @see StaticMethodMatcherPointcut#matches(Method, Class) + */ + @Test // gh-33915 + void staticMethodMatcherPointcutMatchesMethodIsInvokedAgainForActualMethodInvocation() { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + DemoBean.class, DemoPointcutAdvisor.class, DefaultAdvisorAutoProxyCreator.class); + DemoPointcutAdvisor demoPointcutAdvisor = context.getBean(DemoPointcutAdvisor.class); + DemoBean demoBean = context.getBean(DemoBean.class); + + assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations before").isEqualTo(2); + assertThat(demoBean.sayHello()).isEqualTo("Advised: Hello!"); + assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations after").isEqualTo(3); + + context.close(); + } + + + static class DemoBean { + + public String sayHello() { + return "Hello!"; + } + } + + @SuppressWarnings("serial") + static class DemoPointcutAdvisor extends AbstractPointcutAdvisor { + + int matchesInvocationCount = 0; + + @Override + public Pointcut getPointcut() { + StaticMethodMatcherPointcut pointcut = new StaticMethodMatcherPointcut() { + + @Override + public boolean matches(Method method, Class targetClass) { + if (method.getName().equals("sayHello")) { + matchesInvocationCount++; + return true; + } + return false; + } + }; + pointcut.setClassFilter(new RootClassFilter(DemoBean.class)); + return pointcut; + } + + @Override + public Advice getAdvice() { + return (MethodInterceptor) invocation -> "Advised: " + invocation.proceed(); + } + } + +} diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java index 169ac138ed0..0fe3bef54a6 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java @@ -19,6 +19,7 @@ package org.springframework.transaction.interceptor; import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; @@ -52,6 +53,7 @@ import static org.mockito.Mockito.verifyNoInteractions; * * @author Rod Johnson * @author Juergen Hoeller + * @author Sam Brannen * @since 23.04.2003 */ class BeanFactoryTransactionTests { @@ -145,30 +147,25 @@ class BeanFactoryTransactionTests { assertThat(postInterceptor.counter).as("postInterceptor").isEqualTo(3); } - private void assertGetsAreNotTransactional(final ITestBean testBean) { + private void assertGetsAreNotTransactional(ITestBean testBean) { // Install facade PlatformTransactionManager ptm = mock(); PlatformTransactionManagerFacade.delegate = ptm; assertThat(testBean.getAge()).as("Age").isEqualTo(666); - // Expect no methods + // Expect no interactions with the transaction manager. verifyNoInteractions(ptm); // Install facade expecting a call - final TransactionStatus ts = mock(); + AtomicBoolean invoked = new AtomicBoolean(); + TransactionStatus ts = mock(); ptm = new PlatformTransactionManager() { - private boolean invoked; @Override public TransactionStatus getTransaction(@Nullable TransactionDefinition def) throws TransactionException { - if (invoked) { - throw new IllegalStateException("getTransaction should not get invoked more than once"); - } - invoked = true; - if (!(def.getName().contains(DerivedTestBean.class.getName()) && def.getName().contains("setAge"))) { - throw new IllegalStateException( - "transaction name should contain class and method name: " + def.getName()); - } + assertThat(invoked.compareAndSet(false, true)) + .as("getTransaction() should not get invoked more than once").isTrue(); + assertThat(def.getName()).as("transaction name").contains(DerivedTestBean.class.getName(), "setAge"); return ts; } @Override @@ -182,10 +179,10 @@ class BeanFactoryTransactionTests { }; PlatformTransactionManagerFacade.delegate = ptm; - // same as old age to avoid ordering effect for now - int age = 666; - testBean.setAge(age); - assertThat(testBean.getAge()).isEqualTo(age); + assertThat(invoked).as("getTransaction() invoked before setAge()").isFalse(); + testBean.setAge(42); + assertThat(invoked).as("getTransaction() invoked after setAge()").isTrue(); + assertThat(testBean.getAge()).as("Age").isEqualTo(42); } @Test