GEODE-8852: Add additional tests for Redis HVALS command (#5931)
* Also make error response assertions more exact
Co-authored-by: Ray Ingles <ringles@vmware.com>
diff --git a/geode-docs/tools_modules/redis_api_for_geode.html.md.erb b/geode-docs/tools_modules/redis_api_for_geode.html.md.erb
index dfb17e3..2887109 100644
--- a/geode-docs/tools_modules/redis_api_for_geode.html.md.erb
+++ b/geode-docs/tools_modules/redis_api_for_geode.html.md.erb
@@ -68,7 +68,7 @@
**Note**: These commands are supported for Redis 5.
- **Connection**: AUTH, PING, QUIT
-- **Hashes**: HGETALL, HMSET, HSET
+- **Hashes**: HGETALL, HMSET, HSET, HVALS
- **Keys**: DEL, EXISTS, EXPIRE, EXPIREAT, KEYS, PERSIST, PEXPIRE, PEXPIREAT, PTTL, RENAME, TTL,
TYPE
- **Publish/Subscribe**: PUBLISH, PSUBSCRIBE, PUNSUBSCRIBE, SUBSCRIBE, UNSUBSCRIBE
@@ -81,7 +81,7 @@
- **Connection**: ECHO, SELECT
- **Hashes**: HDEL, HEXISTS, HGET, HINCRBY, HINCRBYFLOAT, HKEYS, HLEN, HMGET, HSCAN, HSETNX,
- HSTRLEN, HVALS
+ HSTRLEN
- **Keys**: SCAN, UNLINK
- **Server**: DBSIZE, FLUSHALL (no async option), FLUSHDB (no async option), INFO, SHUTDOWN,
SLOWLOG, TIME
diff --git a/geode-redis/README.md b/geode-redis/README.md
index c9b8b96..72d4a36 100644
--- a/geode-redis/README.md
+++ b/geode-redis/README.md
@@ -167,51 +167,51 @@
| HGETALL | FLUSHALL | ACL LOG |
| HMSET | FLUSHDB | ACL SAVE |
| HSET | GETBIT | ACL SETUSER |
-| KEYS | GETRANGE | ACL USERS |
-| PERSIST | GETSET | ACL WHOAMI |
-| PEXPIRE | HDEL | BGREWRITEAOF |
-| PEXPIREAT | HEXISTS | BGSAVE |
-| PING | HGET | BITFIELD |
-| PSUBSCRIBE | HINCRBY | BLPOP |
-| PTTL | HINCRBYFLOAT | BRPOP |
-| PUBLISH | HKEYS | BRPOPLPUSH |
-| PUNSUBSCRIBE | HLEN | BZPOPMAX |
-| QUIT | HMGET | BZPOPMIN |
-| RENAME | HSCAN | CLIENT CACHING |
-| SADD | HSETNX | CLIENT GETNAME |
-| SET | HSTRLEN | CLIENT GETREDIR |
-| SMEMBERS | HVALS | CLIENT ID |
-| SREM | INCR | CLIENT KILL |
-| SUBSCRIBE | INCRBY | CLIENT LIST |
-| TTL | INCRBYFLOAT | CLIENT PAUSE |
-| TYPE | INFO | CLIENT REPLY |
-| UNSUBSCRIBE | MGET | CLIENT SETNAME |
-| | MSET | CLIENT TRACKING |
-| | MSETNX | CLIENT UNBLOCK |
-| | PSETEX | CLUSTER ADDSLOTS |
-| | SCAN | CLUSTER BUMPEPOCH |
-| | SCARD | CLUSTER COUNT-FAILURE-REPORTS |
-| | SDIFF | CLUSTER COUNTKEYSINSLOT |
-| | SDIFFSTORE | CLUSTER DELSLOTS |
-| | SELECT | CLUSTER FAILOVER |
-| | SETBIT | CLUSTER FLUSHSLOTS |
-| | SETEX | CLUSTER FORGET |
-| | SETNX | CLUSTER GETKEYSINSLOT |
-| | SETRANGE | CLUSTER INFO |
-| | SHUTDOWN | CLUSTER KEYSLOT |
-| | SINTER | CLUSTER MEET |
-| | SINTERSTORE | CLUSTER MYID |
-| | SISMEMBER | CLUSTER NODES |
-| | SLOWLOG | CLUSTER REPLICAS |
-| | SMOVE | CLUSTER REPLICATE |
-| | SPOP | CLUSTER RESET |
-| | SRANDMEMBER | CLUSTER SAVECONFIG |
-| | SSCAN | CLUSTER SET-CONFIG-EPOCH |
-| | STRLEN | CLUSTER SETSLOT |
-| | SUNION | CLUSTER SLAVES |
-| | SUNIONSTORE | CLUSTER SLOTS |
-| | TIME | COMMAND |
-| | UNLINK [1] | COMMAND COUNT |
+| HVALS | GETRANGE | ACL USERS |
+| KEYS | GETSET | ACL WHOAMI |
+| PERSIST | HDEL | BGREWRITEAOF |
+| PEXPIRE | HEXISTS | BGSAVE |
+| PEXPIREAT | HGET | BITFIELD |
+| PING | HINCRBY | BLPOP |
+| PSUBSCRIBE | HINCRBYFLOAT | BRPOP |
+| PTTL | HKEYS | BRPOPLPUSH |
+| PUBLISH | HLEN | BZPOPMAX |
+| PUNSUBSCRIBE | HMGET | BZPOPMIN |
+| QUIT | HSCAN | CLIENT CACHING |
+| RENAME | HSETNX | CLIENT GETNAME |
+| SADD | HSTRLEN | CLIENT GETREDIR |
+| SET | INCR | CLIENT ID |
+| SMEMBERS | INCRBY | CLIENT KILL |
+| SREM | INCRBYFLOAT | CLIENT LIST |
+| SUBSCRIBE | INFO | CLIENT PAUSE |
+| TTL | MGET | CLIENT REPLY |
+| TYPE | MSET | CLIENT SETNAME |
+| UNSUBSCRIBE | MSETNX | CLIENT TRACKING |
+| | PSETEX | CLIENT UNBLOCK |
+| | SCAN | CLUSTER ADDSLOTS |
+| | SCARD | CLUSTER BUMPEPOCH |
+| | SDIFF | CLUSTER COUNT-FAILURE-REPORTS |
+| | SDIFFSTORE | CLUSTER COUNTKEYSINSLOT |
+| | SELECT | CLUSTER DELSLOTS |
+| | SETBIT | CLUSTER FAILOVER |
+| | SETEX | CLUSTER FLUSHSLOTS |
+| | SETNX | CLUSTER FORGET |
+| | SETRANGE | CLUSTER GETKEYSINSLOT |
+| | SHUTDOWN | CLUSTER INFO |
+| | SINTER | CLUSTER KEYSLOT |
+| | SINTERSTORE | CLUSTER MEET |
+| | SISMEMBER | CLUSTER MYID |
+| | SLOWLOG | CLUSTER NODES |
+| | SMOVE | CLUSTER REPLICAS |
+| | SPOP | CLUSTER REPLICATE |
+| | SRANDMEMBER | CLUSTER RESET |
+| | SSCAN | CLUSTER SAVECONFIG |
+| | STRLEN | CLUSTER SET-CONFIG-EPOCH |
+| | SUNION | CLUSTER SETSLOT |
+| | SUNIONSTORE | CLUSTER SLAVES |
+| | TIME | CLUSTER SLOTS |
+| | UNLINK [1] | COMMAND |
+| | | COMMAND COUNT |
| | | COMMAND GETKEYS |
| | | COMMAND INFO |
| | | CONFIG GET |
diff --git a/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HvalsDUnitTest.java b/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HvalsDUnitTest.java
new file mode 100644
index 0000000..60ea735
--- /dev/null
+++ b/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HvalsDUnitTest.java
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.hash;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import java.util.Random;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class HvalsDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static Jedis jedis1;
+ private static Jedis jedis2;
+ private static Jedis jedis3;
+
+ @BeforeClass
+ public static void classSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+
+ int redisServerPort1 = clusterStartUp.getRedisPort(1);
+ int redisServerPort2 = clusterStartUp.getRedisPort(2);
+
+ jedis1 = new Jedis(LOCAL_HOST, redisServerPort1, JEDIS_TIMEOUT);
+ jedis2 = new Jedis(LOCAL_HOST, redisServerPort2, JEDIS_TIMEOUT);
+ jedis3 = new Jedis(LOCAL_HOST, redisServerPort1, JEDIS_TIMEOUT);
+ }
+
+ @Before
+ public void testSetup() {
+ jedis1.flushAll();
+ }
+
+ @Test
+ public void hvalsWorks_whileAlsoUpdatingHash() {
+ String key = "key";
+ int fieldCount = 100;
+ int iterations = 10000;
+ Random rand = new Random();
+
+ for (int i = 0; i < fieldCount; i++) {
+ jedis1.hset(key, "field-" + i, "" + i);
+ }
+
+ new ConcurrentLoopingThreads(iterations,
+ (i) -> {
+ int x = rand.nextInt(fieldCount);
+ String field = "field-" + x;
+ String currentValue = jedis1.hget(key, field);
+ jedis1.hset(key, field, "" + (Long.parseLong(currentValue) + i));
+ },
+ (i) -> assertThat(jedis2.hvals(key)).hasSize(fieldCount),
+ (i) -> assertThat(jedis3.hvals(key)).hasSize(fieldCount))
+ .run();
+
+ List<String> values = jedis1.hvals(key);
+ long finalTotal = values.stream().mapToLong(Long::valueOf).sum();
+
+ // Spell out the formula for a sum of an arithmetic sequence which is: (n / 2) * (start + end)
+ long sumOfBothSequenceSums = (fieldCount / 2) * ((fieldCount - 1) + 0) +
+ (iterations / 2) * ((iterations - 1) + 0);
+ assertThat(finalTotal).isEqualTo(sumOfBothSequenceSums);
+ }
+
+}
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java
index 7212416..3b66ecf 100755
--- a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/hash/AbstractHashesIntegrationTest.java
@@ -39,6 +39,7 @@
import redis.clients.jedis.exceptions.JedisDataException;
import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.internal.RedisConstants;
import org.apache.geode.test.dunit.rules.RedisPortSupplier;
public abstract class AbstractHashesIntegrationTest implements RedisPortSupplier {
@@ -69,11 +70,12 @@
@Test
public void testHMSet_givenWrongNumberOfArguments() {
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMSET))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hmset' command");
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMSET, "1"))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hmset' command");
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMSET, "1", "2"))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hmset' command");
+ // Redis is somewhat inconsistent with the error response here
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMSET, "1", "2", "3", "4"))
.hasMessageContaining("wrong number of arguments");
}
@@ -81,11 +83,12 @@
@Test
public void testHSet_givenWrongNumberOfArguments() {
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSET))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hset' command");
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSET, "1"))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hset' command");
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSET, "1", "2"))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hset' command");
+ // Redis is somewhat inconsistent with the error response here
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSET, "1", "2", "3", "4"))
.hasMessageContaining("wrong number of arguments");
}
@@ -93,9 +96,9 @@
@Test
public void testHGetall_givenWrongNumberOfArguments() {
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HGETALL))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hgetall' command");
assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMSET, "1", "2"))
- .hasMessageContaining("wrong number of arguments");
+ .hasMessage("ERR wrong number of arguments for 'hmset' command");
}
@Test
@@ -167,7 +170,7 @@
assertThatThrownBy(() -> {
jedis.hdel("farm", "chicken");
}).isInstanceOf(JedisDataException.class)
- .hasMessageContaining("WRONGTYPE Operation against a key holding the wrong kind of value");
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
@Test
@@ -192,11 +195,11 @@
public void testHStrLen_failsForNonHashes() {
jedis.sadd("farm", "chicken");
assertThatThrownBy(() -> jedis.hstrlen("farm", "chicken"))
- .hasMessageContaining("WRONGTYPE");
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
jedis.set("tractor", "John Deere");
assertThatThrownBy(() -> jedis.hstrlen("tractor", "chicken"))
- .hasMessageContaining("WRONGTYPE");
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
@Test
@@ -373,23 +376,41 @@
String key = "HVals_key";
String field1 = "field_1";
String field2 = "field_2";
- String value = "value";
+ String value1 = "value_1";
+ String value2 = "value_2";
- List<String> list = jedis.hvals(key);
- assertThat(list == null || list.isEmpty()).isTrue();
+ List<String> list = jedis.hvals("non-existent-key");
+ assertThat(list).isEmpty();
- Long result = jedis.hset(key, field1, value);
+ Long result = jedis.hset(key, field1, value1);
assertThat(result).isEqualTo(1);
- result = jedis.hset(key, field2, value);
+ result = jedis.hset(key, field2, value2);
assertThat(result).isEqualTo(1);
list = jedis.hvals(key);
- assertThat(list).isNotNull();
- assertThat(list).isNotEmpty();
assertThat(list).hasSize(2);
+ assertThat(list).contains(value1, value2);
+ }
- assertThat(list).contains(value);
+ @Test
+ public void hvalsFailsForNonHash() {
+ jedis.sadd("farm", "chicken");
+ assertThatThrownBy(() -> jedis.hvals("farm"))
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+
+ jedis.set("tractor", "John Deere");
+ assertThatThrownBy(() -> jedis.hvals("tractor"))
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void hvalsFails_withIncorrectParameters() {
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HVALS))
+ .hasMessage("ERR wrong number of arguments for 'hvals' command");
+
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HVALS, "1", "too-many"))
+ .hasMessage("ERR wrong number of arguments for 'hvals' command");
}
/**
@@ -637,7 +658,7 @@
assertThatThrownBy(
() -> jedis.hset("key", "field", "something else")).isInstanceOf(JedisDataException.class)
- .hasMessageContaining("WRONGTYPE");
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
}
@Test
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
index beba759..044a1d1 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
@@ -160,6 +160,7 @@
new MinimumParameterRequirements(4).and(new EvenParameterRequirements())),
HSET(new HSetExecutor(), SUPPORTED,
new MinimumParameterRequirements(4).and(new EvenParameterRequirements())),
+ HVALS(new HValsExecutor(), SUPPORTED, new ExactParameterRequirements(2)),
/************* Sets *****************/
@@ -243,7 +244,6 @@
new OddParameterRequirements(ERROR_SYNTAX)),
HSETNX(new HSetNXExecutor(), UNSUPPORTED, new ExactParameterRequirements(4)),
HSTRLEN(new HStrLenExecutor(), UNSUPPORTED, new ExactParameterRequirements(3)),
- HVALS(new HValsExecutor(), UNSUPPORTED, new ExactParameterRequirements(2)),
/***************************************
**************** Sets *****************
diff --git a/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java b/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java
index 4474201..e0b05be 100644
--- a/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java
+++ b/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java
@@ -40,6 +40,7 @@
"HGETALL",
"HMSET",
"HSET",
+ "HVALS",
"KEYS",
"PERSIST",
"PEXPIRE",
@@ -85,7 +86,6 @@
"HSCAN",
"HSETNX",
"HSTRLEN",
- "HVALS",
"INCR",
"INCRBY",
"INCRBYFLOAT",
