Skip to content

Commit f2752d1

Browse files
mp911demp911de
committed
Polishing.
Update tests to simplify assertions and enhance GH issue references. Simplify parsing logic for addressing edge cases and added more test scenarios. See spring-projects#2862 Original Pull Request: spring-projects#3000
1 parent 53709f0 commit f2752d1

File tree

2 files changed

+126
-100
lines changed

2 files changed

+126
-100
lines changed

‎src/main/java/org/springframework/data/redis/connection/convert/Converters.java‎

Lines changed: 59 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@
4444
import org.springframework.data.redis.connection.zset.Tuple;
4545
import org.springframework.data.redis.serializer.RedisSerializer;
4646
import org.springframework.data.redis.util.ByteUtils;
47-
import org.springframework.lang.NonNull;
4847
import org.springframework.lang.Nullable;
4948
import org.springframework.util.Assert;
5049
import org.springframework.util.ClassUtils;
@@ -63,6 +62,7 @@
6362
* @author daihuabin
6463
* @author John Blum
6564
* @author Sorokin Evgeniy
65+
* @author Marcin Grzejszczak
6666
*/
6767
public abstract class Converters {
6868

@@ -572,63 +572,62 @@ enum ClusterNodesConverter implements Converter<String, RedisClusterNode> {
572572
static final int LINK_STATE_INDEX = 7;
573573
static final int SLOTS_INDEX = 8;
574574

575-
record AddressPortHostname(String addressPart, String portPart, @Nullable String hostnamePart) {
576-
577-
static AddressPortHostname of(String[] args) {
578-
Assert.isTrue(args.length >= HOST_PORT_INDEX + 1, "ClusterNode information does not define host and port");
579-
// <ip:port@cport[,hostname]>
580-
String hostPort = args[HOST_PORT_INDEX];
581-
int lastColon = hostPort.lastIndexOf(":");
582-
Assert.isTrue(lastColon != -1, "ClusterNode information does not define host and port");
583-
String addressPart = getAddressPart(hostPort, lastColon);
584-
// Everything to the right of port
585-
int indexOfColon = hostPort.indexOf(",");
586-
boolean hasColon = indexOfColon != -1;
587-
String hostnamePart = getHostnamePart(hasColon, hostPort, indexOfColon);
588-
String portPart = getPortPart(hostPort, lastColon, hasColon, indexOfColon);
575+
/**
576+
* Value object capturing Redis' representation of a cluster node network coordinate.
577+
*
578+
* @author Marcin Grzejszczak
579+
* @author Mark Paluch
580+
*/
581+
record AddressPortHostname(String address, String port, @Nullable String hostname) {
582+
583+
/**
584+
* Parses Redis {@code CLUSTER NODES} host and port segment into {@link AddressPortHostname}.
585+
*/
586+
static AddressPortHostname parse(String hostAndPortPart) {
587+
588+
String[] segments = hostAndPortPart.split(",");
589+
int portSeparator = segments[0].lastIndexOf(":");
590+
Assert.isTrue(portSeparator != -1, "ClusterNode information does not define host and port");
591+
592+
String addressPart = getAddressPart(segments[0].substring(0, portSeparator));
593+
String portPart = getPortPart(segments[0].substring(portSeparator + 1));
594+
String hostnamePart = segments.length > 1 ? segments[1] : null;
595+
589596
return new AddressPortHostname(addressPart, portPart, hostnamePart);
590597
}
591598

592-
@NonNull private static String getAddressPart(String hostPort, int lastColon) {
593-
// Everything to the left of port
594-
// 127.0.0.1:6380
595-
// 127.0.0.1:6380@6381
596-
// :6380
597-
// :6380@6381
598-
// 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380
599-
// 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381
600-
// 127.0.0.1:6380,hostname1
601-
// 127.0.0.1:6380@6381,hostname1
602-
// :6380,hostname1
603-
// :6380@6381,hostname1
604-
// 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380,hostname1
605-
// 2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381,hostname1
606-
String addressPart = hostPort.substring(0, lastColon);
607-
// [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380
608-
// [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381
609-
// [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380,hostname1
610-
// [2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381,hostname1
611-
if (addressPart.startsWith("[") && addressPart.endsWith("]")) {
612-
addressPart = addressPart.substring(1, addressPart.length() - 1);
599+
private static String getAddressPart(String address) {
600+
return address.startsWith("[") && address.endsWith("]") ? address.substring(1, address.length() - 1) : address;
601+
}
602+
603+
private static String getPortPart(String segment) {
604+
605+
if (segment.contains("@")) {
606+
return segment.substring(0, segment.indexOf('@'));
607+
}
608+
609+
if (segment.contains(":")) {
610+
return segment.substring(0, segment.indexOf(':'));
613611
}
614-
return addressPart;
612+
613+
return segment;
614+
}
615+
616+
public int portAsInt() {
617+
return Integer.parseInt(port());
615618
}
616619

617-
@Nullable
618-
private static String getHostnamePart(boolean hasColon, String hostPort, int indexOfColon) {
619-
// Everything to the right starting from comma
620-
String hostnamePart = hasColon ? hostPort.substring(indexOfColon + 1) : null;
621-
return StringUtils.hasText(hostnamePart) ? hostnamePart : null;
620+
public boolean hasHostname() {
621+
return StringUtils.hasText(hostname());
622622
}
623623

624-
@NonNull private static String getPortPart(String hostPort, int lastColon, boolean hasColon, int indexOfColon) {
625-
String portPart = hostPort.substring(lastColon + 1);
626-
if (portPart.contains("@")) {
627-
portPart = portPart.substring(0, portPart.indexOf("@"));
628-
} else if (hasColon) {
629-
portPart = portPart.substring(0, indexOfColon);
624+
public String getRequiredHostname() {
625+
626+
if (StringUtils.hasText(hostname())) {
627+
return hostname();
630628
}
631-
return portPart;
629+
630+
throw new IllegalStateException("Hostname not available");
632631
}
633632
}
634633

@@ -637,24 +636,24 @@ public RedisClusterNode convert(String source) {
637636

638637
String[] args = source.split(" ");
639638

640-
AddressPortHostname addressPortHostname = AddressPortHostname.of(args);
641-
String addressPart = addressPortHostname.addressPart;
642-
String portPart = addressPortHostname.portPart;
643-
String hostnamePart = addressPortHostname.hostnamePart;
639+
Assert.isTrue(args.length >= MASTER_ID_INDEX + 1,
640+
() -> "Invalid ClusterNode information, insufficient segments: %s".formatted(source));
641+
642+
AddressPortHostname endpoint = AddressPortHostname.parse(args[HOST_PORT_INDEX]);
644643

645644
SlotRange range = parseSlotRange(args);
646-
Set<Flag> flags = parseFlags(args);
645+
Set<Flag> flags = parseFlags(args[FLAGS_INDEX]);
647646

648647
RedisClusterNodeBuilder nodeBuilder = RedisClusterNode.newRedisClusterNode()
649-
.listeningAt(addressPart, Integer.parseInt(portPart)) //
648+
.listeningAt(endpoint.address(), endpoint.portAsInt()) //
650649
.withId(args[ID_INDEX]) //
651650
.promotedAs(flags.contains(Flag.MASTER) ? NodeType.MASTER : NodeType.REPLICA) //
652651
.serving(range) //
653652
.withFlags(flags) //
654653
.linkState(parseLinkState(args));
655654

656-
if (hostnamePart != null) {
657-
nodeBuilder.withName(hostnamePart);
655+
if (endpoint.hasHostname()) {
656+
nodeBuilder.withName(endpoint.getRequiredHostname());
658657
}
659658

660659
if (!args[MASTER_ID_INDEX].isEmpty() && !args[MASTER_ID_INDEX].startsWith("-")) {
@@ -664,14 +663,12 @@ public RedisClusterNode convert(String source) {
664663
return nodeBuilder.build();
665664
}
666665

667-
private Set<Flag> parseFlags(String[] args) {
668-
669-
String raw = args[FLAGS_INDEX];
666+
private Set<Flag> parseFlags(String source) {
670667

671668
Set<Flag> flags = new LinkedHashSet<>(8, 1);
672669

673-
if (StringUtils.hasText(raw)) {
674-
for (String flag : raw.split(",")) {
670+
if (StringUtils.hasText(source)) {
671+
for (String flag : source.split(",")) {
675672
flags.add(flagLookupMap.get(flag));
676673
}
677674
}

‎src/test/java/org/springframework/data/redis/connection/convert/ConvertersUnitTests.java‎

Lines changed: 67 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -20,23 +20,24 @@
2020
import java.util.Iterator;
2121
import java.util.stream.Stream;
2222

23-
import org.assertj.core.api.SoftAssertions;
2423
import org.junit.jupiter.api.Test;
2524
import org.junit.jupiter.params.ParameterizedTest;
2625
import org.junit.jupiter.params.provider.Arguments;
2726
import org.junit.jupiter.params.provider.MethodSource;
27+
2828
import org.springframework.data.redis.connection.RedisClusterNode;
2929
import org.springframework.data.redis.connection.RedisClusterNode.Flag;
3030
import org.springframework.data.redis.connection.RedisClusterNode.LinkState;
3131
import org.springframework.data.redis.connection.RedisNode.NodeType;
32-
import org.springframework.data.redis.connection.convert.Converters.ClusterNodesConverter;
32+
import org.springframework.data.redis.connection.convert.Converters.ClusterNodesConverter.AddressPortHostname;
3333

3434
/**
3535
* Unit tests for {@link Converters}.
3636
*
3737
* @author Christoph Strobl
3838
* @author Mark Paluch
3939
* @author Sorokin Evgeniy
40+
* @author Marcin Grzejszczak
4041
*/
4142
class ConvertersUnitTests {
4243

@@ -252,37 +253,35 @@ void toClusterNodeWithIPv6Hostname() {
252253
assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461);
253254
}
254255

255-
@Test // https://github.com/spring-projects/spring-data-redis/issues/2862
256+
@Test // GH-2862
256257
void toClusterNodeWithIPv4EmptyHostname() {
258+
257259
RedisClusterNode node = Converters.toClusterNode(CLUSTER_NODE_WITH_SINGLE_IPV4_EMPTY_HOSTNAME);
258260

259-
SoftAssertions.assertSoftly(softAssertions -> {
260-
softAssertions.assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6");
261-
softAssertions.assertThat(node.getHost()).isEqualTo("10.180.0.33");
262-
softAssertions.assertThat(node.hasValidHost()).isTrue();
263-
softAssertions.assertThat(node.getPort()).isEqualTo(11003);
264-
softAssertions.assertThat(node.getType()).isEqualTo(NodeType.MASTER);
265-
softAssertions.assertThat(node.getFlags()).contains(Flag.MASTER);
266-
softAssertions.assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED);
267-
softAssertions.assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461);
268-
});
261+
assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6");
262+
assertThat(node.getHost()).isEqualTo("10.180.0.33");
263+
assertThat(node.hasValidHost()).isTrue();
264+
assertThat(node.getPort()).isEqualTo(11003);
265+
assertThat(node.getType()).isEqualTo(NodeType.MASTER);
266+
assertThat(node.getFlags()).contains(Flag.MASTER);
267+
assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED);
268+
assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461);
269269
}
270270

271-
@Test // https://github.com/spring-projects/spring-data-redis/issues/2862
271+
@Test // GH-2862
272272
void toClusterNodeWithIPv4Hostname() {
273+
273274
RedisClusterNode node = Converters.toClusterNode(CLUSTER_NODE_WITH_SINGLE_IPV4_HOSTNAME);
274275

275-
SoftAssertions.assertSoftly(softAssertions -> {
276-
softAssertions.assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6");
277-
softAssertions.assertThat(node.getHost()).isEqualTo("10.180.0.33");
278-
softAssertions.assertThat(node.getName()).isEqualTo("hostname1");
279-
softAssertions.assertThat(node.hasValidHost()).isTrue();
280-
softAssertions.assertThat(node.getPort()).isEqualTo(11003);
281-
softAssertions.assertThat(node.getType()).isEqualTo(NodeType.MASTER);
282-
softAssertions.assertThat(node.getFlags()).contains(Flag.MASTER);
283-
softAssertions.assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED);
284-
softAssertions.assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461);
285-
});
276+
assertThat(node.getId()).isEqualTo("3765733728631672640db35fd2f04743c03119c6");
277+
assertThat(node.getHost()).isEqualTo("10.180.0.33");
278+
assertThat(node.getName()).isEqualTo("hostname1");
279+
assertThat(node.hasValidHost()).isTrue();
280+
assertThat(node.getPort()).isEqualTo(11003);
281+
assertThat(node.getType()).isEqualTo(NodeType.MASTER);
282+
assertThat(node.getFlags()).contains(Flag.MASTER);
283+
assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED);
284+
assertThat(node.getSlotRange().getSlots().size()).isEqualTo(5461);
286285
}
287286

288287
@Test // GH-2678
@@ -308,34 +307,64 @@ void toClusterNodeWithInvalidIPv6Hostname() {
308307

309308
@ParameterizedTest // GH-2678
310309
@MethodSource("clusterNodesEndpoints")
311-
void shouldAcceptHostPatterns(String endpoint, String expectedAddress, String expectedPort, String expectedHostname) {
310+
void shouldAcceptHostPatterns(String endpoint, AddressPortHostname expected) {
312311

313-
ClusterNodesConverter.AddressPortHostname addressPortHostname = ClusterNodesConverter.AddressPortHostname.of(new String[] { "id", endpoint });
312+
AddressPortHostname addressPortHostname = AddressPortHostname.parse(endpoint);
314313

315-
assertThat(addressPortHostname.addressPart()).isEqualTo(expectedAddress);
316-
assertThat(addressPortHostname.portPart()).isEqualTo(expectedPort);
317-
assertThat(addressPortHostname.hostnamePart()).isEqualTo(expectedHostname);
314+
assertThat(addressPortHostname).isEqualTo(expected);
318315
}
319316

320317
static Stream<Arguments> clusterNodesEndpoints() {
321318

322-
return Stream.of(
319+
Stream<Arguments> regular = Stream.of(
323320
// IPv4 with Host, Redis 3
324-
Arguments.of("1.2.4.4:7379", "1.2.4.4", "7379", null),
321+
Arguments.of("1.2.4.4:7379", new AddressPortHostname("1.2.4.4", "7379", null)),
325322
// IPv6 with Host, Redis 3
326-
Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380", "6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null),
323+
Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380",
324+
new AddressPortHostname("6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)),
327325
// Assuming IPv6 in brackets with Host, Redis 3
328-
Arguments.of("[6b8:c67:9c:0:6d8b:33da:5a2c]:6380", "6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null),
326+
Arguments.of("[6b8:c67:9c:0:6d8b:33da:5a2c]:6380",
327+
new AddressPortHostname("6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)),
329328

330329
// IPv4 with Host and Bus Port, Redis 4
331-
Arguments.of("127.0.0.1:7382@17382", "127.0.0.1", "7382", null),
330+
Arguments.of("127.0.0.1:7382@17382", new AddressPortHostname("127.0.0.1", "7382", null)),
332331
// IPv6 with Host and Bus Port, Redis 4
333-
Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380", "6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null),
332+
Arguments.of("6b8:c67:9c:0:6d8b:33da:5a2c:6380",
333+
new AddressPortHostname("6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)),
334334

335335
// Hostname with Port and Bus Port, Redis 7
336-
Arguments.of("my.host-name.com:7379@17379", "my.host-name.com", "7379", null),
336+
Arguments.of("my.host-name.com:7379@17379", new AddressPortHostname("my.host-name.com", "7379", null)),
337337

338338
// With hostname, Redis 7
339-
Arguments.of("1.2.4.4:7379@17379,my.host-name.com", "1.2.4.4", "7379", "my.host-name.com"));
339+
Arguments.of("1.2.4.4:7379@17379,my.host-name.com",
340+
new AddressPortHostname("1.2.4.4", "7379", "my.host-name.com")));
341+
342+
Stream<Arguments> weird = Stream.of(
343+
// Port-only
344+
Arguments.of(":6380", new AddressPortHostname("", "6380", null)),
345+
346+
// Port-only with bus-port
347+
Arguments.of(":6380@6381", new AddressPortHostname("", "6380", null)),
348+
// IP with trailing comma
349+
Arguments.of("127.0.0.1:6380,", new AddressPortHostname("127.0.0.1", "6380", null)),
350+
// IPv6 with bus-port
351+
Arguments.of("2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381",
352+
new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)),
353+
// IPv6 with bus-port and hostname
354+
Arguments.of("2a02:6b8:c67:9c:0:6d8b:33da:5a2c:6380@6381,hostname1",
355+
new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", "hostname1")),
356+
// Port-only with hostname
357+
Arguments.of(":6380,hostname1", new AddressPortHostname("", "6380", "hostname1")),
358+
359+
// Port-only with bus-port
360+
Arguments.of(":6380@6381,hostname1", new AddressPortHostname("", "6380", "hostname1")),
361+
// IPv6 in brackets with bus-port
362+
Arguments.of("[2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381",
363+
new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", null)),
364+
// IPv6 in brackets with bus-port and hostname
365+
Arguments.of("[2a02:6b8:c67:9c:0:6d8b:33da:5a2c]:6380@6381,hostname1",
366+
new AddressPortHostname("2a02:6b8:c67:9c:0:6d8b:33da:5a2c", "6380", "hostname1")));
367+
368+
return Stream.concat(regular, weird);
340369
}
341370
}

0 commit comments

Comments
 (0)