From 397aa8298492d74ab64bff900229c11d2f5c5112 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Mon, 14 Apr 2014 21:01:01 +0200 Subject: [PATCH] Fix expression cache Prior to this commit, only the java.lang.reflect.Method was used to identify an annotated method. As a result, if different annotations were placed on different methods (method overriding, interface implementation) only the first one (cached) was used. LazyParamAwareEvaluationContext was affected by the exact same problem and has been also fixed. Issue: SPR-11692 --- .../cache/interceptor/CacheAspectSupport.java | 9 +- .../interceptor/ExpressionEvaluator.java | 74 +++++++---- .../LazyParamAwareEvaluationContext.java | 16 +-- .../cache/interceptor/MethodCacheKey.java | 2 +- .../ExpressionCachingIntegrationTests.java | 122 ++++++++++++++++++ .../interceptor/ExpressionEvaluatorTests.java | 9 +- 6 files changed, 192 insertions(+), 40 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/cache/config/ExpressionCachingIntegrationTests.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 6fa24bf338..1828e3febc 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 @@ -483,12 +483,15 @@ public abstract class CacheAspectSupport implements InitializingBean, Applicatio private final Collection caches; + private final MethodCacheKey methodCacheKey; + public CacheOperationContext(CacheOperationMetadata metadata, Object[] args, Object target) { this.metadata = metadata; this.args = extractArgs(metadata.method, args); this.target = target; this.caches = CacheAspectSupport.this.getCaches(this, metadata.cacheResolver); + this.methodCacheKey = new MethodCacheKey(metadata.method, metadata.targetClass); } @Override @@ -525,7 +528,7 @@ public abstract class CacheAspectSupport implements InitializingBean, Applicatio protected boolean isConditionPassing(Object result) { if (StringUtils.hasText(this.metadata.operation.getCondition())) { EvaluationContext evaluationContext = createEvaluationContext(result); - return evaluator.condition(this.metadata.operation.getCondition(), this.metadata.method, evaluationContext); + return evaluator.condition(this.metadata.operation.getCondition(), this.methodCacheKey, evaluationContext); } return true; } @@ -540,7 +543,7 @@ public abstract class CacheAspectSupport implements InitializingBean, Applicatio } if (StringUtils.hasText(unless)) { EvaluationContext evaluationContext = createEvaluationContext(value); - return !evaluator.unless(unless, this.metadata.method, evaluationContext); + return !evaluator.unless(unless, this.methodCacheKey, evaluationContext); } return true; } @@ -552,7 +555,7 @@ public abstract class CacheAspectSupport implements InitializingBean, Applicatio protected Object generateKey(Object result) { if (StringUtils.hasText(this.metadata.operation.getKey())) { EvaluationContext evaluationContext = createEvaluationContext(result); - return evaluator.key(this.metadata.operation.getKey(), this.metadata.method, evaluationContext); + return evaluator.key(this.metadata.operation.getKey(), this.methodCacheKey, evaluationContext); } return metadata.keyGenerator.generate(this.target, this.metadata.method, this.args); } 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 9405c40346..84c9a962b0 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 @@ -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. @@ -27,16 +27,19 @@ import org.springframework.core.ParameterNameDiscoverer; import org.springframework.expression.EvaluationContext; import org.springframework.expression.Expression; import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.util.ObjectUtils; /** * Utility class handling the SpEL expression parsing. * Meant to be used as a reusable, thread-safe component. * - *

Performs internal caching for performance reasons. + *

Performs internal caching for performance reasons + * using {@link MethodCacheKey}. * * @author Costin Leau * @author Phillip Webb * @author Sam Brannen + * @author Stephane Nicoll * @since 3.1 */ class ExpressionEvaluator { @@ -49,13 +52,16 @@ class ExpressionEvaluator { // shared param discoverer since it caches data internally private final ParameterNameDiscoverer paramNameDiscoverer = new DefaultParameterNameDiscoverer(); - private final Map keyCache = new ConcurrentHashMap(64); + private final Map keyCache + = new ConcurrentHashMap(64); - private final Map conditionCache = new ConcurrentHashMap(64); + private final Map conditionCache + = new ConcurrentHashMap(64); - private final Map unlessCache = new ConcurrentHashMap(64); + private final Map unlessCache + = new ConcurrentHashMap(64); - private final Map targetMethodCache = new ConcurrentHashMap(64); + private final Map targetMethodCache = new ConcurrentHashMap(64); /** @@ -93,22 +99,22 @@ class ExpressionEvaluator { return evaluationContext; } - public Object key(String keyExpression, Method method, EvaluationContext evalContext) { - return getExpression(this.keyCache, keyExpression, method).getValue(evalContext); + public Object key(String keyExpression, MethodCacheKey methodKey, EvaluationContext evalContext) { + return getExpression(this.keyCache, keyExpression, methodKey).getValue(evalContext); } - public boolean condition(String conditionExpression, Method method, EvaluationContext evalContext) { - return getExpression(this.conditionCache, conditionExpression, method).getValue( + public boolean condition(String conditionExpression, MethodCacheKey methodKey, EvaluationContext evalContext) { + return getExpression(this.conditionCache, conditionExpression, methodKey).getValue( evalContext, boolean.class); } - public boolean unless(String unlessExpression, Method method, EvaluationContext evalContext) { - return getExpression(this.unlessCache, unlessExpression, method).getValue( + public boolean unless(String unlessExpression, MethodCacheKey methodKey, EvaluationContext evalContext) { + return getExpression(this.unlessCache, unlessExpression, methodKey).getValue( evalContext, boolean.class); } - private Expression getExpression(Map cache, String expression, Method method) { - String key = toString(method, expression); + private Expression getExpression(Map cache, String expression, MethodCacheKey methodKey) { + ExpressionKey key = createKey(methodKey, expression); Expression rtn = cache.get(key); if (rtn == null) { rtn = this.parser.parseExpression(expression); @@ -117,13 +123,37 @@ class ExpressionEvaluator { return rtn; } - private String toString(Method method, String expression) { - StringBuilder sb = new StringBuilder(); - sb.append(method.getDeclaringClass().getName()); - sb.append("#"); - sb.append(method.toString()); - sb.append("#"); - sb.append(expression); - return sb.toString(); + private ExpressionKey createKey(MethodCacheKey methodCacheKey, String expression) { + return new ExpressionKey(methodCacheKey, expression); } + + + private static class ExpressionKey { + private final MethodCacheKey methodCacheKey; + private final String expression; + + private ExpressionKey(MethodCacheKey methodCacheKey, String expression) { + this.methodCacheKey = methodCacheKey; + this.expression = expression; + } + + @Override + public boolean equals(Object other) { + if (this == other) { + return true; + } + if (!(other instanceof ExpressionKey)) { + return false; + } + ExpressionKey otherKey = (ExpressionKey) other; + return (this.methodCacheKey.equals(otherKey.methodCacheKey) + && ObjectUtils.nullSafeEquals(this.expression, otherKey.expression)); + } + + @Override + public int hashCode() { + return this.methodCacheKey.hashCode() * 29 + (this.expression != null ? this.expression.hashCode() : 0); + } + } + } diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java b/spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java index 631ea6e442..143d9f5f52 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java +++ b/spring-context/src/main/java/org/springframework/cache/interceptor/LazyParamAwareEvaluationContext.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 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. @@ -33,6 +33,7 @@ import org.springframework.util.ObjectUtils; * (rather then a dedicated 'closure'-like class for deferred execution). * * @author Costin Leau + * @author Stephane Nicoll * @since 3.1 */ class LazyParamAwareEvaluationContext extends StandardEvaluationContext { @@ -45,13 +46,13 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext { private final Class targetClass; - private final Map methodCache; + private final Map methodCache; private boolean paramLoaded = false; LazyParamAwareEvaluationContext(Object rootObject, ParameterNameDiscoverer paramDiscoverer, Method method, - Object[] args, Class targetClass, Map methodCache) { + Object[] args, Class targetClass, Map methodCache) { super(rootObject); this.paramDiscoverer = paramDiscoverer; @@ -85,7 +86,7 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext { return; } - String methodKey = toString(this.method); + MethodCacheKey methodKey = new MethodCacheKey(this.method, this.targetClass); Method targetMethod = this.methodCache.get(methodKey); if (targetMethod == null) { targetMethod = AopUtils.getMostSpecificMethod(this.method, this.targetClass); @@ -110,11 +111,4 @@ class LazyParamAwareEvaluationContext extends StandardEvaluationContext { } } - private String toString(Method m) { - StringBuilder sb = new StringBuilder(); - sb.append(m.getDeclaringClass().getName()); - sb.append("#"); - sb.append(m.toString()); - return sb.toString(); - } } diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/MethodCacheKey.java b/spring-context/src/main/java/org/springframework/cache/interceptor/MethodCacheKey.java index 9de1671211..c164c5e3bb 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/MethodCacheKey.java +++ b/spring-context/src/main/java/org/springframework/cache/interceptor/MethodCacheKey.java @@ -27,7 +27,7 @@ import org.springframework.util.ObjectUtils; * * @author Costin Leau * @author Stephane Nicoll - * @since 4.1 + * @since 4.0.4 */ public final class MethodCacheKey { diff --git a/spring-context/src/test/java/org/springframework/cache/config/ExpressionCachingIntegrationTests.java b/spring-context/src/test/java/org/springframework/cache/config/ExpressionCachingIntegrationTests.java new file mode 100644 index 0000000000..3f1f5a9b46 --- /dev/null +++ b/spring-context/src/test/java/org/springframework/cache/config/ExpressionCachingIntegrationTests.java @@ -0,0 +1,122 @@ +/* + * 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.config; + +import org.junit.Test; + +import org.springframework.cache.CacheManager; +import org.springframework.cache.annotation.CachePut; +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; + +/** + * + * @author Stephane Nicoll + */ +public class ExpressionCachingIntegrationTests { + + @SuppressWarnings("unchecked") + @Test // SPR-11692 + public void expressionIsCacheBasedOnActualMethod() { + ConfigurableApplicationContext context = + new AnnotationConfigApplicationContext(SharedConfig.class, Spr11692Config.class); + + BaseDao userDao = (BaseDao) context.getBean("userDao"); + BaseDao orderDao = (BaseDao) context.getBean("orderDao"); + + userDao.persist(new User("1")); + orderDao.persist(new Order("2")); + + context.close(); + } + + + + @Configuration + static class Spr11692Config { + @Bean + public BaseDao userDao() { + return new UserDaoImpl(); + } + + @Bean + public BaseDao orderDao() { + return new OrderDaoImpl(); + } + } + + + private static interface BaseDao { + T persist(T t); + } + + private static class UserDaoImpl implements BaseDao { + @Override + @CachePut(value = "users", key = "#user.id") + public User persist(User user) { + return user; + } + } + + private static class OrderDaoImpl implements BaseDao { + @Override + @CachePut(value = "orders", key = "#order.id") + public Order persist(Order order) { + return order; + } + } + + private static class User { + private final String id; + + private User(String id) { + this.id = id; + } + + public String getId() { + return id; + } + } + + private static class Order { + private final String id; + + private Order(String id) { + this.id = id; + } + + public String getId() { + return id; + } + } + + @Configuration + @EnableCaching + static class SharedConfig extends CachingConfigurerSupport { + @Override + @Bean + public CacheManager cacheManager() { + return new ConcurrentMapCacheManager(); + } + } + +} 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 05f0043e51..090fc7be92 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 @@ -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. @@ -39,6 +39,7 @@ import static org.junit.Assert.*; * @author Costin Leau * @author Phillip Webb * @author Sam Brannen + * @author Stephane Nicoll */ public class ExpressionEvaluatorTests { @@ -82,8 +83,10 @@ public class ExpressionEvaluatorTests { Iterator it = ops.iterator(); - Object keyA = eval.key(it.next().getKey(), method, evalCtx); - Object keyB = eval.key(it.next().getKey(), method, evalCtx); + MethodCacheKey key = new MethodCacheKey(method, AnnotatedClass.class); + + Object keyA = eval.key(it.next().getKey(), key, evalCtx); + Object keyB = eval.key(it.next().getKey(), key, evalCtx); assertEquals(args[0], keyA); assertEquals(args[1], keyB);