Skip to content
Permalink
Browse files

[JENKINS-41124] Adding tests

- Also tests found a few very small holes, which are now fixed!
  • Loading branch information...
stephenc committed Jan 19, 2017
1 parent 9f4be16 commit be909fdb4b7b07d3f9f3520e6e34601ad95ffeed
@@ -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());
@@ -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;
@@ -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 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 @@
* 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);
}
}
}

0 comments on commit be909fd

Please sign in to comment.
You can’t perform that action at this time.