From 87366f1e3268da70570cabe8fb7c3c39f398b7d6 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 1 Dec 2024 16:57:13 +0100 Subject: [PATCH] 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 --- .../org/springframework/aop/framework/AdvisedSupport.java | 3 ++- .../autoproxy/DefaultAdvisorAutoProxyCreatorTests.java | 7 +++++-- .../interceptor/BeanFactoryTransactionTests.java | 8 ++++---- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java b/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java index 009ff490f964..707c98f26760 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java @@ -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 diff --git a/spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java b/spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java index a9c723a04fa4..d84df387d56f 100644 --- a/spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java +++ b/spring-context/src/test/java/org/springframework/aop/framework/autoproxy/DefaultAdvisorAutoProxyCreatorTests.java @@ -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(); } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java index 0fe3bef54a6a..727369f4d757 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/BeanFactoryTransactionTests.java @@ -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); }