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