Repository: redis/lettuce (java)

Issue:
Add hasNoSlots() Method to RedisClusterNode
## Feature Request

<!-- First of all: Have you checked the docs https://github.com/lettuce-io/lettuce-core/wiki, GitHub issues, or Stack Overflow whether someone else has already reported your issue? Maybe the feature already exists?-->

#### Is your feature request related to a problem? Please describe

In bug report [#2769](https://github.com/lettuce-io/lettuce-core/issues/2769), an issue was identified when handling Redis Cluster topology refreshes during node reconfigurations where nodes temporarily had no assigned slots. The maintainer suggested subclassing RedisClusterClient and overriding determinePartitions to address the issue. I followed the suggestion and initially used redisClusterNode.getSlots().isEmpty() to identify this case. However, I found that getSlots() is quite time-consuming, increasing the overhead of topology refreshes.

#### Describe the solution you'd like

To improve efficiency, I propose adding a method `hasNoSlots()` to`RedisClusterNode.` This method has much better performance than `getSlots().isEmpty()`
##### Proposed Method:
```
/**
     * Checks if the node has no slots assigned.
     *
     * @return {@code true} if the slots field is null or empty, {@code false} otherwise.
     */
    public boolean hasNoSlots() {
        return slots == null || slots.isEmpty();
    }

```

#### Describe alternatives you've considered

Currently, the alternative is to use getSlots().isEmpty() to determine if a node has no slots assigned. However, this approach is inefficient because it involves generating a new list object, which adds overhead during topology refreshes.

#### Teachability, Documentation, Adoption, Migration Strategy

This feature request aims to improve the efficiency and readability of RedisClusterNode's API, particularly in scenarios involving cluster scaling and topology adjustments. I appreciate your consideration and am happy to make any additional changes or provide further details if needed.

Pull request:
Add hasNoSlots() method to RedisClusterNode
Closes #3014 

<!--
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 applied code formatting rules using the `mvn formatter:format` target. Don’t submit any formatting related changes.
- [x] You submit test cases (unit or integration tests) that back your changes.

 <!--
Great! Live long and prosper.
-->

Pull Request: Add hasNoSlots() Method to RedisClusterNode

#### Feature Request:

This pull request addresses feature request [#3014](https://github.com/redis/lettuce/issues/3014) and is related to the bug report [#2769](https://github.com/redis/lettuce/issues/2769).

#### Problem:

Currently, checking if a RedisClusterNode has no assigned slots involves accessing the private slots field or using getSlots(), which generates unnecessary objects and adds overhead.

#### Solution:

This PR introduces a new method, hasNoSlots(), in the RedisClusterNode class. This method encapsulates the logic for checking if the slots field is either null or empty, improving performance and code readability.

#### Changes:

	•	Added the method public boolean hasNoSlots() to the RedisClusterNode class.
	•	Added unit tests to validate the behavior of this method.

#### Testing:

	•	All existing tests pass.
	•	New unit tests were added to ensure correct behavior when:
	•	Slots are null.
	•	Slots are empty.
	•	Slots are assigned.


Patch:
diff --git a/src/main/java/io/lettuce/core/ReadFrom.java b/src/main/java/io/lettuce/core/ReadFrom.java
index f247fac94..d4300ccef 100644
--- a/src/main/java/io/lettuce/core/ReadFrom.java
+++ b/src/main/java/io/lettuce/core/ReadFrom.java
@@ -21,6 +21,7 @@ package io.lettuce.core;
 
 import java.util.List;
 import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
 
 import io.lettuce.core.internal.LettuceStrings;
 import io.lettuce.core.models.role.RedisNodeDescription;
@@ -181,9 +182,10 @@ public abstract class ReadFrom {
     }
 
     /**
-     * Retrieve the {@link ReadFrom} preset by name.
+     * Retrieve the {@link ReadFrom} preset by name. For complex types like {@code subnet} or {@code regex}, the following
+     * syntax could be used {@code subnet:192.168.0.0/16,2001:db8:abcd:0000::/52} and {@code regex:.*region-1.*} respectively.
      *
-     * @param name the name of the read from setting
+     * @param name the case-insensitive name of the read from setting
      * @return the {@link ReadFrom} preset
      * @throws IllegalArgumentException if {@code name} is empty, {@code null} or the {@link ReadFrom} preset is unknown.
      */
@@ -193,6 +195,25 @@ public abstract class ReadFrom {
             throw new IllegalArgumentException("Name must not be empty");
         }
 
+        int index = name.indexOf(':');
+        if (index != -1) {
+            String type = name.substring(0, index);
+            String value = name.substring(index + 1);
+            if (LettuceStrings.isEmpty(value)) {
+                throw new IllegalArgumentException("Value must not be empty for the type '" + type + "'");
+            }
+            if (type.equalsIgnoreCase("subnet")) {
+                return subnet(value.split(","));
+            }
+            if (type.equalsIgnoreCase("regex")) {
+                try {
+                    return regex(Pattern.compile(value));
+                } catch (PatternSyntaxException ex) {
+                    throw new IllegalArgumentException("Value '" + value + "' is not a valid regular expression", ex);
+                }
+            }
+        }
+
         if (name.equalsIgnoreCase("master")) {
             return UPSTREAM;
         }
@@ -229,14 +250,6 @@ public abstract class ReadFrom {
             return ANY_REPLICA;
         }
 
-        if (name.equalsIgnoreCase("subnet")) {
-            throw new IllegalArgumentException("subnet must be created via ReadFrom#subnet");
-        }
-
-        if (name.equalsIgnoreCase("regex")) {
-            throw new IllegalArgumentException("regex must be created via ReadFrom#regex");
-        }
-
         throw new IllegalArgumentException("ReadFrom " + name + " not supported");
     }
 
diff --git a/src/main/java/io/lettuce/core/cluster/models/partitions/RedisClusterNode.java b/src/main/java/io/lettuce/core/cluster/models/partitions/RedisClusterNode.java
index 7645281c2..c17006f73 100644
--- a/src/main/java/io/lettuce/core/cluster/models/partitions/RedisClusterNode.java
+++ b/src/main/java/io/lettuce/core/cluster/models/partitions/RedisClusterNode.java
@@ -45,6 +45,7 @@ import io.lettuce.core.models.role.RedisNodeDescription;
  *
  * @author Mark Paluch
  * @author Alessandro Simi
+ * @author Tony Zhang
  * @since 3.0
  */
 @SuppressWarnings("serial")
@@ -294,6 +295,15 @@ public class RedisClusterNode implements Serializable, RedisNodeDescription {
         return slots;
     }
 
+    /**
+     * Checks if the node has no slots assigned.
+     *
+     * @return {@code true} if the slots field is null or empty, {@code false} otherwise.
+     */
+    public boolean hasNoSlots() {
+        return slots == null || slots.isEmpty();
+    }
+
     /**
      * Performs the given action for each slot of this {@link RedisClusterNode} until all elements have been processed or the
      * action throws an exception. Unless otherwise specified by the implementing class, actions are performed in the order of


Test patch:
diff --git a/src/test/java/io/lettuce/core/cluster/ReadFromUnitTests.java b/src/test/java/io/lettuce/core/cluster/ReadFromUnitTests.java
index 3ee0b5945..9f37e07ac 100644
--- a/src/test/java/io/lettuce/core/cluster/ReadFromUnitTests.java
+++ b/src/test/java/io/lettuce/core/cluster/ReadFromUnitTests.java
@@ -31,6 +31,8 @@ import java.util.regex.Pattern;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import io.lettuce.core.ReadFrom;
 import io.lettuce.core.RedisURI;
@@ -220,44 +222,133 @@ class ReadFromUnitTests {
         assertThatThrownBy(() -> ReadFrom.valueOf("unknown")).isInstanceOf(IllegalArgumentException.class);
     }
 
-    @Test
-    void valueOfNearest() {
-        assertThat(ReadFrom.valueOf("nearest")).isEqualTo(ReadFrom.NEAREST);
+    @ParameterizedTest
+    @ValueSource(strings = { "NEAREST", "nearest", "Nearest" })
+    void valueOfNearest(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.NEAREST);
     }
 
-    @Test
-    void valueOfMaster() {
-        assertThat(ReadFrom.valueOf("master")).isEqualTo(ReadFrom.UPSTREAM);
+    @ParameterizedTest
+    @ValueSource(strings = { "lowestLatency", "lowestlatency", "LOWESTLATENCY" })
+    void valueOfLowestLatency(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.LOWEST_LATENCY);
     }
 
-    @Test
-    void valueOfMasterPreferred() {
-        assertThat(ReadFrom.valueOf("masterPreferred")).isEqualTo(ReadFrom.UPSTREAM_PREFERRED);
+    @ParameterizedTest
+    @ValueSource(strings = { "MASTER", "master", "Master" })
+    void valueOfMaster(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.UPSTREAM);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "masterPreferred", "masterpreferred", "MASTERPREFERRED" })
+    void valueOfMasterPreferred(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.UPSTREAM_PREFERRED);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "slave", "SLAVE", "Slave" })
+    void valueOfSlave(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.REPLICA);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "slavePreferred", "slavepreferred", "SLAVEPREFERRED" })
+    void valueOfSlavePreferred(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.REPLICA_PREFERRED);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "replicaPreferred", "replicapreferred", "REPLICAPREFERRED" })
+    void valueOfReplicaPreferred(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.REPLICA_PREFERRED);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "anyReplica", "anyreplica", "ANYREPLICA" })
+    void valueOfAnyReplica(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.ANY_REPLICA);
     }
 
     @Test
-    void valueOfSlave() {
-        assertThat(ReadFrom.valueOf("slave")).isEqualTo(ReadFrom.REPLICA);
+    void valueOfSubnetWithEmptyCidrNotations() {
+        assertThatThrownBy(() -> ReadFrom.valueOf("subnet")).isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "subnet:192.0.2.0/24,2001:db8:abcd:0000::/52", "SUBNET:192.0.2.0/24,2001:db8:abcd:0000::/52" })
+    void valueOfSubnet(String name) {
+        RedisClusterNode nodeInSubnetIpv4 = createNodeWithHost("192.0.2.1");
+        RedisClusterNode nodeNotInSubnetIpv4 = createNodeWithHost("198.51.100.1");
+        RedisClusterNode nodeInSubnetIpv6 = createNodeWithHost("2001:db8:abcd:0000::1");
+        RedisClusterNode nodeNotInSubnetIpv6 = createNodeWithHost("2001:db8:abcd:1000::");
+        ReadFrom sut = ReadFrom.valueOf(name);
+        List<RedisNodeDescription> result = sut
+                .select(getNodes(nodeInSubnetIpv4, nodeNotInSubnetIpv4, nodeInSubnetIpv6, nodeNotInSubnetIpv6));
+        assertThat(result).hasSize(2).containsExactly(nodeInSubnetIpv4, nodeInSubnetIpv6);
     }
 
     @Test
-    void valueOfSlavePreferred() {
-        assertThat(ReadFrom.valueOf("slavePreferred")).isEqualTo(ReadFrom.REPLICA_PREFERRED);
+    void valueOfRegexWithEmptyRegexValue() {
+        assertThatThrownBy(() -> ReadFrom.valueOf("regex")).isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "regex:.*region-1.*", "REGEX:.*region-1.*" })
+    void valueOfRegex(String name) {
+        ReadFrom sut = ReadFrom.valueOf(name);
+
+        RedisClusterNode node1 = createNodeWithHost("redis-node-1.region-1.example.com");
+        RedisClusterNode node2 = createNodeWithHost("redis-node-2.region-1.example.com");
+        RedisClusterNode node3 = createNodeWithHost("redis-node-1.region-2.example.com");
+        RedisClusterNode node4 = createNodeWithHost("redis-node-2.region-2.example.com");
+
+        List<RedisNodeDescription> result = sut.select(getNodes(node1, node2, node3, node4));
+
+        assertThat(sut).hasFieldOrPropertyWithValue("orderSensitive", false);
+        assertThat(result).hasSize(2).containsExactly(node1, node2);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "REPLICA", "replica", "Replica" })
+    void valueOfReplica(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.REPLICA);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "UPSTREAM", "upstream", "Upstream" })
+    void valueOfUpstream(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.UPSTREAM);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "upstreamPreferred", "UPSTREAMPREFERRED", "UpstreamPreferred" })
+    void valueOfUpstreamPreferred(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.UPSTREAM_PREFERRED);
     }
 
     @Test
-    void valueOfAnyReplica() {
-        assertThat(ReadFrom.valueOf("anyReplica")).isEqualTo(ReadFrom.ANY_REPLICA);
+    void valueOfWhenNameIsPresentButValueIsAbsent() {
+        assertThatThrownBy(() -> ReadFrom.valueOf("subnet:")).isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("Value must not be empty for the type 'subnet'");
     }
 
     @Test
-    void valueOfSubnet() {
-        assertThatThrownBy(() -> ReadFrom.valueOf("subnet")).isInstanceOf(IllegalArgumentException.class);
+    void valueOfWhenNameIsEmptyButValueIsPresent() {
+        assertThatThrownBy(() -> ReadFrom.valueOf(":192.0.2.0/24")).isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("ReadFrom :192.0.2.0/24 not supported");
     }
 
     @Test
-    void valueOfRegex() {
-        assertThatThrownBy(() -> ReadFrom.valueOf("regex")).isInstanceOf(IllegalArgumentException.class);
+    void valueOfRegexWithInvalidPatternShouldThrownIllegalArgumentException() {
+        assertThatThrownBy(() -> ReadFrom.valueOf("regex:\\")).isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("is not a valid regular expression");
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "ANY", "any", "Any" })
+    void valueOfAny(String name) {
+        assertThat(ReadFrom.valueOf(name)).isEqualTo(ReadFrom.ANY);
     }
 
     private ReadFrom.Nodes getNodes() {
diff --git a/src/test/java/io/lettuce/core/cluster/models/partitions/RedisClusterNodeUnitTests.java b/src/test/java/io/lettuce/core/cluster/models/partitions/RedisClusterNodeUnitTests.java
index 8c9ed7168..bace1dc5f 100644
--- a/src/test/java/io/lettuce/core/cluster/models/partitions/RedisClusterNodeUnitTests.java
+++ b/src/test/java/io/lettuce/core/cluster/models/partitions/RedisClusterNodeUnitTests.java
@@ -140,4 +140,35 @@ class RedisClusterNodeUnitTests {
         assertThat(node.toString()).contains(RedisClusterNode.class.getSimpleName());
     }
 
+    @Test
+    void shouldReturnTrueWhenSlotsAreNull() {
+
+        BitSet emptySlots = null;
+        RedisClusterNode node = new RedisClusterNode(RedisURI.create("localhost", 6379), "1", true, null, 0, 0, 0, emptySlots,
+                Collections.emptySet());
+
+        assertThat(node.hasNoSlots()).isTrue();
+    }
+
+    @Test
+    void shouldReturnTrueWhenSlotsAreEmpty() {
+
+        BitSet emptySlots = new BitSet(); // Empty BitSet
+        RedisClusterNode node = new RedisClusterNode(RedisURI.create("localhost", 6379), "1", true, null, 0, 0, 0, emptySlots,
+                Collections.emptySet());
+
+        assertThat(node.hasNoSlots()).isTrue();
+    }
+
+    @Test
+    void shouldReturnFalseWhenSlotsAreAssigned() {
+
+        BitSet slots = new BitSet();
+        slots.set(1); // Assign a slot
+        RedisClusterNode node = new RedisClusterNode(RedisURI.create("localhost", 6379), "1", true, null, 0, 0, 0, slots,
+                Collections.emptySet());
+
+        assertThat(node.hasNoSlots()).isFalse();
+    }
+
 }