From b4e75e5f96abc12cfde5eab38e362fefa3d9a311 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 12 May 2021 11:56:22 +0200 Subject: [PATCH] Guard non-initialized RedisConnectionFactory.getConnection() with IllegalStateException. Obtaining a Connection from RedisConnectionFactory and its reactive variant is now guarded by IllegalStateException that is thrown if the connection factory was not yet initialized. Previously, connections could be obtained where configuration was partially applied. Closes #2057 --- .../ReactiveRedisConnectionFactory.java | 6 ++-- .../connection/RedisConnectionFactory.java | 6 ++++ .../jedis/JedisConnectionFactory.java | 21 +++++++++++++ .../lettuce/LettuceConnectionFactory.java | 31 +++++++++++++++++++ ...ectionFactorySentinelIntegrationTests.java | 1 + .../JedisConnectionFactoryUnitTests.java | 10 ++++++ .../LettuceConnectionFactoryUnitTests.java | 14 ++++++++- 7 files changed, 86 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/ReactiveRedisConnectionFactory.java b/src/main/java/org/springframework/data/redis/connection/ReactiveRedisConnectionFactory.java index e9bd578f9..af3eb469e 100644 --- a/src/main/java/org/springframework/data/redis/connection/ReactiveRedisConnectionFactory.java +++ b/src/main/java/org/springframework/data/redis/connection/ReactiveRedisConnectionFactory.java @@ -32,13 +32,15 @@ public interface ReactiveRedisConnectionFactory extends PersistenceExceptionTran /** * @return a reactive Redis connection. - * @since 2.0. + * @throws IllegalStateException if the connection factory requires initialization and the factory was not yet + * initialized. */ ReactiveRedisConnection getReactiveConnection(); /** * @return a reactive Redis Cluster connection. - * @since 2.0 + * @throws IllegalStateException if the connection factory requires initialization and the factory was not yet + * initialized. */ ReactiveRedisClusterConnection getReactiveClusterConnection(); } diff --git a/src/main/java/org/springframework/data/redis/connection/RedisConnectionFactory.java b/src/main/java/org/springframework/data/redis/connection/RedisConnectionFactory.java index bcc5d7f07..85cb649c3 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisConnectionFactory.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisConnectionFactory.java @@ -30,6 +30,8 @@ public interface RedisConnectionFactory extends PersistenceExceptionTranslator { * Provides a suitable connection for interacting with Redis. * * @return connection for interacting with Redis. + * @throws IllegalStateException if the connection factory requires initialization and the factory was not yet + * initialized. */ RedisConnection getConnection(); @@ -37,6 +39,8 @@ public interface RedisConnectionFactory extends PersistenceExceptionTranslator { * Provides a suitable connection for interacting with Redis Cluster. * * @return + * @throws IllegalStateException if the connection factory requires initialization and the factory was not yet + * initialized. * @since 1.7 */ RedisClusterConnection getClusterConnection(); @@ -55,6 +59,8 @@ public interface RedisConnectionFactory extends PersistenceExceptionTranslator { * Provides a suitable connection for interacting with Redis Sentinel. * * @return connection for interacting with Redis Sentinel. + * @throws IllegalStateException if the connection factory requires initialization and the factory was not yet + * initialized. * @since 1.4 */ RedisSentinelConnection getSentinelConnection(); diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java index 397b4ab18..f09edff5a 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java @@ -73,6 +73,9 @@ import org.springframework.util.StringUtils; *
  • {@link RedisSentinelConfiguration}
    • *
  • {@link RedisClusterConfiguration}
    • * + *

    + * This connection factory must be {@link #afterPropertiesSet() initialized} prior to {@link #getConnection obtaining + * connections}. * * @author Costin Leau * @author Thomas Darimont @@ -104,6 +107,9 @@ public class JedisConnectionFactory implements InitializingBean, DisposableBean, private @Nullable ClusterTopologyProvider topologyProvider; private @Nullable ClusterCommandExecutor clusterCommandExecutor; + private boolean initialized; + private boolean destroyed; + /** * Constructs a new JedisConnectionFactory instance with default settings (default connection pooling, no * shard information). @@ -350,6 +356,8 @@ public class JedisConnectionFactory implements InitializingBean, DisposableBean, new JedisClusterConnection.JedisClusterNodeResourceProvider(this.cluster, this.topologyProvider), EXCEPTION_TRANSLATION); } + + this.initialized = true; } private JedisClientConfig createClientConfig(@Nullable String username, RedisPassword password) { @@ -484,6 +492,8 @@ public class JedisConnectionFactory implements InitializingBean, DisposableBean, log.warn("Cannot properly close cluster command executor", ex); } } + + this.destroyed = true; } /* @@ -492,6 +502,8 @@ public class JedisConnectionFactory implements InitializingBean, DisposableBean, */ public RedisConnection getConnection() { + assertInitialized(); + if (isRedisClusterAware()) { return getClusterConnection(); } @@ -517,6 +529,8 @@ public class JedisConnectionFactory implements InitializingBean, DisposableBean, @Override public RedisClusterConnection getClusterConnection() { + assertInitialized(); + if (!isRedisClusterAware()) { throw new InvalidDataAccessApiUsageException("Cluster is not configured!"); } @@ -876,6 +890,8 @@ public class JedisConnectionFactory implements InitializingBean, DisposableBean, @Override public RedisSentinelConnection getSentinelConnection() { + assertInitialized(); + if (!isRedisSentinelAware()) { throw new InvalidDataAccessResourceUsageException("No Sentinels configured"); } @@ -945,6 +961,11 @@ public class JedisConnectionFactory implements InitializingBean, DisposableBean, return (MutableJedisClientConfiguration) clientConfiguration; } + private void assertInitialized() { + Assert.state(this.initialized, "JedisConnectionFactory was not initialized through afterPropertiesSet()"); + Assert.state(!this.destroyed, "JedisConnectionFactory was destroyed and cannot be used anymore"); + } + /** * Mutable implementation of {@link JedisClientConfiguration}. * diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java index 5f2ab8cfc..e04f45bac 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java @@ -84,6 +84,9 @@ import org.springframework.util.StringUtils; *

  • {@link RedisSentinelConfiguration}
    • *
  • {@link RedisClusterConfiguration}
    • * + *

    + * This connection factory must be {@link #afterPropertiesSet() initialized} prior to {@link #getConnection obtaining + * connections}. * * @author Costin Leau * @author Jennifer Hickey @@ -124,6 +127,9 @@ public class LettuceConnectionFactory private @Nullable ClusterCommandExecutor clusterCommandExecutor; + private boolean initialized; + private boolean destroyed; + /** * Constructs a new {@link LettuceConnectionFactory} instance with default settings. */ @@ -293,6 +299,8 @@ public class LettuceConnectionFactory EXCEPTION_TRANSLATION); } + this.initialized = true; + if (getEagerInitialization() && getShareNativeConnection()) { initConnection(); } @@ -329,6 +337,8 @@ public class LettuceConnectionFactory log.warn("Cannot properly close cluster command executor", ex); } } + + this.destroyed = true; } private void dispose(LettuceConnectionProvider connectionProvider) { @@ -351,6 +361,8 @@ public class LettuceConnectionFactory */ public RedisConnection getConnection() { + assertInitialized(); + if (isClusterAware()) { return getClusterConnection(); } @@ -368,6 +380,8 @@ public class LettuceConnectionFactory @Override public RedisClusterConnection getClusterConnection() { + assertInitialized(); + if (!isClusterAware()) { throw new InvalidDataAccessApiUsageException("Cluster is not configured!"); } @@ -437,6 +451,8 @@ public class LettuceConnectionFactory @Override public LettuceReactiveRedisConnection getReactiveConnection() { + assertInitialized(); + if (isClusterAware()) { return getReactiveClusterConnection(); } @@ -453,6 +469,8 @@ public class LettuceConnectionFactory @Override public LettuceReactiveRedisClusterConnection getReactiveClusterConnection() { + assertInitialized(); + if (!isClusterAware()) { throw new InvalidDataAccessApiUsageException("Cluster is not configured!"); } @@ -481,6 +499,8 @@ public class LettuceConnectionFactory */ public void resetConnection() { + assertInitialized(); + Optionals.toStream(Optional.ofNullable(connection), Optional.ofNullable(reactiveConnection)) .forEach(SharedConnection::resetConnection); @@ -496,6 +516,8 @@ public class LettuceConnectionFactory */ public void validateConnection() { + assertInitialized(); + getOrCreateSharedConnection().validateConnection(); getOrCreateSharedReactiveConnection().validateConnection(); } @@ -801,6 +823,7 @@ public class LettuceConnectionFactory */ @Nullable public AbstractRedisClient getNativeClient() { + assertInitialized(); return this.client; } @@ -1167,6 +1190,11 @@ public class LettuceConnectionFactory return redisUri; } + private void assertInitialized() { + Assert.state(this.initialized, "LettuceConnectionFactory was not initialized through afterPropertiesSet()"); + Assert.state(!this.destroyed, "LettuceConnectionFactory was destroyed and cannot be used anymore"); + } + private static void applyToAll(RedisURI source, Consumer action) { action.accept(source); @@ -1214,6 +1242,9 @@ public class LettuceConnectionFactory @Override public RedisSentinelConnection getSentinelConnection() { + + assertInitialized(); + return new LettuceSentinelConnection(connectionProvider); } diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactorySentinelIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactorySentinelIntegrationTests.java index eddbd8464..da4e7fc17 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactorySentinelIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactorySentinelIntegrationTests.java @@ -90,6 +90,7 @@ class JedisConnectionFactorySentinelIntegrationTests { .sentinel("127.0.0.1", 1).sentinel("127.0.0.1", 26379); factory = new JedisConnectionFactory(oneDownSentinelConfig); + factory.afterPropertiesSet(); assertThat(factory.getSentinelConnection().isOpen()).isTrue(); } } diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactoryUnitTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactoryUnitTests.java index ae795fd84..3d5ec4f4e 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactoryUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactoryUnitTests.java @@ -346,6 +346,16 @@ class JedisConnectionFactoryUnitTests { assertThatIllegalStateException().isThrownBy(() -> connectionFactory.setClientName("foo")); } + @Test // GH-2057 + void getConnectionShouldFailIfNotInitialized() { + + JedisConnectionFactory connectionFactory = new JedisConnectionFactory(); + + assertThatIllegalStateException().isThrownBy(connectionFactory::getConnection); + assertThatIllegalStateException().isThrownBy(connectionFactory::getClusterConnection); + assertThatIllegalStateException().isThrownBy(connectionFactory::getSentinelConnection); + } + private JedisConnectionFactory initSpyedConnectionFactory(RedisSentinelConfiguration sentinelConfig, JedisPoolConfig poolConfig) { diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java index fef616f89..3a36f849d 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java @@ -1011,7 +1011,19 @@ class LettuceConnectionFactoryUnitTests { LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(); assertThatIllegalStateException().isThrownBy(connectionFactory::getRequiredNativeClient) - .withMessageContaining("Client not yet initialized"); + .withMessageContaining("was not initialized through"); + } + + @Test // GH-2057 + void getConnectionShouldFailIfNotInitialized() { + + LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(); + + assertThatIllegalStateException().isThrownBy(connectionFactory::getConnection); + assertThatIllegalStateException().isThrownBy(connectionFactory::getClusterConnection); + assertThatIllegalStateException().isThrownBy(connectionFactory::getSentinelConnection); + assertThatIllegalStateException().isThrownBy(connectionFactory::getReactiveConnection); + assertThatIllegalStateException().isThrownBy(connectionFactory::getReactiveClusterConnection); } @Data