[FIXED JENKINS-18028] Providing a supported way to move an item betwe…

…en folders, firing changes as we go.
jenkinsci · Dec 19, 2013 · 9fc87f4 · 9fc87f4
1 parent 3b8fff8
commit 9fc87f4
Show file tree

Hide file tree

Showing 8 changed files with 194 additions and 5 deletions.
diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java
@@ -224,6 +224,7 @@ protected void renameTo(String newName) throws IOException {
                             + " already exists");
 
                 String oldName = this.name;
+                String oldFullName = getFullName();
                 File oldRoot = this.getRootDir();
 
                 doSetName(newName);
@@ -294,8 +295,7 @@ protected void renameTo(String newName) throws IOException {
 
                 callOnRenamed(newName, parent, oldName);
 
-                for (ItemListener l : ItemListener.all())
-                    l.onRenamed(this, oldName, newName);
+                ItemListener.fireLocationChange(this, oldFullName);
             }
         }
     }

diff --git a/core/src/main/java/hudson/model/ItemGroupMixIn.java b/core/src/main/java/hudson/model/ItemGroupMixIn.java
@@ -98,7 +98,13 @@ public boolean accept(File child) {
         CopyOnWriteMap.Tree<K,V> configurations = new CopyOnWriteMap.Tree<K,V>();
         for (File subdir : subdirs) {
             try {
-                V item = (V) Items.load(parent,subdir);
+                // Try to retain the identity of an existing child object if we can.
+                V item = (V) parent.getItem(subdir.getName());
+                if (item == null) {
+                    item = (V) Items.load(parent,subdir);
+                } else {
+                    item.onLoad(parent, subdir.getName());
+                }
                 configurations.put(key.call(item), item);
             } catch (IOException e) {
                 Logger.getLogger(ItemGroupMixIn.class.getName()).log(Level.WARNING, "could not load " + subdir, e);

diff --git a/core/src/main/java/hudson/model/Items.java b/core/src/main/java/hudson/model/Items.java
@@ -30,6 +30,7 @@
 import hudson.matrix.MatrixConfiguration;
 import hudson.XmlFile;
 import hudson.matrix.Axis;
+import hudson.model.listeners.ItemListener;
 import hudson.triggers.Trigger;
 import hudson.util.DescriptorList;
 import hudson.util.EditDistance;
@@ -41,6 +42,8 @@
 import java.io.IOException;
 import java.util.*;
 import javax.annotation.CheckForNull;
+import jenkins.model.DirectlyModifiableTopLevelItemGroup;
+import org.apache.commons.io.FileUtils;
 
 /**
  * Convenience methods related to {@link Item}.
@@ -300,6 +303,38 @@ public int compare(T o1, T o2) {
         return Jenkins.getInstance().getItem(nearest, context, type);
     }
 
+    /**
+     * Moves an item between folders (or top level).
+     * Fires all relevant events but does not verify that the item’s directory is not currently being used in some way (for example by a running build).
+     * Does not check any permissions.
+     * @param item some item (job or folder)
+     * @param destination the destination of the move (a folder or {@link Jenkins}); not the current parent (or you could just call {@link AbstractItem#renameTo})
+     * @throws IOException if the move fails, or some subsequent step fails (directory might have already been moved)
+     * @throws IllegalArgumentException if the move would really be a rename, or the destination cannot accept the item, or the destination already has an item of that name
+     * @since TODO
+     */
+    public static <I extends AbstractItem & TopLevelItem> void move(I item, DirectlyModifiableTopLevelItemGroup destination) throws IOException, IllegalArgumentException {
+        DirectlyModifiableTopLevelItemGroup oldParent = (DirectlyModifiableTopLevelItemGroup) item.getParent();
+        if (oldParent == destination) {
+            throw new IllegalArgumentException();
+        }
+        // TODO verify that destination is to not equal to, or inside, item
+        if (!destination.canAdd(item)) {
+            throw new IllegalArgumentException();
+        }
+        String name = item.getName();
+        if (destination.getItem(name) != null) {
+            throw new IllegalArgumentException(name + " already exists");
+        }
+        String oldFullName = item.getFullName();
+        // TODO AbstractItem.renameTo has a more baroque implementation; factor it out into a utility method perhaps?
+        FileUtils.moveDirectory(item.getRootDir(), destination.getRootDirFor(item));
+        oldParent.remove(item);
+        destination.add(item, name);
+        item.onLoad(destination, name);
+        ItemListener.fireLocationChange(item, oldFullName);
+    }
+
     /**
      * Used to load/save job configuration.
      *

diff --git a/core/src/main/java/hudson/model/listeners/ItemListener.java b/core/src/main/java/hudson/model/listeners/ItemListener.java
@@ -28,6 +28,8 @@
 import hudson.Extension;
 import jenkins.model.Jenkins;
 import hudson.model.Item;
+import hudson.model.ItemGroup;
+import hudson.model.Items;
 
 /**
  * Receives notifications about CRUD operations of {@link Item}.
@@ -94,6 +96,27 @@ public void onDeleted(Item item) {
     public void onRenamed(Item item, String oldName, String newName) {
     }
 
+    /**
+     * Called after an item’s fully-qualified location has changed.
+     * This might be because:
+     * <ul>
+     * <li>This item was renamed.
+     * <li>Some ancestor folder was renamed.
+     * <li>This item was moved between folders (or from a folder to Jenkins root or vice-versa).
+     * <li>Some ancestor folder was moved.
+     * </ul>
+     * Where applicable, {@link #onRenamed} will already have been called on this item or an ancestor.
+     * And where applicable, {@link #onLocationChanged} will already have been called on its ancestors.
+     * <p>This method should be used (instead of {@link #onRenamed}) by any code
+     * which seeks to keep (absolute) references to items up to date:
+     * if a persisted reference matches {@code oldFullName}, replace it with {@code newFullName}.
+     * @param item an item whose absolute position is now different
+     * @param oldFullName the former {@link Item#getFullName}
+     * @param newFullName the current {@link Item#getFullName}
+     * @since TODO
+     */
+    public void onLocationChanged(Item item, String oldFullName, String newFullName) {}
+
     /**
      * Called after a job has its configuration updated.
      *
@@ -149,4 +172,42 @@ public static void fireOnDeleted(Item item) {
         }
     }
 
+    /**
+     * Calls {@link #onRenamed} and {@link #onLocationChanged} as appropriate.
+     * @param rootItem the topmost item whose location has just changed
+     * @param oldFullName the previous {@link Item#getFullName}
+     * @since TODO
+     */
+    public static void fireLocationChange(Item rootItem, String oldFullName) {
+        String prefix = rootItem.getParent().getFullName();
+        if (!prefix.isEmpty()) {
+            prefix += '/';
+        }
+        String newFullName = rootItem.getFullName();
+        assert newFullName.startsWith(prefix);
+        int prefixS = prefix.length();
+        if (oldFullName.startsWith(prefix) && oldFullName.indexOf('/', prefixS) == -1) {
+            String oldName = oldFullName.substring(prefixS);
+            String newName = rootItem.getName();
+            assert newName.equals(newFullName.substring(prefixS));
+            for (ItemListener l : all()) {
+                l.onRenamed(rootItem, oldName, newName);
+            }
+        }
+        for (ItemListener l : all()) {
+            l.onLocationChanged(rootItem, oldFullName, newFullName);
+        }
+        if (rootItem instanceof ItemGroup) {
+            for (Item child : Items.getAllItems((ItemGroup) rootItem, Item.class)) {
+                String childNew = child.getFullName();
+                assert childNew.startsWith(newFullName);
+                assert childNew.charAt(newFullName.length()) == '/';
+                String childOld = oldFullName + childNew.substring(newFullName.length());
+                for (ItemListener l : all()) {
+                    l.onLocationChanged(child, childOld, childNew);
+                }
+            }
+        }
+    }
+
 }
diff --git a/core/src/main/java/jenkins/model/DirectlyModifiableTopLevelItemGroup.java b/core/src/main/java/jenkins/model/DirectlyModifiableTopLevelItemGroup.java
@@ -46,7 +46,6 @@ public interface DirectlyModifiableTopLevelItemGroup extends ModifiableTopLevelI
      * Adds an item to this group.
      * Unlike {@link Jenkins#putItem} this does not try to call {@link Item#delete} on an existing item, nor does it fire {@link ItemListener#onCreated}, nor check permissions.
      * Normally you would call {@link Item#onLoad} after calling this method (the implementation is not expected to do so).
-     * To remove an item, use {@link #onDeleted}.
      * @param <I> the kind of item
      * @param item an item to add which is currently elsewhere
      * @param name the desired item name in this group (might simply be the original {@link Item#getName})
@@ -56,4 +55,13 @@ public interface DirectlyModifiableTopLevelItemGroup extends ModifiableTopLevelI
      */
     <I extends TopLevelItem> I add(I item, String name) throws IOException, IllegalArgumentException;
 
+    /**
+     * Removes an item from this group.
+     * Unlike {@link #onDeleted} this is not expected to fire any events.
+     * @param item an item which was part of this group
+     * @throws IOException if removing fails
+     * @throws IllegalArgumentException if this was not part of the group to begin with
+     */
+    void remove(TopLevelItem item) throws IOException, IllegalArgumentException;
+
 }
diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2463,6 +2463,10 @@ public void onDeleted(TopLevelItem item) throws IOException {
         return item;
     }
 
+    @Override public void remove(TopLevelItem item) throws IOException, IllegalArgumentException {
+        items.remove(item.getName());
+    }
+
     public FingerprintMap getFingerprintMap() {
         return fingerprintMap;
     }

diff --git a/test/src/main/java/org/jvnet/hudson/test/MockFolder.java b/test/src/main/java/org/jvnet/hudson/test/MockFolder.java
@@ -186,6 +186,10 @@ public <T extends TopLevelItem> T createProject(Class<T> type, String name) thro
         }
     }
 
+    @Override public void renameTo(String newName) throws IOException {
+        super.renameTo(newName); // just to make it public
+    }
+
     @Override public void onDeleted(TopLevelItem item) throws IOException {
         ItemListener.fireOnDeleted(item);
         items.remove(item.getName());
@@ -206,6 +210,10 @@ public <T extends TopLevelItem> T createProject(Class<T> type, String name) thro
         return item;
     }
 
+    @Override public void remove(TopLevelItem item) throws IOException, IllegalArgumentException {
+        items.remove(item.getName());
+    }
+
     @Override public TopLevelItemDescriptor getDescriptor() {
         return Jenkins.getInstance().getDescriptorByType(DescriptorImpl.class);
     }

diff --git a/test/src/test/java/org/jvnet/hudson/test/MockFolderTest.java b/test/src/test/java/org/jvnet/hudson/test/MockFolderTest.java
@@ -25,9 +25,12 @@
 package org.jvnet.hudson.test;
 
 import hudson.model.FreeStyleProject;
-import org.junit.Test;
+import hudson.model.Item;
+import hudson.model.Items;
+import hudson.model.listeners.ItemListener;
 import static org.junit.Assert.*;
 import org.junit.Rule;
+import org.junit.Test;
 
 public class MockFolderTest {
 
@@ -39,4 +42,68 @@ public class MockFolderTest {
         assertEquals("dir/p", p.getFullName());
     }
 
+    @Test public void moving() throws Exception {
+        MockFolder top = j.createFolder("top");
+        FreeStyleProject p = top.createProject(FreeStyleProject.class, "p");
+        MockFolder sub = top.createProject(MockFolder.class, "sub");
+        assertNews("created=top created=top/p created=top/sub");
+        Items.move(p, j.jenkins);
+        assertEquals(j.jenkins, p.getParent());
+        assertEquals(p, j.jenkins.getItem("p"));
+        assertNull(top.getItem("p"));
+        assertNews("moved=p;from=top/p");
+        Items.move(p, sub);
+        assertEquals(sub, p.getParent());
+        assertEquals(p, sub.getItem("p"));
+        assertNull(j.jenkins.getItem("p"));
+        assertNews("moved=top/sub/p;from=p");
+        Items.move(sub, j.jenkins);
+        assertEquals(sub, p.getParent());
+        assertEquals(p, sub.getItem("p"));
+        assertEquals(j.jenkins, sub.getParent());
+        assertEquals(sub, j.jenkins.getItem("sub"));
+        assertNull(top.getItem("sub"));
+        assertNews("moved=sub;from=top/sub moved=sub/p;from=top/sub/p");
+        Items.move(sub, top);
+        assertNews("moved=top/sub;from=sub moved=top/sub/p;from=sub/p");
+        assertEquals(sub, top.getItem("sub"));
+        sub.renameTo("lower");
+        assertNews("renamed=top/lower;from=sub moved=top/lower;from=top/sub moved=top/lower/p;from=top/sub/p");
+        top.renameTo("upper");
+        assertNews("renamed=upper;from=top moved=upper;from=top moved=upper/lower;from=top/lower moved=upper/lower/p;from=top/lower/p");
+        assertEquals(p, sub.getItem("p"));
+        p.renameTo("j");
+        assertNews("renamed=upper/lower/j;from=p moved=upper/lower/j;from=upper/lower/p");
+        top.renameTo("upperz");
+        assertNews("renamed=upperz;from=upper moved=upperz;from=upper moved=upperz/lower;from=upper/lower moved=upperz/lower/j;from=upper/lower/j");
+        assertEquals(sub, top.getItem("lower"));
+        sub.renameTo("upperzee");
+        assertNews("renamed=upperz/upperzee;from=lower moved=upperz/upperzee;from=upperz/lower moved=upperz/upperzee/j;from=upperz/lower/j");
+        Items.move(sub, j.jenkins);
+        assertNews("moved=upperzee;from=upperz/upperzee moved=upperzee/j;from=upperz/upperzee/j");
+        assertEquals(p, j.jenkins.getItemByFullName("upperzee/j"));
+    }
+    private void assertNews(String expected) {
+        L l = j.jenkins.getExtensionList(ItemListener.class).get(L.class);
+        assertEquals(expected, l.b.toString().trim());
+        l.b.delete(0, l.b.length());
+    }
+    @TestExtension("moving") public static class L extends ItemListener {
+        final StringBuilder b = new StringBuilder();
+        @Override public void onCreated(Item item) {
+            b.append(" created=").append(item.getFullName());
+        }
+        @Override public void onDeleted(Item item) {
+            b.append(" deleted=").append(item.getFullName());
+        }
+        @Override public void onRenamed(Item item, String oldName, String newName) {
+            assertEquals(item.getName(), newName);
+            b.append(" renamed=").append(item.getFullName()).append(";from=").append(oldName);
+        }
+        @Override public void onLocationChanged(Item item, String oldFullName, String newFullName) {
+            assertEquals(item.getFullName(), newFullName);
+            b.append(" moved=").append(newFullName).append(";from=").append(oldFullName);
+        }
+    }
+
 }