From 865fa33927f1ab3923cc7c7bd6b799fde877983f Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 18 Sep 2023 18:22:08 +0200 Subject: [PATCH] Cache CGLIB proxy classes properly again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The introduction of AdvisedSupport.AdvisorKeyEntry in Spring Framework 6.0.10 resulted in a regression regarding caching of CGLIB generated proxy classes. Specifically, equality checks for the proxy class cache became based partially on identity rather than equivalence. For example, if an ApplicationContext was configured to create a class-based @Transactional proxy, a second attempt to create the ApplicationContext resulted in a duplicate proxy class for the same @Transactional component. On the JVM this went unnoticed; however, when running Spring integration tests within a native image, if a test made use of @⁠DirtiesContext, a second attempt to create the test ApplicationContext resulted in an exception stating, "CGLIB runtime enhancement not supported on native image." This is because Test AOT processing only refreshes a test ApplicationContext once, and the duplicate CGLIB proxy classes are only requested in subsequent refreshes of the same ApplicationContext which means that duplicate proxy classes are not tracked during AOT processing and consequently not included in a native image. This commit addresses this regression as follows. - AdvisedSupport.AdvisorKeyEntry is now based on the toString() representations of the ClassFilter and MethodMatcher in the corresponding Pointcut instead of the filter's and matcher's identities. - Due to the above changes to AdvisorKeyEntry, ClassFilter and MethodMatcher implementations are now required to implement equals(), hashCode(), AND toString(). - Consequently, the following now include proper equals(), hashCode(), and toString() implementations. - CacheOperationSourcePointcut - TransactionAttributeSourcePointcut - PerTargetInstantiationModelPointcut Closes gh-31238 --- .../org/springframework/aop/ClassFilter.java | 13 ++++-- .../springframework/aop/MethodMatcher.java | 13 ++++-- ...ntiationModelAwarePointcutAdvisorImpl.java | 19 ++++++++ .../aop/framework/AdvisedSupport.java | 5 ++- .../CacheOperationSourcePointcut.java | 22 ++++++++++ .../AspectJAutoProxyCreatorTests.java | 44 +++++++++++++++++++ .../config/EnableCachingIntegrationTests.java | 23 +++++++++- .../TransactionAttributeSourcePointcut.java | 26 ++++++++++- .../EnableTransactionManagementTests.java | 21 ++++++++- 9 files changed, 172 insertions(+), 14 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/ClassFilter.java b/spring-aop/src/main/java/org/springframework/aop/ClassFilter.java index de20021fd6..7926be708e 100644 --- a/spring-aop/src/main/java/org/springframework/aop/ClassFilter.java +++ b/spring-aop/src/main/java/org/springframework/aop/ClassFilter.java @@ -23,12 +23,17 @@ package org.springframework.aop; *

Can be used as part of a {@link Pointcut} or for the entire targeting of * an {@link IntroductionAdvisor}. * - *

Concrete implementations of this interface typically should provide proper - * implementations of {@link Object#equals(Object)} and {@link Object#hashCode()} - * in order to allow the filter to be used in caching scenarios — for - * example, in proxies generated by CGLIB. + *

WARNING: Concrete implementations of this interface must + * provide proper implementations of {@link Object#equals(Object)}, + * {@link Object#hashCode()}, and {@link Object#toString()} in order to allow the + * filter to be used in caching scenarios — for example, in proxies generated + * by CGLIB. As of Spring Framework 6.0.13, the {@code toString()} implementation + * must generate a unique string representation that aligns with the logic used + * to implement {@code equals()}. See concrete implementations of this interface + * within the framework for examples. * * @author Rod Johnson + * @author Sam Brannen * @see Pointcut * @see MethodMatcher */ diff --git a/spring-aop/src/main/java/org/springframework/aop/MethodMatcher.java b/spring-aop/src/main/java/org/springframework/aop/MethodMatcher.java index 71f19e97dd..9e04831a01 100644 --- a/spring-aop/src/main/java/org/springframework/aop/MethodMatcher.java +++ b/spring-aop/src/main/java/org/springframework/aop/MethodMatcher.java @@ -41,12 +41,17 @@ import java.lang.reflect.Method; * state changes they have produced in parameters or {@code ThreadLocal} state will * be available at the time of evaluation. * - *

Concrete implementations of this interface typically should provide proper - * implementations of {@link Object#equals(Object)} and {@link Object#hashCode()} - * in order to allow the matcher to be used in caching scenarios — for - * example, in proxies generated by CGLIB. + *

WARNING: Concrete implementations of this interface must + * provide proper implementations of {@link Object#equals(Object)}, + * {@link Object#hashCode()}, and {@link Object#toString()} in order to allow the + * matcher to be used in caching scenarios — for example, in proxies generated + * by CGLIB. As of Spring Framework 6.0.13, the {@code toString()} implementation + * must generate a unique string representation that aligns with the logic used + * to implement {@code equals()}. See concrete implementations of this interface + * within the framework for examples. * * @author Rod Johnson + * @author Sam Brannen * @since 11.11.2003 * @see Pointcut * @see ClassFilter diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/InstantiationModelAwarePointcutAdvisorImpl.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/InstantiationModelAwarePointcutAdvisorImpl.java index 74a2e1b3c4..f4d150d64d 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/InstantiationModelAwarePointcutAdvisorImpl.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/InstantiationModelAwarePointcutAdvisorImpl.java @@ -32,6 +32,7 @@ import org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactory. import org.springframework.aop.support.DynamicMethodMatcherPointcut; import org.springframework.aop.support.Pointcuts; import org.springframework.lang.Nullable; +import org.springframework.util.ObjectUtils; /** * Internal implementation of AspectJPointcutAdvisor. @@ -40,6 +41,7 @@ import org.springframework.lang.Nullable; * * @author Rod Johnson * @author Juergen Hoeller + * @author Sam Brannen * @since 2.0 */ @SuppressWarnings("serial") @@ -297,6 +299,23 @@ final class InstantiationModelAwarePointcutAdvisorImpl private boolean isAspectMaterialized() { return (this.aspectInstanceFactory == null || this.aspectInstanceFactory.isMaterialized()); } + + @Override + public boolean equals(@Nullable Object other) { + return (this == other || (other instanceof PerTargetInstantiationModelPointcut that && + ObjectUtils.nullSafeEquals(this.declaredPointcut.getExpression(), that.declaredPointcut.getExpression()))); + } + + @Override + public int hashCode() { + return ObjectUtils.nullSafeHashCode(this.declaredPointcut.getExpression()); + } + + @Override + public String toString() { + return PerTargetInstantiationModelPointcut.class.getName() + ": " + this.declaredPointcut.getExpression(); + } + } } 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 6020cbbd99..41c5b1d21c 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 @@ -62,6 +62,7 @@ import org.springframework.util.ObjectUtils; * * @author Rod Johnson * @author Juergen Hoeller + * @author Sam Brannen * @see org.springframework.aop.framework.AopProxy */ public class AdvisedSupport extends ProxyConfig implements Advised { @@ -653,8 +654,8 @@ public class AdvisedSupport extends ProxyConfig implements Advised { this.adviceType = advisor.getAdvice().getClass(); if (advisor instanceof PointcutAdvisor pointcutAdvisor) { Pointcut pointcut = pointcutAdvisor.getPointcut(); - this.classFilterKey = ObjectUtils.identityToString(pointcut.getClassFilter()); - this.methodMatcherKey = ObjectUtils.identityToString(pointcut.getMethodMatcher()); + this.classFilterKey = pointcut.getClassFilter().toString(); + this.methodMatcherKey = pointcut.getMethodMatcher().toString(); } else { this.classFilterKey = null; diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheOperationSourcePointcut.java b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheOperationSourcePointcut.java index e7988d5e12..e70275aeae 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheOperationSourcePointcut.java +++ b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheOperationSourcePointcut.java @@ -32,6 +32,7 @@ import org.springframework.util.ObjectUtils; * * @author Costin Leau * @author Juergen Hoeller + * @author Sam Brannen * @since 3.1 */ @SuppressWarnings("serial") @@ -86,6 +87,27 @@ class CacheOperationSourcePointcut extends StaticMethodMatcherPointcut implement } return (cacheOperationSource == null || cacheOperationSource.isCandidateClass(clazz)); } + + private CacheOperationSource getCacheOperationSource() { + return cacheOperationSource; + } + + @Override + public boolean equals(@Nullable Object other) { + return (this == other || (other instanceof CacheOperationSourceClassFilter that && + ObjectUtils.nullSafeEquals(cacheOperationSource, that.getCacheOperationSource()))); + } + + @Override + public int hashCode() { + return CacheOperationSourceClassFilter.class.hashCode(); + } + + @Override + public String toString() { + return CacheOperationSourceClassFilter.class.getName() + ": " + cacheOperationSource; + } + } } diff --git a/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java b/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java index 8a7e0b5011..69f5f4bd88 100644 --- a/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java +++ b/spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java @@ -53,12 +53,14 @@ import org.springframework.beans.factory.config.MethodInvokingFactoryBean; import org.springframework.beans.factory.support.RootBeanDefinition; import org.springframework.beans.testfixture.beans.ITestBean; import org.springframework.beans.testfixture.beans.TestBean; +import org.springframework.cglib.proxy.Factory; import org.springframework.context.ApplicationContext; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.EnableAspectJAutoProxy; +import org.springframework.context.annotation.Scope; import org.springframework.context.support.ClassPathXmlApplicationContext; import org.springframework.context.support.GenericApplicationContext; import org.springframework.core.DecoratingProxy; @@ -208,6 +210,31 @@ class AspectJAutoProxyCreatorTests { assertThat(adrian1.getAge()).isEqualTo(3); } + @Test // gh-31238 + void cglibProxyClassIsCachedAcrossApplicationContextsForPerTargetAspect() { + Class configClass = PerTargetProxyTargetClassTrueConfig.class; + TestBean testBean1; + TestBean testBean2; + + // Round #1 + try (ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(configClass)) { + testBean1 = context.getBean(TestBean.class); + assertThat(AopUtils.isCglibProxy(testBean1)).as("CGLIB proxy").isTrue(); + assertThat(testBean1.getClass().getInterfaces()) + .containsExactlyInAnyOrder(Factory.class, SpringProxy.class, Advised.class); + } + + // Round #2 + try (ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(configClass)) { + testBean2 = context.getBean(TestBean.class); + assertThat(AopUtils.isCglibProxy(testBean2)).as("CGLIB proxy").isTrue(); + assertThat(testBean2.getClass().getInterfaces()) + .containsExactlyInAnyOrder(Factory.class, SpringProxy.class, Advised.class); + } + + assertThat(testBean1.getClass()).isSameAs(testBean2.getClass()); + } + @Test void twoAdviceAspect() { ClassPathXmlApplicationContext bf = newContext("twoAdviceAspect.xml"); @@ -615,6 +642,23 @@ class ProxyTargetClassFalseConfig extends AbstractProxyTargetClassConfig { class ProxyTargetClassTrueConfig extends AbstractProxyTargetClassConfig { } +@Configuration +@EnableAspectJAutoProxy(proxyTargetClass = true) +class PerTargetProxyTargetClassTrueConfig { + + @Bean + @Scope("prototype") + TestBean testBean() { + return new TestBean("Jane", 34); + } + + @Bean + @Scope("prototype") + PerTargetAspect perTargetAspect() { + return new PerTargetAspect(); + } +} + @FunctionalInterface interface MessageGenerator { String generateMessage(); diff --git a/spring-context/src/test/java/org/springframework/cache/config/EnableCachingIntegrationTests.java b/spring-context/src/test/java/org/springframework/cache/config/EnableCachingIntegrationTests.java index 9d371d23a1..f4e25b70ca 100644 --- a/spring-context/src/test/java/org/springframework/cache/config/EnableCachingIntegrationTests.java +++ b/spring-context/src/test/java/org/springframework/cache/config/EnableCachingIntegrationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 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. @@ -21,6 +21,7 @@ import java.util.concurrent.atomic.AtomicLong; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.cache.Cache; import org.springframework.cache.CacheManager; @@ -45,6 +46,7 @@ import static org.springframework.context.testfixture.cache.CacheTestUtils.asser * Tests that represent real use cases with advanced configuration. * * @author Stephane Nicoll + * @author Sam Brannen */ class EnableCachingIntegrationTests { @@ -83,6 +85,25 @@ class EnableCachingIntegrationTests { assertCacheHit(key, value, cache); } + @Test // gh-31238 + public void cglibProxyClassIsCachedAcrossApplicationContexts() { + ConfigurableApplicationContext ctx; + + // Round #1 + ctx = new AnnotationConfigApplicationContext(FooConfigCglib.class); + FooService service1 = ctx.getBean(FooService.class); + assertThat(AopUtils.isCglibProxy(service1)).as("FooService #1 is not a CGLIB proxy").isTrue(); + ctx.close(); + + // Round #2 + ctx = new AnnotationConfigApplicationContext(FooConfigCglib.class); + FooService service2 = ctx.getBean(FooService.class); + assertThat(AopUtils.isCglibProxy(service2)).as("FooService #2 is not a CGLIB proxy").isTrue(); + ctx.close(); + + assertThat(service1.getClass()).isSameAs(service2.getClass()); + } + @Test void barServiceWithCacheableInterfaceCglib() { this.context = new AnnotationConfigApplicationContext(BarConfigCglib.class); diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAttributeSourcePointcut.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAttributeSourcePointcut.java index 9917ab7b2e..10ac08147a 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAttributeSourcePointcut.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAttributeSourcePointcut.java @@ -27,14 +27,15 @@ import org.springframework.transaction.TransactionManager; import org.springframework.util.ObjectUtils; /** - * Abstract class that implements a {@code Pointcut} that matches if the underlying + * Internal class that implements a {@code Pointcut} that matches if the underlying * {@link TransactionAttributeSource} has an attribute for a given method. * * @author Juergen Hoeller + * @author Sam Brannen * @since 2.5.5 */ @SuppressWarnings("serial") -class TransactionAttributeSourcePointcut extends StaticMethodMatcherPointcut implements Serializable { +final class TransactionAttributeSourcePointcut extends StaticMethodMatcherPointcut implements Serializable { @Nullable private TransactionAttributeSource transactionAttributeSource; @@ -87,6 +88,27 @@ class TransactionAttributeSourcePointcut extends StaticMethodMatcherPointcut imp } return (transactionAttributeSource == null || transactionAttributeSource.isCandidateClass(clazz)); } + + private TransactionAttributeSource getTransactionAttributeSource() { + return transactionAttributeSource; + } + + @Override + public boolean equals(@Nullable Object other) { + return (this == other || (other instanceof TransactionAttributeSourceClassFilter that && + ObjectUtils.nullSafeEquals(transactionAttributeSource, that.getTransactionAttributeSource()))); + } + + @Override + public int hashCode() { + return TransactionAttributeSourceClassFilter.class.hashCode(); + } + + @Override + public String toString() { + return TransactionAttributeSourceClassFilter.class.getName() + ": " + transactionAttributeSource; + } + } } diff --git a/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.java b/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.java index 8f33660d36..b9d70ee6a6 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.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. @@ -69,6 +69,25 @@ public class EnableTransactionManagementTests { ctx.close(); } + @Test // gh-31238 + public void cglibProxyClassIsCachedAcrossApplicationContexts() { + ConfigurableApplicationContext ctx; + + // Round #1 + ctx = new AnnotationConfigApplicationContext(EnableTxConfig.class, TxManagerConfig.class); + TransactionalTestBean bean1 = ctx.getBean(TransactionalTestBean.class); + assertThat(AopUtils.isCglibProxy(bean1)).as("testBean #1 is not a CGLIB proxy").isTrue(); + ctx.close(); + + // Round #2 + ctx = new AnnotationConfigApplicationContext(EnableTxConfig.class, TxManagerConfig.class); + TransactionalTestBean bean2 = ctx.getBean(TransactionalTestBean.class); + assertThat(AopUtils.isCglibProxy(bean2)).as("testBean #2 is not a CGLIB proxy").isTrue(); + ctx.close(); + + assertThat(bean1.getClass()).isSameAs(bean2.getClass()); + } + @Test public void transactionProxyIsCreatedWithEnableOnSuperclass() { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(