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 2c2bdb4f6a8..d1ae54b63dd 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 @@ -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 cachePutRequests = new ArrayList(); - 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 cachePutRequests = new LinkedList(); + 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 contexts, - Object result, Collection 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 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 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 contexts, + Object result, Collection putRequests) { - private Cache.ValueWrapper findInAnyCaches(Collection 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) { 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 d7242c4e764..256a3f73f49 100644 --- a/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java +++ b/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 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(); + } + } + }