From f2752d1b029f0d2642df588ab2adca8b0110dee0 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 19 Sep 2024 14:23:46 +0200 Subject: [PATCH] Polishing. Update tests to simplify assertions and enhance GH issue references. Simplify parsing logic for addressing edge cases and added more test scenarios. See #2862 Original Pull Request: #3000 --- .../redis/connection/convert/Converters.java | 121 +++++++++--------- .../convert/ConvertersUnitTests.java | 105 +++++++++------ 2 files changed, 126 insertions(+), 100 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 107ed7891..2dc686bcf 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 @@ -44,7 +44,6 @@ 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; @@ -63,6 +62,7 @@ import org.springframework.util.StringUtils; * @author daihuabin * @author John Blum * @author Sorokin Evgeniy + * @author Marcin Grzejszczak */ public abstract class Converters { @@ -572,63 +572,62 @@ 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) { + /** + * Value object capturing Redis' representation of a cluster node network coordinate. + * + * @author Marcin Grzejszczak + * @author Mark Paluch + */ + record AddressPortHostname(String address, String port, @Nullable String hostname) { + + /** + * Parses Redis {@code CLUSTER NODES} host and port segment into {@link AddressPortHostname}. + */ + static AddressPortHostname parse(String hostAndPortPart) { + + String[] segments = hostAndPortPart.split(","); + int portSeparator = segments[0].lastIndexOf(":"); + Assert.isTrue(portSeparator != -1, "ClusterNode information does not define host and port"); + + String addressPart = getAddressPart(segments[0].substring(0, portSeparator)); + String portPart = getPortPart(segments[0].substring(portSeparator + 1)); + String hostnamePart = segments.length > 1 ? segments[1] : null; - 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; + private static String getAddressPart(String address) { + return address.startsWith("[") && address.endsWith("]") ? address.substring(1, address.length() - 1) : address; } - @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; + private static String getPortPart(String segment) { + + if (segment.contains("@")) { + return segment.substring(0, segment.indexOf('@')); + } + + if (segment.contains(":")) { + return segment.substring(0, segment.indexOf(':')); + } + + return segment; } - @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); + public int portAsInt() { + return Integer.parseInt(port()); + } + + public boolean hasHostname() { + return StringUtils.hasText(hostname()); + } + + public String getRequiredHostname() { + + if (StringUtils.hasText(hostname())) { + return hostname(); } - return portPart; + + throw new IllegalStateException("Hostname not available"); } } @@ -637,24 +636,24 @@ public abstract class Converters { String[] args = source.split(" "); - AddressPortHostname addressPortHostname = AddressPortHostname.of(args); - String addressPart = addressPortHostname.addressPart; - String portPart = addressPortHostname.portPart; - String hostnamePart = addressPortHostname.hostnamePart; + Assert.isTrue(args.length >= MASTER_ID_INDEX + 1, + () -> "Invalid ClusterNode information, insufficient segments: %s".formatted(source)); + + AddressPortHostname endpoint = AddressPortHostname.parse(args[HOST_PORT_INDEX]); SlotRange range = parseSlotRange(args); - Set flags = parseFlags(args); + Set flags = parseFlags(args[FLAGS_INDEX]); RedisClusterNodeBuilder nodeBuilder = RedisClusterNode.newRedisClusterNode() - .listeningAt(addressPart, Integer.parseInt(portPart)) // + .listeningAt(endpoint.address(), endpoint.portAsInt()) // .withId(args[ID_INDEX]) // .promotedAs(flags.contains(Flag.MASTER) ? NodeType.MASTER : NodeType.REPLICA) // .serving(range) // .withFlags(flags) // .linkState(parseLinkState(args)); - if (hostnamePart != null) { - nodeBuilder.withName(hostnamePart); + if (endpoint.hasHostname()) { + nodeBuilder.withName(endpoint.getRequiredHostname()); } if (!args[MASTER_ID_INDEX].isEmpty() && !args[MASTER_ID_INDEX].startsWith("-")) { @@ -664,14 +663,12 @@ public abstract class Converters { return nodeBuilder.build(); } - private Set parseFlags(String[] args) { - - String raw = args[FLAGS_INDEX]; + private Set parseFlags(String source) { Set flags = new LinkedHashSet<>(8, 1); - if (StringUtils.hasText(raw)) { - for (String flag : raw.split(",")) { + if (StringUtils.hasText(source)) { + for (String flag : source.split(",")) { flags.add(flagLookupMap.get(flag)); } } 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 7bd64f213..6bc5938be 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 @@ -20,16 +20,16 @@ import static org.assertj.core.api.Assertions.*; import java.util.Iterator; 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; import org.junit.jupiter.params.provider.MethodSource; + import org.springframework.data.redis.connection.RedisClusterNode; import org.springframework.data.redis.connection.RedisClusterNode.Flag; import org.springframework.data.redis.connection.RedisClusterNode.LinkState; import org.springframework.data.redis.connection.RedisNode.NodeType; -import org.springframework.data.redis.connection.convert.Converters.ClusterNodesConverter; +import org.springframework.data.redis.connection.convert.Converters.ClusterNodesConverter.AddressPortHostname; /** * Unit tests for {@link Converters}. @@ -37,6 +37,7 @@ import org.springframework.data.redis.connection.convert.Converters.ClusterNodes * @author Christoph Strobl * @author Mark Paluch * @author Sorokin Evgeniy + * @author Marcin Grzejszczak */ class ConvertersUnitTests { @@ -252,37 +253,35 @@ class ConvertersUnitTests { assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461); } - @Test // https://github.com/spring-projects/spring-data-redis/issues/2862 + @Test // GH-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); - }); + assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6"); + assertThat(node.getHost()).isEqualTo("10.180.0.33"); + assertThat(node.hasValidHost()).isTrue(); + assertThat(node.getPort()).isEqualTo(11003); + assertThat(node.getType()).isEqualTo(NodeType.MASTER); + assertThat(node.getFlags()).contains(Flag.MASTER); + assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED); + assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461); } - @Test // https://github.com/spring-projects/spring-data-redis/issues/2862 + @Test // GH-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); - }); + assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6"); + assertThat(node.getHost()).isEqualTo("10.180.0.33"); + assertThat(node.getName()).isEqualTo("hostname1"); + assertThat(node.hasValidHost()).isTrue(); + assertThat(node.getPort()).isEqualTo(11003); + assertThat(node.getType()).isEqualTo(NodeType.MASTER); + assertThat(node.getFlags()).contains(Flag.MASTER); + assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED); + assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461); } @Test // GH-2678 @@ -308,34 +307,64 @@ class ConvertersUnitTests { @ParameterizedTest // GH-2678 @MethodSource("clusterNodesEndpoints") - void shouldAcceptHostPatterns(String endpoint, String expectedAddress, String expectedPort, String expectedHostname) { + void shouldAcceptHostPatterns(String endpoint, AddressPortHostname expected) { - ClusterNodesConverter.AddressPortHostname addressPortHostname = ClusterNodesConverter.AddressPortHostname.of(new String[] { "id", endpoint }); + AddressPortHostname addressPortHostname = AddressPortHostname.parse(endpoint); - assertThat(addressPortHostname.addressPart()).isEqualTo(expectedAddress); - assertThat(addressPortHostname.portPart()).isEqualTo(expectedPort); - assertThat(addressPortHostname.hostnamePart()).isEqualTo(expectedHostname); + assertThat(addressPortHostname).isEqualTo(expected); } static Stream clusterNodesEndpoints() { - return Stream.of( + Stream regular = Stream.of( // IPv4 with Host, Redis 3 - Arguments.of("1.2.4.4:7379", "1.2.4.4", "7379", null), + Arguments.of("1.2.4.4:7379", new AddressPortHostname("1.2.4.4", "7379", null)), // IPv6 with Host, Redis 3 - Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380", "6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null), + Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380", + new AddressPortHostname("6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)), // Assuming IPv6 in brackets with Host, Redis 3 - Arguments.of("[6b8:c67:9c:0:6d8b:33da:5a2c]:6380", "6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null), + Arguments.of("[6b8:c67:9c:0:6d8b:33da:5a2c]:6380", + new AddressPortHostname("6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)), // IPv4 with Host and Bus Port, Redis 4 - Arguments.of("127.0.0.1:7382@17382", "127.0.0.1", "7382", null), + Arguments.of("127.0.0.1:7382@17382", new AddressPortHostname("127.0.0.1", "7382", null)), // IPv6 with Host and Bus Port, Redis 4 - Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380", "6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null), + Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380", + new AddressPortHostname("6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)), // Hostname with Port and Bus Port, Redis 7 - Arguments.of("my.host-name.com:7379@17379", "my.host-name.com", "7379", null), + Arguments.of("my.host-name.com:7379@17379", new AddressPortHostname("my.host-name.com", "7379", null)), // With hostname, Redis 7 - Arguments.of("1.2.4.4:7379@17379,my.host-name.com", "1.2.4.4", "7379", "my.host-name.com")); + Arguments.of("1.2.4.4:7379@17379,my.host-name.com", + new AddressPortHostname("1.2.4.4", "7379", "my.host-name.com"))); + + Stream weird = Stream.of( + // Port-only + Arguments.of(":6380", new AddressPortHostname("", "6380", null)), + + // Port-only with bus-port + Arguments.of(":6380@6381", new AddressPortHostname("", "6380", null)), + // IP with trailing comma + Arguments.of("127.0.0.1:6380,", new AddressPortHostname("127.0.0.1", "6380", null)), + // IPv6 with bus-port + Arguments.of("2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381", + new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)), + // IPv6 with bus-port and hostname + Arguments.of("2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381,hostname1", + new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", "hostname1")), + // Port-only with hostname + Arguments.of(":6380,hostname1", new AddressPortHostname("", "6380", "hostname1")), + + // Port-only with bus-port + Arguments.of(":6380@6381,hostname1", new AddressPortHostname("", "6380", "hostname1")), + // IPv6 in brackets with bus-port + Arguments.of("[2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381", + new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)), + // IPv6 in brackets with bus-port and hostname + Arguments.of("[2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381,hostname1", + new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", "hostname1"))); + + return Stream.concat(regular, weird); } }