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.
@@ -20,6 +20,7 @@ import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 
@@ -29,7 +30,6 @@ import org.apache.commons.logging.LogFactory;
 import org.springframework.aop.framework.AopProxyUtils;
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.cache.Cache;
-import org.springframework.cache.Cache.ValueWrapper;
 import org.springframework.cache.CacheManager;
 import org.springframework.cache.support.SimpleValueWrapper;
 import org.springframework.expression.EvaluationContext;
@@ -64,6 +64,7 @@ import org.springframework.util.StringUtils;
  * @author Chris Beams
  * @author Phillip Webb
  * @author Sam Brannen
+ * @author Stephane Nicoll
  * @since 3.1
  */
 public abstract class CacheAspectSupport implements InitializingBean {
@@ -194,15 +195,20 @@ public abstract class CacheAspectSupport implements InitializingBean {
 		// Process any early evictions
 		processCacheEvicts(contexts.get(CacheEvictOperation.class), true, ExpressionEvaluator.NO_RESULT);
 
-		// Collect puts from any @Cachable miss
-		List<CachePutRequest> cachePutRequests = new ArrayList<CachePutRequest>();
-		collectPutRequests(contexts.get(CacheableOperation.class), ExpressionEvaluator.NO_RESULT, cachePutRequests, true);
+		// Check if we have a cached item matching the conditions
+		Cache.ValueWrapper cacheHit = findCachedItem(contexts.get(CacheableOperation.class));
 
-		ValueWrapper result = null;
+		// Collect puts from any @Cacheable miss, if no cached item is found
+		List<CachePutRequest> cachePutRequests = new LinkedList<CachePutRequest>();
+		if (cacheHit == null) {
+			collectPutRequests(contexts.get(CacheableOperation.class), ExpressionEvaluator.NO_RESULT, cachePutRequests);
+		}
+
+		Cache.ValueWrapper result = null;
 
-		// We only attempt to get a cached result if there are no put requests
+		// If there are no put requests, just use the cache hit
 		if (cachePutRequests.isEmpty() && contexts.get(CachePutOperation.class).isEmpty()) {
-			result = findCachedResult(contexts.get(CacheableOperation.class));
+			result = cacheHit;
 		}
 
 		// Invoke the method if don't have a cache hit
@@ -211,7 +217,7 @@ public abstract class CacheAspectSupport implements InitializingBean {
 		}
 
 		// Collect any explicit @CachePuts
-		collectPutRequests(contexts.get(CachePutOperation.class), result.get(), cachePutRequests, false);
+		collectPutRequests(contexts.get(CachePutOperation.class), result.get(), cachePutRequests);
 
 		// Process any collected put requests, either from @CachePut or a @Cacheable miss
 		for (CachePutRequest cachePutRequest : cachePutRequests) {
@@ -257,39 +263,42 @@ public abstract class CacheAspectSupport implements InitializingBean {
 		}
 	}
 
-	private void collectPutRequests(Collection<CacheOperationContext> contexts,
-			Object result, Collection<CachePutRequest> putRequests, boolean whenNotInCache) {
-
+	/**
+	 * Find a cached item only for {@link CacheableOperation} that passes the condition.
+	 * @param contexts the cacheable operations
+	 * @return a {@link Cache.ValueWrapper} holding the cached item,
+	 * or {@code null} if none is found
+	 */
+	private Cache.ValueWrapper findCachedItem(Collection<CacheOperationContext> contexts) {
+		Object result = ExpressionEvaluator.NO_RESULT;
 		for (CacheOperationContext context : contexts) {
 			if (isConditionPassing(context, result)) {
 				Object key = generateKey(context, result);
-				if (!whenNotInCache || findInAnyCaches(contexts, key) == null) {
-					putRequests.add(new CachePutRequest(context, key));
+				Cache.ValueWrapper cached = findInCaches(context, key);
+				if (cached != null) {
+					return cached;
 				}
 			}
 		}
+		return null;
 	}
 
-	private Cache.ValueWrapper findCachedResult(Collection<CacheOperationContext> contexts) {
-		ValueWrapper result = null;
-		for (CacheOperationContext context : contexts) {
-			if (isConditionPassing(context, ExpressionEvaluator.NO_RESULT)) {
-				if (result == null) {
-					result = findInCaches(context, generateKey(context, ExpressionEvaluator.NO_RESULT));
-				}
-			}
-		}
-		return result;
-	}
+	/**
+	 * Collect the {@link CachePutRequest} for all {@link CacheOperation} using
+	 * the specified result item.
+	 * @param contexts the contexts to handle
+	 * @param result the result item (never {@code null})
+	 * @param putRequests the collection to update
+	 */
+	private void collectPutRequests(Collection<CacheOperationContext> contexts,
+			Object result, Collection<CachePutRequest> putRequests) {
 
-	private Cache.ValueWrapper  findInAnyCaches(Collection<CacheOperationContext> contexts, Object key) {
 		for (CacheOperationContext context : contexts) {
-			ValueWrapper wrapper = findInCaches(context, key);
-			if (wrapper != null) {
-				return wrapper;
+			if (isConditionPassing(context, result)) {
+				Object key = generateKey(context, result);
+				putRequests.add(new CachePutRequest(context, key));
 			}
 		}
-		return null;
 	}
 
 	private Cache.ValueWrapper findInCaches(CacheOperationContext context, Object key) {

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,26 +16,32 @@
 
 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 {
 
@@ -59,6 +65,40 @@ public class CacheReproTests {
 		context.close();
 	}
 
+	@Test
+	public void spr11595GetSimple() {
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11595Config.class);
+		Spr11595Service bean = context.getBean(Spr11595Service.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 spr11595GetNeverCache(){
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11595Config.class);
+		Spr11595Service bean = context.getBean(Spr11595Service.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
@@ -100,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");
@@ -136,4 +176,43 @@ public class CacheReproTests {
 		}
 	}
 
+
+	@Configuration
+	@EnableCaching
+	public static class Spr11595Config {
+
+		@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 Spr11595Service service() {
+			return new Spr11595Service();
+		}
+
+	}
+
+
+	public static class Spr11595Service {
+
+		@Cacheable("cache")
+		public Object getSimple(String key) {
+			return new Object();
+		}
+
+		@Cacheable(value = "cache", condition = "false")
+		public Object getNeverCache(String key) {
+			return new Object();
+		}
+	}
+
 }