From e73d68b0a8682fc0a1b2f06feb9c9fc7af3a90ba Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 22 May 2024 10:07:31 +0200 Subject: [PATCH] Select most specific advice method in case of override Closes gh-32865 (cherry picked from commit ea596aa211ea17b814099a73bb5a091a9af1cadd) --- .../ReflectiveAspectJAdvisorFactory.java | 27 +++++++++-------- .../AbstractAspectJAdvisorFactoryTests.java | 30 ++++++++++--------- 2 files changed, 31 insertions(+), 26 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 4e2f5c3bd80..90cc3880cb8 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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,6 +50,7 @@ import org.springframework.core.annotation.AnnotationUtils; import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.ConvertingComparator; import org.springframework.lang.Nullable; +import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; import org.springframework.util.ReflectionUtils.MethodFilter; import org.springframework.util.StringUtils; @@ -133,17 +134,19 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto List advisors = new ArrayList<>(); for (Method method : getAdvisorMethods(aspectClass)) { - // 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); + if (method.equals(ClassUtils.getMostSpecificMethod(method, aspectClass))) { + // 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/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java index 978cd049540..02d968212d5 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -38,7 +38,6 @@ import org.aspectj.lang.annotation.DeclareParents; import org.aspectj.lang.annotation.DeclarePrecedence; import org.aspectj.lang.annotation.Pointcut; import org.aspectj.lang.reflect.MethodSignature; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.springframework.aop.Advisor; @@ -84,15 +83,15 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Test void rejectsPerCflowAspect() { assertThatExceptionOfType(AopConfigException.class) - .isThrownBy(() -> getAdvisorFactory().getAdvisors(aspectInstanceFactory(new PerCflowAspect(), "someBean"))) - .withMessageContaining("PERCFLOW"); + .isThrownBy(() -> getAdvisorFactory().getAdvisors(aspectInstanceFactory(new PerCflowAspect(), "someBean"))) + .withMessageContaining("PERCFLOW"); } @Test void rejectsPerCflowBelowAspect() { assertThatExceptionOfType(AopConfigException.class) - .isThrownBy(() -> getAdvisorFactory().getAdvisors(aspectInstanceFactory(new PerCflowBelowAspect(), "someBean"))) - .withMessageContaining("PERCFLOWBELOW"); + .isThrownBy(() -> getAdvisorFactory().getAdvisors(aspectInstanceFactory(new PerCflowBelowAspect(), "someBean"))) + .withMessageContaining("PERCFLOWBELOW"); } @Test @@ -363,7 +362,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { assertThat(lockable.locked()).as("Already locked").isTrue(); lockable.lock(); assertThat(lockable.locked()).as("Real target ignores locking").isTrue(); - assertThatExceptionOfType(UnsupportedOperationException.class).isThrownBy(() -> lockable.unlock()); + assertThatExceptionOfType(UnsupportedOperationException.class).isThrownBy(lockable::unlock); } @Test @@ -389,9 +388,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { assertThat(lockable.locked()).isTrue(); } - // TODO: Why does this test fail? It hasn't been run before, so it maybe never actually passed... @Test - @Disabled void introductionWithArgumentBinding() { TestBean target = new TestBean(); @@ -648,7 +645,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { static class NamedPointcutAspectWithFQN { @SuppressWarnings("unused") - private ITestBean fieldThatShouldBeIgnoredBySpringAtAspectJProcessing = new TestBean(); + private final ITestBean fieldThatShouldBeIgnoredBySpringAtAspectJProcessing = new TestBean(); @Around("org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.CommonPointcuts.getAge()()") int changeReturnValue(ProceedingJoinPoint pjp) { @@ -765,7 +762,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Aspect - class DoublingAspect { + static class DoublingAspect { @Around("execution(* getAge())") public Object doubleAge(ProceedingJoinPoint pjp) throws Throwable { @@ -773,8 +770,14 @@ abstract class AbstractAspectJAdvisorFactoryTests { } } + @Aspect - class IncrementingAspect extends DoublingAspect { + static class IncrementingAspect extends DoublingAspect { + + @Override + public Object doubleAge(ProceedingJoinPoint pjp) throws Throwable { + return ((int) pjp.proceed()) * 2; + } @Around("execution(* getAge())") public int incrementAge(ProceedingJoinPoint pjp) throws Throwable { @@ -783,7 +786,6 @@ abstract class AbstractAspectJAdvisorFactoryTests { } - @Aspect private static class InvocationTrackingAspect { @@ -1083,7 +1085,7 @@ class PerThisAspect { // Just to check that this doesn't cause problems with introduction processing @SuppressWarnings("unused") - private ITestBean fieldThatShouldBeIgnoredBySpringAtAspectJProcessing = new TestBean(); + private final ITestBean fieldThatShouldBeIgnoredBySpringAtAspectJProcessing = new TestBean(); @Around("execution(int *.getAge())") int returnCountAsAge() {