Fix mutually exclusive use of CachePut and Cacheable

Commit eea230f introduced a regression by adding a support for the
"result" variable in SpEL expression for @CachePut. As such expressions
cannot be evaluated upfront anymore, any method that contains both
@Cacheable and @CachePut annotations are always executed even when
their conditions are mutually exclusive.

This is an example of such mutual exclusion

@Cacheable(condition = "#p1", key = "#p0")
@CachePut(condition = "!#p1", key = "#p0")
public Object getFooById(Object id, boolean flag) { ... }

This commit updates CacheEvaluationContext to define a set of
unavailable variables. When such variable is accessed for a given
expression, an exception is thrown. This is used to restore the
evaluation of the @CachePut condition upfront by registering "result"
as an unavailable variable.

If all @CachePut operations have been excluded by this upfront check,
the @Cacheable operation is processed as it was before. Such upfront
check restore the behavior prior to eea230f.

Issue: SPR-11955

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

@@ -17,6 +17,7 @@
 package org.springframework.cache.interceptor;
 
 import java.lang.reflect.Method;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.LinkedList;
@@ -322,7 +323,7 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		Cache.ValueWrapper result = null;
 
 		// If there are no put requests, just use the cache hit
-		if (cachePutRequests.isEmpty() && contexts.get(CachePutOperation.class).isEmpty()) {
+		if (cachePutRequests.isEmpty() && !hasCachePut(contexts)) {
 			result = cacheHit;
 		}
 
@@ -345,6 +346,27 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker
 		return result.get();
 	}
 
+	private boolean hasCachePut(CacheOperationContexts contexts) {
+		// Evaluate the conditions *without* the result object because we don't have it yet.
+		Collection<CacheOperationContext> cachePutContexts = contexts.get(CachePutOperation.class);
+		Collection<CacheOperationContext> excluded = new ArrayList<CacheOperationContext>();
+		for (CacheOperationContext context : cachePutContexts) {
+			try {
+				if (!context.isConditionPassing(ExpressionEvaluator.RESULT_UNAVAILABLE)) {
+	                excluded.add(context);
+				}
+			}
+			catch (VariableNotAvailableException e) {
+				// Ignoring failure due to missing result, consider the cache put has
+				// to proceed
+			}
+		}
+		// check if  all puts have been excluded by condition
+		return cachePutContexts.size() != excluded.size();
+
+
+	}
+
 	private void processCacheEvicts(Collection<CacheOperationContext> contexts, boolean beforeInvocation, Object result) {
 		for (CacheOperationContext context : contexts) {
 			CacheEvictOperation operation = (CacheEvictOperation) context.metadata.operation;

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

@@ -17,6 +17,8 @@
 package org.springframework.cache.interceptor;
 
 import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 
 import org.springframework.aop.support.AopUtils;
@@ -25,10 +27,15 @@ import org.springframework.expression.spel.support.StandardEvaluationContext;
 import org.springframework.util.ObjectUtils;
 
 /**
- * Evaluation context class that adds a method parameters as SpEL
+ * Cache specific evaluation context that adds a method parameters as SpEL
  * variables, in a lazy manner. The lazy nature eliminates unneeded
  * parsing of classes byte code for parameter discovery.
  *
+ * <p>Also define a set of "unavailable variables" (i.e. variables that should
+ * lead to an exception right the way when they are accessed). This can be useful
+ * to verify a condition does not match even when not all potential variables
+ * are present.
+ *
  * <p>To limit the creation of objects, an ugly constructor is used
  * (rather then a dedicated 'closure'-like class for deferred execution).
  *
@@ -36,7 +43,7 @@ import org.springframework.util.ObjectUtils;
  * @author Stephane Nicoll
  * @since 3.1
  */
-class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
+class CacheEvaluationContext extends StandardEvaluationContext {
 
 	private final ParameterNameDiscoverer paramDiscoverer;
 
@@ -48,10 +55,12 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
 
 	private final Map<MethodCacheKey, Method> methodCache;
 
+	private final List<String> unavailableVariables;
+
 	private boolean paramLoaded = false;
 
 
-	LazyParamAwareEvaluationContext(Object rootObject, ParameterNameDiscoverer paramDiscoverer, Method method,
+	CacheEvaluationContext(Object rootObject, ParameterNameDiscoverer paramDiscoverer, Method method,
 			Object[] args, Class<?> targetClass, Map<MethodCacheKey, Method> methodCache) {
 		super(rootObject);
 
@@ -60,6 +69,18 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
 		this.args = args;
 		this.targetClass = targetClass;
 		this.methodCache = methodCache;
+		this.unavailableVariables = new ArrayList<String>();
+	}
+
+	/**
+	 * Add the specified variable name as unavailable for that context. Any expression trying
+	 * to access this variable should lead to an exception.
+	 * <p>This permits the validation of expressions that could potentially a variable even
+	 * when such variable isn't available yet. Any expression trying to use that variable should
+	 * therefore fail to evaluate.
+	 */
+	public void addUnavailableVariable(String name) {
+		this.unavailableVariables.add(name);
 	}
 
 
@@ -68,6 +89,9 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
 	 */
 	@Override
 	public Object lookupVariable(String name) {
+		if (this.unavailableVariables.contains(name)) {
+			throw new VariableNotAvailableException(name);
+		}
 		Object variable = super.lookupVariable(name);
 		if (variable != null) {
 			return variable;

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

@@ -44,8 +44,21 @@ import org.springframework.util.ObjectUtils;
  */
 class ExpressionEvaluator {
 
+	/**
+	 * Indicate that there is no result variable.
+	 */
 	public static final Object NO_RESULT = new Object();
 
+	/**
+	 * Indicate that the result variable cannot be used at all.
+	 */
+	public static final Object RESULT_UNAVAILABLE = new Object();
+
+	/**
+	 * The name of the variable holding the result object.
+	 */
+	public static final String RESULT_VARIABLE = "result";
+
 
 	private final SpelExpressionParser parser = new SpelExpressionParser();
 
@@ -91,10 +104,12 @@ class ExpressionEvaluator {
 			final Object result) {
 		CacheExpressionRootObject rootObject = new CacheExpressionRootObject(caches,
 				method, args, target, targetClass);
-		LazyParamAwareEvaluationContext evaluationContext = new LazyParamAwareEvaluationContext(rootObject,
+		CacheEvaluationContext evaluationContext = new CacheEvaluationContext(rootObject,
 				this.paramNameDiscoverer, method, args, targetClass, this.targetMethodCache);
-		if(result != NO_RESULT) {
-			evaluationContext.setVariable("result", result);
+		if (result == RESULT_UNAVAILABLE) {
+			evaluationContext.addUnavailableVariable(RESULT_VARIABLE);
+		} else if (result != NO_RESULT) {
+			evaluationContext.setVariable(RESULT_VARIABLE, result);
 		}
 		return evaluationContext;
 	}

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

@@ -0,0 +1,42 @@
+/*
+ * 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
+ *
+ * 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.interceptor;
+
+import org.springframework.expression.EvaluationException;
+
+/**
+ * A specific {@link EvaluationException} to mention that a given variable
+ * used in the expression is not available in the context.
+ *
+ * @author Stephane Nicoll
+ * @since 4.0.6
+ */
+@SuppressWarnings("serial")
+class VariableNotAvailableException extends EvaluationException {
+
+	private final String name;
+
+	public VariableNotAvailableException(String name) {
+		super("Variable '" + name + "' is not available");
+		this.name = name;
+	}
+
+
+	public String getName() {
+		return name;
+	}
+}

spring-context/src/test/java/org/springframework/cache/interceptor/CachePutEvaluationTests.java

@@ -0,0 +1,146 @@
+/*
+ * 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
+ *
+ * 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.interceptor;
+
+import static org.junit.Assert.*;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.springframework.cache.Cache;
+import org.springframework.cache.CacheManager;
+import org.springframework.cache.annotation.CacheConfig;
+import org.springframework.cache.annotation.CachePut;
+import org.springframework.cache.annotation.Cacheable;
+import org.springframework.cache.annotation.CachingConfigurerSupport;
+import org.springframework.cache.annotation.EnableCaching;
+import org.springframework.cache.concurrent.ConcurrentMapCacheManager;
+import org.springframework.context.ConfigurableApplicationContext;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+
+/**
+ * Tests corner case of using {@link Cacheable} and  {@link CachePut} on the
+ * same operation.
+ *
+ * @author Stephane Nicoll
+ */
+public class CachePutEvaluationTests {
+
+	private ConfigurableApplicationContext context;
+
+	private Cache cache;
+
+	private SimpleService service;
+
+	@Before
+	public void setup() {
+		this.context = new AnnotationConfigApplicationContext(Config.class);
+		this.cache = context.getBean(CacheManager.class).getCache("test");
+		this.service = context.getBean(SimpleService.class);
+	}
+
+	@After
+	public void close() {
+		if (this.context != null) {
+			this.context.close();
+		}
+	}
+
+	@Test
+	public void mutualGetPutExclusion() {
+		String key = "1";
+
+		Long first = service.getOrPut(key, true);
+		Long second = service.getOrPut(key, true);
+		assertSame(first, second);
+
+		// This forces the method to be executed again
+		Long expected = first + 1;
+		Long third = service.getOrPut(key, false);
+		assertEquals(expected, third);
+
+		Long fourth = service.getOrPut(key, true);
+		assertSame(third, fourth);
+	}
+
+	@Test
+	public void getAndPut() {
+		cache.clear();
+
+		long key = 1;
+		Long value = service.getAndPut(key);
+
+		assertEquals("Wrong value for @Cacheable key", value, cache.get(key).get());
+		assertEquals("Wrong value for @CachePut key", value, cache.get(value + 100).get()); // See @CachePut
+
+		// CachePut forced a method call
+		Long anotherValue = service.getAndPut(key);
+		assertNotSame(value, anotherValue);
+		// NOTE: while you might expect the main key to have been updated, it hasn't. @Cacheable operations
+		// are only processed in case of a cache miss. This is why combining @Cacheable with @CachePut
+		// is a very bad idea. We could refine the condition now that we can figure out if we are going
+		// to invoke the method anyway but that brings a whole new set of potential regressions.
+		//assertEquals("Wrong value for @Cacheable key", anotherValue, cache.get(key).get());
+		assertEquals("Wrong value for @CachePut key", anotherValue, cache.get(anotherValue + 100).get());
+	}
+
+	@Configuration
+	@EnableCaching
+	static class Config extends CachingConfigurerSupport {
+
+		@Bean
+		@Override
+		public CacheManager cacheManager() {
+			return new ConcurrentMapCacheManager();
+		}
+
+		@Bean
+		public SimpleService simpleService() {
+			return new SimpleService();
+		}
+
+	}
+
+	@CacheConfig(cacheNames = "test")
+	public static class SimpleService {
+		private AtomicLong counter = new AtomicLong();
+
+		/**
+		 * Represent a mutual exclusion use case. The boolean flag exclude one of the two operation.
+		 */
+		@Cacheable(condition = "#p1", key = "#p0")
+		@CachePut(condition = "!#p1", key = "#p0")
+		public Long getOrPut(Object id, boolean flag) {
+			return counter.getAndIncrement();
+		}
+
+		/**
+		 * Represent an invalid use case. If the result of the operation is non null, then we put
+		 * the value with a different key. This forces the method to be executed every time.
+		 */
+		@Cacheable
+		@CachePut(key = "#result + 100", condition = "#result != null")
+		public Long getAndPut(long id) {
+			return counter.getAndIncrement();
+		}
+	}
+}

spring-context/src/test/java/org/springframework/cache/interceptor/ExpressionEvaluatorTests.java

@@ -21,9 +21,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
 
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.springframework.cache.annotation.AnnotationCacheOperationSource;
 import org.springframework.cache.annotation.Cacheable;
 import org.springframework.cache.annotation.Caching;
@@ -43,9 +41,6 @@ import static org.junit.Assert.*;
  */
 public class ExpressionEvaluatorTests {
 
-	@Rule
-	public ExpectedException thrown = ExpectedException.none();
-
 	private ExpressionEvaluator eval = new ExpressionEvaluator();
 
 	private AnnotationCacheOperationSource source = new AnnotationCacheOperationSource();
@@ -113,6 +108,18 @@ public class ExpressionEvaluatorTests {
 		assertThat(value, nullValue());
 	}
 
+	@Test
+	public void unavailableReturnValue() throws Exception {
+		EvaluationContext context = createEvaluationContext(ExpressionEvaluator.RESULT_UNAVAILABLE);
+		try {
+			new SpelExpressionParser().parseExpression("#result").getValue(context);
+			fail("Should have failed to parse expression, result not available");
+		}
+		catch (VariableNotAvailableException e) {
+			assertEquals("wrong variable name", "result", e.getName());
+		}
+	}
+
 	private EvaluationContext createEvaluationContext(Object result) {
 		AnnotatedClass target = new AnnotatedClass();
 		Method method = ReflectionUtils.findMethod(AnnotatedClass.class, "multipleCaching", Object.class,

src/asciidoc/index.adoc

@@ -47173,11 +47173,13 @@ than method flow optimization:
 [IMPORTANT]
 ====
 Note that using `@CachePut` and `@Cacheable` annotations on the same method is generally
-discouraged because they have different behaviors. While the latter causes the method
-execution to be skipped by using the cache, the former forces the execution in order to
-execute a cache update. This leads to unexpected behavior and with the exception of
+strongly discouraged because they have different behaviors. While the latter causes the
+method execution to be skipped by using the cache, the former forces the execution in
+order to execute a cache update. This leads to unexpected behavior and with the exception of
 specific corner-cases (such as annotations having conditions that exclude them from each
-other), such declarations should be avoided.
+other), such declarations should be avoided. Note also that such conditions should not rely
+on the result object (i.e. the `#result` variable) as these are validated upfront to confirm
+the exclusion.
 ====