From fbeecf3bf8f7facd14d13a0ddae9ca00d5c192ad Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 5 Jun 2020 15:22:27 +0200 Subject: [PATCH] Test status quo for invocation order of all advice types Prior to this commit we did not have tests in place to verify the status quo for the invocation order of all advice types when declared within a single aspect, either via the XML namespace or AspectJ auto-proxy support. This commit introduces such tests that demonstrate where such ordering is broken or suboptimal. The only test for which the advice invocation order is correct or at least as expected is the afterAdviceTypes() test method in ReflectiveAspectJAdvisorFactoryTests, where an AOP proxy is hand crafted using ReflectiveAspectJAdvisorFactory without the use of Spring's AspectJPrecedenceComparator. See gh-25186 --- ...aceHandlerAdviceOrderIntegrationTests.java | 32 ++++++--- ...JAutoProxyAdviceOrderIntegrationTests.java | 69 ++++++++++++++++--- ...AdviceOrderIntegrationTests-afterFirst.xml | 10 +-- ...rAdviceOrderIntegrationTests-afterLast.xml | 10 +-- .../AbstractAspectJAdvisorFactoryTests.java | 36 +++++++--- .../AspectJPointcutAdvisorTests.java | 5 +- .../annotation/AspectMetadataTests.java | 6 +- 7 files changed, 124 insertions(+), 44 deletions(-) diff --git a/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java b/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java index d4a659f824..da32ff1201 100644 --- a/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java +++ b/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java @@ -19,6 +19,7 @@ package org.springframework.aop.config; import java.util.ArrayList; import java.util.List; +import org.aspectj.lang.ProceedingJoinPoint; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -45,14 +46,14 @@ class AopNamespaceHandlerAdviceOrderIntegrationTests { class AfterAdviceFirstTests { @Test - void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired EchoAspect aspect) throws Exception { + void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired InvocationTrackingAspect aspect) throws Exception { assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - assertThat(aspect.invocations).containsExactly("after", "after returning"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after returning"); aspect.invocations.clear(); assertThatExceptionOfType(Exception.class).isThrownBy(() -> echo.echo(new Exception())); - assertThat(aspect.invocations).containsExactly("after", "after throwing"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing"); } } @@ -62,14 +63,14 @@ class AopNamespaceHandlerAdviceOrderIntegrationTests { class AfterAdviceLastTests { @Test - void afterAdviceIsInvokedLast(@Autowired Echo echo, @Autowired EchoAspect aspect) throws Exception { + void afterAdviceIsInvokedLast(@Autowired Echo echo, @Autowired InvocationTrackingAspect aspect) throws Exception { assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - assertThat(aspect.invocations).containsExactly("after returning", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after returning", "after"); aspect.invocations.clear(); assertThatExceptionOfType(Exception.class).isThrownBy(() -> echo.echo(new Exception())); - assertThat(aspect.invocations).containsExactly("after throwing", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after throwing", "after"); } } @@ -84,18 +85,29 @@ class AopNamespaceHandlerAdviceOrderIntegrationTests { } } - static class EchoAspect { + static class InvocationTrackingAspect { List invocations = new ArrayList<>(); - void echo() { + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } } - void succeeded() { + void before() { + invocations.add("before"); + } + + void afterReturning() { invocations.add("after returning"); } - void failed() { + void afterThrowing() { invocations.add("after throwing"); } 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 f088cc1024..6e67a881fa 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 @@ -19,10 +19,13 @@ package org.springframework.aop.framework.autoproxy; import java.util.ArrayList; import java.util.List; +import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.After; import org.aspectj.lang.annotation.AfterReturning; import org.aspectj.lang.annotation.AfterThrowing; +import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; +import org.aspectj.lang.annotation.Before; import org.aspectj.lang.annotation.Pointcut; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -47,25 +50,39 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; */ class AspectJAutoProxyAdviceOrderIntegrationTests { + /** + * {@link After @After} advice declared as first after method in source code. + */ @Nested @SpringJUnitConfig(AfterAdviceFirstConfig.class) @DirtiesContext class AfterAdviceFirstTests { @Test - void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired AfterAdviceFirstAspect aspect) throws Exception { + void afterAdviceIsNotInvokedLast(@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"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after returning"); aspect.invocations.clear(); assertThatExceptionOfType(Exception.class).isThrownBy( () -> echo.echo(new Exception())); - assertThat(aspect.invocations).containsExactly("after", "after throwing"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing"); } } + /** + * This test class uses {@link AfterAdviceLastAspect} which declares its + * {@link After @After} advice as the last after advice type method + * 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 + * {@code @AfterReturning} advice methods due to the AspectJ precedence + * rules implemented in + * {@link org.springframework.aop.aspectj.autoproxy.AspectJPrecedenceComparator}. + */ @Nested @SpringJUnitConfig(AfterAdviceLastConfig.class) @DirtiesContext @@ -75,14 +92,12 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { 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"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "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"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing"); } } @@ -145,14 +160,30 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { } @AfterReturning("echo()") - void succeeded() { + void afterReturning() { invocations.add("after returning"); } @AfterThrowing("echo()") - void failed() { + void afterThrowing() { invocations.add("after throwing"); } + + @Before("echo()") + void before() { + invocations.add("before"); + } + + @Around("echo()") + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } + } } /** @@ -167,13 +198,29 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { void echo() { } + @Around("echo()") + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } + } + + @Before("echo()") + void before() { + invocations.add("before"); + } + @AfterReturning("echo()") - void succeeded() { + void afterReturning() { invocations.add("after returning"); } @AfterThrowing("echo()") - void failed() { + void afterThrowing() { invocations.add("after throwing"); } diff --git a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml index 6293b21982..fd98a0c1b4 100644 --- a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml +++ b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml @@ -7,14 +7,16 @@ - + - + + + - - + + diff --git a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml index 376896533a..bcab0df678 100644 --- a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml +++ b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml @@ -7,13 +7,15 @@ - + - + - - + + + + diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java index 67a7f0aa78..c7bad57501 100644 --- a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java @@ -455,7 +455,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { TestBean target = new TestBean(); UnsupportedOperationException expectedException = new UnsupportedOperationException(); List advisors = getFixture().getAdvisors( - new SingletonMetadataAwareAspectInstanceFactory(new ExceptionAspect(expectedException), "someBean")); + new SingletonMetadataAwareAspectInstanceFactory(new ExceptionThrowingAspect(expectedException), "someBean")); assertThat(advisors.size()).as("One advice method was found").isEqualTo(1); ITestBean itb = (ITestBean) createProxy(target, advisors, ITestBean.class); assertThatExceptionOfType(UnsupportedOperationException.class).isThrownBy( @@ -469,7 +469,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { TestBean target = new TestBean(); RemoteException expectedException = new RemoteException(); List advisors = getFixture().getAdvisors( - new SingletonMetadataAwareAspectInstanceFactory(new ExceptionAspect(expectedException), "someBean")); + new SingletonMetadataAwareAspectInstanceFactory(new ExceptionThrowingAspect(expectedException), "someBean")); assertThat(advisors.size()).as("One advice method was found").isEqualTo(1); ITestBean itb = (ITestBean) createProxy(target, advisors, ITestBean.class); assertThatExceptionOfType(UndeclaredThrowableException.class).isThrownBy( @@ -510,18 +510,18 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Test void afterAdviceTypes() throws Exception { - ExceptionHandling aspect = new ExceptionHandling(); + InvocationTrackingAspect aspect = new InvocationTrackingAspect(); List advisors = getFixture().getAdvisors( new SingletonMetadataAwareAspectInstanceFactory(aspect, "exceptionHandlingAspect")); Echo echo = (Echo) createProxy(new Echo(), advisors, Echo.class); assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - assertThat(aspect.invocations).containsExactly("after returning", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "after returning", "after", "around - end"); aspect.invocations.clear(); assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> echo.echo(new FileNotFoundException())); - assertThat(aspect.invocations).containsExactly("after throwing", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "after throwing", "after", "around - end"); } @Test @@ -742,11 +742,11 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Aspect - static class ExceptionAspect { + static class ExceptionThrowingAspect { private final Exception ex; - ExceptionAspect(Exception ex) { + ExceptionThrowingAspect(Exception ex) { this.ex = ex; } @@ -769,7 +769,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Aspect - static class ExceptionHandling { + private static class InvocationTrackingAspect { List invocations = new ArrayList<>(); @@ -778,13 +778,29 @@ abstract class AbstractAspectJAdvisorFactoryTests { void echo() { } + @Around("echo()") + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } + } + + @Before("echo()") + void before() { + invocations.add("before"); + } + @AfterReturning("echo()") - void succeeded() { + void afterReturning() { invocations.add("after returning"); } @AfterThrowing("echo()") - void failed() { + void afterThrowing() { invocations.add("after throwing"); } diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java index b944da483e..806f30587b 100644 --- a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.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 test.aop.PerTargetAspect; import org.springframework.aop.Pointcut; import org.springframework.aop.aspectj.AspectJExpressionPointcut; import org.springframework.aop.aspectj.AspectJExpressionPointcutTests; +import org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.ExceptionThrowingAspect; import org.springframework.aop.framework.AopConfigException; import org.springframework.beans.testfixture.beans.TestBean; @@ -44,7 +45,7 @@ public class AspectJPointcutAdvisorTests { InstantiationModelAwarePointcutAdvisorImpl ajpa = new InstantiationModelAwarePointcutAdvisorImpl( ajexp, TestBean.class.getMethod("getAge"), af, - new SingletonMetadataAwareAspectInstanceFactory(new AbstractAspectJAdvisorFactoryTests.ExceptionAspect(null), "someBean"), + new SingletonMetadataAwareAspectInstanceFactory(new ExceptionThrowingAspect(null), "someBean"), 1, "someBean"); assertThat(ajpa.getAspectMetadata().getPerClausePointcut()).isSameAs(Pointcut.TRUE); diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.java index 55fa481ece..637baa2450 100644 --- a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.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,7 +22,7 @@ import test.aop.PerTargetAspect; import org.springframework.aop.Pointcut; import org.springframework.aop.aspectj.AspectJExpressionPointcut; -import org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.ExceptionAspect; +import org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.ExceptionThrowingAspect; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; @@ -42,7 +42,7 @@ class AspectMetadataTests { @Test void singletonAspect() { - AspectMetadata am = new AspectMetadata(ExceptionAspect.class, "someBean"); + AspectMetadata am = new AspectMetadata(ExceptionThrowingAspect.class, "someBean"); assertThat(am.isPerThisOrPerTarget()).isFalse(); assertThat(am.getPerClausePointcut()).isSameAs(Pointcut.TRUE); assertThat(am.getAjType().getPerClause().getKind()).isEqualTo(PerClauseKind.SINGLETON);