[JENKINS-41124] Adding tests

- Also tests found a few very small holes, which are now fixed!
jenkinsci · Jan 19, 2017 · be909fd · be909fd
1 parent 9f4be16
commit be909fd
Show file tree

Hide file tree

Showing 9 changed files with 1,788 additions and 16 deletions.
diff --git a/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolder.java b/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolder.java
@@ -539,12 +539,24 @@ public boolean accept(File child) {
                     if (xmlFile.exists()) {
                         item = (V) xmlFile.read();
                         String name;
+                        boolean itemNeedsSave = false;
                         if (childNameGenerator == null) {
                             name = subdir.getName();
                         } else {
                             String dirName = childNameGenerator.dirNameFromItem(parent, item);
                             if (dirName == null) {
                                 dirName = childNameGenerator.dirNameFromLegacy(parent, childName);
+                                BulkChange bc = new BulkChange(item); // suppress any attempt to save as parent not set
+                                try {
+                                    childNameGenerator.recordLegacyName(parent, item, childName);
+                                    itemNeedsSave = true;
+                                } catch (IOException e) {
+                                    LOGGER.log(Level.WARNING, "Ignoring {0} as could not record legacy name",
+                                            subdir);
+                                    continue;
+                                } finally {
+                                    bc.abort();
+                                }
                             }
                             if (!subdir.getName().equals(dirName)) {
                                 File newSubdir = parent.getRootDirFor(dirName);
@@ -567,20 +579,30 @@ public boolean accept(File child) {
                             if (name == null) {
                                 name = childNameGenerator.itemNameFromLegacy(parent, childName);
                                 FileUtils.writeStringToFile(nameFile, name, "UTF-8");
-                            } else if (!childName.equals(name) || legacy) {
-                                FileUtils.writeStringToFile(nameFile, name, "UTF-8");
-                            }
-                            if (!dirName.equals(name) && item instanceof AbstractItem
-                                    && ((AbstractItem) item).getDisplayNameOrNull() == null) {
-                                BulkChange bc = new BulkChange(item);
+                                BulkChange bc = new BulkChange(item); // suppress any attempt to save as parent not set
                                 try {
-                                    ((AbstractItem) item).setDisplayName(childName);
+                                    childNameGenerator.recordLegacyName(parent, item, childName);
+                                    itemNeedsSave = true;
+                                } catch (IOException e) {
+                                    LOGGER.log(Level.WARNING, "Ignoring {0} as could not record legacy name",
+                                            subdir);
+                                    continue;
                                 } finally {
                                     bc.abort();
                                 }
+                            } else if (!childName.equals(name) || legacy) {
+                                FileUtils.writeStringToFile(nameFile, name, "UTF-8");
                             }
                         }
                         item.onLoad(parent, name);
+                        if (itemNeedsSave) {
+                            try {
+                                item.save();
+                            } catch (IOException e) {
+                                LOGGER.log(Level.WARNING, "Could not update {0} after applying folder naming rules",
+                                        item.getFullName());
+                            }
+                        }
                     } else {
                         Logger.getLogger(AbstractFolder.class.getName())
                                 .log(Level.WARNING, "could not find file " + xmlFile.getFile());

diff --git a/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolderDescriptor.java b/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolderDescriptor.java
@@ -24,18 +24,22 @@
 
 package com.cloudbees.hudson.plugins.folder;
 
+import com.cloudbees.hudson.plugins.folder.computed.ComputedFolder;
 import com.cloudbees.hudson.plugins.folder.health.FolderHealthMetricDescriptor;
 import hudson.model.DescriptorVisibilityFilter;
+import hudson.model.Item;
 import hudson.model.TopLevelItem;
 import hudson.model.TopLevelItemDescriptor;
 import hudson.views.ViewsTabBar;
+import java.io.File;
 import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
 import jenkins.model.Jenkins;
+import jenkins.model.ProjectNamingStrategy;
 import org.apache.commons.jelly.Script;
 import org.apache.commons.jelly.XMLOutput;
 import org.jenkins.ui.icon.IconSpec;
@@ -185,6 +189,50 @@ public boolean isLookAndFeelConfigurable(AbstractFolder<?> folder) {
         return isIconConfigurable() || (isTabBarConfigurable() && folder.getFolderViews().isTabBarModifiable()) || (folder.getViews().size() > 1 && folder.getFolderViews().isPrimaryModifiable());
     }
 
+    /**
+     * Folders, especially computed folders, may have requirements for using a different on-disk file name for child
+     * items than the url-segment name. Typically this is to work around filesystem naming rules.
+     * Regular folders typically would leave the naming of child items to {@link ProjectNamingStrategy} and thereby
+     * prevent users from creating child items with names that do not comply with the {@link ProjectNamingStrategy}.
+     * <p>
+     * <strong>However,</strong> {@link ComputedFolder} instances may not have that luxury. The children of a
+     * {@link ComputedFolder} may have names that come from an external system and the matching item must be created,
+     * always. The obvious solution is that the {@link ComputedFolder} should mangle the supplied {@link Item#getName()}
+     * but the side-effect is that the URLs of the child items will now be mangled. Additionally the filename
+     * requirements can be rather onerous. Here is the most portable list of filename specification:
+     * <ul>
+     *     <li>Assume case insensitive</li>
+     *     <li>Assume no filename can be longer than 32 characters</li>
+     *     <li>Assume that only the characters {@code A-Za-z0-9_.-} are available</li>
+     *     <li>Assume that there are some special reserved names such as {@code .} and {@code ..}</li>
+     *     <li>Assume that there are some problematic names to avoid such as {@code AUX}, {@code CON}, {@code NUL}, etc.
+     *         See <a href="https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx">
+     *         Microsoft's page on "Naming Files, Paths, and Namespaces"</a>
+     *         (What's that you say, "Oh but we only run on Linux", perhaps but users may want to migrate from one OS
+     *         to an other by <strong>moving</strong> their {@code JENKINS_HOME} (or even parts of it) so if you are
+     *         mangling names, be sure to ensure that the mangled name is the same on all OSes
+     *     </li>
+     *     <li><a href="http://unicode.org/reports/tr15/">NFC vs NFD</a> may be a concern as different filesystems
+     *     apply different rules and normalization to the filenames. This is primarily a concern when migrating from
+     *     before having a {@link ChildNameGenerator} to having a {@link ChildNameGenerator} as the migration will
+     *     require inference of the unmangled name from the filesystem, which may or may not match the unmangeld
+     *     name from the source of the computation. Now POSIX does not specify how the filesystem is supposed to handle
+     *     encoding of filenames and there can be strange behaviours, e.g.
+     *     <a href="http://stackoverflow.com/a/32663908/1589700">{@link File#listFiles()} is rumoured to always return
+     *     NFC on OS-X</a>
+     *     </li>
+     * </ul>
+     * The {@link ChildNameGenerator} at least allows an {@link AbstractFolder} to apply an on-disk naming strategy
+     * that differs from the names used for the URLs.
+     * <p>
+     * <strong>IF YOU IMPLEMENT A {@link ChildNameGenerator} YOU MUST RETURN A SINGLETON FROM THIS METHOD</strong>
+     *
+     * @param <I> A wildcard parameter to assist type matching.
+     * @return a <strong>SINGLETON</strong> instance of {@link ChildNameGenerator} for all instanced of the concrete
+     * {@link AbstractFolder} class or {@code null} if no name mangling will be performed.
+     * @since 5.17
+     */
+    // TODO figure out how one could un-wind name mangling if one ever wanted to
     @CheckForNull
     public <I extends TopLevelItem> ChildNameGenerator<AbstractFolder<I>,I> childNameGenerator() {
         return null;

diff --git a/src/main/java/com/cloudbees/hudson/plugins/folder/ChildNameGenerator.java b/src/main/java/com/cloudbees/hudson/plugins/folder/ChildNameGenerator.java
@@ -32,10 +32,13 @@
 import hudson.model.ItemGroup;
 import hudson.model.JobProperty;
 import hudson.model.TopLevelItem;
+import java.io.Closeable;
+import java.io.IOException;
 import java.util.Map;
 import java.util.WeakHashMap;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import jenkins.model.TransientActionFactory;
 
 /**
  * Provides a way for a {@link ComputedFolder} may need to break the association between the directory names on disk
@@ -46,7 +49,8 @@
  * <p>
  * Challenges:
  * <ul>
- * <li>See the notes on {@link #itemNameFromItem(AbstractFolder, TopLevelItem)} and {@link #dirNameFromItem(AbstractFolder, TopLevelItem)} regarding the constraints on how to name things</li>
+ * <li>See the notes on {@link #itemNameFromItem(AbstractFolder, TopLevelItem)} and
+ * {@link #dirNameFromItem(AbstractFolder, TopLevelItem)} regarding the constraints on how to name things</li>
  * <li>There are some items which need the {@link Item#getRootDir()} during construction (those are bold evil item types
  * that leak side-effects, you should fix them if you find them). While you wait for them to be fixed you will need
  * to work-arround the issue by ensuring that you call {@link #beforeCreateItem(AbstractFolder, String, String)}
@@ -69,7 +73,7 @@ public abstract class ChildNameGenerator<P extends AbstractFolder<I>, I extends
      */
     public static final String CHILD_NAME_FILE = "name-utf8.txt";
 
-    private final Map<Trace,String> idealNames = new WeakHashMap<Trace,String>();
+    private static final Map<Trace,String> idealNames = new WeakHashMap<Trace,String>();
 
     /**
      * Work-around helper method to "fix" {@link Item} constructors that have on-disk side-effects and therefore
@@ -79,10 +83,13 @@ public abstract class ChildNameGenerator<P extends AbstractFolder<I>, I extends
      *                 the second parameter of {@link AbstractItem#AbstractItem(ItemGroup, String)}. This one would be
      *                 the one with URL path segment escaping.
      * @param idealName the original name before whatever URL path segment escaping you applied
-     * @return the {@link Trace} to keep track of when we can remove the memory of the creation process.
+     * @return the {@link Trace} to keep track of when we can remove the memory of the creation process. Please
+     * {@link Trace#close()} the trace after the item is created.
      */
     @Nonnull
-    public final Trace beforeCreateItem(@Nonnull P project, @Nonnull String itemName, @Nonnull String idealName) {
+    public static final Trace beforeCreateItem(@Nonnull AbstractFolder<?> project,
+                                               @Nonnull String itemName,
+                                               @Nonnull String idealName) {
         final Trace trace = new Trace(project, itemName);
         synchronized (idealNames) {
             idealNames.put(trace, idealName);
@@ -91,10 +98,10 @@ public final Trace beforeCreateItem(@Nonnull P project, @Nonnull String itemName
     }
 
     /**
-     * Clean up for a creation {@link Trace}. Not strictly required to be called, but nice implementations will do this.
+     * Clean up for a creation {@link Trace}. Not strictly required, but nice implementations will do this via {@link Trace#close()}.
      * @param trace the trace.
      */
-    public final void afterItemCreated(@Nonnull Trace trace) {
+    private static final void afterItemCreated(@Nonnull Trace trace) {
         synchronized (idealNames) {
             idealNames.remove(trace);
         }
@@ -119,7 +126,9 @@ protected final String idealNameFromItem(@Nonnull P parent, @Nonnull I item) {
     /**
      * Infers the {@link Item#getName()} from the {@link Item} instance itself. For a valid implementation, the
      * {@link ComputedFolder} using this {@link ChildNameGenerator} will be attaching into the {@link Item} the
-     * actual name, typically via a {@link JobProperty} or {@link Action}. This method's task is to find that
+     * actual name, typically via a {@link JobProperty} or {@link Action} (beware {@link TransientActionFactory}
+     * implementations may want to invoke {@link Item#getRootDir()} which will trigger a stack overflow though, so
+     * safer to stick with the {@link JobProperty} or equivalent). This method's task is to find that
      * and return the name stored within or {@code null} if that information is missing (in which case
      * {@link #itemNameFromLegacy(AbstractFolder, String)} will be called to try and infer the name from the
      * disk name that the {@link Item} is being loaded from.
@@ -142,7 +151,9 @@ protected final String idealNameFromItem(@Nonnull P parent, @Nonnull I item) {
     /**
      * Infers the directory name in which the {@link Item} instance itself should be stored. For a valid
      * implementation, the {@link ComputedFolder} using this {@link ChildNameGenerator} will be attaching into the
-     * {@link Item} the actual name, typically via a {@link JobProperty} or {@link Action}. This method's task is to
+     * {@link Item} the actual name, typically via a {@link JobProperty} or {@link Action} (beware
+     * {@link TransientActionFactory} implementations may want to invoke {@link Item#getRootDir()} which will trigger
+     * a stack overflow though, so safer to stick with the {@link JobProperty} or equivalent) . This method's task is to
      * find that and return the filesystem safe mangled equivalent name stored within or {@code null} if that
      * information is missing (in which case {@link #dirNameFromLegacy(AbstractFolder, String)}
      * will be called to try and infer the filesystem safe mangled equivalent name from the disk name that the
@@ -214,10 +225,12 @@ protected final String idealNameFromItem(@Nonnull P parent, @Nonnull I item) {
     @Nonnull
     public abstract String dirNameFromLegacy(@Nonnull P parent, @Nonnull String legacyDirName);
 
+    public abstract void recordLegacyName(P parent, I item, String legacyDirName) throws IOException;
+
     /**
      * Traces the creation of a new Item in a folder.
      */
-    public static final class Trace {
+    public static final class Trace implements Closeable {
         /**
          * The folder.
          */
@@ -265,5 +278,13 @@ public int hashCode() {
             result = 31 * result + itemName.hashCode();
             return result;
         }
+
+        /**
+         * {@inheritDoc}
+         */
+        @Override
+        public void close() {
+            afterItemCreated(this);
+        }
     }
 }