upthewaterspout commented on a change in pull request #6831:
URL: https://github.com/apache/geode/pull/6831#discussion_r706443424
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -243,10 +243,9 @@ public int hstrlen(byte[] field) {
throw new IllegalArgumentException("Requested array size exceeds VM
limit");
}
List<byte[]> resultList = new ArrayList<>((int) maximumCapacity);
- do {
- cursor = hash.scan(cursor, 1,
- (list, key, value) -> addIfMatching(matchPattern, list, key, value),
resultList);
- } while (cursor != 0 && resultList.size() < maximumCapacity);
+
+ cursor = hash.scan(cursor, count,
+ (list, key, value) -> addIfMatching(matchPattern, list, key, value),
resultList);
Review comment:
I think this is going to underpopulate the resultList if there is a
matchPattern. The scan method is only going to call `addIfMatching` for
`maximumCapacity` iterations, rather than waiting until resultList has reached
`maximumCapacity`.
##########
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:
How about we just use Integer.parseInt here (or Long.parseLong, if you
prefer). Using BigInteger is a performance impact. Since the behavior is
undetermined if the user passes as a cursor value other than 0 or a number that
we previously returned, it seems like we could just treat numbers that are too
large as 0 or integer max value or whatever.
https://redis.io/commands/scan#calling-scan-with-a-corrupted-cursor
--
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]
