From d231acef1616f5ccf56bf78a32a24f734db7a424 Mon Sep 17 00:00:00 2001 From: ggivo Date: Thu, 26 Jun 2025 10:52:36 +0300 Subject: [PATCH 1/2] Fix: JedisCluster throws NullPointerException when maxAttempts is set to 0 This change adds input validation to the RetryableCommandExecutor constructor to prevent usage with invalid configuration values, specifically when maxAttempts is set to 0. Previously, setting maxAttempts to 0 in JedisCluster would result in a NullPointerException during command execution, due to lastException being uninitialized when no attempt is made. With this fix, an IllegalArgumentException is thrown early if maxAttempts < 1, providing clearer feedback to the user and preventing runtime errors. Added unit tests to verify the validation behavior. Fixes #4185 --- .../executors/ClusterCommandExecutor.java | 9 +- .../executors/RetryableCommandExecutor.java | 9 +- .../clients/jedis/util/JedisAsserts.java | 36 +++++ .../RetryableCommandExecutorTest.java | 138 ++++++++++++++++++ 4 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 src/main/java/redis/clients/jedis/util/JedisAsserts.java create mode 100644 src/test/java/redis/clients/jedis/executors/RetryableCommandExecutorTest.java diff --git a/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java b/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java index 3dbcac0a8c..766e2660c9 100644 --- a/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java +++ b/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java @@ -17,6 +17,7 @@ import redis.clients.jedis.exceptions.*; import redis.clients.jedis.providers.ClusterConnectionProvider; import redis.clients.jedis.util.IOUtils; +import redis.clients.jedis.util.JedisAsserts; public class ClusterCommandExecutor implements CommandExecutor { @@ -28,6 +29,10 @@ public class ClusterCommandExecutor implements CommandExecutor { public ClusterCommandExecutor(ClusterConnectionProvider provider, int maxAttempts, Duration maxTotalRetriesDuration) { + JedisAsserts.notNull(provider, "provider must not be null"); + JedisAsserts.isTrue(maxAttempts > 0, "maxAttempts must be greater than 0"); + JedisAsserts.notNull(maxTotalRetriesDuration, "maxTotalRetriesDuration must not be null"); + this.provider = provider; this.maxAttempts = maxAttempts; this.maxTotalRetriesDuration = maxTotalRetriesDuration; @@ -137,7 +142,9 @@ private T doExecuteCommand(CommandObject commandObject, boolean toReplica JedisClusterOperationException maxAttemptsException = new JedisClusterOperationException("No more cluster attempts left."); - maxAttemptsException.addSuppressed(lastException); + if (lastException != null) { + maxAttemptsException.addSuppressed(lastException); + } throw maxAttemptsException; } diff --git a/src/main/java/redis/clients/jedis/executors/RetryableCommandExecutor.java b/src/main/java/redis/clients/jedis/executors/RetryableCommandExecutor.java index 4351f3fab4..ef4ca92966 100644 --- a/src/main/java/redis/clients/jedis/executors/RetryableCommandExecutor.java +++ b/src/main/java/redis/clients/jedis/executors/RetryableCommandExecutor.java @@ -13,6 +13,7 @@ import redis.clients.jedis.exceptions.JedisException; import redis.clients.jedis.util.IOUtils; import redis.clients.jedis.providers.ConnectionProvider; +import redis.clients.jedis.util.JedisAsserts; public class RetryableCommandExecutor implements CommandExecutor { @@ -24,6 +25,10 @@ public class RetryableCommandExecutor implements CommandExecutor { public RetryableCommandExecutor(ConnectionProvider provider, int maxAttempts, Duration maxTotalRetriesDuration) { + JedisAsserts.notNull(provider, "provider must not be null"); + JedisAsserts.isTrue(maxAttempts > 0, "maxAttempts must be greater than 0"); + JedisAsserts.notNull(maxTotalRetriesDuration, "maxTotalRetriesDuration must not be null"); + this.provider = provider; this.maxAttempts = maxAttempts; this.maxTotalRetriesDuration = maxTotalRetriesDuration; @@ -68,7 +73,9 @@ public final T executeCommand(CommandObject commandObject) { } JedisException maxAttemptsException = new JedisException("No more attempts left."); - maxAttemptsException.addSuppressed(lastException); + if (lastException != null) { + maxAttemptsException.addSuppressed(lastException); + } throw maxAttemptsException; } diff --git a/src/main/java/redis/clients/jedis/util/JedisAsserts.java b/src/main/java/redis/clients/jedis/util/JedisAsserts.java new file mode 100644 index 0000000000..11cb2837ba --- /dev/null +++ b/src/main/java/redis/clients/jedis/util/JedisAsserts.java @@ -0,0 +1,36 @@ +package redis.clients.jedis.util; + +/** + * Assertion utility class that assists in validating arguments. This class is part of the internal API and may change without + * further notice. + * + * @author ivo.gaydazhiev + */ +public class JedisAsserts { + + /** + * Assert that an object is not {@code null} . + * + * @param object the object to check + * @param message the exception message to use if the assertion fails + * @throws IllegalArgumentException if the object is {@code null} + */ + public static void notNull(Object object, String message) { + if (object == null) { + throw new IllegalArgumentException(message); + } + } + + /** + * Assert that {@code value} is {@code true}. + * + * @param value the value to check + * @param message the exception message to use if the assertion fails + * @throws IllegalArgumentException if the object array contains a {@code null} element + */ + public static void isTrue(boolean value, String message) { + if (!value) { + throw new IllegalArgumentException(message); + } + } +} diff --git a/src/test/java/redis/clients/jedis/executors/RetryableCommandExecutorTest.java b/src/test/java/redis/clients/jedis/executors/RetryableCommandExecutorTest.java new file mode 100644 index 0000000000..adff81749e --- /dev/null +++ b/src/test/java/redis/clients/jedis/executors/RetryableCommandExecutorTest.java @@ -0,0 +1,138 @@ +package redis.clients.jedis.executors; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.time.Duration; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import redis.clients.jedis.CommandObject; +import redis.clients.jedis.Connection; +import redis.clients.jedis.exceptions.JedisConnectionException; +import redis.clients.jedis.exceptions.JedisException; +import redis.clients.jedis.providers.ConnectionProvider; + +@ExtendWith(MockitoExtension.class) +public class RetryableCommandExecutorTest { + + @Mock + private ConnectionProvider mockProvider; + + @Mock + private Connection mockConnection; + + @Mock + private CommandObject mockCommandObject; + + @Test + public void testConstructorWithNullProvider() { + IllegalArgumentException exception = assertThrows( + IllegalArgumentException.class, + () -> new RetryableCommandExecutor(null, 3, Duration.ofSeconds(1)), + "Should throw IllegalArgumentException when provider is null" + ); + + assertTrue(exception.getMessage().contains("provider"), + "Exception message should mention 'provider'"); + } + + @Test + public void testConstructorWithInvalidMaxAttempts() { + // Test with zero + IllegalArgumentException exceptionZero = assertThrows( + IllegalArgumentException.class, + () -> new RetryableCommandExecutor(mockProvider, 0, Duration.ofSeconds(1)), + "Should throw IllegalArgumentException when maxAttempts is zero" + ); + + assertTrue(exceptionZero.getMessage().contains("maxAttempts"), + "Exception message should mention 'maxAttempts'"); + + // Test with negative value + IllegalArgumentException exceptionNegative = assertThrows( + IllegalArgumentException.class, + () -> new RetryableCommandExecutor(mockProvider, -1, Duration.ofSeconds(1)), + "Should throw IllegalArgumentException when maxAttempts is negative" + ); + + assertTrue(exceptionNegative.getMessage().contains("maxAttempts"), + "Exception message should mention 'maxAttempts'"); + } + + @Test + public void testValidConstruction() { + // Should not throw any exceptions + assertDoesNotThrow(() -> new RetryableCommandExecutor(mockProvider, 1, Duration.ofSeconds(1))); + assertDoesNotThrow(() -> new RetryableCommandExecutor(mockProvider, 3, Duration.ZERO)); + assertDoesNotThrow(() -> new RetryableCommandExecutor(mockProvider, 10, Duration.ofMinutes(5))); + } + + @Test + public void testMaxAttemptsIsRespected() throws Exception { + // Set up the mock to return a connection but throw an exception when executing + when(mockProvider.getConnection(any())).thenReturn(mockConnection); + when(mockConnection.executeCommand(any(CommandObject.class))).thenThrow(new JedisConnectionException("Connection failed")); + + // Create the executor with exactly 3 attempts + final int maxAttempts = 3; + RetryableCommandExecutor executor = spy(new RetryableCommandExecutor(mockProvider, maxAttempts, Duration.ofSeconds(10))); + + // Mock the sleep method to avoid actual sleeping + doNothing().when(executor).sleep(anyLong()); + + // Execute the command and expect an exception + assertThrows(JedisException.class, () -> executor.executeCommand(mockCommandObject)); + + // Verify that we tried exactly maxAttempts times + verify(mockProvider, times(maxAttempts)).getConnection(any()); + verify(mockConnection, times(maxAttempts)).close(); + } + + @Test + public void testExecuteCommandWithNoRetries() throws Exception { + // Set up the mock to return a connection and have it execute the command successfully + when(mockProvider.getConnection(any())).thenReturn(mockConnection); + when(mockConnection.executeCommand(mockCommandObject)).thenReturn("success"); + + // Create the executor with just 1 attempt (no retries) + RetryableCommandExecutor executor = new RetryableCommandExecutor(mockProvider, 1, Duration.ofSeconds(1)); + + // Execute the command + String result = executor.executeCommand(mockCommandObject); + + // Verify the result and that the connection was closed + assertEquals("success", result); + verify(mockConnection, times(1)).close(); + verify(mockProvider, times(1)).getConnection(any()); + } + + @Test + public void testMaxAttemptsExceeded() throws Exception { + // Set up the mock to return a connection but throw an exception when executing + when(mockProvider.getConnection(any())).thenReturn(mockConnection); + when(mockConnection.executeCommand(any(CommandObject.class))).thenThrow(new JedisConnectionException("Connection failed")); + + // Create the executor with 3 attempts + RetryableCommandExecutor executor = spy(new RetryableCommandExecutor(mockProvider, 3, Duration.ofSeconds(1))); + + // Mock the sleep method to avoid actual sleeping + doNothing().when(executor).sleep(anyLong()); + + // Execute the command and expect an exception + JedisException exception = assertThrows( + JedisException.class, + () -> executor.executeCommand(mockCommandObject), + "Should throw JedisException when max attempts are exceeded" + ); + + // Verify the exception and that we tried the correct number of times + assertEquals("No more attempts left.", exception.getMessage()); + assertEquals(1, exception.getSuppressed().length); + verify(mockProvider, times(3)).getConnection(any()); + verify(mockConnection, times(3)).close(); + } +} \ No newline at end of file From 08452db13b65d744c205c9bdaecef58239fb624e Mon Sep 17 00:00:00 2001 From: ggivo Date: Thu, 26 Jun 2025 13:27:23 +0300 Subject: [PATCH 2/2] Fix API doc JedisAsserts.isTrue --- src/main/java/redis/clients/jedis/util/JedisAsserts.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/redis/clients/jedis/util/JedisAsserts.java b/src/main/java/redis/clients/jedis/util/JedisAsserts.java index 11cb2837ba..70b2f1b527 100644 --- a/src/main/java/redis/clients/jedis/util/JedisAsserts.java +++ b/src/main/java/redis/clients/jedis/util/JedisAsserts.java @@ -26,7 +26,7 @@ public static void notNull(Object object, String message) { * * @param value the value to check * @param message the exception message to use if the assertion fails - * @throws IllegalArgumentException if the object array contains a {@code null} element + * @throws IllegalArgumentException if the value is {@code false} */ public static void isTrue(boolean value, String message) { if (!value) {