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
This commit is contained in:
Sam Brannen 2024-12-01 16:57:13 +01:00
parent d990449b0d
commit 8d69370b95
3 changed files with 11 additions and 7 deletions

View File

@ -668,7 +668,8 @@ public class AdvisedSupport extends ProxyConfig implements Advised {
@Override
public boolean equals(@Nullable Object other) {
return (this == other || (other instanceof MethodCacheKey that && this.method == that.method));
return (this == other || (other instanceof MethodCacheKey that &&
(this.method == that.method || this.method.equals(that.method))));
}
@Override

View File

@ -43,15 +43,18 @@ class DefaultAdvisorAutoProxyCreatorTests {
* @see StaticMethodMatcherPointcut#matches(Method, Class)
*/
@Test // gh-33915
void staticMethodMatcherPointcutMatchesMethodIsInvokedAgainForActualMethodInvocation() {
void staticMethodMatcherPointcutMatchesMethodIsNotInvokedAgainForActualMethodInvocation() {
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
DemoBean.class, DemoPointcutAdvisor.class, DefaultAdvisorAutoProxyCreator.class);
DemoPointcutAdvisor demoPointcutAdvisor = context.getBean(DemoPointcutAdvisor.class);
DemoBean demoBean = context.getBean(DemoBean.class);
assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations before").isEqualTo(2);
// Invoke multiple times to ensure additional invocations don't affect the outcome.
assertThat(demoBean.sayHello()).isEqualTo("Advised: Hello!");
assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations after").isEqualTo(3);
assertThat(demoBean.sayHello()).isEqualTo("Advised: Hello!");
assertThat(demoBean.sayHello()).isEqualTo("Advised: Hello!");
assertThat(demoPointcutAdvisor.matchesInvocationCount).as("matches() invocations after").isEqualTo(2);
context.close();
}

View File

@ -139,10 +139,10 @@ class BeanFactoryTransactionTests {
// Invokes: getAge() * 2 and setAge() * 1 --> 2 + 1 = 3 method invocations.
assertGetsAreNotTransactional(testBean);
// The transaction pointcut is currently asked if it matches() for all method
// invocations, but we cannot assert it's equal to 3 since the pointcut may be
// optimized and only invoked once.
assertThat(txnPointcut.counter).as("txnPointcut").isGreaterThanOrEqualTo(1).isLessThanOrEqualTo(3);
// The matches(Method, Class) method of the static transaction pointcut should not
// have been invoked for the actual invocation of the getAge() and setAge() methods.
assertThat(txnPointcut.counter).as("txnPointcut").isZero();
assertThat(preInterceptor.counter).as("preInterceptor").isEqualTo(3);
assertThat(postInterceptor.counter).as("postInterceptor").isEqualTo(3);
}