DonalEvans commented on a change in pull request #6831:
URL: https://github.com/apache/geode/pull/6831#discussion_r706509027
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/key/AbstractScanExecutor.java
##########
@@ -15,25 +15,158 @@
*/
package org.apache.geode.redis.internal.executor.key;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_CURSOR;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToString;
+import static
org.apache.geode.redis.internal.netty.Coder.equalsIgnoreCaseBytes;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bCOUNT;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bMATCH;
+
import java.math.BigInteger;
+import java.util.List;
import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.logging.log4j.Logger;
+import org.apache.geode.annotations.VisibleForTesting;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.redis.internal.RedisException;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisDataType;
+import org.apache.geode.redis.internal.data.RedisDataTypeMismatchException;
+import org.apache.geode.redis.internal.data.RedisKey;
import org.apache.geode.redis.internal.executor.AbstractExecutor;
import org.apache.geode.redis.internal.executor.GlobPattern;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
public abstract class AbstractScanExecutor extends AbstractExecutor {
-
- protected final BigInteger UNSIGNED_LONG_CAPACITY = new
BigInteger("18446744073709551615");
+ private static final Logger logger = LogService.getLogger();
+ public static final BigInteger UNSIGNED_LONG_CAPACITY = new
BigInteger("18446744073709551615");
protected final int DEFAULT_COUNT = 10;
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElems = command.getProcessedCommand();
+
+ int cursor;
+ try {
+ cursor = getIntCursor(bytesToString(commandElems.get(2)));
+ } catch (RedisException ex) {
+ return RedisResponse.error(ERROR_CURSOR);
+ }
+
+ RedisKey key = command.getKey();
+
+ // Because we're trying to preserve the same semantics of error
conditions, with native redis,
+ // the ordering of input validation is reflected here. To that end the
first check ends up
+ // being an existence check of the key. That causes a race since the data
value needs to be
+ // accessed again when the actual command does its work. If the relevant
bucket doesn't get
+ // locked throughout the call, the bucket may move producing inconsistent
results.
+ return context.getRegionProvider().execute(key, () -> {
+ RedisData value = context.getRegionProvider().getRedisData(key);
+ if (value.isNull()) {
+ context.getRedisStats().incKeyspaceMisses();
+ return RedisResponse.emptyScan();
+ }
+
+ if (value.getType() != getDataType()) {
+ throw new RedisDataTypeMismatchException(ERROR_WRONG_TYPE);
+ }
+
+ command.getCommandType().checkDeferredParameters(command, context);
+ int count = DEFAULT_COUNT;
+ String globPattern = null;
+
+ for (int i = 3; i < commandElems.size(); i = i + 2) {
+ byte[] commandElemBytes = commandElems.get(i);
+ if (equalsIgnoreCaseBytes(commandElemBytes, bMATCH)) {
+ commandElemBytes = commandElems.get(i + 1);
+ globPattern = bytesToString(commandElemBytes);
+
+ } else if (equalsIgnoreCaseBytes(commandElemBytes, bCOUNT)) {
+ commandElemBytes = commandElems.get(i + 1);
+ try {
+ count = narrowLongToInt(bytesToLong(commandElemBytes));
+ } catch (NumberFormatException e) {
+ return RedisResponse.error(ERROR_NOT_INTEGER);
+ }
+
+ if (count < 1) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+
+ } else {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ }
+
+ Pattern matchPattern;
+ try {
+ matchPattern = convertGlobToRegex(globPattern);
+ } catch (PatternSyntaxException e) {
+ logger.warn(
+ "Could not compile the pattern: '{}' due to the following
exception: '{}'. {} will return an empty list.",
+ globPattern, e.getMessage(), command.getCommandType().name());
+
+ return RedisResponse.emptyScan();
+ }
+
+ Pair<Integer, List<byte[]>> scanResult;
+ try {
+ scanResult = executeScan(context, key, matchPattern, count, cursor);
+ } catch (IllegalArgumentException ex) {
+ return RedisResponse.error(ERROR_NOT_INTEGER);
+ }
+
+ return RedisResponse.scan(scanResult.getLeft(), scanResult.getRight());
+ });
+ }
+
/**
* @param pattern A glob pattern.
* @return A regex pattern to recognize the given glob pattern.
*/
- protected Pattern convertGlobToRegex(String pattern) {
+ @VisibleForTesting
+ public Pattern convertGlobToRegex(String pattern) {
if (pattern == null) {
return null;
}
return GlobPattern.compile(pattern);
}
+ // Redis allows values for CURSOR up to UNSIGNED_LONG_CAPACITY, but
internally it only makes sense
+ // for us to use values up to Integer.MAX_VALUE, so safely narrow any cursor
values to int here
+
+ @VisibleForTesting
+ public int getIntCursor(String cursorString) {
Review comment:
In order to match Redis' error behaviour, we need to be able to tell the
difference between a value for cursor that's not an integer of any kind
("123abc" for example), and a value that's larger than `Long.MAX_VALUE` but
smaller than the maximum capacity of an unsigned long. Redis returns an error
for the first case, but doesn't for the second (even though it's a nonsense
value for the cursor). In order to check if the cursor value is in that second
range, BigInteger is needed.
The alternative is to deviate from Redis' error behaviour here, but that's
something we've avoided doing everywhere else. It's definitely an obscure
corner case, since users realistically shouldn't ever be passing values for
CURSOR greater than `Integer.MAX_VALUE`, let alone as big as an unsigned long,
so maybe we're okay with having a difference here that we can document, where a
cursor value greater than `Long.MAX_VALUE` returns an error, since we have no
way to differentiate between a value that's too big and a value that's
gibberish.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
