Skip to content

Commit e20ac27

Browse files
snicollsnicoll
committed
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
1 parent a8848cb commit e20ac27

File tree

7 files changed

+274
-16
lines changed

7 files changed

+274
-16
lines changed

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.springframework.cache.interceptor;
1818

1919
import java.lang.reflect.Method;
20+
import java.util.ArrayList;
2021
import java.util.Collection;
2122
import java.util.Collections;
2223
import java.util.LinkedList;
@@ -322,7 +323,7 @@ private Object execute(CacheOperationInvoker invoker, CacheOperationContexts con
322323
Cache.ValueWrapper result = null;
323324

324325
// If there are no put requests, just use the cache hit
325-
if (cachePutRequests.isEmpty() && contexts.get(CachePutOperation.class).isEmpty()) {
326+
if (cachePutRequests.isEmpty() && !hasCachePut(contexts)) {
326327
result = cacheHit;
327328
}
328329

@@ -345,6 +346,27 @@ private Object execute(CacheOperationInvoker invoker, CacheOperationContexts con
345346
return result.get();
346347
}
347348

349+
private boolean hasCachePut(CacheOperationContexts contexts) {
350+
// Evaluate the conditions *without* the result object because we don't have it yet.
351+
Collection<CacheOperationContext> cachePutContexts = contexts.get(CachePutOperation.class);
352+
Collection<CacheOperationContext> excluded = new ArrayList<CacheOperationContext>();
353+
for (CacheOperationContext context : cachePutContexts) {
354+
try {
355+
if (!context.isConditionPassing(ExpressionEvaluator.RESULT_UNAVAILABLE)) {
356+
excluded.add(context);
357+
}
358+
}
359+
catch (VariableNotAvailableException e) {
360+
// Ignoring failure due to missing result, consider the cache put has
361+
// to proceed
362+
}
363+
}
364+
// check if all puts have been excluded by condition
365+
return cachePutContexts.size() != excluded.size();
366+
367+
368+
}
369+
348370
private void processCacheEvicts(Collection<CacheOperationContext> contexts, boolean beforeInvocation, Object result) {
349371
for (CacheOperationContext context : contexts) {
350372
CacheEvictOperation operation = (CacheEvictOperation) context.metadata.operation;

‎spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java renamed to ‎spring-context/src/main/java/org/springframework/cache/interceptor/CacheEvaluationContext.java

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package org.springframework.cache.interceptor;
1818

1919
import java.lang.reflect.Method;
20+
import java.util.ArrayList;
21+
import java.util.List;
2022
import java.util.Map;
2123

2224
import org.springframework.aop.support.AopUtils;
@@ -25,18 +27,23 @@
2527
import org.springframework.util.ObjectUtils;
2628

2729
/**
28-
* Evaluation context class that adds a method parameters as SpEL
30+
* Cache specific evaluation context that adds a method parameters as SpEL
2931
* variables, in a lazy manner. The lazy nature eliminates unneeded
3032
* parsing of classes byte code for parameter discovery.
3133
*
34+
* <p>Also define a set of "unavailable variables" (i.e. variables that should
35+
* lead to an exception right the way when they are accessed). This can be useful
36+
* to verify a condition does not match even when not all potential variables
37+
* are present.
38+
*
3239
* <p>To limit the creation of objects, an ugly constructor is used
3340
* (rather then a dedicated 'closure'-like class for deferred execution).
3441
*
3542
* @author Costin Leau
3643
* @author Stephane Nicoll
3744
* @since 3.1
3845
*/
39-
class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
46+
class CacheEvaluationContext extends StandardEvaluationContext {
4047

4148
private final ParameterNameDiscoverer paramDiscoverer;
4249

@@ -48,10 +55,12 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
4855

4956
private final Map<MethodCacheKey, Method> methodCache;
5057

58+
private final List<String> unavailableVariables;
59+
5160
private boolean paramLoaded = false;
5261

5362

54-
LazyParamAwareEvaluationContext(Object rootObject, ParameterNameDiscoverer paramDiscoverer, Method method,
63+
CacheEvaluationContext(Object rootObject, ParameterNameDiscoverer paramDiscoverer, Method method,
5564
Object[] args, Class<?> targetClass, Map<MethodCacheKey, Method> methodCache) {
5665
super(rootObject);
5766

@@ -60,6 +69,18 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
6069
this.args = args;
6170
this.targetClass = targetClass;
6271
this.methodCache = methodCache;
72+
this.unavailableVariables = new ArrayList<String>();
73+
}
74+
75+
/**
76+
* Add the specified variable name as unavailable for that context. Any expression trying
77+
* to access this variable should lead to an exception.
78+
* <p>This permits the validation of expressions that could potentially a variable even
79+
* when such variable isn't available yet. Any expression trying to use that variable should
80+
* therefore fail to evaluate.
81+
*/
82+
public void addUnavailableVariable(String name) {
83+
this.unavailableVariables.add(name);
6384
}
6485

6586

@@ -68,6 +89,9 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext {
6889
*/
6990
@Override
7091
public Object lookupVariable(String name) {
92+
if (this.unavailableVariables.contains(name)) {
93+
throw new VariableNotAvailableException(name);
94+
}
7195
Object variable = super.lookupVariable(name);
7296
if (variable != null) {
7397
return variable;

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

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,21 @@
4444
*/
4545
class ExpressionEvaluator {
4646

47+
/**
48+
* Indicate that there is no result variable.
49+
*/
4750
public static final Object NO_RESULT = new Object();
4851

52+
/**
53+
* Indicate that the result variable cannot be used at all.
54+
*/
55+
public static final Object RESULT_UNAVAILABLE = new Object();
56+
57+
/**
58+
* The name of the variable holding the result object.
59+
*/
60+
public static final String RESULT_VARIABLE = "result";
61+
4962

5063
private final SpelExpressionParser parser = new SpelExpressionParser();
5164

@@ -91,10 +104,12 @@ public EvaluationContext createEvaluationContext(Collection<? extends Cache> cac
91104
final Object result) {
92105
CacheExpressionRootObject rootObject = new CacheExpressionRootObject(caches,
93106
method, args, target, targetClass);
94-
LazyParamAwareEvaluationContext evaluationContext = new LazyParamAwareEvaluationContext(rootObject,
107+
CacheEvaluationContext evaluationContext = new CacheEvaluationContext(rootObject,
95108
this.paramNameDiscoverer, method, args, targetClass, this.targetMethodCache);
96-
if(result != NO_RESULT) {
97-
evaluationContext.setVariable("result", result);
109+
if (result == RESULT_UNAVAILABLE) {
110+
evaluationContext.addUnavailableVariable(RESULT_VARIABLE);
111+
} else if (result != NO_RESULT) {
112+
evaluationContext.setVariable(RESULT_VARIABLE, result);
98113
}
99114
return evaluationContext;
100115
}

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
/*
2+
* Copyright 2002-2014 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.cache.interceptor;
18+
19+
import org.springframework.expression.EvaluationException;
20+
21+
/**
22+
* A specific {@link EvaluationException} to mention that a given variable
23+
* used in the expression is not available in the context.
24+
*
25+
* @author Stephane Nicoll
26+
* @since 4.0.6
27+
*/
28+
@SuppressWarnings("serial")
29+
class VariableNotAvailableException extends EvaluationException {
30+
31+
private final String name;
32+
33+
public VariableNotAvailableException(String name) {
34+
super("Variable '" + name + "' is not available");
35+
this.name = name;
36+
}
37+
38+
39+
public String getName() {
40+
return name;
41+
}
42+
}

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

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
/*
2+
* Copyright 2002-2014 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.cache.interceptor;
18+
19+
import static org.junit.Assert.*;
20+
21+
import java.util.concurrent.atomic.AtomicLong;
22+
23+
import org.junit.After;
24+
import org.junit.Before;
25+
import org.junit.Test;
26+
27+
import org.springframework.cache.Cache;
28+
import org.springframework.cache.CacheManager;
29+
import org.springframework.cache.annotation.CacheConfig;
30+
import org.springframework.cache.annotation.CachePut;
31+
import org.springframework.cache.annotation.Cacheable;
32+
import org.springframework.cache.annotation.CachingConfigurerSupport;
33+
import org.springframework.cache.annotation.EnableCaching;
34+
import org.springframework.cache.concurrent.ConcurrentMapCacheManager;
35+
import org.springframework.context.ConfigurableApplicationContext;
36+
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
37+
import org.springframework.context.annotation.Bean;
38+
import org.springframework.context.annotation.Configuration;
39+
40+
/**
41+
* Tests corner case of using {@link Cacheable} and {@link CachePut} on the
42+
* same operation.
43+
*
44+
* @author Stephane Nicoll
45+
*/
46+
public class CachePutEvaluationTests {
47+
48+
private ConfigurableApplicationContext context;
49+
50+
private Cache cache;
51+
52+
private SimpleService service;
53+
54+
@Before
55+
public void setup() {
56+
this.context = new AnnotationConfigApplicationContext(Config.class);
57+
this.cache = context.getBean(CacheManager.class).getCache("test");
58+
this.service = context.getBean(SimpleService.class);
59+
}
60+
61+
@After
62+
public void close() {
63+
if (this.context != null) {
64+
this.context.close();
65+
}
66+
}
67+
68+
@Test
69+
public void mutualGetPutExclusion() {
70+
String key = "1";
71+
72+
Long first = service.getOrPut(key, true);
73+
Long second = service.getOrPut(key, true);
74+
assertSame(first, second);
75+
76+
// This forces the method to be executed again
77+
Long expected = first + 1;
78+
Long third = service.getOrPut(key, false);
79+
assertEquals(expected, third);
80+
81+
Long fourth = service.getOrPut(key, true);
82+
assertSame(third, fourth);
83+
}
84+
85+
@Test
86+
public void getAndPut() {
87+
cache.clear();
88+
89+
long key = 1;
90+
Long value = service.getAndPut(key);
91+
92+
assertEquals("Wrong value for @Cacheable key", value, cache.get(key).get());
93+
assertEquals("Wrong value for @CachePut key", value, cache.get(value + 100).get()); // See @CachePut
94+
95+
// CachePut forced a method call
96+
Long anotherValue = service.getAndPut(key);
97+
assertNotSame(value, anotherValue);
98+
// NOTE: while you might expect the main key to have been updated, it hasn't. @Cacheable operations
99+
// are only processed in case of a cache miss. This is why combining @Cacheable with @CachePut
100+
// is a very bad idea. We could refine the condition now that we can figure out if we are going
101+
// to invoke the method anyway but that brings a whole new set of potential regressions.
102+
//assertEquals("Wrong value for @Cacheable key", anotherValue, cache.get(key).get());
103+
assertEquals("Wrong value for @CachePut key", anotherValue, cache.get(anotherValue + 100).get());
104+
}
105+
106+
@Configuration
107+
@EnableCaching
108+
static class Config extends CachingConfigurerSupport {
109+
110+
@Bean
111+
@Override
112+
public CacheManager cacheManager() {
113+
return new ConcurrentMapCacheManager();
114+
}
115+
116+
@Bean
117+
public SimpleService simpleService() {
118+
return new SimpleService();
119+
}
120+
121+
}
122+
123+
@CacheConfig(cacheNames = "test")
124+
public static class SimpleService {
125+
private AtomicLong counter = new AtomicLong();
126+
127+
/**
128+
* Represent a mutual exclusion use case. The boolean flag exclude one of the two operation.
129+
*/
130+
@Cacheable(condition = "#p1", key = "#p0")
131+
@CachePut(condition = "!#p1", key = "#p0")
132+
public Long getOrPut(Object id, boolean flag) {
133+
return counter.getAndIncrement();
134+
}
135+
136+
/**
137+
* Represent an invalid use case. If the result of the operation is non null, then we put
138+
* the value with a different key. This forces the method to be executed every time.
139+
*/
140+
@Cacheable
141+
@CachePut(key = "#result + 100", condition = "#result != null")
142+
public Long getAndPut(long id) {
143+
return counter.getAndIncrement();
144+
}
145+
}
146+
}

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

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,7 @@
2121
import java.util.Collections;
2222
import java.util.Iterator;
2323

24-
import org.junit.Rule;
2524
import org.junit.Test;
26-
import org.junit.rules.ExpectedException;
2725
import org.springframework.cache.annotation.AnnotationCacheOperationSource;
2826
import org.springframework.cache.annotation.Cacheable;
2927
import org.springframework.cache.annotation.Caching;
@@ -43,9 +41,6 @@
4341
*/
4442
public class ExpressionEvaluatorTests {
4543

46-
@Rule
47-
public ExpectedException thrown = ExpectedException.none();
48-
4944
private ExpressionEvaluator eval = new ExpressionEvaluator();
5045

5146
private AnnotationCacheOperationSource source = new AnnotationCacheOperationSource();
@@ -113,6 +108,18 @@ public void withoutReturnValue() throws Exception {
113108
assertThat(value, nullValue());
114109
}
115110

111+
@Test
112+
public void unavailableReturnValue() throws Exception {
113+
EvaluationContext context = createEvaluationContext(ExpressionEvaluator.RESULT_UNAVAILABLE);
114+
try {
115+
new SpelExpressionParser().parseExpression("#result").getValue(context);
116+
fail("Should have failed to parse expression, result not available");
117+
}
118+
catch (VariableNotAvailableException e) {
119+
assertEquals("wrong variable name", "result", e.getName());
120+
}
121+
}
122+
116123
private EvaluationContext createEvaluationContext(Object result) {
117124
AnnotatedClass target = new AnnotatedClass();
118125
Method method = ReflectionUtils.findMethod(AnnotatedClass.class, "multipleCaching", Object.class,

0 commit comments

Comments
 (0)