From 53709f0281ed317985b2496ae1fe6661cec250d4 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 18 Sep 2024 14:35:59 +0200 Subject: [PATCH] Improve Cluster Nodes parsing. Without this fix there's a problem with parsing of the cluster nodes command output (e.g. a trailing comma after cport is making the parsing fail) with this change we're converting regexp parsing to code parsing which also includes support for trailing commas after cport Closes #2862 Original Pull Request: #3000 --- .../redis/connection/convert/Converters.java | 84 ++++++++++++++++--- .../convert/ConvertersUnitTests.java | 48 +++++++++-- 2 files changed, 115 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/convert/Converters.java b/src/main/java/org/springframework/data/redis/connection/convert/Converters.java index b0ef79f89..107ed7891 100644 --- a/src/main/java/org/springframework/data/redis/connection/convert/Converters.java +++ b/src/main/java/org/springframework/data/redis/connection/convert/Converters.java @@ -20,11 +20,10 @@ import java.nio.ByteBuffer; import java.time.Duration; import java.util.*; import java.util.concurrent.TimeUnit; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.core.convert.converter.Converter; import org.springframework.data.geo.Distance; import org.springframework.data.geo.GeoResult; @@ -45,6 +44,7 @@ import org.springframework.data.redis.connection.RedisNode.NodeType; import org.springframework.data.redis.connection.zset.Tuple; import org.springframework.data.redis.serializer.RedisSerializer; import org.springframework.data.redis.util.ByteUtils; +import org.springframework.lang.NonNull; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -545,10 +545,15 @@ public abstract class Converters { *
  • {@code %s:%i} (Redis 3)
    • *
  • {@code %s:%i@%i} (Redis 4, with bus port)
    • *
  • {@code %s:%i@%i,%s} (Redis 7, with announced hostname)
    • + * + * The output of the {@code CLUSTER NODES } command is just a space-separated CSV string, where each + * line represents a node in the cluster. The following is an example of output on Redis 7.2.0. + * You can check the latest here. + * + * {@code ... } + * * */ - static final Pattern clusterEndpointPattern = Pattern - .compile("\\[?([0-9a-zA-Z\\-_\\.:]*)\\]?:([0-9]+)(?:@[0-9]+(?:,([^,].*))?)?"); private static final Map flagLookupMap; static { @@ -567,18 +572,75 @@ public abstract class Converters { static final int LINK_STATE_INDEX = 7; static final int SLOTS_INDEX = 8; + record AddressPortHostname(String addressPart, String portPart, @Nullable String hostnamePart) { + + static AddressPortHostname of(String[] args) { + Assert.isTrue(args.length >= HOST_PORT_INDEX + 1, "ClusterNode information does not define host and port"); + // + String hostPort = args[HOST_PORT_INDEX]; + int lastColon = hostPort.lastIndexOf(":"); + Assert.isTrue(lastColon != -1, "ClusterNode information does not define host and port"); + String addressPart = getAddressPart(hostPort, lastColon); + // Everything to the right of port + int indexOfColon = hostPort.indexOf(","); + boolean hasColon = indexOfColon != -1; + String hostnamePart = getHostnamePart(hasColon, hostPort, indexOfColon); + String portPart = getPortPart(hostPort, lastColon, hasColon, indexOfColon); + return new AddressPortHostname(addressPart, portPart, hostnamePart); + } + + @NonNull private static String getAddressPart(String hostPort, int lastColon) { + // Everything to the left of port + // 127.0.0.1:6380 + // 127.0.0.1:6380@6381 + // :6380 + // :6380@6381 + // 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380 + // 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381 + // 127.0.0.1:6380,hostname1 + // 127.0.0.1:6380@6381,hostname1 + // :6380,hostname1 + // :6380@6381,hostname1 + // 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380,hostname1 + // 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381,hostname1 + String addressPart = hostPort.substring(0, lastColon); + // [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380 + // [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381 + // [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380,hostname1 + // [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381,hostname1 + if (addressPart.startsWith("[") && addressPart.endsWith("]")) { + addressPart = addressPart.substring(1, addressPart.length() - 1); + } + return addressPart; + } + + @Nullable + private static String getHostnamePart(boolean hasColon, String hostPort, int indexOfColon) { + // Everything to the right starting from comma + String hostnamePart = hasColon ? hostPort.substring(indexOfColon + 1) : null; + return StringUtils.hasText(hostnamePart) ? hostnamePart : null; + } + + @NonNull private static String getPortPart(String hostPort, int lastColon, boolean hasColon, int indexOfColon) { + String portPart = hostPort.substring(lastColon + 1); + if (portPart.contains("@")) { + portPart = portPart.substring(0, portPart.indexOf("@")); + } else if (hasColon) { + portPart = portPart.substring(0, indexOfColon); + } + return portPart; + } + } + @Override public RedisClusterNode convert(String source) { String[] args = source.split(" "); - Matcher matcher = clusterEndpointPattern.matcher(args[HOST_PORT_INDEX]); - - Assert.isTrue(matcher.matches(), "ClusterNode information does not define host and port"); - - String addressPart = matcher.group(1); - String portPart = matcher.group(2); - String hostnamePart = matcher.group(3); + AddressPortHostname addressPortHostname = AddressPortHostname.of(args); + String addressPart = addressPortHostname.addressPart; + String portPart = addressPortHostname.portPart; + String hostnamePart = addressPortHostname.hostnamePart; SlotRange range = parseSlotRange(args); Set flags = parseFlags(args); diff --git a/src/test/java/org/springframework/data/redis/connection/convert/ConvertersUnitTests.java b/src/test/java/org/springframework/data/redis/connection/convert/ConvertersUnitTests.java index 74d3f7df8..7bd64f213 100644 --- a/src/test/java/org/springframework/data/redis/connection/convert/ConvertersUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/convert/ConvertersUnitTests.java @@ -18,9 +18,9 @@ package org.springframework.data.redis.connection.convert; import static org.assertj.core.api.Assertions.*; import java.util.Iterator; -import java.util.regex.Matcher; import java.util.stream.Stream; +import org.assertj.core.api.SoftAssertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -72,6 +72,10 @@ class ConvertersUnitTests { private static final String CLUSTER_NODE_WITH_SINGLE_INVALID_IPV6_HOST = "67adfe3df1058896e3cb49d2863e0f70e7e159fa 2a02:6b8:c67:9c:0:6d8b:33da:5a2c: master,nofailover - 0 1692108412315 1 connected 0-5460"; + private static final String CLUSTER_NODE_WITH_SINGLE_IPV4_EMPTY_HOSTNAME = "3765733728631672640db35fd2f04743c03119c6 10.180.0.33:11003@16379, master - 0 1708041426947 2 connected 0-5460"; + + private static final String CLUSTER_NODE_WITH_SINGLE_IPV4_HOSTNAME = "3765733728631672640db35fd2f04743c03119c6 10.180.0.33:11003@16379,hostname1 master - 0 1708041426947 2 connected 0-5460"; + @Test // DATAREDIS-315 void toSetOfRedis30ClusterNodesShouldConvertSingleStringNodesResponseCorrectly() { @@ -248,6 +252,39 @@ class ConvertersUnitTests { assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461); } + @Test // https://github.com/spring-projects/spring-data-redis/issues/2862 + void toClusterNodeWithIPv4EmptyHostname() { + RedisClusterNode node = Converters.toClusterNode(CLUSTER_NODE_WITH_SINGLE_IPV4_EMPTY_HOSTNAME); + + SoftAssertions.assertSoftly(softAssertions -> { + softAssertions.assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6"); + softAssertions.assertThat(node.getHost()).isEqualTo("10.180.0.33"); + softAssertions.assertThat(node.hasValidHost()).isTrue(); + softAssertions.assertThat(node.getPort()).isEqualTo(11003); + softAssertions.assertThat(node.getType()).isEqualTo(NodeType.MASTER); + softAssertions.assertThat(node.getFlags()).contains(Flag.MASTER); + softAssertions.assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED); + softAssertions.assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461); + }); + } + + @Test // https://github.com/spring-projects/spring-data-redis/issues/2862 + void toClusterNodeWithIPv4Hostname() { + RedisClusterNode node = Converters.toClusterNode(CLUSTER_NODE_WITH_SINGLE_IPV4_HOSTNAME); + + SoftAssertions.assertSoftly(softAssertions -> { + softAssertions.assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6"); + softAssertions.assertThat(node.getHost()).isEqualTo("10.180.0.33"); + softAssertions.assertThat(node.getName()).isEqualTo("hostname1"); + softAssertions.assertThat(node.hasValidHost()).isTrue(); + softAssertions.assertThat(node.getPort()).isEqualTo(11003); + softAssertions.assertThat(node.getType()).isEqualTo(NodeType.MASTER); + softAssertions.assertThat(node.getFlags()).contains(Flag.MASTER); + softAssertions.assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED); + softAssertions.assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461); + }); + } + @Test // GH-2678 void toClusterNodeWithIPv6HostnameSquareBrackets() { @@ -273,12 +310,11 @@ class ConvertersUnitTests { @MethodSource("clusterNodesEndpoints") void shouldAcceptHostPatterns(String endpoint, String expectedAddress, String expectedPort, String expectedHostname) { - Matcher matcher = ClusterNodesConverter.clusterEndpointPattern.matcher(endpoint); - assertThat(matcher.matches()).isTrue(); + ClusterNodesConverter.AddressPortHostname addressPortHostname = ClusterNodesConverter.AddressPortHostname.of(new String[] { "id", endpoint }); - assertThat(matcher.group(1)).isEqualTo(expectedAddress); - assertThat(matcher.group(2)).isEqualTo(expectedPort); - assertThat(matcher.group(3)).isEqualTo(expectedHostname); + assertThat(addressPortHostname.addressPart()).isEqualTo(expectedAddress); + assertThat(addressPortHostname.portPart()).isEqualTo(expectedPort); + assertThat(addressPortHostname.hostnamePart()).isEqualTo(expectedHostname); } static Stream clusterNodesEndpoints() {