Skip to content

Commit

Permalink
Test status quo for invocation order of all advice types
Browse files Browse the repository at this point in the history 
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 <aop:aspect> 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
  • Loading branch information
@sbrannen
sbrannen committed Jun 5, 2020
1 parent 04c8de4 commit fbeecf3
Show file tree
Hide file tree
Showing 7 changed files with 124 additions and 44 deletions.
32 changes: 22 additions & 10 deletions ...t/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java
View file
Expand Up @@ -19,6 +19,7 @@
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;

Expand All @@ -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");
}
}

Expand All @@ -62,14 +63,14 @@ void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired EchoAspect aspec
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");
}
}

Expand All @@ -84,18 +85,29 @@ Object echo(Object obj) throws Exception {
}
}

static class EchoAspect {
static class InvocationTrackingAspect {

List<String> 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 before() {
invocations.add("before");
}

void succeeded() {
void afterReturning() {
invocations.add("after returning");
}

void failed() {
void afterThrowing() {
invocations.add("after throwing");
}

Expand Down
69 changes: 58 additions & 11 deletions .../springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java
View file
Expand Up @@ -19,10 +19,13 @@
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;
Expand All @@ -47,25 +50,39 @@
*/
class AspectJAutoProxyAdviceOrderIntegrationTests {

/**
* {@link After @After} advice declared as first <em>after</em> 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 <em>after advice type</em> method
* 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
* {@code @AfterReturning} advice methods due to the AspectJ precedence
* rules implemented in
* {@link org.springframework.aop.aspectj.autoproxy.AspectJPrecedenceComparator}.
*/
@Nested
@SpringJUnitConfig(AfterAdviceLastConfig.class)
@DirtiesContext
Expand All @@ -75,14 +92,12 @@ class AfterAdviceLastTests {
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");
}
}

Expand Down Expand Up @@ -145,14 +160,30 @@ void after() {
}

@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");
}
}
}

/**
Expand All @@ -167,13 +198,29 @@ static class AfterAdviceLastAspect {
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");
}

Expand Down
10 changes: 6 additions & 4 deletions .../springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml
View file
Expand Up @@ -7,14 +7,16 @@

<bean id="echo" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$Echo"/>

<bean id="echoAspect" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$EchoAspect" />
<bean id="invocationTrackingAspect" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$InvocationTrackingAspect" />

<aop:config>
<aop:aspect id="echoAdvice" ref="echoAspect">
<aop:aspect id="echoAdvice" ref="invocationTrackingAspect">
<aop:pointcut id="echoMethod" expression="execution(* echo(*))" />
<aop:around method="around" pointcut-ref="echoMethod" />
<aop:before method="before" pointcut-ref="echoMethod" />
<aop:after method="after" pointcut-ref="echoMethod" />
<aop:after-throwing method="failed" pointcut-ref="echoMethod" />
<aop:after-returning method="succeeded" pointcut-ref="echoMethod" />
<aop:after-throwing method="afterThrowing" pointcut-ref="echoMethod" />
<aop:after-returning method="afterReturning" pointcut-ref="echoMethod" />
</aop:aspect>
</aop:config>

Expand Down
10 changes: 6 additions & 4 deletions ...g/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml
View file
Expand Up @@ -7,13 +7,15 @@

<bean id="echo" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$Echo"/>

<bean id="echoAspect" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$EchoAspect" />
<bean id="invocationTrackingAspect" class="org.springframework.aop.config.AopNamespaceHandlerAdviceOrderIntegrationTests$InvocationTrackingAspect" />

<aop:config>
<aop:aspect id="echoAdvice" ref="echoAspect">
<aop:aspect id="echoAdvice" ref="invocationTrackingAspect">
<aop:pointcut id="echoMethod" expression="execution(* echo(*))" />
<aop:after-returning method="succeeded" pointcut-ref="echoMethod" />
<aop:after-throwing method="failed" pointcut-ref="echoMethod" />
<aop:around method="around" pointcut-ref="echoMethod" />
<aop:before method="before" pointcut-ref="echoMethod" />
<aop:after-throwing method="afterThrowing" pointcut-ref="echoMethod" />
<aop:after-returning method="afterReturning" pointcut-ref="echoMethod" />
<aop:after method="after" pointcut-ref="echoMethod" />
</aop:aspect>
</aop:config>
Expand Down
36 changes: 26 additions & 10 deletions ...t/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java
View file
Expand Up @@ -455,7 +455,7 @@ void aspectMethodThrowsExceptionLegalOnSignature() {
TestBean target = new TestBean();
UnsupportedOperationException expectedException = new UnsupportedOperationException();
List<Advisor> 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(
Expand All @@ -469,7 +469,7 @@ void aspectMethodThrowsExceptionIllegalOnSignature() {
TestBean target = new TestBean();
RemoteException expectedException = new RemoteException();
List<Advisor> 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(
Expand Down Expand Up @@ -510,18 +510,18 @@ void twoAdvicesOnOneAspect() {

@Test
void afterAdviceTypes() throws Exception {
ExceptionHandling aspect = new ExceptionHandling();
InvocationTrackingAspect aspect = new InvocationTrackingAspect();
List<Advisor> 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
Expand Down Expand Up @@ -742,11 +742,11 @@ String reverseAdvice(ProceedingJoinPoint pjp, int b, int c, String d, StringBuff


@Aspect
static class ExceptionAspect {
static class ExceptionThrowingAspect {

private final Exception ex;

ExceptionAspect(Exception ex) {
ExceptionThrowingAspect(Exception ex) {
this.ex = ex;
}

Expand All @@ -769,7 +769,7 @@ Object echo(Object o) throws Exception {


@Aspect
static class ExceptionHandling {
private static class InvocationTrackingAspect {

List<String> invocations = new ArrayList<>();

Expand All @@ -778,13 +778,29 @@ static class ExceptionHandling {
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");
}

Expand Down
5 changes: 3 additions & 2 deletions ...src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java
View file
@@ -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.
Expand All @@ -22,6 +22,7 @@
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;

Expand All @@ -44,7 +45,7 @@ public void testSingleton() throws SecurityException, NoSuchMethodException {

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);
Expand Down

0 comments on commit fbeecf3

Please sign in to comment.