Refine RedisCollectionFactoryBean collection creation.

We now cross-check the existing key type against the specified CollectionType to avoid collection creation that doesn't match the configured CollectionType. If the existing key type doesn't match the configured CollectionType, collection creation fails with a validation error.

Closes #2633
Original Pull Request: #2637

Author: mp911de

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,60 +30,88 @@
 
 /**
  * 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<RedisStore>, BeanNameAware, InitializingBean {
 
 	/**
 	 * 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<CollectionType> ifNotFound) {
+
+			for (CollectionType collectionType : values()) {
+				if (collectionType.dataType() == dataType) {
+					return collectionType;
+				}
+			}
+
+			return ifNotFound.get();
+		}
 	}
 
-	private @Nullable Lazy<RedisStore> store;
-	private @Nullable CollectionType type = null;
+	private @Nullable CollectionType type;
 	private @Nullable RedisTemplate<String, ?> template;
 	private @Nullable String key;
 	private @Nullable String beanName;
 
+	private @Nullable Lazy<RedisStore> 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<String, ?> 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();
 	}
 

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'");
 
 	}
 }