diff --git a/src/main/java/org/springframework/data/redis/connection/ClusterTopology.java b/src/main/java/org/springframework/data/redis/connection/ClusterTopology.java index fbf8ec4a0..da9c2b23f 100644 --- a/src/main/java/org/springframework/data/redis/connection/ClusterTopology.java +++ b/src/main/java/org/springframework/data/redis/connection/ClusterTopology.java @@ -197,11 +197,11 @@ public class ClusterTopology { Assert.notNull(node, "RedisClusterNode must not be null!"); - if (nodes.contains(node) && StringUtils.hasText(node.getHost()) && StringUtils.hasText(node.getId())) { + if (nodes.contains(node) && node.hasValidHost() && StringUtils.hasText(node.getId())) { return node; } - if (StringUtils.hasText(node.getHost()) && node.getPort() != null) { + if (node.hasValidHost() && node.getPort() != null) { return lookup(node.getHost(), node.getPort()); } diff --git a/src/main/java/org/springframework/data/redis/connection/RedisNode.java b/src/main/java/org/springframework/data/redis/connection/RedisNode.java index d15768044..054fd4ac7 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisNode.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisNode.java @@ -18,6 +18,7 @@ package org.springframework.data.redis.connection; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; +import org.springframework.util.StringUtils; /** * @author Christoph Strobl @@ -50,6 +51,16 @@ public class RedisNode implements NamedNode { protected RedisNode() {} + private RedisNode(RedisNode redisNode) { + + this.id = redisNode.id; + this.name = redisNode.name; + this.host = redisNode.host; + this.port = redisNode.port; + this.type = redisNode.type; + this.masterId = redisNode.masterId; + } + /** * @return can be {@literal null}. */ @@ -58,6 +69,14 @@ public class RedisNode implements NamedNode { return host; } + /** + * @return whether this node has a valid host (not null and not empty). + * @since 2.3.8 + */ + public boolean hasValidHost() { + return StringUtils.hasText(host); + } + /** * @return can be {@literal null}. */ @@ -229,7 +248,7 @@ public class RedisNode implements NamedNode { */ public RedisNodeBuilder listeningAt(String host, int port) { - Assert.hasText(host, "Hostname must not be empty or null."); + Assert.notNull(host, "Hostname must not be null."); node.host = host; node.port = port; return this; @@ -290,7 +309,7 @@ public class RedisNode implements NamedNode { * @return */ public RedisNode build() { - return this.node; + return new RedisNode(this.node); } } 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 065bba6e1..0f8463b35 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 @@ -554,6 +554,7 @@ abstract public class Converters { static final int LINK_STATE_INDEX = 7; static final int SLOTS_INDEX = 8; + @Override public RedisClusterNode convert(String source) { String[] args = source.split(" "); diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java index 5c977ce96..e31ecc2eb 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java @@ -951,6 +951,11 @@ public class JedisClusterConnection implements DefaultedRedisClusterConnection { RedisClusterNode member = topologyProvider.getTopology().lookup(node); + if (!member.hasValidHost()) { + throw new DataAccessResourceFailureException(String + .format("Cannot obtain connection to node %ss as it is not associated with a hostname!", node.getId())); + } + if (member != null && connectionHandler != null) { return connectionHandler.getConnectionFromNode(new HostAndPort(member.getHost(), member.getPort())); } 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 07dd10806..78554c6ac 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 @@ -27,6 +27,8 @@ import org.springframework.data.redis.connection.RedisClusterNode.LinkState; import org.springframework.data.redis.connection.RedisNode.NodeType; /** + * Unit tests for {@link Converters}. + * * @author Christoph Strobl * @author Mark Paluch */ @@ -56,6 +58,8 @@ class ConvertersUnitTests { private static final String CLUSTER_NODE_IMPORTING_SLOT = "ef570f86c7b1a953846668debc177a3a16733420 127.0.0.1:6379 myself,master - 0 0 1 connected [5461-<-0f2ee5df45d18c50aca07228cc18b1da96fd5e84]"; + private static final String CLUSTER_NODE_WITHOUT_HOST = "ef570f86c7b1a953846668debc177a3a16733420 :6379 fail,master - 0 0 1 connected"; + @Test // DATAREDIS-315 void toSetOfRedis30ClusterNodesShouldConvertSingleStringNodesResponseCorrectly() { @@ -194,6 +198,23 @@ class ConvertersUnitTests { RedisClusterNode node = nodes.next(); assertThat(node.getId()).isEqualTo("ef570f86c7b1a953846668debc177a3a16733420"); assertThat(node.getHost()).isEqualTo("127.0.0.1"); + assertThat(node.hasValidHost()).isTrue(); + assertThat(node.getPort()).isEqualTo(6379); + assertThat(node.getType()).isEqualTo(NodeType.MASTER); + assertThat(node.getFlags()).contains(Flag.MASTER); + assertThat(node.getLinkState()).isEqualTo(LinkState.CONNECTED); + assertThat(node.getSlotRange().getSlots().size()).isEqualTo(0); + } + + @Test // GH-1985 + void toSetOfRedisClusterNodesShouldAllowEmptyHostname() { + + Iterator nodes = Converters.toSetOfRedisClusterNodes(CLUSTER_NODE_WITHOUT_HOST).iterator(); + + RedisClusterNode node = nodes.next(); + assertThat(node.getId()).isEqualTo("ef570f86c7b1a953846668debc177a3a16733420"); + assertThat(node.getHost()).isEmpty(); + assertThat(node.hasValidHost()).isFalse(); assertThat(node.getPort()).isEqualTo(6379); assertThat(node.getType()).isEqualTo(NodeType.MASTER); assertThat(node.getFlags()).contains(Flag.MASTER); diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java index bc9c21462..0fdbf19e4 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java @@ -383,6 +383,25 @@ class JedisClusterConnectionUnitTests { verify(con1Mock, times(2)).clusterNodes(); } + @Test // GH-1985 + void nodeWithoutHostShouldRejectConnectionAttempt() { + + reset(con1Mock, con2Mock, con3Mock); + + when(con1Mock.clusterNodes()) + .thenReturn("ef570f86c7b1a953846668debc177a3a16733420 :6379 fail,master - 0 0 1 connected"); + when(con2Mock.clusterNodes()) + .thenReturn("ef570f86c7b1a953846668debc177a3a16733420 :6379 fail,master - 0 0 1 connected"); + when(con3Mock.clusterNodes()) + .thenReturn("ef570f86c7b1a953846668debc177a3a16733420 :6379 fail,master - 0 0 1 connected"); + + JedisClusterConnection connection = new JedisClusterConnection(clusterMock); + + assertThatThrownBy(() -> connection.ping(new RedisClusterNode("ef570f86c7b1a953846668debc177a3a16733420"))) + .isInstanceOf(DataAccessResourceFailureException.class) + .hasMessageContaining("ef570f86c7b1a953846668debc177a3a16733420"); + } + static class StubJedisCluster extends JedisCluster { JedisClusterConnectionHandler connectionHandler;