You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ud...@apache.org on 2020/07/02 18:10:38 UTC
[geode] 02/29: GEODE-8288: Match Native Redis's glob-style pattern
(#5282)
This is an automated email from the ASF dual-hosted git repository.
udo pushed a commit to branch feature/GEODE-8294
in repository https://gitbox.apache.org/repos/asf/geode.git
commit 9b99c38b9faa4d7e4b3d8a526f8a691e347256cd
Author: Sarah Abbey <41...@users.noreply.github.com>
AuthorDate: Wed Jun 24 15:25:02 2020 -0400
GEODE-8288: Match Native Redis's glob-style pattern (#5282)
---
...a => GlobPatternNativeRedisAcceptanceTest.java} | 14 +-
.../key/KeysNativeRedisAcceptanceTest.java | 6 -
.../executor/GlobPatternIntegrationTest.java | 264 +++++++++++++++++++++
.../internal/executor/key/KeysIntegrationTest.java | 10 +-
.../geode/redis/internal/executor/GlobPattern.java | 48 +---
.../redis/internal/executor/key/KeysExecutor.java | 11 +-
6 files changed, 290 insertions(+), 63 deletions(-)
diff --git a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/KeysNativeRedisAcceptanceTest.java b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/GlobPatternNativeRedisAcceptanceTest.java
similarity index 81%
copy from geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/KeysNativeRedisAcceptanceTest.java
copy to geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/GlobPatternNativeRedisAcceptanceTest.java
index 960b76d..47d400f 100644
--- a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/KeysNativeRedisAcceptanceTest.java
+++ b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/GlobPatternNativeRedisAcceptanceTest.java
@@ -13,21 +13,17 @@
* the License.
*/
-package org.apache.geode.redis.internal.executor.key;
+package org.apache.geode.redis.internal.executor;
-import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.ClassRule;
-import org.junit.experimental.categories.Category;
import org.junit.rules.TestRule;
import org.testcontainers.containers.GenericContainer;
import redis.clients.jedis.Jedis;
-import org.apache.geode.test.junit.categories.RedisTest;
import org.apache.geode.test.junit.rules.IgnoreOnWindowsRule;
-@Category({RedisTest.class})
-public class KeysNativeRedisAcceptanceTest extends KeysIntegrationTest {
+public class GlobPatternNativeRedisAcceptanceTest extends GlobPatternIntegrationTest {
// Docker compose does not work on windows in CI. Ignore this test on windows
// Using a RuleChain to make sure we ignore the test before the rule comes into play
@@ -40,10 +36,4 @@ public class KeysNativeRedisAcceptanceTest extends KeysIntegrationTest {
redisContainer.start();
jedis = new Jedis("localhost", redisContainer.getFirstMappedPort(), REDIS_CLIENT_TIMEOUT);
}
-
- @AfterClass
- public static void tearDown() {
- jedis.close();
- }
-
}
diff --git a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/KeysNativeRedisAcceptanceTest.java b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/KeysNativeRedisAcceptanceTest.java
index 960b76d..e82fda7 100644
--- a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/KeysNativeRedisAcceptanceTest.java
+++ b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/KeysNativeRedisAcceptanceTest.java
@@ -15,7 +15,6 @@
package org.apache.geode.redis.internal.executor.key;
-import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.experimental.categories.Category;
@@ -41,9 +40,4 @@ public class KeysNativeRedisAcceptanceTest extends KeysIntegrationTest {
jedis = new Jedis("localhost", redisContainer.getFirstMappedPort(), REDIS_CLIENT_TIMEOUT);
}
- @AfterClass
- public static void tearDown() {
- jedis.close();
- }
-
}
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/GlobPatternIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/GlobPatternIntegrationTest.java
new file mode 100644
index 0000000..6c0c533
--- /dev/null
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/GlobPatternIntegrationTest.java
@@ -0,0 +1,264 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.redis.GeodeRedisServerRule;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+
+public class GlobPatternIntegrationTest {
+
+ public static Jedis jedis;
+ public static int REDIS_CLIENT_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+
+ @ClassRule
+ public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+
+ @BeforeClass
+ public static void setUp() {
+ jedis = new Jedis("localhost", server.getPort(), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void flushAll() {
+ jedis.flushAll();
+ }
+
+ @AfterClass
+ public static void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void asterisk_matchesAnySequenceOfCharacters() {
+ jedis.set("ble", "value");
+ jedis.set("blo9%^e", "value");
+ jedis.set("blooo", "value");
+
+ assertThat(jedis.keys("bl*e")).containsExactlyInAnyOrder("ble", "blo9%^e");
+ }
+
+ @Test
+ public void questionMark_matchesAnySingleCharacter() {
+ jedis.set("hollo", "value");
+ jedis.set("hello", "value");
+ jedis.set("hoollo", "value");
+
+ assertThat(jedis.keys("h?llo")).containsExactlyInAnyOrder("hollo", "hello");
+ }
+
+ @Test
+ public void charactersInsideBrackets_matchSpecifiedCharacters() {
+ jedis.set("top", "value");
+ jedis.set("tap", "value");
+ jedis.set("tep", "value");
+
+ assertThat(jedis.keys("t[oa]p")).containsExactlyInAnyOrder("top", "tap");
+ }
+
+ @Test
+ public void dashInsideBrackets_matchesSpecifiedRangeOfCharacters() {
+ jedis.set("smch", "value");
+ jedis.set("srch", "value");
+ jedis.set("such", "value");
+
+ assertThat(jedis.keys("s[m-s]ch")).containsExactlyInAnyOrder("smch", "srch");
+ }
+
+ @Test
+ public void caretInsideBrackets_doesNotMatchSpecifiedCharacter() {
+ jedis.set("patches", "value");
+ jedis.set("petches", "value");
+ jedis.set("potches", "value");
+
+ assertThat(jedis.keys("p[^o]tches")).containsExactlyInAnyOrder("petches", "patches");
+ }
+
+ @Test
+ public void charactersInsideUnclosedBracket_matchSpecifiedCharacters() {
+ jedis.set("*", "value");
+ jedis.set("}", "value");
+ jedis.set("4", "value");
+ jedis.set("[", "value");
+
+ assertThat(jedis.keys("[*}4")).containsExactlyInAnyOrder("*", "}", "4");
+ }
+
+ @Test
+ public void dashInsideUnclosedBracket_matchesSpecifiedRangeOfCharacters() {
+ jedis.set("sm", "value");
+ jedis.set("ss", "value");
+ jedis.set("ssch", "value");
+
+ assertThat(jedis.keys("s[m-sch")).containsExactlyInAnyOrder("sm", "ss");
+ }
+
+ @Test
+ public void caretInsideUnclosedBracket_doesNotMatchSpecifiedCharacter() {
+ jedis.set("kermi", "value");
+ jedis.set("kermu", "value");
+ jedis.set("kermo", "value");
+
+ assertThat(jedis.keys("kerm[^i")).containsExactlyInAnyOrder("kermu", "kermo");
+ }
+
+ @Test
+ public void doubleOpenBracket_matchesSpecifiedCharacters() {
+ jedis.set("*", "value");
+ jedis.set("[", "value");
+ jedis.set("[4", "value");
+ jedis.set("&8t", "value");
+
+ assertThat(jedis.keys("[[*")).containsExactlyInAnyOrder("*", "[");
+ }
+
+ @Test
+ public void escapedOpenBracket_matchesVerbatim() {
+ jedis.set("[", "value");
+ jedis.set("[o9$", "value");
+ jedis.set("such", "value");
+
+ assertThat(jedis.keys("\\[*")).containsExactlyInAnyOrder("[", "[o9$");
+ }
+
+ @Test
+ public void escapedQuestionMark_matchesVerbatim() {
+ jedis.set("?", "value");
+ jedis.set("?oo", "value");
+ jedis.set("such", "value");
+
+ assertThat(jedis.keys("\\?*")).containsExactlyInAnyOrder("?", "?oo");
+ }
+
+ @Test
+ public void escapedAsterisk_matchesVerbatim() {
+ jedis.set("*", "value");
+ jedis.set("*9x", "value");
+ jedis.set("such", "value");
+
+ assertThat(jedis.keys("\\**")).containsExactlyInAnyOrder("*", "*9x");
+ }
+
+ @Test
+ public void caretOutsideBrackets_matchesVerbatim() {
+ jedis.set("^o", "value");
+ jedis.set("^o99", "value");
+ jedis.set("^O", "value");
+
+ assertThat(jedis.keys("^o*")).containsExactlyInAnyOrder("^o", "^o99");
+ }
+
+ @Test
+ public void openCurly_matchesVerbatim() {
+ jedis.set("{", "value");
+ jedis.set("{&0", "value");
+ jedis.set("bogus", "value");
+
+ assertThat(jedis.keys("{*")).containsExactlyInAnyOrder("{", "{&0");
+ }
+
+ @Test
+ public void closedCurly_matchesVerbatim() {
+ jedis.set("}", "value");
+ jedis.set("}&0", "value");
+ jedis.set("bogus", "value");
+
+ assertThat(jedis.keys("}*")).containsExactlyInAnyOrder("}", "}&0");
+ }
+
+ @Test
+ public void comma_matchesVerbatim() {
+ jedis.set("kermit", "value");
+ jedis.set("kerm,t", "value");
+ jedis.set("kermot", "value");
+ jedis.set("kermat", "value");
+
+ assertThat(jedis.keys("kerm[i,o]t")).containsExactlyInAnyOrder("kermit", "kermot", "kerm,t");
+ }
+
+ @Test
+ public void exclamationPoint_matchesVerbatim() {
+ jedis.set("kerm!t", "value");
+ jedis.set("kermot", "value");
+ jedis.set("kermit", "value");
+
+ assertThat(jedis.keys("kerm[!i]t")).containsExactlyInAnyOrder("kermit", "kerm!t");
+ }
+
+ @Test
+ public void period_matchesVerbatim() {
+ jedis.set("...", "value");
+ jedis.set(".", "value");
+ jedis.set(".&*", "value");
+ jedis.set("kermat", "value");
+
+ assertThat(jedis.keys(".*")).containsExactlyInAnyOrder("...", ".", ".&*");
+ }
+
+ @Test
+ public void plusSign_matchesVerbatim() {
+ jedis.set("ab", "value");
+ jedis.set("aaaaab", "value");
+ jedis.set("a+b", "value");
+
+ assertThat(jedis.keys("a+b")).containsExactlyInAnyOrder("a+b");
+ }
+
+ @Test
+ public void orSign_matchesVerbatim() {
+ jedis.set("cat|dog", "value");
+ jedis.set("cat", "value");
+ jedis.set("dog", "value");
+
+ assertThat(jedis.keys("cat|dog")).containsExactlyInAnyOrder("cat|dog");
+ }
+
+ @Test
+ public void dollarSign_matchesVerbatim() {
+ jedis.set("the cat", "value");
+ jedis.set("the cat$", "value");
+ jedis.set("dog", "value");
+
+ assertThat(jedis.keys("the cat$")).containsExactlyInAnyOrder("the cat$");
+ }
+
+ @Test
+ public void parentheses_matchVerbatim() {
+ jedis.set("orange(cat)z", "value");
+ jedis.set("orange", "value");
+ jedis.set("orangecat", "value");
+
+ assertThat(jedis.keys("orange(cat)?")).containsExactlyInAnyOrder("orange(cat)z");
+ }
+
+ @Test
+ public void escapedBackslash_matchesVerbatim() {
+ jedis.set("\\", "value");
+ jedis.set("\\!", "value");
+ jedis.set("*", "value");
+
+ assertThat(jedis.keys("\\\\*")).containsExactlyInAnyOrder("\\", "\\!");
+ }
+}
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/KeysIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/KeysIntegrationTest.java
index 8ff4273..d41538e 100644
--- a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/KeysIntegrationTest.java
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/KeysIntegrationTest.java
@@ -52,7 +52,7 @@ public class KeysIntegrationTest {
}
@Test
- public void keys_givenSplat_withASCIIdata_returnsExpectedMatches() {
+ public void givenSplat_withASCIIdata_returnsExpectedMatches() {
jedis.set("string1", "v1");
jedis.sadd("set1", "member1");
jedis.hset("hash1", "key1", "field1");
@@ -63,7 +63,7 @@ public class KeysIntegrationTest {
}
@Test
- public void keys_givenSplat_withBinaryData_returnsExpectedMatches() {
+ public void givenSplat_withBinaryData_returnsExpectedMatches() {
byte[] stringKey =
new byte[] {(byte) 0xac, (byte) 0xed, 0, 4, 0, 5, 's', 't', 'r', 'i', 'n', 'g', '1'};
byte[] value = new byte[] {'v', '1'};
@@ -86,11 +86,15 @@ public class KeysIntegrationTest {
}
@Test
- public void keys_givenBinaryValue_withExactMatch_preservesBinaryData() {
+ public void givenBinaryValue_withExactMatch_preservesBinaryData() {
byte[] stringKey =
new byte[] {(byte) 0xac, (byte) 0xed, 0, 4, 0, 5, 's', 't', 'r', 'i', 'n', 'g', '1'};
jedis.set(stringKey, stringKey);
assertThat(jedis.keys(stringKey)).containsExactlyInAnyOrder(stringKey);
}
+ @Test
+ public void givenMalformedGlobPattern_returnsEmptySet() {
+ assertThat(jedis.keys("[][]")).isEmpty();
+ }
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/GlobPattern.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/GlobPattern.java
index c34de9f..71ecaac 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/GlobPattern.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/GlobPattern.java
@@ -81,7 +81,6 @@ public class GlobPattern {
private Pattern createPattern(String glob) {
StringBuilder regex = new StringBuilder();
int setOpen = 0;
- int curlyOpen = 0;
int len = glob.length();
hasWildcard = false;
@@ -90,17 +89,19 @@ public class GlobPattern {
switch (c) {
case BACKSLASH:
- if (++i >= len) {
- error("Missing escaped character", glob, i);
+ if (++i < len) {
+ regex.append(c).append(glob.charAt(i));
+ continue;
}
- regex.append(c).append(glob.charAt(i));
- continue;
+ break;
case '.':
case '$':
case '(':
case ')':
case '|':
case '+':
+ case '{':
+ case '!':
// escape regex special chars that are not glob special chars
regex.append(BACKSLASH);
break;
@@ -112,25 +113,9 @@ public class GlobPattern {
regex.append('.');
hasWildcard = true;
continue;
- case '{': // start of a group
- regex.append("(?:"); // non-capturing
- curlyOpen++;
- hasWildcard = true;
- continue;
- case ',':
- regex.append(curlyOpen > 0 ? '|' : c);
- continue;
- case '}':
- if (curlyOpen > 0) {
- // end of a group
- curlyOpen--;
- regex.append(")");
- continue;
- }
- break;
case '[':
if (setOpen > 0) {
- error("Unclosed character class", glob, i);
+ regex.append(BACKSLASH);
}
setOpen++;
hasWildcard = true;
@@ -140,13 +125,7 @@ public class GlobPattern {
regex.append(BACKSLASH);
}
break;
- case '!': // [! needs to be translated to [^
- regex.append(setOpen > 0 && '[' == glob.charAt(i - 1) ? '^' : '!');
- continue;
case ']':
- // Many set errors like [][] could not be easily detected here,
- // as []], []-] and [-] are all valid POSIX glob and java regex.
- // We'll just let the regex compiler do the real work.
setOpen = 0;
break;
default:
@@ -155,19 +134,10 @@ public class GlobPattern {
}
if (setOpen > 0) {
- error("Unclosed character class", glob, len);
+ regex.append(']');
}
- if (curlyOpen > 0) {
- error("Unclosed group", glob, len);
- }
- return Pattern.compile(regex.toString());
- }
- /**
- * @return true if this is a wildcard pattern (with special chars)
- */
- public boolean hasWildcard() {
- return hasWildcard;
+ return Pattern.compile(regex.toString());
}
@Override
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/KeysExecutor.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/KeysExecutor.java
index 206ad2e..9556055 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/KeysExecutor.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/KeysExecutor.java
@@ -15,14 +15,15 @@
*/
package org.apache.geode.redis.internal.executor.key;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_ILLEGAL_GLOB;
-
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
+import org.apache.logging.log4j.Logger;
+
+import org.apache.geode.logging.internal.log4j.api.LogService;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.executor.AbstractExecutor;
import org.apache.geode.redis.internal.executor.GlobPattern;
@@ -32,6 +33,7 @@ import org.apache.geode.redis.internal.netty.Command;
import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
public class KeysExecutor extends AbstractExecutor {
+ private static final Logger logger = LogService.getLogger();
@Override
public RedisResponse executeCommand(Command command,
@@ -45,7 +47,10 @@ public class KeysExecutor extends AbstractExecutor {
try {
pattern = GlobPattern.compile(glob);
} catch (PatternSyntaxException e) {
- return RedisResponse.error(ERROR_ILLEGAL_GLOB);
+ logger.warn(
+ "Could not compile the pattern: '{}' due to the following exception: '{}'. KEYS will return an empty list.",
+ glob, e.getMessage());
+ return RedisResponse.emptyArray();
}
for (ByteArrayWrapper bytesKey : allKeys) {