Fix subtask-factory lookup to not fail when preloaded from another file
Before this change, plugin failed to lookup a subtasks-factory when
a task with this subtasks-factory was preloaded using a task
reference [1]. While preloading a task, all the subtasks are copied
to the current task. Since the subtasks-factory property was a String,
it does not maintain the information regarding the location of the
subtasks-factory. Thus, fix this issue by using ConfigSourcedValue as
the type for subtasks-factory property, which helps in tracking the
location of the subtasks-factory. Add tests for the same.
[1]
file: `All-Projects:refs/meta/config:task.config`
```
[root "root task"]
applicable = status:open
preload-task = //common.config^simple task with subtasks
```
file: `All-Projects:refs/meta/config:task/common.config`
```
[task "simple task with subtasks"]
applicable = is:open
subtasks-factory = simple static tasks-factory
[tasks-factory "simple static tasks-factory"]
names-factory = names-factory static list
pass = True
[names-factory "names-factory static list"]
type = static
name = my a task
```
Change-Id: Ie173ad1e29c511bb028a49d4c9b54108461bbe67
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
index 2f1c358..ca1ae2c 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
@@ -255,7 +255,7 @@
}
}
}
- } catch (IOException | RuntimeException e) {
+ } catch (IOException | RuntimeException | ConfigInvalidException e) {
return Optional.of(invalid()); // bad applicability query
}
return Optional.empty();
@@ -356,7 +356,8 @@
}
}
- protected List<TaskAttribute> getSubTasks() throws IOException, StorageException {
+ protected List<TaskAttribute> getSubTasks()
+ throws IOException, StorageException, ConfigInvalidException {
List<TaskAttribute> subTasks = new ArrayList<>();
for (Node subNode :
options.onlyApplicable ? node.getApplicableSubNodes() : node.getSubNodes()) {
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 138bcd6..aa25e7f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -66,7 +66,7 @@
public String readyHint;
public List<ConfigSourcedValue> subTasks;
public List<String> subTasksExternals;
- public List<String> subTasksFactories;
+ public List<ConfigSourcedValue> subTasksFactories;
public List<String> subTasksFiles;
public boolean isVisible;
@@ -91,7 +91,10 @@
.map(subTask -> ConfigSourcedValue.create(s.file(), subTask))
.collect(Collectors.toList());
subTasksExternals = getStringList(s, KEY_SUBTASKS_EXTERNAL);
- subTasksFactories = getStringList(s, KEY_SUBTASKS_FACTORY);
+ subTasksFactories =
+ getStringList(s, KEY_SUBTASKS_FACTORY).stream()
+ .map(subTask -> ConfigSourcedValue.create(s.file(), subTask))
+ .collect(Collectors.toList());
subTasksFiles = getStringList(s, KEY_SUBTASKS_FILE);
}
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 5d4d773..01afb89 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -94,6 +94,7 @@
protected final PredicateCache predicateCache;
protected final MatchCache matchCache;
protected final Preloader preloader;
+ protected final TaskConfigCache taskConfigCache;
protected final TaskExpression.Factory taskExpressionFactory;
protected final NodeList root = new NodeList();
protected final Provider<ChangeQueryBuilder> changeQueryBuilderProvider;
@@ -127,6 +128,7 @@
this.changeQueryBuilderProvider = changeQueryBuilderProvider;
this.predicateCache = predicateCache;
this.matchCache = new MatchCache(predicateCache);
+ this.taskConfigCache = taskConfigCache;
this.taskExpressionFactory = taskExpressionFactory;
this.preloader = preloaderFactory.create(taskConfigCache);
}
@@ -263,7 +265,7 @@
return String.valueOf(getChangeData().getId().get()) + TaskConfig.SEP + taskKey;
}
- public List<Node> getSubNodes() throws IOException, StorageException {
+ public List<Node> getSubNodes() throws IOException, StorageException, ConfigInvalidException {
if (cachedNodes != null) {
return refresh(cachedNodes);
}
@@ -286,12 +288,14 @@
return nodes;
}
- public List<Node> getApplicableSubNodes() throws IOException, StorageException {
+ public List<Node> getApplicableSubNodes()
+ throws IOException, StorageException, ConfigInvalidException {
return hasUnfilterableSubNodes ? getSubNodes() : new ApplicableNodeFilter().getSubNodes();
}
@Override
- protected List<Node> loadSubNodes() throws IOException, StorageException {
+ protected List<Node> loadSubNodes()
+ throws IOException, StorageException, ConfigInvalidException {
List<Task> cachedDefinitions = definitionsBySubSection.get(task.key().subSection());
if (cachedDefinitions != null) {
return new SubNodeFactory().createFromPreloaded(cachedDefinitions);
@@ -357,7 +361,7 @@
protected List<Node> nodes = new ArrayList<>();
protected SubNodeFactory factory = new SubNodeFactory();
- public List<Node> getSubNodes() throws IOException, StorageException {
+ public List<Node> getSubNodes() throws IOException, StorageException, ConfigInvalidException {
addSubTasks();
addSubTasksFactoryTasks();
addSubTasksFiles();
@@ -407,11 +411,18 @@
}
}
- protected void addSubTasksFactoryTasks() throws IOException, StorageException {
- for (String tasksFactoryName : task.subTasksFactories) {
- TasksFactory tasksFactory = task.config.getTasksFactory(tasksFactoryName);
+ protected void addSubTasksFactoryTasks()
+ throws IOException, StorageException, ConfigInvalidException {
+ for (ConfigSourcedValue configSourcedValue : task.subTasksFactories) {
+ TasksFactory tasksFactory =
+ taskConfigCache
+ .getTaskConfig(configSourcedValue.sourceFile())
+ .getTasksFactory(configSourcedValue.value());
if (tasksFactory != null) {
- NamesFactory namesFactory = task.config.getNamesFactory(tasksFactory.namesFactory);
+ NamesFactory namesFactory =
+ taskConfigCache
+ .getTaskConfig(configSourcedValue.sourceFile())
+ .getNamesFactory(tasksFactory.namesFactory);
if (namesFactory != null && namesFactory.type != null) {
namesFactory = properties.getNamesFactory(namesFactory);
switch (NamesFactoryType.getNamesFactoryType(namesFactory.type)) {
@@ -493,7 +504,7 @@
public ApplicableNodeFilter() throws StorageException {}
- public List<Node> getSubNodes() throws IOException, StorageException {
+ public List<Node> getSubNodes() throws IOException, StorageException, ConfigInvalidException {
if (nodesByBranch != null) {
List<Node> nodes = nodesByBranch.get(branch);
if (nodes != null) {
diff --git a/src/main/resources/Documentation/test/task_states.md b/src/main/resources/Documentation/test/task_states.md
index 880b955..4b296b2 100644
--- a/src/main/resources/Documentation/test/task_states.md
+++ b/src/main/resources/Documentation/test/task_states.md
@@ -2517,6 +2517,42 @@
]
}
+[root "Root Preload from all-projects sub-dir which has subtasks-factory in same file"]
+ preload-task = //dir/common.config^Sample relative task in sub dir with subtasks-factory from same file
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from all-projects sub-dir which has subtasks-factory in same file",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "my a task",
+ "status" : "PASS"
+ },
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "my b task",
+ "status" : "PASS"
+ },
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "my c task",
+ "status" : "PASS"
+ },
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "my d task",
+ "status" : "PASS"
+ }
+ ]
+}
+
[root "Root Preload from user ref"]
preload-task = @testuser/dir/relative.config^Relative Task
@@ -3238,6 +3274,31 @@
pass = is:open
subtask = Sample relative task in sub dir
+[task "Sample relative task in sub dir with subtasks-factory from same file"]
+ applicable = is:open
+ pass = is:open
+ set-my-factory-prop = simple static tasks-factory 2
+ subtasks-factory = simple static tasks-factory 1
+ subtasks-factory = ${my-factory-prop}
+
+[tasks-factory "simple static tasks-factory 1"]
+ names-factory = names-factory static list 1
+ pass = True
+
+[names-factory "names-factory static list 1"]
+ type = static
+ name = my a task
+ name = my b task
+ name = my c task
+
+[tasks-factory "simple static tasks-factory 2"]
+ names-factory = names-factory static list 2
+ pass = True
+
+[names-factory "names-factory static list 2"]
+ type = static
+ name = my d task
+
[task "Sample relative task in sub dir with subtask from different file"]
applicable = is:open
pass = is:open
