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 f6278b109709..6be1c84ecdc5 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 @@ -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 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,9 +409,9 @@ 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 { @@ -406,12 +419,12 @@ private boolean maybeBindJoinPoint(Class candidateParameterType) { } } - 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)"); diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJAdviceParameterNameDiscoverer.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJAdviceParameterNameDiscoverer.java index e581f6814dbe..cf249b69101f 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJAdviceParameterNameDiscoverer.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJAdviceParameterNameDiscoverer.java @@ -48,11 +48,11 @@ *

Algorithm Details

*

This class interprets arguments in the following way: *

    - *
  1. If the first parameter of the method is of type {@link JoinPoint} + *
  2. 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"}.
    3. - *
  3. If the first parameter of the method is of type + *
  4. 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"}.
    5. @@ -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; + } } } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java index 8400c54360a9..1c220eb8be25 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java +++ b/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); } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java index 2775bc9fd32c..afe151162e4d 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java +++ b/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()) { 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 f6768b0e8440..c58fc5ad6f74 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 @@ -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 hasArgumentNames(String... argumentNames) { diff --git a/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AbstractTransactionAspect.aj b/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AbstractTransactionAspect.aj index 782ca35e0777..af731097a5af 100644 --- a/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AbstractTransactionAspect.aj +++ b/spring-aspects/src/main/java/org/springframework/transaction/aspectj/AbstractTransactionAspect.aj @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 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. @@ -16,6 +16,7 @@ package org.springframework.transaction.aspectj; +import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.SuppressAjWarnings; import org.aspectj.lang.reflect.MethodSignature; @@ -43,69 +44,76 @@ import org.springframework.transaction.interceptor.TransactionAttributeSource; * @author Rod Johnson * @author Ramnivas Laddad * @author Juergen Hoeller + * @author Joshua Chen * @since 2.0 */ public abstract aspect AbstractTransactionAspect extends TransactionAspectSupport implements DisposableBean { - /** - * Construct the aspect using the given transaction metadata retrieval strategy. - * @param tas TransactionAttributeSource implementation, retrieving Spring - * transaction metadata for each joinpoint. Implement the subclass to pass in - * {@code null} if it is intended to be configured through Setter Injection. - */ - protected AbstractTransactionAspect(TransactionAttributeSource tas) { - setTransactionAttributeSource(tas); - } + /** + * Construct the aspect using the given transaction metadata retrieval strategy. + * @param tas TransactionAttributeSource implementation, retrieving Spring + * transaction metadata for each joinpoint. Implement the subclass to pass in + * {@code null} if it is intended to be configured through Setter Injection. + */ + protected AbstractTransactionAspect(TransactionAttributeSource tas) { + setTransactionAttributeSource(tas); + } - @Override - public void destroy() { - // An aspect is basically a singleton -> cleanup on destruction - clearTransactionManagerCache(); - } + @Override + public void destroy() { + // An aspect is basically a singleton -> cleanup on destruction + clearTransactionManagerCache(); + } - @SuppressAjWarnings("adviceDidNotMatch") - Object around(final Object txObject): transactionalMethodExecution(txObject) { - MethodSignature methodSignature = (MethodSignature) thisJoinPoint.getSignature(); - // Adapt to TransactionAspectSupport's invokeWithinTransaction... - try { - return invokeWithinTransaction(methodSignature.getMethod(), txObject.getClass(), new InvocationCallback() { - public Object proceedWithInvocation() throws Throwable { - return proceed(txObject); - } - }); - } - catch (RuntimeException | Error ex) { - throw ex; - } - catch (Throwable thr) { - Rethrower.rethrow(thr); - throw new IllegalStateException("Should never get here", thr); - } - } + @SuppressAjWarnings("adviceDidNotMatch") + Object around(final Object txObject): transactionalMethodExecution(txObject) { + ProceedingJoinPoint proceedingJoinPoint = (ProceedingJoinPoint) thisJoinPoint; + MethodSignature methodSignature = (MethodSignature) proceedingJoinPoint.getSignature(); + // Adapt to TransactionAspectSupport's invokeWithinTransaction... + try { + return invokeWithinTransaction(methodSignature.getMethod(), txObject.getClass(), new InvocationCallback() { + public Object proceedWithInvocation() throws Throwable { + if (proceedingJoinPoint.getArgs().length > 0) { + // Support for method with arguments, in case of exception + return proceed(txObject); + } + else { + return proceedingJoinPoint.proceed(); + } + } + }); + } + catch (RuntimeException | Error ex) { + throw ex; + } + catch (Throwable thr) { + Rethrower.rethrow(thr); + throw new IllegalStateException("Should never get here", thr); + } + } - /** - * Concrete subaspects must implement this pointcut, to identify - * transactional methods. For each selected joinpoint, TransactionMetadata - * will be retrieved using Spring's TransactionAttributeSource interface. - */ - protected abstract pointcut transactionalMethodExecution(Object txObject); + /** + * Concrete subaspects must implement this pointcut, to identify + * transactional methods. For each selected joinpoint, TransactionMetadata + * will be retrieved using Spring's TransactionAttributeSource interface. + */ + protected abstract pointcut transactionalMethodExecution(Object txObject); + /** + * Ugly but safe workaround: We need to be able to propagate checked exceptions, + * despite AspectJ around advice supporting specifically declared exceptions only. + */ + private static class Rethrower { - /** - * Ugly but safe workaround: We need to be able to propagate checked exceptions, - * despite AspectJ around advice supporting specifically declared exceptions only. - */ - private static class Rethrower { - - public static void rethrow(final Throwable exception) { - class CheckedExceptionRethrower { - @SuppressWarnings("unchecked") - private void rethrow(Throwable exception) throws T { - throw (T) exception; - } - } - new CheckedExceptionRethrower().rethrow(exception); - } - } + public static void rethrow(final Throwable exception) { + class CheckedExceptionRethrower { + @SuppressWarnings("unchecked") + private void rethrow(Throwable exception) throws T { + throw (T) exception; + } + } + new CheckedExceptionRethrower().rethrow(exception); + } + } }