Merge branch '5.2.x'

integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright 2002-2020 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.config;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+/**
+ * Integration tests for advice invocation order for advice configured via the
+ * AOP namespace.
+ *
+ * @author Sam Brannen
+ * @since 5.0.18
+ * @see org.springframework.aop.framework.autoproxy.AspectJAutoProxyAdviceOrderIntegrationTests
+ */
+class AopNamespaceHandlerAdviceOrderIntegrationTests {
+
+	@Nested
+	@SpringJUnitConfig(locations = "AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml")
+	@DirtiesContext
+	class AfterAdviceFirstTests {
+
+		@Test
+		void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired EchoAspect aspect) throws Exception {
+			assertThat(aspect.invocations).isEmpty();
+			assertThat(echo.echo(42)).isEqualTo(42);
+			assertThat(aspect.invocations).containsExactly("after", "after returning");
+
+			aspect.invocations.clear();
+			assertThatExceptionOfType(Exception.class).isThrownBy(() -> echo.echo(new Exception()));
+			assertThat(aspect.invocations).containsExactly("after", "after throwing");
+		}
+	}
+
+	@Nested
+	@SpringJUnitConfig(locations = "AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml")
+	@DirtiesContext
+	class AfterAdviceLastTests {
+
+		@Test
+		void afterAdviceIsInvokedLast(@Autowired Echo echo, @Autowired EchoAspect aspect) throws Exception {
+			assertThat(aspect.invocations).isEmpty();
+			assertThat(echo.echo(42)).isEqualTo(42);
+			assertThat(aspect.invocations).containsExactly("after returning", "after");
+
+			aspect.invocations.clear();
+			assertThatExceptionOfType(Exception.class).isThrownBy(() -> echo.echo(new Exception()));
+			assertThat(aspect.invocations).containsExactly("after throwing", "after");
+		}
+	}
+
+
+	static class Echo {
+
+		Object echo(Object obj) throws Exception {
+			if (obj instanceof Exception) {
+				throw (Exception) obj;
+			}
+			return obj;
+		}
+	}
+
+	static class EchoAspect {
+
+		List<String> invocations = new ArrayList<>();
+
+		void echo() {
+		}
+
+		void succeeded() {
+			invocations.add("after returning");
+		}
+
+		void failed() {
+			invocations.add("after throwing");
+		}
+
+		void after() {
+			invocations.add("after");
+		}
+	}
+
+}

integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java

@@ -0,0 +1,186 @@
+/*
+ * Copyright 2002-2020 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.util.ArrayList;
+import java.util.List;
+
+import org.aspectj.lang.annotation.After;
+import org.aspectj.lang.annotation.AfterReturning;
+import org.aspectj.lang.annotation.AfterThrowing;
+import org.aspectj.lang.annotation.Aspect;
+import org.aspectj.lang.annotation.Pointcut;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.EnableAspectJAutoProxy;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+/**
+ * Integration tests for advice invocation order for advice configured via
+ * AspectJ auto-proxy support.
+ *
+ * @author Sam Brannen
+ * @since 5.0.18
+ * @see org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests
+ */
+class AspectJAutoProxyAdviceOrderIntegrationTests {
+
+	@Nested
+	@SpringJUnitConfig(AfterAdviceFirstConfig.class)
+	@DirtiesContext
+	class AfterAdviceFirstTests {
+
+		@Test
+		void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired AfterAdviceFirstAspect aspect) throws Exception {
+			assertThat(aspect.invocations).isEmpty();
+			assertThat(echo.echo(42)).isEqualTo(42);
+			assertThat(aspect.invocations).containsExactly("after", "after returning");
+
+			aspect.invocations.clear();
+			assertThatExceptionOfType(Exception.class).isThrownBy(
+					() -> echo.echo(new Exception()));
+			assertThat(aspect.invocations).containsExactly("after", "after throwing");
+		}
+	}
+
+
+	@Nested
+	@SpringJUnitConfig(AfterAdviceLastConfig.class)
+	@DirtiesContext
+	class AfterAdviceLastTests {
+
+		@Test
+		void afterAdviceIsNotInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceLastAspect aspect) throws Exception {
+			assertThat(aspect.invocations).isEmpty();
+			assertThat(echo.echo(42)).isEqualTo(42);
+			// On Java versions prior to JDK 7, we would expect the @After advice to be invoked last.
+			assertThat(aspect.invocations).containsExactly("after", "after returning");
+
+			aspect.invocations.clear();
+			assertThatExceptionOfType(Exception.class).isThrownBy(
+					() -> echo.echo(new Exception()));
+			// On Java versions prior to JDK 7, we would expect the @After advice to be invoked last.
+			assertThat(aspect.invocations).containsExactly("after", "after throwing");
+		}
+	}
+
+
+	@Configuration
+	@EnableAspectJAutoProxy(proxyTargetClass = true)
+	static class AfterAdviceFirstConfig {
+
+		@Bean
+		AfterAdviceFirstAspect echoAspect() {
+			return new AfterAdviceFirstAspect();
+		}
+
+		@Bean
+		Echo echo() {
+			return new Echo();
+		}
+	}
+
+	@Configuration
+	@EnableAspectJAutoProxy(proxyTargetClass = true)
+	static class AfterAdviceLastConfig {
+
+		@Bean
+		AfterAdviceLastAspect echoAspect() {
+			return new AfterAdviceLastAspect();
+		}
+
+		@Bean
+		Echo echo() {
+			return new Echo();
+		}
+	}
+
+	static class Echo {
+
+		Object echo(Object obj) throws Exception {
+			if (obj instanceof Exception) {
+				throw (Exception) obj;
+			}
+			return obj;
+		}
+	}
+
+	/**
+	 * {@link After @After} advice declared as first <em>after</em> method in source code.
+	 */
+	@Aspect
+	static class AfterAdviceFirstAspect {
+
+		List<String> invocations = new ArrayList<>();
+
+		@Pointcut("execution(* echo(*))")
+		void echo() {
+		}
+
+		@After("echo()")
+		void after() {
+			invocations.add("after");
+		}
+
+		@AfterReturning("echo()")
+		void succeeded() {
+			invocations.add("after returning");
+		}
+
+		@AfterThrowing("echo()")
+		void failed() {
+			invocations.add("after throwing");
+		}
+	}
+
+	/**
+	 * {@link After @After} advice declared as last <em>after</em> method in source code.
+	 */
+	@Aspect
+	static class AfterAdviceLastAspect {
+
+		List<String> invocations = new ArrayList<>();
+
+		@Pointcut("execution(* echo(*))")
+		void echo() {
+		}
+
+		@AfterReturning("echo()")
+		void succeeded() {
+			invocations.add("after returning");
+		}
+
+		@AfterThrowing("echo()")
+		void failed() {
+			invocations.add("after throwing");
+		}
+
+		@After("echo()")
+		void after() {
+			invocations.add("after");
+		}
+	}
+
+}

integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml

@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans"
+		xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+		xmlns:aop="http://www.springframework.org/schema/aop"
+		xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
+				http://www.springframework.org/schema/aop https://www.springframework.org/schema/aop/spring-aop.xsd">
+
+	<bean id="echo" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$Echo"/>
+
+	<bean id="echoAspect" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$EchoAspect" />
+
+	<aop:config>
+		<aop:aspect id="echoAdvice" ref="echoAspect">
+			<aop:pointcut id="echoMethod" expression="execution(* echo(*))" />
+			<aop:after method="after" pointcut-ref="echoMethod" />
+			<aop:after-throwing method="failed" pointcut-ref="echoMethod" />
+			<aop:after-returning method="succeeded" pointcut-ref="echoMethod" />
+		</aop:aspect>
+	</aop:config>
+
+</beans>

integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml

@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans"
+		xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+		xmlns:aop="http://www.springframework.org/schema/aop"
+		xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
+				http://www.springframework.org/schema/aop https://www.springframework.org/schema/aop/spring-aop.xsd">
+
+	<bean id="echo" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$Echo"/>
+
+	<bean id="echoAspect" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$EchoAspect" />
+
+	<aop:config>
+		<aop:aspect id="echoAdvice" ref="echoAspect">
+			<aop:pointcut id="echoMethod" expression="execution(* echo(*))" />
+			<aop:after-returning method="succeeded" pointcut-ref="echoMethod" />
+			<aop:after-throwing method="failed" pointcut-ref="echoMethod" />
+			<aop:after method="after" pointcut-ref="echoMethod" />
+		</aop:aspect>
+	</aop:config>
+
+</beans>

spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 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.
@@ -22,6 +22,7 @@ import java.lang.annotation.RetentionPolicy;
 import java.lang.reflect.Method;
 import java.lang.reflect.UndeclaredThrowableException;
 import java.rmi.RemoteException;
+import java.util.ArrayList;
 import java.util.LinkedList;
 import java.util.List;
 
@@ -63,14 +64,16 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException
 import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 
 /**
- * Abstract tests for AspectJAdvisorFactory.
+ * Abstract tests for {@link AspectJAdvisorFactory} implementations.
- * See subclasses for tests of concrete factories.
+ *
+ * <p>See subclasses for tests of concrete factories.
  *
  * @author Rod Johnson
  * @author Chris Beams
  * @author Phillip Webb
+ * @author Sam Brannen
  */
-public abstract class AbstractAspectJAdvisorFactoryTests {
+abstract class AbstractAspectJAdvisorFactoryTests {
 
 	/**
 	 * To be overridden by concrete test subclasses.
@@ -80,7 +83,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 
 	@Test
-	public void testRejectsPerCflowAspect() {
+	void rejectsPerCflowAspect() {
 		assertThatExceptionOfType(AopConfigException.class).isThrownBy(() ->
 				getFixture().getAdvisors(
 						new SingletonMetadataAwareAspectInstanceFactory(new PerCflowAspect(), "someBean")))
@@ -88,7 +91,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testRejectsPerCflowBelowAspect() {
+	void rejectsPerCflowBelowAspect() {
 		assertThatExceptionOfType(AopConfigException.class).isThrownBy(() ->
 					getFixture().getAdvisors(
 							new SingletonMetadataAwareAspectInstanceFactory(new PerCflowBelowAspect(), "someBean")))
@@ -96,7 +99,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testPerTargetAspect() throws SecurityException, NoSuchMethodException {
+	void perTargetAspect() throws SecurityException, NoSuchMethodException {
 		TestBean target = new TestBean();
 		int realAge = 65;
 		target.setAge(realAge);
@@ -128,7 +131,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testMultiplePerTargetAspects() throws SecurityException, NoSuchMethodException {
+	void multiplePerTargetAspects() throws SecurityException, NoSuchMethodException {
 		TestBean target = new TestBean();
 		int realAge = 65;
 		target.setAge(realAge);
@@ -156,7 +159,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testMultiplePerTargetAspectsWithOrderAnnotation() throws SecurityException, NoSuchMethodException {
+	void multiplePerTargetAspectsWithOrderAnnotation() throws SecurityException, NoSuchMethodException {
 		TestBean target = new TestBean();
 		int realAge = 65;
 		target.setAge(realAge);
@@ -182,7 +185,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testPerThisAspect() throws SecurityException, NoSuchMethodException {
+	void perThisAspect() throws SecurityException, NoSuchMethodException {
 		TestBean target = new TestBean();
 		int realAge = 65;
 		target.setAge(realAge);
@@ -218,7 +221,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testPerTypeWithinAspect() throws SecurityException, NoSuchMethodException {
+	void perTypeWithinAspect() throws SecurityException, NoSuchMethodException {
 		TestBean target = new TestBean();
 		int realAge = 65;
 		target.setAge(realAge);
@@ -259,22 +262,22 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testNamedPointcutAspectWithFQN() {
+	void namedPointcutAspectWithFQN() {
-		testNamedPointcuts(new NamedPointcutAspectWithFQN());
+		namedPointcuts(new NamedPointcutAspectWithFQN());
 	}
 
 	@Test
-	public void testNamedPointcutAspectWithoutFQN() {
+	void namedPointcutAspectWithoutFQN() {
-		testNamedPointcuts(new NamedPointcutAspectWithoutFQN());
+		namedPointcuts(new NamedPointcutAspectWithoutFQN());
 	}
 
 	@Test
-	public void testNamedPointcutFromAspectLibrary() {
+	void namedPointcutFromAspectLibrary() {
-		testNamedPointcuts(new NamedPointcutAspectFromLibrary());
+		namedPointcuts(new NamedPointcutAspectFromLibrary());
 	}
 
 	@Test
-	public void testNamedPointcutFromAspectLibraryWithBinding() {
+	void namedPointcutFromAspectLibraryWithBinding() {
 		TestBean target = new TestBean();
 		ITestBean itb = (ITestBean) createProxy(target,
 				getFixture().getAdvisors(new SingletonMetadataAwareAspectInstanceFactory(
@@ -285,7 +288,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 		assertThat(target.getAge()).isEqualTo(20);
 	}
 
-	private void testNamedPointcuts(Object aspectInstance) {
+	private void namedPointcuts(Object aspectInstance) {
 		TestBean target = new TestBean();
 		int realAge = 65;
 		target.setAge(realAge);
@@ -297,7 +300,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testBindingWithSingleArg() {
+	void bindingWithSingleArg() {
 		TestBean target = new TestBean();
 		ITestBean itb = (ITestBean) createProxy(target,
 				getFixture().getAdvisors(
@@ -309,7 +312,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testBindingWithMultipleArgsDifferentlyOrdered() {
+	void bindingWithMultipleArgsDifferentlyOrdered() {
 		ManyValuedArgs target = new ManyValuedArgs();
 		ManyValuedArgs mva = (ManyValuedArgs) createProxy(target,
 				getFixture().getAdvisors(
@@ -329,7 +332,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	 * In this case the introduction will be made.
 	 */
 	@Test
-	public void testIntroductionOnTargetNotImplementingInterface() {
+	void introductionOnTargetNotImplementingInterface() {
 		NotLockable notLockableTarget = new NotLockable();
 		assertThat(notLockableTarget instanceof Lockable).isFalse();
 		NotLockable notLockable1 = (NotLockable) createProxy(notLockableTarget,
@@ -358,7 +361,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testIntroductionAdvisorExcludedFromTargetImplementingInterface() {
+	void introductionAdvisorExcludedFromTargetImplementingInterface() {
 		assertThat(AopUtils.findAdvisorsThatCanApply(
 		getFixture().getAdvisors(
 				new SingletonMetadataAwareAspectInstanceFactory(new MakeLockable(), "someBean")),
@@ -368,7 +371,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testIntroductionOnTargetImplementingInterface() {
+	void introductionOnTargetImplementingInterface() {
 		CannotBeUnlocked target = new CannotBeUnlocked();
 		Lockable proxy = (Lockable) createProxy(target,
 				// Ensure that we exclude
@@ -388,7 +391,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testIntroductionOnTargetExcludedByTypePattern() {
+	void introductionOnTargetExcludedByTypePattern() {
 		LinkedList<Object> target = new LinkedList<>();
 		List<?> proxy = (List<?>) createProxy(target,
 				AopUtils.findAdvisorsThatCanApply(
@@ -400,7 +403,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testIntroductionBasedOnAnnotationMatch_SPR5307() {
+	void introductionBasedOnAnnotationMatch_SPR5307() {
 		AnnotatedTarget target = new AnnotatedTargetImpl();
 		List<Advisor> advisors = getFixture().getAdvisors(
 				new SingletonMetadataAwareAspectInstanceFactory(new MakeAnnotatedTypeModifiable(), "someBean"));
@@ -414,7 +417,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	// TODO: Why does this test fail? It hasn't been run before, so it maybe never actually passed...
 	@Test
 	@Disabled
-	public void testIntroductionWithArgumentBinding() {
+	void introductionWithArgumentBinding() {
 		TestBean target = new TestBean();
 
 		List<Advisor> advisors = getFixture().getAdvisors(
@@ -448,7 +451,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testAspectMethodThrowsExceptionLegalOnSignature() {
+	void aspectMethodThrowsExceptionLegalOnSignature() {
 		TestBean target = new TestBean();
 		UnsupportedOperationException expectedException = new UnsupportedOperationException();
 		List<Advisor> advisors = getFixture().getAdvisors(
@@ -462,7 +465,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	// TODO document this behaviour.
 	// Is it different AspectJ behaviour, at least for checked exceptions?
 	@Test
-	public void testAspectMethodThrowsExceptionIllegalOnSignature() {
+	void aspectMethodThrowsExceptionIllegalOnSignature() {
 		TestBean target = new TestBean();
 		RemoteException expectedException = new RemoteException();
 		List<Advisor> advisors = getFixture().getAdvisors(
@@ -491,7 +494,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testTwoAdvicesOnOneAspect() {
+	void twoAdvicesOnOneAspect() {
 		TestBean target = new TestBean();
 		TwoAdviceAspect twoAdviceAspect = new TwoAdviceAspect();
 		List<Advisor> advisors = getFixture().getAdvisors(
@@ -506,25 +509,23 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testAfterAdviceTypes() throws Exception {
+	void afterAdviceTypes() throws Exception {
-		Echo target = new Echo();
+		ExceptionHandling aspect = new ExceptionHandling();
-		ExceptionHandling afterReturningAspect = new ExceptionHandling();
 		List<Advisor> advisors = getFixture().getAdvisors(
-				new SingletonMetadataAwareAspectInstanceFactory(afterReturningAspect, "someBean"));
+				new SingletonMetadataAwareAspectInstanceFactory(aspect, "exceptionHandlingAspect"));
-		Echo echo = (Echo) createProxy(target, advisors, Echo.class);
+		Echo echo = (Echo) createProxy(new Echo(), advisors, Echo.class);
-		assertThat(afterReturningAspect.successCount).isEqualTo(0);
+
-		assertThat(echo.echo("")).isEqualTo("");
+		assertThat(aspect.invocations).isEmpty();
-		assertThat(afterReturningAspect.successCount).isEqualTo(1);
+		assertThat(echo.echo(42)).isEqualTo(42);
-		assertThat(afterReturningAspect.failureCount).isEqualTo(0);
+		assertThat(aspect.invocations).containsExactly("after returning", "after");
-		assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() ->
+
-			echo.echo(new FileNotFoundException()));
+		aspect.invocations.clear();
-		assertThat(afterReturningAspect.successCount).isEqualTo(1);
+		assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> echo.echo(new FileNotFoundException()));
-		assertThat(afterReturningAspect.failureCount).isEqualTo(1);
+		assertThat(aspect.invocations).containsExactly("after throwing", "after");
-		assertThat(afterReturningAspect.afterCount).isEqualTo(afterReturningAspect.failureCount + afterReturningAspect.successCount);
 	}
 
 	@Test
-	public void testFailureWithoutExplicitDeclarePrecedence() {
+	void failureWithoutExplicitDeclarePrecedence() {
 		TestBean target = new TestBean();
 		MetadataAwareAspectInstanceFactory aspectInstanceFactory = new SingletonMetadataAwareAspectInstanceFactory(
 				new NoDeclarePrecedenceShouldFail(), "someBean");
@@ -534,7 +535,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 	}
 
 	@Test
-	public void testDeclarePrecedenceNotSupported() {
+	void declarePrecedenceNotSupported() {
 		TestBean target = new TestBean();
 		assertThatIllegalArgumentException().isThrownBy(() -> {
 				MetadataAwareAspectInstanceFactory aspectInstanceFactory = new SingletonMetadataAwareAspectInstanceFactory(
@@ -545,28 +546,28 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 
 	@Aspect("percflow(execution(* *(..)))")
-	public static class PerCflowAspect {
+	static class PerCflowAspect {
 	}
 
 
 	@Aspect("percflowbelow(execution(* *(..)))")
-	public static class PerCflowBelowAspect {
+	static class PerCflowBelowAspect {
 	}
 
 
 	@Aspect("pertarget(execution(* *.getSpouse()))")
 	@Order(10)
-	public static class PerTargetAspectWithOrderAnnotation10 {
+	static class PerTargetAspectWithOrderAnnotation10 {
 
-		public int count;
+		int count;
 
 		@Around("execution(int *.getAge())")
-		public int returnCountAsAge() {
+		int returnCountAsAge() {
 			return count++;
 		}
 
 		@Before("execution(void *.set*(int))")
-		public void countSetter() {
+		void countSetter() {
 			++count;
 		}
 	}
@@ -574,34 +575,34 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 	@Aspect("pertarget(execution(* *.getSpouse()))")
 	@Order(5)
-	public static class PerTargetAspectWithOrderAnnotation5 {
+	static class PerTargetAspectWithOrderAnnotation5 {
 
-		public int count;
+		int count;
 
 		@Around("execution(int *.getAge())")
-		public int returnCountAsAge() {
+		int returnCountAsAge() {
 			return count++;
 		}
 
 		@Before("execution(void *.set*(int))")
-		public void countSetter() {
+		void countSetter() {
 			++count;
 		}
 	}
 
 
 	@Aspect("pertypewithin(org.springframework.beans.testfixture.beans.IOther+)")
-	public static class PerTypeWithinAspect {
+	static class PerTypeWithinAspect {
 
-		public int count;
+		int count;
 
 		@Around("execution(int *.getAge())")
-		public int returnCountAsAge() {
+		int returnCountAsAge() {
 			return count++;
 		}
 
 		@Before("execution(void *.*(..))")
-		public void countAnythingVoid() {
+		void countAnythingVoid() {
 			++count;
 		}
 	}
@@ -611,7 +612,7 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 		private int count;
 
-		public int getInstantiationCount() {
+		int getInstantiationCount() {
 			return this.count;
 		}
 
@@ -644,97 +645,96 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 
 	@Aspect
-	public static class NamedPointcutAspectWithFQN {
+	static class NamedPointcutAspectWithFQN {
 
 		@SuppressWarnings("unused")
 		private ITestBean fieldThatShouldBeIgnoredBySpringAtAspectJProcessing = new TestBean();
 
 		@Pointcut("execution(* getAge())")
-		public void getAge() {
+		void getAge() {
 		}
 
 		@Around("org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.NamedPointcutAspectWithFQN.getAge()")
-		public int changeReturnValue(ProceedingJoinPoint pjp) {
+		int changeReturnValue(ProceedingJoinPoint pjp) {
 			return -1;
 		}
 	}
 
 
 	@Aspect
-	public static class NamedPointcutAspectWithoutFQN {
+	static class NamedPointcutAspectWithoutFQN {
 
 		@Pointcut("execution(* getAge())")
-		public void getAge() {
+		void getAge() {
 		}
 
 		@Around("getAge()")
-		public int changeReturnValue(ProceedingJoinPoint pjp) {
+		int changeReturnValue(ProceedingJoinPoint pjp) {
 			return -1;
 		}
 	}
 
 
 	@Aspect
-	public static class NamedPointcutAspectFromLibrary {
+	static class NamedPointcutAspectFromLibrary {
 
 		@Around("org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.Library.propertyAccess()")
-		public int changeReturnType(ProceedingJoinPoint pjp) {
+		int changeReturnType(ProceedingJoinPoint pjp) {
 			return -1;
 		}
 
 		@Around(value="org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.Library.integerArgOperation(x)", argNames="x")
-		public void doubleArg(ProceedingJoinPoint pjp, int x) throws Throwable {
+		void doubleArg(ProceedingJoinPoint pjp, int x) throws Throwable {
 			pjp.proceed(new Object[] {x*2});
 		}
 	}
 
 
 	@Aspect
-	public static class Library {
+	static class Library {
 
 		@Pointcut("execution(!void get*())")
-		public void propertyAccess() {}
+		void propertyAccess() {}
 
 		@Pointcut("execution(* *(..)) && args(i)")
-		public void integerArgOperation(int i) {}
+		void integerArgOperation(int i) {}
-
 	}
 
 
 	@Aspect
-	public static class NamedPointcutAspectFromLibraryWithBinding {
+	static class NamedPointcutAspectFromLibraryWithBinding {
 
 		@Around(value="org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.Library.integerArgOperation(x)", argNames="x")
-		public void doubleArg(ProceedingJoinPoint pjp, int x) throws Throwable {
+		void doubleArg(ProceedingJoinPoint pjp, int x) throws Throwable {
 			pjp.proceed(new Object[] {x*2});
 		}
 	}
 
 
 	@Aspect
-	public static class BindingAspectWithSingleArg {
+	static class BindingAspectWithSingleArg {
 
 		@Pointcut(value="args(a)", argNames="a")
-		public void setAge(int a) {}
+		void setAge(int a) {}
 
 		@Around(value="setAge(age)",argNames="age")
 		// @ArgNames({"age"})	// AMC needs more work here? ignoring pjp arg... ok??
 		// argNames should be supported in Around as it is in Pointcut
-		public void changeReturnType(ProceedingJoinPoint pjp, int age) throws Throwable {
+		void changeReturnType(ProceedingJoinPoint pjp, int age) throws Throwable {
 			pjp.proceed(new Object[] {age*2});
 		}
 	}
 
 
 	@Aspect
-	public static class ManyValuedArgs {
+	static class ManyValuedArgs {
 
-		public String mungeArgs(String a, int b, int c, String d, StringBuffer e) {
+		String mungeArgs(String a, int b, int c, String d, StringBuffer e) {
 			return a + b + c + d + e;
 		}
 
 		@Around(value="execution(String mungeArgs(..)) && args(a, b, c, d, e)", argNames="b,c,d,e,a")
-		public String reverseAdvice(ProceedingJoinPoint pjp, int b, int c, String d, StringBuffer e, String a) throws Throwable {
+		String reverseAdvice(ProceedingJoinPoint pjp, int b, int c, String d, StringBuffer e, String a) throws Throwable {
 			assertThat(pjp.proceed()).isEqualTo(a + b+ c+ d+ e);
 			return a + b + c + d + e;
 		}
@@ -742,24 +742,24 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 
 	@Aspect
-	public static class ExceptionAspect {
+	static class ExceptionAspect {
 
 		private final Exception ex;
 
-		public ExceptionAspect(Exception ex) {
+		ExceptionAspect(Exception ex) {
 			this.ex = ex;
 		}
 
 		@Before("execution(* getAge())")
-		public void throwException() throws Exception {
+		void throwException() throws Exception {
 			throw ex;
 		}
 	}
 
 
-	public static class Echo {
+	static class Echo {
 
-		public Object echo(Object o) throws Exception {
+		Object echo(Object o) throws Exception {
 			if (o instanceof Exception) {
 				throw (Exception) o;
 			}
@@ -769,45 +769,46 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 
 	@Aspect
-	public static class ExceptionHandling {
+	static class ExceptionHandling {
 
-		public int successCount;
+		List<String> invocations = new ArrayList<>();
 
-		public int failureCount;
 
-		public int afterCount;
+		@Pointcut("execution(* echo(*))")
+		void echo() {
+		}
 
-		@AfterReturning("execution(* echo(*))")
+		@AfterReturning("echo()")
-		public void succeeded() {
+		void succeeded() {
-			++successCount;
+			invocations.add("after returning");
 		}
 
-		@AfterThrowing("execution(* echo(*))")
+		@AfterThrowing("echo()")
-		public void failed() {
+		void failed() {
-			++failureCount;
+			invocations.add("after throwing");
 		}
 
-		@After("execution(* echo(*))")
+		@After("echo()")
-		public void invoked() {
+		void after() {
-			++afterCount;
+			invocations.add("after");
 		}
 	}
 
 
 	@Aspect
-	public static class NoDeclarePrecedenceShouldFail {
+	static class NoDeclarePrecedenceShouldFail {
 
 		@Pointcut("execution(int *.getAge())")
-		public void getAge() {
+		void getAge() {
 		}
 
 		@Before("getAge()")
-		public void blowUpButDoesntMatterBecauseAroundAdviceWontLetThisBeInvoked() {
+		void blowUpButDoesntMatterBecauseAroundAdviceWontLetThisBeInvoked() {
 			throw new IllegalStateException();
 		}
 
 		@Around("getAge()")
-		public int preventExecution(ProceedingJoinPoint pjp) {
+		int preventExecution(ProceedingJoinPoint pjp) {
 			return 666;
 		}
 	}
@@ -815,19 +816,19 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 
 	@Aspect
 	@DeclarePrecedence("test..*")
-	public static class DeclarePrecedenceShouldSucceed {
+	static class DeclarePrecedenceShouldSucceed {
 
 		@Pointcut("execution(int *.getAge())")
-		public void getAge() {
+		void getAge() {
 		}
 
 		@Before("getAge()")
-		public void blowUpButDoesntMatterBecauseAroundAdviceWontLetThisBeInvoked() {
+		void blowUpButDoesntMatterBecauseAroundAdviceWontLetThisBeInvoked() {
 			throw new IllegalStateException();
 		}
 
 		@Around("getAge()")
-		public int preventExecution(ProceedingJoinPoint pjp) {
+		int preventExecution(ProceedingJoinPoint pjp) {
 			return 666;
 		}
 	}
@@ -845,12 +846,12 @@ public abstract class AbstractAspectJAdvisorFactoryTests {
 @Aspect
 abstract class AbstractMakeModifiable {
 
-	public interface MutableModifiable extends Modifiable {
+	interface MutableModifiable extends Modifiable {
 
 		void markDirty();
 	}
 
-	public static class ModifiableImpl implements MutableModifiable {
+	static class ModifiableImpl implements MutableModifiable {
 
 		private boolean modified;
 
@@ -871,7 +872,7 @@ abstract class AbstractMakeModifiable {
 	}
 
 	@Before(value="execution(void set*(*)) && this(modifiable) && args(newValue)", argNames="modifiable,newValue")
-	public void recordModificationIfSetterArgumentDiffersFromOldValue(
+	void recordModificationIfSetterArgumentDiffersFromOldValue(
 			JoinPoint jp, MutableModifiable mixin, Object newValue) {
 
 		/*
@@ -933,7 +934,7 @@ class MakeITestBeanModifiable extends AbstractMakeModifiable {
 
 	@DeclareParents(value = "org.springframework.beans.testfixture.beans.ITestBean+",
 			defaultImpl=ModifiableImpl.class)
-	public static MutableModifiable mixin;
+	static MutableModifiable mixin;
 
 }
 
@@ -948,7 +949,7 @@ class MakeAnnotatedTypeModifiable extends AbstractMakeModifiable {
 
 	@DeclareParents(value = "(@org.springframework.aop.aspectj.annotation.Measured *)",
 			defaultImpl = DefaultLockable.class)
-	public static Lockable mixin;
+	static Lockable mixin;
 
 }
 
@@ -960,10 +961,10 @@ class MakeAnnotatedTypeModifiable extends AbstractMakeModifiable {
 class MakeLockable {
 
 	@DeclareParents(value = "org.springframework..*", defaultImpl = DefaultLockable.class)
-	public static Lockable mixin;
+	static Lockable mixin;
 
 	@Before(value="execution(void set*(*)) && this(mixin)", argNames="mixin")
-	public void checkNotLocked( Lockable mixin) {
+	void checkNotLocked( Lockable mixin) {
 		// Can also obtain the mixin (this) this way
 		//Lockable mixin = (Lockable) jp.getThis();
 		if (mixin.locked()) {
@@ -1032,11 +1033,11 @@ class NotLockable {
 
 	private int intValue;
 
-	public int getIntValue() {
+	int getIntValue() {
 		return intValue;
 	}
 
-	public void setIntValue(int intValue) {
+	void setIntValue(int intValue) {
 		this.intValue = intValue;
 	}
 
@@ -1046,19 +1047,19 @@ class NotLockable {
 @Aspect("perthis(execution(* *.getSpouse()))")
 class PerThisAspect {
 
-	public int count;
+	int count;
 
 	// Just to check that this doesn't cause problems with introduction processing
 	@SuppressWarnings("unused")
 	private ITestBean fieldThatShouldBeIgnoredBySpringAtAspectJProcessing = new TestBean();
 
 	@Around("execution(int *.getAge())")
-	public int returnCountAsAge() {
+	int returnCountAsAge() {
 		return count++;
 	}
 
 	@Before("execution(void *.set*(int))")
-	public void countSetter() {
+	void countSetter() {
 		++count;
 	}
 

spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactoryTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2020 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.
@@ -17,13 +17,14 @@
 package org.springframework.aop.aspectj.annotation;
 
 /**
- * Tests for ReflectiveAtAspectJAdvisorFactory.
+ * Tests for {@link ReflectiveAspectJAdvisorFactory}.
- * Tests are inherited: we only set the test fixture here.
+ *
+ * <p>Tests are inherited: we only set the test fixture here.
  *
  * @author Rod Johnson
  * @since 2.0
  */
-public class ReflectiveAspectJAdvisorFactoryTests extends AbstractAspectJAdvisorFactoryTests {
+class ReflectiveAspectJAdvisorFactoryTests extends AbstractAspectJAdvisorFactoryTests {
 
 	@Override
 	protected AspectJAdvisorFactory getFixture() {