Add logic to handle blank subtask and blank name in names-factory
Before the change, NPE was thrown in case the subtask or name field in
names-factory is blank. This makes the parent task invalid which is not
ideal. Add logic to handle these cases so that only the concerned task
is marked as invalid.
Change-Id: I1f8faf06f7bc55b6619a1ef40bd40ee98bd3b51b
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 0c3136e..be7f120 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -300,8 +300,9 @@
}
protected List<String> getStringList(SubSectionKey s, String key) {
- return Collections.unmodifiableList(
- Arrays.asList(cfg.getStringList(s.section(), s.subSection(), key)));
+ List<String> stringList = Arrays.asList(cfg.getStringList(s.section(), s.subSection(), key));
+ stringList.replaceAll(str -> str == null ? "" : str);
+ return Collections.unmodifiableList(stringList);
}
protected SubSectionKey subSectionKey(String section, String subSection) {
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 ac65a0f..501e9db 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -58,6 +58,7 @@
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.util.StringUtils;
/**
* Add structure to access the task definitions from the config as a tree.
@@ -430,7 +431,11 @@
protected void addStaticTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory)
throws ConfigInvalidException, IOException, StorageException {
for (String name : namesFactory.names) {
- addPreloaded(preloader.preload(task.config.new Task(tasksFactory, name)));
+ if (StringUtils.isEmptyOrNull(name)) {
+ addInvalidNode();
+ } else {
+ addPreloaded(preloader.preload(task.config.new Task(tasksFactory, name)));
+ }
}
}
diff --git a/src/main/resources/Documentation/test/preview.md b/src/main/resources/Documentation/test/preview.md
index 6aff577..510f073 100644
--- a/src/main/resources/Documentation/test/preview.md
+++ b/src/main/resources/Documentation/test/preview.md
@@ -102,6 +102,18 @@
]
},
{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Subtask Blank",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
"applicable" : false,
"hasPass" : true,
"name" : "NA Bad PASS query",
@@ -164,6 +176,18 @@
{
"applicable" : true,
"hasPass" : false,
+ "name" : "task (names-factory name Blank)",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
+ "applicable" : true,
+ "hasPass" : false,
"name" : "task (names-factory duplicate)",
"status" : "WAITING",
"subTasks" : [
@@ -404,6 +428,18 @@
]
},
{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Subtask Blank",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
"applicable" : false,
"hasPass" : true,
"name" : "NA Bad PASS query",
@@ -466,6 +502,18 @@
{
"applicable" : true,
"hasPass" : false,
+ "name" : "task (names-factory name Blank)",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
+ "applicable" : true,
+ "hasPass" : false,
"name" : "task (names-factory duplicate)",
"status" : "WAITING",
"subTasks" : [
diff --git a/src/main/resources/Documentation/test/task_states.md b/src/main/resources/Documentation/test/task_states.md
index f48f528..f5d4cd8 100644
--- a/src/main/resources/Documentation/test/task_states.md
+++ b/src/main/resources/Documentation/test/task_states.md
@@ -2266,6 +2266,18 @@
]
},
{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Subtask Blank",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
"applicable" : false,
"hasPass" : true,
"name" : "NA Bad PASS query",
@@ -2328,6 +2340,18 @@
{
"applicable" : true,
"hasPass" : false,
+ "name" : "task (names-factory name Blank)",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
+ "applicable" : true,
+ "hasPass" : false,
"name" : "task (names-factory duplicate)",
"status" : "WAITING",
"subTasks" : [
@@ -2509,6 +2533,18 @@
]
},
{
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Subtask Blank",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
"applicable" : false,
"hasPass" : true,
"name" : "NA Bad PASS query",
@@ -2571,6 +2607,18 @@
{
"applicable" : true,
"hasPass" : false,
+ "name" : "task (names-factory name Blank)",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "name" : "UNKNOWN",
+ "status" : "INVALID"
+ }
+ ]
+ },
+ {
+ "applicable" : true,
+ "hasPass" : false,
"name" : "task (names-factory duplicate)",
"status" : "WAITING",
"subTasks" : [
@@ -2669,6 +2717,10 @@
[task "Subtask Optional"]
subtask = MISSING | MISSING
+[task "Subtask Blank"]
+ pass = True
+ subtask =
+
[task "NA Bad PASS query"]
applicable = NOT is:open # Assumes test query is "is:open"
fail = True
@@ -2699,6 +2751,9 @@
[task "task (names-factory type INVALID)"]
subtasks-factory = tasks-factory (names-factory type INVALID)
+[task "task (names-factory name Blank)"]
+ subtasks-factory = tasks-factory (names-factory name Blank)
+
[task "task (names-factory duplicate)"]
subtasks-factory = tasks-factory (names-factory duplicate)
@@ -2718,6 +2773,10 @@
[tasks-factory "tasks-factory (names-factory type INVALID)"]
names-factory = name-factory (type INVALID)
+[tasks-factory "tasks-factory (names-factory name Blank)"]
+ names-factory = names-factory (name Blank)
+ fail = True
+
[tasks-factory "tasks-factory (names-factory duplicate)"]
names-factory = names-factory duplicate
fail = True
@@ -2744,6 +2803,10 @@
name = invalid type test
type = invalid
+[names-factory "names-factory (name Blank)"]
+ name =
+ type = static
+
[names-factory "names-factory duplicate"]
name = duplicate
name = duplicate
