From 9fc70fa1f8db135bf271d851572796707509f702 Mon Sep 17 00:00:00 2001 From: Yury-Fridlyand Date: Tue, 16 Apr 2024 16:24:44 -0700 Subject: [PATCH] Java: Add `SMOVE` command. (#1249) * Add `SMOVE` command. (#182) * Add `SMOVE` command. Signed-off-by: Yury-Fridlyand * Update javadoc. Signed-off-by: Yury-Fridlyand * Update javadoc. Signed-off-by: Yury-Fridlyand * PR comments. Signed-off-by: Yury-Fridlyand * PR comments. Signed-off-by: Yury-Fridlyand --------- Signed-off-by: Yury-Fridlyand * PR comments. Signed-off-by: Yury-Fridlyand * PR comments. Signed-off-by: Yury-Fridlyand * Typo fix. Signed-off-by: Yury-Fridlyand * Update UT. Signed-off-by: Yury-Fridlyand * PR comments. Signed-off-by: Yury-Fridlyand * update doc. Signed-off-by: Yury-Fridlyand * update doc. Signed-off-by: Yury-Fridlyand --------- Signed-off-by: Yury-Fridlyand --- glide-core/src/client/value_conversion.rs | 11 +++- glide-core/src/protobuf/redis_request.proto | 1 + glide-core/src/request_type.rs | 3 + .../src/main/java/glide/api/BaseClient.java | 8 +++ .../glide/api/commands/SetBaseCommands.java | 21 +++++++ .../glide/api/models/BaseTransaction.java | 19 ++++++ .../test/java/glide/api/RedisClientTest.java | 25 ++++++++ .../glide/api/models/TransactionTests.java | 4 ++ .../test/java/glide/SharedCommandTests.java | 61 +++++++++++++++++++ .../java/glide/TransactionTestUtilities.java | 3 + 10 files changed, 155 insertions(+), 1 deletion(-) diff --git a/glide-core/src/client/value_conversion.rs b/glide-core/src/client/value_conversion.rs index f903187273..04a91f1ee2 100644 --- a/glide-core/src/client/value_conversion.rs +++ b/glide-core/src/client/value_conversion.rs @@ -213,13 +213,14 @@ fn convert_array_to_map( pub(crate) fn expected_type_for_cmd(cmd: &Cmd) -> Option { let command = cmd.command()?; + // TODO use enum to avoid mistakes match command.as_slice() { b"HGETALL" | b"XREAD" | b"CONFIG GET" | b"FT.CONFIG GET" | b"HELLO" => { Some(ExpectedReturnType::Map) } b"INCRBYFLOAT" | b"HINCRBYFLOAT" => Some(ExpectedReturnType::Double), b"HEXISTS" | b"HSETNX" | b"EXPIRE" | b"EXPIREAT" | b"PEXPIRE" | b"PEXPIREAT" - | b"SISMEMBER" | b"PERSIST" => Some(ExpectedReturnType::Boolean), + | b"SISMEMBER" | b"PERSIST" | b"SMOVE" => Some(ExpectedReturnType::Boolean), b"SMEMBERS" => Some(ExpectedReturnType::Set), b"ZSCORE" => Some(ExpectedReturnType::DoubleOrNull), b"ZPOPMIN" | b"ZPOPMAX" => Some(ExpectedReturnType::MapOfStringToDouble), @@ -325,6 +326,14 @@ mod tests { assert!(expected_type_for_cmd(redis::cmd("ZREVRANK").arg("key").arg("member")).is_none()); } + #[test] + fn convert_smove_to_bool() { + assert!(matches!( + expected_type_for_cmd(redis::cmd("SMOVE").arg("key1").arg("key2").arg("elem")), + Some(ExpectedReturnType::Boolean) + )); + } + #[test] fn test_convert_to_map_of_string_to_double() { assert_eq!( diff --git a/glide-core/src/protobuf/redis_request.proto b/glide-core/src/protobuf/redis_request.proto index efb853028b..5019362b35 100644 --- a/glide-core/src/protobuf/redis_request.proto +++ b/glide-core/src/protobuf/redis_request.proto @@ -154,6 +154,7 @@ enum RequestType { SInterStore = 114; ZRangeStore = 115; GetRange = 116; + SMove = 117; } message Command { diff --git a/glide-core/src/request_type.rs b/glide-core/src/request_type.rs index e058d604f0..119e698a23 100644 --- a/glide-core/src/request_type.rs +++ b/glide-core/src/request_type.rs @@ -122,6 +122,7 @@ pub enum RequestType { SInterStore = 114, ZRangeStore = 115, GetRange = 116, + SMove = 117, } fn get_two_word_command(first: &str, second: &str) -> Cmd { @@ -247,6 +248,7 @@ impl From<::protobuf::EnumOrUnknown> for RequestType { ProtobufRequestType::SInterStore => RequestType::SInterStore, ProtobufRequestType::ZRangeStore => RequestType::ZRangeStore, ProtobufRequestType::GetRange => RequestType::GetRange, + ProtobufRequestType::SMove => RequestType::SMove, } } } @@ -368,6 +370,7 @@ impl RequestType { RequestType::SInterStore => Some(cmd("SINTERSTORE")), RequestType::ZRangeStore => Some(cmd("ZRANGESTORE")), RequestType::GetRange => Some(cmd("GETRANGE")), + RequestType::SMove => Some(cmd("SMOVE")), } } } diff --git a/java/client/src/main/java/glide/api/BaseClient.java b/java/client/src/main/java/glide/api/BaseClient.java index 014b11f457..979664f09e 100644 --- a/java/client/src/main/java/glide/api/BaseClient.java +++ b/java/client/src/main/java/glide/api/BaseClient.java @@ -56,6 +56,7 @@ import static redis_request.RedisRequestOuterClass.RequestType.SInterStore; import static redis_request.RedisRequestOuterClass.RequestType.SIsMember; import static redis_request.RedisRequestOuterClass.RequestType.SMembers; +import static redis_request.RedisRequestOuterClass.RequestType.SMove; import static redis_request.RedisRequestOuterClass.RequestType.SRem; import static redis_request.RedisRequestOuterClass.RequestType.SetRange; import static redis_request.RedisRequestOuterClass.RequestType.SetString; @@ -547,6 +548,13 @@ public CompletableFuture scard(@NonNull String key) { return commandManager.submitNewCommand(SCard, new String[] {key}, this::handleLongResponse); } + @Override + public CompletableFuture smove( + @NonNull String source, @NonNull String destination, @NonNull String member) { + return commandManager.submitNewCommand( + SMove, new String[] {source, destination, member}, this::handleBooleanResponse); + } + @Override public CompletableFuture sinterstore(@NonNull String destination, @NonNull String[] keys) { String[] arguments = ArrayUtils.addFirst(keys, destination); diff --git a/java/client/src/main/java/glide/api/commands/SetBaseCommands.java b/java/client/src/main/java/glide/api/commands/SetBaseCommands.java index bb07577559..b5d54ded84 100644 --- a/java/client/src/main/java/glide/api/commands/SetBaseCommands.java +++ b/java/client/src/main/java/glide/api/commands/SetBaseCommands.java @@ -76,6 +76,27 @@ public interface SetBaseCommands { */ CompletableFuture scard(String key); + /** + * Moves member from the set at source to the set at destination + * , removing it from the source set. Creates a new destination set if needed. The + * operation is atomic. + * + * @apiNote When in cluster mode, source and destination must map to the + * same hash slot. + * @see redis.io for details. + * @param source The key of the set to remove the element from. + * @param destination The key of the set to add the element to. + * @param member The set element to move. + * @return true on success, or false if the source set does + * not exist or the element is not a member of the source set. + * @example + * 
{@code
+     * Boolean moved = client.smove("set1", "set2", "element").get();
+     * assert moved;
+     * }
+ */ + CompletableFuture smove(String source, String destination, String member); + /** * Returns if member is a member of the set stored at key. * diff --git a/java/client/src/main/java/glide/api/models/BaseTransaction.java b/java/client/src/main/java/glide/api/models/BaseTransaction.java index c00613d996..e080e5b544 100644 --- a/java/client/src/main/java/glide/api/models/BaseTransaction.java +++ b/java/client/src/main/java/glide/api/models/BaseTransaction.java @@ -67,6 +67,7 @@ import static redis_request.RedisRequestOuterClass.RequestType.SInterStore; import static redis_request.RedisRequestOuterClass.RequestType.SIsMember; import static redis_request.RedisRequestOuterClass.RequestType.SMembers; +import static redis_request.RedisRequestOuterClass.RequestType.SMove; import static redis_request.RedisRequestOuterClass.RequestType.SRem; import static redis_request.RedisRequestOuterClass.RequestType.SetRange; import static redis_request.RedisRequestOuterClass.RequestType.SetString; @@ -926,6 +927,24 @@ public T scard(@NonNull String key) { return getThis(); } + /** + * Moves member from the set at source to the set at destination + * , removing it from the source set. Creates a new destination set if needed. The + * operation is atomic. + * + * @see redis.io for details. + * @param source The key of the set to remove the element from. + * @param destination The key of the set to add the element to. + * @param member The set element to move. + * @return Command response - true on success, or false if the + * source set does not exist or the element is not a member of the source set. + */ + public T smove(@NonNull String source, @NonNull String destination, @NonNull String member) { + ArgsArray commandArgs = buildArgs(source, destination, member); + protobufTransaction.addCommands(buildCommand(SMove, commandArgs)); + return getThis(); + } + /** * Stores the members of the intersection of all given sets specified by keys into a * new set at destination. diff --git a/java/client/src/test/java/glide/api/RedisClientTest.java b/java/client/src/test/java/glide/api/RedisClientTest.java index 31da5bbc21..c3ae44f05e 100644 --- a/java/client/src/test/java/glide/api/RedisClientTest.java +++ b/java/client/src/test/java/glide/api/RedisClientTest.java @@ -87,6 +87,7 @@ import static redis_request.RedisRequestOuterClass.RequestType.SInterStore; import static redis_request.RedisRequestOuterClass.RequestType.SIsMember; import static redis_request.RedisRequestOuterClass.RequestType.SMembers; +import static redis_request.RedisRequestOuterClass.RequestType.SMove; import static redis_request.RedisRequestOuterClass.RequestType.SRem; import static redis_request.RedisRequestOuterClass.RequestType.Select; import static redis_request.RedisRequestOuterClass.RequestType.SetRange; @@ -1732,6 +1733,30 @@ public void scard_returns_success() { assertEquals(value, payload); } + @SneakyThrows + @Test + public void smove_returns_success() { + // setup + String source = "src"; + String destination = "dst"; + String member = "elem"; + String[] arguments = {source, destination, member}; + + CompletableFuture testResponse = new CompletableFuture<>(); + testResponse.complete(true); + + // match on protobuf request + when(commandManager.submitNewCommand(eq(SMove), eq(arguments), any())) + .thenReturn(testResponse); + + // exercise + CompletableFuture response = service.smove(source, destination, member); + + // verify + assertEquals(testResponse, response); + assertTrue(response.get()); + } + @SneakyThrows @Test public void sinterstore_returns_success() { diff --git a/java/client/src/test/java/glide/api/models/TransactionTests.java b/java/client/src/test/java/glide/api/models/TransactionTests.java index 6997792f79..0b6cc25664 100644 --- a/java/client/src/test/java/glide/api/models/TransactionTests.java +++ b/java/client/src/test/java/glide/api/models/TransactionTests.java @@ -72,6 +72,7 @@ import static redis_request.RedisRequestOuterClass.RequestType.SInterStore; import static redis_request.RedisRequestOuterClass.RequestType.SIsMember; import static redis_request.RedisRequestOuterClass.RequestType.SMembers; +import static redis_request.RedisRequestOuterClass.RequestType.SMove; import static redis_request.RedisRequestOuterClass.RequestType.SRem; import static redis_request.RedisRequestOuterClass.RequestType.SetRange; import static redis_request.RedisRequestOuterClass.RequestType.SetString; @@ -273,6 +274,9 @@ public void transaction_builds_protobuf_request(BaseTransaction transaction) transaction.scard("key"); results.add(Pair.of(SCard, buildArgs("key"))); + transaction.smove("key1", "key2", "elem"); + results.add(Pair.of(SMove, buildArgs("key1", "key2", "elem"))); + transaction.sinterstore("key", new String[] {"set1", "set2"}); results.add(Pair.of(SInterStore, buildArgs("key", "set1", "set2"))); diff --git a/java/integTest/src/test/java/glide/SharedCommandTests.java b/java/integTest/src/test/java/glide/SharedCommandTests.java index 7ed014fccb..6ea776dee3 100644 --- a/java/integTest/src/test/java/glide/SharedCommandTests.java +++ b/java/integTest/src/test/java/glide/SharedCommandTests.java @@ -15,6 +15,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -914,6 +915,66 @@ public void sadd_srem_scard_smembers_key_with_non_set_value(BaseClient client) { assertTrue(e.getCause() instanceof RequestException); } + @SneakyThrows + @ParameterizedTest(autoCloseArguments = false) + @MethodSource("getClients") + public void smove(BaseClient client) { + String setKey1 = "{key}" + UUID.randomUUID(); + String setKey2 = "{key}" + UUID.randomUUID(); + String setKey3 = "{key}" + UUID.randomUUID(); + String nonSetKey = "{key}" + UUID.randomUUID(); + + assertEquals(3, client.sadd(setKey1, new String[] {"1", "2", "3"}).get()); + assertEquals(2, client.sadd(setKey2, new String[] {"2", "3"}).get()); + + // move an elem + assertTrue(client.smove(setKey1, setKey2, "1").get()); + assertEquals(Set.of("2", "3"), client.smembers(setKey1).get()); + assertEquals(Set.of("1", "2", "3"), client.smembers(setKey2).get()); + + // move an elem which preset at destination + assertTrue(client.smove(setKey2, setKey1, "2").get()); + assertEquals(Set.of("2", "3"), client.smembers(setKey1).get()); + assertEquals(Set.of("1", "3"), client.smembers(setKey2).get()); + + // move from missing key + assertFalse(client.smove(setKey3, setKey1, "4").get()); + assertEquals(Set.of("2", "3"), client.smembers(setKey1).get()); + + // move to a new set + assertTrue(client.smove(setKey1, setKey3, "2").get()); + assertEquals(Set.of("3"), client.smembers(setKey1).get()); + assertEquals(Set.of("2"), client.smembers(setKey3).get()); + + // move missing element + assertFalse(client.smove(setKey1, setKey3, "42").get()); + assertEquals(Set.of("3"), client.smembers(setKey1).get()); + assertEquals(Set.of("2"), client.smembers(setKey3).get()); + + // move missing element to missing key + assertFalse(client.smove(setKey1, nonSetKey, "42").get()); + assertEquals(Set.of("3"), client.smembers(setKey1).get()); + assertEquals("none", client.type(nonSetKey).get()); + + // Key exists, but it is not a set + assertEquals(OK, client.set(nonSetKey, "bar").get()); + ExecutionException executionException = + assertThrows(ExecutionException.class, () -> client.smove(nonSetKey, setKey1, "_").get()); + assertInstanceOf(RequestException.class, executionException.getCause()); + + executionException = + assertThrows(ExecutionException.class, () -> client.smove(setKey1, nonSetKey, "_").get()); + assertInstanceOf(RequestException.class, executionException.getCause()); + + // same-slot requirement + if (client instanceof RedisClusterClient) { + executionException = + assertThrows(ExecutionException.class, () -> client.smove("abc", "zxy", "lkn").get()); + assertInstanceOf(RequestException.class, executionException.getCause()); + assertTrue(executionException.getMessage().toLowerCase().contains("crossslot")); + } + } + @SneakyThrows @ParameterizedTest(autoCloseArguments = false) @MethodSource("getClients") diff --git a/java/integTest/src/test/java/glide/TransactionTestUtilities.java b/java/integTest/src/test/java/glide/TransactionTestUtilities.java index 879b7344c9..d0bcf7c013 100644 --- a/java/integTest/src/test/java/glide/TransactionTestUtilities.java +++ b/java/integTest/src/test/java/glide/TransactionTestUtilities.java @@ -105,8 +105,10 @@ public static BaseTransaction transactionTest(BaseTransaction baseTransact baseTransaction.scard(key7); baseTransaction.sismember(key7, "baz"); baseTransaction.smembers(key7); + baseTransaction.sadd(setKey2, new String[] {"a", "b"}); baseTransaction.sinterstore(setKey3, new String[] {setKey2, key7}); + baseTransaction.smove(key7, setKey2, "baz"); baseTransaction.zadd(key8, Map.of("one", 1.0, "two", 2.0, "three", 3.0)); baseTransaction.zrank(key8, "one"); @@ -214,6 +216,7 @@ public static Object[] transactionTestResult() { Set.of("baz"), 2L, // sadd(setKey2, new String[] { "a", "b" }) 0L, // sinterstore(setKey3, new String[] { setKey2, key7 }) + true, // smove(key7, setKey2, "baz") 3L, 0L, // zrank(key8, "one") 4.0,