Implement reliable advice invocation order within an @aspect

The AspectJPrecedenceComparator was designed to mimic the precedence
order enforced by the AspectJ compiler with regard to multiple 'after'
methods defined within the same aspect whose pointcuts match the same
joinpoint. Specifically, if an aspect declares multiple @after,
@AfterReturning, or @AfterThrowing advice methods whose pointcuts match
the same joinpoint, such 'after' advice methods should be invoked in
the reverse order in which they are declared in the source code.

When the AspectJPrecedenceComparator was introduced in Spring Framework
2.0, it achieved its goal of mimicking the AspectJ compiler since the
JDK at that time (i.e., Java 5) ensured that an invocation of
Class#geDeclaredMethods() returned an array of methods that matched the
order of declaration in the source code. However, Java 7 removed this
guarantee. Consequently, in Java 7 or higher,
AspectJPrecedenceComparator no longer works as it is documented or as
it was designed when sorting advice methods in a single @aspect class.
Note, however, that AspectJPrecedenceComparator continues to work as
documented and designed when sorting advice configured via the
<aop:aspect> XML namespace element.

PR gh-24673 highlights a use case where AspectJPrecedenceComparator
fails to assign the highest precedence to an @after advice method
declared last in the source code. Note that an @after advice method
with a precedence higher than @AfterReturning and @AfterThrowing advice
methods in the same aspect will effectively be invoked last due to the
try-finally implementation in AspectJAfterAdvice.invoke() which invokes
proceed() in the try-block and invokeAdviceMethod() in the
finally-block.

Since Spring cannot reliably determine the source code declaration
order of annotated advice methods without using ASM to analyze the byte
code, this commit introduces reliable invocation order for advice
methods declared within a single @aspect. Specifically, the
getAdvisors(...) method in ReflectiveAspectJAdvisorFactory now hard
codes the declarationOrderInAspect to `0` instead of using the index of
the current advice method. This is necessary since the index no longer
has any correlation to the method declaration order in the source code.
The result is that all advice methods discovered via reflection will
now be sorted only according to the precedence rules defined in the
ReflectiveAspectJAdvisorFactory.METHOD_COMPARATOR. Specifically, advice
methods within a single @aspect will be sorted in the following order
(with @after advice methods effectively invoked after @AfterReturning
and @AfterThrowing advice methods): @around, @before, @after,
@AfterReturning, @AfterThrowing.

The modified assertions in AspectJAutoProxyAdviceOrderIntegrationTests
demonstrate the concrete effects of this change.

Closes gh-25186

Author: sbrannen

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

@@ -59,15 +59,15 @@ class AspectJAutoProxyAdviceOrderIntegrationTests {
 	class AfterAdviceFirstTests {
 
 		@Test
-		void afterAdviceIsNotInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceFirstAspect aspect) throws Exception {
+		void afterAdviceIsInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceFirstAspect aspect) throws Exception {
 			assertThat(aspect.invocations).isEmpty();
 			assertThat(echo.echo(42)).isEqualTo(42);
-			assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after returning");
+			assertThat(aspect.invocations).containsExactly("around - start", "before", "after returning", "after", "around - end");
 
 			aspect.invocations.clear();
 			assertThatExceptionOfType(Exception.class).isThrownBy(
 					() -> echo.echo(new Exception()));
-			assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing");
+			assertThat(aspect.invocations).containsExactly("around - start", "before", "after throwing", "after", "around - end");
 		}
 	}
 
@@ -78,7 +78,7 @@ void afterAdviceIsNotInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceFir
 	 * in its source code.
 	 *
 	 * <p>On Java versions prior to JDK 7, we would have expected the {@code @After}
-	 * advice method to be invoked after {@code @AfterThrowing} and
+	 * advice method to be invoked before {@code @AfterThrowing} and
 	 * {@code @AfterReturning} advice methods due to the AspectJ precedence
 	 * rules implemented in
 	 * {@link org.springframework.aop.aspectj.autoproxy.AspectJPrecedenceComparator}.
@@ -89,15 +89,15 @@ void afterAdviceIsNotInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceFir
 	class AfterAdviceLastTests {
 
 		@Test
-		void afterAdviceIsNotInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceLastAspect aspect) throws Exception {
+		void afterAdviceIsInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceLastAspect aspect) throws Exception {
 			assertThat(aspect.invocations).isEmpty();
 			assertThat(echo.echo(42)).isEqualTo(42);
-			assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after returning");
+			assertThat(aspect.invocations).containsExactly("around - start", "before", "after returning", "after", "around - end");
 
 			aspect.invocations.clear();
 			assertThatExceptionOfType(Exception.class).isThrownBy(
 					() -> echo.echo(new Exception()));
-			assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing");
+			assertThat(aspect.invocations).containsExactly("around - start", "before", "after throwing", "after", "around - end");
 		}
 	}
 

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java

@@ -64,6 +64,7 @@
  * @author Juergen Hoeller
  * @author Ramnivas Laddad
  * @author Phillip Webb
+ * @author Sam Brannen
  * @since 2.0
  */
 @SuppressWarnings("serial")
@@ -72,6 +73,11 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto
 	private static final Comparator<Method> METHOD_COMPARATOR;
 
 	static {
+		// Note: although @After is ordered before @AfterReturning and @AfterThrowing,
+		// an @After advice method will actually be invoked after @AfterReturning and
+		// @AfterThrowing methods due to the fact that AspectJAfterAdvice.invoke(MethodInvocation)
+		// invokes proceed() in a `try` block and only invokes the @After advice method
+		// in a corresponding `finally` block.
 		Comparator<Method> adviceKindComparator = new ConvertingComparator<>(
 				new InstanceComparator<>(
 						Around.class, Before.class, After.class, AfterReturning.class, AfterThrowing.class),
@@ -122,7 +128,15 @@ public List<Advisor> getAdvisors(MetadataAwareAspectInstanceFactory aspectInstan
 
 		List<Advisor> advisors = new ArrayList<>();
 		for (Method method : getAdvisorMethods(aspectClass)) {
-			Advisor advisor = getAdvisor(method, lazySingletonAspectInstanceFactory, advisors.size(), aspectName);
+			// Prior to Spring Framework 5.2.7, advisors.size() was supplied as the declarationOrderInAspect
+			// to getAdvisor(...) to represent the "current position" in the declared methods list.
+			// However, since Java 7 the "current position" is not valid since the JDK no longer
+			// returns declared methods in the order in which they are declared in the source code.
+			// Thus, we now hard code the declarationOrderInAspect to 0 for all advice methods
+			// discovered via reflection in order to support reliable advice ordering across JVM launches.
+			// Specifically, a value of 0 aligns with the default value used in
+			// AspectJPrecedenceComparator.getAspectDeclarationOrder(Advisor).
+			Advisor advisor = getAdvisor(method, lazySingletonAspectInstanceFactory, 0, aspectName);
 			if (advisor != null) {
 				advisors.add(advisor);
 			}