Repository: redis/lettuce (java)

Issue:
Add support for creating regex- and subnet-based ReadFrom instances from a single string
## Feature Request

Spring Boot would like to be able to offer support for configuring Lettuce's `ReadFrom` setting using an application property. This is straightforward for the many `ReadFrom` constants that are supported by `ReadFrom#valueOf` but harder for the `regex` and `subnet` variants. We'd like to make this easier by enhancing `valueOf` to support `regex` and `subnet` as well.

#### Is your feature request related to a problem? Please describe

Please see https://github.com/spring-projects/spring-boot/pull/42588 for background and a description of the problem we're currently facing with trying to configure the `regex` and `subnet` variants with a single property.

#### Describe the solution you'd like

Our proposal is to enhance `valueOf` with support for a special syntax that allow the caller to indicate that a `regex` or `subnet` instance should be created and to provide the patterns or CIDR notations at the same time. The variant would be indicated by a prefix such as `regex:` or `subnet:` with the remainder of the string being the value. For example, `"regex:.*region-1.*"` or `"subnet:192.168.0.0/16"`. Comma-separated values could be supported as well such as `"subnet:192.168.0.0/16,2001:db8:abcd:0000::/52"`.

Supporting an order-sensitive `regex` hasn't really been considered yet. Perhaps an `order-sensitive-regex` would be the cleanest option?

#### Describe alternatives you've considered

We've considered defining this syntax in Spring Boot and pre-processing the value before calling `ReadFrom#pattern` or `ReadFrom#subnet` as appropriate. We concluded that it would be better for Lettuce itself to support the syntax as it broadens the benefit and also means that Spring Boot won't be an odd-on-out with a feature that may clash with a future change in Lettuce.

#### Teachability, Documentation, Adoption, Migration Strategy

Users will be able to call `ReadFrom#valueOf` and create any of the supported variants if they pass in an appropriately formatted string. No migration would be required as I would expect the existing `subnet` and `regex` methods to remain for those who prefer not to use a "magic" string.

Pull request:
Add support for creating regex and subnet-based ReadFrom instances from a single string
Closes #3013

Before this commit, it was not possible to use ReadFrom.valueOf for subnet and regex types. This commit introduces support for these types, as well as the use of names in underscore format




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");
     }
 


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() {