Consistent cache key implementation across transaction and cache attribute sources

Includes consistent applicability of class-level metadata to user-level methods only.

Issue: SPR-14017
Issue: SPR-14095

Author: jhoeller

spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/AbstractFallbackJCacheOperationSource.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -24,8 +24,8 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import org.springframework.context.expression.AnnotatedElementKey;
 import org.springframework.core.BridgeMethodResolver;
+import org.springframework.core.MethodClassKey;
 import org.springframework.util.ClassUtils;
 
 /**
@@ -51,12 +51,12 @@ public abstract class AbstractFallbackJCacheOperationSource implements JCacheOpe
 
 	protected final Log logger = LogFactory.getLog(getClass());
 
-	private final Map<Object, Object> cache = new ConcurrentHashMap<Object, Object>(1024);
+	private final Map<MethodClassKey, Object> cache = new ConcurrentHashMap<MethodClassKey, Object>(1024);
 
 
 	@Override
 	public JCacheOperation<?> getCacheOperation(Method method, Class<?> targetClass) {
-		Object cacheKey = new AnnotatedElementKey(method, targetClass);
+		MethodClassKey cacheKey = new MethodClassKey(method, targetClass);
 		Object cached = this.cache.get(cacheKey);
 
 		if (cached != null) {
@@ -95,7 +95,7 @@ private JCacheOperation<?> computeCacheOperation(Method method, Class<?> targetC
 			return operation;
 		}
 		if (specificMethod != method) {
-			// Fall back is to look at the original method.
+			// Fallback is to look at the original method.
 			operation = findCacheOperation(method, targetClass);
 			if (operation != null) {
 				return operation;

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -26,27 +26,24 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import org.springframework.context.expression.AnnotatedElementKey;
 import org.springframework.core.BridgeMethodResolver;
+import org.springframework.core.MethodClassKey;
 import org.springframework.util.ClassUtils;
 
 /**
- * Abstract implementation of {@link CacheOperation} that caches
- * attributes for methods and implements a fallback policy: 1. specific
- * target method; 2. target class; 3. declaring method; 4. declaring
- * class/interface.
+ * Abstract implementation of {@link CacheOperation} that caches attributes
+ * for methods and implements a fallback policy: 1. specific target method;
+ * 2. target class; 3. declaring method; 4. declaring class/interface.
  *
  * <p>Defaults to using the target class's caching attribute if none is
- * associated with the target method. Any caching attribute associated
- * with the target method completely overrides a class caching attribute.
- * If none found on the target class, the interface that the invoked
- * method has been called through (in case of a JDK proxy) will be
- * checked.
+ * associated with the target method. Any caching attribute associated with
+ * the target method completely overrides a class caching attribute.
+ * If none found on the target class, the interface that the invoked method
+ * has been called through (in case of a JDK proxy) will be checked.
  *
- * <p>This implementation caches attributes by method after they are
- * first used. If it is ever desirable to allow dynamic changing of
- * cacheable attributes (which is very unlikely), caching could be made
- * configurable.
+ * <p>This implementation caches attributes by method after they are first
+ * used. If it is ever desirable to allow dynamic changing of cacheable
+ * attributes (which is very unlikely), caching could be made configurable.
  *
  * @author Costin Leau
  * @author Juergen Hoeller
@@ -69,7 +66,7 @@ public abstract class AbstractFallbackCacheOperationSource implements CacheOpera
 	protected final Log logger = LogFactory.getLog(getClass());
 
 	/**
-	 * Cache of CacheOperations, keyed by {@link AnnotatedElementKey}.
+	 * Cache of CacheOperations, keyed by method on a specific target class.
 	 * <p>As this base class is not marked Serializable, the cache will be recreated
 	 * after serialization - provided that the concrete subclass is Serializable.
 	 */
@@ -117,7 +114,7 @@ public Collection<CacheOperation> getCacheOperations(Method method, Class<?> tar
 	 * @return the cache key (never {@code null})
 	 */
 	protected Object getCacheKey(Method method, Class<?> targetClass) {
-		return new AnnotatedElementKey(method, targetClass);
+		return new MethodClassKey(method, targetClass);
 	}
 
 	private Collection<CacheOperation> computeCacheOperations(Method method, Class<?> targetClass) {
@@ -140,19 +137,23 @@ private Collection<CacheOperation> computeCacheOperations(Method method, Class<?
 
 		// Second try is the caching operation on the target class.
 		opDef = findCacheOperations(specificMethod.getDeclaringClass());
-		if (opDef != null) {
+		if (opDef != null && ClassUtils.isUserLevelMethod(method)) {
 			return opDef;
 		}
 
 		if (specificMethod != method) {
-			// Fall back is to look at the original method.
+			// Fallback is to look at the original method.
 			opDef = findCacheOperations(method);
 			if (opDef != null) {
 				return opDef;
 			}
-			// Last fall back is the class of the original method.
-			return findCacheOperations(method.getDeclaringClass());
+			// Last fallback is the class of the original method.
+			opDef = findCacheOperations(method.getDeclaringClass());
+			if (opDef != null && ClassUtils.isUserLevelMethod(method)) {
+				return opDef;
+			}
 		}
+
 		return null;
 	}
 

spring-context/src/main/java/org/springframework/context/expression/CachedExpressionEvaluator.java

@@ -84,14 +84,14 @@ private ExpressionKey createKey(AnnotatedElementKey elementKey, String expressio
 	}
 
 
-	protected static class ExpressionKey {
+	protected static class ExpressionKey implements Comparable<ExpressionKey> {
 
-		private final AnnotatedElementKey key;
+		private final AnnotatedElementKey element;
 
 		private final String expression;
 
-		protected ExpressionKey(AnnotatedElementKey key, String expression) {
-			this.key = key;
+		protected ExpressionKey(AnnotatedElementKey element, String expression) {
+			this.element = element;
 			this.expression = expression;
 		}
 
@@ -104,13 +104,27 @@ public boolean equals(Object other) {
 				return false;
 			}
 			ExpressionKey otherKey = (ExpressionKey) other;
-			return (this.key.equals(otherKey.key) &&
+			return (this.element.equals(otherKey.element) &&
 					ObjectUtils.nullSafeEquals(this.expression, otherKey.expression));
 		}
 
 		@Override
 		public int hashCode() {
-			return this.key.hashCode() + (this.expression != null ? this.expression.hashCode() * 29 : 0);
+			return this.element.hashCode() + (this.expression != null ? this.expression.hashCode() * 29 : 0);
+		}
+
+		@Override
+		public String toString() {
+			return this.element + (this.expression != null ? " with expression \"" + this.expression : "\"");
+		}
+
+		@Override
+		public int compareTo(ExpressionKey other) {
+			int result = this.element.toString().compareTo(other.element.toString());
+			if (result == 0 && this.expression != null) {
+				result = this.expression.compareTo(other.expression);
+			}
+			return result;
 		}
 	}
 

spring-core/src/main/java/org/springframework/core/MethodClassKey.java

@@ -0,0 +1,85 @@
+/*
+ * Copyright 2002-2016 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.core;
+
+import java.lang.reflect.Method;
+
+import org.springframework.util.ObjectUtils;
+
+/**
+ * A common key class for a method against a specific target class,
+ * including {@link #toString()} representation and {@link Comparable}
+ * support (as suggested for custom {@code HashMap} keys as of Java 8).
+ *
+ * @author Juergen Hoeller
+ * @since 4.3
+ */
+public final class MethodClassKey implements Comparable<MethodClassKey> {
+
+	private final Method method;
+
+	private final Class<?> targetClass;
+
+
+	/**
+	 * Create a key object for the given method and target class.
+	 * @param method the method to wrap (must not be {@code null})
+	 * @param targetClass the target class that the method will be invoked
+	 * on (may be {@code null} if identical to the declaring class)
+	 */
+	public MethodClassKey(Method method, Class<?> targetClass) {
+		this.method = method;
+		this.targetClass = targetClass;
+	}
+
+
+	@Override
+	public boolean equals(Object other) {
+		if (this == other) {
+			return true;
+		}
+		if (!(other instanceof MethodClassKey)) {
+			return false;
+		}
+		MethodClassKey otherKey = (MethodClassKey) other;
+		return (this.method.equals(otherKey.method) &&
+				ObjectUtils.nullSafeEquals(this.targetClass, otherKey.targetClass));
+	}
+
+	@Override
+	public int hashCode() {
+		return this.method.hashCode() + (this.targetClass != null ? this.targetClass.hashCode() * 29 : 0);
+	}
+
+	@Override
+	public String toString() {
+		return this.method + (this.targetClass != null ? " on " + this.targetClass : "");
+	}
+
+	@Override
+	public int compareTo(MethodClassKey other) {
+		int result = this.method.getName().compareTo(other.method.getName());
+		if (result == 0) {
+			result = this.method.toString().compareTo(other.method.toString());
+			if (result == 0 && this.targetClass != null) {
+				result = this.targetClass.getName().compareTo(other.targetClass.getName());
+			}
+		}
+		return result;
+	}
+
+}

spring-tx/src/main/java/org/springframework/transaction/interceptor/AbstractFallbackTransactionAttributeSource.java

@@ -25,8 +25,8 @@
 import org.apache.commons.logging.LogFactory;
 
 import org.springframework.core.BridgeMethodResolver;
+import org.springframework.core.MethodClassKey;
 import org.springframework.util.ClassUtils;
-import org.springframework.util.ObjectUtils;
 
 /**
  * Abstract implementation of {@link TransactionAttributeSource} that caches
@@ -65,7 +65,7 @@ public abstract class AbstractFallbackTransactionAttributeSource implements Tran
 	protected final Log logger = LogFactory.getLog(getClass());
 
 	/**
-	 * Cache of TransactionAttributes, keyed by DefaultCacheKey (Method + target Class).
+	 * Cache of TransactionAttributes, keyed by method on a specific target class.
 	 * <p>As this base class is not marked Serializable, the cache will be recreated
 	 * after serialization - provided that the concrete subclass is Serializable.
 	 */
@@ -123,7 +123,7 @@ public TransactionAttribute getTransactionAttribute(Method method, Class<?> targ
 	 * @return the cache key (never {@code null})
 	 */
 	protected Object getCacheKey(Method method, Class<?> targetClass) {
-		return new DefaultCacheKey(method, targetClass);
+		return new MethodClassKey(method, targetClass);
 	}
 
 	/**
@@ -203,55 +203,4 @@ protected boolean allowPublicMethodsOnly() {
 		return false;
 	}
 
-
-	/**
-	 * Default cache key for the TransactionAttribute cache.
-	 */
-	private static final class DefaultCacheKey implements Comparable<DefaultCacheKey> {
-
-		private final Method method;
-
-		private final Class<?> targetClass;
-
-		public DefaultCacheKey(Method method, Class<?> targetClass) {
-			this.method = method;
-			this.targetClass = targetClass;
-		}
-
-		@Override
-		public boolean equals(Object other) {
-			if (this == other) {
-				return true;
-			}
-			if (!(other instanceof DefaultCacheKey)) {
-				return false;
-			}
-			DefaultCacheKey otherKey = (DefaultCacheKey) other;
-			return (this.method.equals(otherKey.method) &&
-					ObjectUtils.nullSafeEquals(this.targetClass, otherKey.targetClass));
-		}
-
-		@Override
-		public int hashCode() {
-			return this.method.hashCode() + (this.targetClass != null ? this.targetClass.hashCode() * 29 : 0);
-		}
-
-		@Override
-		public String toString() {
-			return this.method + (this.targetClass != null ? " on " + this.targetClass : "");
-		}
-
-		@Override
-		public int compareTo(DefaultCacheKey other) {
-			int result = this.method.getName().compareTo(other.method.getName());
-			if (result == 0) {
-				result = this.method.toString().compareTo(other.method.toString());
-				if (result == 0 && this.targetClass != null) {
-					result = this.targetClass.getName().compareTo(other.targetClass.getName());
-				}
-			}
-			return result;
-		}
-	}
-
 }