diff --git a/pom.xml b/pom.xml index b20e7c9dcc..f038682fb1 100644 --- a/pom.xml +++ b/pom.xml @@ -1,11 +1,13 @@ - + 4.0.0 org.springframework.data spring-data-redis - 2.5.0-SNAPSHOT + 2.5.0-GH-1985-SNAPSHOT Spring Data Redis 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 fbf8ec4a04..da9c2b23fb 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 RedisClusterNode lookup(RedisClusterNode node) { 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 d157680444..9b6357fe3a 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 @@ 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 RedisNode(String host, int port) { 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 String getHost() { return host; } + /** + * @return whether this node has a valid host (not null and not empty). + * @since 2.5 + */ + public boolean hasValidHost() { + return StringUtils.hasText(host); + } + /** * @return can be {@literal null}. */ @@ -229,7 +248,7 @@ public RedisNodeBuilder withName(String name) { */ 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 RedisNodeBuilder replicaOf(String masterId) { * @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 065bba6e19..ab679f81a6 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 @@ -178,7 +178,7 @@ public static Set toSetOfRedisClusterNodes(Collection */ public static Set toSetOfRedisClusterNodes(String clusterNodes) { - if (StringUtils.isEmpty(clusterNodes)) { + if (!StringUtils.hasText(clusterNodes)) { return Collections.emptySet(); } @@ -554,6 +554,7 @@ enum ClusterNodesConverter implements Converter { 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 5c977ce96c..75021e96b6 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 @@ private Jedis getConnectionForSpecificNode(RedisClusterNode node) { RedisClusterNode member = topologyProvider.getTopology().lookup(node); + if (!member.hasValidHost()) { + throw new DataAccessResourceFailureException( + "Cannot obtain connection to node " + node.getId() + " as it is not associated with a hostname"); + } + if (member != null && connectionHandler != null) { return connectionHandler.getConnectionFromNode(new HostAndPort(member.getHost(), member.getPort())); } diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisVersionUtil.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisVersionUtil.java index c11571844c..2b65b5bbef 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisVersionUtil.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisVersionUtil.java @@ -15,16 +15,15 @@ */ package org.springframework.data.redis.connection.jedis; +import redis.clients.jedis.Jedis; + import java.io.IOException; import java.util.Properties; import org.springframework.core.io.support.PropertiesLoaderUtils; -import org.springframework.data.redis.Version; -import org.springframework.data.redis.VersionParser; +import org.springframework.data.util.Version; import org.springframework.util.StringUtils; -import redis.clients.jedis.Jedis; - /** * @author Christoph Strobl * @since 1.3 @@ -47,7 +46,7 @@ public static Version jedisVersion() { * @return */ static Version parseVersion(String version) { - return VersionParser.parseVersion(version); + return Version.parse(version); } /** 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 07dd10806a..78554c6aca 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.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 @@ void toSetOfRedisClusterNodesShouldIgnoreImportingSlot() { 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 bc9c214621..0fdbf19e49 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 @@ void clusterTopologyProviderShouldRequestTopology() { 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;