Expand var-args before passing to KeyGenerator

Phillip Webb · Phillip Webb · commit 05072e17625e · 2013-09-23T15:14:58.000-07:00
Update `CacheAspectSupport` to expand any var-arg parameters before
calling `KeyGenerator` implementations. Prior to this commit var-args
would be passed to `KeyGenerator` implementations as a nested array,
often causing the same key to be generated regardless of the arguments.

Issue: SPR-10870
diff --git a/spring-aspects/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java b/spring-aspects/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java
@@ -87,6 +87,12 @@ public Object key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
 
+	@Override
+	@Cacheable(value = "default")
+	public Object varArgsKey(Object... args) {
+		return counter.getAndIncrement();
+	}
+
 	@Override
 	@Cacheable(value = "default", key = "#root.methodName + #root.caches[0].name")
 	public Object name(Object arg1) {
diff --git a/spring-aspects/src/test/java/org/springframework/cache/config/CacheableService.java b/spring-aspects/src/test/java/org/springframework/cache/config/CacheableService.java
@@ -44,6 +44,8 @@ public interface CacheableService<T> {
 
 	T key(Object arg1, Object arg2);
 
+	T varArgsKey(Object... args);
+
 	T name(Object arg1);
 
 	T nullValue(Object arg1);
diff --git a/spring-aspects/src/test/java/org/springframework/cache/config/DefaultCacheableService.java b/spring-aspects/src/test/java/org/springframework/cache/config/DefaultCacheableService.java
@@ -91,6 +91,12 @@ public Long key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
 
+	@Override
+	@Cacheable(value = "default")
+	public Long varArgsKey(Object... args) {
+		return counter.getAndIncrement();
+	}
+
 	@Override
 	@Cacheable(value = "default", key = "#root.methodName")
 	public Long name(Object arg1) {
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
@@ -25,7 +25,6 @@
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.aop.framework.AopProxyUtils;
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.cache.Cache;
@@ -349,15 +348,27 @@ protected class CacheOperationContext {
 
 		private final Collection<Cache> caches;
 
-		public CacheOperationContext(CacheOperation operation, Method method, Object[] args, Object target, Class<?> targetClass) {
+		public CacheOperationContext(CacheOperation operation, Method method,
+				Object[] args, Object target, Class<?> targetClass) {
 			this.operation = operation;
 			this.method = method;
-			this.args = args;
+			this.args = extractArgs(method, args);
 			this.target = target;
 			this.targetClass = targetClass;
 			this.caches = CacheAspectSupport.this.getCaches(operation);
 		}
 
+		private Object[] extractArgs(Method method, Object[] args) {
+			if (!method.isVarArgs()) {
+				return args;
+			}
+			Object[] varArgs = (Object[]) args[args.length - 1];
+			Object[] combinedArgs = new Object[args.length - 1 + varArgs.length];
+			System.arraycopy(args, 0, combinedArgs, 0, args.length - 1);
+			System.arraycopy(varArgs, 0, combinedArgs, args.length - 1, varArgs.length);
+			return combinedArgs;
+		}
+
 		protected boolean isConditionPassing(Object result) {
 			if (StringUtils.hasText(this.operation.getCondition())) {
 				EvaluationContext evaluationContext = createEvaluationContext(result);
diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/KeyGenerator.java b/spring-context/src/main/java/org/springframework/cache/interceptor/KeyGenerator.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2013 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,10 +24,18 @@
  *
  * @author Costin Leau
  * @author Chris Beams
+ * @author Phillip Webb
  * @since 3.1
  */
 public interface KeyGenerator {
 
+	/**
+	 * Generate a key for the given method and its parameters.
+	 * @param target the target instance
+	 * @param method the method being called
+	 * @param params the method parameters (with any var-args expanded)
+	 * @return a generated key
+	 */
 	Object generate(Object target, Method method, Object... params);
 
 }
diff --git a/spring-context/src/test/java/org/springframework/cache/config/AbstractAnnotationTests.java b/spring-context/src/test/java/org/springframework/cache/config/AbstractAnnotationTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2010-2013 the original author or authors.
+ * Copyright 2002-2013 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.
@@ -211,6 +211,19 @@ public void testKeyExpression(CacheableService<?> service) throws Exception {
 		assertNotSame(r3, r4);
 	}
 
+	public void testVarArgsKey(CacheableService<?> service) throws Exception {
+		Object r1 = service.varArgsKey(1, 2, 3);
+		Object r2 = service.varArgsKey(1, 2, 3);
+
+		assertSame(r1, r2);
+
+		Object r3 = service.varArgsKey(1, 2, 3);
+		Object r4 = service.varArgsKey(1, 2);
+
+		assertNotSame(r3, r4);
+	}
+
+
 	public void testNullValue(CacheableService<?> service) throws Exception {
 		Object key = new Object();
 		assertNull(service.nullValue(key));
@@ -478,6 +491,11 @@ public void testKeyExpression() throws Exception {
 		testKeyExpression(cs);
 	}
 
+	@Test
+	public void testVarArgsKey() throws Exception {
+		testVarArgsKey(cs);
+	}
+
 	@Test
 	public void testClassCacheCacheable() throws Exception {
 		testCacheable(ccs);
diff --git a/spring-context/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java b/spring-context/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java
@@ -89,6 +89,12 @@ public Object key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
 
+	@Override
+	@Cacheable(value = "default")
+	public Object varArgsKey(Object... args) {
+		return counter.getAndIncrement();
+	}
+
 	@Override
 	@Cacheable(value = "default", key = "#root.methodName + #root.caches[0].name")
 	public Object name(Object arg1) {
diff --git a/spring-context/src/test/java/org/springframework/cache/config/CacheableService.java b/spring-context/src/test/java/org/springframework/cache/config/CacheableService.java
@@ -44,6 +44,8 @@ public interface CacheableService<T> {
 
 	T key(Object arg1, Object arg2);
 
+	T varArgsKey(Object... args);
+
 	T name(Object arg1);
 
 	T nullValue(Object arg1);
diff --git a/spring-context/src/test/java/org/springframework/cache/config/DefaultCacheableService.java b/spring-context/src/test/java/org/springframework/cache/config/DefaultCacheableService.java
@@ -91,6 +91,12 @@ public Long key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
 
+	@Override
+	@Cacheable(value = "default")
+	public Long varArgsKey(Object... args) {
+		return counter.getAndIncrement();
+	}
+
 	@Override
 	@Cacheable(value = "default", key = "#root.methodName")
 	public Long name(Object arg1) {
diff --git a/spring-context/src/test/resources/org/springframework/cache/config/cache-advice.xml b/spring-context/src/test/resources/org/springframework/cache/config/cache-advice.xml
@@ -13,6 +13,7 @@
 			<cache:cacheable method="conditional" condition="#classField == 3"/>
 			<cache:cacheable method="unless" unless="#result > 10"/>
 			<cache:cacheable method="key" key="#p0"/>
+			<cache:cacheable method="varArgsKey"/>
 			<cache:cacheable method="nam*" key="#root.methodName"/>
 			<cache:cacheable method="rootVars" key="#root.methodName + #root.method.name + #root.targetClass + #root.target"/>
 			<cache:cacheable method="nullValue" cache="default"/>
@@ -52,6 +53,7 @@
 	<cache:advice id="cacheAdviceClass" cache-manager="cacheManager" key-generator="keyGenerator">
 		<cache:caching cache="default">
 			<cache:cacheable method="key" key="#p0"/>
+			<cache:cacheable method="varArgsKey"/>
 			<cache:cacheable method="nam*" key="#root.methodName + #root.caches[0].name"/>
 			<cache:cacheable method="rootVars" key="#root.methodName + #root.method.name + #root.targetClass + #root.target"/>
 			<cache:cacheable method="cache"/>
