GEODE-8865: Create additional dunit and integration tests for Redis HMGET (#5945)
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 2887109..c4327a4 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, HVALS
+- **Hashes**: HGETALL, HMGET, HMSET, HSET, HVALS
- **Keys**: DEL, EXISTS, EXPIRE, EXPIREAT, KEYS, PERSIST, PEXPIRE, PEXPIREAT, PTTL, RENAME, TTL,
TYPE
- **Publish/Subscribe**: PUBLISH, PSUBSCRIBE, PUNSUBSCRIBE, SUBSCRIBE, UNSUBSCRIBE
@@ -80,7 +80,7 @@
exactly as expected.
- **Connection**: ECHO, SELECT
-- **Hashes**: HDEL, HEXISTS, HGET, HINCRBY, HINCRBYFLOAT, HKEYS, HLEN, HMGET, HSCAN, HSETNX,
+- **Hashes**: HDEL, HEXISTS, HGET, HINCRBY, HINCRBYFLOAT, HKEYS, HLEN, HSCAN, HSETNX,
HSTRLEN
- **Keys**: SCAN, UNLINK
- **Server**: DBSIZE, FLUSHALL (no async option), FLUSHDB (no async option), INFO, SHUTDOWN,
diff --git a/geode-redis/README.md b/geode-redis/README.md
index 72d4a36..f690001 100644
--- a/geode-redis/README.md
+++ b/geode-redis/README.md
@@ -165,52 +165,52 @@
| EXPIREAT | DECRBY | ACL LIST |
| GET | ECHO | ACL LOAD |
| HGETALL | FLUSHALL | ACL LOG |
-| HMSET | FLUSHDB | ACL SAVE |
-| HSET | GETBIT | ACL SETUSER |
-| 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 |
+| HMGET | FLUSHDB | ACL SAVE |
+| HMSET | GETBIT | ACL SETUSER |
+| HSET | GETRANGE | ACL USERS |
+| HVALS | GETSET | ACL WHOAMI |
+| KEYS | HDEL | BGREWRITEAOF |
+| PERSIST | HEXISTS | BGSAVE |
+| PEXPIRE | HGET | BITFIELD |
+| PEXPIREAT | HINCRBY | BLPOP |
+| PING | HINCRBYFLOAT | BRPOP |
+| PSUBSCRIBE | HKEYS | BRPOPLPUSH |
+| PTTL | HLEN | BZPOPMAX |
+| PUBLISH | HSCAN | BZPOPMIN |
+| PUNSUBSCRIBE | HSETNX | CLIENT CACHING |
+| QUIT | HSTRLEN | CLIENT GETNAME |
+| RENAME | INCR | CLIENT GETREDIR |
+| SADD | INCRBY | CLIENT ID |
+| SET | INCRBYFLOAT | CLIENT KILL |
+| SMEMBERS | INFO | CLIENT LIST |
+| SREM | MGET | CLIENT PAUSE |
+| SUBSCRIBE | MSET | CLIENT REPLY |
+| TTL | MSETNX | CLIENT SETNAME |
+| TYPE | PSETEX | CLIENT TRACKING |
+| UNSUBSCRIBE | SCAN | CLIENT UNBLOCK |
+| | SCARD | CLUSTER ADDSLOTS |
+| | SDIFF | CLUSTER BUMPEPOCH |
+| | SDIFFSTORE | CLUSTER COUNT-FAILURE-REPORTS |
+| | SELECT | CLUSTER COUNTKEYSINSLOT |
+| | SETBIT | CLUSTER DELSLOTS |
+| | SETEX | CLUSTER FAILOVER |
+| | SETNX | CLUSTER FLUSHSLOTS |
+| | SETRANGE | CLUSTER FORGET |
+| | SHUTDOWN | CLUSTER GETKEYSINSLOT |
+| | SINTER | CLUSTER INFO |
+| | SINTERSTORE | CLUSTER KEYSLOT |
+| | SISMEMBER | CLUSTER MEET |
+| | SLOWLOG | CLUSTER MYID |
+| | SMOVE | CLUSTER NODES |
+| | SPOP | CLUSTER REPLICAS |
+| | SRANDMEMBER | CLUSTER REPLICATE |
+| | SSCAN | CLUSTER RESET |
+| | STRLEN | CLUSTER SAVECONFIG |
+| | SUNION | CLUSTER SET-CONFIG-EPOCH |
+| | SUNIONSTORE | CLUSTER SETSLOT |
+| | TIME | CLUSTER SLAVES |
+| | UNLINK [1] | CLUSTER SLOTS |
+| | | COMMAND |
| | | COMMAND COUNT |
| | | COMMAND GETKEYS |
| | | COMMAND INFO |
diff --git a/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HMgetDUnitTest.java b/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HMgetDUnitTest.java
new file mode 100644
index 0000000..c2d7422
--- /dev/null
+++ b/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HMgetDUnitTest.java
@@ -0,0 +1,112 @@
+/*
+ * 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.apache.geode.distributed.ConfigurationProperties.MAX_WAIT_TIME_RECONNECT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+
+import org.junit.AfterClass;
+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 HMgetDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int HASH_SIZE = 1000;
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static Jedis jedis1;
+ private static Jedis jedis2;
+ private static Jedis jedis3;
+
+ private static Properties locatorProperties;
+
+ private static MemberVM locator;
+ private static MemberVM server1;
+ private static MemberVM server2;
+
+ private static int redisServerPort1;
+ private static int redisServerPort2;
+
+ @BeforeClass
+ public static void classSetup() {
+ locatorProperties = new Properties();
+ locatorProperties.setProperty(MAX_WAIT_TIME_RECONNECT, "15000");
+
+ locator = clusterStartUp.startLocatorVM(0, locatorProperties);
+ server1 = clusterStartUp.startRedisVM(1, locator.getPort());
+ server2 = clusterStartUp.startRedisVM(2, locator.getPort());
+
+ redisServerPort1 = clusterStartUp.getRedisPort(1);
+ 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();
+ }
+
+ @AfterClass
+ public static void tearDown() {
+ jedis1.disconnect();
+ jedis2.disconnect();
+
+ server1.stop();
+ server2.stop();
+ }
+
+ @Test
+ public void testConcurrentHMget_whileUpdatingValues() {
+ String key = "key";
+
+ Map<String, String> testMap = makeHashMap(HASH_SIZE, "field-", "value-");
+
+ jedis1.hset(key, testMap);
+
+ new ConcurrentLoopingThreads(HASH_SIZE,
+ (i) -> jedis1.hset(key, "field-" + i, "value-" + i),
+ (i) -> assertThat(jedis2.hmget(key, "field-" + i)).isNotNull(),
+ (i) -> assertThat(jedis3.hmget(key, "field-" + i)).isNotNull());
+ }
+
+ private Map<String, String> makeHashMap(int hashSize, String baseFieldName,
+ String baseValueName) {
+ Map<String, String> map = new HashMap<>();
+ for (int i = 0; i < hashSize; i++) {
+ map.put(baseFieldName + i, baseValueName + i);
+ }
+ return map;
+ }
+}
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 9454e1b..f0c3f2c 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
@@ -115,6 +115,22 @@
}
@Test
+ public void testHMSetErrorMessage_givenIncorrectDataType() {
+ Map<String, String> animalMap = new HashMap<>();
+ animalMap.put("chicken", "eggs");
+
+ jedis.set("farm", "chicken");
+ assertThatThrownBy(() -> jedis.hmset("farm", animalMap))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the wrong kind of value");
+
+ jedis.sadd("zoo", "elephant");
+ assertThatThrownBy(() -> jedis.hmset("zoo", animalMap))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the wrong kind of value");
+ }
+
+ @Test
public void testHSet() {
String key = "key";
Map<String, String> hash = new HashMap<String, String>();
@@ -136,27 +152,26 @@
}
@Test
- public void testHMGetHDelHGetAllHVals() {
+ public void testHMGet_HDel_HGetAll_HVals() {
String key = "key";
- Map<String, String> hash = new HashMap<String, String>();
+ Map<String, String> hash = new HashMap<>();
for (int i = 0; i < 10; i++) {
hash.put("field_" + i, "member_" + i);
}
jedis.hmset(key, hash);
+
Set<String> keys = hash.keySet();
String[] keyArray = keys.toArray(new String[keys.size()]);
List<String> retList = jedis.hmget(key, keyArray);
- for (int i = 0; i < keys.size(); i++) {
- assertThat(hash.get(keyArray[i])).isEqualTo(retList.get(i));
- }
+ assertThat(retList).containsExactlyInAnyOrderElementsOf(hash.values());
Map<String, String> retMap = jedis.hgetAll(key);
assertThat(retMap).containsExactlyInAnyOrderEntriesOf(hash);
List<String> retVals = jedis.hvals(key);
- Set<String> retSet = new HashSet<String>(retVals);
+ Set<String> retSet = new HashSet<>(retVals);
assertThat(retSet.containsAll(hash.values())).isTrue();
@@ -165,6 +180,38 @@
}
@Test
+ public void testHMGet_returnNull_forUnknownFields() {
+ String key = "key";
+ jedis.hset(key, "rooster", "crows");
+ jedis.hset(key, "duck", "quacks");
+
+ List<String> result =
+ jedis.hmget(key, "unknown-1", "rooster", "unknown-2", "duck", "unknown-3");
+ assertThat(result).containsExactly(null, "crows", null, "quacks", null);
+ }
+
+ @Test
+ public void testHMGet_givenWrongNumberOfArguments() {
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMGET))
+ .hasMessage("ERR wrong number of arguments for 'hmget' command");
+ assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMGET, "1"))
+ .hasMessage("ERR wrong number of arguments for 'hmget' command");
+ }
+
+ @Test
+ public void testHMGetErrorMessage_givenIncorrectDataType() {
+ jedis.set("farm", "chicken");
+ assertThatThrownBy(() -> jedis.hmget("farm", "chicken"))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+
+ jedis.sadd("zoo", "elephant");
+ assertThatThrownBy(() -> jedis.hmget("zoo", "chicken"))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
public void testHDelErrorMessage_givenIncorrectDataType() {
jedis.set("farm", "chicken");
assertThatThrownBy(() -> {
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 4efdb0c..97d90a1 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
@@ -156,6 +156,7 @@
/************* Hashes *****************/
HGETALL(new HGetAllExecutor(), SUPPORTED, new ExactParameterRequirements(2)),
+ HMGET(new HMGetExecutor(), SUPPORTED, new MinimumParameterRequirements(3)),
HMSET(new HMSetExecutor(), SUPPORTED,
new MinimumParameterRequirements(4).and(new EvenParameterRequirements())),
HSET(new HSetExecutor(), SUPPORTED,
@@ -238,7 +239,6 @@
HINCRBYFLOAT(new HIncrByFloatExecutor(), UNSUPPORTED, new ExactParameterRequirements(4)),
HKEYS(new HKeysExecutor(), UNSUPPORTED, new ExactParameterRequirements(2)),
HLEN(new HLenExecutor(), UNSUPPORTED, new ExactParameterRequirements(2)),
- HMGET(new HMGetExecutor(), UNSUPPORTED, new MinimumParameterRequirements(3)),
HSCAN(new HScanExecutor(), UNSUPPORTED, new MinimumParameterRequirements(3),
new OddParameterRequirements(ERROR_SYNTAX)),
HSETNX(new HSetNXExecutor(), UNSUPPORTED, new ExactParameterRequirements(4)),
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 e0b05be..52deaff 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
@@ -38,6 +38,7 @@
"EXPIREAT",
"GET",
"HGETALL",
+ "HMGET",
"HMSET",
"HSET",
"HVALS",
@@ -82,7 +83,6 @@
"HINCRBYFLOAT",
"HKEYS",
"HLEN",
- "HMGET",
"HSCAN",
"HSETNX",
"HSTRLEN",
