Skip to content

Commit

Permalink
Consider logical equality in AdvisedSupport.MethodCacheKey#equals
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sbrannen committed Dec 4, 2024
1 parent d990449 commit 87366f1
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,8 @@ public MethodCacheKey(Method method) {

@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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,10 @@ void getsAreNotTransactionalWithProxyFactory3() {
// 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);
}
Expand Down

0 comments on commit 87366f1

Please sign in to comment.