[FIXED JENKINS-39404] Restore symmetry by adding removeAction and rem…

…oveActions methods to Actionable
jenkinsci · Nov 1, 2016 · 1990e3d · 1990e3d
1 parent 2f19ef0
commit 1990e3d
Show file tree

Hide file tree

Showing 2 changed files with 109 additions and 3 deletions.
diff --git a/core/src/main/java/hudson/model/Actionable.java b/core/src/main/java/hudson/model/Actionable.java
@@ -23,6 +23,7 @@
  */
 package hudson.model;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.ExtensionList;
 import hudson.Util;
 import java.util.ArrayList;
@@ -33,6 +34,8 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import jenkins.model.ModelObjectWithContextMenu;
 import jenkins.model.TransientActionFactory;
 import org.kohsuke.stapler.StaplerRequest;
@@ -124,8 +127,12 @@ public <T extends Action> List<T> getActions(Class<T> type) {
      *
      * The default implementation calls {@code getActions().add(a)}.
      */
-    public void addAction(Action a) {
-        if(a==null) throw new IllegalArgumentException();
+    @SuppressWarnings("ConstantConditions")
+    @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE")
+    public void addAction(@Nonnull Action a) {
+        if(a==null) {
+            throw new IllegalArgumentException("Action must be non-null");
+        }
         getActions().add(a);
     }
 
@@ -134,7 +141,12 @@ public void addAction(Action a) {
      * @param a an action to add/replace
      * @since 1.548
      */
-    public void replaceAction(Action a) {
+    @SuppressWarnings("ConstantConditions")
+    @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE")
+    public void replaceAction(@Nonnull Action a) {
+        if (a == null) {
+            throw new IllegalArgumentException("Action must be non-null");
+        }
         // CopyOnWriteArrayList does not support Iterator.remove, so need to do it this way:
         List<Action> old = new ArrayList<Action>(1);
         List<Action> current = getActions();
@@ -147,6 +159,45 @@ public void replaceAction(Action a) {
         addAction(a);
     }
 
+    /**
+     * Remove an action.
+     *
+     * @param a an action to remove (if {@code null} then this will be a no-op)
+     * @return {@code true} if this actions changed as a result of the call
+     * @since FIXME
+     */
+    public boolean removeAction(@Nullable Action a) {
+        if (a == null) {
+            return false;
+        }
+        // CopyOnWriteArrayList does not support Iterator.remove, so need to do it this way:
+        return getActions().removeAll(Collections.singleton(a));
+    }
+
+    /**
+     * Removes any actions of the specified type.
+     *
+     * @param clazz the type of actions to remove
+     * @return {@code true} if this actions changed as a result of the call
+     * @since FIXME
+     */
+    @SuppressWarnings("ConstantConditions")
+    @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE")
+    public boolean removeActions(@Nonnull Class<? extends Action> clazz) {
+        if (clazz == null) {
+            throw new IllegalArgumentException("Action type must be non-null");
+        }
+        // CopyOnWriteArrayList does not support Iterator.remove, so need to do it this way:
+        List<Action> old = new ArrayList<Action>();
+        List<Action> current = getActions();
+        for (Action a : current) {
+            if (clazz.isInstance(a)) {
+                old.add(a);
+            }
+        }
+        return current.removeAll(old);
+    }
+
     /** @deprecated No clear purpose, since subclasses may have overridden {@link #getActions}, and does not consider {@link TransientActionFactory}. */
     @Deprecated
     public Action getAction(int index) {

diff --git a/core/src/test/java/hudson/model/ActionableTest.java b/core/src/test/java/hudson/model/ActionableTest.java
@@ -25,7 +25,11 @@
 package hudson.model;
 
 import java.util.Arrays;
+
+import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.*;
+
+import java.util.Collections;
 import org.junit.Test;
 
 public class ActionableTest {
@@ -45,4 +49,55 @@ public class ActionableTest {
         assertEquals(Arrays.asList(a2, a3), thing.getActions());
     }
 
+    @SuppressWarnings("deprecation")
+    @Test public void removeAction() {
+        Actionable thing = new Actionable() {
+            @Override public String getDisplayName() {return  null;}
+            @Override public String getSearchUrl() {return null;}
+        };
+        CauseAction a1 = new CauseAction();
+        ParametersAction a2 = new ParametersAction();
+        thing.addAction(a1);
+        thing.addAction(a2);
+        assertEquals(Arrays.asList(a1, a2), thing.getActions());
+        assertThat(thing.removeAction(a1), is(true));
+        assertEquals(Arrays.asList(a2), thing.getActions());
+        assertThat(thing.removeAction(a1), is(false));
+        assertEquals(Arrays.asList(a2), thing.getActions());
+        assertThat(thing.removeAction(null), is(false));
+        assertEquals(Arrays.asList(a2), thing.getActions());
+    }
+
+    @SuppressWarnings("deprecation")
+    @Test public void removeActions() {
+        Actionable thing = new Actionable() {
+            @Override public String getDisplayName() {return  null;}
+            @Override public String getSearchUrl() {return null;}
+        };
+        CauseAction a1 = new CauseAction();
+        ParametersAction a2 = new ParametersAction();
+        thing.addAction(a1);
+        thing.addAction(a2);
+        assertEquals(Arrays.asList(a1, a2), thing.getActions());
+        assertThat(thing.removeActions(CauseAction.class), is(true));
+        assertEquals(Arrays.asList(a2), thing.getActions());
+        assertThat(thing.removeActions(CauseAction.class), is(false));
+        assertEquals(Arrays.asList(a2), thing.getActions());
+    }
+
+    @SuppressWarnings("deprecation")
+    @Test public void addAction() {
+        Actionable thing = new Actionable() {
+            @Override public String getDisplayName() {return  null;}
+            @Override public String getSearchUrl() {return null;}
+        };
+        CauseAction a1 = new CauseAction();
+        ParametersAction a2 = new ParametersAction();
+        assertEquals(Collections.<Action>emptyList(), thing.getActions());
+        thing.addAction(a1);
+        assertEquals(Collections.singletonList(a1), thing.getActions());
+        thing.addAction(a2);
+        assertEquals(Arrays.asList(a1, a2), thing.getActions());
+    }
+
 }