From 3ddd9dae75b1cc5b0f3ed45148be225542eb4f57 Mon Sep 17 00:00:00 2001 From: Ray Ingles Date: Tue, 26 Jan 2021 09:11:59 -0500 Subject: [PATCH] Geode-8853: DUNIT tests for HSTRLEN command (#5939) - test the error messages for hgetall and hstrlen more specifically - fix an hgetall test that was testing the wrong command Co-authored-by: Ray Ingles Co-authored-by: Helena A. Bales --- .../redis_api_for_geode.html.md.erb | 5 +- geode-redis/README.md | 88 ++++++++--------- .../executor/hash/HstrlenDUnitTest.java | 96 +++++++++++++++++++ .../hash/AbstractHashesIntegrationTest.java | 22 +++-- 4 files changed, 158 insertions(+), 53 deletions(-) create mode 100644 geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HstrlenDUnitTest.java 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 c4327a4e5071..16a034f1ae72 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 @@ The Redis API for <%=vars.product_name%> currently supports the following comman **Note**: These commands are supported for Redis 5. - **Connection**: AUTH, PING, QUIT -- **Hashes**: HGETALL, HMGET, HMSET, HSET, HVALS +- **Hashes**: HGETALL, HMGET, HMSET, HSET, HSTRLEN, HVALS - **Keys**: DEL, EXISTS, EXPIRE, EXPIREAT, KEYS, PERSIST, PEXPIRE, PEXPIREAT, PTTL, RENAME, TTL, TYPE - **Publish/Subscribe**: PUBLISH, PSUBSCRIBE, PUNSUBSCRIBE, SUBSCRIBE, UNSUBSCRIBE @@ -80,8 +80,7 @@ commands are available to use, but have not been fully tested. There is no guara exactly as expected. - **Connection**: ECHO, SELECT -- **Hashes**: HDEL, HEXISTS, HGET, HINCRBY, HINCRBYFLOAT, HKEYS, HLEN, HSCAN, HSETNX, - HSTRLEN +- **Hashes**: HDEL, HEXISTS, HGET, HINCRBY, HINCRBYFLOAT, HKEYS, HLEN, HSCAN, HSETNX - **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 f690001b16af..b8fd770181e4 100644 --- a/geode-redis/README.md +++ b/geode-redis/README.md @@ -165,51 +165,51 @@ start server \ | EXPIREAT | DECRBY | ACL LIST | | GET | ECHO | ACL LOAD | | HGETALL | FLUSHALL | ACL LOG | -| HMGET | FLUSHDB | ACL SAVE | +| 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 | +| HSET | GETRANGE | ACL USERS | +| HSTRLEN | GETSET | ACL WHOAMI | +| HVALS | HDEL | BGREWRITEAOF | +| KEYS | HEXISTS | BGSAVE | +| PERSIST | HGET | BITFIELD | +| PEXPIRE | HINCRBY | BLPOP | +| PEXPIREAT | HINCRBYFLOAT | BRPOP | +| PING | HKEYS | BRPOPLPUSH | +| PSUBSCRIBE | HLEN | BZPOPMAX | +| PTTL | HSCAN | BZPOPMIN | +| PUBLISH | HSETNX | CLIENT CACHING | +| PUNSUBSCRIBE | INCR | CLIENT GETNAME | +| QUIT | INCRBY | CLIENT GETREDIR | +| RENAME | INCRBYFLOAT | CLIENT ID | +| SADD | INFO | CLIENT KILL | +| SET | MGET | CLIENT LIST | +| SMEMBERS | MSET | CLIENT PAUSE | +| SREM | MSETNX | CLIENT REPLY | +| SUBSCRIBE | PSETEX | CLIENT SETNAME | +| TTL | SCAN | CLIENT TRACKING | +| TYPE | SCARD | CLIENT UNBLOCK | +| UNSUBSCRIBE | SDIFF | CLUSTER ADDSLOTS | +| | SDIFFSTORE | CLUSTER BUMPEPOCH | +| | SELECT | CLUSTER COUNT-FAILURE-REPORTS | +| | SETBIT | CLUSTER COUNTKEYSINSLOT | +| | SETEX | CLUSTER DELSLOTS | +| | SETNX | CLUSTER FAILOVER | +| | SETRANGE | CLUSTER FLUSHSLOTS | +| | SHUTDOWN | CLUSTER FORGET | +| | SINTER | CLUSTER GETKEYSINSLOT | +| | SINTERSTORE | CLUSTER INFO | +| | SISMEMBER | CLUSTER KEYSLOT | +| | SLOWLOG | CLUSTER MEET | +| | SMOVE | CLUSTER MYID | +| | SPOP | CLUSTER NODES | +| | SRANDMEMBER | CLUSTER REPLICAS | +| | SSCAN | CLUSTER REPLICATE | +| | STRLEN | CLUSTER RESET | +| | SUNION | CLUSTER SAVECONFIG | +| | SUNIONSTORE | CLUSTER SET-CONFIG-EPOCH | +| | TIME | CLUSTER SETSLOT | +| | UNLINK [1] | CLUSTER SLAVES | +| | | CLUSTER SLOTS | | | | COMMAND | | | | COMMAND COUNT | | | | COMMAND GETKEYS | diff --git a/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HstrlenDUnitTest.java b/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HstrlenDUnitTest.java new file mode 100644 index 000000000000..9fecd736b847 --- /dev/null +++ b/geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HstrlenDUnitTest.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.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 HstrlenDUnitTest { + + @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 hstrlenDoesNotCorruptData_whileHashIsConcurrentlyUpdated() { + String key = "key"; + String field = "field"; + int iterations = 10000; + Random rand = new Random(); + + jedis1.hset(key, field, "22"); + + new ConcurrentLoopingThreads(iterations, + (i) -> { + int newLength = rand.nextInt(9) + 1; + String newVal = makeStringOfRepeatedDigits(newLength); + jedis1.hset(key, field, newVal); + }, + (i) -> assertThat(jedis2.hstrlen(key, field)).isBetween(1L, 9L), + (i) -> assertThat(jedis3.hstrlen(key, field)).isBetween(1L, 9L)) + .run(); + + String value = jedis1.hget(key, field); + String encodedStringLength = Character.toString(value.charAt(0)); + int expectedLength = Integer.parseInt(encodedStringLength); + assertThat(jedis2.hstrlen(key, field)).isEqualTo(expectedLength); + } + + private String makeStringOfRepeatedDigits(int newLength) { + String stringOfRepeatedDigits = ""; + for (int i = 0; i < newLength; i++) { + stringOfRepeatedDigits += newLength; + } + return stringOfRepeatedDigits; + } +} 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 f0c3f2cf6790..7c0de651fc9c 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 @@ -97,8 +97,8 @@ public void testHSet_givenWrongNumberOfArguments() { public void testHGetall_givenWrongNumberOfArguments() { assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HGETALL)) .hasMessage("ERR wrong number of arguments for 'hgetall' command"); - assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HMSET, "1", "2")) - .hasMessage("ERR wrong number of arguments for 'hmset' command"); + assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HGETALL, "1", "2")) + .hasMessage("ERR wrong number of arguments for 'hgetall' command"); } @Test @@ -249,6 +249,16 @@ public void testHStrLen_failsForNonHashes() { .hasMessage("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE); } + @Test + public void testHStrlen_givenWrongNumberOfArguments() { + assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSTRLEN)) + .hasMessageContaining("ERR wrong number of arguments for 'hstrlen' command"); + assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSTRLEN, "1")) + .hasMessageContaining("ERR wrong number of arguments for 'hstrlen' command"); + assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSTRLEN, "1", "2", "3")) + .hasMessageContaining("ERR wrong number of arguments for 'hstrlen' command"); + } + @Test public void testHkeys() { String key = "key"; @@ -440,13 +450,13 @@ public void hsetNX_shouldThrowErrorIfKeyIsWrongType() { @Test public void hsetnx_shouldThrowError_givenWrongNumberOfArguments() { assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSETNX)) - .hasMessageContaining("wrong number of arguments"); + .hasMessageContaining("ERR wrong number of arguments for 'hsetnx' command"); assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSETNX, "1")) - .hasMessageContaining("wrong number of arguments"); + .hasMessageContaining("ERR wrong number of arguments for 'hsetnx' command"); assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSETNX, "1", "2")) - .hasMessageContaining("wrong number of arguments"); + .hasMessageContaining("ERR wrong number of arguments for 'hsetnx' command"); assertThatThrownBy(() -> jedis.sendCommand(Protocol.Command.HSETNX, "1", "2", "3", "4")) - .hasMessageContaining("wrong number of arguments"); + .hasMessageContaining("ERR wrong number of arguments for 'hsetnx' command"); } /**