CacheAspectSupport checks Cache.get(key) once per invocation only

Issue: SPR-11592

spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -103,7 +103,7 @@ public abstract class CacheAspectSupport implements InitializingBean {
 	 * @param cacheOperationSources must not be {@code null}
 	 */
 	public void setCacheOperationSources(CacheOperationSource... cacheOperationSources) {
-		Assert.notEmpty(cacheOperationSources);
+		Assert.notEmpty(cacheOperationSources, "At least 1 CacheOperationSource needs to be specified");
 		this.cacheOperationSource = (cacheOperationSources.length > 1 ?
 				new CompositeCacheOperationSource(cacheOperationSources) : cacheOperationSources[0]);
 	}
@@ -132,10 +132,10 @@ public abstract class CacheAspectSupport implements InitializingBean {
 
 	public void afterPropertiesSet() {
 		if (this.cacheManager == null) {
-			throw new IllegalStateException("'cacheManager' is required");
+			throw new IllegalStateException("Property 'cacheManager' is required");
 		}
 		if (this.cacheOperationSource == null) {
-			throw new IllegalStateException("The 'cacheOperationSources' property is required: " +
+			throw new IllegalStateException("Property 'cacheOperationSources' is required: " +
 					"If there are no cacheable methods, then don't use a cache aspect.");
 		}
 

spring-context/src/test/java/org/springframework/cache/CacheReproTests.java

@@ -1,11 +1,11 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 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.
  * You may obtain a copy of the License at
  *
- * http://www.apache.org/licenses/LICENSE-2.0
+ *      http://www.apache.org/licenses/LICENSE-2.0
  *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
@@ -16,31 +16,38 @@
 
 package org.springframework.cache;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
 import org.junit.Test;
+import org.mockito.Mockito;
+
 import org.springframework.cache.annotation.Cacheable;
 import org.springframework.cache.annotation.Caching;
 import org.springframework.cache.annotation.EnableCaching;
+import org.springframework.cache.concurrent.ConcurrentMapCache;
 import org.springframework.cache.concurrent.ConcurrentMapCacheManager;
+import org.springframework.cache.support.SimpleCacheManager;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 
 import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
 
 /**
  * Tests to reproduce raised caching issues.
  *
  * @author Phillip Webb
+ * @author Juergen Hoeller
+ * @author Stephane Nicoll
  */
 public class CacheReproTests {
 
 	@Test
 	public void spr11124() throws Exception {
-		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
-				Spr11124Config.class);
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11124Config.class);
 		Spr11124Service bean = context.getBean(Spr11124Service.class);
 		bean.single(2);
 		bean.single(2);
@@ -49,6 +56,50 @@ public class CacheReproTests {
 		context.close();
 	}
 
+	@Test
+	public void spr11249() throws Exception {
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11249Config.class);
+		Spr11249Service bean = context.getBean(Spr11249Service.class);
+		Object result = bean.doSomething("op", 2, 3);
+		assertSame(result, bean.doSomething("op", 2, 3));
+		context.close();
+	}
+
+	@Test
+	public void spr11592GetSimple() {
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11592Config.class);
+		Spr11592Service bean = context.getBean(Spr11592Service.class);
+		Cache cache = context.getBean("cache", Cache.class);
+
+		String key = "1";
+		Object result = bean.getSimple("1");
+		verify(cache, times(1)).get(key); // first call: cache miss
+
+		Object cachedResult = bean.getSimple("1");
+		assertSame(result, cachedResult);
+		verify(cache, times(2)).get(key); // second call: cache hit
+
+		context.close();
+	}
+
+	@Test
+	public void spr11592GetNeverCache(){
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11592Config.class);
+		Spr11592Service bean = context.getBean(Spr11592Service.class);
+		Cache cache = context.getBean("cache", Cache.class);
+
+		String key = "1";
+		Object result = bean.getNeverCache("1");
+		verify(cache, times(0)).get(key); // no cache hit at all, caching disabled
+
+		Object cachedResult = bean.getNeverCache("1");
+		assertNotSame(result, cachedResult);
+		verify(cache, times(0)).get(key); // caching disabled
+
+		context.close();
+	}
+
+
 	@Configuration
 	@EnableCaching
 	public static class Spr11124Config {
@@ -62,23 +113,23 @@ public class CacheReproTests {
 		public Spr11124Service service() {
 			return new Spr11124ServiceImpl();
 		}
-
 	}
 
+
 	public interface Spr11124Service {
 
 		public List<String> single(int id);
 
 		public List<String> multiple(int id);
-
 	}
 
+
 	public static class Spr11124ServiceImpl implements Spr11124Service {
 
 		private int multipleCount = 0;
 
 		@Override
-		@Cacheable(value = "smallCache")
+		@Cacheable("smallCache")
 		public List<String> single(int id) {
 			if (this.multipleCount > 0) {
 				fail("Called too many times");
@@ -89,8 +140,8 @@ public class CacheReproTests {
 
 		@Override
 		@Caching(cacheable = {
-			@Cacheable(value = "bigCache", unless = "#result.size() < 4"),
-			@Cacheable(value = "smallCache", unless = "#result.size() > 3") })
+				@Cacheable(value = "bigCache", unless = "#result.size() < 4"),
+				@Cacheable(value = "smallCache", unless = "#result.size() > 3") })
 		public List<String> multiple(int id) {
 			if (this.multipleCount > 0) {
 				fail("Called too many times");
@@ -98,7 +149,69 @@ public class CacheReproTests {
 			this.multipleCount++;
 			return Collections.emptyList();
 		}
+	}
 
+
+	@Configuration
+	@EnableCaching
+	public static class Spr11249Config {
+
+		@Bean
+		public CacheManager cacheManager() {
+			return new ConcurrentMapCacheManager();
+		}
+
+		@Bean
+		public Spr11249Service service() {
+			return new Spr11249Service();
+		}
+	}
+
+
+	public static class Spr11249Service {
+
+		@Cacheable("smallCache")
+		public Object doSomething(String name, int... values) {
+			return new Object();
+		}
+	}
+
+
+	@Configuration
+	@EnableCaching
+	public static class Spr11592Config {
+
+		@Bean
+		public CacheManager cacheManager() {
+			SimpleCacheManager cacheManager = new SimpleCacheManager();
+			cacheManager.setCaches(Arrays.asList(cache()));
+			return cacheManager;
+		}
+
+		@Bean
+		public Cache cache() {
+			Cache cache = new ConcurrentMapCache("cache");
+			return Mockito.spy(cache);
+		}
+
+		@Bean
+		public Spr11592Service service() {
+			return new Spr11592Service();
+		}
+	}
+
+
+	public static class Spr11592Service {
+
+		@Cacheable("cache")
+		public Object getSimple(String key) {
+			return new Object();
+		}
+
+		@Cacheable(value = "cache", condition = "false")
+		public Object getNeverCache(String key) {
+			return new Object();
+		}
 	}
 
 }