[JENKINS-21311] Make the queue/cancelItem API an API. (#4395)

* [JENKINS-21311] Make the queue/cancelItem API an API.

This change:
* documents the API
* adds meaningful return codes to help API caller.

* [JENKINS-21311] Code review

* [JENKINS-21311] Update failing integration test

core/src/main/java/hudson/model/Queue.java

@@ -132,6 +132,8 @@ import com.thoughtworks.xstream.converters.basic.AbstractSingleValueConverter;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnegative;
+import javax.servlet.http.HttpServletResponse;
+
 import jenkins.model.queue.AsynchronousExecution;
 import jenkins.model.queue.CompositeCauseOfBlockage;
 import org.kohsuke.stapler.QueryParameter;
@@ -760,17 +762,21 @@ public class Queue extends ResourceController implements Saveable {
     }
 
     /**
-     * Called from {@code queue.jelly} and {@code entries.jelly}.
+     * Called from {@code queue.jelly} and {@code queue-items.jelly}.
      */
     @RequirePOST
     public HttpResponse doCancelItem(@QueryParameter long id) throws IOException, ServletException {
         Item item = getItem(id);
         if (item != null) {
             if(item.hasCancelPermission()){
-                cancel(item);
+                if(cancel(item)) {
+                    return HttpResponses.status(HttpServletResponse.SC_NO_CONTENT);
+                }
+                return HttpResponses.error(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Could not cancel run for id " + id);
             }
+            return HttpResponses.error(422, "Item for id (" + id + ") is not cancellable");
         } // else too late, ignore (JENKINS-14813)
-        return HttpResponses.forwardToPreviousPage();
+        return HttpResponses.error(HttpServletResponse.SC_NOT_FOUND, "Provided id (" + id + ") not found");
     }
 
     public boolean isEmpty() {
@@ -2303,11 +2309,11 @@ public class Queue extends ResourceController implements Saveable {
         /** @deprecated Use {@link #doCancelItem} instead. */
         @Deprecated
         @RequirePOST
-        public HttpResponse doCancelQueue() throws IOException, ServletException {
+        public HttpResponse doCancelQueue() {
             if(hasCancelPermission()){
                 Jenkins.get().getQueue().cancel(this);
             }
-            return HttpResponses.forwardToPreviousPage();
+            return HttpResponses.status(HttpServletResponse.SC_NO_CONTENT);
         }
 
         /**

core/src/main/resources/hudson/model/queue/_api.jelly

+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
+
+  <h2>Canceling the build of an item in the Queue</h2>
+  <p>
+    To programmatically cancel the build of an item in the Queue, post to <a href="../cancelItem">this URL</a>.
+    The id of the item to cancel is expected.
+
+    Response will be:
+    <ul>
+      <li> 204 if the run tied with the given id was successfully cancelled</li>
+      <li> 422 if the provided id does not exist or match an item that cannot be cancelled</li>
+      <li> 500 if the provided id exists but Jenkins could not cancel the run</li>
+    </ul>
+  </p>
+
+</j:jelly>
\ No newline at end of file

core/src/test/java/hudson/model/QueueTest.java

+package hudson.model;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.jvnet.hudson.test.Issue;
+import org.kohsuke.stapler.HttpResponse;
+import org.kohsuke.stapler.StaplerResponse;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.io.PrintWriter;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class QueueTest {
+
+    @Mock
+    StaplerResponse resp;
+    @Mock
+    Queue.Task task;
+    @Mock
+    PrintWriter writer;
+
+    @Before
+    public void setup() throws IOException {
+        when(resp.getWriter()).thenReturn(writer);
+    }
+
+    @Issue("JENKINS-21311")
+    @Test
+    public void cancelItemOnaValidItemShouldReturnA204() throws IOException, ServletException {
+        when(task.hasAbortPermission()).thenReturn(true);
+        Queue queue = new Queue(LoadBalancer.CONSISTENT_HASH);
+        queue.schedule(task, 6000);
+
+        HttpResponse httpResponse = queue.doCancelItem(Queue.WaitingItem.getCurrentCounterValue());
+        httpResponse.generateResponse(null, resp, null);
+
+        verify(resp).setStatus(HttpServletResponse.SC_NO_CONTENT);
+    }
+
+    @Issue("JENKINS-21311")
+    @Test
+    public void cancelItemOnANonExistingItemShouldReturnA404()  throws IOException, ServletException {
+        Queue queue = new Queue(LoadBalancer.CONSISTENT_HASH);
+        queue.schedule(task, 6000);
+
+        HttpResponse httpResponse = queue.doCancelItem(Queue.WaitingItem.getCurrentCounterValue() + 1);
+        httpResponse.generateResponse(null, resp, null);
+
+        verify(resp).setStatus(HttpServletResponse.SC_NOT_FOUND);
+    }
+
+    @Issue("JENKINS-21311")
+    @Test
+    public void cancelItemOnANonCancellableItemShouldReturnA422()  throws IOException, ServletException {
+        when(task.hasAbortPermission()).thenReturn(false);
+        Queue queue = new Queue(LoadBalancer.CONSISTENT_HASH);
+        queue.schedule(task, 6000);
+
+        HttpResponse httpResponse = queue.doCancelItem(Queue.WaitingItem.getCurrentCounterValue());
+        httpResponse.generateResponse(null, resp, null);
+
+        verify(resp).setStatus(422);
+    }
+}

test/src/test/java/hudson/model/QueueTest.java

@@ -1086,16 +1086,22 @@ public class QueueTest {
     @Test
     @Issue("SECURITY-891")
     public void doCancelItem_PermissionIsChecked() throws Exception {
-        checkCancelOperationUsingUrl(item -> "queue/cancelItem?id=" + item.getId());
+        checkCancelOperationUsingUrl(item -> "queue/cancelItem?id=" + item.getId(), false);
     }
 
     @Test
     @Issue("SECURITY-891")
     public void doCancelQueue_PermissionIsChecked() throws Exception {
-        checkCancelOperationUsingUrl(item -> "queue/item/" + item.getId() + "/cancelQueue");
+        checkCancelOperationUsingUrl(item -> "queue/item/" + item.getId() + "/cancelQueue", true);
     }
 
-    private void checkCancelOperationUsingUrl(Function<Queue.Item, String> urlProvider) throws Exception {
+    /**
+     *
+     * @param urlProvider the endpoint to query
+     * @param legacyRedirect whether the endpoint has the legacy behavior (ie makes a redirect no matter the result)
+     *                       Or it uses the newer response codes introduced by JENKINS-21311
+     */
+    private void checkCancelOperationUsingUrl(Function<Queue.Item, String> urlProvider, boolean legacyRedirect) throws Exception {
         Queue q = r.jenkins.getQueue();
 
         r.jenkins.setCrumbIssuer(null);
@@ -1124,11 +1130,13 @@ public class QueueTest {
                     .withRedirectEnabled(false)
                     .withThrowExceptionOnFailingStatusCode(false);
             wc.login("user");
-            Page p = wc.getPage(request);
-            // currently the endpoint return a redirection to the previously visited page, none in our case
-            // (so force no redirect to avoid false positive error)
-            assertThat(p.getWebResponse().getStatusCode(), lessThan(400));
-
+            if(legacyRedirect) {
+                Page p = wc.getPage(request);
+                // the legacy endpoint returns a redirection to the previously visited page, none in our case
+                // (so force no redirect to avoid false positive error)
+                // see JENKINS-21311
+                assertThat(p.getWebResponse().getStatusCode(), lessThan(400));
+            }
             assertFalse(currentOne.getFuture().isCancelled());
         }
         { // user with right can