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 db6898f310..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 @@ -56,14 +56,15 @@ 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 * @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); } 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