From 13ba770428fb0e7e5571279f7bf6dd7b3c624c79 Mon Sep 17 00:00:00 2001 From: Joshua Chen <27291761@qq.com> Date: Fri, 24 Jan 2025 22:12:31 +0800 Subject: [PATCH 1/2] Handle arbitrary JoinPoint argument index See gh-34316 Signed-off-by: Joshua Chen <27291761@qq.com> --- .../aop/aspectj/AbstractAspectJAdvice.java | 20 +-- .../aspectj/AbstractAspectJAdviceTests.java | 132 ++++++++++++++++++ 2 files changed, 144 insertions(+), 8 deletions(-) create mode 100644 spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java index 2e2ee857f3d4..50bfa14dfe1f 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java @@ -276,14 +276,18 @@ public void setArgumentNamesFromStringArray(String... argumentNames) { } if (this.aspectJAdviceMethod.getParameterCount() == this.argumentNames.length + 1) { // May need to add implicit join point arg name... - Class firstArgType = this.aspectJAdviceMethod.getParameterTypes()[0]; - if (firstArgType == JoinPoint.class || - firstArgType == ProceedingJoinPoint.class || - firstArgType == JoinPoint.StaticPart.class) { - String[] oldNames = this.argumentNames; - this.argumentNames = new String[oldNames.length + 1]; - this.argumentNames[0] = "THIS_JOIN_POINT"; - System.arraycopy(oldNames, 0, this.argumentNames, 1, oldNames.length); + for (int i = 0; i < this.aspectJAdviceMethod.getParameterCount(); i++) { + Class argType = this.aspectJAdviceMethod.getParameterTypes()[i]; + if (argType == JoinPoint.class || + argType == ProceedingJoinPoint.class || + argType == JoinPoint.StaticPart.class) { + String[] oldNames = this.argumentNames; + this.argumentNames = new String[oldNames.length + 1]; + System.arraycopy(oldNames, 0, this.argumentNames, 0, i); + this.argumentNames[i] = "THIS_JOIN_POINT"; + System.arraycopy(oldNames, i, this.argumentNames, i + 1, oldNames.length - i); + break; + } } } } diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java new file mode 100644 index 000000000000..0cba61aacd6c --- /dev/null +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java @@ -0,0 +1,132 @@ +/* + * Copyright 2002-2025 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.aspectj; + +import java.io.Serial; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.Arrays; + +import org.aspectj.lang.JoinPoint; +import org.aspectj.lang.ProceedingJoinPoint; +import org.jetbrains.annotations.NotNull; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +/** + * Tests for {@link AbstractAspectJAdvice}. + * + * @author Joshua Chen + */ +class AbstractAspectJAdviceTests { + + @Test + void setArgumentNamesFromStringArray_withJoinPointAsFirstParameter() { + AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsFirstParameter"); + assertArgumentNamesFromStringArray(advice); + } + + @Test + void setArgumentNamesFromStringArray_withJoinPointAsLastParameter() { + AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsLastParameter"); + assertThat(getArgumentNames(advice)[0]).isEqualTo("arg1"); + assertThat(getArgumentNames(advice)[1]).isEqualTo("arg2"); + assertThat(getArgumentNames(advice)[2]).isEqualTo("THIS_JOIN_POINT"); + } + + @Test + void setArgumentNamesFromStringArray_withJoinPointAsMiddleParameter() { + AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsMiddleParameter"); + assertThat(getArgumentNames(advice)[0]).isEqualTo("arg1"); + assertThat(getArgumentNames(advice)[1]).isEqualTo("THIS_JOIN_POINT"); + assertThat(getArgumentNames(advice)[2]).isEqualTo("arg2"); + } + + @Test + void setArgumentNamesFromStringArray_withProceedingJoinPoint() { + AbstractAspectJAdvice advice = getAspectJAdvice("methodWithProceedingJoinPoint"); + assertArgumentNamesFromStringArray(advice); + } + + @Test + void setArgumentNamesFromStringArray_withStaticPart() { + AbstractAspectJAdvice advice = getAspectJAdvice("methodWithStaticPart"); + assertArgumentNamesFromStringArray(advice); + } + + private void assertArgumentNamesFromStringArray(AbstractAspectJAdvice advice) { + assertThat(getArgumentNames(advice)[0]).isEqualTo("THIS_JOIN_POINT"); + assertThat(getArgumentNames(advice)[1]).isEqualTo("arg1"); + assertThat(getArgumentNames(advice)[2]).isEqualTo("arg2"); + } + + private @NotNull AbstractAspectJAdvice getAspectJAdvice(final String methodName) { + AbstractAspectJAdvice advice = new TestAspectJAdvice(getMethod(methodName), mock(AspectJExpressionPointcut.class), mock(AspectInstanceFactory.class)); + advice.setArgumentNamesFromStringArray("arg1", "arg2"); + return advice; + } + + private Method getMethod(final String name) { + return Arrays.stream(this.getClass().getDeclaredMethods()).filter(m -> m.getName().equals(name)).findFirst().orElseThrow(); + } + + private String[] getArgumentNames(final AbstractAspectJAdvice advice) { + try { + Field field = AbstractAspectJAdvice.class.getDeclaredField("argumentNames"); + field.setAccessible(true); + return (String[]) field.get(advice); + } + catch (NoSuchFieldException | IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + public static class TestAspectJAdvice extends AbstractAspectJAdvice { + @Serial private static final long serialVersionUID = 1L; + + public TestAspectJAdvice(Method aspectJAdviceMethod, AspectJExpressionPointcut pointcut, AspectInstanceFactory aspectInstanceFactory) { + super(aspectJAdviceMethod, pointcut, aspectInstanceFactory); + } + + @Override + public boolean isBeforeAdvice() { + return false; + } + + @Override + public boolean isAfterAdvice() { + return false; + } + } + + void methodWithJoinPointAsFirstParameter(JoinPoint joinPoint, String arg1, String arg2) { + } + + void methodWithJoinPointAsLastParameter(String arg1, String arg2, JoinPoint joinPoint) { + } + + void methodWithJoinPointAsMiddleParameter(String arg1, JoinPoint joinPoint, String arg2) { + } + + void methodWithProceedingJoinPoint(ProceedingJoinPoint joinPoint, String arg1, String arg2) { + } + + void methodWithStaticPart(JoinPoint.StaticPart staticPart, String arg1, String arg2) { + } +} From fb6e86551a46d2f02b12d613048604d895588860 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Nicoll?= Date: Wed, 5 Feb 2025 11:45:17 +0100 Subject: [PATCH 2/2] Polish "Handle arbitrary JoinPoint argument index" See gh-34316 --- .../aop/aspectj/AbstractAspectJAdvice.java | 4 +- .../aspectj/AbstractAspectJAdviceTests.java | 87 ++++++++++--------- 2 files changed, 47 insertions(+), 44 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java index 50bfa14dfe1f..f6278b109709 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2025 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. @@ -281,7 +281,7 @@ public void setArgumentNamesFromStringArray(String... argumentNames) { if (argType == JoinPoint.class || argType == ProceedingJoinPoint.class || argType == JoinPoint.StaticPart.class) { - String[] oldNames = this.argumentNames; + String[] oldNames = this.argumentNames; this.argumentNames = new String[oldNames.length + 1]; System.arraycopy(oldNames, 0, this.argumentNames, 0, i); this.argumentNames[i] = "THIS_JOIN_POINT"; diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java index 0cba61aacd6c..f6768b0e8440 100644 --- a/spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java @@ -16,14 +16,13 @@ package org.springframework.aop.aspectj; -import java.io.Serial; -import java.lang.reflect.Field; import java.lang.reflect.Method; import java.util.Arrays; +import java.util.function.Consumer; import org.aspectj.lang.JoinPoint; import org.aspectj.lang.ProceedingJoinPoint; -import org.jetbrains.annotations.NotNull; +import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; @@ -33,74 +32,70 @@ * Tests for {@link AbstractAspectJAdvice}. * * @author Joshua Chen + * @author Stephane Nicoll */ class AbstractAspectJAdviceTests { + @Test + void setArgumentNamesFromStringArray_withoutJoinPointParameter() { + AbstractAspectJAdvice advice = getAspectJAdvice("methodWithNoJoinPoint"); + assertThat(advice).satisfies(hasArgumentNames("arg1", "arg2")); + } + @Test void setArgumentNamesFromStringArray_withJoinPointAsFirstParameter() { AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsFirstParameter"); - assertArgumentNamesFromStringArray(advice); + assertThat(advice).satisfies(hasArgumentNames("THIS_JOIN_POINT", "arg1", "arg2")); } @Test void setArgumentNamesFromStringArray_withJoinPointAsLastParameter() { AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsLastParameter"); - assertThat(getArgumentNames(advice)[0]).isEqualTo("arg1"); - assertThat(getArgumentNames(advice)[1]).isEqualTo("arg2"); - assertThat(getArgumentNames(advice)[2]).isEqualTo("THIS_JOIN_POINT"); + assertThat(advice).satisfies(hasArgumentNames("arg1", "arg2", "THIS_JOIN_POINT")); } @Test void setArgumentNamesFromStringArray_withJoinPointAsMiddleParameter() { AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsMiddleParameter"); - assertThat(getArgumentNames(advice)[0]).isEqualTo("arg1"); - assertThat(getArgumentNames(advice)[1]).isEqualTo("THIS_JOIN_POINT"); - assertThat(getArgumentNames(advice)[2]).isEqualTo("arg2"); + assertThat(advice).satisfies(hasArgumentNames("arg1", "THIS_JOIN_POINT", "arg2")); } @Test void setArgumentNamesFromStringArray_withProceedingJoinPoint() { AbstractAspectJAdvice advice = getAspectJAdvice("methodWithProceedingJoinPoint"); - assertArgumentNamesFromStringArray(advice); + assertThat(advice).satisfies(hasArgumentNames("THIS_JOIN_POINT", "arg1", "arg2")); } @Test void setArgumentNamesFromStringArray_withStaticPart() { AbstractAspectJAdvice advice = getAspectJAdvice("methodWithStaticPart"); - assertArgumentNamesFromStringArray(advice); + assertThat(advice).satisfies(hasArgumentNames("THIS_JOIN_POINT", "arg1", "arg2")); } - private void assertArgumentNamesFromStringArray(AbstractAspectJAdvice advice) { - assertThat(getArgumentNames(advice)[0]).isEqualTo("THIS_JOIN_POINT"); - assertThat(getArgumentNames(advice)[1]).isEqualTo("arg1"); - assertThat(getArgumentNames(advice)[2]).isEqualTo("arg2"); + private Consumer hasArgumentNames(String... argumentNames) { + return advice -> assertThat(advice).extracting("argumentNames") + .asInstanceOf(InstanceOfAssertFactories.array(String[].class)) + .containsExactly(argumentNames); } - private @NotNull AbstractAspectJAdvice getAspectJAdvice(final String methodName) { - AbstractAspectJAdvice advice = new TestAspectJAdvice(getMethod(methodName), mock(AspectJExpressionPointcut.class), mock(AspectInstanceFactory.class)); + private AbstractAspectJAdvice getAspectJAdvice(final String methodName) { + AbstractAspectJAdvice advice = new TestAspectJAdvice(getMethod(methodName), + mock(AspectJExpressionPointcut.class), mock(AspectInstanceFactory.class)); advice.setArgumentNamesFromStringArray("arg1", "arg2"); return advice; } - private Method getMethod(final String name) { - return Arrays.stream(this.getClass().getDeclaredMethods()).filter(m -> m.getName().equals(name)).findFirst().orElseThrow(); - } - - private String[] getArgumentNames(final AbstractAspectJAdvice advice) { - try { - Field field = AbstractAspectJAdvice.class.getDeclaredField("argumentNames"); - field.setAccessible(true); - return (String[]) field.get(advice); - } - catch (NoSuchFieldException | IllegalAccessException e) { - throw new RuntimeException(e); - } + private Method getMethod(final String methodName) { + return Arrays.stream(Sample.class.getDeclaredMethods()) + .filter(method -> method.getName().equals(methodName)).findFirst() + .orElseThrow(); } + @SuppressWarnings("serial") public static class TestAspectJAdvice extends AbstractAspectJAdvice { - @Serial private static final long serialVersionUID = 1L; - public TestAspectJAdvice(Method aspectJAdviceMethod, AspectJExpressionPointcut pointcut, AspectInstanceFactory aspectInstanceFactory) { + public TestAspectJAdvice(Method aspectJAdviceMethod, AspectJExpressionPointcut pointcut, + AspectInstanceFactory aspectInstanceFactory) { super(aspectJAdviceMethod, pointcut, aspectInstanceFactory); } @@ -115,18 +110,26 @@ public boolean isAfterAdvice() { } } - void methodWithJoinPointAsFirstParameter(JoinPoint joinPoint, String arg1, String arg2) { - } + @SuppressWarnings("unused") + static class Sample { - void methodWithJoinPointAsLastParameter(String arg1, String arg2, JoinPoint joinPoint) { - } + void methodWithNoJoinPoint(String arg1, String arg2) { + } - void methodWithJoinPointAsMiddleParameter(String arg1, JoinPoint joinPoint, String arg2) { - } + void methodWithJoinPointAsFirstParameter(JoinPoint joinPoint, String arg1, String arg2) { + } - void methodWithProceedingJoinPoint(ProceedingJoinPoint joinPoint, String arg1, String arg2) { - } + void methodWithJoinPointAsLastParameter(String arg1, String arg2, JoinPoint joinPoint) { + } + + void methodWithJoinPointAsMiddleParameter(String arg1, JoinPoint joinPoint, String arg2) { + } + + void methodWithProceedingJoinPoint(ProceedingJoinPoint joinPoint, String arg1, String arg2) { + } - void methodWithStaticPart(JoinPoint.StaticPart staticPart, String arg1, String arg2) { + void methodWithStaticPart(JoinPoint.StaticPart staticPart, String arg1, String arg2) { + } } + }