Skip to content

Commit 8d69370

Browse files
committed
Consider logical equality in AdvisedSupport.MethodCacheKey#equals
Prior to this commit, the equals() implementation in AdvisedSupport's MethodCacheKey only considered methods to be equal based on an identity comparison (`==`), which led to duplicate entries in the method cache for the same logical method. This is caused by the fact that AdvisedSupport's getInterceptorsAndDynamicInterceptionAdvice() method is invoked at various stages with different Method instances for the same method: 1) when creating the proxy 2) when invoking the method via the proxy The reason the Method instances are different is due to the following. - Methods such as Class#getDeclaredMethods() and Class#getDeclaredMethod() always returns "child copies" of the underlying Method instances -- which means that `equals()` should be used instead of (or in addition to) `==` whenever the compared Method instances can come from different sources. With this commit, the equals() implementation in MethodCacheKey now considers methods equal based on identity or logical equality, giving preference to the quicker identity check. See gh-32586 Closes gh-33915
1 parent d990449 commit 8d69370

File tree

3 files changed

+11
-7
lines changed

3 files changed

+11
-7
lines changed

spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,8 @@ public MethodCacheKey(Method method) {
668668

669669
@Override
670670
public boolean equals(@Nullable Object other) {
671-
return (this == other || (other instanceof MethodCacheKey that && this.method == that.method));
671+
return (this == other || (other instanceof MethodCacheKey that &&
672+
(this.method == that.method || this.method.equals(that.method))));
672673
}
673674

674675
@Override

spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java

+5-2
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,18 @@ class DefaultAdvisorAutoProxyCreatorTests {
4343
* @see StaticMethodMatcherPointcut#matches(Method, Class)
4444
*/
4545
@Test // gh-33915
46-
void staticMethodMatcherPointcutMatchesMethodIsInvokedAgainForActualMethodInvocation() {
46+
void staticMethodMatcherPointcutMatchesMethodIsNotInvokedAgainForActualMethodInvocation() {
4747
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
4848
DemoBean.class, DemoPointcutAdvisor.class, DefaultAdvisorAutoProxyCreator.class);
4949
DemoPointcutAdvisor demoPointcutAdvisor = context.getBean(DemoPointcutAdvisor.class);
5050
DemoBean demoBean = context.getBean(DemoBean.class);
5151

5252
assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations before").isEqualTo(2);
53+
// Invoke multiple times to ensure additional invocations don't affect the outcome.
5354
assertThat(demoBean.sayHello()).isEqualTo("Advised: Hello!");
54-
assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations after").isEqualTo(3);
55+
assertThat(demoBean.sayHello()).isEqualTo("Advised: Hello!");
56+
assertThat(demoBean.sayHello()).isEqualTo("Advised: Hello!");
57+
assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations after").isEqualTo(2);
5558

5659
context.close();
5760
}

spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -139,10 +139,10 @@ void getsAreNotTransactionalWithProxyFactory3() {
139139
// Invokes: getAge() * 2 and setAge() * 1 --> 2 + 1 = 3 method invocations.
140140
assertGetsAreNotTransactional(testBean);
141141

142-
// The transaction pointcut is currently asked if it matches() for all method
143-
// invocations, but we cannot assert it's equal to 3 since the pointcut may be
144-
// optimized and only invoked once.
145-
assertThat(txnPointcut.counter).as("txnPointcut").isGreaterThanOrEqualTo(1).isLessThanOrEqualTo(3);
142+
// The matches(Method, Class) method of the static transaction pointcut should not
143+
// have been invoked for the actual invocation of the getAge() and setAge() methods.
144+
assertThat(txnPointcut.counter).as("txnPointcut").isZero();
145+
146146
assertThat(preInterceptor.counter).as("preInterceptor").isEqualTo(3);
147147
assertThat(postInterceptor.counter).as("postInterceptor").isEqualTo(3);
148148
}

0 commit comments

Comments
 (0)