Change TaskExpressions to iterate over TaskKeys This allows TaskExpression to expand parsing tasks from other file references in future. A per request TaskConfig cache is introduced to avoid loading the same file again in the flow. This change removes the dependency of TaskTree on TaskConfigFactory. Change-Id: Id530af601e87451534030cc43d945fafc5826e87
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 c48ed6e..b240c3b 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
@@ -14,7 +14,10 @@ package com.googlesource.gerrit.plugins.task; +import com.google.inject.Inject; import com.googlesource.gerrit.plugins.task.TaskConfig.Task; +import com.googlesource.gerrit.plugins.task.cli.PatchSetArgument; +import java.io.IOException; import java.lang.reflect.Field; import java.util.ArrayList; import java.util.HashMap; @@ -26,17 +29,23 @@ /** Use to pre-load a task definition with values from its preload-task definition. */ public class Preloader { + protected final TaskConfigFactory taskConfigFactory; protected final Map<TaskExpressionKey, Optional<Task>> optionalTaskByExpression = new HashMap<>(); - public List<Task> getRootTasks(TaskConfig cfg) { - return getTasks(cfg, TaskConfig.SECTION_ROOT); + @Inject + public Preloader(TaskConfigFactory taskConfigFactory) { + this.taskConfigFactory = taskConfigFactory; } - public List<Task> getTasks(TaskConfig cfg) { - return getTasks(cfg, TaskConfig.SECTION_TASK); + public List<Task> getRootTasks() throws IOException, ConfigInvalidException { + return getTasks(taskConfigFactory.getRootConfig(), TaskConfig.SECTION_ROOT); } - protected List<Task> getTasks(TaskConfig cfg, String type) { + public List<Task> getTasks(FileKey file) throws IOException, ConfigInvalidException { + return getTasks(taskConfigFactory.getTaskConfig(file), TaskConfig.SECTION_TASK); + } + + protected List<Task> getTasks(TaskConfig cfg, String type) throws IOException { List<Task> preloaded = new ArrayList<>(); for (Task task : cfg.getTasks(type)) { try { @@ -55,27 +64,27 @@ * @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 { + public Optional<Task> getOptionalTask(TaskExpression expression) + throws ConfigInvalidException, IOException { Optional<Task> task = optionalTaskByExpression.get(expression.key); if (task == null) { - task = preloadOptionalTask(cfg, expression); + task = preloadOptionalTask(expression); optionalTaskByExpression.put(expression.key, task); } return task; } - protected Optional<Task> preloadOptionalTask(TaskConfig cfg, TaskExpression expression) - throws ConfigInvalidException { - Optional<Task> definition = loadOptionalTask(cfg, expression); + protected Optional<Task> preloadOptionalTask(TaskExpression expression) + throws ConfigInvalidException, IOException { + Optional<Task> definition = loadOptionalTask(expression); return definition.isPresent() ? Optional.of(preload(definition.get())) : definition; } - public Task preload(Task definition) throws ConfigInvalidException { + public Task preload(Task definition) throws ConfigInvalidException, IOException { String expression = definition.preloadTask; if (expression != null) { Optional<Task> preloadFrom = - getOptionalTask(definition.config, new TaskExpression(definition.file(), expression)); + getOptionalTask(new TaskExpression(definition.file(), expression)); if (preloadFrom.isPresent()) { return preloadFrom(definition, preloadFrom.get()); } @@ -83,11 +92,11 @@ return definition; } - protected Optional<Task> loadOptionalTask(TaskConfig cfg, TaskExpression expression) - throws ConfigInvalidException { + protected Optional<Task> loadOptionalTask(TaskExpression expression) + throws ConfigInvalidException, IOException { try { - for (String name : expression) { - Optional<Task> task = cfg.getOptionalTask(name); + for (TaskKey key : expression) { + Optional<Task> task = getOptionalTask(key); if (task.isPresent()) { return task; } @@ -117,6 +126,14 @@ return preloadTo; } + protected Optional<Task> getOptionalTask(TaskKey key) throws IOException, ConfigInvalidException { + return taskConfigFactory.getTaskConfig(key.subSection().file()).getOptionalTask(key.task()); + } + + public void masquerade(PatchSetArgument psa) { + taskConfigFactory.masquerade(psa); + } + protected static <S, K, V> void preloadField( Field field, Task definition, Task preloadFrom, Task preloadTo) throws IllegalArgumentException, IllegalAccessException {
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 08dcf5c..9236db7 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -192,7 +192,7 @@ protected static final String SECTION_EXTERNAL = "external"; protected static final String SECTION_NAMES_FACTORY = "names-factory"; protected static final String SECTION_ROOT = "root"; - protected static final String SECTION_TASK = "task"; + protected static final String SECTION_TASK = TaskKey.CONFIG_SECTION; protected static final String SECTION_TASKS_FACTORY = "tasks-factory"; protected static final String KEY_APPLICABLE = "applicable"; protected static final String KEY_CHANGES = "changes";
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfigFactory.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfigFactory.java index feedd6a..34f9b07 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfigFactory.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfigFactory.java
@@ -45,6 +45,7 @@ protected final AllProjectsName allProjects; protected final Map<Branch.NameKey, PatchSetArgument> psaMasquerades = new HashMap<>(); + protected final Map<FileKey, TaskConfig> taskCfgByFile = new HashMap<>(); @Inject protected TaskConfigFactory( @@ -70,7 +71,16 @@ return new Branch.NameKey(allProjects, "refs/meta/config"); } - public TaskConfig getTaskConfig(FileKey file) throws ConfigInvalidException, IOException { + public TaskConfig getTaskConfig(FileKey key) throws ConfigInvalidException, IOException { + TaskConfig cfg = taskCfgByFile.get(key); + if (cfg == null) { + cfg = loadTaskConfig(key); + taskCfgByFile.put(key, cfg); + } + return cfg; + } + + private TaskConfig loadTaskConfig(FileKey file) throws ConfigInvalidException, IOException { Branch.NameKey branch = file.branch(); PatchSetArgument psa = psaMasquerades.get(branch); boolean visible = true; // invisible psas are filtered out by commandline
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 b3fc530..5a61d29 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java
@@ -35,7 +35,7 @@ * <li><code> "foo | bar |" -> ("foo", "bar") optional</code> * </ul> */ -public class TaskExpression implements Iterable<String> { +public class TaskExpression implements Iterable<TaskKey> { protected static final Pattern EXPRESSION_PATTERN = Pattern.compile("([^ |]+[^|]*)(\\|)?"); protected final TaskExpressionKey key; @@ -44,8 +44,8 @@ } @Override - public Iterator<String> iterator() { - return new Iterator<String>() { + public Iterator<TaskKey> iterator() { + return new Iterator<TaskKey>() { Matcher m = EXPRESSION_PATTERN.matcher(key.expression()); Boolean hasNext; boolean optional; @@ -65,14 +65,14 @@ } @Override - public String next() { + public TaskKey next() { // Can't use @SuppressWarnings("ReturnValueIgnored") on method call boolean ignored = 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(); + return TaskKey.create(key.file(), m.group(1).trim()); } }; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskKey.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskKey.java index 0af75c8..481cbd5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskKey.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskKey.java
@@ -20,11 +20,18 @@ /** An immutable reference to a task in task config file. */ @AutoValue public abstract class TaskKey { + protected static final String CONFIG_SECTION = "task"; + /** Creates a TaskKey with task name as the name of sub section. */ public static TaskKey create(SubSectionKey section) { return create(section, section.subSection()); } + /** Creates a TaskKey with given FileKey and task name and sub section's name as 'task'. */ + public static TaskKey create(FileKey file, String task) { + return create(SubSectionKey.create(file, CONFIG_SECTION, task)); + } + /** Creates a TaskKey from a sub section and task name, generally used by TasksFactory. */ public static TaskKey create(SubSectionKey section, String task) { return new AutoValue_TaskKey(section, task);
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 3657974..2ba88ea 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -69,7 +69,6 @@ protected final AccountResolver accountResolver; 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; @@ -83,21 +82,19 @@ AllUsersNameProvider allUsers, AnonymousUser anonymousUser, CurrentUser user, - TaskConfigFactory taskFactory, Provider<ChangeQueryBuilder> changeQueryBuilderProvider, 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) { - taskFactory.masquerade(psa); + preloader.masquerade(psa); } public List<Node> getRootNodes(ChangeData changeData) @@ -117,7 +114,7 @@ protected Set<String> names = new HashSet<>(); protected void addSubNodes() throws ConfigInvalidException, IOException, OrmException { - addPreloaded(preloader.getRootTasks(taskFactory.getRootConfig())); + addPreloaded(preloader.getRootTasks()); } protected void addPreloaded(List<Task> defs) throws ConfigInvalidException, OrmException { @@ -237,18 +234,18 @@ } @Override - protected void addSubNodes() throws ConfigInvalidException, OrmException { + protected void addSubNodes() throws ConfigInvalidException, IOException, OrmException { addSubTasks(); addSubTasksFactoryTasks(); addSubTasksFiles(); addSubTasksExternals(); } - protected void addSubTasks() throws ConfigInvalidException, OrmException { + protected void addSubTasks() throws ConfigInvalidException, IOException, OrmException { for (String expression : task.subTasks) { try { Optional<Task> def = - preloader.getOptionalTask(task.config, new TaskExpression(task.file(), expression)); + preloader.getOptionalTask(new TaskExpression(task.file(), expression)); if (def.isPresent()) { addPreloaded(def.get()); } @@ -262,7 +259,7 @@ for (String file : task.subTasksFiles) { try { addPreloaded( - getPreloadedTasks(FileKey.create(task.key().branch(), resolveTaskFileName(file)))); + preloader.getTasks(FileKey.create(task.key().branch(), resolveTaskFileName(file)))); } catch (ConfigInvalidException | IOException e) { addInvalidNode(); } @@ -284,7 +281,8 @@ } } - protected void addSubTasksFactoryTasks() throws ConfigInvalidException, OrmException { + protected void addSubTasksFactoryTasks() + throws ConfigInvalidException, IOException, OrmException { for (String tasksFactoryName : task.subTasksFactories) { TasksFactory tasksFactory = task.config.getTasksFactory(tasksFactoryName); if (tasksFactory != null) { @@ -306,14 +304,14 @@ } protected void addStaticTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) - throws ConfigInvalidException, OrmException { + throws ConfigInvalidException, IOException, OrmException { for (String name : namesFactory.names) { addPreloaded(preloader.preload(task.config.new Task(tasksFactory, name))); } } protected void addChangeTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) - throws ConfigInvalidException, OrmException { + throws ConfigInvalidException, IOException, OrmException { try { if (namesFactory.changes != null) { List<ChangeData> changeDataList = @@ -349,15 +347,10 @@ protected List<Task> getPreloadedTasks(External external) throws ConfigInvalidException, IOException, OrmException { - return getPreloadedTasks( + return preloader.getTasks( FileKey.create(resolveUserBranch(external.user), resolveTaskFileName(external.file))); } - protected List<Task> getPreloadedTasks(FileKey file) - throws ConfigInvalidException, IOException { - return preloader.getTasks(taskFactory.getTaskConfig(file)); - } - @Override protected Properties getProperties() { return properties;
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 ac4ee88..1f7f529 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/task/TaskExpressionTest.java
@@ -34,107 +34,111 @@ public static String PEACE = "peace"; public static FileKey file = createFileKey("foo", "bar", "baz"); + public static TaskKey SIMPLE_TASK = TaskKey.create(file, SIMPLE); + public static TaskKey WORLD_TASK = TaskKey.create(file, WORLD); + public static TaskKey PEACE_TASK = TaskKey.create(file, PEACE); + public void testBlank() { TaskExpression exp = getTaskExpression(""); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); assertNoSuchElementException(it); } public void testRequiredSingleName() { TaskExpression exp = getTaskExpression(SIMPLE); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), SIMPLE); + assertEquals(it.next(), SIMPLE_TASK); assertTrue(it.hasNext()); assertNoSuchElementException(it); } public void testOptionalSingleName() { TaskExpression exp = getTaskExpression(SIMPLE + "|"); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), SIMPLE); + assertEquals(it.next(), SIMPLE_TASK); assertFalse(it.hasNext()); } public void testRequiredTwoNames() { TaskExpression exp = getTaskExpression(WORLD + "|" + PEACE); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), WORLD); + assertEquals(it.next(), WORLD_TASK); assertTrue(it.hasNext()); - assertEquals(it.next(), PEACE); + assertEquals(it.next(), PEACE_TASK); assertTrue(it.hasNext()); assertNoSuchElementException(it); } public void testOptionalTwoNames() { TaskExpression exp = getTaskExpression(WORLD + "|" + PEACE + "|"); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), WORLD); + assertEquals(it.next(), WORLD_TASK); assertTrue(it.hasNext()); - assertEquals(it.next(), PEACE); + assertEquals(it.next(), PEACE_TASK); assertFalse(it.hasNext()); } public void testBlankSpaces() { TaskExpression exp = getTaskExpression(" "); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); assertNoSuchElementException(it); } public void testRequiredSingleNameLeadingSpaces() { TaskExpression exp = getTaskExpression(" " + SIMPLE); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), SIMPLE); + assertEquals(it.next(), SIMPLE_TASK); assertTrue(it.hasNext()); assertNoSuchElementException(it); } public void testRequiredSingleNameTrailingSpaces() { TaskExpression exp = getTaskExpression(SIMPLE + " "); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), SIMPLE); + assertEquals(it.next(), SIMPLE_TASK); assertTrue(it.hasNext()); assertNoSuchElementException(it); } public void testOptionalSingleNameLeadingSpaces() { TaskExpression exp = getTaskExpression(" " + SIMPLE + "|"); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), SIMPLE); + assertEquals(it.next(), SIMPLE_TASK); assertFalse(it.hasNext()); } public void testOptionalSingleNameTrailingSpaces() { TaskExpression exp = getTaskExpression(SIMPLE + "| "); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), SIMPLE); + assertEquals(it.next(), SIMPLE_TASK); assertFalse(it.hasNext()); } public void testOptionalSingleNameMiddleSpaces() { TaskExpression exp = getTaskExpression(SIMPLE + " |"); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), SIMPLE); + assertEquals(it.next(), SIMPLE_TASK); assertFalse(it.hasNext()); } public void testRequiredTwoNamesMiddleSpaces() { TaskExpression exp = getTaskExpression(WORLD + " | " + PEACE); - Iterator<String> it = exp.iterator(); + Iterator<TaskKey> it = exp.iterator(); assertTrue(it.hasNext()); - assertEquals(it.next(), WORLD); + assertEquals(it.next(), WORLD_TASK); assertTrue(it.hasNext()); - assertEquals(it.next(), PEACE); + assertEquals(it.next(), PEACE_TASK); assertTrue(it.hasNext()); assertNoSuchElementException(it); } @@ -163,7 +167,7 @@ assertFalse(exp.key.equals(otherExp.key)); } - protected static void assertNoSuchElementException(Iterator<String> it) { + protected static void assertNoSuchElementException(Iterator<TaskKey> it) { try { it.next(); assertTrue(false);