diff --git a/pom.xml b/pom.xml index a67dc48660..9d9624b813 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 3.2.0-SNAPSHOT + 3.2.0-GH-2633-SNAPSHOT Spring Data Redis Spring Data module for Redis diff --git a/src/main/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBean.java b/src/main/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBean.java index 3c0f8ca537..376181c1fd 100644 --- a/src/main/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBean.java +++ b/src/main/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBean.java @@ -15,10 +15,13 @@ */ package org.springframework.data.redis.support.collections; +import java.util.function.Supplier; + import org.springframework.beans.factory.BeanNameAware; import org.springframework.beans.factory.InitializingBean; import org.springframework.beans.factory.SmartFactoryBean; import org.springframework.data.redis.connection.DataType; +import org.springframework.data.redis.core.RedisOperations; import org.springframework.data.redis.core.RedisTemplate; import org.springframework.data.util.Lazy; import org.springframework.lang.Nullable; @@ -27,11 +30,13 @@ /** * Factory bean that facilitates creation of Redis-based collections. Supports list, set, zset (or sortedSet), map (or - * hash) and properties. Will use the key type if it exists or to create a dedicated collection (Properties vs Map). - * Otherwise uses the provided type (default is list). + * hash) and properties. Uses the key and {@link CollectionType} to determine what collection type to use. The factory + * verifies the key type if a {@link CollectionType} is specified. Defaults to {@link CollectionType#LIST}. * * @author Costin Leau * @author Christoph Strobl + * @author Mark Paluch + * @see RedisStore */ public class RedisCollectionFactoryBean implements SmartFactoryBean, BeanNameAware, InitializingBean { @@ -39,48 +44,74 @@ public class RedisCollectionFactoryBean implements SmartFactoryBean, * Collection types supported by this factory. * * @author Costin Leau + * @author Mark Paluch */ public enum CollectionType { LIST { + @Override public DataType dataType() { return DataType.LIST; } }, SET { + @Override public DataType dataType() { return DataType.SET; } }, ZSET { + @Override public DataType dataType() { return DataType.ZSET; } }, MAP { + @Override public DataType dataType() { return DataType.HASH; } }, PROPERTIES { + @Override public DataType dataType() { return DataType.HASH; } }; abstract DataType dataType(); + + /** + * Attempt to find a {@link CollectionType} by {@link DataType}. Defaults to {@link Supplier ifNotFound} when + * {@code dataType} is {@literal null} or the collection type cannot be determined. + * + * @param dataType the {@link DataType} to look up. + * @param ifNotFound supplier for a default value. + * @since 3.2 + */ + static CollectionType findCollectionType(@Nullable DataType dataType, Supplier ifNotFound) { + + for (CollectionType collectionType : values()) { + if (collectionType.dataType() == dataType) { + return collectionType; + } + } + + return ifNotFound.get(); + } } - private @Nullable Lazy store; - private @Nullable CollectionType type = null; + private @Nullable CollectionType type; private @Nullable RedisTemplate template; private @Nullable String key; private @Nullable String beanName; + private @Nullable Lazy store; + @Override public void afterPropertiesSet() { @@ -93,46 +124,40 @@ public void afterPropertiesSet() { store = Lazy.of(() -> { - DataType dt = template.type(key); + DataType keyType = template.type(key); // can't create store - Assert.isTrue(!DataType.STRING.equals(dt), "Cannot create store on keys of type 'string'"); + Assert.isTrue(!DataType.STREAM.equals(keyType), "Cannot create store on keys of type 'STREAM'"); - RedisStore tmp = createStore(dt); + if (this.type == null) { + this.type = CollectionType.findCollectionType(keyType, () -> CollectionType.LIST); + } - if (tmp == null) { - if (type == null) { - type = CollectionType.LIST; - } - tmp = createStore(type.dataType()); + if (keyType != null && DataType.NONE != keyType && this.type.dataType() != keyType) { + throw new IllegalArgumentException( + String.format("Cannot create collection type '%s' for a key containing '%s'", this.type, keyType)); } - return tmp; + + return createStore(this.type, key, template); }); } - @SuppressWarnings("unchecked") - private RedisStore createStore(DataType dt) { - switch (dt) { - case LIST: - return RedisList.create(key, template); - - case SET: - return new DefaultRedisSet(key, template); - - case ZSET: - return RedisZSet.create(key, template); + private RedisStore createStore(CollectionType collectionType, String key, RedisOperations operations) { - case HASH: - if (CollectionType.PROPERTIES.equals(type)) { - return new RedisProperties(key, template); - } - return new DefaultRedisMap(key, template); - } - return null; + return switch (collectionType) { + case LIST -> RedisList.create(key, operations); + case SET -> new DefaultRedisSet<>(key, operations); + case ZSET -> RedisZSet.create(key, operations); + case PROPERTIES -> new RedisProperties(key, operations); + case MAP -> new DefaultRedisMap<>(key, operations); + }; } @Override public RedisStore getObject() { + + Assert.state(store != null, + "RedisCollectionFactoryBean is not initialized. Ensure to initialize this factory by calling afterPropertiesSet() before obtaining the factory object."); return store.get(); } diff --git a/src/test/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBeanTests.java b/src/test/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBeanTests.java index 1ef51d63b2..3af400c75f 100644 --- a/src/test/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBeanTests.java +++ b/src/test/java/org/springframework/data/redis/support/collections/RedisCollectionFactoryBeanTests.java @@ -17,9 +17,11 @@ import static org.assertj.core.api.Assertions.*; +import java.util.Map; + import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; - import org.springframework.data.redis.ObjectFactory; import org.springframework.data.redis.StringObjectFactory; import org.springframework.data.redis.connection.jedis.JedisConnectionFactory; @@ -30,7 +32,10 @@ import org.springframework.data.redis.test.extension.RedisStanalone; /** + * Integration tests for {@link RedisCollectionFactoryBean}. + * * @author Costin Leau + * @author Mark Paluch */ public class RedisCollectionFactoryBeanTests { @@ -45,6 +50,12 @@ public class RedisCollectionFactoryBeanTests { this.template = new StringRedisTemplate(jedisConnFactory); } + @BeforeEach + void setUp() { + this.template.delete("key"); + this.template.delete("nosrt"); + } + @AfterEach void tearDown() throws Exception { // clean up the whole db @@ -86,8 +97,30 @@ void testNone() throws Exception { assertThat(store).isInstanceOf(DefaultRedisList.class); } + @Test // GH-2633 + void testExisting() { + + template.delete("key"); + template.opsForHash().put("key", "k", "v"); + + assertThat(createCollection("key")).isInstanceOf(DefaultRedisMap.class); + assertThat(createCollection("key", CollectionType.MAP)).isInstanceOf(DefaultRedisMap.class); + + template.delete("key"); + template.opsForSet().add("key", "1", "2"); + + assertThat(createCollection("key")).isInstanceOf(DefaultRedisSet.class); + assertThat(createCollection("key", CollectionType.SET)).isInstanceOf(DefaultRedisSet.class); + + template.delete("key"); + template.opsForList().leftPush("key", "1", "2"); + + assertThat(createCollection("key")).isInstanceOf(DefaultRedisList.class); + assertThat(createCollection("key", CollectionType.LIST)).isInstanceOf(DefaultRedisList.class); + } + @Test - void testExistingCol() throws Exception { + void testExistingCol() { String key = "set"; String val = "value"; @@ -102,6 +135,28 @@ void testExistingCol() throws Exception { col = createCollection(key, CollectionType.PROPERTIES); assertThat(col).isInstanceOf(RedisProperties.class); + } + + @Test // GH-2633 + void testIncompatibleCollections() { + + template.opsForValue().set("key", "value"); + assertThatIllegalArgumentException().isThrownBy(() -> createCollection("key", CollectionType.LIST)) + .withMessageContaining("Cannot create collection type 'LIST' for a key containing 'STRING'"); + + template.delete("key"); + template.opsForList().leftPush("key", "value"); + assertThatIllegalArgumentException().isThrownBy(() -> createCollection("key", CollectionType.SET)) + .withMessageContaining("Cannot create collection type 'SET' for a key containing 'LIST'"); + + } + + @Test // GH-2633 + void shouldFailForStreamCreation() { + + template.opsForStream().add("key", Map.of("k", "v")); + assertThatIllegalArgumentException().isThrownBy(() -> createCollection("key", CollectionType.LIST)) + .withMessageContaining("Cannot create store on keys of type 'STREAM'"); } }