Repository: redis/lettuce
Pull request #2871 (2024-06-04 14:28:26)

Remove not readonly commands #2832
Issue: #2832 
<!--
Thank you for proposing a pull request. This template will guide you through the essential steps necessary for a pull request.
-->
Make sure that:

- [x] You have read the [contribution guidelines](https://github.com/lettuce-io/lettuce-core/blob/main/.github/CONTRIBUTING.md).
- [x] You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
- [x] You use the code formatters provided [here](https://github.com/lettuce-io/lettuce-core/blob/main/formatting.xml) and have them applied to your changes. Don’t submit any formatting related changes.
- [ ] You submit test cases (unit or integration tests) that back your changes.

 <!--
Great! Live long and prosper.
-->


tishun: Just talked to @mp911de and he agreed that we should probably remove `io.lettuce.core.masterreplica. ReadOnlyCommands`. This could be done in another PR, but it would be awesome if we can do it here.
thachlp: > Hey @thachlp , thanks for submitting this PR.
> 
> I think there are a few things we needs to address first:
> 
> 1. Seems like there are two `ReadOnlyCommands` classes - one in the `io.lettuce.core.masterreplica` package and one in the `io.lettuce.core.protocol` package. The one you modified (in the `io.lettuce.core.masterreplica`) appears to be unused. I believe the change needs to be done in the `io.lettuce.core.protocol.ReadOnlyCommands` class. Let's change both classes for now similar to how it was done in [Add GEOSEARCH to read-only commands #2568 #2569](https://github.com/redis/lettuce/pull/2569)
> 2. The `GEOSEARCH` command is read-only, let's not remove it from the list:
> 
> ```
> COMMAND INFO GEOSEARCH
> 1) 1) "geosearch"
>    2) "-7"
>    3) 1) "readonly"
>    4) "1"
> ```
> 
> 3. In a perfect world I'd love to see an integration test that loops over these commands and verifies that they are readonly so we can quickly adapt to server change (optional, but would be very nice).

@tishun,  
You are correct, I rechecked and updated the list, please help review 🚀
Is there any way that I can run the integration test in local? I try to write the test that you recommend but I got the error
`java: package jdk.net does not exist`. How can I fix it?
tishun: > > Hey @thachlp , thanks for submitting this PR.
> > I think there are a few things we needs to address first:
> > 
> > 1. Seems like there are two `ReadOnlyCommands` classes - one in the `io.lettuce.core.masterreplica` package and one in the `io.lettuce.core.protocol` package. The one you modified (in the `io.lettuce.core.masterreplica`) appears to be unused. I believe the change needs to be done in the `io.lettuce.core.protocol.ReadOnlyCommands` class. Let's change both classes for now similar to how it was done in [Add GEOSEARCH to read-only commands #2568 #2569](https://github.com/redis/lettuce/pull/2569)
> > 2. The `GEOSEARCH` command is read-only, let's not remove it from the list:
> > 
> > ```
> > COMMAND INFO GEOSEARCH
> > 1) 1) "geosearch"
> >    2) "-7"
> >    3) 1) "readonly"
> >    4) "1"
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > 
> > 3. In a perfect world I'd love to see an integration test that loops over these commands and verifies that they are readonly so we can quickly adapt to server change (optional, but would be very nice).
> 
> @tishun, You are correct, I rechecked and updated the list, please help review 🚀 Is there any way that I can run the integration test in local? I try to write the test that you recommend but I got the error `java: package jdk.net does not exist`. How can I fix it?

You should be able to run test locally if you call `make test`
tishun: You'd need to format your new changes too, try using `mvn formatter:format`
thachlp: > You'd need to format your new changes too, try using `mvn formatter:format`

Thanks @tishun 🙇
thachlp: > > You'd need to format your new changes too, try using `mvn formatter:format`
> 
> Thanks @tishun 🙇

Hi @tishun, the error from CI is weird, could you re-run the job? Do you have any recommend? 
tishun: > > > You'd need to format your new changes too, try using `mvn formatter:format`
> > 
> > 
> > Thanks @tishun 🙇
> 
> Hi @tishun, the error from CI is weird, could you re-run the job? Do you have any recommend?

Hey @thachlp , [List.of()](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/List.html#of()) is a feature of Java 9, and we compile with Java 8 to achieve maximum backwards compatibility.

Could you change the use of this method with something else like `Arrays.asList()`
thachlp: > > > > You'd need to format your new changes too, try using `mvn formatter:format`
> > > 
> > > 
> > > Thanks @tishun 🙇
> > 
> > 
> > Hi @tishun, the error from CI is weird, could you re-run the job? Do you have any recommend?
> 
> Hey @thachlp , [List.of()](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/List.html#of()) is a feature of Java 9, and we compile with Java 8 to achieve maximum backwards compatibility.
> 
> Could you change the use of this method with something else like `Arrays.asList()`

Nice, thanks
tishun: I think you need to rebase your changes to include the latest fixes to the unit tests to pass the CI
thachlp: > I think you need to rebase your changes to include the latest fixes to the unit tests to pass the CI

@tishun, it's done 🙇

Patch:
diff --git a/src/main/java/io/lettuce/core/masterreplica/ReadOnlyCommands.java b/src/main/java/io/lettuce/core/masterreplica/ReadOnlyCommands.java
deleted file mode 100644
index 67616a7ef..000000000
--- a/src/main/java/io/lettuce/core/masterreplica/ReadOnlyCommands.java
+++ /dev/null
@@ -1,72 +0,0 @@
-/*
- * Copyright 2020-Present, Redis Ltd. and Contributors
- * All rights reserved.
- *
- * Licensed under the MIT License.
- *
- * This file contains contributions from third-party contributors
- * licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package io.lettuce.core.masterreplica;
-
-import java.util.Collections;
-import java.util.EnumSet;
-import java.util.Set;
-
-import io.lettuce.core.protocol.CommandType;
-import io.lettuce.core.protocol.ProtocolKeyword;
-
-/**
- * Contains all command names that are read-only commands.
- *
- * @author Mark Paluch
- */
-class ReadOnlyCommands {
-
-    private static final Set<CommandType> READ_ONLY_COMMANDS = EnumSet.noneOf(CommandType.class);
-
-    static {
-        for (CommandName commandNames : CommandName.values()) {
-            READ_ONLY_COMMANDS.add(CommandType.valueOf(commandNames.name()));
-        }
-    }
-
-    /**
-     * @param protocolKeyword must not be {@code null}.
-     * @return {@code true} if {@link ProtocolKeyword} is a read-only command.
-     */
-    public static boolean isReadOnlyCommand(ProtocolKeyword protocolKeyword) {
-        return READ_ONLY_COMMANDS.contains(protocolKeyword);
-    }
-
-    /**
-     * @return an unmodifiable {@link Set} of {@link CommandType read-only} commands.
-     */
-    public static Set<CommandType> getReadOnlyCommands() {
-        return Collections.unmodifiableSet(READ_ONLY_COMMANDS);
-    }
-
-    enum CommandName {
-        ASKING, BITCOUNT, BITPOS, CLIENT, COMMAND, DUMP, ECHO, EVAL_RO, EVALSHA_RO, EXISTS, FCALL_RO, //
-        GEODIST, GEOPOS, GEORADIUS, GEORADIUS_RO, GEORADIUSBYMEMBER, GEORADIUSBYMEMBER_RO, GEOSEARCH, GEOHASH, GET, GETBIT, //
-        GETRANGE, HEXISTS, HGET, HGETALL, HKEYS, HLEN, HMGET, HRANDFIELD, HSCAN, HSTRLEN, //
-        HVALS, INFO, KEYS, LINDEX, LLEN, LPOS, LRANGE, SORT_RO, MGET, PFCOUNT, PTTL, //
-        RANDOMKEY, READWRITE, SCAN, SCARD, SCRIPT, //
-        SDIFF, SINTER, SISMEMBER, SMISMEMBER, SMEMBERS, SRANDMEMBER, SSCAN, STRLEN, //
-        SUNION, TIME, TTL, TYPE, //
-        XINFO, XLEN, XPENDING, XRANGE, XREVRANGE, XREAD, //
-        ZCARD, ZCOUNT, ZLEXCOUNT, ZRANGE, //
-        ZRANDMEMBER, ZRANGEBYLEX, ZRANGEBYSCORE, ZRANK, ZREVRANGE, ZREVRANGEBYLEX, ZREVRANGEBYSCORE, ZREVRANK, ZSCAN, ZSCORE,
-    }
-
-}
diff --git a/src/main/java/io/lettuce/core/protocol/ReadOnlyCommands.java b/src/main/java/io/lettuce/core/protocol/ReadOnlyCommands.java
index a6bfc6333..81671bf76 100644
--- a/src/main/java/io/lettuce/core/protocol/ReadOnlyCommands.java
+++ b/src/main/java/io/lettuce/core/protocol/ReadOnlyCommands.java
@@ -71,7 +71,7 @@ public class ReadOnlyCommands {
 
     enum CommandName {
         ASKING, BITCOUNT, BITPOS, CLIENT, COMMAND, DUMP, ECHO, EVAL_RO, EVALSHA_RO, EXISTS, FCALL_RO, //
-        GEODIST, GEOPOS, GEORADIUS, GEORADIUS_RO, GEORADIUSBYMEMBER, GEORADIUSBYMEMBER_RO, GEOSEARCH, GEOHASH, GET, GETBIT, //
+        GEODIST, GEOPOS, GEORADIUS_RO, GEORADIUSBYMEMBER_RO, GEOSEARCH, GEOHASH, GET, GETBIT, //
         GETRANGE, HEXISTS, HGET, HGETALL, HKEYS, HLEN, HMGET, HRANDFIELD, HSCAN, HSTRLEN, //
         HVALS, INFO, KEYS, LINDEX, LLEN, LPOS, LRANGE, SORT_RO, MGET, PFCOUNT, PTTL, //
         RANDOMKEY, READWRITE, SCAN, SCARD, SCRIPT, //


Test patch:
diff --git a/src/test/java/io/lettuce/core/cluster/ClusterReadOnlyCommandsUnitTests.java b/src/test/java/io/lettuce/core/cluster/ClusterReadOnlyCommandsUnitTests.java
index b1f6cad9f..502198b7a 100644
--- a/src/test/java/io/lettuce/core/cluster/ClusterReadOnlyCommandsUnitTests.java
+++ b/src/test/java/io/lettuce/core/cluster/ClusterReadOnlyCommandsUnitTests.java
@@ -16,7 +16,7 @@ class ClusterReadOnlyCommandsUnitTests {
 
     @Test
     void testCount() {
-        assertThat(ClusterReadOnlyCommands.getReadOnlyCommands()).hasSize(86);
+        assertThat(ClusterReadOnlyCommands.getReadOnlyCommands()).hasSize(84);
     }
 
     @Test
diff --git a/src/test/java/io/lettuce/core/commands/HashCommandIntegrationTests.java b/src/test/java/io/lettuce/core/commands/HashCommandIntegrationTests.java
index 498491464..2cd1608e4 100644
--- a/src/test/java/io/lettuce/core/commands/HashCommandIntegrationTests.java
+++ b/src/test/java/io/lettuce/core/commands/HashCommandIntegrationTests.java
@@ -561,7 +561,6 @@ public class HashCommandIntegrationTests extends TestSupport {
         assertThat(redis.hset(MY_KEY, MY_FIELD, MY_VALUE)).isTrue();
         assertThat(redis.hexpire(MY_KEY, 1, MY_FIELD, MY_SECOND_FIELD)).containsExactly(1L, -2L);
         assertThat(redis.hexpire("invalidKey", 1, MY_FIELD)).containsExactly(-2L);
-
         await().until(() -> redis.hget(MY_KEY, MY_FIELD) == null);
     }
 
@@ -573,7 +572,6 @@ public class HashCommandIntegrationTests extends TestSupport {
         assertThat(redis.hexpire(MY_KEY, Duration.ofSeconds(1), ExpireArgs.Builder.xx(), MY_FIELD)).containsExactly(1L);
         assertThat(redis.hexpire(MY_KEY, Duration.ofSeconds(10), ExpireArgs.Builder.gt(), MY_FIELD)).containsExactly(1L);
         assertThat(redis.hexpire(MY_KEY, Duration.ofSeconds(1), ExpireArgs.Builder.lt(), MY_FIELD)).containsExactly(1L);
-
         await().until(() -> redis.hget(MY_KEY, MY_FIELD) == null);
     }
 
@@ -585,7 +583,6 @@ public class HashCommandIntegrationTests extends TestSupport {
         assertThat(redis.hset(MY_KEY, MY_FIELD, MY_VALUE)).isTrue();
         assertThat(redis.hexpireat(MY_KEY, Instant.now().plusSeconds(1), MY_FIELD)).containsExactly(1L);
         assertThat(redis.hexpireat("invalidKey", Instant.now().plusSeconds(1), MY_FIELD)).containsExactly(-2L);
-
         await().until(() -> redis.hget(MY_KEY, MY_FIELD) == null);
     }
 
@@ -610,7 +607,6 @@ public class HashCommandIntegrationTests extends TestSupport {
     @EnabledOnCommand("HPERSIST")
     void hpersist() {
         assertThat(redis.hpersist(MY_KEY, MY_FIELD)).containsExactly(-2L);
-
         assertThat(redis.hset(MY_KEY, MY_FIELD, MY_VALUE)).isTrue();
         assertThat(redis.hpersist(MY_KEY, MY_FIELD)).containsExactly(-1L);
 
diff --git a/src/test/java/io/lettuce/core/commands/ServerCommandIntegrationTests.java b/src/test/java/io/lettuce/core/commands/ServerCommandIntegrationTests.java
index b9be862e7..a64414edf 100644
--- a/src/test/java/io/lettuce/core/commands/ServerCommandIntegrationTests.java
+++ b/src/test/java/io/lettuce/core/commands/ServerCommandIntegrationTests.java
@@ -33,6 +33,8 @@ import java.util.regex.Pattern;
 
 import javax.inject.Inject;
 
+import io.lettuce.core.cluster.ClusterReadOnlyCommands;
+import io.lettuce.core.protocol.ProtocolKeyword;
 import io.lettuce.test.condition.RedisConditions;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
@@ -592,6 +594,13 @@ public class ServerCommandIntegrationTests extends TestSupport {
         assertThat(redis.clientInfo().contains("lib-name=lettuce")).isTrue();
     }
 
+    @Test
+    void testReadOnlyCommands() {
+        for (ProtocolKeyword readOnlyCommand : ClusterReadOnlyCommands.getReadOnlyCommands()) {
+            assertThat(isCommandReadOnly(readOnlyCommand.name())).isTrue();
+        }
+    }
+
     private boolean noSaveInProgress() {
 
         String info = redis.info();
@@ -599,4 +608,19 @@ public class ServerCommandIntegrationTests extends TestSupport {
         return !info.contains("aof_rewrite_in_progress:1") && !info.contains("rdb_bgsave_in_progress:1");
     }
 
+    private boolean isCommandReadOnly(String commandName) {
+        List<Object> commandInfo = redis.commandInfo(commandName);
+        if (commandInfo == null || commandInfo.isEmpty()) {
+            throw new IllegalArgumentException("Command not found: " + commandName);
+        }
+
+        List<CommandDetail> details = CommandDetailParser.parse(commandInfo);
+        if (details.isEmpty()) {
+            throw new IllegalArgumentException("Command details could not be parsed: " + commandName);
+        }
+
+        CommandDetail detail = details.get(0);
+        return !detail.getFlags().contains(CommandDetail.Flag.WRITE);
+    }
+
 }