commit 0d65e85e93ac4181fc19d816d600dd4da7c18393
author Martin Fick <martin.fick@linaro.org> Wed Jul 20 14:59:05 2022 -0600
committer Martin Fick <martin.fick@linaro.org> Wed Sep 07 15:39:08 2022 -0600
tree 369f467c8f5acbf70e72df33d677574b1b052f39
parent 2e42e94bd769b5ffbaf4dff5cb307dfe4c25616f

Use a TaskTree.Node.Invalid instead of nulls

Previously nulls were used in the TaskTree.NodeList subnodes list to
indicate a special invalid use case, use a new TaskTree.Node.Invalid
class to indicate this. This change helps clarify the explicit invalid
case, but also makes unexpected NPEs during development less likely.
Since it is more obvious now when Nodes are invalid, go ahead and mark
invalid Nodes as cacheable for Branch filtering since they are
immutable.

Change-Id: I1551072ed2824a2d596060259d3da98b92abe04e

src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java

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 690b035..ae40471 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
@@ -96,7 +96,7 @@
     TaskPluginAttribute a = new TaskPluginAttribute();
     try {
       for (Node node : definitions.getRootNodes(c)) {
-        if (node == null) {
+        if (node instanceof Node.Invalid) {
           a.roots.add(invalid());
         } else {
           new AttributeFactory(node, matchCache).create().ifPresent(t -> a.roots.add(t));
@@ -262,7 +262,7 @@
       List<TaskAttribute> subTasks = new ArrayList<>();
       for (Node subNode :
           options.onlyApplicable ? node.getSubNodes(matchCache) : node.getSubNodes()) {
-        if (subNode == null) {
+        if (subNode instanceof Node.Invalid) {
           subTasks.add(invalid());
         } else {
           MatchCache subMatchCache = matchCache;

src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java

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 e4a2f10..058bb2a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -179,12 +179,17 @@
       }
 
       protected void addInvalidNode() {
-        nodes.add(null); // null node indicates invalid
+        nodes.add(new Node().new Invalid());
       }
     }
   }
 
   public class Node extends NodeList {
+    public class Invalid extends Node {
+      @Override
+      public void refreshTask() throws ConfigInvalidException, OrmException {}
+    }
+
     public Task task;
     public boolean isDuplicate;
 
@@ -193,6 +198,11 @@
     protected Map<Branch.NameKey, List<Node>> nodesByBranch;
     protected boolean hasUnfilterableSubNodes = false;
 
+    protected Node() { // Only for Invalid
+      taskKey = null;
+      properties = null;
+    }
+
     public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException {
       this.parent = parent;
       taskKey = task.key();
@@ -222,7 +232,7 @@
         hasUnfilterableSubNodes = true;
         cachedNodeByTask.clear();
         nodes.stream()
-            .filter(n -> n != null && !n.isChange())
+            .filter(n -> !(n instanceof Invalid) && !n.isChange())
             .forEach(n -> cachedNodeByTask.put(n.task.key(), n));
       }
       return nodes;
@@ -423,7 +433,9 @@
         int filterable = 0;
         List<Node> applicableNodes = new ArrayList<>();
         for (Node node : nodes) {
-          if (node != null && isApplicableCacheableByBranch(node)) {
+          if (node instanceof Invalid) {
+            filterable++;
+          } else if (isApplicableCacheableByBranch(node)) {
             filterable++;
             try {
               if (!matchCache.match(node.task.applicable)) {
@@ -484,9 +496,7 @@
   protected static List<Node> refresh(List<Node> nodes)
       throws ConfigInvalidException, OrmException {
     for (Node node : nodes) {
-      if (node != null) {
-        node.refreshTask();
-      }
+      node.refreshTask();
     }
     return nodes;
   }