From ef73ba93c87a83e481a5c9918440c5d162c4cfa9 Mon Sep 17 00:00:00 2001
From: Yanming Zhou <zhouyanming@gmail.com>
Date: Wed, 24 May 2023 12:23:32 +0800
Subject: [PATCH] Always fall back to original method if annotation pointcut
 used

Prior to this commit, AspectJExpressionPointcut doesn't fall back to original method if `!@annotation()` is used, it can cause false positive result.

Fix GH-27119
---
 .../aspectj/AspectJExpressionPointcut.java    | 10 +++++--
 .../AspectJExpressionPointcutTests.java       | 28 ++++++++++++++++++-
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java
index 10ed76efd52c..68f3883aebc4 100644
--- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java
+++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java
@@ -78,6 +78,7 @@
  * @author Juergen Hoeller
  * @author Ramnivas Laddad
  * @author Dave Syer
+ * @author Yanming Zhou
  * @since 2.0
  */
 @SuppressWarnings("serial")
@@ -470,8 +471,7 @@ private ShadowMatch getShadowMatch(Method targetMethod, Method originalMethod) {
 								fallbackExpression = null;
 							}
 						}
-						if (targetMethod != originalMethod && (shadowMatch == null ||
-								(shadowMatch.neverMatches() && Proxy.isProxyClass(targetMethod.getDeclaringClass())))) {
+						if (targetMethod != originalMethod && (shadowMatch == null || shouldFallback(targetMethod))) {
 							// Fall back to the plain original method in case of no resolvable match or a
 							// negative match on a proxy class (which doesn't carry any annotations on its
 							// redeclared methods).
@@ -513,6 +513,12 @@ else if (shadowMatch.maybeMatches() && fallbackExpression != null) {
 		return shadowMatch;
 	}
 
+	private boolean shouldFallback(Method targetMethod) {
+		if (!Proxy.isProxyClass(targetMethod.getDeclaringClass())) {
+			return false;
+		}
+		return this.resolveExpression().contains("@annotation");
+	}
 
 	@Override
 	public boolean equals(@Nullable Object other) {
diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/AspectJExpressionPointcutTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/AspectJExpressionPointcutTests.java
index e1b2a93b9589..27aac8409128 100644
--- a/spring-aop/src/test/java/org/springframework/aop/aspectj/AspectJExpressionPointcutTests.java
+++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/AspectJExpressionPointcutTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 the original author or authors.
+ * Copyright 2002-2023 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 @@
  * @author Rob Harrop
  * @author Rod Johnson
  * @author Chris Beams
+ * @author Yanming Zhou
  */
 public class AspectJExpressionPointcutTests {
 
@@ -424,6 +425,19 @@ public void testAnnotationOnCglibProxyMethod() throws Exception {
 		assertThat(ajexp.matches(BeanA.class.getMethod("getAge"), proxy.getClass())).isTrue();
 	}
 
+
+	@Test
+	public void testNotAnnotationOnCglibProxyMethod() throws Exception {
+		String expression = "!@annotation(test.annotation.transaction.Tx)";
+		AspectJExpressionPointcut ajexp = new AspectJExpressionPointcut();
+		ajexp.setExpression(expression);
+
+		ProxyFactory factory = new ProxyFactory(new BeanA());
+		factory.setProxyTargetClass(true);
+		BeanA proxy = (BeanA) factory.getProxy();
+		assertThat(ajexp.matches(BeanA.class.getMethod("getAge"), proxy.getClass())).isFalse();
+	}
+
 	@Test
 	public void testAnnotationOnDynamicProxyMethod() throws Exception {
 		String expression = "@annotation(test.annotation.transaction.Tx)";
@@ -436,6 +450,18 @@ public void testAnnotationOnDynamicProxyMethod() throws Exception {
 		assertThat(ajexp.matches(IBeanA.class.getMethod("getAge"), proxy.getClass())).isTrue();
 	}
 
+	@Test
+	public void testNotAnnotationOnDynamicProxyMethod() throws Exception {
+		String expression = "!@annotation(test.annotation.transaction.Tx)";
+		AspectJExpressionPointcut ajexp = new AspectJExpressionPointcut();
+		ajexp.setExpression(expression);
+
+		ProxyFactory factory = new ProxyFactory(new BeanA());
+		factory.setProxyTargetClass(false);
+		IBeanA proxy = (IBeanA) factory.getProxy();
+		assertThat(ajexp.matches(IBeanA.class.getMethod("getAge"), proxy.getClass())).isFalse();
+	}
+
 	@Test
 	public void testAnnotationOnMethodWithWildcard() throws Exception {
 		String expression = "execution(@(test.annotation..*) * *(..))";