plugins/task: Move TaskExpression iteration to Preloader
Having this iteration inside TaskConfig class prevents us from
referencing tasks from other files from a TaskExpression in
future. Ideally, TaskConfig shouldn't know about the Preloader
and this change removes cyclic dependency between them.
Change-Id: I34e710b2df2567217653ac545cdc26f21105aa10
diff --git a/BUILD b/BUILD
index 108dae2..dbe73f9 100644
--- a/BUILD
+++ b/BUILD
@@ -1,6 +1,7 @@
load(
"//tools/bzl:plugin.bzl",
"PLUGIN_DEPS",
+ "PLUGIN_TEST_DEPS",
"gerrit_plugin",
)
load("//tools/bzl:genrule2.bzl", "genrule2")
@@ -70,7 +71,7 @@
name = "junit-tests",
size = "small",
srcs = glob(["src/test/java/**/*Test.java"]),
- deps = [plugin_name],
+ deps = PLUGIN_TEST_DEPS + [plugin_name],
)
sh_test(
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 63fb0db..c48ed6e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
@@ -20,36 +20,62 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.NoSuchElementException;
import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
/** Use to pre-load a task definition with values from its preload-task definition. */
public class Preloader {
- protected final TaskConfig config;
- protected final Map<String, Optional<Task>> optionalTaskByExpression = new HashMap<>();
+ protected final Map<TaskExpressionKey, Optional<Task>> optionalTaskByExpression = new HashMap<>();
- public Preloader(TaskConfig config) {
- this.config = config;
+ public List<Task> getRootTasks(TaskConfig cfg) {
+ return getTasks(cfg, TaskConfig.SECTION_ROOT);
}
- public Optional<Task> preloadOptional(TaskExpression expression) throws ConfigInvalidException {
- Optional<Task> task = optionalTaskByExpression.get(expression.getKey());
+ public List<Task> getTasks(TaskConfig cfg) {
+ return getTasks(cfg, TaskConfig.SECTION_TASK);
+ }
+
+ protected List<Task> getTasks(TaskConfig cfg, String type) {
+ List<Task> preloaded = new ArrayList<>();
+ for (Task task : cfg.getTasks(type)) {
+ try {
+ preloaded.add(preload(task));
+ } catch (ConfigInvalidException e) {
+ preloaded.add(null);
+ }
+ }
+ return preloaded;
+ }
+
+ /**
+ * Get a preloaded Task for this TaskExpression.
+ *
+ * @param expression
+ * @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> getOptionalTask(TaskConfig cfg, TaskExpression expression)
+ throws ConfigInvalidException {
+ Optional<Task> task = optionalTaskByExpression.get(expression.key);
if (task == null) {
- task = loadOptional(expression);
- optionalTaskByExpression.put(expression.getKey(), task);
+ task = preloadOptionalTask(cfg, expression);
+ optionalTaskByExpression.put(expression.key, task);
}
return task;
}
- protected Optional<Task> loadOptional(TaskExpression expression) throws ConfigInvalidException {
- Optional<Task> definition = config.getOptionalTask(expression);
+ protected Optional<Task> preloadOptionalTask(TaskConfig cfg, TaskExpression expression)
+ throws ConfigInvalidException {
+ Optional<Task> definition = loadOptionalTask(cfg, expression);
return definition.isPresent() ? Optional.of(preload(definition.get())) : definition;
}
public Task preload(Task definition) throws ConfigInvalidException {
String expression = definition.preloadTask;
if (expression != null) {
- Optional<Task> preloadFrom = preloadOptional(new TaskExpression(expression));
+ Optional<Task> preloadFrom =
+ getOptionalTask(definition.config, new TaskExpression(definition.file(), expression));
if (preloadFrom.isPresent()) {
return preloadFrom(definition, preloadFrom.get());
}
@@ -57,6 +83,22 @@
return definition;
}
+ protected Optional<Task> loadOptionalTask(TaskConfig cfg, TaskExpression expression)
+ throws ConfigInvalidException {
+ try {
+ for (String name : expression) {
+ Optional<Task> task = cfg.getOptionalTask(name);
+ 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();
+ }
+
protected static Task preloadFrom(Task definition, Task preloadFrom) {
Task preloadTo = definition.config.new Task(definition.subSection);
for (Field field : definition.getClass().getFields()) {
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 8b172d7..698b33a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -26,10 +26,8 @@
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 org.eclipse.jgit.errors.ConfigInvalidException;
/** Task Configuration file living in git */
public class TaskConfig extends AbstractVersionedMetaData {
@@ -138,6 +136,10 @@
return key.task();
}
+ public FileKey file() {
+ return key.subSection().file();
+ }
+
public TaskKey key() {
return key;
}
@@ -214,8 +216,6 @@
public boolean isVisible;
public boolean isTrusted;
- protected final Preloader preloader;
-
public TaskConfig(FileKey file, boolean isVisible, boolean isTrusted) {
this(file.branch(), file, isVisible, isTrusted);
}
@@ -226,27 +226,6 @@
this.file = file;
this.isVisible = isVisible;
this.isTrusted = isTrusted;
- preloader = new Preloader(this);
- }
-
- public List<Task> getPreloadedRootTasks() {
- return getPreloadedTasks(SECTION_ROOT);
- }
-
- public List<Task> getPreloadedTasks() {
- return getPreloadedTasks(SECTION_TASK);
- }
-
- protected List<Task> getPreloadedTasks(String type) {
- List<Task> preloaded = new ArrayList<>();
- for (Task task : getTasks(type)) {
- try {
- preloaded.add(preloader.preload(task));
- } catch (ConfigInvalidException e) {
- preloaded.add(null);
- }
- }
- return preloaded;
}
protected List<Task> getTasks(String type) {
@@ -267,34 +246,6 @@
return externals;
}
- /**
- * Get a preloaded Task for this TaskExpression.
- *
- * @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> getPreloadedOptionalTask(TaskExpression expression)
- throws ConfigInvalidException {
- return preloader.preloadOptional(expression);
- }
-
- protected Optional<Task> getOptionalTask(TaskExpression expression)
- throws ConfigInvalidException {
- try {
- for (String name : expression) {
- Optional<Task> task = getOptionalTask(name);
- 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();
- }
-
protected Optional<Task> getOptionalTask(String name) {
SubSectionKey subSection = subSectionKey(SECTION_TASK, name);
return getNames(subSection).isEmpty()
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java
index 036071b..90dffff 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java
@@ -37,21 +37,16 @@
*/
public class TaskExpression implements Iterable<String> {
protected static final Pattern EXPRESSION_PATTERN = Pattern.compile("([^ |]+[^|]*)(\\|)?");
+ protected final TaskExpressionKey key;
- protected final String expression;
-
- public TaskExpression(String expression) {
- this.expression = expression;
- }
-
- public String getKey() {
- return expression;
+ public TaskExpression(FileKey key, String expression) {
+ this.key = TaskExpressionKey.create(key, expression);
}
@Override
public Iterator<String> iterator() {
return new Iterator<String>() {
- Matcher m = EXPRESSION_PATTERN.matcher(expression);
+ Matcher m = EXPRESSION_PATTERN.matcher(key.expression());
Boolean hasNext;
boolean optional;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpressionKey.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpressionKey.java
new file mode 100644
index 0000000..69afd26
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpressionKey.java
@@ -0,0 +1,15 @@
+package com.googlesource.gerrit.plugins.task;
+
+import com.google.auto.value.AutoValue;
+
+/** A key for TaskExpression. */
+@AutoValue
+public abstract class TaskExpressionKey {
+ public static TaskExpressionKey create(FileKey file, String expression) {
+ return new AutoValue_TaskExpressionKey(file, expression);
+ }
+
+ public abstract FileKey file();
+
+ public abstract String expression();
+}
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 93565f6..cb92805 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -70,6 +70,7 @@
protected final AllUsersNameProvider allUsers;
protected final CurrentUser user;
protected final TaskConfigFactory taskFactory;
+ protected final Preloader preloader;
protected final NodeList root = new NodeList();
protected final Provider<ChangeQueryBuilder> changeQueryBuilderProvider;
protected final Provider<ChangeQueryProcessor> changeQueryProcessorProvider;
@@ -84,13 +85,15 @@
CurrentUser user,
TaskConfigFactory taskFactory,
Provider<ChangeQueryBuilder> changeQueryBuilderProvider,
- Provider<ChangeQueryProcessor> changeQueryProcessorProvider) {
+ Provider<ChangeQueryProcessor> changeQueryProcessorProvider,
+ Preloader preloader) {
this.accountResolver = accountResolver;
this.allUsers = allUsers;
this.user = user != null ? user : anonymousUser;
this.taskFactory = taskFactory;
this.changeQueryProcessorProvider = changeQueryProcessorProvider;
this.changeQueryBuilderProvider = changeQueryBuilderProvider;
+ this.preloader = preloader;
}
public void masquerade(PatchSetArgument psa) {
@@ -112,7 +115,7 @@
protected Set<String> names = new HashSet<>();
protected void addSubNodes() throws ConfigInvalidException, IOException, OrmException {
- addPreloaded(taskFactory.getRootConfig().getPreloadedRootTasks());
+ addPreloaded(preloader.getRootTasks(taskFactory.getRootConfig()));
}
protected void addPreloaded(List<Task> defs) throws ConfigInvalidException, OrmException {
@@ -228,7 +231,8 @@
protected void addSubTasks() throws ConfigInvalidException, OrmException {
for (String expression : task.subTasks) {
try {
- Optional<Task> def = task.config.getPreloadedOptionalTask(new TaskExpression(expression));
+ Optional<Task> def =
+ preloader.getOptionalTask(task.config, new TaskExpression(task.file(), expression));
if (def.isPresent()) {
addPreloaded(def.get());
}
@@ -288,7 +292,7 @@
protected void addStaticTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory)
throws ConfigInvalidException, OrmException {
for (String name : namesFactory.names) {
- addPreloaded(preload(task.config.new Task(tasksFactory, name)));
+ addPreloaded(preloader.preload(task.config.new Task(tasksFactory, name)));
}
}
@@ -303,7 +307,8 @@
.entities();
for (ChangeData changeData : changeDataList) {
addPreloaded(
- preload(task.config.new Task(tasksFactory, changeData.getId().toString())),
+ preloader.preload(
+ task.config.new Task(tasksFactory, changeData.getId().toString())),
(parent, definition) ->
new Node(parent, definition) {
@Override
@@ -329,7 +334,7 @@
protected List<Task> getPreloadedTasks(FileKey file)
throws ConfigInvalidException, IOException {
- return taskFactory.getTaskConfig(file, task.isTrusted).getPreloadedTasks();
+ return preloader.getTasks(taskFactory.getTaskConfig(file, task.isTrusted));
}
@Override
@@ -360,8 +365,4 @@
}
return new Branch.NameKey(allUsers.get(), RefNames.refsUsers(acct.getId()));
}
-
- protected static Task preload(Task task) throws ConfigInvalidException {
- return task.config.preloader.preload(task);
- }
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java b/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java
index 13e471f..ac4ee88 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java
@@ -14,6 +14,8 @@
package com.googlesource.gerrit.plugins.task;
+import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Project;
import java.util.Iterator;
import java.util.NoSuchElementException;
import junit.framework.TestCase;
@@ -30,16 +32,17 @@
public static String SIMPLE = "simple";
public static String WORLD = "world";
public static String PEACE = "peace";
+ public static FileKey file = createFileKey("foo", "bar", "baz");
public void testBlank() {
- TaskExpression exp = new TaskExpression("");
+ TaskExpression exp = getTaskExpression("");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertNoSuchElementException(it);
}
public void testRequiredSingleName() {
- TaskExpression exp = new TaskExpression(SIMPLE);
+ TaskExpression exp = getTaskExpression(SIMPLE);
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), SIMPLE);
@@ -48,7 +51,7 @@
}
public void testOptionalSingleName() {
- TaskExpression exp = new TaskExpression(SIMPLE + "|");
+ TaskExpression exp = getTaskExpression(SIMPLE + "|");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), SIMPLE);
@@ -56,7 +59,7 @@
}
public void testRequiredTwoNames() {
- TaskExpression exp = new TaskExpression(WORLD + "|" + PEACE);
+ TaskExpression exp = getTaskExpression(WORLD + "|" + PEACE);
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), WORLD);
@@ -67,7 +70,7 @@
}
public void testOptionalTwoNames() {
- TaskExpression exp = new TaskExpression(WORLD + "|" + PEACE + "|");
+ TaskExpression exp = getTaskExpression(WORLD + "|" + PEACE + "|");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), WORLD);
@@ -77,14 +80,14 @@
}
public void testBlankSpaces() {
- TaskExpression exp = new TaskExpression(" ");
+ TaskExpression exp = getTaskExpression(" ");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertNoSuchElementException(it);
}
public void testRequiredSingleNameLeadingSpaces() {
- TaskExpression exp = new TaskExpression(" " + SIMPLE);
+ TaskExpression exp = getTaskExpression(" " + SIMPLE);
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), SIMPLE);
@@ -93,7 +96,7 @@
}
public void testRequiredSingleNameTrailingSpaces() {
- TaskExpression exp = new TaskExpression(SIMPLE + " ");
+ TaskExpression exp = getTaskExpression(SIMPLE + " ");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), SIMPLE);
@@ -102,7 +105,7 @@
}
public void testOptionalSingleNameLeadingSpaces() {
- TaskExpression exp = new TaskExpression(" " + SIMPLE + "|");
+ TaskExpression exp = getTaskExpression(" " + SIMPLE + "|");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), SIMPLE);
@@ -110,7 +113,7 @@
}
public void testOptionalSingleNameTrailingSpaces() {
- TaskExpression exp = new TaskExpression(SIMPLE + "| ");
+ TaskExpression exp = getTaskExpression(SIMPLE + "| ");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), SIMPLE);
@@ -118,7 +121,7 @@
}
public void testOptionalSingleNameMiddleSpaces() {
- TaskExpression exp = new TaskExpression(SIMPLE + " |");
+ TaskExpression exp = getTaskExpression(SIMPLE + " |");
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), SIMPLE);
@@ -126,7 +129,7 @@
}
public void testRequiredTwoNamesMiddleSpaces() {
- TaskExpression exp = new TaskExpression(WORLD + " | " + PEACE);
+ TaskExpression exp = getTaskExpression(WORLD + " | " + PEACE);
Iterator<String> it = exp.iterator();
assertTrue(it.hasNext());
assertEquals(it.next(), WORLD);
@@ -136,6 +139,30 @@
assertNoSuchElementException(it);
}
+ public void testDifferentKeyOnDifferentFile() {
+ TaskExpression exp = getTaskExpression(createFileKey("foo", "bar", "baz"), SIMPLE);
+ TaskExpression otherExp = getTaskExpression(createFileKey("foo", "bar", "other"), SIMPLE);
+ assertFalse(exp.key.equals(otherExp.key));
+ }
+
+ public void testDifferentKeyOnDifferentBranch() {
+ TaskExpression exp = getTaskExpression(createFileKey("foo", "bar", "baz"), SIMPLE);
+ TaskExpression otherExp = getTaskExpression(createFileKey("foo", "other", "baz"), SIMPLE);
+ assertFalse(exp.key.equals(otherExp.key));
+ }
+
+ public void testDifferentKeyOnDifferentProject() {
+ TaskExpression exp = getTaskExpression(createFileKey("foo", "bar", "baz"), SIMPLE);
+ TaskExpression otherExp = getTaskExpression(createFileKey("other", "bar", "baz"), SIMPLE);
+ assertFalse(exp.key.equals(otherExp.key));
+ }
+
+ public void testDifferentKeyOnDifferentExpression() {
+ TaskExpression exp = getTaskExpression(SIMPLE);
+ TaskExpression otherExp = getTaskExpression(PEACE);
+ assertFalse(exp.key.equals(otherExp.key));
+ }
+
protected static void assertNoSuchElementException(Iterator<String> it) {
try {
it.next();
@@ -144,4 +171,16 @@
assertTrue(true);
}
}
+
+ protected TaskExpression getTaskExpression(String expression) {
+ return getTaskExpression(file, expression);
+ }
+
+ protected TaskExpression getTaskExpression(FileKey file, String expression) {
+ return new TaskExpression(file, expression);
+ }
+
+ protected static FileKey createFileKey(String project, String branch, String file) {
+ return FileKey.create(new Branch.NameKey(new Project.NameKey(project), branch), file);
+ }
}
diff --git a/tools/bzl/plugin.bzl b/tools/bzl/plugin.bzl
index 89a1643..67536ef 100644
--- a/tools/bzl/plugin.bzl
+++ b/tools/bzl/plugin.bzl
@@ -2,7 +2,9 @@
"@com_googlesource_gerrit_bazlets//:gerrit_plugin.bzl",
_gerrit_plugin = "gerrit_plugin",
_plugin_deps = "PLUGIN_DEPS",
+ _plugin_test_deps = "PLUGIN_TEST_DEPS",
)
gerrit_plugin = _gerrit_plugin
PLUGIN_DEPS = _plugin_deps
+PLUGIN_TEST_DEPS = _plugin_test_deps
\ No newline at end of file
