Skip to content

Commit

Permalink
Accept ajc-compiled @aspect classes for Spring AOP proxy usage
Browse files Browse the repository at this point in the history 
AspectJExpressionPointcut leniently ignores unsupported expression.

Closes gh-32793
  • Loading branch information
@jhoeller
jhoeller committed May 14, 2024
1 parent ca9eb57 commit 0c65a5e
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 80 deletions.
25 changes: 15 additions & 10 deletions spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -174,25 +174,30 @@ public void setBeanFactory(BeanFactory beanFactory) {

@Override
public ClassFilter getClassFilter() {
obtainPointcutExpression();
checkExpression();
return this;
}

@Override
public MethodMatcher getMethodMatcher() {
obtainPointcutExpression();
checkExpression();
return this;
}


/**
* Check whether this pointcut is ready to match,
* lazily building the underlying AspectJ pointcut expression.
* Check whether this pointcut is ready to match.
*/
private PointcutExpression obtainPointcutExpression() {
private void checkExpression() {
if (getExpression() == null) {
throw new IllegalStateException("Must set property 'expression' before attempting to match");
}
}

/**
* Lazily build the underlying AspectJ pointcut expression.
*/
private PointcutExpression obtainPointcutExpression() {
if (this.pointcutExpression == null) {
this.pointcutClassLoader = determinePointcutClassLoader();
this.pointcutExpression = buildPointcutExpression(this.pointcutClassLoader);
Expand Down Expand Up @@ -269,10 +274,9 @@ public PointcutExpression getPointcutExpression() {

@Override
public boolean matches(Class<?> targetClass) {
PointcutExpression pointcutExpression = obtainPointcutExpression();
try {
try {
return pointcutExpression.couldMatchJoinPointsInType(targetClass);
return obtainPointcutExpression().couldMatchJoinPointsInType(targetClass);
}
catch (ReflectionWorldException ex) {
logger.debug("PointcutExpression matching rejected target class - trying fallback expression", ex);
Expand All @@ -283,6 +287,9 @@ public boolean matches(Class<?> targetClass) {
}
}
}
catch (IllegalArgumentException | IllegalStateException ex) {
throw ex;
}
catch (Throwable ex) {
logger.debug("PointcutExpression matching rejected target class", ex);
}
Expand All @@ -291,7 +298,6 @@ public boolean matches(Class<?> targetClass) {

@Override
public boolean matches(Method method, Class<?> targetClass, boolean hasIntroductions) {
obtainPointcutExpression();
ShadowMatch shadowMatch = getTargetShadowMatch(method, targetClass);

// Special handling for this, target, @this, @target, @annotation
Expand Down Expand Up @@ -329,7 +335,6 @@ public boolean isRuntime() {

@Override
public boolean matches(Method method, Class<?> targetClass, Object... args) {
obtainPointcutExpression();
ShadowMatch shadowMatch = getTargetShadowMatch(method, targetClass);

// Bind Spring AOP proxy to AspectJ "this" and Spring AOP target to AspectJ target,
Expand Down
34 changes: 3 additions & 31 deletions ...c/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 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.
Expand All @@ -18,7 +18,6 @@

import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.HashMap;
Expand Down Expand Up @@ -57,8 +56,6 @@
*/
public abstract class AbstractAspectJAdvisorFactory implements AspectJAdvisorFactory {

private static final String AJC_MAGIC = "ajc$";

private static final Class<?>[] ASPECTJ_ANNOTATION_CLASSES = new Class<?>[] {
Pointcut.class, Around.class, Before.class, After.class, AfterReturning.class, AfterThrowing.class};

Expand All @@ -69,37 +66,11 @@ public abstract class AbstractAspectJAdvisorFactory implements AspectJAdvisorFac
protected final ParameterNameDiscoverer parameterNameDiscoverer = new AspectJAnnotationParameterNameDiscoverer();


/**
* We consider something to be an AspectJ aspect suitable for use by the Spring AOP system
* if it has the @Aspect annotation, and was not compiled by ajc. The reason for this latter test
* is that aspects written in the code-style (AspectJ language) also have the annotation present
* when compiled by ajc with the -1.5 flag, yet they cannot be consumed by Spring AOP.
*/
@Override
public boolean isAspect(Class<?> clazz) {
return (hasAspectAnnotation(clazz) && !compiledByAjc(clazz));
}

private boolean hasAspectAnnotation(Class<?> clazz) {
return (AnnotationUtils.findAnnotation(clazz, Aspect.class) != null);
}

/**
* We need to detect this as "code-style" AspectJ aspects should not be
* interpreted by Spring AOP.
*/
private boolean compiledByAjc(Class<?> clazz) {
// The AJTypeSystem goes to great lengths to provide a uniform appearance between code-style and
// annotation-style aspects. Therefore there is no 'clean' way to tell them apart. Here we rely on
// an implementation detail of the AspectJ compiler.
for (Field field : clazz.getDeclaredFields()) {
if (field.getName().startsWith(AJC_MAGIC)) {
return true;
}
}
return false;
}

@Override
public void validate(Class<?> aspectClass) throws AopConfigException {
// If the parent has the annotation and isn't abstract it's an error
Expand All @@ -124,6 +95,7 @@ public void validate(Class<?> aspectClass) throws AopConfigException {
}
}


/**
* Find and return the first AspectJ annotation on the given method
* (there <i>should</i> only be one anyway...).
Expand Down Expand Up @@ -163,7 +135,7 @@ protected enum AspectJAnnotationType {


/**
* Class modelling an AspectJ annotation, exposing its type enumeration and
* Class modeling an AspectJ annotation, exposing its type enumeration and
* pointcut String.
* @param <A> the annotation type
*/
Expand Down
73 changes: 34 additions & 39 deletions spring-aop/src/test/java/org/springframework/aop/aspectj/AspectJExpressionPointcutTests.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2022 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.
Expand All @@ -23,9 +23,6 @@

import org.aopalliance.intercept.MethodInterceptor;
import org.aopalliance.intercept.MethodInvocation;
import org.aspectj.weaver.tools.PointcutExpression;
import org.aspectj.weaver.tools.PointcutPrimitive;
import org.aspectj.weaver.tools.UnsupportedPointcutPrimitiveException;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import test.annotation.EmptySpringAnnotation;
Expand All @@ -42,7 +39,6 @@
import org.springframework.beans.testfixture.beans.subpkg.DeepBean;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;

Expand All @@ -65,7 +61,7 @@ public class AspectJExpressionPointcutTests {


@BeforeEach
public void setUp() throws NoSuchMethodException {
public void setup() throws NoSuchMethodException {
getAge = TestBean.class.getMethod("getAge");
setAge = TestBean.class.getMethod("setAge", int.class);
setSomeNumber = TestBean.class.getMethod("setSomeNumber", Number.class);
Expand Down Expand Up @@ -175,25 +171,25 @@ private void testWithinPackage(boolean matchSubpackages) throws SecurityExceptio
@Test
public void testFriendlyErrorOnNoLocationClassMatching() {
AspectJExpressionPointcut pc = new AspectJExpressionPointcut();
assertThatIllegalStateException().isThrownBy(() ->
pc.matches(ITestBean.class))
.withMessageContaining("expression");
assertThatIllegalStateException()
.isThrownBy(() -> pc.getClassFilter().matches(ITestBean.class))
.withMessageContaining("expression");
}

@Test
public void testFriendlyErrorOnNoLocation2ArgMatching() {
AspectJExpressionPointcut pc = new AspectJExpressionPointcut();
assertThatIllegalStateException().isThrownBy(() ->
pc.matches(getAge, ITestBean.class))
.withMessageContaining("expression");
assertThatIllegalStateException()
.isThrownBy(() -> pc.getMethodMatcher().matches(getAge, ITestBean.class))
.withMessageContaining("expression");
}

@Test
public void testFriendlyErrorOnNoLocation3ArgMatching() {
AspectJExpressionPointcut pc = new AspectJExpressionPointcut();
assertThatIllegalStateException().isThrownBy(() ->
pc.matches(getAge, ITestBean.class, (Object[]) null))
.withMessageContaining("expression");
assertThatIllegalStateException()
.isThrownBy(() -> pc.getMethodMatcher().matches(getAge, ITestBean.class, (Object[]) null))
.withMessageContaining("expression");
}


Expand All @@ -210,8 +206,10 @@ public void testMatchWithArgs() throws Exception {
// not currently testable in a reliable fashion
//assertDoesNotMatchStringClass(classFilter);

assertThat(methodMatcher.matches(setSomeNumber, TestBean.class, 12D)).as("Should match with setSomeNumber with Double input").isTrue();
assertThat(methodMatcher.matches(setSomeNumber, TestBean.class, 11)).as("Should not match setSomeNumber with Integer input").isFalse();
assertThat(methodMatcher.matches(setSomeNumber, TestBean.class, 12D))
.as("Should match with setSomeNumber with Double input").isTrue();
assertThat(methodMatcher.matches(setSomeNumber, TestBean.class, 11))
.as("Should not match setSomeNumber with Integer input").isFalse();
assertThat(methodMatcher.matches(getAge, TestBean.class)).as("Should not match getAge").isFalse();
assertThat(methodMatcher.isRuntime()).as("Should be a runtime match").isTrue();
}
Expand Down Expand Up @@ -246,14 +244,13 @@ public void testDynamicMatchingProxy() {
@Test
public void testInvalidExpression() {
String expression = "execution(void org.springframework.beans.testfixture.beans.TestBean.setSomeNumber(Number) && args(Double)";
assertThatIllegalArgumentException().isThrownBy(
getPointcut(expression)::getClassFilter); // call to getClassFilter forces resolution
assertThatIllegalArgumentException().isThrownBy(() -> getPointcut(expression).getClassFilter().matches(Object.class));
}

private TestBean getAdvisedProxy(String pointcutExpression, CallCountingInterceptor interceptor) {
TestBean target = new TestBean();

Pointcut pointcut = getPointcut(pointcutExpression);
AspectJExpressionPointcut pointcut = getPointcut(pointcutExpression);

DefaultPointcutAdvisor advisor = new DefaultPointcutAdvisor();
advisor.setAdvice(interceptor);
Expand All @@ -277,40 +274,29 @@ private void assertMatchesTestBeanClass(ClassFilter classFilter) {
@Test
public void testWithUnsupportedPointcutPrimitive() {
String expression = "call(int org.springframework.beans.testfixture.beans.TestBean.getAge())";
assertThatExceptionOfType(UnsupportedPointcutPrimitiveException.class).isThrownBy(() ->
getPointcut(expression).getClassFilter()) // call to getClassFilter forces resolution...
.satisfies(ex -> assertThat(ex.getUnsupportedPrimitive()).isEqualTo(PointcutPrimitive.CALL));
assertThat(getPointcut(expression).getClassFilter().matches(Object.class)).isFalse();
}

@Test
public void testAndSubstitution() {
Pointcut pc = getPointcut("execution(* *(..)) and args(String)");
PointcutExpression expr = ((AspectJExpressionPointcut) pc).getPointcutExpression();
assertThat(expr.getPointcutExpression()).isEqualTo("execution(* *(..)) && args(String)");
AspectJExpressionPointcut pc = getPointcut("execution(* *(..)) and args(String)");
String expr = pc.getPointcutExpression().getPointcutExpression();
assertThat(expr).isEqualTo("execution(* *(..)) && args(String)");
}

@Test
public void testMultipleAndSubstitutions() {
Pointcut pc = getPointcut("execution(* *(..)) and args(String) and this(Object)");
PointcutExpression expr = ((AspectJExpressionPointcut) pc).getPointcutExpression();
assertThat(expr.getPointcutExpression()).isEqualTo("execution(* *(..)) && args(String) && this(Object)");
AspectJExpressionPointcut pc = getPointcut("execution(* *(..)) and args(String) and this(Object)");
String expr = pc.getPointcutExpression().getPointcutExpression();
assertThat(expr).isEqualTo("execution(* *(..)) && args(String) && this(Object)");
}

private Pointcut getPointcut(String expression) {
private AspectJExpressionPointcut getPointcut(String expression) {
AspectJExpressionPointcut pointcut = new AspectJExpressionPointcut();
pointcut.setExpression(expression);
return pointcut;
}


public static class OtherIOther implements IOther {

@Override
public void absquatulate() {
// Empty
}
}

@Test
public void testMatchGenericArgument() {
String expression = "execution(* set*(java.util.List<org.springframework.beans.testfixture.beans.TestBean>) )";
Expand Down Expand Up @@ -505,6 +491,15 @@ public void testAnnotationOnMethodArgumentsWithWildcards() throws Exception {
}


public static class OtherIOther implements IOther {

@Override
public void absquatulate() {
// Empty
}
}


public static class HasGeneric {

public void setFriends(List<TestBean> friends) {
Expand Down

0 comments on commit 0c65a5e

Please sign in to comment.