From e20ac27fb4328f5a7ae9eb2120eac5416e9c09f2 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Sat, 5 Jul 2014 11:26:05 +0200 Subject: [PATCH] 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 --- .../cache/interceptor/CacheAspectSupport.java | 24 ++- ...ntext.java => CacheEvaluationContext.java} | 30 +++- .../interceptor/ExpressionEvaluator.java | 21 ++- .../VariableNotAvailableException.java | 42 +++++ .../interceptor/CachePutEvaluationTests.java | 146 ++++++++++++++++++ .../interceptor/ExpressionEvaluatorTests.java | 17 +- src/asciidoc/index.adoc | 10 +- 7 files changed, 274 insertions(+), 16 deletions(-) rename spring-context/src/main/java/org/springframework/cache/interceptor/{LazyParamAwareEvaluationContext.java => CacheEvaluationContext.java} (71%) create mode 100644 spring-context/src/main/java/org/springframework/cache/interceptor/VariableNotAvailableException.java create mode 100644 spring-context/src/test/java/org/springframework/cache/interceptor/CachePutEvaluationTests.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 4d3e62f1a7..d0080584bf 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 @@ -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 cachePutContexts = contexts.get(CachePutOperation.class); + Collection excluded = new ArrayList(); + 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 contexts, boolean beforeInvocation, Object result) { for (CacheOperationContext context : contexts) { CacheEvictOperation operation = (CacheEvictOperation) context.metadata.operation; diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheEvaluationContext.java similarity index 71% rename from spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java rename to spring-context/src/main/java/org/springframework/cache/interceptor/CacheEvaluationContext.java index 143d9f5f52..492e8a6ead 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java +++ b/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. * + *

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. + * *

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 methodCache; + private final List unavailableVariables; + private boolean paramLoaded = false; - LazyParamAwareEvaluationContext(Object rootObject, ParameterNameDiscoverer paramDiscoverer, Method method, + CacheEvaluationContext(Object rootObject, ParameterNameDiscoverer paramDiscoverer, Method method, Object[] args, Class targetClass, Map methodCache) { super(rootObject); @@ -60,6 +69,18 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext { this.args = args; this.targetClass = targetClass; this.methodCache = methodCache; + this.unavailableVariables = new ArrayList(); + } + + /** + * Add the specified variable name as unavailable for that context. Any expression trying + * to access this variable should lead to an exception. + *

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; diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/ExpressionEvaluator.java b/spring-context/src/main/java/org/springframework/cache/interceptor/ExpressionEvaluator.java index 84c9a962b0..6cfaa1b4e7 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/ExpressionEvaluator.java +++ b/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; } diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/VariableNotAvailableException.java b/spring-context/src/main/java/org/springframework/cache/interceptor/VariableNotAvailableException.java new file mode 100644 index 0000000000..b6f52d61f2 --- /dev/null +++ b/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; + } +} diff --git a/spring-context/src/test/java/org/springframework/cache/interceptor/CachePutEvaluationTests.java b/spring-context/src/test/java/org/springframework/cache/interceptor/CachePutEvaluationTests.java new file mode 100644 index 0000000000..3085bc9a45 --- /dev/null +++ b/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(); + } + } +} diff --git a/spring-context/src/test/java/org/springframework/cache/interceptor/ExpressionEvaluatorTests.java b/spring-context/src/test/java/org/springframework/cache/interceptor/ExpressionEvaluatorTests.java index 090fc7be92..fe182a05a1 100644 --- a/spring-context/src/test/java/org/springframework/cache/interceptor/ExpressionEvaluatorTests.java +++ b/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, diff --git a/src/asciidoc/index.adoc b/src/asciidoc/index.adoc index f517d37357..969b2abedb 100644 --- a/src/asciidoc/index.adoc +++ b/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. ====