From cce017cdecd04e506778378ec79523239404c065 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Tue, 16 May 2023 09:01:56 +0200 Subject: [PATCH] Introduce `jenkins.model.queue.QueueItem` interface (#7926) * Introduce jenkins.model.queue.QueueItem interface This formalizes methods used by taglib to do the rendering of queue items. * Apply suggestions from code review Co-authored-by: Jesse Glick * Implement default Queue.Task methods to simplify Mock item * Do the right thing automatically if the Queue.Task implements AccessControlled --------- Co-authored-by: Jesse Glick --- core/src/main/java/hudson/model/Queue.java | 57 ++++++++------ .../hudson/widgets/BuildHistoryWidget.java | 12 +-- .../java/jenkins/model/queue/QueueItem.java | 78 +++++++++++++++++++ .../jenkins/widgets/HistoryPageEntry.java | 8 +- .../jenkins/widgets/HistoryPageFilter.java | 23 +++--- .../src/main/resources/lib/hudson/queue.jelly | 4 +- core/src/test/java/hudson/model/MockItem.java | 29 ++++++- .../widgets/HistoryPageFilterTest.java | 16 ++-- 8 files changed, 170 insertions(+), 57 deletions(-) create mode 100644 core/src/main/java/jenkins/model/queue/QueueItem.java diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 7a4c7c7a93..910d4637f2 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -26,6 +26,7 @@ package hudson.model; import static hudson.init.InitMilestone.JOB_CONFIG_ADAPTED; +import static hudson.model.Item.CANCEL; import static hudson.util.Iterators.reverse; import com.google.common.annotations.VisibleForTesting; @@ -112,6 +113,7 @@ import javax.servlet.http.HttpServletResponse; import jenkins.model.Jenkins; import jenkins.model.queue.AsynchronousExecution; import jenkins.model.queue.CompositeCauseOfBlockage; +import jenkins.model.queue.QueueItem; import jenkins.security.QueueItemAuthenticator; import jenkins.security.QueueItemAuthenticatorProvider; import jenkins.security.stapler.StaplerAccessibleType; @@ -1929,25 +1931,34 @@ public class Queue extends ResourceController implements Saveable { * Checks the permission to see if the current user can abort this executable. * Returns normally from this method if it's OK. *

- * NOTE: If you have implemented {@link AccessControlled} this should just be + * NOTE: If you have implemented {@link AccessControlled} this defaults to * {@code checkPermission(hudson.model.Item.CANCEL);} * * @throws AccessDeniedException if the permission is not granted. */ - void checkAbortPermission(); + default void checkAbortPermission() { + if (this instanceof AccessControlled) { + ((AccessControlled) this).checkPermission(CANCEL); + } + } /** * Works just like {@link #checkAbortPermission()} except it indicates the status by a return value, * instead of exception. * Also used by default for {@link hudson.model.Queue.Item#hasCancelPermission}. *

- * NOTE: If you have implemented {@link AccessControlled} this should just be + * NOTE: If you have implemented {@link AccessControlled} this returns by default * {@code return hasPermission(hudson.model.Item.CANCEL);} * * @return false * if the user doesn't have the permission. */ - boolean hasAbortPermission(); + default boolean hasAbortPermission() { + if (this instanceof AccessControlled) { + return ((AccessControlled) this).hasPermission(CANCEL); + } + return true; + } /** * Returns the URL of this task relative to the context root of the application. @@ -2117,7 +2128,7 @@ public class Queue extends ResourceController implements Saveable { * Item in a queue. */ @ExportedBean(defaultVisibility = 999) - public abstract static class Item extends Actionable { + public abstract static class Item extends Actionable implements QueueItem { private final long id; @@ -2128,6 +2139,7 @@ public class Queue extends ResourceController implements Saveable { * @since 1.601 */ @Exported + @Override public long getId() { return id; } @@ -2147,12 +2159,19 @@ public class Queue extends ResourceController implements Saveable { * Project to be built. */ @Exported + @NonNull public final Task task; private /*almost final*/ transient FutureImpl future; private final long inQueueSince; + @Override + @NonNull + public Task getTask() { + return task; + } + /** * Build is blocked because another build is in progress, * required {@link Resource}s are not available, or otherwise blocked @@ -2173,6 +2192,7 @@ public class Queue extends ResourceController implements Saveable { * True if the item is starving for an executor for too long. */ @Exported + @Override public boolean isStuck() { return false; } /** @@ -2188,6 +2208,7 @@ public class Queue extends ResourceController implements Saveable { * Returns a human readable presentation of how long this item is already in the queue. * E.g. something like '3 minutes 40 seconds' */ + @Override public String getInQueueForString() { long duration = System.currentTimeMillis() - this.inQueueSince; return Util.getTimeSpanString(duration); @@ -2250,6 +2271,7 @@ public class Queue extends ResourceController implements Saveable { } @Restricted(DoNotUse.class) // used from Jelly + @Override public String getCausesDescription() { List causes = getCauses(); StringBuilder s = new StringBuilder(); @@ -2259,15 +2281,11 @@ public class Queue extends ResourceController implements Saveable { return s.toString(); } - protected Item(Task task, List actions, long id, FutureImpl future) { - this.task = task; - this.id = id; - this.future = future; - this.inQueueSince = System.currentTimeMillis(); - for (Action action : actions) addAction(action); + protected Item(@NonNull Task task, @NonNull List actions, long id, FutureImpl future) { + this(task, actions, id, future, System.currentTimeMillis()); } - protected Item(Task task, List actions, long id, FutureImpl future, long inQueueSince) { + protected Item(@NonNull Task task, @NonNull List actions, long id, FutureImpl future, long inQueueSince) { this.task = task; this.id = id; this.future = future; @@ -2297,6 +2315,7 @@ public class Queue extends ResourceController implements Saveable { * Gets a human-readable status message describing why it's in the queue. */ @Exported + @Override public final String getWhy() { CauseOfBlockage cob = getCauseOfBlockage(); return cob != null ? cob.getShortDescription() : null; @@ -2312,6 +2331,7 @@ public class Queue extends ResourceController implements Saveable { * @return String */ @Exported + @Override public String getParams() { StringBuilder s = new StringBuilder(); for (ParametersAction pa : getActions(ParametersAction.class)) { @@ -2322,19 +2342,6 @@ public class Queue extends ResourceController implements Saveable { return s.toString(); } - /** - * Checks whether a scheduled item may be canceled. - * @return by default, the same as {@link hudson.model.Queue.Task#hasAbortPermission} - */ - public boolean hasCancelPermission() { - return task.hasAbortPermission(); - } - - @Override - public String getDisplayName() { - return null; - } - @Override public String getSearchUrl() { return null; diff --git a/core/src/main/java/hudson/widgets/BuildHistoryWidget.java b/core/src/main/java/hudson/widgets/BuildHistoryWidget.java index da35767714..7316de0914 100644 --- a/core/src/main/java/hudson/widgets/BuildHistoryWidget.java +++ b/core/src/main/java/hudson/widgets/BuildHistoryWidget.java @@ -24,11 +24,11 @@ package hudson.widgets; -import hudson.model.Queue.Item; import hudson.model.Queue.Task; import java.util.LinkedList; import java.util.List; import jenkins.model.Jenkins; +import jenkins.model.queue.QueueItem; import jenkins.widgets.HistoryPageFilter; /** @@ -52,17 +52,17 @@ public class BuildHistoryWidget extends HistoryWidget { /** * Returns the first queue item if the owner is scheduled for execution in the queue. */ - public Item getQueuedItem() { + public QueueItem getQueuedItem() { return Jenkins.get().getQueue().getItem(owner); } /** * Returns the queue item if the owner is scheduled for execution in the queue, in REVERSE ORDER */ - public List getQueuedItems() { - LinkedList list = new LinkedList<>(); - for (Item item : Jenkins.get().getQueue().getItems()) { - if (item.task == owner) { + public List getQueuedItems() { + LinkedList list = new LinkedList<>(); + for (QueueItem item : Jenkins.get().getQueue().getItems()) { + if (item.getTask() == owner) { list.addFirst(item); } } diff --git a/core/src/main/java/jenkins/model/queue/QueueItem.java b/core/src/main/java/jenkins/model/queue/QueueItem.java new file mode 100644 index 0000000000..ed54aed5dd --- /dev/null +++ b/core/src/main/java/jenkins/model/queue/QueueItem.java @@ -0,0 +1,78 @@ +package jenkins.model.queue; + +import edu.umd.cs.findbugs.annotations.CheckForNull; +import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.model.Cause; +import hudson.model.ModelObject; +import hudson.model.Queue; +import hudson.model.Run; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.Beta; + +/** + * Interface used by Jelly views to render a queue item through {@code }. + * @since TODO + */ +@Restricted(Beta.class) +public interface QueueItem extends ModelObject { + /** + * @return true if the item is starving for an executor for too long. + */ + boolean isStuck(); + + /** + * @return The underlying {@link Queue.Task} currently in queue. + */ + @NonNull + Queue.Task getTask(); + + /** + * Checks whether a scheduled item may be canceled. + * @return by default, the same as {@link Queue.Task#hasAbortPermission} + */ + default boolean hasCancelPermission() { + return getTask().hasAbortPermission(); + } + + /** + * Unique ID (per controller) that tracks the {@link Queue.Task} as it moves through different stages + * in the queue (each represented by different implementations of {@link QueueItem} and into any subsequent + * {@link Run} instance (see {@link Run#getQueueId()}). + */ + long getId(); + + /** + * Convenience method that returns a read only view of the {@link Cause}s associated with this item in the queue as a single string. + */ + @NonNull + String getCausesDescription(); + + /** + * Gets a human-readable status message describing why it's in the queue. + * May return null if there is no cause of blockage. + */ + @CheckForNull + String getWhy(); + + /** + * Gets a human-readable message about the parameters of this item + */ + @NonNull + String getParams(); + + /** + * Returns a human readable presentation of how long this item is already in the queue. + * E.g. something like '3 minutes 40 seconds' + */ + @NonNull + String getInQueueForString(); + + /** + * @return a display name for this queue item; by default, {@link Queue.Task#getFullDisplayName()} + */ + @CheckForNull + @Override + default String getDisplayName() { + return getTask().getFullDisplayName(); + } +} diff --git a/core/src/main/java/jenkins/widgets/HistoryPageEntry.java b/core/src/main/java/jenkins/widgets/HistoryPageEntry.java index 5028fbcf0a..ac5e0a7726 100644 --- a/core/src/main/java/jenkins/widgets/HistoryPageEntry.java +++ b/core/src/main/java/jenkins/widgets/HistoryPageEntry.java @@ -25,14 +25,14 @@ package jenkins.widgets; import edu.umd.cs.findbugs.annotations.NonNull; -import hudson.model.Queue; import hudson.model.Run; +import jenkins.model.queue.QueueItem; /** * Represents an entry used by the {@link HistoryPageFilter}. * *

- * Wraps {@link Queue.Item} and {@link Run} instances from the build queue, normalizing + * Wraps {@link QueueItem} and {@link Run} instances from the build queue, normalizing * access to the info required for pagination. * * @@ -55,8 +55,8 @@ public class HistoryPageEntry { } protected static long getEntryId(@NonNull Object entry) { - if (entry instanceof Queue.Item) { - return ((Queue.Item) entry).getId(); + if (entry instanceof QueueItem) { + return ((QueueItem) entry).getId(); } else if (entry instanceof Run) { Run run = (Run) entry; return Long.MIN_VALUE + run.getNumber(); diff --git a/core/src/main/java/jenkins/widgets/HistoryPageFilter.java b/core/src/main/java/jenkins/widgets/HistoryPageFilter.java index d289db70a7..adc09b12f2 100644 --- a/core/src/main/java/jenkins/widgets/HistoryPageFilter.java +++ b/core/src/main/java/jenkins/widgets/HistoryPageFilter.java @@ -42,6 +42,7 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; +import jenkins.model.queue.QueueItem; /** * History page filter. @@ -55,9 +56,9 @@ public class HistoryPageFilter { private Long olderThan; private String searchString; - // Need to use different Lists for Queue.Items and Runs because + // Need to use different Lists for QueueItem and Runs because // we need access to them separately in the jelly files for rendering. - public final List> queueItems = new ArrayList<>(); + public final List> queueItems = new ArrayList<>(); public final List> runs = new ArrayList<>(); @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", justification = "read by Stapler") @@ -128,7 +129,7 @@ public class HistoryPageFilter { * @param queueItems The queue items to be added. Queue items do not need to be sorted. * @since 2.17 */ - public void add(@NonNull Iterable runItems, @NonNull List queueItems) { + public void add(@NonNull Iterable runItems, @NonNull List queueItems) { sort(queueItems); addInternal(Iterables.concat(queueItems, runItems)); } @@ -136,7 +137,7 @@ public class HistoryPageFilter { /** * Add items to the History page, internal implementation. * @param items The items to be added. - * @param The type of items should either be T or Queue.Item. + * @param The type of items should either be T or {@link QueueItem}. */ private void addInternal(@NonNull Iterable items) { // Note that items can be a large lazily evaluated collection, @@ -240,8 +241,8 @@ public class HistoryPageFilter { } private long getNextBuildNumber(@NonNull Object entry) { - if (entry instanceof Queue.Item) { - Queue.Task task = ((Queue.Item) entry).task; + if (entry instanceof QueueItem) { + Queue.Task task = ((QueueItem) entry).getTask(); if (task instanceof Job) { return ((Job) task).getNextBuildNumber(); } @@ -253,8 +254,8 @@ public class HistoryPageFilter { return HistoryPageEntry.getEntryId(entry) + 1; } - private void addQueueItem(Queue.Item item) { - HistoryPageEntry entry = new HistoryPageEntry<>(item); + private void addQueueItem(QueueItem item) { + HistoryPageEntry entry = new HistoryPageEntry<>(item); queueItems.add(entry); updateNewestOldest(entry.getEntryId()); } @@ -279,8 +280,8 @@ public class HistoryPageFilter { private boolean add(Object entry) { // Purposely not calling isFull(). May need to add a greater number of entries // to the page initially, newerThan then cutting it back down to size using cutLeading() - if (entry instanceof Queue.Item) { - Queue.Item item = (Queue.Item) entry; + if (entry instanceof QueueItem) { + QueueItem item = (QueueItem) entry; if (searchString != null && !fitsSearchParams(item)) { return false; } @@ -310,7 +311,7 @@ public class HistoryPageFilter { return Math.max(0, maxEntries - size()); } - private boolean fitsSearchParams(@NonNull Queue.Item item) { + private boolean fitsSearchParams(@NonNull QueueItem item) { if (fitsSearchString(item.getDisplayName())) { return true; } else if (fitsSearchString(item.getId())) { diff --git a/core/src/main/resources/lib/hudson/queue.jelly b/core/src/main/resources/lib/hudson/queue.jelly index 1b6d66d2ee..e5d541b888 100644 --- a/core/src/main/resources/lib/hudson/queue.jelly +++ b/core/src/main/resources/lib/hudson/queue.jelly @@ -75,7 +75,7 @@ THE SOFTWARE. - + @@ -93,7 +93,7 @@ THE SOFTWARE. - + diff --git a/core/src/test/java/hudson/model/MockItem.java b/core/src/test/java/hudson/model/MockItem.java index 8e5be77d30..73f78743ab 100644 --- a/core/src/test/java/hudson/model/MockItem.java +++ b/core/src/test/java/hudson/model/MockItem.java @@ -34,7 +34,7 @@ import java.util.List; public class MockItem extends Queue.Item { public MockItem(long id) { - super(null, Collections.emptyList(), id, null); + super(new MockQueueTask(), Collections.emptyList(), id, null); } public MockItem(Queue.Task task) { @@ -67,4 +67,31 @@ public class MockItem extends Queue.Item { boolean leave(Queue q) { return true; } + + private static class MockQueueTask implements Queue.Task { + @Override + public String getDisplayName() { + return null; + } + + @Override + public String getName() { + return null; + } + + @Override + public String getFullDisplayName() { + return null; + } + + @Override + public String getUrl() { + return null; + } + + @Override + public Queue.Executable createExecutable() { + return null; + } + } } diff --git a/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java b/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java index 0205eaad83..3ce79bf426 100644 --- a/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java +++ b/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java @@ -32,7 +32,6 @@ import hudson.model.MockItem; import hudson.model.ModelObject; import hudson.model.ParameterValue; import hudson.model.ParametersAction; -import hudson.model.Queue; import hudson.model.Result; import hudson.model.Run; import hudson.model.StringParameterValue; @@ -44,6 +43,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.stream.Collectors; +import jenkins.model.queue.QueueItem; import org.junit.Assert; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -78,7 +78,7 @@ public class HistoryPageFilterTest { public void test_latest_partial_page() throws IOException { HistoryPageFilter historyPageFilter = newPage(5, null, null); Iterable runs = newRuns(1, 2); - List queueItems = newQueueItems(3, 4); + var queueItems = newQueueItems(3, 4); historyPageFilter.add(runs, queueItems); @@ -102,7 +102,7 @@ public class HistoryPageFilterTest { public void test_latest_longer_list() throws IOException { HistoryPageFilter historyPageFilter = newPage(5, null, null); Iterable runs = newRuns(1, 10); - List queueItems = newQueueItems(11, 12); + var queueItems = newQueueItems(11, 12); historyPageFilter.add(runs, queueItems); @@ -270,7 +270,7 @@ public class HistoryPageFilterTest { public void test_newerThan_doesntIncludeQueuedItems() throws IOException { HistoryPageFilter historyPageFilter = newPage(5, 5L, null); Iterable runs = newRuns(1, 10); - List queueItems = newQueueItems(11, 12); + var queueItems = newQueueItems(11, 12); historyPageFilter.add(runs, queueItems); @@ -313,7 +313,7 @@ public class HistoryPageFilterTest { //given HistoryPageFilter historyPageFilter = newPage(5, null, null); Iterable runs = newRuns(23, 24); - List queueItems = newQueueItems(25, 26); + var queueItems = newQueueItems(25, 26); //and historyPageFilter.setSearchString("23"); @@ -371,7 +371,7 @@ public class HistoryPageFilterTest { //and historyPageFilter.setSearchString(searchString); //and - List queueItems = newQueueItems(3, 4); + var queueItems = newQueueItems(3, 4); //when historyPageFilter.add(runs, queueItems); @@ -381,8 +381,8 @@ public class HistoryPageFilterTest { Assert.assertEquals(HistoryPageEntry.getEntryId(2), historyPageFilter.runs.get(0).getEntryId()); } - private List newQueueItems(long startId, long endId) { - List items = new ArrayList<>(); + private List newQueueItems(long startId, long endId) { + var items = new ArrayList(); for (long queueId = startId; queueId <= endId; queueId++) { items.add(new MockItem(queueId)); }