Enhance aspect to support superclass aspects

Signed-off-by: Joshua Chen <[email protected]>

Author: JoshuaChen

spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java

@@ -29,6 +29,7 @@
 import org.aopalliance.intercept.MethodInvocation;
 import org.aspectj.lang.JoinPoint;
 import org.aspectj.lang.ProceedingJoinPoint;
+import org.aspectj.runtime.internal.AroundClosure;
 import org.aspectj.weaver.tools.JoinPointMatch;
 import org.aspectj.weaver.tools.PointcutParameter;
 
@@ -57,6 +58,7 @@
  * @author Adrian Colyer
  * @author Juergen Hoeller
  * @author Ramnivas Laddad
+ * @author Joshua Chen
  * @since 2.0
  */
 @SuppressWarnings("serial")
@@ -145,6 +147,12 @@ public static JoinPoint currentJoinPoint() {
 	 */
 	private int joinPointStaticPartArgumentIndex = -1;
 
+	/**
+	 * Index for around closure argument (currently only
+	 * supported at index 0 if present at all).
+	 */
+	private int aroundClosureArgumentIndex = -1;
+
 	@Nullable
 	private Map<String, Integer> argumentBindings;
 
@@ -271,7 +279,7 @@ public void setArgumentNamesFromStringArray(String... argumentNames) {
 			if (!isVariableName(this.argumentNames[i])) {
 				throw new IllegalArgumentException(
 						"'argumentNames' property of AbstractAspectJAdvice contains an argument name '" +
-						this.argumentNames[i] + "' that is not a valid Java identifier");
+								this.argumentNames[i] + "' that is not a valid Java identifier");
 			}
 		}
 		if (this.aspectJAdviceMethod.getParameterCount() == this.argumentNames.length + 1) {
@@ -284,7 +292,7 @@ public void setArgumentNamesFromStringArray(String... 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";
+					this.argumentNames[i] = "thisJoinPoint";
 					System.arraycopy(oldNames, i, this.argumentNames, i + 1, oldNames.length - i);
 					break;
 				}
@@ -383,9 +391,14 @@ public final void calculateArgumentBindings() {
 
 		int numUnboundArgs = this.parameterTypes.length;
 		Class<?>[] parameterTypes = this.aspectJAdviceMethod.getParameterTypes();
-		if (maybeBindJoinPoint(parameterTypes[0]) || maybeBindProceedingJoinPoint(parameterTypes[0]) ||
-				maybeBindJoinPointStaticPart(parameterTypes[0])) {
-			numUnboundArgs--;
+		for (int i = 0; i < parameterTypes.length; i++) {
+			if (maybeBindJoinPoint(parameterTypes[i], i) ||
+					maybeBindProceedingJoinPoint(parameterTypes[i], i) ||
+					maybeBindJoinPointStaticPart(parameterTypes[i], i) ||
+					maybeBindAroundClosure(parameterTypes[i], i)
+			) {
+				numUnboundArgs--;
+			}
 		}
 
 		if (numUnboundArgs > 0) {
@@ -396,22 +409,22 @@ public final void calculateArgumentBindings() {
 		this.argumentsIntrospected = true;
 	}
 
-	private boolean maybeBindJoinPoint(Class<?> candidateParameterType) {
+	private boolean maybeBindJoinPoint(Class<?> candidateParameterType, int index) {
 		if (JoinPoint.class == candidateParameterType) {
-			this.joinPointArgumentIndex = 0;
+			this.joinPointArgumentIndex = index;
 			return true;
 		}
 		else {
 			return false;
 		}
 	}
 
-	private boolean maybeBindProceedingJoinPoint(Class<?> candidateParameterType) {
+	private boolean maybeBindProceedingJoinPoint(Class<?> candidateParameterType, int index) {
 		if (ProceedingJoinPoint.class == candidateParameterType) {
 			if (!supportsProceedingJoinPoint()) {
 				throw new IllegalArgumentException("ProceedingJoinPoint is only supported for around advice");
 			}
-			this.joinPointArgumentIndex = 0;
+			this.joinPointArgumentIndex = index;
 			return true;
 		}
 		else {
@@ -423,9 +436,19 @@ protected boolean supportsProceedingJoinPoint() {
 		return false;
 	}
 
-	private boolean maybeBindJoinPointStaticPart(Class<?> candidateParameterType) {
+	private boolean maybeBindJoinPointStaticPart(Class<?> candidateParameterType, int index) {
 		if (JoinPoint.StaticPart.class == candidateParameterType) {
-			this.joinPointStaticPartArgumentIndex = 0;
+			this.joinPointStaticPartArgumentIndex = index;
+			return true;
+		}
+		else {
+			return false;
+		}
+	}
+
+	private boolean maybeBindAroundClosure(Class<?> candidateParameterType, int index) {
+		if (AroundClosure.class == candidateParameterType) {
+			this.aroundClosureArgumentIndex = index;
 			return true;
 		}
 		else {
@@ -480,7 +503,22 @@ private void bindExplicitArguments(int numArgumentsLeftToBind) {
 
 		// So we match in number...
 		int argumentIndexOffset = this.parameterTypes.length - numArgumentsLeftToBind;
-		for (int i = argumentIndexOffset; i < this.argumentNames.length; i++) {
+		if (this.joinPointStaticPartArgumentIndex > -1) {
+			argumentIndexOffset--;
+		}
+		if (this.joinPointArgumentIndex > -1) {
+			argumentIndexOffset--;
+		}
+		if (this.aroundClosureArgumentIndex > -1) {
+			argumentIndexOffset--;
+		}
+		for (int i = 0; i < this.argumentNames.length; i++) {
+			if (this.joinPointStaticPartArgumentIndex > -1 && i == this.joinPointStaticPartArgumentIndex) {
+				continue;
+			}
+			if (this.joinPointArgumentIndex > -1 && i == this.joinPointArgumentIndex) {
+				continue;
+			}
 			this.argumentBindings.put(this.argumentNames[i], i);
 		}
 
@@ -525,17 +563,34 @@ private void configurePointcutParameters(String[] argumentNames, int argumentInd
 		if (this.throwingName != null) {
 			numParametersToRemove++;
 		}
+		if (this.joinPointStaticPartArgumentIndex > -1) {
+			numParametersToRemove++;
+		}
+		if (this.joinPointArgumentIndex > -1) {
+			numParametersToRemove++;
+		}
+		if (this.aroundClosureArgumentIndex > -1) {
+			numParametersToRemove++;
+		}
+
 		String[] pointcutParameterNames = new String[argumentNames.length - numParametersToRemove];
 		Class<?>[] pointcutParameterTypes = new Class<?>[pointcutParameterNames.length];
 		Class<?>[] methodParameterTypes = this.aspectJAdviceMethod.getParameterTypes();
 
 		int index = 0;
 		for (int i = 0; i < argumentNames.length; i++) {
-			if (i < argumentIndexOffset) {
+			if (this.aroundClosureArgumentIndex > -1 && i == this.aroundClosureArgumentIndex) {
+				continue;
+			}
+
+			if (this.joinPointStaticPartArgumentIndex > -1 && i == this.joinPointStaticPartArgumentIndex) {
+				continue;
+			}
+			if (this.joinPointArgumentIndex > -1 && i == this.joinPointArgumentIndex) {
 				continue;
 			}
 			if (argumentNames[i].equals(this.returningName) ||
-				argumentNames[i].equals(this.throwingName)) {
+					argumentNames[i].equals(this.throwingName)) {
 				continue;
 			}
 			pointcutParameterNames[index] = argumentNames[i];
@@ -600,7 +655,9 @@ else if (this.joinPointStaticPartArgumentIndex != -1) {
 			}
 		}
 
-		if (numBound != this.parameterTypes.length) {
+		// if jp is a ProceedingJoinPoint, can ignore numBound check,
+		// because use the ProceedingJoinPoint.proceed() method in aj
+		if (!(jp instanceof ProceedingJoinPoint) && numBound != this.parameterTypes.length) {
 			throw new IllegalStateException("Required to bind " + this.parameterTypes.length +
 					" arguments, but only bound " + numBound + " (JoinPointMatch " +
 					(jpMatch == null ? "was NOT" : "WAS") + " bound in invocation)");

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJAdviceParameterNameDiscoverer.java

@@ -48,11 +48,11 @@
  * <h3>Algorithm Details</h3>
  * <p>This class interprets arguments in the following way:
  * <ol>
- * <li>If the first parameter of the method is of type {@link JoinPoint}
+ * <li>If the parameter of the method is of type {@link JoinPoint}
  * or {@link ProceedingJoinPoint}, it is assumed to be for passing
  * {@code thisJoinPoint} to the advice, and the parameter name will
  * be assigned the value {@code "thisJoinPoint"}.</li>
- * <li>If the first parameter of the method is of type
+ * <li>If the parameter of the method is of type
  * {@code JoinPoint.StaticPart}, it is assumed to be for passing
  * {@code "thisJoinPointStaticPart"} to the advice, and the parameter name
  * will be assigned the value {@code "thisJoinPointStaticPart"}.</li>
@@ -115,6 +115,7 @@
  *
  * @author Adrian Colyer
  * @author Juergen Hoeller
+ * @author Joshua Chen
  * @since 2.0
  */
 public class AspectJAdviceParameterNameDiscoverer implements ParameterNameDiscoverer {
@@ -308,22 +309,29 @@ private void bindParameterName(int index, @Nullable String name) {
 	}
 
 	/**
-	 * If the first parameter is of type JoinPoint or ProceedingJoinPoint, bind "thisJoinPoint" as
+	 * If the parameter is of type JoinPoint or ProceedingJoinPoint, bind "thisJoinPoint" as
 	 * parameter name and return true, else return false.
 	 */
 	private boolean maybeBindThisJoinPoint() {
-		if ((this.argumentTypes[0] == JoinPoint.class) || (this.argumentTypes[0] == ProceedingJoinPoint.class)) {
-			bindParameterName(0, THIS_JOIN_POINT);
-			return true;
-		}
-		else {
-			return false;
+		for (int i = 0; i < this.argumentTypes.length; i++) {
+			if (isUnbound(i) && (this.argumentTypes[i] == JoinPoint.class || this.argumentTypes[i] == ProceedingJoinPoint.class)) {
+				bindParameterName(i, THIS_JOIN_POINT);
+				return true;
+			}
 		}
+		return false;
 	}
 
+	/**
+	 * If the parameter is of type JoinPoint.StaticPart, bind "thisJoinPointStaticPart" as
+	 * parameter name.
+	 */
 	private void maybeBindThisJoinPointStaticPart() {
-		if (this.argumentTypes[0] == JoinPoint.StaticPart.class) {
-			bindParameterName(0, THIS_JOIN_POINT_STATIC_PART);
+		for (int i = 0; i < this.argumentTypes.length; i++) {
+			if (isUnbound(i) && this.argumentTypes[i] == JoinPoint.StaticPart.class) {
+				bindParameterName(i, THIS_JOIN_POINT_STATIC_PART);
+				return;
+			}
 		}
 	}
 

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2024 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.
@@ -53,6 +53,7 @@
  * @author Adrian Colyer
  * @author Juergen Hoeller
  * @author Sam Brannen
+ * @author Joshua Chen
  * @since 2.0
  */
 public abstract class AbstractAspectJAdvisorFactory implements AspectJAdvisorFactory {
@@ -93,6 +94,17 @@ public boolean isAspect(Class<?> clazz) {
 	@Override
 	public void validate(Class<?> aspectClass) throws AopConfigException {
 		AjType<?> ajType = AjTypeSystem.getAjType(aspectClass);
+		if (aspectClass.getSuperclass() != null) {
+			Class<?> currSupperClass = aspectClass;
+			while (currSupperClass != Object.class) {
+				AjType<?> ajTypeToCheck = AjTypeSystem.getAjType(currSupperClass);
+				if (ajTypeToCheck.isAspect()) {
+					ajType = ajTypeToCheck;
+					break;
+				}
+				currSupperClass = currSupperClass.getSuperclass();
+			}
+		}
 		if (!ajType.isAspect()) {
 			throw new NotAnAtAspectException(aspectClass);
 		}

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2024 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.
@@ -40,6 +40,7 @@
  *
  * @author Rod Johnson
  * @author Juergen Hoeller
+ * @author Joshua Chen
  * @since 2.0
  * @see org.springframework.aop.aspectj.AspectJExpressionPointcut
  */
@@ -81,23 +82,26 @@ public class AspectMetadata implements Serializable {
 	public AspectMetadata(Class<?> aspectClass, String aspectName) {
 		this.aspectName = aspectName;
 
-		Class<?> currClass = aspectClass;
-		AjType<?> ajType = null;
-		while (currClass != Object.class) {
-			AjType<?> ajTypeToCheck = AjTypeSystem.getAjType(currClass);
-			if (ajTypeToCheck.isAspect()) {
-				ajType = ajTypeToCheck;
-				break;
+		AjType<?> ajType = AjTypeSystem.getAjType(aspectClass);
+		if(aspectClass.getSuperclass() != null) {
+			Class<?> currSupperClass = aspectClass;
+			while (currSupperClass != Object.class) {
+				AjType<?> ajTypeToCheck = AjTypeSystem.getAjType(currSupperClass);
+				if (ajTypeToCheck.isAspect()) {
+					ajType = ajTypeToCheck;
+					break;
+				}
+				currSupperClass = currSupperClass.getSuperclass();
 			}
-			currClass = currClass.getSuperclass();
 		}
-		if (ajType == null) {
+
+		if (ajType == null || !ajType.isAspect()) {
 			throw new IllegalArgumentException("Class '" + aspectClass.getName() + "' is not an @AspectJ aspect");
 		}
 		if (ajType.getDeclarePrecedence().length > 0) {
 			throw new IllegalArgumentException("DeclarePrecedence not presently supported in Spring AOP");
 		}
-		this.aspectClass = ajType.getJavaClass();
+		this.aspectClass = aspectClass;
 		this.ajType = ajType;
 
 		switch (this.ajType.getPerClause().getKind()) {

spring-aop/src/test/java/org/springframework/aop/aspectj/AbstractAspectJAdviceTests.java

@@ -45,31 +45,31 @@ void setArgumentNamesFromStringArray_withoutJoinPointParameter() {
 	@Test
 	void setArgumentNamesFromStringArray_withJoinPointAsFirstParameter() {
 		AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsFirstParameter");
-		assertThat(advice).satisfies(hasArgumentNames("THIS_JOIN_POINT", "arg1", "arg2"));
+		assertThat(advice).satisfies(hasArgumentNames("thisJoinPoint", "arg1", "arg2"));
 	}
 
 	@Test
 	void setArgumentNamesFromStringArray_withJoinPointAsLastParameter() {
 		AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsLastParameter");
-		assertThat(advice).satisfies(hasArgumentNames("arg1", "arg2", "THIS_JOIN_POINT"));
+		assertThat(advice).satisfies(hasArgumentNames("arg1", "arg2", "thisJoinPoint"));
 	}
 
 	@Test
 	void setArgumentNamesFromStringArray_withJoinPointAsMiddleParameter() {
 		AbstractAspectJAdvice advice = getAspectJAdvice("methodWithJoinPointAsMiddleParameter");
-		assertThat(advice).satisfies(hasArgumentNames("arg1", "THIS_JOIN_POINT", "arg2"));
+		assertThat(advice).satisfies(hasArgumentNames("arg1", "thisJoinPoint", "arg2"));
 	}
 
 	@Test
 	void setArgumentNamesFromStringArray_withProceedingJoinPoint() {
 		AbstractAspectJAdvice advice = getAspectJAdvice("methodWithProceedingJoinPoint");
-		assertThat(advice).satisfies(hasArgumentNames("THIS_JOIN_POINT", "arg1", "arg2"));
+		assertThat(advice).satisfies(hasArgumentNames("thisJoinPoint", "arg1", "arg2"));
 	}
 
 	@Test
 	void setArgumentNamesFromStringArray_withStaticPart() {
 		AbstractAspectJAdvice advice = getAspectJAdvice("methodWithStaticPart");
-		assertThat(advice).satisfies(hasArgumentNames("THIS_JOIN_POINT", "arg1", "arg2"));
+		assertThat(advice).satisfies(hasArgumentNames("thisJoinPoint", "arg1", "arg2"));
 	}
 
 	private Consumer<AbstractAspectJAdvice> hasArgumentNames(String... argumentNames) {