Create a TaskExpression with unit tests
Simplify optional Task RE pattern and approach in the new class since
the previous regular expression was difficult to understand and
apparently very slow and hard to make an iterator with.
Change-Id: I8271ae26dba51d6e5f0543528e8cc8cc8210ec26
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java b/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
index c5a5b0f..d16c5f6 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
@@ -28,7 +28,8 @@
public static Task preload(Task definition) throws ConfigInvalidException {
String expression = definition.preloadTask;
if (expression != null) {
- Optional<Task> preloadFrom = definition.config.getOptionalTaskForExpression(expression);
+ Optional<Task> preloadFrom =
+ definition.config.getOptionalTask(new TaskExpression(expression));
if (preloadFrom.isPresent()) {
return preloadFrom(definition, preload(preloadFrom.get()));
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
index 32496cf..e1a079b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -24,10 +24,9 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Set;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
import org.eclipse.jgit.errors.ConfigInvalidException;
/** Task Configuration file living in git */
@@ -181,9 +180,6 @@
public static final String SEP = "\0";
- protected static final Pattern OPTIONAL_TASK_PATTERN =
- Pattern.compile("([^ |]*( *[^ |])*) *\\| *");
-
protected static final String SECTION_EXTERNAL = "external";
protected static final String SECTION_NAMES_FACTORY = "names-factory";
protected static final String SECTION_ROOT = "root";
@@ -245,42 +241,22 @@
}
/**
- * Get a Task for this expression.
+ * Get a Task for this TaskExpression.
*
- * @param expression A task expression represents a config string pointing to an expression which
- * includes zero or more task names separated by a '|', and potentially termintated by a '|'.
- * If the expression is not terminated by a '|' it indicates that task resolution of at least
- * one task is required. Task selection priority is from left to right. This can be expressed
- * as: <code>EXPR = [ TASK_NAME '|' ] TASK_NAME [ '|' ]</code>
- * <p>Example expressions to prioritized names and requirements:
- * <ul>
- * <li><code> "simple" -> ("simple") required</code>
- * <li><code> "world | peace" -> ("world", "peace") required</code>
- * <li><code> "shadenfreud |" -> ("shadenfreud") optional</code>
- * <li><code> "foo | bar |" -> ("foo", "bar") optional</code>
- * </ul>
- *
+ * @param TaskExpression
* @return Optional<Task> which is empty if the expression is optional and no tasks are resolved
* @throws ConfigInvalidException if the expression requires a task and no tasks are resolved
*/
- public Optional<Task> getOptionalTaskForExpression(String expression)
- throws ConfigInvalidException {
- int end = 0;
- Matcher m = OPTIONAL_TASK_PATTERN.matcher(expression);
- while (m.find()) {
- end = m.end();
- Optional<Task> task = getOptionalTask(m.group(1));
- if (task.isPresent()) {
- return task;
+ public Optional<Task> getOptionalTask(TaskExpression expression) throws ConfigInvalidException {
+ try {
+ for (String name : expression) {
+ Optional<Task> task = getOptionalTask(name);
+ if (task.isPresent()) {
+ return task;
+ }
}
- }
-
- String last = expression.substring(end);
- if (!"".equals(last)) { // expression was not optional
- Optional<Task> task = getOptionalTask(last);
- if (task.isPresent()) {
- return task;
- }
+ } catch (NoSuchElementException e) {
+ // expression was not optional but we ran out of names to try
throw new ConfigInvalidException("task not defined");
}
return Optional.empty();
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java
new file mode 100644
index 0000000..ee55937
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java
@@ -0,0 +1,79 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed 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 com.googlesource.gerrit.plugins.task;
+
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * A TaskExpression represents a config string pointing to an expression which includes zero or more
+ * task names separated by a '|', and potentially termintated by a '|'. If the expression is not
+ * terminated by a '|' it indicates that task resolution of at least one task is required. Task
+ * selection priority is from left to right. This can be expressed as: <code>
+ * EXPR = [ TASK_NAME '|' ] TASK_NAME [ '|' ]</code>
+ *
+ * <p>Example expressions to prioritized names and requirements:
+ *
+ * <ul>
+ * <li><code> "simple" -> ("simple") required</code>
+ * <li><code> "world | peace" -> ("world", "peace") required</code>
+ * <li><code> "shadenfreud |" -> ("shadenfreud") optional</code>
+ * <li><code> "foo | bar |" -> ("foo", "bar") optional</code>
+ * </ul>
+ */
+public class TaskExpression implements Iterable<String> {
+ protected static final Pattern EXPRESSION_PATTERN = Pattern.compile("([^ |]+[^|]*)(\\|)?");
+
+ protected final String expression;
+
+ public TaskExpression(String expression) {
+ this.expression = expression;
+ }
+
+ @Override
+ public Iterator<String> iterator() {
+ return new Iterator<String>() {
+ Matcher m = EXPRESSION_PATTERN.matcher(expression);
+ Boolean hasNext;
+ boolean optional;
+
+ @Override
+ public boolean hasNext() {
+ if (hasNext == null) {
+ hasNext = m.find();
+ if (hasNext) {
+ optional = m.group(2) != null;
+ }
+ }
+ if (!hasNext && !optional) {
+ return true; // fake it so next() throws an Exception
+ }
+ return hasNext;
+ }
+
+ @Override
+ public String next() {
+ hasNext(); // in case next() was (re)called w/o calling hasNext()
+ if (!hasNext) {
+ throw new NoSuchElementException("No more names, yet expression was not optional");
+ }
+ hasNext = null;
+ return m.group(1).trim();
+ }
+ };
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
index fd37c9a..ae76aae 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -179,7 +179,7 @@
protected void addSubTaskDefinitions() {
for (String expression : task.subTasks) {
try {
- Optional<Task> def = task.config.getOptionalTaskForExpression(expression);
+ Optional<Task> def = task.config.getOptionalTask(new TaskExpression(expression));
if (def.isPresent()) {
addSubDefinition(def.get());
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java b/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java
new file mode 100644
index 0000000..13e471f
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java
@@ -0,0 +1,147 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed 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 com.googlesource.gerrit.plugins.task;
+
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import junit.framework.TestCase;
+
+/*
+ * <ul>
+ * <li><code> "simple" -> ("simple") required</code>
+ * <li><code> "world | peace" -> ("world", "peace") required</code>
+ * <li><code> "shadenfreud |" -> ("shadenfreud") optional</code>
+ * <li><code> "foo | bar |" -> ("foo", "bar") optional</code>
+ * </ul>
+ */
+public class TaskExpressionTest extends TestCase {
+ public static String SIMPLE = "simple";
+ public static String WORLD = "world";
+ public static String PEACE = "peace";
+
+ public void testBlank() {
+ TaskExpression exp = new TaskExpression("");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertNoSuchElementException(it);
+ }
+
+ public void testRequiredSingleName() {
+ TaskExpression exp = new TaskExpression(SIMPLE);
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), SIMPLE);
+ assertTrue(it.hasNext());
+ assertNoSuchElementException(it);
+ }
+
+ public void testOptionalSingleName() {
+ TaskExpression exp = new TaskExpression(SIMPLE + "|");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), SIMPLE);
+ assertFalse(it.hasNext());
+ }
+
+ public void testRequiredTwoNames() {
+ TaskExpression exp = new TaskExpression(WORLD + "|" + PEACE);
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), WORLD);
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), PEACE);
+ assertTrue(it.hasNext());
+ assertNoSuchElementException(it);
+ }
+
+ public void testOptionalTwoNames() {
+ TaskExpression exp = new TaskExpression(WORLD + "|" + PEACE + "|");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), WORLD);
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), PEACE);
+ assertFalse(it.hasNext());
+ }
+
+ public void testBlankSpaces() {
+ TaskExpression exp = new TaskExpression(" ");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertNoSuchElementException(it);
+ }
+
+ public void testRequiredSingleNameLeadingSpaces() {
+ TaskExpression exp = new TaskExpression(" " + SIMPLE);
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), SIMPLE);
+ assertTrue(it.hasNext());
+ assertNoSuchElementException(it);
+ }
+
+ public void testRequiredSingleNameTrailingSpaces() {
+ TaskExpression exp = new TaskExpression(SIMPLE + " ");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), SIMPLE);
+ assertTrue(it.hasNext());
+ assertNoSuchElementException(it);
+ }
+
+ public void testOptionalSingleNameLeadingSpaces() {
+ TaskExpression exp = new TaskExpression(" " + SIMPLE + "|");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), SIMPLE);
+ assertFalse(it.hasNext());
+ }
+
+ public void testOptionalSingleNameTrailingSpaces() {
+ TaskExpression exp = new TaskExpression(SIMPLE + "| ");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), SIMPLE);
+ assertFalse(it.hasNext());
+ }
+
+ public void testOptionalSingleNameMiddleSpaces() {
+ TaskExpression exp = new TaskExpression(SIMPLE + " |");
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), SIMPLE);
+ assertFalse(it.hasNext());
+ }
+
+ public void testRequiredTwoNamesMiddleSpaces() {
+ TaskExpression exp = new TaskExpression(WORLD + " | " + PEACE);
+ Iterator<String> it = exp.iterator();
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), WORLD);
+ assertTrue(it.hasNext());
+ assertEquals(it.next(), PEACE);
+ assertTrue(it.hasNext());
+ assertNoSuchElementException(it);
+ }
+
+ protected static void assertNoSuchElementException(Iterator<String> it) {
+ try {
+ it.next();
+ assertTrue(false);
+ } catch (NoSuchElementException e) {
+ assertTrue(true);
+ }
+ }
+}
