You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ri...@apache.org on 2021/06/30 20:44:47 UTC
[geode] branch GEODE-9375-Implement-ZRANGE-Radish-command updated:
Parse ints early, return correct error
This is an automated email from the ASF dual-hosted git repository.
ringles pushed a commit to branch GEODE-9375-Implement-ZRANGE-Radish-command
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/GEODE-9375-Implement-ZRANGE-Radish-command by this push:
new ea0b543 Parse ints early, return correct error
ea0b543 is described below
commit ea0b543f67cbef6013aef1c87a0f638872b40461
Author: Ray Ingles <ri...@vmware.com>
AuthorDate: Wed Jun 30 16:43:42 2021 -0400
Parse ints early, return correct error
---
.../sortedset/AbstractZRangeIntegrationTest.java | 27 ++++++++++++++--------
.../redis/internal/data/NullRedisSortedSet.java | 2 +-
.../geode/redis/internal/data/RedisSortedSet.java | 8 +++----
.../RedisSortedSetCommandsFunctionExecutor.java | 2 +-
.../executor/sortedset/RedisSortedSetCommands.java | 2 +-
.../executor/sortedset/ZRangeExecutor.java | 11 +++++++--
.../redis/internal/data/RedisSortedSetTest.java | 25 ++++++++++----------
7 files changed, 45 insertions(+), 32 deletions(-)
diff --git a/geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeIntegrationTest.java b/geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeIntegrationTest.java
index 62817a6..196abd2 100755
--- a/geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeIntegrationTest.java
+++ b/geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeIntegrationTest.java
@@ -14,6 +14,7 @@
*/
package org.apache.geode.redis.internal.executor.sortedset;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
@@ -30,14 +31,7 @@ import org.apache.geode.redis.internal.RedisConstants;
public abstract class AbstractZRangeIntegrationTest implements RedisIntegrationTest {
private JedisCluster jedis;
- private final String member = "member";
- private final String incrOption = "INCR";
- private final double initial = 355.681000005;
- private final double increment = 9554257.921450001;
- private final double expected = initial + increment;
-
private static final String SORTED_SET_KEY = "ss_key";
- private static final int INITIAL_MEMBER_COUNT = 5;
@Before
public void setUp() {
@@ -60,11 +54,26 @@ public abstract class AbstractZRangeIntegrationTest implements RedisIntegrationT
}
@Test
+ public void shouldError_givenNonIntegerRangeValues() {
+ jedis.zadd(SORTED_SET_KEY, 1.0, "member");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(SORTED_SET_KEY, Protocol.Command.ZRANGE, SORTED_SET_KEY,
+ "NOT_AN_INT", "2"))
+ .hasMessageContaining(ERROR_NOT_INTEGER);
+ assertThatThrownBy(
+ () -> jedis.sendCommand(SORTED_SET_KEY, Protocol.Command.ZRANGE, SORTED_SET_KEY, "1",
+ "ALSO_NOT_AN_INT"))
+ .hasMessageContaining(ERROR_NOT_INTEGER);
+ }
+
+ @Test
public void shouldReturnSyntaxError_givenWrongWithscoresFlag() {
- jedis.zadd(SORTED_SET_KEY, 1.0, member);
+ jedis.zadd(SORTED_SET_KEY, 1.0, "member");
assertThatThrownBy(
() -> jedis.sendCommand(SORTED_SET_KEY, Protocol.Command.ZRANGE, SORTED_SET_KEY, "1", "2",
- "WITHSCOREZ"))
+ "WITSCOREZ"))
.hasMessageContaining(ERROR_SYNTAX);
}
+
+
}
diff --git a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSortedSet.java b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSortedSet.java
index b771be4..8028076 100644
--- a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSortedSet.java
+++ b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSortedSet.java
@@ -87,7 +87,7 @@ class NullRedisSortedSet extends RedisSortedSet {
}
@Override
- List<byte[]> zrange(byte[] min, byte[] max,
+ List<byte[]> zrange(int min, int max,
boolean withScores) {
return null;
}
diff --git a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
index 4a0f6a7..3ab0e10 100644
--- a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
+++ b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
@@ -19,7 +19,6 @@ package org.apache.geode.redis.internal.data;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_A_VALID_FLOAT;
import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SORTED_SET;
import static org.apache.geode.redis.internal.netty.Coder.bytesToDouble;
-import static org.apache.geode.redis.internal.netty.Coder.bytesToInt;
import static org.apache.geode.redis.internal.netty.Coder.doubleToBytes;
import java.io.DataInput;
@@ -313,11 +312,10 @@ public class RedisSortedSet extends AbstractRedisData {
return membersRemoved;
}
- List<byte[]> zrange(byte[] min, byte[] max,
- boolean withScores) {
+ List<byte[]> zrange(int min, int max, boolean withScores) {
ArrayList<byte[]> result = new ArrayList<>();
- int start = getBoundedStartIndex(bytesToInt(min), getSortedSetSize());
- int end = getBoundedEndIndex(bytesToInt(max), getSortedSetSize());
+ int start = getBoundedStartIndex(min, getSortedSetSize());
+ int end = getBoundedEndIndex(max, getSortedSetSize());
if (start > end || start == getSortedSetSize()) {
return result;
}
diff --git a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java
index ae106d4..08c8923 100644
--- a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java
+++ b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java
@@ -41,7 +41,7 @@ public class RedisSortedSetCommandsFunctionExecutor extends RedisDataCommandsFun
}
@Override
- public List<byte[]> zrange(RedisKey key, byte[] min, byte[] max, boolean withScores) {
+ public List<byte[]> zrange(RedisKey key, int min, int max, boolean withScores) {
return stripedExecute(key,
() -> getRedisSortedSet(key, false).zrange(min, max, withScores));
}
diff --git a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/RedisSortedSetCommands.java b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/RedisSortedSetCommands.java
index 268d760..014130d 100644
--- a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/RedisSortedSetCommands.java
+++ b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/RedisSortedSetCommands.java
@@ -23,7 +23,7 @@ public interface RedisSortedSetCommands {
Object zadd(RedisKey key, List<byte[]> scoresAndMembersToAdd, ZAddOptions options);
- List<byte[]> zrange(RedisKey key, byte[] min, byte[] max, boolean withScores);
+ List<byte[]> zrange(RedisKey key, int min, int max, boolean withScores);
byte[] zscore(RedisKey key, byte[] member);
diff --git a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeExecutor.java b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeExecutor.java
index d583c92..ad75b7a 100644
--- a/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeExecutor.java
+++ b/geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeExecutor.java
@@ -14,7 +14,9 @@
*/
package org.apache.geode.redis.internal.executor.sortedset;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToInt;
import static org.apache.geode.redis.internal.netty.Coder.equalsIgnoreCaseBytes;
import java.util.List;
@@ -29,11 +31,16 @@ public class ZRangeExecutor extends AbstractExecutor {
@Override
public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
boolean withScores = false;
+ int min, max;
RedisSortedSetCommands redisSortedSetCommands = context.getSortedSetCommands();
List<byte[]> commandElements = command.getProcessedCommand();
- byte[] min = commandElements.get(1);
- byte[] max = commandElements.get(2);
+ try {
+ min = bytesToInt(commandElements.get(1));
+ max = bytesToInt(commandElements.get(2));
+ } catch (NumberFormatException nfe) {
+ return RedisResponse.error(ERROR_NOT_INTEGER);
+ }
if (commandElements.size() == 5) {
if (equalsIgnoreCaseBytes(commandElements.get(3), "WITHSCORES".getBytes())) {
withScores = true;
diff --git a/geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java b/geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
index 9ab3b0b..5537cbe 100644
--- a/geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
+++ b/geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
@@ -18,7 +18,6 @@ package org.apache.geode.redis.internal.data;
import static java.lang.Math.round;
import static org.apache.geode.redis.internal.data.RedisSortedSet.BASE_REDIS_SORTED_SET_OVERHEAD;
-import static org.apache.geode.redis.internal.netty.Coder.intToBytes;
import static org.apache.geode.redis.internal.netty.Coder.stringToBytes;
import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
import static org.assertj.core.api.Assertions.assertThat;
@@ -313,57 +312,57 @@ public class RedisSortedSetTest {
@Test
public void zrange_ShouldReturnEmptyList_GivenInvalidRanges() {
- Collection<byte[]> rangeList = rangeSortedSet.zrange(intToBytes(5), intToBytes(0), false);
+ Collection<byte[]> rangeList = rangeSortedSet.zrange(5, 0, false);
assertThat(rangeList).isEmpty();
- rangeList = rangeSortedSet.zrange(intToBytes(13), intToBytes(15), false);
+ rangeList = rangeSortedSet.zrange(13, 15, false);
assertThat(rangeList).isEmpty();
- rangeList = rangeSortedSet.zrange(intToBytes(17), intToBytes(-2), false);
+ rangeList = rangeSortedSet.zrange(17, -2, false);
assertThat(rangeList).isEmpty();
- rangeList = rangeSortedSet.zrange(intToBytes(12), intToBytes(12), false);
+ rangeList = rangeSortedSet.zrange(12, 12, false);
assertThat(rangeList).isEmpty();
}
@Test
public void zrange_ShouldReturnSimpleRanges() {
- Collection<byte[]> rangeList = rangeSortedSet.zrange(intToBytes(0), intToBytes(5), false);
+ Collection<byte[]> rangeList = rangeSortedSet.zrange(0, 5, false);
assertThat(rangeList).hasSize(6);
assertThat(rangeList)
.containsExactly("member1".getBytes(), "member2".getBytes(), "member3".getBytes(),
"member4".getBytes(), "member5".getBytes(), "member6".getBytes());
- rangeList = rangeSortedSet.zrange(intToBytes(5), intToBytes(10), false);
+ rangeList = rangeSortedSet.zrange(5, 10, false);
assertThat(rangeList).hasSize(6);
assertThat(rangeList)
.containsExactly("member6".getBytes(), "member7".getBytes(), "member8".getBytes(),
"member9".getBytes(), "member10".getBytes(), "member11".getBytes());
- rangeList = rangeSortedSet.zrange(intToBytes(10), intToBytes(13), false);
+ rangeList = rangeSortedSet.zrange(10, 13, false);
assertThat(rangeList).hasSize(2);
assertThat(rangeList).containsExactly("member11".getBytes(), "member12".getBytes());
}
@Test
public void zrange_ShouldReturnRanges_SpecifiedWithNegativeOffsets() {
- Collection<byte[]> rangeList = rangeSortedSet.zrange(intToBytes(-2), intToBytes(12), false);
+ Collection<byte[]> rangeList = rangeSortedSet.zrange(-2, 12, false);
assertThat(rangeList).hasSize(2);
assertThat(rangeList).containsExactly("member11".getBytes(), "member12".getBytes());
- rangeList = rangeSortedSet.zrange(intToBytes(-6), intToBytes(-1), false);
+ rangeList = rangeSortedSet.zrange(-6, -1, false);
assertThat(rangeList).hasSize(6);
assertThat(rangeList)
.containsExactly("member7".getBytes(), "member8".getBytes(),
"member9".getBytes(), "member10".getBytes(), "member11".getBytes(),
"member12".getBytes());
- rangeList = rangeSortedSet.zrange(intToBytes(-11), intToBytes(-5), false);
+ rangeList = rangeSortedSet.zrange(-11, -5, false);
assertThat(rangeList).hasSize(7);
assertThat(rangeList)
.containsExactly("member2".getBytes(), "member3".getBytes(),
"member4".getBytes(), "member5".getBytes(), "member6".getBytes(),
"member7".getBytes(), "member8".getBytes());
- rangeList = rangeSortedSet.zrange(intToBytes(-12), intToBytes(-11), false);
+ rangeList = rangeSortedSet.zrange(-12, -11, false);
assertThat(rangeList).hasSize(2);
assertThat(rangeList)
.containsExactly("member1".getBytes(), "member2".getBytes());
@@ -371,7 +370,7 @@ public class RedisSortedSetTest {
@Test
public void zrange_shouldAlsoReturnScores_whenWithScoresSpecified() {
- Collection<byte[]> rangeList = rangeSortedSet.zrange(intToBytes(0), intToBytes(5), true);
+ Collection<byte[]> rangeList = rangeSortedSet.zrange(0, 5, true);
assertThat(rangeList).hasSize(12);
assertThat(rangeList).containsExactly("member1".getBytes(), "1.0".getBytes(),
"member2".getBytes(), "1.1".getBytes(), "member3".getBytes(), "1.2".getBytes(),