From 73a8a1b9667fe4554bfe6500fb48931caa45278c Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 26 Nov 2013 17:13:50 -0800 Subject: [PATCH] Detect cache hit with multiple @Cachables Fix CacheAspectSupport to consider a cache hit from any of the multiple @Cachables that may have been specified using the @Caching annotation. Prior to this commit the following scenario would never produce a hit: @Caching(cacheable = { @Cacheable(value = "c1", unless = "#result.size() < 4"), @Cacheable(value = "c2", unless = "#result.size() > 3") }) Issue: SPR-11124 --- .../cache/interceptor/CacheAspectSupport.java | 12 +- .../cache/CacheReproTests.java | 104 ++++++++++++++++++ 2 files changed, 108 insertions(+), 8 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/cache/CacheReproTests.java 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 85346ed741..53b1d4b5b6 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 @@ -269,7 +269,7 @@ public abstract class CacheAspectSupport implements InitializingBean { private CacheStatus inspectCacheables(Collection cacheables) { Map cacheUpdates = new LinkedHashMap(cacheables.size()); - boolean updateRequired = false; + boolean cacheHit = false; Object retVal = null; if (!cacheables.isEmpty()) { @@ -288,21 +288,17 @@ public abstract class CacheAspectSupport implements InitializingBean { } // add op/key (in case an update is discovered later on) cacheUpdates.put(context, key); - boolean localCacheHit = false; // check whether the cache needs to be inspected or not (the method will be invoked anyway) - if (!updateRequired) { + if (!cacheHit) { for (Cache cache : context.getCaches()) { Cache.ValueWrapper wrapper = cache.get(key); if (wrapper != null) { retVal = wrapper.get(); - localCacheHit = true; + cacheHit = true; break; } } } - if (!localCacheHit) { - updateRequired = true; - } } else { if (log) { @@ -313,7 +309,7 @@ public abstract class CacheAspectSupport implements InitializingBean { // return a status only if at least one cacheable matched if (atLeastOnePassed) { - return new CacheStatus(cacheUpdates, updateRequired, retVal); + return new CacheStatus(cacheUpdates, !cacheHit, retVal); } } diff --git a/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java b/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java new file mode 100644 index 0000000000..c89520a0a4 --- /dev/null +++ b/spring-context/src/test/java/org/springframework/cache/CacheReproTests.java @@ -0,0 +1,104 @@ +/* + * Copyright 2002-2013 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 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cache; + +import java.util.Collections; +import java.util.List; + +import org.junit.Test; +import org.springframework.cache.annotation.Cacheable; +import org.springframework.cache.annotation.Caching; +import org.springframework.cache.annotation.EnableCaching; +import org.springframework.cache.concurrent.ConcurrentMapCacheManager; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +import static org.junit.Assert.*; + +/** + * Tests to reproduce raised caching issues. + * + * @author Phillip Webb + */ +public class CacheReproTests { + + @Test + public void spr11124() throws Exception { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + Spr11124Config.class); + Spr11124Service bean = context.getBean(Spr11124Service.class); + bean.single(2); + bean.single(2); + bean.multiple(2); + bean.multiple(2); + context.close(); + } + + @Configuration + @EnableCaching + public static class Spr11124Config { + + @Bean + public CacheManager cacheManager() { + return new ConcurrentMapCacheManager(); + } + + @Bean + public Spr11124Service service() { + return new Spr11124ServiceImpl(); + } + + } + + public interface Spr11124Service { + + public List single(int id); + + public List multiple(int id); + + } + + public static class Spr11124ServiceImpl implements Spr11124Service { + + private int multipleCount = 0; + + @Override + @Cacheable(value = "smallCache") + public List single(int id) { + if (this.multipleCount > 0) { + fail("Called too many times"); + } + this.multipleCount++; + return Collections.emptyList(); + } + + @Override + @Caching(cacheable = { + @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"); + } + this.multipleCount++; + return Collections.emptyList(); + } + + } + +}