From daf12a972306aa3e2e104be9666529882ec7db5b Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 3 Jun 2015 11:42:49 +0200 Subject: [PATCH] Allow Cache annotations to not specify any cache name Since Spring 4.1, a CacheResolver may be configured to customize the way the cache(s) to use for a given cache operation are retrieved. Since a CacheResolver implementation may not use the cache names information at all, this attribute has been made optional. However, a fix was still applied, preventing a Cache operation without a cache name to be defined properly. We now allow this valid use case. Issue: SPR-13081 --- .../SpringCacheAnnotationParser.java | 5 -- .../cache/CacheReproTests.java | 82 ++++++++++++++++++- .../AnnotationCacheOperationSourceTests.java | 9 +- 3 files changed, 85 insertions(+), 11 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/cache/annotation/SpringCacheAnnotationParser.java b/spring-context/src/main/java/org/springframework/cache/annotation/SpringCacheAnnotationParser.java index 325b23720a7..1c7faa431cd 100644 --- a/spring-context/src/main/java/org/springframework/cache/annotation/SpringCacheAnnotationParser.java +++ b/spring-context/src/main/java/org/springframework/cache/annotation/SpringCacheAnnotationParser.java @@ -235,11 +235,6 @@ public class SpringCacheAnnotationParser implements CacheAnnotationParser, Seria "default cache resolver if none is set. If a cache resolver is set, the cache manager" + "won't be used."); } - if (operation.getCacheNames().isEmpty()) { - throw new IllegalStateException("No cache names could be detected on '" + - ae.toString() + "'. Make sure to set the value parameter on the annotation or " + - "declare a @CacheConfig at the class-level with the default cache name(s) to use."); - } } @Override diff --git a/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java b/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java index d60c14c170b..0fa2501ff8f 100644 --- a/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java +++ b/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java @@ -17,17 +17,24 @@ package org.springframework.cache; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.List; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.mockito.Mockito; import org.springframework.cache.annotation.Cacheable; import org.springframework.cache.annotation.Caching; +import org.springframework.cache.annotation.CachingConfigurerSupport; import org.springframework.cache.annotation.EnableCaching; import org.springframework.cache.concurrent.ConcurrentMapCache; import org.springframework.cache.concurrent.ConcurrentMapCacheManager; +import org.springframework.cache.interceptor.AbstractCacheResolver; +import org.springframework.cache.interceptor.CacheOperationInvocationContext; +import org.springframework.cache.interceptor.CacheResolver; import org.springframework.cache.support.SimpleCacheManager; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; @@ -45,6 +52,9 @@ import static org.mockito.Mockito.*; */ public class CacheReproTests { + @Rule + public final ExpectedException thrown = ExpectedException.none(); + @Test public void spr11124() throws Exception { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11124Config.class); @@ -83,7 +93,7 @@ public class CacheReproTests { } @Test - public void spr11592GetNeverCache(){ + public void spr11592GetNeverCache() { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11592Config.class); Spr11592Service bean = context.getBean(Spr11592Service.class); Cache cache = context.getBean("cache", Cache.class); @@ -99,6 +109,27 @@ public class CacheReproTests { context.close(); } + @Test + public void spr13081ConfigNoCacheNameIsRequired() { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr13081Config.class); + MyCacheResolver cacheResolver = context.getBean(MyCacheResolver.class); + Spr13081Service bean = context.getBean(Spr13081Service.class); + assertNull(cacheResolver.getCache("foo").get("foo")); + Object result = bean.getSimple("foo"); // cache name = id + assertEquals(result, cacheResolver.getCache("foo").get("foo").get()); + } + + @Test + public void spr13081ConfigFailIfCacheResolverReturnsNullCacheName() { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr13081Config.class); + Spr13081Service bean = context.getBean(Spr13081Service.class); + + + thrown.expect(IllegalStateException.class); + thrown.expectMessage(MyCacheResolver.class.getName()); + bean.getSimple(null); + } + @Configuration @EnableCaching @@ -118,9 +149,9 @@ public class CacheReproTests { public interface Spr11124Service { - public List single(int id); + List single(int id); - public List multiple(int id); + List multiple(int id); } @@ -214,4 +245,49 @@ public class CacheReproTests { } } + @Configuration + @EnableCaching + public static class Spr13081Config extends CachingConfigurerSupport { + + @Bean + @Override + public CacheResolver cacheResolver() { + return new MyCacheResolver(); + } + + @Bean + public Spr13081Service service() { + return new Spr13081Service(); + } + + } + + public static class MyCacheResolver extends AbstractCacheResolver { + + public MyCacheResolver() { + super(new ConcurrentMapCacheManager()); + } + + @Override + protected Collection getCacheNames(CacheOperationInvocationContext context) { + String cacheName = (String) context.getArgs()[0]; + if (cacheName != null) { + return Collections.singleton(cacheName); + } + return null; + } + + public Cache getCache(String name) { + return getCacheManager().getCache(name); + } + } + + public static class Spr13081Service { + + @Cacheable + public Object getSimple(String cacheName) { + return new Object(); + } + } + } diff --git a/spring-context/src/test/java/org/springframework/cache/annotation/AnnotationCacheOperationSourceTests.java b/spring-context/src/test/java/org/springframework/cache/annotation/AnnotationCacheOperationSourceTests.java index 710b3f5b370..0629181cbcb 100644 --- a/spring-context/src/test/java/org/springframework/cache/annotation/AnnotationCacheOperationSourceTests.java +++ b/spring-context/src/test/java/org/springframework/cache/annotation/AnnotationCacheOperationSourceTests.java @@ -191,9 +191,12 @@ public class AnnotationCacheOperationSourceTests { } @Test - public void validateAtLeastOneCacheNameMustBeSet() { - thrown.expect(IllegalStateException.class); - getOps(AnnotatedClass.class, "noCacheNameSpecified"); + public void validateNoCacheIsValid() { + // Valid as a CacheResolver might return the cache names to use with other info + Collection ops = getOps(AnnotatedClass.class, "noCacheNameSpecified"); + CacheOperation cacheOperation = ops.iterator().next(); + assertNotNull("cache names set must not be null", cacheOperation.getCacheNames()); + assertEquals("no cache names specified", 0, cacheOperation.getCacheNames().size()); } @Test