You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/06/04 23:06:16 UTC
[GitHub] [geode] jhutchison opened a new pull request #5216: Refactor set executor
jhutchison opened a new pull request #5216:
URL: https://github.com/apache/geode/pull/5216
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r445205109
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/StringsIntegrationTest.java
##########
@@ -0,0 +1,1637 @@
+/*
Review comment:
whoops. thanks- moved Jen's earlier changes into the SetsIntegrationTest and deleted StringsIntegrationTest
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r436056100
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
Review comment:
This could be simplified to:
```
keepTTL = optionalParametersStrings.contains("KEEPTL");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r440443210
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/RenameExecutor.java
##########
@@ -16,34 +16,35 @@
package org.apache.geode.redis.internal.executor.key;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
-
import java.util.List;
+import org.apache.geode.redis.internal.RedisConstants;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.executor.AbstractExecutor;
-import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Coder;
import org.apache.geode.redis.internal.netty.Command;
import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
public class RenameExecutor extends AbstractExecutor {
-
@Override
- public RedisResponse executeCommand(Command command,
- ExecutionHandlerContext context) {
+ public void executeCommand(Command command, ExecutionHandlerContext context) {
Review comment:
looking into it- thx
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r440347391
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/RenameExecutor.java
##########
@@ -16,34 +16,35 @@
package org.apache.geode.redis.internal.executor.key;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
-
import java.util.List;
+import org.apache.geode.redis.internal.RedisConstants;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.executor.AbstractExecutor;
-import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Coder;
import org.apache.geode.redis.internal.netty.Command;
import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
public class RenameExecutor extends AbstractExecutor {
-
@Override
- public RedisResponse executeCommand(Command command,
- ExecutionHandlerContext context) {
+ public void executeCommand(Command command, ExecutionHandlerContext context) {
Review comment:
The changes here look like an incorrect merge perhaps.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r436079328
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
+
+ if (optionalParametersStrings.contains("PX")) {
+ int index = optionalParametersStrings.indexOf("PX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("PX", expirationTime);
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ int index = optionalParametersStrings.indexOf("EX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
}
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("EX", expirationTime);
+ }
+
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
}
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
- try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
throw new IllegalArgumentException(ERROR_SYNTAX);
}
+ }
+
+
+ private boolean previousOptionIsValidAndExpectsANumber(String paramter,
+ List<String> optionalParameters) {
+ List<String> validParamaters = Arrays.asList("EX", "PX");
+
+ if (optionalParameters.size() < 2) {
+ return false;
+ }
+
+ int indexOfParameter = optionalParameters.indexOf(paramter);
+ String previousParameter = optionalParameters.get(indexOfParameter - 1);
+
+ return validParamaters.contains(previousParameter);
+ }
+
+
+ private void throwExceptionIfIncompatableParamaterOptions(List<String> passedParametersStrings) {
+
+ if (passedParametersStrings.contains("PX")
+ && passedParametersStrings.contains("EX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ if (passedParametersStrings.contains("XX")
+ && passedParametersStrings.contains("NX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+
+ private long parseExpirationTime(String expirationParameter, String expirationTimeString)
+ throws IllegalArgumentException {
+
+ long expirationTimeLong =
+ convertToLongOrThrowException(expirationTimeString);
+
+ isExpirationTimeLegal(expirationTimeLong);
+
+ if (expirationParameter.equals("EX")) {
+ expirationTimeLong *= 1000;
Review comment:
makes sense- rebase oversight. done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r436187614
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
+
+ if (optionalParametersStrings.contains("PX")) {
+ int index = optionalParametersStrings.indexOf("PX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("PX", expirationTime);
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ int index = optionalParametersStrings.indexOf("EX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
}
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("EX", expirationTime);
+ }
+
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
}
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
- try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
throw new IllegalArgumentException(ERROR_SYNTAX);
}
+ }
+
+
+ private boolean previousOptionIsValidAndExpectsANumber(String paramter,
+ List<String> optionalParameters) {
+ List<String> validParamaters = Arrays.asList("EX", "PX");
+
+ if (optionalParameters.size() < 2) {
+ return false;
+ }
+
+ int indexOfParameter = optionalParameters.indexOf(paramter);
+ String previousParameter = optionalParameters.get(indexOfParameter - 1);
+
+ return validParamaters.contains(previousParameter);
+ }
+
+
+ private void throwExceptionIfIncompatableParamaterOptions(List<String> passedParametersStrings) {
+
+ if (passedParametersStrings.contains("PX")
+ && passedParametersStrings.contains("EX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ if (passedParametersStrings.contains("XX")
+ && passedParametersStrings.contains("NX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+
+ private long parseExpirationTime(String expirationParameter, String expirationTimeString)
+ throws IllegalArgumentException {
+
+ long expirationTimeLong =
+ convertToLongOrThrowException(expirationTimeString);
+
+ isExpirationTimeLegal(expirationTimeLong);
+
+ if (expirationParameter.equals("EX")) {
+ expirationTimeLong *= 1000;
+ } else if (!expirationParameter.equals("PX")) {
+ throw new IllegalArgumentException(ERROR_NOT_INTEGER);
Review comment:
pulled this logic apart- thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r445204696
##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringSetExecutorJUnitTest.java
##########
@@ -213,4 +229,34 @@ public void testSET_XXandNX_inSameCommand_ReturnsError() {
assertThat(response.toString()).contains(RedisConstants.ERROR_SYNTAX);
}
+ @Test
+ public void testSET_unknownOption_ReturnsError() {
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439070387
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+ private boolean isANumber(String parameter) {
try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ Long.parseLong(parameter);
+ return true;
+ } catch (NumberFormatException e) {
+ return false;
+ }
+ }
+
+ private boolean previousOptionIsValidAndExpectsANumber(String paramter,
+ List<String> optionalParameters) {
+
+ List<String> validParamaters = Arrays.asList("EX", "PX");
+ if (optionalParameters.size() < 2) {
+ return false;
+ }
+
+ int indexOfParameter = optionalParameters.indexOf(paramter);
+ String previousParameter = optionalParameters.get(indexOfParameter - 1);
+
+ return validParamaters.contains(previousParameter);
+ }
+
+
+ private void throwExceptionIfIncompatableParamaterOptions(List<String> passedParametersStrings) {
+
+ if (passedParametersStrings.contains("PX")
+ && passedParametersStrings.contains("EX")) {
throw new IllegalArgumentException(ERROR_SYNTAX);
}
+ if (passedParametersStrings.contains("XX")
+ && passedParametersStrings.contains("NX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+
+ private long parseExpirationTime(String expirationParameter, String timeUntilExpiration) {
+
+ long millisecondsUntilExpirationLong =
+ Long.parseLong(timeUntilExpiration);
+
+ if (expirationParameter.equals("EX")) {
Review comment:
ok- done (when commit arrives)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r445204473
##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringSetExecutorJUnitTest.java
##########
@@ -213,4 +229,34 @@ public void testSET_XXandNX_inSameCommand_ReturnsError() {
assertThat(response.toString()).contains(RedisConstants.ERROR_SYNTAX);
}
+ @Test
+ public void testSET_unknownOption_ReturnsError() {
Review comment:
done. (1 test deleted instead of moved as the same basic test already existed in SetIntegrationTest )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439065784
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
Review comment:
you're right- just checked native redis. I'll look into lack of test here, too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439053138
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
Review comment:
thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #5216: GEODE-8303: refactor Redis (String)SetExecutor
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #5216:
URL: https://github.com/apache/geode/pull/5216
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439053138
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
Review comment:
thanks- I'll look into why no test caught this...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439078174
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
Review comment:
thanks for catching that done (in forthcoming commit)
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+ private boolean isANumber(String parameter) {
try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ Long.parseLong(parameter);
Review comment:
(see below)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r445111647
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/StringsIntegrationTest.java
##########
@@ -0,0 +1,1637 @@
+/*
Review comment:
It looks like you may have actually brought StringsIntegrationTest back to life. I think it has been removed from develop and split up into multiple, command focused tests. We now have a SetIntegrationTest so amy tests for SET that you added to this class should now be added to it instead.
##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/StringSetExecutorJUnitTest.java
##########
@@ -213,4 +229,34 @@ public void testSET_XXandNX_inSameCommand_ReturnsError() {
assertThat(response.toString()).contains(RedisConstants.ERROR_SYNTAX);
}
+ @Test
+ public void testSET_unknownOption_ReturnsError() {
Review comment:
I'm not sure these tests should be added here. If you added them instead to SetIntegrationTest then I'd feel better because they would also run against native redis.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r436187466
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
+
+ if (optionalParametersStrings.contains("PX")) {
+ int index = optionalParametersStrings.indexOf("PX");
Review comment:
done - thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r438455761
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
Review comment:
change "KEEPTL" to "KEEPTTL"
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
Review comment:
Change "KEEPTL" to "KEEPTTL"
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+ private boolean isANumber(String parameter) {
try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ Long.parseLong(parameter);
Review comment:
I don't like that we now call parseLong twice. Once in isANumber and once in parseExpirationTime. Just have parseExpirationTime handle NumberFormatException and throw IllegalArgumentException(ERROR_NOT_INTEGER)
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
Review comment:
so actually it parses the long three times! If you are going to do it here then you should have it also fill a HashMap whose keys are the option that expects a number and whose value is the number. Then this could be used later instead of parsing again.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
Review comment:
The old code used ERROR_NOT_INTEGER if it was not a number. You have changed that to ERROR_SYNTAX
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+ private boolean isANumber(String parameter) {
try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ Long.parseLong(parameter);
+ return true;
+ } catch (NumberFormatException e) {
+ return false;
+ }
+ }
+
+ private boolean previousOptionIsValidAndExpectsANumber(String paramter,
+ List<String> optionalParameters) {
+
+ List<String> validParamaters = Arrays.asList("EX", "PX");
+ if (optionalParameters.size() < 2) {
+ return false;
+ }
+
+ int indexOfParameter = optionalParameters.indexOf(paramter);
+ String previousParameter = optionalParameters.get(indexOfParameter - 1);
+
+ return validParamaters.contains(previousParameter);
+ }
+
+
+ private void throwExceptionIfIncompatableParamaterOptions(List<String> passedParametersStrings) {
+
+ if (passedParametersStrings.contains("PX")
+ && passedParametersStrings.contains("EX")) {
throw new IllegalArgumentException(ERROR_SYNTAX);
}
+ if (passedParametersStrings.contains("XX")
+ && passedParametersStrings.contains("NX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+
+ private long parseExpirationTime(String expirationParameter, String timeUntilExpiration) {
+
+ long millisecondsUntilExpirationLong =
+ Long.parseLong(timeUntilExpiration);
+
+ if (expirationParameter.equals("EX")) {
Review comment:
I think it would be better to do this in the caller that does this for EX. Then you don't need to pass expirationParameter to this method. Its job is just parsing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r435621743
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
+
+ if (optionalParametersStrings.contains("PX")) {
+ int index = optionalParametersStrings.indexOf("PX");
Review comment:
consider having a method called getNextParameter("PX") which would do the indexOf, the size check and the get and return it. Then this type of code could look like:
if (optionalParametersStrings.contains("PX")) {
String expirationTime = getNextParameter(optionalParametersStrings, "PX");
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
+
+ if (optionalParametersStrings.contains("PX")) {
+ int index = optionalParametersStrings.indexOf("PX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("PX", expirationTime);
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ int index = optionalParametersStrings.indexOf("EX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
}
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("EX", expirationTime);
+ }
+
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
}
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
- try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
throw new IllegalArgumentException(ERROR_SYNTAX);
}
+ }
+
+
+ private boolean previousOptionIsValidAndExpectsANumber(String paramter,
+ List<String> optionalParameters) {
+ List<String> validParamaters = Arrays.asList("EX", "PX");
+
+ if (optionalParameters.size() < 2) {
+ return false;
+ }
+
+ int indexOfParameter = optionalParameters.indexOf(paramter);
+ String previousParameter = optionalParameters.get(indexOfParameter - 1);
+
+ return validParamaters.contains(previousParameter);
+ }
+
+
+ private void throwExceptionIfIncompatableParamaterOptions(List<String> passedParametersStrings) {
+
+ if (passedParametersStrings.contains("PX")
+ && passedParametersStrings.contains("EX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ if (passedParametersStrings.contains("XX")
+ && passedParametersStrings.contains("NX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+
+ private long parseExpirationTime(String expirationParameter, String expirationTimeString)
+ throws IllegalArgumentException {
+
+ long expirationTimeLong =
+ convertToLongOrThrowException(expirationTimeString);
+
+ isExpirationTimeLegal(expirationTimeLong);
+
+ if (expirationParameter.equals("EX")) {
+ expirationTimeLong *= 1000;
Review comment:
Please convert this back to SECONDS.toMillis(expirationTimeLong). It is a standard part of the jdk and makes clear what units we are converting from and to.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
+
+ if (optionalParametersStrings.contains("PX")) {
+ int index = optionalParametersStrings.indexOf("PX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("PX", expirationTime);
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ int index = optionalParametersStrings.indexOf("EX");
+
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
}
+
+ String expirationTime = optionalParametersStrings.get(index + 1);
+ expiration = parseExpirationTime("EX", expirationTime);
+ }
+
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
}
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
- try {
- expirationString = Coder.bytesToString(commandElems.get(index));
- } catch (IndexOutOfBoundsException e) {
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
+
+ List<String> parametersInQuestion =
+ optionalParameters
+ .stream()
+ .filter(parameter -> (!validOptionalParamaters.contains(parameter)))
+ .collect(Collectors.toList());
+
+ parametersInQuestion.forEach(parameter -> throwErrorIfNotANumberInExpectedPosition(
+ parameter,
+ optionalParameters));
+ }
+
+
+ private void throwErrorIfNotANumberInExpectedPosition(
+ String parameter,
+ List<String> optionalParameters) {
+
+ if (previousOptionIsValidAndExpectsANumber(parameter, optionalParameters)) {
+ convertToLongOrThrowException(parameter);
+ } else {
throw new IllegalArgumentException(ERROR_SYNTAX);
}
+ }
+
+
+ private boolean previousOptionIsValidAndExpectsANumber(String paramter,
+ List<String> optionalParameters) {
+ List<String> validParamaters = Arrays.asList("EX", "PX");
+
+ if (optionalParameters.size() < 2) {
+ return false;
+ }
+
+ int indexOfParameter = optionalParameters.indexOf(paramter);
+ String previousParameter = optionalParameters.get(indexOfParameter - 1);
+
+ return validParamaters.contains(previousParameter);
+ }
+
+
+ private void throwExceptionIfIncompatableParamaterOptions(List<String> passedParametersStrings) {
+
+ if (passedParametersStrings.contains("PX")
+ && passedParametersStrings.contains("EX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ if (passedParametersStrings.contains("XX")
+ && passedParametersStrings.contains("NX")) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ }
+
+ private long parseExpirationTime(String expirationParameter, String expirationTimeString)
+ throws IllegalArgumentException {
+
+ long expirationTimeLong =
+ convertToLongOrThrowException(expirationTimeString);
+
+ isExpirationTimeLegal(expirationTimeLong);
+
+ if (expirationParameter.equals("EX")) {
+ expirationTimeLong *= 1000;
+ } else if (!expirationParameter.equals("PX")) {
+ throw new IllegalArgumentException(ERROR_NOT_INTEGER);
Review comment:
This is not the correct exception here. You actually have an integer but the parameter was not one of EX or PX. I wold argue that you should not even pass expirationParameter into this method. Let the caller that knows he is EX convert the value returned from this method to milliseconds. So less responsibility for this method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r436079564
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -68,72 +75,141 @@ private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+
+ if (optionalParametersStrings.contains("KEEPTL")) {
+ keepTTL = true;
+ }
Review comment:
thanks. done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5216: Refactor set executor
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5216:
URL: https://github.com/apache/geode/pull/5216#discussion_r439055432
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/SetExecutor.java
##########
@@ -35,105 +37,182 @@
public RedisResponse executeCommandWithResponse(Command command,
ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
ByteArrayWrapper keyToSet = command.getKey();
- ByteArrayWrapper valueToSet = getValueToSet(commandElems);
-
+ List<byte[]> commandElementsBytes = command.getProcessedCommand();
+ List<byte[]> optionalParameterBytes = getOptionalParameters(commandElementsBytes);
+ ByteArrayWrapper valueToSet = getValueToSet(commandElementsBytes);
RedisStringCommands redisStringCommands = getRedisStringCommands(context);
SetOptions setOptions;
+
try {
- setOptions = parseCommandElems(commandElems);
+ setOptions = parseOptionalParameters(optionalParameterBytes);
} catch (IllegalArgumentException ex) {
return RedisResponse.error(ex.getMessage());
}
- return doSet(command, context, keyToSet, valueToSet, redisStringCommands, setOptions);
+ return doSet(keyToSet, valueToSet, redisStringCommands, setOptions);
+ }
+
+ private List<byte[]> getOptionalParameters(List<byte[]> commandElementsBytes) {
+ return commandElementsBytes.subList(3, commandElementsBytes.size());
}
- private RedisResponse doSet(Command command, ExecutionHandlerContext context,
- ByteArrayWrapper key,
- ByteArrayWrapper value, RedisStringCommands redisStringCommands, SetOptions setOptions) {
+ private RedisResponse doSet(ByteArrayWrapper key,
+ ByteArrayWrapper value,
+ RedisStringCommands redisStringCommands,
+ SetOptions setOptions) {
- boolean result = redisStringCommands.set(key, value, setOptions);
+ boolean setCompletedSuccessfully = redisStringCommands.set(key, value, setOptions);
- if (result) {
+ if (setCompletedSuccessfully) {
return RedisResponse.string(SUCCESS);
+ } else {
+ return RedisResponse.nil();
}
-
- return RedisResponse.nil();
}
private ByteArrayWrapper getValueToSet(List<byte[]> commandElems) {
byte[] value = commandElems.get(2);
return new ByteArrayWrapper(value);
}
+ private SetOptions parseOptionalParameters(List<byte[]> optionalParameterBytes)
+ throws IllegalArgumentException {
- private SetOptions parseCommandElems(List<byte[]> commandElems) throws IllegalArgumentException {
boolean keepTTL = false;
SetOptions.Exists existsOption = SetOptions.Exists.NONE;
long 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":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- expiration = SECONDS.toMillis(expiration);
- break;
- case "PX":
- if (expiration != 0) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- i++;
- expiration = parseExpirationTime(i, commandElems);
- break;
- case "NX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.NX;
- break;
- case "XX":
- if (existsOption != SetOptions.Exists.NONE) {
- throw new IllegalArgumentException(ERROR_SYNTAX);
- }
- existsOption = SetOptions.Exists.XX;
- break;
- default:
- throw new IllegalArgumentException(ERROR_SYNTAX);
+ List<String> optionalParametersStrings =
+ optionalParameterBytes.stream()
+ .map(item -> Coder.bytesToString(item).toUpperCase())
+ .collect(Collectors.toList());
+
+ throwExceptionIfUnknownParameter(optionalParametersStrings);
+ throwExceptionIfIncompatableParamaterOptions(optionalParametersStrings);
+ keepTTL = optionalParametersStrings.contains("KEEPTL");
+
+ if (optionalParametersStrings.contains("PX")) {
+ String nextParameter =
+ getNextParameter("PX", optionalParametersStrings);
+
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+
+ expiration = parseExpirationTime("PX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
+ }
+
+ } else if (optionalParametersStrings.contains("EX")) {
+ String nextParameter =
+ getNextParameter("EX", optionalParametersStrings);
+ if (!isANumber(nextParameter)) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ expiration = parseExpirationTime("EX", nextParameter);
+ if (expiration <= 0) {
+ throw new IllegalArgumentException(ERROR_INVALID_EXPIRE_TIME);
}
}
+ if (optionalParametersStrings.contains("NX")) {
+ existsOption = SetOptions.Exists.NX;
+ } else if (optionalParametersStrings.contains("XX")) {
+ existsOption = SetOptions.Exists.XX;
+ }
return new SetOptions(existsOption, expiration, keepTTL);
}
- private long parseExpirationTime(int index, List<byte[]> commandElems)
- throws IllegalArgumentException {
- String expirationString;
+ private String getNextParameter(String currentParameter,
+ List<String> optionalParametersStrings) {
+ int index = optionalParametersStrings.indexOf(currentParameter);
+ if (optionalParametersStrings.size() <= index + 1) {
+ throw new IllegalArgumentException(ERROR_SYNTAX);
+ }
+ return optionalParametersStrings.get(index + 1);
+ }
+
+ private void throwExceptionIfUnknownParameter(List<String> optionalParameters) {
+ List<String> validOptionalParamaters = Arrays.asList("EX", "PX", "NX", "XX", "KEEPTL");
Review comment:
same
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org