Java: Add SMOVE command. (#1249)

* Add `SMOVE` command. (#182) * Add `SMOVE` command. Signed-off-by: Yury-Fridlyand <[email protected]> * Update javadoc. Signed-off-by: Yury-Fridlyand <[email protected]> * Update javadoc. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * Typo fix. Signed-off-by: Yury-Fridlyand <[email protected]> * Update UT. Signed-off-by: Yury-Fridlyand <[email protected]> * PR comments. Signed-off-by: Yury-Fridlyand <[email protected]> * update doc. Signed-off-by: Yury-Fridlyand <[email protected]> * update doc. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]>
valkey-io · Apr 16, 2024 · 9fc70fa · 9fc70fa
1 parent ddf5bf8
commit 9fc70fa
Show file tree

Hide file tree

Showing 10 changed files with 155 additions and 1 deletion.
diff --git 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<ExpectedReturnType> {
     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
@@ -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
@@ -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<ProtobufRequestType>> 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
@@ -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<Long> scard(@NonNull String key) {
         return commandManager.submitNewCommand(SCard, new String[] {key}, this::handleLongResponse);
     }
 
+    @Override
+    public CompletableFuture<Boolean> smove(
+            @NonNull String source, @NonNull String destination, @NonNull String member) {
+        return commandManager.submitNewCommand(
+                SMove, new String[] {source, destination, member}, this::handleBooleanResponse);
+    }
+
     @Override
     public CompletableFuture<Long> 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
@@ -76,6 +76,27 @@ public interface SetBaseCommands {
      */
     CompletableFuture<Long> scard(String key);
 
+    /**
+     * Moves <code>member</code> from the set at <code>source</code> to the set at <code>destination
+     * </code>, removing it from the source set. Creates a new destination set if needed. The
+     * operation is atomic.
+     *
+     * @apiNote When in cluster mode, <code>source</code> and <code>destination</code> must map to the
+     *     same <code>hash slot</code>.
+     * @see <a href="https://redis.io/commands/smove/">redis.io</a> 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 <code>true</code> on success, or <code>false</code> if the <code>source</code> set does
+     *     not exist or the element is not a member of the source set.
+     * @example
+     *     <pre>{@code
+     * Boolean moved = client.smove("set1", "set2", "element").get();
+     * assert moved;
+     * }</pre>
+     */
+    CompletableFuture<Boolean> smove(String source, String destination, String member);
+
     /**
      * Returns if <code>member</code> is a member of the set stored at <code>key</code>.
      *

diff --git 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 <code>member</code> from the set at <code>source</code> to the set at <code>destination
+     * </code>, removing it from the source set. Creates a new destination set if needed. The
+     * operation is atomic.
+     *
+     * @see <a href="https://redis.io/commands/smove/">redis.io</a> 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 - <code>true</code> on success, or <code>false</code> if the <code>
+     *     source</code> 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 <code>keys</code> into a
      * new set at <code>destination</code>.

diff --git 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<Boolean> testResponse = new CompletableFuture<>();
+        testResponse.complete(true);
+
+        // match on protobuf request
+        when(commandManager.<Boolean>submitNewCommand(eq(SMove), eq(arguments), any()))
+                .thenReturn(testResponse);
+
+        // exercise
+        CompletableFuture<Boolean> 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
@@ -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
@@ -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
@@ -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,