From 9850614a5d608f4e6c463b841a1976a100e2769a Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 3 Sep 2019 13:34:29 +0200 Subject: [PATCH 1/3] DATAREDIS-1032 - Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 6e446970fc..a84774ee44 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 2.2.0.BUILD-SNAPSHOT + 2.2.0.DATAREDIS-1032-SNAPSHOT Spring Data Redis From 897d39507470b4e7734ae39100052997a1fe1477 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 3 Sep 2019 12:12:13 +0200 Subject: [PATCH 2/3] DATAREDIS-1032 - Fix CacheKey conversions for List, Map and Arrays. --- .../data/redis/cache/RedisCache.java | 58 +++++++++++++++++-- .../data/redis/cache/RedisCacheTests.java | 55 +++++++++++++++++- 2 files changed, 107 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCache.java b/src/main/java/org/springframework/data/redis/cache/RedisCache.java index e6bcb0eb41..607b032ec2 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCache.java +++ b/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. - *

+ *

* 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 provide a suitable Converter via 'RedisCacheConfiguration.withConversionService(...)' 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) { diff --git a/src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java b/src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java index 4a5c971ee8..dcfdaa2ece 100644 --- a/src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java +++ b/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 callback) { RedisConnection connection = connectionFactory.getConnection(); try { From d3f1ec6ef3246c8495005d9a3bcbecdc8155f490 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 3 Sep 2019 13:04:57 +0200 Subject: [PATCH 3/3] DATAREDIS-1032 - Improve cache key converter registration. --- .../data/redis/cache/RedisCache.java | 2 +- .../redis/cache/RedisCacheConfiguration.java | 33 +++++++++++++++++++ .../RedisCacheConfigurationUnitTests.java | 24 ++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCache.java b/src/main/java/org/springframework/data/redis/cache/RedisCache.java index 607b032ec2..9331e8a3d8 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCache.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCache.java @@ -309,7 +309,7 @@ protected String convertKey(Object key) { } throw new IllegalStateException(String.format( - "Cannot convert cache key %s to String. Please provide a suitable Converter via 'RedisCacheConfiguration.withConversionService(...)' or override '%s.toString()'.", + "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")); } diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java b/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java index 3e7ba074d9..5ee530116c 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java +++ b/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 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 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: *