Fix subtask lookup to not fail when preloaded from another file
Before this change, plugin failed to lookup a subtask when a task
with this subtask was preloaded using a task reference [1]. While
preloading a task, all the subtasks are copied to the current task.
Since the subtask property was a String, it does not maintain the
information regarding the location of the subtask. Thus, fix this
issue by using ConfigSourcedValue as the type for subtask property,
which helps in tracking the location. 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 subtask
```
file: `All-Projects:refs/meta/config:task/common.config`
```
[task "simple task with subtask"]
applicable = is:open
subtask = passing task
[task "passing task"]
applicable = is:open
pass = True
```
Change-Id: Ieff1da7a47b63514fb8d0437340e096e03ad54bd
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/ConfigSourcedValue.java b/src/main/java/com/googlesource/gerrit/plugins/task/ConfigSourcedValue.java
new file mode 100644
index 0000000..d5a8ad1
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/ConfigSourcedValue.java
@@ -0,0 +1,32 @@
+// Copyright (C) 2024 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 com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class ConfigSourcedValue {
+ public static ConfigSourcedValue create(FileKey sourceFile, String value) {
+ return new AutoValue_ConfigSourcedValue(sourceFile, value);
+ }
+
+ public static Class<? extends ConfigSourcedValue> getClassType() {
+ return AutoValue_ConfigSourcedValue.class;
+ }
+
+ public abstract FileKey sourceFile();
+
+ public abstract String value();
+}
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 f5f0bd4..138bcd6 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -27,6 +27,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.stream.Collectors;
/** Task Configuration file living in git */
public class TaskConfig extends AbstractVersionedMetaData {
@@ -63,7 +64,7 @@
public String preloadTask;
public Map<String, String> properties;
public String readyHint;
- public List<String> subTasks;
+ public List<ConfigSourcedValue> subTasks;
public List<String> subTasksExternals;
public List<String> subTasksFactories;
public List<String> subTasksFiles;
@@ -85,7 +86,10 @@
preloadTask = getString(s, KEY_PRELOAD_TASK, null);
properties = getProperties(s, KEY_PROPERTIES_PREFIX);
readyHint = getString(s, KEY_READY_HINT, null);
- subTasks = getStringList(s, KEY_SUBTASK);
+ subTasks =
+ getStringList(s, KEY_SUBTASK).stream()
+ .map(subTask -> ConfigSourcedValue.create(s.file(), subTask))
+ .collect(Collectors.toList());
subTasksExternals = getStringList(s, KEY_SUBTASKS_EXTERNAL);
subTasksFactories = getStringList(s, KEY_SUBTASKS_FACTORY);
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 44fcadc..73bd127 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -365,10 +365,12 @@
}
protected void addSubTasks() throws IOException, StorageException {
- for (String expression : task.subTasks) {
+ for (ConfigSourcedValue configSourcedValue : task.subTasks) {
try {
Optional<Task> def =
- preloader.getOptionalTask(taskExpressionFactory.create(task.file(), expression));
+ preloader.getOptionalTask(
+ taskExpressionFactory.create(
+ configSourcedValue.sourceFile(), configSourcedValue.value()));
if (def.isPresent()) {
addPreloaded(def.get());
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/properties/AbstractExpander.java b/src/main/java/com/googlesource/gerrit/plugins/task/properties/AbstractExpander.java
index c970cec..ce781d9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/properties/AbstractExpander.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/properties/AbstractExpander.java
@@ -17,7 +17,9 @@
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;
@@ -42,6 +44,16 @@
public abstract class AbstractExpander {
protected Consumer<Matcher.Statistics> statisticsConsumer;
+ protected final Map<Class<?>, Function<?, ?>> expanderByClass = new HashMap<>();
+
+ protected AbstractExpander() {
+ registerClassExpander(String.class, this::expandText);
+ }
+
+ public <T> void registerClassExpander(Class<? extends T> classType, Function<T, T> expander) {
+ expanderByClass.put(classType, expander);
+ }
+
public void setStatisticsConsumer(Consumer<Matcher.Statistics> statisticsConsumer) {
this.statisticsConsumer = statisticsConsumer;
}
@@ -101,9 +113,7 @@
field.set(cow.getForWrite(), expanded);
}
} else if (o instanceof List) {
- @SuppressWarnings("unchecked")
- List<String> forceCheck = List.class.cast(o);
- List<String> expanded = expand(forceCheck);
+ List<?> expanded = expand((List<?>) o);
if (expanded != o) {
field.set(cow.getForWrite(), expanded);
}
@@ -118,12 +128,12 @@
* Returns expanded unmodifiable List if property found. Returns same object if no expansions
* occurred.
*/
- public List<String> expand(List<String> list) {
+ public <T> List<T> expand(List<T> list) {
if (list != null) {
boolean hasProperty = false;
- List<String> expandedList = new ArrayList<>(list.size());
- for (String value : list) {
- String expanded = expandText(value);
+ List<T> expandedList = new ArrayList<>(list.size());
+ for (T value : list) {
+ T expanded = expand(value);
hasProperty = hasProperty || value != expanded;
expandedList.add(expanded);
}
@@ -133,6 +143,20 @@
}
/**
+ * Expand all properties (${property_name} -> property_value) in the given generic value. Returns
+ * same object if no expansions occurred.
+ */
+ public <T> T expand(T value) {
+ if (value == null) {
+ return null;
+ }
+ @SuppressWarnings("unchecked")
+ Function<T, T> expander =
+ (Function<T, T>) expanderByClass.getOrDefault(value.getClass(), Function.identity());
+ return expander.apply(value);
+ }
+
+ /**
* Expand all properties (${property_name} -> property_value) in the given text. Returns same
* object if no expansions occurred.
*/
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java b/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java
index 591773a..6177f2b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/properties/Properties.java
@@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.query.change.ChangeData;
+import com.googlesource.gerrit.plugins.task.ConfigSourcedValue;
import com.googlesource.gerrit.plugins.task.TaskConfig;
import com.googlesource.gerrit.plugins.task.TaskConfig.NamesFactory;
import com.googlesource.gerrit.plugins.task.TaskConfig.Task;
@@ -92,6 +93,8 @@
.setNanosConsumer(l -> Statistics.setNanoseconds(statistics, l))) {
loader = new Loader(origTask, changeData, getParentMapper());
expander = new Expander(n -> loader.load(n));
+ expander.registerClassExpander(
+ ConfigSourcedValue.getClassType(), getConfigSourcedValueExpander(expander));
expander.setStatisticsConsumer(matcherStatisticsConsumer);
if (isTaskRefreshRequired || init) {
expander.expand(task, TaskConfig.KEY_APPLICABLE);
@@ -116,6 +119,18 @@
return task.getForRead();
}
+ protected Function<ConfigSourcedValue, ConfigSourcedValue> getConfigSourcedValueExpander(
+ Expander expander) {
+ return t -> {
+ String toExpand = t.value();
+ String expanded = expander.expandText(toExpand);
+ if (toExpand != expanded) {
+ return ConfigSourcedValue.create(t.sourceFile(), expanded);
+ }
+ return t;
+ };
+ }
+
public void setStatisticsConsumer(Consumer<Statistics> statisticsConsumer) {
if (statisticsConsumer != null) {
this.statisticsConsumer = statisticsConsumer;
diff --git a/src/main/resources/Documentation/test/task_states.md b/src/main/resources/Documentation/test/task_states.md
index 199306f..880b955 100644
--- a/src/main/resources/Documentation/test/task_states.md
+++ b/src/main/resources/Documentation/test/task_states.md
@@ -2471,6 +2471,162 @@
"status" : "PASS"
}
+[root "Root Preload from all-projects sub-dir"]
+ preload-task = //dir/common.config^Sample relative task in sub dir
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from all-projects sub-dir",
+ "status" : "PASS"
+}
+
+[root "Root Preload from all-projects sub-dir which has subtask in same file"]
+ preload-task = //dir/common.config^Sample relative task in sub dir with subtask from same file
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from all-projects sub-dir which has subtask in same file",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Sample relative task in sub dir",
+ "status" : "PASS"
+ }
+ ]
+}
+
+[root "Root Preload from all-projects sub-dir which has subtask in different file"]
+ preload-task = //dir/common.config^Sample relative task in sub dir with subtask from different file
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from all-projects sub-dir which has subtask in different file",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Passing task",
+ "status" : "PASS"
+ }
+ ]
+}
+
+[root "Root Preload from user ref"]
+ preload-task = @testuser/dir/relative.config^Relative Task
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from user ref",
+ "status" : "PASS"
+}
+
+[root "Root Preload from user ref which has subtask in same file"]
+ preload-task = @testuser/dir/relative.config^Relative Task with subtask
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from user ref which has subtask in same file",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Passing task",
+ "status" : "PASS"
+ }
+ ]
+}
+
+[root "Root Preload from user ref which has subtask in different file"]
+ preload-task = @testuser/dir/relative.config^Import All-Projects root task
+
+{
+ "applicable" : true,
+ "hasPass" : false,
+ "name" : "Root Preload from user ref which has subtask in different file",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Subtask PASS",
+ "status" : "PASS"
+ }
+ ]
+}
+
+[root "Root Preload from group ref"]
+ preload-task = %{non_secret_group_name_without_space}^task in group root config file 1
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from group ref",
+ "status" : "PASS"
+}
+
+[root "Root Preload from group ref which has subtask in same file"]
+ preload-task = %{non_secret_group_name_without_space}^task in group root with subtask
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from group ref which has subtask in same file",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Passing task",
+ "status" : "PASS"
+ }
+ ]
+}
+
+[root "Root Preload from group ref which has subtask in different file"]
+ preload-task = %{non_secret_group_name_without_space}^task in group root with subtask from all-projects
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from group ref which has subtask in different file",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Subtask PASS",
+ "status" : "PASS"
+ }
+ ]
+}
+
+[root "Root Preload from group ref which has subtask in different group ref"]
+ preload-task = %{non_secret_group_name_without_space}^task in group root with subtask from another group
+
+{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Root Preload from group ref which has subtask in different group ref",
+ "status" : "PASS",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "task in group root config file 3",
+ "status" : "PASS"
+ }
+ ]
+}
+
[root "Root INVALID Preload"]
preload-task = missing
@@ -3065,6 +3221,10 @@
```
[task "Root Import task from subdir using relative syntax"]
subtask = dir/common.config^Sample relative task in sub dir
+
+[task "Passing task"]
+ applicable = is:open
+ pass = True
```
file: `All-Projects:refs/meta/config:task/dir/common.config`
@@ -3073,6 +3233,16 @@
applicable = is:open
pass = is:open
+[task "Sample relative task in sub dir with subtask from same file"]
+ applicable = is:open
+ pass = is:open
+ subtask = Sample relative task in sub dir
+
+[task "Sample relative task in sub dir with subtask from different file"]
+ applicable = is:open
+ pass = is:open
+ subtask = //relative.config^Passing task
+
[task "Root Import task from root task.config"]
applicable = is:open
subtask = ^Subtask PASS
@@ -3294,6 +3464,15 @@
applicable = is:open
pass = is:open
+[task "Relative Task with subtask"]
+ applicable = is:open
+ pass = is:open
+ subtask = Passing task
+
+[task "Passing task"]
+ applicable = is:open
+ pass = True
+
[task "Import All-Projects root task"]
applicable = is:open
subtask = //^Subtask PASS
@@ -3345,6 +3524,25 @@
applicable = is:open
pass = is:open
+[task "task in group root with subtask"]
+ applicable = is:open
+ pass = is:open
+ subtask = Passing task
+
+[task "Passing task"]
+ applicable = is:open
+ pass = True
+
+[task "task in group root with subtask from all-projects"]
+ applicable = is:open
+ pass = is:open
+ subtask = //^Subtask PASS
+
+[task "task in group root with subtask from another group"]
+ applicable = is:open
+ pass = is:open
+ subtask = %{non_secret_group_name_with_space}^task in group root config file 3
+
[task "task in group root config file 2"]
applicable = is:open
pass = is:open
