From 9239f9001c99c826f0d2de3c9ac294e9d62bebae Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 4 Jun 2020 14:09:02 +0200 Subject: [PATCH] Test status quo for 'after' advice invocation order See gh-25186 --- ...aceHandlerAdviceOrderIntegrationTests.java | 107 ++++++++++ ...JAutoProxyAdviceOrderIntegrationTests.java | 186 ++++++++++++++++++ ...AdviceOrderIntegrationTests-afterFirst.xml | 21 ++ ...rAdviceOrderIntegrationTests-afterLast.xml | 21 ++ .../AbstractAspectJAdvisorFactoryTests.java | 50 ++--- 5 files changed, 360 insertions(+), 25 deletions(-) create mode 100644 integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java create mode 100644 integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java create mode 100644 integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml create mode 100644 integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml 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 new file mode 100644 index 000000000000..dd19236ee4a5 --- /dev/null +++ b/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 invocations = new ArrayList<>(); + + void echo() { + } + + void succeeded() { + invocations.add("after returning"); + } + + void failed() { + invocations.add("after throwing"); + } + + void after() { + invocations.add("after"); + } + } + +} 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 new file mode 100644 index 000000000000..67563c19505e --- /dev/null +++ b/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 after method in source code. + */ + @Aspect + static class AfterAdviceFirstAspect { + + List 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 after method in source code. + */ + @Aspect + static class AfterAdviceLastAspect { + + List 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"); + } + } + +} 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 new file mode 100644 index 000000000000..6293b2198270 --- /dev/null +++ b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml @@ -0,0 +1,21 @@ + + + + + + + + + + + + + + + + + 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 new file mode 100644 index 000000000000..376896533abd --- /dev/null +++ b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml @@ -0,0 +1,21 @@ + + + + + + + + + + + + + + + + + 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 8b9675c8afe8..67a7f0aa7890 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 @@ -22,6 +22,7 @@ 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; @@ -509,20 +510,18 @@ void twoAdvicesOnOneAspect() { @Test void afterAdviceTypes() throws Exception { - Echo target = new Echo(); - ExceptionHandling afterReturningAspect = new ExceptionHandling(); + ExceptionHandling aspect = new ExceptionHandling(); List advisors = getFixture().getAdvisors( - new SingletonMetadataAwareAspectInstanceFactory(afterReturningAspect, "someBean")); - Echo echo = (Echo) createProxy(target, advisors, Echo.class); - assertThat(afterReturningAspect.successCount).isEqualTo(0); - assertThat(echo.echo("")).isEqualTo(""); - assertThat(afterReturningAspect.successCount).isEqualTo(1); - assertThat(afterReturningAspect.failureCount).isEqualTo(0); - assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> - echo.echo(new FileNotFoundException())); - assertThat(afterReturningAspect.successCount).isEqualTo(1); - assertThat(afterReturningAspect.failureCount).isEqualTo(1); - assertThat(afterReturningAspect.afterCount).isEqualTo(afterReturningAspect.failureCount + afterReturningAspect.successCount); + 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"); + + aspect.invocations.clear(); + assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> echo.echo(new FileNotFoundException())); + assertThat(aspect.invocations).containsExactly("after throwing", "after"); } @Test @@ -772,25 +771,26 @@ Object echo(Object o) throws Exception { @Aspect static class ExceptionHandling { - public int successCount; + List invocations = new ArrayList<>(); - public int failureCount; - public int afterCount; + @Pointcut("execution(* echo(*))") + void echo() { + } - @AfterReturning("execution(* echo(*))") - public void succeeded() { - ++successCount; + @AfterReturning("echo()") + void succeeded() { + invocations.add("after returning"); } - @AfterThrowing("execution(* echo(*))") - public void failed() { - ++failureCount; + @AfterThrowing("echo()") + void failed() { + invocations.add("after throwing"); } - @After("execution(* echo(*))") - public void invoked() { - ++afterCount; + @After("echo()") + void after() { + invocations.add("after"); } }