GEODE-8582: Redis SCAN returns internal server error (apache#5603)

Co-authored-by: Darrel Schneider <[email protected]>

geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/ScanNativeRedisAcceptanceTest.java

@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.key;
+
+import org.junit.ClassRule;
+
+import org.apache.geode.NativeRedisTestRule;
+
+public class ScanNativeRedisAcceptanceTest extends AbstractScanIntegrationTest {
+
+  @ClassRule
+  public static NativeRedisTestRule redis = new NativeRedisTestRule();
+
+  @Override
+  public int getPort() {
+    return redis.getPort();
+  }
+
+}

geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java

@@ -16,9 +16,11 @@
 package org.apache.geode.redis.internal.executor.pubsub;
 
 import org.junit.ClassRule;
+import org.junit.Ignore;
 
 import org.apache.geode.NativeRedisTestRule;
 
+@Ignore("GEODE-8577")
 public class PubSubNativeRedisAcceptanceTest extends AbstractPubSubIntegrationTest {
   @ClassRule
   public static NativeRedisTestRule redis = new NativeRedisTestRule();

geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/AbstractScanIntegrationTest.java

@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.key;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.ScanParams;
+import redis.clients.jedis.ScanResult;
+
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.RedisPortSupplier;
+
+public abstract class AbstractScanIntegrationTest implements RedisPortSupplier {
+
+  private Jedis jedis;
+  private static final int REDIS_CLIENT_TIMEOUT =
+      Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+
+  @Before
+  public void setUp() {
+    jedis = new Jedis("localhost", getPort(), REDIS_CLIENT_TIMEOUT);
+  }
+
+  @After
+  public void tearDown() {
+    jedis.flushAll();
+    jedis.close();
+  }
+
+  @Test
+  public void givenOneKey_scanReturnsKey() {
+    jedis.set("a", "1");
+    ScanResult<String> result = jedis.scan("0");
+
+    assertThat(result.isCompleteIteration()).isTrue();
+    assertThat(result.getResult()).containsExactly("a");
+  }
+
+  @Test
+  public void givenEmptyDatabase_scanReturnsEmptyResult() {
+    ScanResult<String> result = jedis.scan("0");
+
+    assertThat(result.isCompleteIteration()).isTrue();
+    assertThat(result.getResult()).isEmpty();
+  }
+
+  @Test
+  public void givenMultipleKeys_scanReturnsAllKeys() {
+    jedis.set("a", "1");
+    jedis.sadd("b", "green", "orange");
+    jedis.hset("c", "potato", "sweet");
+    ScanResult<String> result = jedis.scan("0");
+
+    assertThat(result.isCompleteIteration()).isTrue();
+    assertThat(result.getResult()).containsExactlyInAnyOrder("a", "b", "c");
+  }
+
+  @Test
+  public void givenMultipleKeysWithCount_scanReturnsAllKeysWithoutDuplicates() {
+    jedis.set("a", "1");
+    jedis.sadd("b", "green", "orange");
+    jedis.hset("c", "potato", "sweet");
+    ScanParams scanParams = new ScanParams();
+    scanParams.count(1);
+
+    ScanResult<String> result = jedis.scan("0", scanParams);
+    List<String> allKeysFromScan = new ArrayList<>();
+    allKeysFromScan.addAll(result.getResult());
+
+    while (!result.isCompleteIteration()) {
+      result = jedis.scan(result.getCursor(), scanParams);
+      allKeysFromScan.addAll(result.getResult());
+    }
+
+    assertThat(result.isCompleteIteration()).isTrue();
+    assertThat(allKeysFromScan).containsExactlyInAnyOrder("a", "b", "c");
+  }
+
+  @Test
+  public void givenMultipleKeysWithMatch_scanReturnsAllMatchingKeysWithoutDuplicates() {
+    jedis.set("a", "1");
+    jedis.sadd("b", "green", "orange");
+    jedis.hset("c", "potato", "sweet");
+    ScanParams scanParams = new ScanParams();
+    scanParams.match("a*");
+
+    ScanResult<String> result = jedis.scan("0", scanParams);
+
+    assertThat(result.isCompleteIteration()).isTrue();
+    assertThat(result.getResult()).containsExactly("a");
+  }
+}

geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/ScanIntegrationTest.java

@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.key;
+
+import org.junit.ClassRule;
+
+import org.apache.geode.redis.GeodeRedisServerRule;
+
+public class ScanIntegrationTest extends AbstractScanIntegrationTest {
+
+  @ClassRule
+  public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+
+  @Override
+  public int getPort() {
+    return server.getPort();
+  }
+
+}

geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/ScanExecutor.java

@@ -24,6 +24,7 @@
 import java.util.regex.PatternSyntaxException;
 
 import org.apache.geode.redis.internal.RedisConstants.ArityDef;
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
 import org.apache.geode.redis.internal.executor.RedisResponse;
 import org.apache.geode.redis.internal.netty.Coder;
 import org.apache.geode.redis.internal.netty.Command;
@@ -32,8 +33,7 @@
 public class ScanExecutor extends AbstractScanExecutor {
 
   @Override
-  public RedisResponse executeCommand(Command command,
-      ExecutionHandlerContext context) {
+  public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
     List<byte[]> commandElems = command.getProcessedCommand();
 
     if (commandElems.size() < 2) {
@@ -93,44 +93,45 @@ public RedisResponse executeCommand(Command command,
       return RedisResponse.error(ERROR_ILLEGAL_GLOB);
     }
 
-    @SuppressWarnings("unchecked")
-    List<String> returnList = (List<String>) getIteration(getDataRegion(context).keySet(),
-        matchPattern, count, cursor);
+    List<String> returnList = getIteration(getDataRegion(context).keySet(), matchPattern, count,
+        cursor);
 
     return RedisResponse.scan(returnList);
   }
 
-  @SuppressWarnings("unchecked")
-  private List<?> getIteration(Collection<?> list, Pattern matchPattern, int count, int cursor) {
-    List<String> returnList = new ArrayList<String>();
+  private List<String> getIteration(Collection<ByteArrayWrapper> list, Pattern matchPattern,
+      int count, int cursor) {
+    List<String> returnList = new ArrayList<>();
     int size = list.size();
     int beforeCursor = 0;
     int numElements = 0;
     int i = -1;
-    for (String key : (Collection<String>) list) {
+    for (ByteArrayWrapper key : list) {
       i++;
       if (beforeCursor < cursor) {
         beforeCursor++;
         continue;
-      } else if (numElements < count) {
-        if (matchPattern != null) {
-          if (matchPattern.matcher(key).matches()) {
-            returnList.add(key);
-            numElements++;
-          }
-        } else {
-          returnList.add(key);
+      }
+
+      String keyString = key.toString();
+      if (matchPattern != null) {
+        if (matchPattern.matcher(keyString).matches()) {
+          returnList.add(keyString);
           numElements++;
         }
       } else {
+        returnList.add(keyString);
+        numElements++;
+      }
+      if (numElements == count) {
         break;
       }
     }
 
     if (i >= size - 1) {
       returnList.add(0, String.valueOf(0));
     } else {
-      returnList.add(0, String.valueOf(i));
+      returnList.add(0, String.valueOf(i + 1));
     }
     return returnList;
   }