Revised retrieval of cache strategy beans

Issue: SPR-12336

Author: jhoeller

spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/AbstractCacheInterceptor.java

@@ -28,6 +28,7 @@
 import org.springframework.cache.interceptor.CacheErrorHandler;
 import org.springframework.cache.interceptor.CacheOperationInvocationContext;
 import org.springframework.cache.interceptor.CacheOperationInvoker;
+import org.springframework.util.CollectionUtils;
 
 /**
  * A base interceptor for JSR-107 cache annotations.
@@ -41,12 +42,15 @@ abstract class AbstractCacheInterceptor<O extends AbstractJCacheOperation<A>, A
 
 	protected final Log logger = LogFactory.getLog(getClass());
 
+
 	protected AbstractCacheInterceptor(CacheErrorHandler errorHandler) {
 		super(errorHandler);
 	}
 
-	protected abstract Object invoke(CacheOperationInvocationContext<O> context,
-			CacheOperationInvoker invoker) throws Throwable;
+
+	protected abstract Object invoke(CacheOperationInvocationContext<O> context, CacheOperationInvoker invoker)
+			throws Throwable;
+
 
 	/**
 	 * Resolve the cache to use.
@@ -68,15 +72,15 @@ protected Cache resolveCache(CacheOperationInvocationContext<O> context) {
 	 * @return the singe element or {@code null} if the collection is empty
 	 */
 	static Cache extractFrom(Collection<? extends Cache> caches) {
-		if (caches == null || caches.size() == 0) {
+		if (CollectionUtils.isEmpty(caches)) {
 			return null;
 		}
 		else if (caches.size() == 1) {
 			return caches.iterator().next();
 		}
 		else {
-			throw new IllegalStateException("Unsupported cache resolution result "
-					+ caches + " JSR-107 only supports a single cache.");
+			throw new IllegalStateException("Unsupported cache resolution result " + caches +
+					": JSR-107 only supports a single cache.");
 		}
 	}
 

spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/AnnotationJCacheOperationSource.java

@@ -43,44 +43,22 @@
  */
 public abstract class AnnotationJCacheOperationSource extends AbstractFallbackJCacheOperationSource {
 
-	/**
-	 * Locate or create an instance of the specified {@code type}.
-	 * @param type the type of the bean to manage
-	 * @return the required bean
-	 */
-	protected abstract <T> T getBean(Class<T> type);
-
-	/**
-	 * Return the default {@link CacheResolver} if none is set.
-	 */
-	protected abstract CacheResolver getDefaultCacheResolver();
-
-	/**
-	 * Return the default exception {@link CacheResolver} if none is set.
-	 */
-	protected abstract CacheResolver getDefaultExceptionCacheResolver();
-
-	/**
-	 * Return the default {@link KeyGenerator} if none is set.
-	 */
-	protected abstract KeyGenerator getDefaultKeyGenerator();
-
-
 	@Override
 	protected JCacheOperation<?> findCacheOperation(Method method, Class<?> targetType) {
 		CacheResult cacheResult = method.getAnnotation(CacheResult.class);
 		CachePut cachePut = method.getAnnotation(CachePut.class);
 		CacheRemove cacheRemove = method.getAnnotation(CacheRemove.class);
 		CacheRemoveAll cacheRemoveAll = method.getAnnotation(CacheRemoveAll.class);
+
 		int found = countNonNull(cacheResult, cachePut, cacheRemove, cacheRemoveAll);
 		if (found == 0) {
 			return null;
 		}
 		if (found > 1) {
 			throw new IllegalStateException("More than one cache annotation found on '" + method + "'");
 		}
-		CacheDefaults defaults = getCacheDefaults(method, targetType);
 
+		CacheDefaults defaults = getCacheDefaults(method, targetType);
 		if (cacheResult != null) {
 			return createCacheResultOperation(method, defaults, cacheResult);
 		}
@@ -103,9 +81,7 @@ protected CacheDefaults getCacheDefaults(Method method, Class<?> targetType) {
 		return targetType.getAnnotation(CacheDefaults.class);
 	}
 
-
-	protected CacheResultOperation createCacheResultOperation(Method method, CacheDefaults defaults,
-			CacheResult ann) {
+	protected CacheResultOperation createCacheResultOperation(Method method, CacheDefaults defaults, CacheResult ann) {
 		String cacheName = determineCacheName(method, defaults, ann.cacheName());
 		CacheResolverFactory cacheResolverFactory =
 				determineCacheResolverFactory(defaults, ann.cacheResolverFactory());
@@ -123,49 +99,39 @@ protected CacheResultOperation createCacheResultOperation(Method method, CacheDe
 		return new CacheResultOperation(methodDetails, cacheResolver, keyGenerator, exceptionCacheResolver);
 	}
 
-	protected CachePutOperation createCachePutOperation(Method method, CacheDefaults defaults,
-			CachePut ann) {
+	protected CachePutOperation createCachePutOperation(Method method, CacheDefaults defaults, CachePut ann) {
 		String cacheName = determineCacheName(method, defaults, ann.cacheName());
 		CacheResolverFactory cacheResolverFactory =
 				determineCacheResolverFactory(defaults, ann.cacheResolverFactory());
 		KeyGenerator keyGenerator = determineKeyGenerator(defaults, ann.cacheKeyGenerator());
 
 		CacheMethodDetails<CachePut> methodDetails = createMethodDetails(method, ann, cacheName);
-
 		CacheResolver cacheResolver = getCacheResolver(cacheResolverFactory, methodDetails);
-
 		return new CachePutOperation(methodDetails, cacheResolver, keyGenerator);
 	}
 
-	protected CacheRemoveOperation createCacheRemoveOperation(Method method, CacheDefaults defaults,
-			CacheRemove ann) {
+	protected CacheRemoveOperation createCacheRemoveOperation(Method method, CacheDefaults defaults, CacheRemove ann) {
 		String cacheName = determineCacheName(method, defaults, ann.cacheName());
 		CacheResolverFactory cacheResolverFactory =
 				determineCacheResolverFactory(defaults, ann.cacheResolverFactory());
 		KeyGenerator keyGenerator = determineKeyGenerator(defaults, ann.cacheKeyGenerator());
 
 		CacheMethodDetails<CacheRemove> methodDetails = createMethodDetails(method, ann, cacheName);
-
 		CacheResolver cacheResolver = getCacheResolver(cacheResolverFactory, methodDetails);
-
 		return new CacheRemoveOperation(methodDetails, cacheResolver, keyGenerator);
 	}
 
-	protected CacheRemoveAllOperation createCacheRemoveAllOperation(Method method, CacheDefaults defaults,
-			CacheRemoveAll ann) {
+	protected CacheRemoveAllOperation createCacheRemoveAllOperation(Method method, CacheDefaults defaults, CacheRemoveAll ann) {
 		String cacheName = determineCacheName(method, defaults, ann.cacheName());
 		CacheResolverFactory cacheResolverFactory =
 				determineCacheResolverFactory(defaults, ann.cacheResolverFactory());
 
 		CacheMethodDetails<CacheRemoveAll> methodDetails = createMethodDetails(method, ann, cacheName);
-
 		CacheResolver cacheResolver = getCacheResolver(cacheResolverFactory, methodDetails);
-
 		return new CacheRemoveAllOperation(methodDetails, cacheResolver);
 	}
 
-	private <A extends Annotation> CacheMethodDetails<A> createMethodDetails(
-			Method method, A annotation, String cacheName) {
+	private <A extends Annotation> CacheMethodDetails<A> createMethodDetails(Method method, A annotation, String cacheName) {
 		return new DefaultCacheMethodDetails<A>(method, annotation, cacheName);
 	}
 
@@ -181,6 +147,7 @@ protected CacheResolver getCacheResolver(CacheResolverFactory factory, CacheMeth
 
 	protected CacheResolver getExceptionCacheResolver(CacheResolverFactory factory,
 			CacheMethodDetails<CacheResult> details) {
+
 		if (factory != null) {
 			javax.cache.annotation.CacheResolver cacheResolver = factory.getExceptionCacheResolver(details);
 			return new CacheResolverAdapter(cacheResolver);
@@ -192,6 +159,7 @@ protected CacheResolver getExceptionCacheResolver(CacheResolverFactory factory,
 
 	protected CacheResolverFactory determineCacheResolverFactory(CacheDefaults defaults,
 			Class<? extends CacheResolverFactory> candidate) {
+
 		if (!CacheResolverFactory.class.equals(candidate)) {
 			return getBean(candidate);
 		}
@@ -203,8 +171,7 @@ else if (defaults != null && !CacheResolverFactory.class.equals(defaults.cacheRe
 		}
 	}
 
-	protected KeyGenerator determineKeyGenerator(CacheDefaults defaults,
-			Class<? extends CacheKeyGenerator> candidate) {
+	protected KeyGenerator determineKeyGenerator(CacheDefaults defaults, Class<? extends CacheKeyGenerator> candidate) {
 		if (!CacheKeyGenerator.class.equals(candidate)) {
 			return new KeyGeneratorAdapter(this, getBean(candidate));
 		}
@@ -233,28 +200,48 @@ protected String determineCacheName(Method method, CacheDefaults defaults, Strin
 	 */
 	protected String generateDefaultCacheName(Method method) {
 		Class<?>[] parameterTypes = method.getParameterTypes();
-		List<String> parameters = new ArrayList<String>();
+		List<String> parameters = new ArrayList<String>(parameterTypes.length);
 		for (Class<?> parameterType : parameterTypes) {
 			parameters.add(parameterType.getName());
 		}
 
-		StringBuilder sb = new StringBuilder();
-		sb.append(method.getDeclaringClass().getName())
-				.append(".")
-				.append(method.getName())
-				.append("(")
-				.append(StringUtils.collectionToCommaDelimitedString(parameters))
-				.append(")");
+		StringBuilder sb = new StringBuilder(method.getDeclaringClass().getName());
+		sb.append(".").append(method.getName());
+		sb.append("(").append(StringUtils.collectionToCommaDelimitedString(parameters)).append(")");
 		return sb.toString();
 	}
 
 	private int countNonNull(Object... instances) {
 		int result = 0;
-		for (Object o : instances) {
-			if (o != null) {
+		for (Object instance : instances) {
+			if (instance != null) {
 				result += 1;
 			}
 		}
 		return result;
 	}
+
+
+	/**
+	 * Locate or create an instance of the specified cache strategy {@code type}.
+	 * @param type the type of the bean to manage
+	 * @return the required bean
+	 */
+	protected abstract <T> T getBean(Class<T> type);
+
+	/**
+	 * Return the default {@link CacheResolver} if none is set.
+	 */
+	protected abstract CacheResolver getDefaultCacheResolver();
+
+	/**
+	 * Return the default exception {@link CacheResolver} if none is set.
+	 */
+	protected abstract CacheResolver getDefaultExceptionCacheResolver();
+
+	/**
+	 * Return the default {@link KeyGenerator} if none is set.
+	 */
+	protected abstract KeyGenerator getDefaultKeyGenerator();
+
 }

spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/DefaultJCacheOperationSource.java

@@ -16,11 +16,10 @@
 
 package org.springframework.cache.jcache.interceptor;
 
-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.NoSuchBeanDefinitionException;
+import org.springframework.beans.factory.NoUniqueBeanDefinitionException;
 import org.springframework.beans.factory.SmartInitializingSingleton;
 import org.springframework.cache.CacheManager;
 import org.springframework.cache.interceptor.CacheResolver;
@@ -40,7 +39,7 @@
  * @since 4.1
  */
 public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSource
-		implements InitializingBean, SmartInitializingSingleton, ApplicationContextAware {
+		implements ApplicationContextAware, InitializingBean, SmartInitializingSingleton {
 
 	private CacheManager cacheManager;
 
@@ -54,6 +53,7 @@ public class DefaultJCacheOperationSource extends AnnotationJCacheOperationSourc
 
 	private ApplicationContext applicationContext;
 
+
 	/**
 	 * Set the default {@link CacheManager} to use to lookup cache by name. Only mandatory
 	 * if the {@linkplain CacheResolver cache resolvers} have not been set.
@@ -104,24 +104,33 @@ public void setApplicationContext(ApplicationContext applicationContext) {
 		this.applicationContext = applicationContext;
 	}
 
+
 	@Override
 	public void afterPropertiesSet() {
 		this.adaptedKeyGenerator = new KeyGeneratorAdapter(this, this.keyGenerator);
 	}
 
 	@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.");
+	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
 	protected <T> T getBean(Class<T> type) {
-		Map<String, T> map = BeanFactoryUtils.beansOfTypeIncludingAncestors(this.applicationContext, type);
-		if (map.size() == 1) {
-			return map.values().iterator().next();
+		try {
+			return this.applicationContext.getBean(type);
 		}
-		else {
+		catch (NoUniqueBeanDefinitionException ex) {
+			throw new IllegalStateException("No unique [" + type.getName() + "] bean found in application context - " +
+					"mark one as primary, or declare a more specific implementation type for your cache", ex);
+		}
+		catch (NoSuchBeanDefinitionException ex) {
+			if (logger.isDebugEnabled()) {
+				logger.debug("No bean of type [" + type.getName() + "] found in application context", ex);
+			}
 			return BeanUtils.instantiateClass(type);
 		}
 	}
@@ -149,11 +158,16 @@ protected KeyGenerator getDefaultKeyGenerator() {
 
 	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.");
+			try {
+				this.cacheManager = this.applicationContext.getBean(CacheManager.class);
+			}
+			catch (NoUniqueBeanDefinitionException ex) {
+				throw new IllegalStateException("No unique bean of type CacheManager found. "+
+						"Mark one as primary or declare a specific CacheManager to use.");
+			}
+			catch (NoSuchBeanDefinitionException ex) {
+				throw new IllegalStateException("No bean of type CacheManager found. Register a CacheManager "+
+						"bean or remove the @EnableCaching annotation from your configuration.");
 			}
 		}
 		return this.cacheManager;

spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/JCacheAspectSupport.java

@@ -78,9 +78,9 @@ public JCacheOperationSource getCacheOperationSource() {
 	}
 
 	public void afterPropertiesSet() {
-		Assert.state(this.cacheOperationSource != null, "The 'cacheOperationSource' property is required: " +
+		Assert.state(getCacheOperationSource() != null, "The 'cacheOperationSource' 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(getErrorHandler() != null, "The 'errorHandler' is required");
 
 		this.cacheResultInterceptor = new CacheResultInterceptor(getErrorHandler());
 		this.cachePutInterceptor = new CachePutInterceptor(getErrorHandler());
@@ -92,8 +92,7 @@ public void afterPropertiesSet() {
 
 
 	protected Object execute(CacheOperationInvoker invoker, Object target, Method method, Object[] args) {
-		// check whether aspect is enabled
-		// to cope with cases where the AJ is pulled in automatically
+		// Check whether aspect is enabled to cope with cases where the AJ is pulled in automatically
 		if (this.initialized) {
 			Class<?> targetClass = getTargetClass(target);
 			JCacheOperation<?> operation = getCacheOperationSource().getCacheOperation(method, targetClass);
@@ -108,9 +107,9 @@ protected Object execute(CacheOperationInvoker invoker, Object target, Method me
 	}
 
 	@SuppressWarnings("unchecked")
-	private CacheOperationInvocationContext<?> createCacheOperationInvocationContext(Object target,
-			Object[] args,
-			JCacheOperation<?> operation) {
+	private CacheOperationInvocationContext<?> createCacheOperationInvocationContext(
+			Object target, Object[] args, JCacheOperation<?> operation) {
+
 		return new DefaultCacheInvocationContext<Annotation>(
 				(JCacheOperation<Annotation>) operation, target, args);
 	}
@@ -124,12 +123,10 @@ private Class<?> getTargetClass(Object target) {
 	}
 
 	@SuppressWarnings("unchecked")
-	private Object execute(CacheOperationInvocationContext<?> context,
-			CacheOperationInvoker invoker) {
-
+	private Object execute(CacheOperationInvocationContext<?> context, CacheOperationInvoker invoker) {
 		CacheOperationInvoker adapter = new CacheOperationInvokerAdapter(invoker);
-
 		BasicOperation operation = context.getOperation();
+
 		if (operation instanceof CacheResultOperation) {
 			return cacheResultInterceptor.invoke(
 					(CacheOperationInvocationContext<CacheResultOperation>) context, adapter);