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/05 00:03:25 UTC
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5216: Refactor set executor
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