Skip to content

Commit 9fc13e1

Browse files
jhoellerjhoeller
committed
CacheAspectSupport checks Cache.get(key) once per invocation only
Issue: SPR-11592
1 parent f2f355e commit 9fc13e1

File tree

2 files changed

+122
-34
lines changed

2 files changed

+122
-34
lines changed

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

Lines changed: 39 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2013 the original author or authors.
2+
* Copyright 2002-2014 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
2020
import java.util.ArrayList;
2121
import java.util.Collection;
2222
import java.util.Collections;
23+
import java.util.LinkedList;
2324
import java.util.List;
2425
import java.util.Set;
2526

@@ -29,7 +30,6 @@
2930
import org.springframework.aop.framework.AopProxyUtils;
3031
import org.springframework.beans.factory.InitializingBean;
3132
import org.springframework.cache.Cache;
32-
import org.springframework.cache.Cache.ValueWrapper;
3333
import org.springframework.cache.CacheManager;
3434
import org.springframework.cache.support.SimpleValueWrapper;
3535
import org.springframework.expression.EvaluationContext;
@@ -64,6 +64,7 @@
6464
* @author Chris Beams
6565
* @author Phillip Webb
6666
* @author Sam Brannen
67+
* @author Stephane Nicoll
6768
* @since 3.1
6869
*/
6970
public abstract class CacheAspectSupport implements InitializingBean {
@@ -194,15 +195,20 @@ private Object execute(Invoker invoker, CacheOperationContexts contexts) {
194195
// Process any early evictions
195196
processCacheEvicts(contexts.get(CacheEvictOperation.class), true, ExpressionEvaluator.NO_RESULT);
196197

197-
// Collect puts from any @Cachable miss
198-
List<CachePutRequest> cachePutRequests = new ArrayList<CachePutRequest>();
199-
collectPutRequests(contexts.get(CacheableOperation.class), ExpressionEvaluator.NO_RESULT, cachePutRequests, true);
198+
// Check if we have a cached item matching the conditions
199+
Cache.ValueWrapper cacheHit = findCachedItem(contexts.get(CacheableOperation.class));
200200

201-
ValueWrapper result = null;
201+
// Collect puts from any @Cacheable miss, if no cached item is found
202+
List<CachePutRequest> cachePutRequests = new LinkedList<CachePutRequest>();
203+
if (cacheHit == null) {
204+
collectPutRequests(contexts.get(CacheableOperation.class), ExpressionEvaluator.NO_RESULT, cachePutRequests);
205+
}
206+
207+
Cache.ValueWrapper result = null;
202208

203-
// We only attempt to get a cached result if there are no put requests
209+
// If there are no put requests, just use the cache hit
204210
if (cachePutRequests.isEmpty() && contexts.get(CachePutOperation.class).isEmpty()) {
205-
result = findCachedResult(contexts.get(CacheableOperation.class));
211+
result = cacheHit;
206212
}
207213

208214
// Invoke the method if don't have a cache hit
@@ -211,7 +217,7 @@ private Object execute(Invoker invoker, CacheOperationContexts contexts) {
211217
}
212218

213219
// Collect any explicit @CachePuts
214-
collectPutRequests(contexts.get(CachePutOperation.class), result.get(), cachePutRequests, false);
220+
collectPutRequests(contexts.get(CachePutOperation.class), result.get(), cachePutRequests);
215221

216222
// Process any collected put requests, either from @CachePut or a @Cacheable miss
217223
for (CachePutRequest cachePutRequest : cachePutRequests) {
@@ -257,39 +263,42 @@ private void logInvalidating(CacheOperationContext context, CacheEvictOperation
257263
}
258264
}
259265

260-
private void collectPutRequests(Collection<CacheOperationContext> contexts,
261-
Object result, Collection<CachePutRequest> putRequests, boolean whenNotInCache) {
262-
266+
/**
267+
* Find a cached item only for {@link CacheableOperation} that passes the condition.
268+
* @param contexts the cacheable operations
269+
* @return a {@link Cache.ValueWrapper} holding the cached item,
270+
* or {@code null} if none is found
271+
*/
272+
private Cache.ValueWrapper findCachedItem(Collection<CacheOperationContext> contexts) {
273+
Object result = ExpressionEvaluator.NO_RESULT;
263274
for (CacheOperationContext context : contexts) {
264275
if (isConditionPassing(context, result)) {
265276
Object key = generateKey(context, result);
266-
if (!whenNotInCache || findInAnyCaches(contexts, key) == null) {
267-
putRequests.add(new CachePutRequest(context, key));
277+
Cache.ValueWrapper cached = findInCaches(context, key);
278+
if (cached != null) {
279+
return cached;
268280
}
269281
}
270282
}
283+
return null;
271284
}
272285

273-
private Cache.ValueWrapper findCachedResult(Collection<CacheOperationContext> contexts) {
274-
ValueWrapper result = null;
275-
for (CacheOperationContext context : contexts) {
276-
if (isConditionPassing(context, ExpressionEvaluator.NO_RESULT)) {
277-
if (result == null) {
278-
result = findInCaches(context, generateKey(context, ExpressionEvaluator.NO_RESULT));
279-
}
280-
}
281-
}
282-
return result;
283-
}
286+
/**
287+
* Collect the {@link CachePutRequest} for all {@link CacheOperation} using
288+
* the specified result item.
289+
* @param contexts the contexts to handle
290+
* @param result the result item (never {@code null})
291+
* @param putRequests the collection to update
292+
*/
293+
private void collectPutRequests(Collection<CacheOperationContext> contexts,
294+
Object result, Collection<CachePutRequest> putRequests) {
284295

285-
private Cache.ValueWrapper findInAnyCaches(Collection<CacheOperationContext> contexts, Object key) {
286296
for (CacheOperationContext context : contexts) {
287-
ValueWrapper wrapper = findInCaches(context, key);
288-
if (wrapper != null) {
289-
return wrapper;
297+
if (isConditionPassing(context, result)) {
298+
Object key = generateKey(context, result);
299+
putRequests.add(new CachePutRequest(context, key));
290300
}
291301
}
292-
return null;
293302
}
294303

295304
private Cache.ValueWrapper findInCaches(CacheOperationContext context, Object key) {

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

Lines changed: 83 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
/*
2-
* Copyright 2002-2013 the original author or authors.
2+
* Copyright 2002-2014 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
66
* You may obtain a copy of the License at
77
*
8-
* http://www.apache.org/licenses/LICENSE-2.0
8+
* http://www.apache.org/licenses/LICENSE-2.0
99
*
1010
* Unless required by applicable law or agreed to in writing, software
1111
* distributed under the License is distributed on an "AS IS" BASIS,
@@ -16,26 +16,32 @@
1616

1717
package org.springframework.cache;
1818

19+
import java.util.Arrays;
1920
import java.util.Collections;
2021
import java.util.List;
2122

2223
import org.junit.Test;
24+
import org.mockito.Mockito;
2325

2426
import org.springframework.cache.annotation.Cacheable;
2527
import org.springframework.cache.annotation.Caching;
2628
import org.springframework.cache.annotation.EnableCaching;
29+
import org.springframework.cache.concurrent.ConcurrentMapCache;
2730
import org.springframework.cache.concurrent.ConcurrentMapCacheManager;
31+
import org.springframework.cache.support.SimpleCacheManager;
2832
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
2933
import org.springframework.context.annotation.Bean;
3034
import org.springframework.context.annotation.Configuration;
3135

3236
import static org.junit.Assert.*;
37+
import static org.mockito.Mockito.*;
3338

3439
/**
3540
* Tests to reproduce raised caching issues.
3641
*
3742
* @author Phillip Webb
3843
* @author Juergen Hoeller
44+
* @author Stephane Nicoll
3945
*/
4046
public class CacheReproTests {
4147

@@ -59,6 +65,40 @@ public void spr11249() throws Exception {
5965
context.close();
6066
}
6167

68+
@Test
69+
public void spr11595GetSimple() {
70+
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11595Config.class);
71+
Spr11595Service bean = context.getBean(Spr11595Service.class);
72+
Cache cache = context.getBean("cache", Cache.class);
73+
74+
String key = "1";
75+
Object result = bean.getSimple("1");
76+
verify(cache, times(1)).get(key); // first call: cache miss
77+
78+
Object cachedResult = bean.getSimple("1");
79+
assertSame(result, cachedResult);
80+
verify(cache, times(2)).get(key); // second call: cache hit
81+
82+
context.close();
83+
}
84+
85+
@Test
86+
public void spr11595GetNeverCache(){
87+
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(Spr11595Config.class);
88+
Spr11595Service bean = context.getBean(Spr11595Service.class);
89+
Cache cache = context.getBean("cache", Cache.class);
90+
91+
String key = "1";
92+
Object result = bean.getNeverCache("1");
93+
verify(cache, times(0)).get(key); // no cache hit at all, caching disabled
94+
95+
Object cachedResult = bean.getNeverCache("1");
96+
assertNotSame(result, cachedResult);
97+
verify(cache, times(0)).get(key); // caching disabled
98+
99+
context.close();
100+
}
101+
62102

63103
@Configuration
64104
@EnableCaching
@@ -100,8 +140,8 @@ public List<String> single(int id) {
100140

101141
@Override
102142
@Caching(cacheable = {
103-
@Cacheable(value = "bigCache", unless = "#result.size() < 4"),
104-
@Cacheable(value = "smallCache", unless = "#result.size() > 3") })
143+
@Cacheable(value = "bigCache", unless = "#result.size() < 4"),
144+
@Cacheable(value = "smallCache", unless = "#result.size() > 3") })
105145
public List<String> multiple(int id) {
106146
if (this.multipleCount > 0) {
107147
fail("Called too many times");
@@ -136,4 +176,43 @@ public Object doSomething(String name, int... values) {
136176
}
137177
}
138178

179+
180+
@Configuration
181+
@EnableCaching
182+
public static class Spr11595Config {
183+
184+
@Bean
185+
public CacheManager cacheManager() {
186+
SimpleCacheManager cacheManager = new SimpleCacheManager();
187+
cacheManager.setCaches(Arrays.asList(cache()));
188+
return cacheManager;
189+
}
190+
191+
@Bean
192+
public Cache cache() {
193+
Cache cache = new ConcurrentMapCache("cache");
194+
return Mockito.spy(cache);
195+
}
196+
197+
@Bean
198+
public Spr11595Service service() {
199+
return new Spr11595Service();
200+
}
201+
202+
}
203+
204+
205+
public static class Spr11595Service {
206+
207+
@Cacheable("cache")
208+
public Object getSimple(String key) {
209+
return new Object();
210+
}
211+
212+
@Cacheable(value = "cache", condition = "false")
213+
public Object getNeverCache(String key) {
214+
return new Object();
215+
}
216+
}
217+
139218
}

0 commit comments

Comments
 (0)