From d6525cfb9775871146f83095b33db27d560e5216 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 8 Jun 2020 14:17:39 +0200 Subject: [PATCH 1/2] Polishing --- .../ReflectiveAspectJAdvisorFactory.java | 4 +- .../AspectJAwareAdvisorAutoProxyCreator.java | 42 ++++++++-------- .../AspectJPrecedenceComparator.java | 48 ++++++++++--------- .../AbstractAdvisorAutoProxyCreator.java | 12 +++-- 4 files changed, 56 insertions(+), 50 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java index db6898f310..da5a9070a2 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java @@ -56,8 +56,8 @@ import org.springframework.util.comparator.InstanceComparator; /** * Factory that can create Spring AOP Advisors given AspectJ classes from - * classes honoring the AspectJ 5 annotation syntax, using reflection to - * invoke the corresponding advice methods. + * classes honoring AspectJ's annotation syntax, using reflection to invoke the + * corresponding advice methods. * * @author Rod Johnson * @author Adrian Colyer diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJAwareAdvisorAutoProxyCreator.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJAwareAdvisorAutoProxyCreator.java index ea21644dc1..2d2aabd07f 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJAwareAdvisorAutoProxyCreator.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJAwareAdvisorAutoProxyCreator.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 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. @@ -50,27 +50,27 @@ public class AspectJAwareAdvisorAutoProxyCreator extends AbstractAdvisorAutoProx /** - * Sort the rest by AspectJ precedence. If two pieces of advice have - * come from the same aspect they will have the same order. - * Advice from the same aspect is then further ordered according to the + * Sort the supplied {@link Advisor} instances according to AspectJ precedence. + *

If two pieces of advice come from the same aspect, they will have the same + * order. Advice from the same aspect is then further ordered according to the * following rules: *

*

Important: Advisors are sorted in precedence order, from highest * precedence to lowest. "On the way in" to a join point, the highest precedence - * advisor should run first. "On the way out" of a join point, the highest precedence - * advisor should run last. + * advisor should run first. "On the way out" of a join point, the highest + * precedence advisor should run last. */ @Override - @SuppressWarnings("unchecked") protected List sortAdvisors(List advisors) { List partiallyComparableAdvisors = new ArrayList<>(advisors.size()); - for (Advisor element : advisors) { + for (Advisor advisor : advisors) { partiallyComparableAdvisors.add( - new PartiallyComparableAdvisorHolder(element, DEFAULT_PRECEDENCE_COMPARATOR)); + new PartiallyComparableAdvisorHolder(advisor, DEFAULT_PRECEDENCE_COMPARATOR)); } List sorted = PartialOrder.sort(partiallyComparableAdvisors); if (sorted != null) { @@ -86,8 +86,8 @@ public class AspectJAwareAdvisorAutoProxyCreator extends AbstractAdvisorAutoProx } /** - * Adds an {@link ExposeInvocationInterceptor} to the beginning of the advice chain. - * These additional advices are needed when using AspectJ expression pointcuts + * Add an {@link ExposeInvocationInterceptor} to the beginning of the advice chain. + *

This additional advice is needed when using AspectJ pointcut expressions * and when using AspectJ-style advice. */ @Override @@ -110,7 +110,7 @@ public class AspectJAwareAdvisorAutoProxyCreator extends AbstractAdvisorAutoProx /** - * Implements AspectJ PartialComparable interface for defining partial orderings. + * Implements AspectJ's {@link PartialComparable} interface for defining partial orderings. */ private static class PartiallyComparableAdvisorHolder implements PartialComparable { @@ -140,17 +140,19 @@ public class AspectJAwareAdvisorAutoProxyCreator extends AbstractAdvisorAutoProx @Override public String toString() { - StringBuilder sb = new StringBuilder(); Advice advice = this.advisor.getAdvice(); - sb.append(ClassUtils.getShortName(advice.getClass())); - sb.append(": "); + StringBuilder sb = new StringBuilder(ClassUtils.getShortName(advice.getClass())); + boolean appended = false; if (this.advisor instanceof Ordered) { - sb.append("order ").append(((Ordered) this.advisor).getOrder()).append(", "); + sb.append(": order = ").append(((Ordered) this.advisor).getOrder()); + appended = true; } if (advice instanceof AbstractAspectJAdvice) { + sb.append(!appended ? ": " : ", "); AbstractAspectJAdvice ajAdvice = (AbstractAspectJAdvice) advice; + sb.append("aspect name = "); sb.append(ajAdvice.getAspectName()); - sb.append(", declaration order "); + sb.append(", declaration order = "); sb.append(ajAdvice.getDeclarationOrder()); } return sb.toString(); diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJPrecedenceComparator.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJPrecedenceComparator.java index d013cda2c9..2d243fadc7 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJPrecedenceComparator.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/autoproxy/AspectJPrecedenceComparator.java @@ -27,20 +27,22 @@ import org.springframework.util.Assert; /** * Orders AspectJ advice/advisors by precedence (not invocation order). * - *

Given two pieces of advice, {@code a} and {@code b}: + *

Given two pieces of advice, {@code A} and {@code B}: *

* - *

Important: Note that unlike a normal comparator a return of 0 means - * we don't care about the ordering, not that the two elements must be sorted - * identically. Used with AspectJ PartialOrder class. + *

Important: This comparator is used with AspectJ's + * {@link org.aspectj.util.PartialOrder PartialOrder} sorting utility. Thus, unlike + * a normal {@link Comparator}, a return value of {@code 0} from this comparator + * means we don't care about the ordering, not that the two elements must be sorted + * identically. * * @author Adrian Colyer * @author Juergen Hoeller @@ -59,16 +61,16 @@ class AspectJPrecedenceComparator implements Comparator { /** - * Create a default AspectJPrecedenceComparator. + * Create a default {@code AspectJPrecedenceComparator}. */ public AspectJPrecedenceComparator() { this.advisorComparator = AnnotationAwareOrderComparator.INSTANCE; } /** - * Create a AspectJPrecedenceComparator, using the given Comparator + * Create an {@code AspectJPrecedenceComparator}, using the given {@link Comparator} * for comparing {@link org.springframework.aop.Advisor} instances. - * @param advisorComparator the Comparator to use for Advisors + * @param advisorComparator the {@code Comparator} to use for advisors */ public AspectJPrecedenceComparator(Comparator advisorComparator) { Assert.notNull(advisorComparator, "Advisor comparator must not be null"); @@ -125,20 +127,20 @@ class AspectJPrecedenceComparator implements Comparator { getAspectName(advisor1).equals(getAspectName(advisor2))); } - private boolean hasAspectName(Advisor anAdvisor) { - return (anAdvisor instanceof AspectJPrecedenceInformation || - anAdvisor.getAdvice() instanceof AspectJPrecedenceInformation); + private boolean hasAspectName(Advisor advisor) { + return (advisor instanceof AspectJPrecedenceInformation || + advisor.getAdvice() instanceof AspectJPrecedenceInformation); } // pre-condition is that hasAspectName returned true - private String getAspectName(Advisor anAdvisor) { - AspectJPrecedenceInformation pi = AspectJAopUtils.getAspectJPrecedenceInformationFor(anAdvisor); - Assert.state(pi != null, "Unresolvable precedence information"); - return pi.getAspectName(); + private String getAspectName(Advisor advisor) { + AspectJPrecedenceInformation precedenceInfo = AspectJAopUtils.getAspectJPrecedenceInformationFor(advisor); + Assert.state(precedenceInfo != null, () -> "Unresolvable AspectJPrecedenceInformation for " + advisor); + return precedenceInfo.getAspectName(); } - private int getAspectDeclarationOrder(Advisor anAdvisor) { - AspectJPrecedenceInformation precedenceInfo = AspectJAopUtils.getAspectJPrecedenceInformationFor(anAdvisor); + private int getAspectDeclarationOrder(Advisor advisor) { + AspectJPrecedenceInformation precedenceInfo = AspectJAopUtils.getAspectJPrecedenceInformationFor(advisor); return (precedenceInfo != null ? precedenceInfo.getDeclarationOrder() : 0); } diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAdvisorAutoProxyCreator.java b/spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAdvisorAutoProxyCreator.java index f169de2a7a..4900c3d465 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAdvisorAutoProxyCreator.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAdvisorAutoProxyCreator.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. @@ -36,11 +36,13 @@ import org.springframework.util.Assert; * also override the inherited {@link #shouldSkip} method to exclude certain * objects from auto-proxying. * - *

Advisors or advices requiring ordering should implement the + *

Advisors or advices requiring ordering should be annotated with + * {@link org.springframework.core.annotation.Order @Order} or implement the * {@link org.springframework.core.Ordered} interface. This class sorts - * Advisors by Ordered order value. Advisors that don't implement the - * Ordered interface will be considered as unordered; they will appear - * at the end of the advisor chain in undefined order. + * advisors using the {@link AnnotationAwareOrderComparator}. Advisors that are + * not annotated with {@code @Order} or don't implement the {@code Ordered} + * interface will be considered as unordered; they will appear at the end of the + * advisor chain in an undefined order. * * @author Rod Johnson * @author Juergen Hoeller From 0998bd49ef1c2d45a1f52202854098577d8f7a66 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 8 Jun 2020 14:18:28 +0200 Subject: [PATCH 2/2] 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 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 --- ...ectJAutoProxyAdviceOrderIntegrationTests.java | 14 +++++++------- .../ReflectiveAspectJAdvisorFactory.java | 16 +++++++++++++++- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java b/integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java index 6e67a881fa..78d45922be 100644 --- a/integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java +++ b/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 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { * in its source code. * *

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 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { 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"); } } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java index da5a9070a2..5355b2bbb3 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java @@ -64,6 +64,7 @@ import org.springframework.util.comparator.InstanceComparator; * @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_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 adviceKindComparator = new ConvertingComparator<>( new InstanceComparator<>( Around.class, Before.class, After.class, AfterReturning.class, AfterThrowing.class), @@ -122,7 +128,15 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto List 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); }