[JENKINS-46759] Fixed bug in build queue filtering for views (#3008)

* [JENKINS-46759] Fixed bug related to views which filters build queue, jobs which has multiple sub steps were filtered incorrectly. * Added test case for build queue filtering in views. * [JENKINS-46759] Improved code based on feedback from PR; Fixed logic bug, moved logic to separate method and added limit to number of iteration when looking for owner task.
jenkinsci · Oct 14, 2017 · ce2d0f9 · ce2d0f9
1 parent 02b8e7f
commit ce2d0f9
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 12 deletions.
diff --git a/core/src/main/java/hudson/model/View.java b/core/src/main/java/hudson/model/View.java
@@ -114,6 +114,7 @@
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
 import static jenkins.scm.RunWithSCM.*;
@@ -455,25 +456,49 @@ private boolean isRelevant(Collection<Label> labels, Computer computer) {
         return false;
     }
 
+    private final static int FILTER_LOOP_MAX_COUNT = 10;
+
     private List<Queue.Item> filterQueue(List<Queue.Item> base) {
         if (!isFilterQueue()) {
             return base;
         }
-
         Collection<TopLevelItem> items = getItems();
-        List<Queue.Item> result = new ArrayList<Queue.Item>();
-        for (Queue.Item qi : base) {
-            if (items.contains(qi.task)) {
-                result.add(qi);
-            } else
-            if (qi.task instanceof AbstractProject<?, ?>) {
-                AbstractProject<?,?> project = (AbstractProject<?, ?>) qi.task;
-                if (items.contains(project.getRootProject())) {
-                    result.add(qi);
-                }
+        return base.stream().filter(qi -> filterQueueItemTest(qi, items))
+                .collect(Collectors.toList());
+    }
+
+    private boolean filterQueueItemTest(Queue.Item item, Collection<TopLevelItem> viewItems) {
+        // Check if the task of parent tasks are in the list of viewItems.
+        // Pipeline jobs and other jobs which allow parts require us to
+        // check owner tasks as well.
+        Queue.Task currentTask = item.task;
+        for (int count = 1;; count++) {
+            if (viewItems.contains(currentTask)) {
+                return true;
+            }
+            Queue.Task next = currentTask.getOwnerTask();
+            if (next == currentTask) {
+                break;
+            } else {
+                currentTask = next;
+            }
+            if (count == FILTER_LOOP_MAX_COUNT) {
+                LOGGER.warning(String.format(
+                        "Failed to find root task for queue item '%s' for " +
+                        "view '%s' in under %d iterations, aborting!",
+                        item.getDisplayName(), getDisplayName(),
+                        FILTER_LOOP_MAX_COUNT));
+                break;
             }
         }
-        return result;
+        // Check root project for sub-job projects (e.g. matrix jobs).
+        if (item.task instanceof AbstractProject<?, ?>) {
+            AbstractProject<?,?> project = (AbstractProject<?, ?>) item.task;
+            if (viewItems.contains(project.getRootProject())) {
+                return true;
+            }
+        }
+        return false;
     }
 
     public List<Queue.Item> getQueueItems() {

diff --git a/core/src/test/java/hudson/model/MockItem.java b/core/src/test/java/hudson/model/MockItem.java
@@ -37,6 +37,10 @@ public MockItem(long id) {
         super(null, Collections.<Action>emptyList(), id, null);
     }
 
+    public MockItem(Queue.Task task) {
+        super(task, Collections.emptyList(), -1, null);
+    }
+
     public MockItem(Queue.Task task, List<Action> actions, long id) {
         super(task, actions, id, null);
     }

diff --git a/core/src/test/java/hudson/model/ViewTest.java b/core/src/test/java/hudson/model/ViewTest.java
@@ -22,6 +22,7 @@
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
 import org.mockito.Mockito;
+import org.powermock.reflect.Whitebox;
 
 public class ViewTest {
 
@@ -112,6 +113,62 @@ private TopLevelItem createJob(String jobName) {
         return rootJob;
     }
 
+    @Test
+    public void buildQueueFiltering() throws Exception {
+        // Mimic a freestyle job
+        FreeStyleProject singleItemJob = Mockito.mock(FreeStyleProject.class);
+        Mockito.when(singleItemJob.getOwnerTask()).thenReturn(singleItemJob);
+        Queue.Item singleItemQueueItem = new MockItem(singleItemJob);
+
+        // Mimic pattern of a Matrix job, i.e. with root item in view and sub
+        // items in queue.
+        FreeStyleProject multiItemJob = Mockito.mock(FreeStyleProject.class);
+        Project multiItemSubJob = Mockito.mock(Project.class);
+        Mockito.when(multiItemSubJob.getRootProject()).thenReturn(multiItemJob);
+        Mockito.when(multiItemSubJob.getOwnerTask()).thenReturn(multiItemSubJob);
+        Queue.Item multiItemQueueItem = new MockItem(multiItemSubJob);
+
+        // Mimic pattern of a Pipeline job, i.e. with item in view and
+        // sub-steps in queue.
+        BuildableTopLevelItem multiStepJob
+                = Mockito.mock(BuildableTopLevelItem.class);
+        Mockito.when(multiStepJob.getOwnerTask()).thenReturn(multiStepJob);
+        BuildableItem multiStepSubStep = Mockito.mock(BuildableItem.class);
+        Mockito.when(multiStepSubStep.getOwnerTask()).thenReturn(multiStepJob);
+        Queue.Item multiStepQueueItem = new MockItem(multiStepSubStep);
+
+        // Construct the view
+        View view = Mockito.mock(View.class);
+        List<Queue.Item> queue = Arrays.asList(singleItemQueueItem,
+                multiItemQueueItem, multiStepQueueItem);
+        Mockito.when(view.isFilterQueue()).thenReturn(true);
+
+        // Positive test, ensure that queue items are included
+        List<TopLevelItem> viewJobs = Arrays.asList(singleItemJob, multiItemJob, multiStepJob);
+        Mockito.when(view.getItems()).thenReturn(viewJobs);
+        assertEquals(
+                Arrays.asList(singleItemQueueItem,
+                        multiItemQueueItem, multiStepQueueItem),
+                Whitebox.invokeMethod(view, "filterQueue", queue)
+        );
+
+        // Negative test, ensure that queue items are excluded
+        Mockito.when(view.getItems()).thenReturn(Collections.emptyList());
+        List<Queue.Item> expected = Arrays.asList(singleItemQueueItem,
+                multiItemQueueItem, multiStepQueueItem);
+        assertEquals(
+                Collections.emptyList(),
+                Whitebox.<List<Queue.Item>>invokeMethod(view, "filterQueue", queue)
+        );
+    }
+
+    /**
+     * This interface fulfills both TopLevelItem and BuildableItem interface,
+     * this allows it for being present in a view as well as the build queue!
+     */
+    private interface BuildableTopLevelItem extends TopLevelItem, BuildableItem {
+    }
+
     public static class CompositeView extends View implements ViewGroup {
 
         private View[] views;