Evaluate @Cacheable(condition) once per method invocation only

Issue: SPR-17024

Author: jhoeller

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

@@ -693,6 +693,9 @@ protected class CacheOperationContext implements CacheOperationInvocationContext
 
 		private final Collection<String> cacheNames;
 
+		@Nullable
+		private Boolean conditionPassing;
+
 		public CacheOperationContext(CacheOperationMetadata metadata, Object[] args, Object target) {
 			this.metadata = metadata;
 			this.args = extractArgs(metadata.method, args);
@@ -733,12 +736,17 @@ private Object[] extractArgs(Method method, Object[] args) {
 		}
 
 		protected boolean isConditionPassing(@Nullable Object result) {
-			if (StringUtils.hasText(this.metadata.operation.getCondition())) {
-				EvaluationContext evaluationContext = createEvaluationContext(result);
-				return evaluator.condition(this.metadata.operation.getCondition(),
-						this.metadata.methodKey, evaluationContext);
+			if (this.conditionPassing == null) {
+				if (StringUtils.hasText(this.metadata.operation.getCondition())) {
+					EvaluationContext evaluationContext = createEvaluationContext(result);
+					this.conditionPassing = evaluator.condition(this.metadata.operation.getCondition(),
+							this.metadata.methodKey, evaluationContext);
+				}
+				else {
+					this.conditionPassing = true;
+				}
 			}
-			return true;
+			return this.conditionPassing;
 		}
 
 		protected boolean canPutToCache(@Nullable Object value) {

spring-context/src/test/java/org/springframework/cache/config/EnableCachingIntegrationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -21,6 +21,7 @@
 import org.junit.After;
 import org.junit.Test;
 
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.cache.Cache;
 import org.springframework.cache.CacheManager;
 import org.springframework.cache.CacheTestUtils;
@@ -33,7 +34,10 @@
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
+import org.springframework.core.env.Environment;
+import org.springframework.mock.env.MockEnvironment;
 
+import static org.junit.Assert.*;
 import static org.springframework.cache.CacheTestUtils.*;
 
 /**
@@ -45,13 +49,15 @@ public class EnableCachingIntegrationTests {
 
 	private ConfigurableApplicationContext context;
 
+
 	@After
 	public void closeContext() {
 		if (this.context != null) {
 			this.context.close();
 		}
 	}
 
+
 	@Test
 	public void fooServiceWithInterface() {
 		this.context = new AnnotationConfigApplicationContext(FooConfig.class);
@@ -77,57 +83,91 @@ private void fooGetSimple(FooService service) {
 	}
 
 	@Test
-	public void beanCondition() {
+	public void beanConditionOff() {
 		this.context = new AnnotationConfigApplicationContext(BeanConditionConfig.class);
-		Cache cache = getCache();
 		FooService service = this.context.getBean(FooService.class);
+		Cache cache = getCache();
 
 		Object key = new Object();
 		service.getWithCondition(key);
 		assertCacheMiss(key, cache);
+		service.getWithCondition(key);
+		assertCacheMiss(key, cache);
+
+		assertEquals(2, this.context.getBean(BeanConditionConfig.Bar.class).count);
+	}
+
+	@Test
+	public void beanConditionOn() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.setEnvironment(new MockEnvironment().withProperty("bar.enabled", "true"));
+		ctx.register(BeanConditionConfig.class);
+		ctx.refresh();
+		this.context = ctx;
+
+		FooService service = this.context.getBean(FooService.class);
+		Cache cache = getCache();
+
+		Object key = new Object();
+		Object value = service.getWithCondition(key);
+		assertCacheHit(key, value, cache);
+		value = service.getWithCondition(key);
+		assertCacheHit(key, value, cache);
+
+		assertEquals(2, this.context.getBean(BeanConditionConfig.Bar.class).count);
 	}
 
 	private Cache getCache() {
 		return this.context.getBean(CacheManager.class).getCache("testCache");
 	}
 
+
 	@Configuration
 	static class SharedConfig extends CachingConfigurerSupport {
+
 		@Override
 		@Bean
 		public CacheManager cacheManager() {
 			return CacheTestUtils.createSimpleCacheManager("testCache");
 		}
 	}
 
+
 	@Configuration
 	@Import(SharedConfig.class)
 	@EnableCaching
 	static class FooConfig {
+
 		@Bean
 		public FooService fooService() {
 			return new FooServiceImpl();
 		}
 	}
 
+
 	@Configuration
 	@Import(SharedConfig.class)
 	@EnableCaching(proxyTargetClass = true)
 	static class FooConfigCglib {
+
 		@Bean
 		public FooService fooService() {
 			return new FooServiceImpl();
 		}
 	}
 
+
 	private interface FooService {
+
 		Object getSimple(Object key);
 
 		Object getWithCondition(Object key);
 	}
 
+
 	@CacheConfig(cacheNames = "testCache")
 	private static class FooServiceImpl implements FooService {
+
 		private final AtomicLong counter = new AtomicLong();
 
 		@Override
@@ -143,24 +183,33 @@ public Object getWithCondition(Object key) {
 		}
 	}
 
+
 	@Configuration
 	@Import(FooConfig.class)
 	@EnableCaching
 	static class BeanConditionConfig {
 
+		@Autowired
+		Environment env;
+
 		@Bean
 		public Bar bar() {
-			return new Bar(false);
+			return new Bar(Boolean.valueOf(env.getProperty("bar.enabled")));
 		}
 
+
 		static class Bar {
+
+			public int count;
+
 			private final boolean enabled;
 
 			public Bar(boolean enabled) {
 				this.enabled = enabled;
 			}
 
 			public boolean isEnabled() {
+				this.count++;
 				return this.enabled;
 			}
 		}