Skip to content

Commit cf31202

Browse files
sazzad16sazzad16
authored
JedisClusterCRC16.getSlot() wraps NullPointerException (#1774)
* This is similar to SafeEncoder.encode(String). * This helps JedisClusterCommand specially in case of multi key operations. Also includes, * Move CRC16 tests in one place. * Fix a wrong usage of DEFAULT_REDIRECTIONS.
1 parent c0b8ac0 commit cf31202

File tree

5 files changed

+88
-40
lines changed

5 files changed

+88
-40
lines changed

‎src/main/java/redis/clients/jedis/JedisClusterCommand.java‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@
1111

1212
public abstract class JedisClusterCommand<T> {
1313

14-
private static final String NO_DISPATCH_MESSAGE = "No way to dispatch this command to Redis Cluster.";
15-
1614
private final JedisClusterConnectionHandler connectionHandler;
1715
private final int maxAttempts;
1816
private final ThreadLocal<Jedis> askConnection = new ThreadLocal<Jedis>();
@@ -25,16 +23,12 @@ public JedisClusterCommand(JedisClusterConnectionHandler connectionHandler, int
2523
public abstract T execute(Jedis connection);
2624

2725
public T run(String key) {
28-
if (key == null) {
29-
throw new JedisClusterOperationException(NO_DISPATCH_MESSAGE);
30-
}
31-
3226
return runWithRetries(JedisClusterCRC16.getSlot(key), this.maxAttempts, false, false);
3327
}
3428

3529
public T run(int keyCount, String... keys) {
3630
if (keys == null || keys.length == 0) {
37-
throw new JedisClusterOperationException(NO_DISPATCH_MESSAGE);
31+
throw new JedisClusterOperationException("No way to dispatch this command to Redis Cluster.");
3832
}
3933

4034
// For multiple keys, only execute if they all share the same connection slot.
@@ -53,16 +47,12 @@ public T run(int keyCount, String... keys) {
5347
}
5448

5549
public T runBinary(byte[] key) {
56-
if (key == null) {
57-
throw new JedisClusterOperationException(NO_DISPATCH_MESSAGE);
58-
}
59-
6050
return runWithRetries(JedisClusterCRC16.getSlot(key), this.maxAttempts, false, false);
6151
}
6252

6353
public T runBinary(int keyCount, byte[]... keys) {
6454
if (keys == null || keys.length == 0) {
65-
throw new JedisClusterOperationException(NO_DISPATCH_MESSAGE);
55+
throw new JedisClusterOperationException("No way to dispatch this command to Redis Cluster.");
6656
}
6757

6858
// For multiple keys, only execute if they all share the same connection slot.

‎src/main/java/redis/clients/jedis/util/JedisClusterCRC16.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package redis.clients.jedis.util;
22

3+
import redis.clients.jedis.exceptions.JedisClusterOperationException;
4+
35
/**
46
* CRC16 Implementation according to CCITT standard Polynomial : 1021 (x^16 + x^12 + x^5 + 1) See <a
57
* href="http://redis.io/topics/cluster-spec">Appendix A. CRC16 reference implementation in ANSI
@@ -36,13 +38,20 @@ private JedisClusterCRC16(){
3638
}
3739

3840
public static int getSlot(String key) {
41+
if (key == null) {
42+
throw new JedisClusterOperationException("Slot calculation of null is impossible");
43+
}
44+
3945
key = JedisClusterHashTagUtil.getHashTag(key);
40-
// optimization with modulo operator with power of 2
41-
// equivalent to getCRC16(key) % 16384
46+
// optimization with modulo operator with power of 2 equivalent to getCRC16(key) % 16384
4247
return getCRC16(key) & (16384 - 1);
4348
}
4449

4550
public static int getSlot(byte[] key) {
51+
if (key == null) {
52+
throw new JedisClusterOperationException("Slot calculation of null is impossible");
53+
}
54+
4655
int s = -1;
4756
int e = -1;
4857
boolean sFound = false;

‎src/test/java/redis/clients/jedis/tests/JedisClusterTest.java‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import static org.junit.Assert.assertEquals;
44
import static org.junit.Assert.assertFalse;
5-
import static org.junit.Assert.assertNotEquals;
65
import static org.junit.Assert.assertNotNull;
76
import static org.junit.Assert.assertNull;
87
import static org.junit.Assert.fail;
@@ -376,16 +375,6 @@ public void testRedisClusterMaxRedirections() {
376375
jc.set("51", "foo");
377376
}
378377

379-
@Test
380-
public void testRedisHashtag() {
381-
assertEquals(JedisClusterCRC16.getSlot("{user1000}.following"),
382-
JedisClusterCRC16.getSlot("{user1000}.followers"));
383-
assertEquals(JedisClusterCRC16.getSlot("bar"), JedisClusterCRC16.getSlot("foo{bar}{zap}"));
384-
assertNotEquals(JedisClusterCRC16.getSlot("bar"), JedisClusterCRC16.getSlot("foo{}{bar}"));
385-
assertNotEquals(JedisClusterCRC16.getSlot(""), JedisClusterCRC16.getSlot("foo{}{bar}"));
386-
assertEquals(JedisClusterCRC16.getSlot("{bar"), JedisClusterCRC16.getSlot("foo{{bar}}zap"));
387-
}
388-
389378
@Test
390379
public void testClusterForgetNode() throws InterruptedException {
391380
// at first, join node4 to cluster
@@ -642,6 +631,59 @@ public void testInvalidStartNodeNotAdded() {
642631
assertFalse(clusterNodes.containsKey(JedisClusterInfoCache.getNodeKey(invalidHost)));
643632
}
644633

634+
@Test
635+
public void nullKeys() {
636+
Set<HostAndPort> jedisClusterNode = new HashSet<HostAndPort>();
637+
jedisClusterNode.add(new HostAndPort(nodeInfo1.getHost(), nodeInfo1.getPort()));
638+
JedisCluster cluster = new JedisCluster(jedisClusterNode, DEFAULT_TIMEOUT, DEFAULT_TIMEOUT,
639+
DEFAULT_REDIRECTIONS, "cluster", DEFAULT_CONFIG);
640+
641+
String foo = "foo";
642+
byte[] bfoo = new byte[]{0x0b, 0x0f, 0x00, 0x00};
643+
644+
try {
645+
cluster.exists((String) null);
646+
fail();
647+
} catch (JedisClusterOperationException coe) {
648+
// expected
649+
}
650+
651+
try {
652+
cluster.exists(foo, null);
653+
fail();
654+
} catch (JedisClusterOperationException coe) {
655+
// expected
656+
}
657+
658+
try {
659+
cluster.exists(null, foo);
660+
fail();
661+
} catch (JedisClusterOperationException coe) {
662+
// expected
663+
}
664+
665+
try {
666+
cluster.exists((byte[]) null);
667+
fail();
668+
} catch (JedisClusterOperationException coe) {
669+
// expected
670+
}
671+
672+
try {
673+
cluster.exists(bfoo, null);
674+
fail();
675+
} catch (JedisClusterOperationException coe) {
676+
// expected
677+
}
678+
679+
try {
680+
cluster.exists(null, bfoo);
681+
fail();
682+
} catch (JedisClusterOperationException coe) {
683+
// expected
684+
}
685+
}
686+
645687
private static String getNodeServingSlotRange(String infoOutput) {
646688
// f4f3dc4befda352a4e0beccf29f5e8828438705d 127.0.0.1:7380 master - 0
647689
// 1394372400827 0 connected 5461-10922

‎src/test/java/redis/clients/jedis/tests/commands/ClusterBinaryJedisCommandsTest.java‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import static org.junit.Assert.assertArrayEquals;
44
import static org.junit.Assert.assertEquals;
5-
import static org.junit.Assert.assertNotEquals;
65
import static org.junit.Assert.assertTrue;
76

87
import java.util.ArrayList;
@@ -180,20 +179,6 @@ public void failKeys() {
180179
jedisCluster.keys("*".getBytes());
181180
}
182181

183-
@Test
184-
public void testGetSlot() {
185-
assertEquals(JedisClusterCRC16.getSlot("{user1000}.following".getBytes()),
186-
JedisClusterCRC16.getSlot("{user1000}.followers".getBytes()));
187-
assertEquals(JedisClusterCRC16.getSlot("bar".getBytes()),
188-
JedisClusterCRC16.getSlot("foo{bar}{zap}".getBytes()));
189-
assertNotEquals(JedisClusterCRC16.getSlot("bar".getBytes()),
190-
JedisClusterCRC16.getSlot("foo{}{bar}".getBytes()));
191-
assertNotEquals(JedisClusterCRC16.getSlot(new byte[0]),
192-
JedisClusterCRC16.getSlot("foo{}{bar}".getBytes()));
193-
assertEquals(JedisClusterCRC16.getSlot("{bar".getBytes()),
194-
JedisClusterCRC16.getSlot("foo{{bar}}zap".getBytes()));
195-
}
196-
197182
private static String getNodeId(String infoOutput) {
198183
for (String infoLine : infoOutput.split("\n")) {
199184
if (infoLine.contains("myself")) {

‎src/test/java/redis/clients/jedis/tests/utils/JedisClusterCRC16Test.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package redis.clients.jedis.tests.utils;
22

33
import static org.junit.Assert.assertEquals;
4+
import static org.junit.Assert.assertNotEquals;
45

56
import java.util.HashMap;
67
import java.util.Map;
@@ -42,4 +43,25 @@ private Map<String, Integer> prepareSolutionSet() {
4243
solutionMap.put("Hello, World!", 0x4FD6);
4344
return solutionMap;
4445
}
46+
47+
@Test
48+
public void testRedisHashtagGetSlot() {
49+
assertEquals(JedisClusterCRC16.getSlot("{bar"), JedisClusterCRC16.getSlot("foo{{bar}}zap"));
50+
assertEquals(JedisClusterCRC16.getSlot("{user1000}.following"),
51+
JedisClusterCRC16.getSlot("{user1000}.followers"));
52+
assertNotEquals(JedisClusterCRC16.getSlot("foo{}{bar}"), JedisClusterCRC16.getSlot("bar"));
53+
assertEquals(JedisClusterCRC16.getSlot("foo{bar}{zap}"), JedisClusterCRC16.getSlot("bar"));
54+
}
55+
56+
@Test
57+
public void testBinaryHashtagGetSlot() {
58+
assertEquals(JedisClusterCRC16.getSlot("{bar".getBytes()), JedisClusterCRC16.getSlot("{bar".getBytes()));
59+
assertEquals(JedisClusterCRC16.getSlot("{user1000}.following".getBytes()),
60+
JedisClusterCRC16.getSlot("{user1000}.followers".getBytes()));
61+
assertNotEquals(JedisClusterCRC16.getSlot("foo{}{bar}".getBytes()),
62+
JedisClusterCRC16.getSlot("bar".getBytes()));
63+
assertEquals(JedisClusterCRC16.getSlot("foo{bar}{zap}".getBytes()),
64+
JedisClusterCRC16.getSlot("bar".getBytes()));
65+
}
66+
4567
}

0 commit comments

Comments
 (0)