From 20228e78c5f5e3183dac6cb9ace85e56198ce530 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 25 Mar 2014 23:56:16 +0100 Subject: [PATCH] CacheAspectSupport checks Cache.get(key) once per invocation only Issue: SPR-11592 --- .../cache/interceptor/CacheAspectSupport.java | 8 +- .../cache/CacheReproTests.java | 131 ++++++++++++++++-- 2 files changed, 126 insertions(+), 13 deletions(-) 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 53b1d4b5b6..bf6b8439e7 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. @@ -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."); } 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 c89520a0a4..d60c14c170 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,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 single(int id); public List multiple(int id); - } + public static class Spr11124ServiceImpl implements Spr11124Service { private int multipleCount = 0; @Override - @Cacheable(value = "smallCache") + @Cacheable("smallCache") public List 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 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(); + } } }