DATAREDIS-1032 - Fix CacheKey conversions for List, Map and Arrays.

pom.xml

@@ -5,7 +5,7 @@
 
 	<groupId>org.springframework.data</groupId>
 	<artifactId>spring-data-redis</artifactId>
-	<version>2.2.0.BUILD-SNAPSHOT</version>
+	<version>2.2.0.DATAREDIS-1032-SNAPSHOT</version>
 
 	<name>Spring Data Redis</name>
 

src/main/java/org/springframework/data/redis/cache/RedisCache.java

@@ -17,11 +17,17 @@
 
 import java.lang.reflect.Method;
 import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.StringJoiner;
 import java.util.concurrent.Callable;
 
 import org.springframework.cache.support.AbstractValueAdaptingCache;
 import org.springframework.cache.support.NullValue;
 import org.springframework.cache.support.SimpleValueWrapper;
+import org.springframework.core.convert.ConversionFailedException;
 import org.springframework.core.convert.ConversionService;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.data.redis.serializer.RedisSerializer;
@@ -33,14 +39,14 @@
 
 /**
  * {@link org.springframework.cache.Cache} implementation using for Redis as underlying store.
- * <p />
+ * <p/>
  * Use {@link RedisCacheManager} to create {@link RedisCache} instances.
  *
  * @author Christoph Strobl
  * @author Mark Paluch
- * @since 2.0
  * @see RedisCacheConfiguration
  * @see RedisCacheWriter
+ * @since 2.0
  */
 public class RedisCache extends AbstractValueAdaptingCache {
 
@@ -281,8 +287,19 @@ protected String createCacheKey(Object key) {
 	protected String convertKey(Object key) {
 
 		TypeDescriptor source = TypeDescriptor.forObject(key);
+
 		if (conversionService.canConvert(source, TypeDescriptor.valueOf(String.class))) {
-			return conversionService.convert(key, String.class);
+			try {
+				return conversionService.convert(key, String.class);
+			} catch (ConversionFailedException e) {
+
+				// may fail if the given key is a collection
+				if (isCollectionLikeOrMap(source)) {
+					return convertCollectionLikeOrMapKey(key, source);
+				}
+
+				throw e;
+			}
 		}
 
 		Method toString = ReflectionUtils.findMethod(key.getClass(), "toString");
@@ -291,8 +308,39 @@ protected String convertKey(Object key) {
 			return key.toString();
 		}
 
-		throw new IllegalStateException(
-				String.format("Cannot convert %s to String. Register a Converter or override toString().", source));
+		throw new IllegalStateException(String.format(
+				"Cannot convert cache key %s to String. Please register a suitable Converter via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'.",
+				source, key != null ? key.getClass().getSimpleName() : "Object"));
+	}
+
+	@Nullable
+	private String convertCollectionLikeOrMapKey(Object key, TypeDescriptor source) {
+
+		if (source.isMap()) {
+
+			String target = "{";
+			for (Entry<?, ?> entry : ((Map<?, ?>) key).entrySet()) {
+				target += (convertKey(entry.getKey()) + "=" + convertKey(entry.getValue()));
+			}
+			target += "}";
+			return target;
+		} else if (source.isCollection() || source.isArray()) {
+
+			StringJoiner sj = new StringJoiner(",");
+
+			Collection<?> collection = source.isCollection() ? (Collection<?>) key
+					: Arrays.asList(ObjectUtils.toObjectArray(key));
+
+			for (Object val : collection) {
+				sj.add(convertKey(val));
+			}
+			return "[" + sj.toString() + "]";
+		}
+		return null;
+	}
+
+	private boolean isCollectionLikeOrMap(TypeDescriptor source) {
+		return source.isArray() || source.isCollection() || source.isMap();
 	}
 
 	private byte[] createAndConvertCacheKey(Object key) {

src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java

@@ -18,10 +18,12 @@
 import java.nio.charset.StandardCharsets;
 import java.time.Duration;
 import java.util.Optional;
+import java.util.function.Consumer;
 
 import org.springframework.cache.Cache;
 import org.springframework.cache.interceptor.SimpleKey;
 import org.springframework.core.convert.ConversionService;
+import org.springframework.core.convert.converter.Converter;
 import org.springframework.core.convert.converter.ConverterRegistry;
 import org.springframework.data.redis.serializer.RedisSerializationContext.SerializationPair;
 import org.springframework.data.redis.serializer.RedisSerializer;
@@ -303,6 +305,37 @@ public ConversionService getConversionService() {
 		return conversionService;
 	}
 
+	/**
+	 * Add a {@link Converter} for extracting the {@link String} representation of a cache key if no suitable
+	 * {@link Object#toString()} method is present.
+	 *
+	 * @param cacheKeyConverter
+	 * @throws IllegalStateException if {@link #getConversionService()} does not allow converter registration.
+	 * @since 2.2
+	 */
+	public void addCacheKeyConverter(Converter<?, String> cacheKeyConverter) {
+		configureKeyConverters(it -> it.addConverter(cacheKeyConverter));
+	}
+
+	/**
+	 * Configure the underlying conversion system used to extract the cache key.
+	 *
+	 * @param registryConsumer never {@literal null}.
+	 * @throws IllegalStateException if {@link #getConversionService()} does not allow converter registration.
+	 * @since 2.2
+	 */
+	public void configureKeyConverters(Consumer<ConverterRegistry> registryConsumer) {
+
+		if (!(getConversionService() instanceof ConverterRegistry)) {
+			throw new IllegalStateException(String.format(
+					"'%s' returned by getConversionService() does not allow converter registration." //
+							+ " Please make sure to provide a ConversionService that implements ConverterRegistry.",
+					getConversionService().getClass().getSimpleName()));
+		}
+
+		registryConsumer.accept((ConverterRegistry) getConversionService());
+	}
+
 	/**
 	 * Registers default cache key converters. The following converters get registered:
 	 * <ul>

src/test/java/org/springframework/data/redis/cache/RedisCacheConfigurationUnitTests.java

@@ -19,7 +19,9 @@
 
 import org.junit.Test;
 import org.springframework.beans.DirectFieldAccessor;
+import org.springframework.core.convert.converter.Converter;
 import org.springframework.instrument.classloading.ShadowingClassLoader;
+import org.springframework.lang.Nullable;
 
 /**
  * Unit tests for {@link RedisCacheConfiguration}.
@@ -43,4 +45,26 @@ public void shouldSetClassLoader() {
 
 		assertThat(usedClassLoader).isSameAs(classLoader);
 	}
+
+	@Test // DATAREDIS-1032
+	public void shouldAllowConverterRegistration() {
+
+		RedisCacheConfiguration config = RedisCacheConfiguration.defaultCacheConfig();
+		config.configureKeyConverters(registry -> registry.addConverter(new DomainTypeConverter()));
+
+		assertThat(config.getConversionService().canConvert(DomainType.class, String.class)).isTrue();
+	}
+
+	private static class DomainType {
+
+	}
+
+	static class DomainTypeConverter implements Converter<DomainType, String> {
+
+		@Nullable
+		@Override
+		public String convert(DomainType source) {
+			return null;
+		}
+	}
 }

src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java

@@ -25,6 +25,7 @@
 import java.io.Serializable;
 import java.nio.charset.Charset;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Date;
 import java.util.function.Consumer;
 
@@ -36,11 +37,11 @@
 import org.junit.runners.Parameterized.Parameters;
 import org.springframework.cache.Cache.ValueWrapper;
 import org.springframework.cache.interceptor.SimpleKey;
+import org.springframework.cache.interceptor.SimpleKeyGenerator;
 import org.springframework.cache.support.NullValue;
 import org.springframework.data.redis.ConnectionFactoryTracker;
 import org.springframework.data.redis.connection.RedisConnection;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
-import org.springframework.data.redis.serializer.JdkSerializationRedisSerializer;
 import org.springframework.data.redis.serializer.RedisSerializationContext.SerializationPair;
 import org.springframework.data.redis.serializer.RedisSerializer;
 
@@ -300,6 +301,58 @@ public void fetchKeyWithComputedPrefixReturnsExpectedResult() {
 		assertThat(result.get()).isEqualTo(sample);
 	}
 
+	@Test // DATAREDIS-1032
+	public void cacheShouldAllowListKeyCacheKeysOfSimpleTypes() {
+
+		Object key = SimpleKeyGenerator.generateKey(Collections.singletonList("my-cache-key-in-a-list"));
+		cache.put(key, sample);
+
+		Object target = cache.get(SimpleKeyGenerator.generateKey(Collections.singletonList("my-cache-key-in-a-list")));
+		assertThat(((ValueWrapper) target).get()).isEqualTo(sample);
+	}
+
+	@Test // DATAREDIS-1032
+	public void cacheShouldAllowArrayKeyCacheKeysOfSimpleTypes() {
+
+		Object key = SimpleKeyGenerator.generateKey(new String[] { "my-cache-key-in-an-array" });
+		cache.put(key, sample);
+
+		Object target = cache.get(SimpleKeyGenerator.generateKey(new String[] { "my-cache-key-in-an-array" }));
+		assertThat(((ValueWrapper) target).get()).isEqualTo(sample);
+	}
+
+	@Test // DATAREDIS-1032
+	public void cacheShouldAllowListCacheKeysOfComplexTypes() {
+
+		Object key = SimpleKeyGenerator
+				.generateKey(Collections.singletonList(new ComplexKey(sample.getFirstame(), sample.getBirthdate())));
+		cache.put(key, sample);
+
+		Object target = cache.get(SimpleKeyGenerator
+				.generateKey(Collections.singletonList(new ComplexKey(sample.getFirstame(), sample.getBirthdate()))));
+		assertThat(((ValueWrapper) target).get()).isEqualTo(sample);
+	}
+
+	@Test // DATAREDIS-1032
+	public void cacheShouldAllowMapCacheKeys() {
+
+		Object key = SimpleKeyGenerator
+				.generateKey(Collections.singletonMap("map-key", new ComplexKey(sample.getFirstame(), sample.getBirthdate())));
+		cache.put(key, sample);
+
+		Object target = cache.get(SimpleKeyGenerator
+				.generateKey(Collections.singletonMap("map-key", new ComplexKey(sample.getFirstame(), sample.getBirthdate()))));
+		assertThat(((ValueWrapper) target).get()).isEqualTo(sample);
+	}
+
+	@Test // DATAREDIS-1032
+	public void cacheShouldFailOnNonConvertibleCacheKey() {
+
+		Object key = SimpleKeyGenerator
+				.generateKey(Collections.singletonList(new InvalidKey(sample.getFirstame(), sample.getBirthdate())));
+		assertThatExceptionOfType(IllegalStateException.class).isThrownBy(() -> cache.put(key, sample));
+	}
+
 	void doWithConnection(Consumer<RedisConnection> callback) {
 		RedisConnection connection = connectionFactory.getConnection();
 		try {