From 5aefcc802ef05abc51bbfbeb4a78b3032ff9eee3 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 15 Oct 2014 14:19:28 +0200 Subject: [PATCH] Prevent early bean initialization with @EnableCaching Prior to this commmit, any configuration class holding a CacheManager bean would be eagerly instantiated. This is because the CacheConfiguration infrastructure requests all beans of type CacheManager. This commit defers the resolution of the CacheManager as late as possible. Issue: SPR-12336 --- .../DefaultJCacheOperationSource.java | 41 ++++++++----- .../interceptor/JCacheInterceptorTests.java | 5 +- .../AbstractCachingConfiguration.java | 57 ++++--------------- .../cache/interceptor/CacheAspectSupport.java | 22 +++++-- .../cache/config/EnableCachingTests.java | 6 +- 5 files changed, 64 insertions(+), 67 deletions(-) diff --git a/spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/DefaultJCacheOperationSource.java b/spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/DefaultJCacheOperationSource.java index bd31dbc52bf..aaeec8ec8b9 100644 --- a/spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/DefaultJCacheOperationSource.java +++ b/spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/DefaultJCacheOperationSource.java @@ -21,6 +21,7 @@ import java.util.Map; import org.springframework.beans.BeanUtils; import org.springframework.beans.factory.BeanFactoryUtils; import org.springframework.beans.factory.InitializingBean; +import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.cache.CacheManager; import org.springframework.cache.interceptor.CacheResolver; import org.springframework.cache.interceptor.KeyGenerator; @@ -39,7 +40,7 @@ import org.springframework.util.Assert; * @since 4.1 */ public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSource - implements InitializingBean, ApplicationContextAware { + implements InitializingBean, SmartInitializingSingleton, ApplicationContextAware { private CacheManager cacheManager; @@ -83,7 +84,7 @@ public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSourc } public CacheResolver getCacheResolver() { - return this.cacheResolver; + return getDefaultCacheResolver(); } /** @@ -95,7 +96,7 @@ public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSourc } public CacheResolver getExceptionCacheResolver() { - return this.exceptionCacheResolver; + return getDefaultExceptionCacheResolver(); } @Override @@ -105,17 +106,13 @@ public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSourc @Override public void afterPropertiesSet() { - Assert.state((this.cacheResolver != null && this.exceptionCacheResolver != null) - || this.cacheManager != null, "'cacheManager' is required if cache resolvers are not set."); - Assert.state(this.applicationContext != null, "The application context was not injected as it should."); - this.adaptedKeyGenerator = new KeyGeneratorAdapter(this, this.keyGenerator); - if (this.cacheResolver == null) { - this.cacheResolver = new SimpleCacheResolver(this.cacheManager); - } - if (this.exceptionCacheResolver == null) { - this.exceptionCacheResolver = new SimpleExceptionCacheResolver(this.cacheManager); - } + } + + @Override + public void afterSingletonsInstantiated() { // Make sure those are initialized on startup + Assert.notNull(getDefaultCacheResolver(), "Cache resolver should have been initialized."); + Assert.notNull(getDefaultExceptionCacheResolver(), "Exception cache resolver should have been initialized."); } @Override @@ -131,11 +128,17 @@ public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSourc @Override protected CacheResolver getDefaultCacheResolver() { + if (this.cacheResolver == null) { + this.cacheResolver = new SimpleCacheResolver(getCacheManager()); + } return this.cacheResolver; } @Override protected CacheResolver getDefaultExceptionCacheResolver() { + if (this.exceptionCacheResolver == null) { + this.exceptionCacheResolver = new SimpleExceptionCacheResolver(getCacheManager()); + } return this.exceptionCacheResolver; } @@ -144,4 +147,16 @@ public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSourc return this.adaptedKeyGenerator; } + private CacheManager getCacheManager() { + if (this.cacheManager == null) { + this.cacheManager = this.applicationContext.getBean(CacheManager.class); + if (this.cacheManager == null) { + throw new IllegalStateException("No bean of type CacheManager could be found. " + + "Register a CacheManager bean or remove the @EnableCaching annotation " + + "from your configuration."); + } + } + return this.cacheManager; + } + } diff --git a/spring-context-support/src/test/java/org/springframework/cache/jcache/interceptor/JCacheInterceptorTests.java b/spring-context-support/src/test/java/org/springframework/cache/jcache/interceptor/JCacheInterceptorTests.java index d45e99002a5..aadfa9cba27 100644 --- a/spring-context-support/src/test/java/org/springframework/cache/jcache/interceptor/JCacheInterceptorTests.java +++ b/spring-context-support/src/test/java/org/springframework/cache/jcache/interceptor/JCacheInterceptorTests.java @@ -20,6 +20,7 @@ import java.lang.reflect.Method; import org.junit.Test; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.cache.CacheManager; import org.springframework.cache.interceptor.CacheOperationInvoker; import org.springframework.cache.interceptor.CacheResolver; @@ -80,8 +81,7 @@ public class JCacheInterceptorTests extends AbstractJCacheTests { @Test public void cacheManagerMandatoryIfCacheResolverNotSetSet() { - thrown.expect(IllegalStateException.class); - thrown.expectMessage("'cacheManager' is required"); + thrown.expect(NoSuchBeanDefinitionException.class); createOperationSource(null, null, null, defaultKeyGenerator); } @@ -117,6 +117,7 @@ public class JCacheInterceptorTests extends AbstractJCacheTests { source.setExceptionCacheResolver(exceptionCacheResolver); source.setKeyGenerator(keyGenerator); source.afterPropertiesSet(); + source.afterSingletonsInstantiated(); return source; } diff --git a/spring-context/src/main/java/org/springframework/cache/annotation/AbstractCachingConfiguration.java b/spring-context/src/main/java/org/springframework/cache/annotation/AbstractCachingConfiguration.java index 8d25623dc57..3e7847ef1c5 100644 --- a/spring-context/src/main/java/org/springframework/cache/annotation/AbstractCachingConfiguration.java +++ b/spring-context/src/main/java/org/springframework/cache/annotation/AbstractCachingConfiguration.java @@ -53,61 +53,28 @@ public abstract class AbstractCachingConfiguration protected CacheErrorHandler errorHandler; - @Autowired(required=false) - private Collection cacheManagerBeans; - - @Autowired(required=false) - private Collection cachingConfigurers; - - @Override public void setImportMetadata(AnnotationMetadata importMetadata) { this.enableCaching = AnnotationAttributes.fromMap( importMetadata.getAnnotationAttributes(EnableCaching.class.getName(), false)); Assert.notNull(this.enableCaching, "@EnableCaching is not present on importing class " + - importMetadata.getClassName()); + importMetadata.getClassName()); } - - /** - * Determine which {@code CacheManager} bean to use. Prefer the result of - * {@link CachingConfigurer#cacheManager()} over any by-type matching. If none, fall - * back to by-type matching on {@code CacheManager}. - * @throws IllegalArgumentException if no CacheManager can be found; if more than one - * CachingConfigurer implementation exists; if multiple CacheManager beans and no - * CachingConfigurer exists to disambiguate. - */ - @PostConstruct - protected void reconcileCacheManager() { - if (!CollectionUtils.isEmpty(cachingConfigurers)) { - int nConfigurers = cachingConfigurers.size(); - if (nConfigurers > 1) { - throw new IllegalStateException(nConfigurers + " implementations of " + - "CachingConfigurer were found when only 1 was expected. " + - "Refactor the configuration such that CachingConfigurer is " + - "implemented only once or not at all."); - } - C cachingConfigurer = cachingConfigurers.iterator().next(); - useCachingConfigurer(cachingConfigurer); + @Autowired(required = false) + void setConfigurers(Collection configurers) { + if (CollectionUtils.isEmpty(configurers)) { + return; } - if (this.cacheManager == null && !CollectionUtils.isEmpty(cacheManagerBeans)) { - int nManagers = cacheManagerBeans.size(); - if (nManagers > 1) { - throw new IllegalStateException(nManagers + " beans of type CacheManager " + - "were found when only 1 was expected. Remove all but one of the " + - "CacheManager bean definitions, or implement CachingConfigurer " + - "to make explicit which CacheManager should be used for " + - "annotation-driven cache management."); - } - this.cacheManager = cacheManagerBeans.iterator().next(); - // keyGenerator remains null; will fall back to default within CacheInterceptor - } - if (this.cacheManager == null) { - throw new IllegalStateException("No bean of type CacheManager could be found. " + - "Register a CacheManager bean or remove the @EnableCaching annotation " + - "from your configuration."); + if (configurers.size() > 1) { + throw new IllegalStateException(configurers.size() + " implementations of " + + "CachingConfigurer were found when only 1 was expected. " + + "Refactor the configuration such that CachingConfigurer is " + + "implemented only once or not at all."); } + C configurer = configurers.iterator().next(); + useCachingConfigurer(configurer); } /** diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java index 2e450ab6618..cb4f5779e7e 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java +++ b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java @@ -30,6 +30,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.aop.framework.AopProxyUtils; import org.springframework.beans.factory.InitializingBean; +import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.beans.factory.annotation.BeanFactoryAnnotationUtils; import org.springframework.cache.Cache; import org.springframework.cache.CacheManager; @@ -72,7 +73,7 @@ import org.springframework.util.StringUtils; * @since 3.1 */ public abstract class CacheAspectSupport extends AbstractCacheInvoker - implements InitializingBean, ApplicationContextAware { + implements InitializingBean, SmartInitializingSingleton, ApplicationContextAware { protected final Log logger = LogFactory.getLog(getClass()); @@ -167,15 +168,26 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker } public void afterPropertiesSet() { - Assert.state(this.cacheResolver != null, "'cacheResolver' is required. Either set the cache resolver " + - "to use or set the cache manager to create a default cache resolver based on it."); Assert.state(this.cacheOperationSource != null, "The 'cacheOperationSources' property is required: " + "If there are no cacheable methods, then don't use a cache aspect."); Assert.state(this.getErrorHandler() != null, "The 'errorHandler' is required."); - Assert.state(this.applicationContext != null, "The application context was not injected as it should."); - this.initialized = true; } + @Override + public void afterSingletonsInstantiated() { + if (getCacheResolver() == null) { // lazy initialize cache resolver + CacheManager cacheManager = this.applicationContext.getBean(CacheManager.class); + if (cacheManager == null) { + throw new IllegalStateException("No bean of type CacheManager could be found. " + + "Register a CacheManager bean or remove the @EnableCaching annotation " + + "from your configuration."); + } + setCacheManager(cacheManager); + } + Assert.state(this.cacheResolver != null, "'cacheResolver' is required. Either set the cache resolver " + + "to use or set the cache manager to create a default cache resolver based on it."); + this.initialized = true; + } /** * Convenience method to return a String representation of this Method diff --git a/spring-context/src/test/java/org/springframework/cache/config/EnableCachingTests.java b/spring-context/src/test/java/org/springframework/cache/config/EnableCachingTests.java index b7974f610b3..e3c1a6e01ef 100644 --- a/spring-context/src/test/java/org/springframework/cache/config/EnableCachingTests.java +++ b/spring-context/src/test/java/org/springframework/cache/config/EnableCachingTests.java @@ -19,6 +19,8 @@ package org.springframework.cache.config; import org.junit.Test; import org.springframework.beans.factory.BeanCreationException; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; +import org.springframework.beans.factory.NoUniqueBeanDefinitionException; import org.springframework.cache.CacheManager; import org.springframework.cache.CacheTestUtils; import org.springframework.cache.annotation.CachingConfigurerSupport; @@ -73,7 +75,7 @@ public class EnableCachingTests extends AbstractAnnotationTests { ctx.refresh(); } - @Test(expected=IllegalStateException.class) + @Test(expected=NoUniqueBeanDefinitionException.class) public void multipleCacheManagerBeans() throws Throwable { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); ctx.register(MultiCacheManagerConfig.class); @@ -106,7 +108,7 @@ public class EnableCachingTests extends AbstractAnnotationTests { } } - @Test(expected=IllegalStateException.class) + @Test(expected=NoSuchBeanDefinitionException.class) public void noCacheManagerBeans() throws Throwable { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); ctx.register(EmptyConfig.class);