diff --git a/core/src/main/java/hudson/model/Action.java b/core/src/main/java/hudson/model/Action.java index 2e8d2e641f..9e48742986 100644 --- a/core/src/main/java/hudson/model/Action.java +++ b/core/src/main/java/hudson/model/Action.java @@ -68,6 +68,11 @@ import javax.annotation.CheckForNull; * (for example with {@link Build}) via XStream. In some other cases, * {@link Action}s are transient and not persisted (such as * when it's used with {@link Job}). + *

+ * The {@link Actionable#replaceAction(Action)}, {@link Actionable#addOrReplaceAction(Action)}, and + * {@link Actionable#removeAction(Action)} methods use {@link Action#equals(Object)} to determine whether to update + * or replace or remove an {@link Action}. As such, {@link Action} subclasses that provide a deep + * {@link #equals(Object)} will assist in reducing the need for unnecessary persistence. * * @author Kohsuke Kawaguchi */ diff --git a/core/src/main/java/hudson/model/Actionable.java b/core/src/main/java/hudson/model/Actionable.java index 7734785421..7a5e92daa1 100644 --- a/core/src/main/java/hudson/model/Actionable.java +++ b/core/src/main/java/hudson/model/Actionable.java @@ -133,8 +133,9 @@ public abstract class Actionable extends AbstractModelObject implements ModelObj } /** - * Adds a new action. Note calls to {@link #getAllActions()} that happen before calls to this method may not see the - * update.Note: this method will always modify the actions + * Adds a new action. + * Note: calls to {@link #getAllActions()} that happen before calls to this method may not see the update. + * Note: this method will always modify the actions * * The default implementation is mostly equivalent to the call chain {@code getActions().add(a)}. */ @@ -148,15 +149,34 @@ public abstract class Actionable extends AbstractModelObject implements ModelObj } /** - * Add an action, replacing any existing action of the (exact) same class. Note calls to {@link #getAllActions()} - * that happen before calls to this method may not see the update. Note: this method will always modify the - * actions + * Add an action, replacing any existing actions of the (exact) same class. + * Note: calls to {@link #getAllActions()} that happen before calls to this method may not see the update. + * Note: this method does not affect transient actions contributed by a {@link TransientActionFactory} + * * @param a an action to add/replace * @since 1.548 + * @see #addOrReplaceAction(Action) if you want to know whether the backing {@link #actions} was modified, for + * example in cases where the caller would need to persist the {@link Actionable} in order to persist the change + * and there is a desire to elide unneccessary persistence of unmodified objects. */ @SuppressWarnings({"ConstantConditions", "deprecation"}) @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE") public void replaceAction(@Nonnull Action a) { + addOrReplaceAction(a); + } + + /** + * Add an action, replacing any existing actions of the (exact) same class. + * Note: calls to {@link #getAllActions()} that happen before calls to this method may not see the update. + * Note: this method does not affect transient actions contributed by a {@link TransientActionFactory} + * + * @param a an action to add/replace + * @return {@code true} if this actions changed as a result of the call + * @since FIXME + */ + @SuppressWarnings({"ConstantConditions", "deprecation"}) + @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE") + public boolean addOrReplaceAction(@Nonnull Action a) { if (a == null) { throw new IllegalArgumentException("Action must be non-null"); } @@ -175,11 +195,13 @@ public abstract class Actionable extends AbstractModelObject implements ModelObj if (!found) { addAction(a); } + return !found || !old.isEmpty(); } /** - * Remove an action. Note calls to {@link #getAllActions()} that happen before calls to this method may not see the - * update. + * Remove an action. + * Note: calls to {@link #getAllActions()} that happen before calls to this method may not see the update. + * Note: this method does not affect transient actions contributed by a {@link TransientActionFactory} * * @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 @@ -195,8 +217,9 @@ public abstract class Actionable extends AbstractModelObject implements ModelObj } /** - * Removes any actions of the specified type. Note calls to {@link #getAllActions()} that happen before calls to - * this method may not see the update. + * Removes any actions of the specified type. + * Note: calls to {@link #getAllActions()} that happen before calls to this method may not see the update. + * Note: this method does not affect transient actions contributed by a {@link TransientActionFactory} * * @param clazz the type of actions to remove * @return {@code true} if this actions changed as a result of the call @@ -220,8 +243,9 @@ public abstract class Actionable extends AbstractModelObject implements ModelObj } /** - * Replaces any actions of the specified type by the supplied action. Note calls to {@link #getAllActions()} that - * happen before calls to this method may not see the update. + * Replaces any actions of the specified type by the supplied action. + * Note: calls to {@link #getAllActions()} that happen before calls to this method may not see the update. + * Note: this method does not affect transient actions contributed by a {@link TransientActionFactory} * * @param clazz the type of actions to replace (note that the action you are replacing this with need not extend * this class) @@ -235,6 +259,9 @@ public abstract class Actionable extends AbstractModelObject implements ModelObj if (clazz == null) { throw new IllegalArgumentException("Action type must be non-null"); } + 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 old = new ArrayList(); List current = getOrCreateActions(); diff --git a/core/src/test/java/hudson/model/ActionableTest.java b/core/src/test/java/hudson/model/ActionableTest.java index d380229668..654b08e67e 100644 --- a/core/src/test/java/hudson/model/ActionableTest.java +++ b/core/src/test/java/hudson/model/ActionableTest.java @@ -34,13 +34,11 @@ import org.junit.Test; public class ActionableTest { + private Actionable thing = new ActionableImpl(); + @SuppressWarnings("deprecation") @Test public void replaceAction() { - 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); @@ -50,13 +48,27 @@ public class ActionableTest { assertEquals(Arrays.asList(a2, a3), thing.getActions()); } + @SuppressWarnings("deprecation") + @Test + public void addOrReplaceAction() { + CauseAction a1 = new CauseAction(); + ParametersAction a2 = new ParametersAction(); + thing.addAction(a1); + thing.addAction(a2); + CauseAction a3 = new CauseAction(); + assertTrue(thing.addOrReplaceAction(a3)); + assertEquals(Arrays.asList(a2, a3), thing.getActions()); + assertFalse(thing.addOrReplaceAction(a3)); + assertEquals(Arrays.asList(a2, a3), thing.getActions()); + thing.addAction(a1); + assertEquals(Arrays.asList(a2, a3, a1), thing.getActions()); + assertTrue(thing.addOrReplaceAction(a3)); + assertEquals(Arrays.asList(a2, a3), thing.getActions()); + } + @SuppressWarnings("deprecation") @Test public void replaceActions() { - 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); @@ -71,10 +83,6 @@ public class ActionableTest { @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); @@ -91,10 +99,6 @@ public class ActionableTest { @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); @@ -109,10 +113,6 @@ public class ActionableTest { @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.emptyList(), thing.getActions()); @@ -122,4 +122,44 @@ public class ActionableTest { assertEquals(Arrays.asList(a1, a2), thing.getActions()); } + @Test(expected = IllegalArgumentException.class) + public void addAction_null() { + thing.addAction(null); + } + + @Test(expected = IllegalArgumentException.class) + public void replaceAction_null() { + thing.replaceAction(null); + } + + @Test(expected = IllegalArgumentException.class) + public void replaceActions_null() { + thing.replaceActions(CauseAction.class, null); + } + + @Test(expected = IllegalArgumentException.class) + public void replaceActions_null_null() { + thing.replaceActions(null, null); + } + + @Test(expected = IllegalArgumentException.class) + public void addOrReplaceAction_null() { + thing.addOrReplaceAction(null); + } + + @Test + public void removeAction_null() { + assertFalse(thing.removeAction(null)); + } + + @Test(expected = IllegalArgumentException.class) + public void removeActions_null() { + thing.removeActions(null); + } + + private static class ActionableImpl extends Actionable { + @Override public String getDisplayName() {return null;} + + @Override public String getSearchUrl() {return null;} + } }