Do not modify definition during preload
If a preload is detected, create a new Task and copy in the new
preloaded fields from either the original definition or the preload
Task. Not modifying the original task paves the way to allow more
caching (potentially even of the preloads themselves), and the copies
are done as minimally as possible to reduce their impact.
Change-Id: I104b5d3bdd2d53cc863c8aef2e90c70116ebcf4d
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 44c3773..c5a5b0f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
@@ -25,58 +25,57 @@
/** Use to pre-load a task definition with values from its preload-task definition. */
public class Preloader {
- public static void preload(Task definition) throws ConfigInvalidException {
+ public static Task preload(Task definition) throws ConfigInvalidException {
String expression = definition.preloadTask;
if (expression != null) {
- Optional<Task> task = definition.config.getOptionalTaskForExpression(expression);
- if (task.isPresent()) {
- preload(task.get());
- preloadFrom(definition, task.get());
+ Optional<Task> preloadFrom = definition.config.getOptionalTaskForExpression(expression);
+ if (preloadFrom.isPresent()) {
+ return preloadFrom(definition, preload(preloadFrom.get()));
}
}
+ return definition;
}
- protected static void preloadFrom(Task definition, Task preloadFrom) {
+ protected static Task preloadFrom(Task definition, Task preloadFrom) {
+ Task preloadTo = definition.config.new Task(definition.subSection);
for (Field field : definition.getClass().getFields()) {
String name = field.getName();
- if ("isVisible".equals(name) || "isTrusted".equals(name) || "config".equals(name)) {
+ if ("config".equals(name) || "subSection".equals(name)) {
continue;
}
try {
field.setAccessible(true);
- preloadField(field.getType(), field, definition, preloadFrom);
+ preloadField(field, definition, preloadFrom, preloadTo);
} catch (IllegalAccessException | IllegalArgumentException e) {
throw new RuntimeException();
}
}
+ return preloadTo;
}
- protected static <T, S, K, V> void preloadField(
- Class<T> clz, Field field, Task definition, Task preloadFrom)
+ protected static <S, K, V> void preloadField(
+ Field field, Task definition, Task preloadFrom, Task preloadTo)
throws IllegalArgumentException, IllegalAccessException {
- T pre = getField(clz, field, preloadFrom);
- if (pre != null) {
- T val = getField(clz, field, definition);
- if (val == null) {
- field.set(definition, pre);
- } else if (val instanceof List) {
- List<?> valList = List.class.cast(val);
- List<?> preList = List.class.cast(pre);
- field.set(definition, preloadListFrom(castUnchecked(valList), castUnchecked(preList)));
- } else if (val instanceof Map) {
- Map<?, ?> valMap = Map.class.cast(val);
- Map<?, ?> preMap = Map.class.cast(pre);
- field.set(definition, preloadMapFrom(castUnchecked(valMap), castUnchecked(preMap)));
- } // nothing to do for overridden preloaded scalars
+ Object pre = field.get(preloadFrom);
+ Object val = field.get(definition);
+ if (val == null) {
+ field.set(preloadTo, pre);
+ } else if (pre == null) {
+ field.set(preloadTo, val);
+ } else if (val instanceof List) {
+ List<?> valList = List.class.cast(val);
+ List<?> preList = List.class.cast(pre);
+ field.set(preloadTo, preloadListFrom(castUnchecked(valList), castUnchecked(preList)));
+ } else if (val instanceof Map) {
+ Map<?, ?> valMap = Map.class.cast(val);
+ Map<?, ?> preMap = Map.class.cast(pre);
+ field.set(preloadTo, preloadMapFrom(castUnchecked(valMap), castUnchecked(preMap)));
+ } else {
+ field.set(preloadTo, val);
}
}
- protected static <T> T getField(Class<T> clz, Field field, Object obj)
- throws IllegalArgumentException, IllegalAccessException {
- return clz.cast(field.get(obj));
- }
-
@SuppressWarnings("unchecked")
protected static <S> List<S> castUnchecked(List<?> list) {
List<S> forceCheck = (List<S>) list;
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 4e78749..32496cf 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -96,9 +96,13 @@
}
protected TaskBase(TaskBase base) {
- super(base.subSection);
+ this(base.subSection);
Copier.deepCopyDeclaredFields(TaskBase.class, base, this, false);
}
+
+ protected TaskBase(SubSection s) {
+ super(s);
+ }
}
public class Task extends TaskBase {
@@ -119,6 +123,10 @@
this.name = name;
}
+ public Task(SubSection s) {
+ super(s);
+ }
+
protected Map<String, String> getAllProperties() {
Map<String, String> all = new HashMap<>(properties);
all.putAll(exported);
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 9bf9b73..fd37c9a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -158,8 +158,7 @@
public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException {
this.parent = parent;
- Preloader.preload(task);
- properties = new Properties(task, parent.getProperties());
+ properties = new Properties(Preloader.preload(task), parent.getProperties());
this.task = properties.getTask(getChangeData());
this.path.addAll(parent.path);
this.path.add(key());
