You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by je...@apache.org on 2020/04/10 13:42:06 UTC
[geode] branch develop updated: GEODE-7949: Geode Redis - Get/Set
commands for RedisString datatype to cover new parameters (#4907)
This is an automated email from the ASF dual-hosted git repository.
jensdeppe pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 2ffc874 GEODE-7949: Geode Redis - Get/Set commands for RedisString datatype to cover new parameters (#4907)
2ffc874 is described below
commit 2ffc8749ce727601bc5fa0f73f362fd5bd485c4a
Author: Venkateswara Prasath Durairaj <xt...@users.noreply.github.com>
AuthorDate: Fri Apr 10 09:41:38 2020 -0400
GEODE-7949: Geode Redis - Get/Set commands for RedisString datatype to cover new parameters (#4907)
Co-authored-by: John Hutchison <jh...@pivotal.io>
Co-authored-by: Sarah <sa...@pivotal.io>
---
.../apache/geode/redis/HashesIntegrationTest.java | 10 +
.../apache/geode/redis/StringsIntegrationTest.java | 343 ++++++++++++++++++++-
.../apache/geode/redis/internal/KeyRegistrar.java | 13 +-
.../geode/redis/internal/RedisConstants.java | 2 +
.../internal/executor/string/GetExecutor.java | 2 +-
.../internal/executor/string/SetExecutor.java | 241 ++++++++++-----
.../string/StringGetExecutorJUnitTest.java | 131 ++++++++
.../string/StringSetExecutorJUnitTest.java | 190 +++++++++++-
8 files changed, 833 insertions(+), 99 deletions(-)
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
index 47e42b2..4d35d48 100755
--- a/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
@@ -18,6 +18,7 @@ import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assertions.offset;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
@@ -53,6 +54,7 @@ import org.junit.Test;
import org.junit.experimental.categories.Category;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.ScanResult;
+import redis.clients.jedis.exceptions.JedisDataException;
import org.apache.geode.cache.CacheFactory;
import org.apache.geode.cache.GemFireCache;
@@ -668,6 +670,14 @@ public class HashesIntegrationTest {
}
}
+ @Test
+ public void testHSet_keyExistsWithDifferentDataType() {
+ jedis.set("key", "value");
+
+ assertThatThrownBy(
+ () -> jedis.hset("key", "field", "something else")).isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE");
+ }
@Test
public void testConcurrentHSetHDel_sameKeyPerClient() throws InterruptedException {
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/StringsIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/StringsIntegrationTest.java
index 94413b3..9f38251 100755
--- a/geode-redis/src/integrationTest/java/org/apache/geode/redis/StringsIntegrationTest.java
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/StringsIntegrationTest.java
@@ -22,7 +22,6 @@ import static org.apache.geode.redis.GeodeRedisServer.REDIS_META_DATA_REGION;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import java.io.IOException;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
@@ -39,10 +38,12 @@ import java.util.concurrent.TimeoutException;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.BeforeClass;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.exceptions.JedisDataException;
+import redis.clients.jedis.params.SetParams;
import org.apache.geode.cache.CacheFactory;
import org.apache.geode.cache.GemFireCache;
@@ -63,7 +64,7 @@ public class StringsIntegrationTest {
private static int ITERATION_COUNT = 4000;
@BeforeClass
- public static void setUp() throws IOException {
+ public static void setUp() {
rand = new Random();
CacheFactory cf = new CacheFactory();
cf.set(LOG_LEVEL, "error");
@@ -93,12 +94,339 @@ public class StringsIntegrationTest {
}
@Test
+ public void testSET_shouldSetStringValueToKey_givenEmptyKey() {
+
+ String key = "key";
+ String value = "value";
+
+ String result = jedis.get(key);
+ assertThat(result).isNull();
+
+ jedis.set(key, value);
+ result = jedis.get(key);
+ assertThat(result).isEqualTo(value);
+ }
+
+ @Test
+ public void testSET_shouldSetStringValueToKey_givenKeyIsOfDataTypeSet() {
+ String key = "key";
+ String stringValue = "value";
+
+ jedis.sadd(key, "member1", "member2");
+
+ jedis.set(key, stringValue);
+ String result = jedis.get(key);
+
+ assertThat(result).isEqualTo(stringValue);
+ }
+
+ @Test
+ public void testSET_shouldSetStringValueToKey_givenKeyIsOfDataTypeHash() {
+ String key = "key";
+ String stringValue = "value";
+
+ jedis.hset(key, "field", "something else");
+
+ String result = jedis.set(key, stringValue);
+ assertThat(result).isEqualTo("OK");
+
+ assertThat(stringValue).isEqualTo(jedis.get(key));
+ }
+
+ @Test
+ public void testSET_shouldSetNX_evenIfKeyContainsOtherDataType() {
+ String key = "key";
+ String stringValue = "value";
+
+ jedis.sadd(key, "member1", "member2");
+ SetParams setParams = new SetParams();
+ setParams.nx();
+
+ String result = jedis.set(key, stringValue, setParams);
+ assertThat(result).isNull();
+ }
+
+ @Test
+ public void testSET_shouldSetXX_evenIfKeyContainsOtherDataType() {
+ String key = "key";
+ String stringValue = "value";
+
+ jedis.sadd(key, "member1", "member2");
+ SetParams setParams = new SetParams();
+ setParams.xx();
+
+ jedis.set(key, stringValue, setParams);
+ String result = jedis.get(key);
+
+ assertThat(result).isEqualTo(stringValue);
+ }
+
+ @Test
+ public void testSET_withEXargument_shouldSetExpireTime() {
+ String key = "key";
+ String value = "value";
+ int secondsUntilExpiration = 20;
+
+ SetParams setParams = new SetParams();
+ setParams.ex(secondsUntilExpiration);
+
+ jedis.set(key, value, setParams);
+
+ Long result = jedis.ttl(key);
+
+ assertThat(result).isGreaterThan(15l);
+ }
+
+ @Test
+ public void testSET_withNegative_EX_time_shouldReturnError() {
+ String key = "key";
+ String value = "value";
+ int millisecondsUntilExpiration = -1;
+
+ SetParams setParams = new SetParams();
+ setParams.ex(millisecondsUntilExpiration);
+
+ assertThatThrownBy(() -> jedis.set(key, value, setParams))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining(RedisConstants.ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ @Test
+ public void testSET_withPXargument_shouldSetExpireTime() {
+ String key = "key";
+ String value = "value";
+ int millisecondsUntilExpiration = 20000;
+
+ SetParams setParams = new SetParams();
+ setParams.px(millisecondsUntilExpiration);
+
+ jedis.set(key, value, setParams);
+
+ Long result = jedis.ttl(key);
+
+ assertThat(result).isGreaterThan(15l);
+ }
+
+ @Test
+ public void testSET_with_Negative_PX_time_shouldReturnError() {
+ String key = "key";
+ String value = "value";
+ int millisecondsUntilExpiration = -1;
+
+ SetParams setParams = new SetParams();
+ setParams.px(millisecondsUntilExpiration);
+
+ assertThatThrownBy(() -> jedis.set(key, value, setParams))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining(RedisConstants.ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ @Test
+ public void testSET_shouldClearPreviousTTL_onSuccess() {
+ String key = "key";
+ String value = "value";
+ int secondsUntilExpiration = 20;
+
+ SetParams setParams = new SetParams();
+ setParams.ex(secondsUntilExpiration);
+
+ jedis.set(key, value, setParams);
+
+ jedis.set(key, "other value");
+
+ Long result = jedis.ttl(key);
+
+ assertThat(result).isEqualTo(-1L);
+ }
+
+ @Test
+ public void testSET_withXXArgument_shouldClearPreviousTTL_Success() {
+ String key = "xx_key";
+ String value = "did exist";
+ int secondsUntilExpiration = 20;
+ SetParams setParamsXX = new SetParams();
+ setParamsXX.xx();
+ SetParams setParamsEX = new SetParams();
+ setParamsEX.ex(secondsUntilExpiration);
+ String result_EX = jedis.set(key, value, setParamsEX);
+ assertThat(result_EX).isEqualTo("OK");
+ assertThat(jedis.ttl(key)).isGreaterThan(15L);
+
+ String result_XX = jedis.set(key, value, setParamsXX);
+
+ assertThat(result_XX).isEqualTo("OK");
+ Long result = jedis.ttl(key);
+ assertThat(result).isEqualTo(-1L);
+ }
+
+ @Test
+ public void testSET_should_not_clearPreviousTTL_onFailure() {
+ String key_NX = "nx_key";
+ String value_NX = "set only if key did not exist";
+ int secondsUntilExpiration = 20;
+
+ SetParams setParamsEX = new SetParams();
+ setParamsEX.ex(secondsUntilExpiration);
+
+ SetParams setParamsNX = new SetParams();
+ setParamsNX.nx();
+
+ jedis.set(key_NX, value_NX, setParamsEX);
+ String result_NX = jedis.set(key_NX, value_NX, setParamsNX);
+ assertThat(result_NX).isNull();
+
+ Long result = jedis.ttl(key_NX);
+ assertThat(result).isGreaterThan(15L);
+ }
+
+ @Test
+ @Ignore
+ public void testSET_with_KEEPTTL_shouldRetainPreviousTTL_OnSuccess() {
+ String key = "key";
+ String value = "value";
+ int secondsToExpire = 30;
+
+ SetParams setParamsEx = new SetParams();
+ setParamsEx.ex(secondsToExpire);
+
+ jedis.set(key, value, setParamsEx);
+
+ SetParams setParamsKeepTTL = new SetParams();
+ // setParamsKeepTTL.keepTtl();
+ // Jedis Doesn't support KEEPTTL yet.
+
+ jedis.set(key, "newValue", setParamsKeepTTL);
+
+ Long result = jedis.ttl(key);
+ assertThat(result).isGreaterThan(15L);
+ }
+
+ @Test
+ public void testSET_withNXargument_shouldOnlySetKeyIfKeyDoesNotExist() {
+ String key1 = "key_1";
+ String key2 = "key_2";
+ String value1 = "value_1";
+ String value2 = "value_2";
+
+ jedis.set(key1, value1);
+
+ SetParams setParams = new SetParams();
+ setParams.nx();
+
+ jedis.set(key1, value2, setParams);
+ String result1 = jedis.get(key1);
+
+ assertThat(result1).isEqualTo(value1);
+
+ jedis.set(key2, value2, setParams);
+ String result2 = jedis.get(key2);
+
+ assertThat(result2).isEqualTo(value2);
+ }
+
+ @Test
+ public void testSET_withXXargument_shouldOnlySetKeyIfKeyExists() {
+ String key1 = "key_1";
+ String key2 = "key_2";
+ String value1 = "value_1";
+ String value2 = "value_2";
+
+ jedis.set(key1, value1);
+
+ SetParams setParams = new SetParams();
+ setParams.xx();
+
+ jedis.set(key1, value2, setParams);
+ String result1 = jedis.get(key1);
+
+ assertThat(result1).isEqualTo(value2);
+
+ jedis.set(key2, value2, setParams);
+ String result2 = jedis.get(key2);
+
+ assertThat(result2).isNull();
+ }
+
+ @Test
+ public void testSET_XX_NX_arguments_should_return_OK_if_Successful() {
+ String key_NX = "nx_key";
+ String key_XX = "xx_key";
+ String value_NX = "did not exist";
+ String value_XX = "did exist";
+
+ SetParams setParamsXX = new SetParams();
+ setParamsXX.xx();
+
+ SetParams setParamsNX = new SetParams();
+ setParamsNX.nx();
+
+ String result_NX = jedis.set(key_NX, value_NX, setParamsNX);
+ assertThat(result_NX).isEqualTo("OK");
+
+ jedis.set(key_XX, value_XX);
+ String result_XX = jedis.set(key_NX, value_NX, setParamsXX);
+ assertThat(result_XX).isEqualTo("OK");
+ }
+
+ @Test
+ public void testSET_XX_NX_arguments_should_return_NULL_if_Not_Successful() {
+ String key_NX = "nx_key";
+ String key_XX = "xx_key";
+ String value_NX = "set only if key did not exist";
+ String value_XX = "set only if key did exist";
+
+ SetParams setParamsXX = new SetParams();
+ setParamsXX.xx();
+
+ SetParams setParamsNX = new SetParams();
+ setParamsNX.nx();
+
+ jedis.set(key_NX, value_NX);
+ String result_NX = jedis.set(key_NX, value_NX, setParamsNX);
+ assertThat(result_NX).isNull();
+
+ String result_XX = jedis.set(key_XX, value_XX, setParamsXX);
+ assertThat(result_XX).isNull();
+ }
+
+ @Test
+ public void testGET_shouldReturnValueOfKey_givenValueIsAString() {
+ String key = "key";
+ String value = "value";
+
+ String result = jedis.get(key);
+ assertThat(result).isNull();
+
+ jedis.set(key, value);
+ result = jedis.get(key);
+ assertThat(result).isEqualTo(value);
+ }
+
+ @Test
+ public void testGET_shouldReturnNil_givenKeyIsEmpty() {
+ String key = "this key does not exist";
+
+ String result = jedis.get(key);
+ assertThat(result).isNull();
+ }
+
+ @Test(expected = JedisDataException.class)
+ public void testGET_shouldThrow_JedisDataExceptiondError_givenValueIs_Not_A_String() {
+ String key = "key";
+ String field = "field";
+ String member = "member";
+
+ jedis.sadd(key, field, member);
+
+ jedis.get(key);
+ }
+
+ @Test
public void testAppend_shouldAppendValueWithInputStringAndReturnResultingLength() {
String key = "key";
String value = randString();
int originalValueLength = value.length();
- // @todo: for jedis this is bool, redis docs state int value...
boolean result = jedis.exists(key);
assertThat(result).isFalse();
@@ -269,15 +597,6 @@ public class StringsIntegrationTest {
}
@Test
- public void testSet_keyExistsWithDifferentDataType_returnsRedisDataTypeMismatchException() {
- jedis.hset("key", "field", "value");
-
- assertThatThrownBy(
- () -> jedis.set("key", "something else")).isInstanceOf(JedisDataException.class)
- .hasMessageContaining("WRONGTYPE");
- }
-
- @Test
public void testSet_protectedRedisDataType_throwsRedisDataTypeMismatchException() {
assertThatThrownBy(
() -> jedis.set(REDIS_META_DATA_REGION, "something else"))
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/KeyRegistrar.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/KeyRegistrar.java
index 412240f..76df13e 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/KeyRegistrar.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/KeyRegistrar.java
@@ -76,8 +76,17 @@ public class KeyRegistrar {
}
}
+ /**
+ * Checks if the given key is a protected string in GeodeRedis
+ *
+ * @param key Key to check
+ */
+ public boolean isProtected(ByteArrayWrapper key) {
+ return RedisDataType.REDIS_PROTECTED.equals(redisMetaRegion.get(key.toString()));
+ }
+
private boolean isValidDataType(RedisDataType actualDataType, RedisDataType expectedDataType) {
- return isKeyUnused(actualDataType) || actualDataType == expectedDataType;
+ return isKeyUnused(actualDataType) || actualDataType.equals(expectedDataType);
}
private boolean isKeyUnused(RedisDataType dataType) {
@@ -85,7 +94,7 @@ public class KeyRegistrar {
}
private void throwDataTypeException(ByteArrayWrapper key, RedisDataType dataType) {
- if (dataType == RedisDataType.REDIS_PROTECTED) {
+ if (RedisDataType.REDIS_PROTECTED.equals(dataType)) {
throw new RedisDataTypeMismatchException("The key name \"" + key + "\" is protected");
} else {
throw new RedisDataTypeMismatchException(
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
index 2d38a8f..0f64cb5 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
@@ -68,6 +68,8 @@ public class RedisConstants {
public static final String ERROR_NOT_INTEGER = "value is not an integer or out of range";
public static final String ERROR_OVERFLOW = "increment or decrement would overflow";
public static final String ERROR_NO_SUCH_KEY = "no such key";
+ public static final String ERROR_SYNTAX = "syntax error";
+ public static final String ERROR_INVALID_EXPIRE_TIME = "invalid expire time in set";
public static class ArityDef {
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/GetExecutor.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/GetExecutor.java
index c6c3bdd..0805daf 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/GetExecutor.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/GetExecutor.java
@@ -27,7 +27,7 @@ public class GetExecutor extends StringExecutor {
@Override
public void executeCommand(Command command, ExecutionHandlerContext context) {
- if (command.getProcessedCommand().size() < 2) {
+ if (command.getProcessedCommand().size() != 2) {
command
.setResponse(Coder.getErrorResponse(context.getByteBufAllocator(), ArityDef.GETEXECUTOR));
return;
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
index 7439a26..ac4a19a 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
@@ -14,6 +14,10 @@
*/
package org.apache.geode.redis.internal.executor.string;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INVALID_EXPIRE_TIME;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+
import java.util.List;
import org.apache.geode.cache.Region;
@@ -23,6 +27,7 @@ import org.apache.geode.redis.internal.Command;
import org.apache.geode.redis.internal.ExecutionHandlerContext;
import org.apache.geode.redis.internal.RedisConstants.ArityDef;
import org.apache.geode.redis.internal.RedisDataType;
+import org.apache.geode.redis.internal.RedisDataTypeMismatchException;
import org.apache.geode.redis.internal.executor.AbstractExecutor;
public class SetExecutor extends StringExecutor {
@@ -31,132 +36,214 @@ public class SetExecutor extends StringExecutor {
private final int VALUE_INDEX = 2;
+ private boolean NX = false; // Set only if not exists, incompatible with XX
+ private boolean XX = false; // Set only if exists, incompatible with NX
+ private boolean KEEPTTL = false; // Keep existing TTL on key
+ private long expiration = 0L;
+
@Override
public void executeCommand(Command command, ExecutionHandlerContext context) {
List<byte[]> commandElems = command.getProcessedCommand();
- Region<ByteArrayWrapper, ByteArrayWrapper> r = context.getRegionProvider().getStringsRegion();
-
if (commandElems.size() < 3) {
command.setResponse(Coder.getErrorResponse(context.getByteBufAllocator(), ArityDef.SET));
return;
}
ByteArrayWrapper key = command.getKey();
- checkDataType(key, RedisDataType.REDIS_STRING, context);
- byte[] value = commandElems.get(VALUE_INDEX);
- ByteArrayWrapper valueWrapper = new ByteArrayWrapper(value);
+ ByteArrayWrapper value = getValue(commandElems);
- boolean NX = false; // Set only if not exists
- boolean XX = false; // Set only if exists
- long expiration = 0L;
+ if (context.getKeyRegistrar().isProtected(key)) {
+ throw new RedisDataTypeMismatchException("The key name \"" + key + "\" is protected");
+ }
- if (commandElems.size() >= 6) {
- String elem4;
- String elem5;
- String elem6;
+ String parseError = parseCommandElems(commandElems);
+ if (parseError != null) {
+ command.setResponse(Coder.getErrorResponse(context.getByteBufAllocator(), parseError));
+ return;
+ }
- elem4 = Coder.bytesToString(commandElems.get(3));
- elem5 = Coder.bytesToString(commandElems.get(4));
- elem6 = Coder.bytesToString(commandElems.get(5));
+ Region<ByteArrayWrapper, ByteArrayWrapper> region =
+ context.getRegionProvider().getStringsRegion();
- if (elem4.equalsIgnoreCase("XX") || elem6.equalsIgnoreCase("XX")) {
- XX = true;
- } else if (elem4.equalsIgnoreCase("NX") || elem6.equalsIgnoreCase("NX")) {
- NX = true;
- }
+ if (NX) {
+ setNX(region, command, key, value, context);
+ return;
+ }
- if (elem4.equalsIgnoreCase("PX")) {
- expiration = getExpirationMillis(elem4, elem5);
- } else if (elem5.equalsIgnoreCase("PX")) {
- expiration = getExpirationMillis(elem5, elem6);
- } else if (elem4.equalsIgnoreCase("EX")) {
- expiration = getExpirationMillis(elem4, elem5);
- } else if (elem5.equalsIgnoreCase("EX")) {
- expiration = getExpirationMillis(elem5, elem6);
- }
+ if (XX) {
+ setXX(region, command, key, value, context);
+ return;
+ }
- } else if (commandElems.size() >= 5) {
- String elem4;
- String expiry;
+ set(command, context, region, key, value);
+ }
+
+ private ByteArrayWrapper getValue(List<byte[]> commandElems) {
+ byte[] value = commandElems.get(VALUE_INDEX);
+ return new ByteArrayWrapper(value);
+ }
- elem4 = Coder.bytesToString(commandElems.get(3));
- expiry = Coder.bytesToString(commandElems.get(4));
+ private Region getRegion(ExecutionHandlerContext context, ByteArrayWrapper key) {
+ RedisDataType redisDataType = context.getKeyRegistrar().getType(key);
+ return context.getRegionProvider().getRegionForType(redisDataType);
+ }
- expiration = getExpirationMillis(elem4, expiry);
- } else if (commandElems.size() >= 4) {
- byte[] elem4 = commandElems.get(3);
- if (elem4.length == 2 && Character.toUpperCase(elem4[1]) == 'X') {
- if (Character.toUpperCase(elem4[0]) == 'N') {
+ private String parseCommandElems(List<byte[]> commandElems) {
+ // Set only if exists, incompatible with EX
+ boolean PX = false;
+ // Set only if not exists, incompatible with PX
+ boolean EX = false;
+
+ NX = XX = EX = PX = KEEPTTL = false;
+
+ expiration = 0L;
+ for (int i = 3; i < commandElems.size(); i++) {
+ String current_arg = Coder.bytesToString(commandElems.get(i)).toUpperCase();
+ switch (current_arg) {
+ case "KEEPTTL":
+ KEEPTTL = true;
+ break;
+ case "EX":
+ EX = true;
+ i++;
+ expiration = parseExpirationTime(current_arg, i, commandElems);
+ break;
+ case "PX":
+ PX = true;
+ i++;
+ expiration = parseExpirationTime(current_arg, i, commandElems);
+ break;
+ case "NX":
NX = true;
- } else if (Character.toUpperCase(elem4[0]) == 'X') {
+ break;
+ case "XX":
XX = true;
- }
+ break;
+ default:
+ return ERROR_SYNTAX;
}
}
- boolean keyWasSet = false;
+ if (EX && PX) {
+ return ERROR_SYNTAX;
+ }
- if (NX) {
- keyWasSet = setNX(r, command, key, valueWrapper, context);
- } else if (XX) {
- keyWasSet = setXX(r, command, key, valueWrapper, context);
- } else {
- checkAndSetDataType(key, context);
- r.put(key, valueWrapper);
- command.setResponse(Coder.getSimpleStringResponse(context.getByteBufAllocator(), SUCCESS));
- keyWasSet = true;
+ if (NX && XX) {
+ return ERROR_SYNTAX;
}
- if (keyWasSet && expiration > 0L) {
- context.getRegionProvider().setExpiration(key, expiration);
+ if (EX || PX) {
+ if (expiration == -2L) {
+ return ERROR_SYNTAX;
+ }
+ if (expiration == -1L) {
+ return ERROR_NOT_INTEGER;
+ }
+ if (expiration == 0L) {
+ return ERROR_INVALID_EXPIRE_TIME;
+ }
}
+ return null;
}
- private boolean setNX(Region<ByteArrayWrapper, ByteArrayWrapper> r, Command command,
+ private long parseExpirationTime(String arg, int index, List<byte[]> commandElems) {
+ String expirationString;
+
+ try {
+ expirationString = Coder.bytesToString(commandElems.get(index));
+ } catch (IndexOutOfBoundsException e) {
+ return -2L;
+ }
+
+ long expiration = 0L;
+ try {
+ expiration = Long.parseLong(expirationString);
+ } catch (NumberFormatException e) {
+ return -1L;
+ }
+
+ if (expiration <= 0) {
+ return 0L;
+ }
+
+ if (arg.equalsIgnoreCase("EX")) {
+ return expiration * AbstractExecutor.millisInSecond;
+ } else if (arg.equalsIgnoreCase("PX")) {
+ return expiration;
+ } else {
+ return -1L;
+ }
+ }
+
+ private void setNX(Region<ByteArrayWrapper, ByteArrayWrapper> region, Command command,
ByteArrayWrapper key, ByteArrayWrapper valueWrapper,
ExecutionHandlerContext context) {
+ if (keyAlreadyExistsForDifferentDataType(context, key)) {
+ command.setResponse(Coder.getNilResponse(context.getByteBufAllocator()));
+ return;
+ }
+
checkAndSetDataType(key, context);
- Object oldValue = r.putIfAbsent(key, valueWrapper);
+ Object oldValue = region.putIfAbsent(key, valueWrapper);
+
if (oldValue != null) {
command.setResponse(Coder.getNilResponse(context.getByteBufAllocator()));
- return false;
- } else {
- command.setResponse(Coder.getSimpleStringResponse(context.getByteBufAllocator(), SUCCESS));
- return true;
+ return;
}
+
+ command.setResponse(Coder.getSimpleStringResponse(context.getByteBufAllocator(), SUCCESS));
+ handleExpiration(context, key);
}
- private boolean setXX(Region<ByteArrayWrapper, ByteArrayWrapper> r, Command command,
+ private void setXX(Region<ByteArrayWrapper, ByteArrayWrapper> region, Command command,
ByteArrayWrapper key, ByteArrayWrapper valueWrapper,
ExecutionHandlerContext context) {
- if (r.containsKey(key)) {
- checkAndSetDataType(key, context);
- r.put(key, valueWrapper);
- command.setResponse(Coder.getSimpleStringResponse(context.getByteBufAllocator(), SUCCESS));
- return true;
+ if (region.containsKey(key) || keyAlreadyExistsForDifferentDataType(context, key)) {
+ set(command, context, region, key, valueWrapper);
} else {
command.setResponse(Coder.getNilResponse(context.getByteBufAllocator()));
- return false;
}
}
- private long getExpirationMillis(String expx, String expirationString) {
- long expiration = 0L;
+ private void set(Command command, ExecutionHandlerContext context,
+ Region<ByteArrayWrapper, ByteArrayWrapper> stringsRegion, ByteArrayWrapper key,
+ ByteArrayWrapper valueWrapper) {
+ if (keyAlreadyExistsForDifferentDataType(context, key)) {
+ removeOldValueAndDataTypeAssociation(context, key);
+ }
+ checkAndSetDataType(key, context);
+ stringsRegion.put(key, valueWrapper);
+ command.setResponse(Coder.getSimpleStringResponse(context.getByteBufAllocator(), SUCCESS));
+ handleExpiration(context, key);
+ }
+
+ private boolean keyAlreadyExistsForDifferentDataType(ExecutionHandlerContext context,
+ ByteArrayWrapper key) {
try {
- expiration = Long.parseLong(expirationString);
- } catch (NumberFormatException e) {
- return 0L;
+ checkDataType(key, RedisDataType.REDIS_STRING, context);
+ } catch (RedisDataTypeMismatchException e) {
+ return true;
}
- if (expx.equalsIgnoreCase("EX")) {
- return expiration * AbstractExecutor.millisInSecond;
- } else if (expx.equalsIgnoreCase("PX")) {
- return expiration;
+ return false;
+ }
+
+ private void removeOldValueAndDataTypeAssociation(ExecutionHandlerContext context,
+ ByteArrayWrapper key) {
+ Region oldRegion = getRegion(context, key);
+ oldRegion.remove(key);
+ context.getKeyRegistrar().unregister(key);
+ }
+
+ private void handleExpiration(ExecutionHandlerContext context, ByteArrayWrapper key) {
+ if (expiration > 0L) {
+ context.getRegionProvider().setExpiration(key, expiration);
} else {
- return 0L;
+ if (!KEEPTTL) {
+ context.getRegionProvider().cancelKeyExpiration(key);
+ }
}
}
-
}
diff --git a/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringGetExecutorJUnitTest.java b/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringGetExecutorJUnitTest.java
new file mode 100644
index 0000000..1fafdc2
--- /dev/null
+++ b/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringGetExecutorJUnitTest.java
@@ -0,0 +1,131 @@
+/*
+ * 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.string;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.buffer.Unpooled;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.ByteArrayWrapper;
+import org.apache.geode.redis.internal.Command;
+import org.apache.geode.redis.internal.ExecutionHandlerContext;
+import org.apache.geode.redis.internal.KeyRegistrar;
+import org.apache.geode.redis.internal.RegionProvider;
+
+@SuppressWarnings("unchecked")
+public class StringGetExecutorJUnitTest {
+
+ private Command command;
+ private ExecutionHandlerContext context;
+ private GetExecutor executor;
+ private ByteBuf buffer;
+ private Region<ByteArrayWrapper, ByteArrayWrapper> region;
+ private RegionProvider regionProvider;
+
+ @Before
+ public void setup() {
+ command = mock(Command.class);
+ context = mock(ExecutionHandlerContext.class);
+
+ regionProvider = mock(RegionProvider.class);
+ when(context.getRegionProvider()).thenReturn(regionProvider);
+ region = mock(Region.class);
+ when(regionProvider.getStringsRegion()).thenReturn(region);
+
+ ByteBufAllocator allocator = mock(ByteBufAllocator.class);
+ buffer = Unpooled.buffer();
+ when(allocator.buffer()).thenReturn(buffer);
+ when(allocator.buffer(anyInt())).thenReturn(buffer);
+ when(context.getByteBufAllocator()).thenReturn(allocator);
+
+ KeyRegistrar keyRegistrar = mock(KeyRegistrar.class);
+ when(context.getKeyRegistrar()).thenReturn(keyRegistrar);
+
+ executor = spy(new GetExecutor());
+ }
+
+ @Test
+ public void testBasicGet() {
+ List<byte[]> args = Arrays.asList("get".getBytes(), "key".getBytes());
+ when(command.getProcessedCommand()).thenReturn(args);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ ArgumentCaptor<ByteArrayWrapper> keyCaptor = ArgumentCaptor.forClass(ByteArrayWrapper.class);
+ verify(region).get(keyCaptor.capture());
+
+ assertThat(keyCaptor.getValue()).isEqualTo("key");
+ }
+
+ @Test
+ public void testGET_withIncorrectArgumentsReturnsError() {
+ List<byte[]> commandArgumentWithTooFewArgs = Arrays.asList(
+ "GET".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithTooFewArgs);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .startsWith("-ERR The wrong number of arguments or syntax was provided");
+ }
+
+ @Test
+ public void testGET_withTooManyArgumentsReturnsError() {
+ List<byte[]> commandArgumentWithEXNoParameter = Arrays.asList(
+ "GET".getBytes(),
+ "key".getBytes(),
+ "something".getBytes(),
+ "somethingelse".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithEXNoParameter);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .contains(
+ "-ERR The wrong number of arguments or syntax was provided, the format for the GET command is \"GET key\"");
+ }
+}
diff --git a/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringSetExecutorJUnitTest.java b/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringSetExecutorJUnitTest.java
index c05cf00..1e2313b 100644
--- a/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringSetExecutorJUnitTest.java
+++ b/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringSetExecutorJUnitTest.java
@@ -42,6 +42,7 @@ import org.apache.geode.redis.internal.ByteArrayWrapper;
import org.apache.geode.redis.internal.Command;
import org.apache.geode.redis.internal.ExecutionHandlerContext;
import org.apache.geode.redis.internal.KeyRegistrar;
+import org.apache.geode.redis.internal.RedisConstants;
import org.apache.geode.redis.internal.RegionProvider;
public class StringSetExecutorJUnitTest {
@@ -84,13 +85,6 @@ public class StringSetExecutorJUnitTest {
}
@Test
- public void testTooFewOptions() {
- executor.executeCommand(command, context);
-
- assertThat(getBuffer()).startsWith("-ERR");
- }
-
- @Test
public void testBasicSet() {
List<byte[]> args = Arrays.asList("SET".getBytes(), "key".getBytes(), "value".getBytes());
when(command.getProcessedCommand()).thenReturn(args);
@@ -108,6 +102,164 @@ public class StringSetExecutorJUnitTest {
}
@Test
+ public void testSET_TooFewArgumentsReturnsError() {
+ List<byte[]> commandArgumentWithTooFewArgs = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithTooFewArgs);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .startsWith("-ERR The wrong number of arguments or syntax was provided");
+ }
+
+ @Test
+ public void testSET_EXargument_withoutParameterReturnsError() {
+ List<byte[]> commandArgumentWithEXNoParameter = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes(),
+ "value".getBytes(),
+ "EX".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithEXNoParameter);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .contains(RedisConstants.ERROR_SYNTAX);
+ }
+
+ @Test
+ public void testSET_EXargument_withNonNumericParameter_returnsError() {
+ List<byte[]> commandArgumentWithEXNoParameter = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes(),
+ "value".getBytes(),
+ "EX".getBytes(),
+ "NotANumberAtAll".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithEXNoParameter);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .contains(RedisConstants.ERROR_NOT_INTEGER);
+ }
+
+ @Test
+ public void testSET_EXargument_withZeroExpireTime_returnsError() {
+ List<byte[]> commandArgumentWithEXNoParameter = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes(),
+ "value".getBytes(),
+ "EX".getBytes(),
+ "0".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithEXNoParameter);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .contains(RedisConstants.ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ @Test
+ public void testSET_PXargument_withoutParameterReturnsError() {
+ List<byte[]> commandArgumentWithEXNoParameter = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes(),
+ "value".getBytes(),
+ "PX".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithEXNoParameter);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .contains(RedisConstants.ERROR_SYNTAX);
+ }
+
+ @Test
+ public void testSET_PXandEX_inSameCommand_ReturnsError() {
+ List<byte[]> commandArgumentWithEXNoParameter = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes(),
+ "value".getBytes(),
+ "PX".getBytes(),
+ "30000".getBytes(),
+ "EX".getBytes(),
+ "30".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithEXNoParameter);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .contains(RedisConstants.ERROR_SYNTAX);
+ }
+
+ @Test
+ public void testSET_NXandXX_inSameCommand_ReturnsError() {
+ List<byte[]> commandArgumentWithEXNoParameter = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes(),
+ "value".getBytes(),
+ "NX".getBytes(),
+ "XX".getBytes());
+
+ ArgumentCaptor<ByteBuf> argsErrorCaptor = ArgumentCaptor.forClass(ByteBuf.class);
+
+ when(command.getProcessedCommand()).thenReturn(commandArgumentWithEXNoParameter);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ verify(command).setResponse(argsErrorCaptor.capture());
+
+ List<ByteBuf> capturedErrors = argsErrorCaptor.getAllValues();
+ assertThat(capturedErrors.get(0).toString(Charset.defaultCharset()))
+ .contains(RedisConstants.ERROR_SYNTAX);
+ }
+
+ @Test
public void testSetNX_WhenKeyDoesNotExist() {
List<byte[]> args = Arrays.asList(
"SET".getBytes(),
@@ -217,4 +369,28 @@ public class StringSetExecutorJUnitTest {
assertThat(expireValue.getValue()).isEqualTo(5000);
assertThat(getBuffer()).startsWith("+OK");
}
+
+ @Test
+ public void testSet_withKEEPTTL() {
+ List<byte[]> args = Arrays.asList(
+ "SET".getBytes(),
+ "key".getBytes(),
+ "value".getBytes(),
+ "KEEPTTL".getBytes());
+ when(command.getProcessedCommand()).thenReturn(args);
+ when(command.getKey()).thenReturn(new ByteArrayWrapper("key".getBytes()));
+
+ executor.executeCommand(command, context);
+
+ ArgumentCaptor<ByteArrayWrapper> keyCaptor = ArgumentCaptor.forClass(ByteArrayWrapper.class);
+ ArgumentCaptor<ByteArrayWrapper> valueCaptor = ArgumentCaptor.forClass(ByteArrayWrapper.class);
+ verify(region).put(keyCaptor.capture(), valueCaptor.capture());
+
+ assertThat(keyCaptor.getValue()).isEqualTo("key");
+ assertThat(valueCaptor.getValue()).isEqualTo("value");
+ assertThat(getBuffer()).startsWith("+OK");
+
+ ByteArrayWrapper keyWrapper = new ByteArrayWrapper("key".getBytes());
+ verify(regionProvider, never()).cancelKeyExpiration(keyWrapper);
+ }
}