Skip to content

Commit

Permalink
[JENKINS-41124] Address code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@stephenc
stephenc committed Jan 21, 2017
1 parent ea7eb5d commit e151a7b
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 52 deletions.
5 changes: 2 additions & 3 deletions src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolder.java
View file
Expand Up @@ -471,7 +471,7 @@ public boolean replaceActions(@Nonnull Class<? extends Action> clazz, Action a)
*
* @param modulesDir Directory that contains sub-directories for each child item.
*/
// TODO replace with ItemGroup.loadChildren once baseline core has JENKINS-41222 merged
// TODO replace with ItemGroupMixIn.loadChildren once baseline core has JENKINS-41222 merged
public static <K, V extends TopLevelItem> Map<K, V> loadChildren(AbstractFolder<V> parent, File modulesDir,
Function1<? extends K, ? super V> key) {
CopyOnWriteMap.Tree<K, V> configurations = new CopyOnWriteMap.Tree<K, V>();
Expand Down Expand Up @@ -596,8 +596,7 @@ public boolean accept(File child) {
}
item.onLoad(parent, name);
} else {
Logger.getLogger(AbstractFolder.class.getName())
.log(Level.WARNING, "could not find file " + xmlFile.getFile());
LOGGER.log(Level.WARNING, "could not find file " + xmlFile.getFile());
continue;
}
} else {
Expand Down
10 changes: 6 additions & 4 deletions src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolderDescriptor.java
View file
Expand Up @@ -212,10 +212,10 @@ public boolean isLookAndFeelConfigurable(AbstractFolder<?> folder) {
* 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
* <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
* require inference of the un-mangled name from the filesystem, which may or may not match the un-mangled
* 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
Expand All @@ -225,14 +225,16 @@ public boolean isLookAndFeelConfigurable(AbstractFolder<?> folder) {
* 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>
* If you implement a {@link ChildNameGenerator} it is strongly recommended to return a singleton for performance
* reasons.
*
* @param <I> A wildcard parameter to assist type matching.
* @return a <strong>SINGLETON</strong> instance of {@link ChildNameGenerator} for all instanced of the concrete
* @return a (ideally singleton) instance of {@link ChildNameGenerator} for all instances 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
// TODO move the name mangling code from branch-api to this plugin so that everyone can use it
@CheckForNull
public <I extends TopLevelItem> ChildNameGenerator<AbstractFolder<I>,I> childNameGenerator() {
return null;
Expand Down
103 changes: 58 additions & 45 deletions src/main/java/com/cloudbees/hudson/plugins/folder/ChildNameGenerator.java
View file
Expand Up @@ -41,7 +41,7 @@
import jenkins.model.TransientActionFactory;

/**
* Provides a way for a {@link ComputedFolder} may need to break the association between the directory names on disk
* Provides a way for a {@link ComputedFolder} to break the association between the directory names on disk
* that are used to store its items and the {@link Item#getName()} which is used to create the URL of the item.
* <p>
* <strong>NOTE:</strong> if you need to implement this functionality, you need to ensure that users cannot rename
Expand All @@ -62,14 +62,30 @@
* for is missing.</li>
* </ul>
*
* 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} (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.
*
* @param <P> the type of {@link AbstractFolder}.
* @param <I> the type of {@link TopLevelItem} within the folder.
* @since 5.17
*/
// TODO migrate this functionality (by changing the base class) into core once baseline Jenkins has JENKINS-41222 merged
public abstract class ChildNameGenerator<P extends AbstractFolder<I>, I extends TopLevelItem> {
/**
* The name of the file that contains the actual name of the child item.
* The name of the file that contains the actual name of the child item. This file is to allow a Jenkins
* Administrator to determine which child is which when dealing with a folder containing child names that have
* been mangled.
* <p>
* If there is nothing else to go on, this file will be used in preference to the child directory name, but as it
* is too easy for users to mistakenly think changing the contents of the file will rename the child (which could
* cause data loss for the computed folder's child) it is better for implementations to store the definitive
* ideal name in a {@link JobProperty}, {@link Action} or equivalent that is attached directly to the {@link Item}.
*/
public static final String CHILD_NAME_FILE = "name-utf8.txt";

Expand Down Expand Up @@ -110,28 +126,23 @@ private static final void afterItemCreated(@Nonnull Trace trace) {
/**
* Looks up the {@link Item} to see if we stored the ideal name before invoking the constructor that is having
* on-disk side-effects before the object has escaped {@link #beforeCreateItem(AbstractFolder, String, String)}
* @param parent
* @param item
* @return
* @param parent the parent within which the item is being created.
* @param item the partially created item.
* @return the ideal name of the item.
*/
@CheckForNull
protected final String idealNameFromItem(@Nonnull P parent, @Nonnull I item) {
String itemName = item.getName();
if (itemName == null) return null;
if (itemName == null) {
return null;
}
synchronized (idealNames) {
return idealNames.get(new Trace(parent, itemName));
}
}

/**
* 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} (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.
* Infers the {@link Item#getName()} from the {@link Item} instance itself.
*
* Challenges include:
* <ul>
Expand All @@ -142,38 +153,30 @@ protected final String idealNameFromItem(@Nonnull P parent, @Nonnull I item) {
* </ul>
* @param parent the parent within which the item is being loaded.
* @param item the partially loaded item (take care what methods you call, the item will not have a reference to
* its parent.
* its parent).
* @return the name of the item.
*/
@CheckForNull
public abstract String itemNameFromItem(@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} (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
* {@link Item} is being loaded from.
* Infers the directory name in which the {@link Item} instance itself should be stored.
*
* Challenges include:
* <ul>
* <li>The only really filesystem safe characters are {@code A-Za-z0-9_.-}</li>
* <li>Because of Windows and allowing for users to migrate their Jenkins from unix to windows and vice-versa,
* some names are reserved names under Windows:
* {@code AUX, COM1, COM2, ..., COM9, CON, LPT1, LPT2, ..., LPT9, NUL, PRN} plus all case variations of these
* names plus the variants where a single {@code .} is appended, you need to map those to something else</li>
* <li>Don't make the filenames too long. Try to keep them under 32 characters. If you can go smaller, even
* better.</li>
* <li>Get it right first time</li>
* <li>The only really filesystem safe characters are {@code A-Za-z0-9_.-}</li>
* <li>Because of Windows and allowing for users to migrate their Jenkins from Wnix to Windows and vice-versa,
* some names are reserved names under Windows:
* {@code AUX, COM1, COM2, ..., COM9, CON, LPT1, LPT2, ..., LPT9, NUL, PRN} plus all case variations of these
* names plus the variants where a single {@code .} is appended, you need to map those to something else</li>
* <li>Don't make the filenames too long. Try to keep them under 32 characters. If you can go smaller, even
* better.</li>
* <li>Get it right the first time</li>
* </ul>
*
* @param parent the parent within which the item is being loaded.
* @param item the partially loaded item (take care what methods you call, the item will not have a reference to
* its parent.
* its parent).
* @return the filesystem safe mangled equivalent name of the item.
*/
@CheckForNull
Expand All @@ -195,40 +198,50 @@ protected final String idealNameFromItem(@Nonnull P parent, @Nonnull I item) {
* </ul>
* @param parent the parent within which the item is being loaded.
* @param legacyDirName the directory name that we are loading an item from.
* @return
* @return the name of the item.
*/
@Nonnull
public abstract String itemNameFromLegacy(@Nonnull P parent, @Nonnull String legacyDirName);

/**
* {@link #dirNameFromLegacy(AbstractFolder, String)} could not help, we are loading the item for the first
* {@link #dirNameFromItem(AbstractFolder, TopLevelItem)} could not help, we are loading the item for the first
* time since the {@link ChildNameGenerator} was enabled for the parent folder type, this method's mission is
* to pretend the {@code legacyDirName} is the "mostly correct" name and turn this into the filesystem safe
* mangled equivalent name to use going forward.
*
* Challenges include:
* <ul>
* <li>The only really filesystem safe characters are {@code A-Za-z0-9_.-}</li>
* <li>Because of Windows and allowing for users to migrate their Jenkins from unix to windows and vice-versa,
* some names are reserved names under Windows:
* {@code AUX, COM1, COM2, ..., COM9, CON, LPT1, LPT2, ..., LPT9, NUL, PRN} plus all case variations of these
* names plus the variants where a single {@code .} is appended, you need to map those to something else</li>
* <li>Don't make the filenames too long. Try to keep them under 32 characters. If you can go smaller, even
* better.</li>
* <li>Get it right first time</li>
* <li>The only really filesystem safe characters are {@code A-Za-z0-9_.-}</li>
* <li>Because of Windows and allowing for users to migrate their Jenkins from Unix to Windows and vice-versa,
* some names are reserved names under Windows:
* {@code AUX, COM1, COM2, ..., COM9, CON, LPT1, LPT2, ..., LPT9, NUL, PRN} plus all case variations of these
* names plus the variants where a single {@code .} is appended, you need to map those to something else</li>
* <li>Don't make the filenames too long. Try to keep them under 32 characters. If you can go smaller, even
* better.</li>
* <li>Get it right the first time</li>
* </ul>
*
* @param parent the parent within which the item is being loaded.
* @param legacyDirName the directory name that we are loading an item from.
* @return
* @return the filesystem safe mangled equivalent name of the item.
*/
@Nonnull
public abstract String dirNameFromLegacy(@Nonnull P parent, @Nonnull String legacyDirName);

/**
* Record the ideal name inferred in the item when it was missing and has been inferred from the legacy directory
* name.
*
* @param parent the parent.
* @param item the item.
* @param legacyDirName the name of the directory that the item was loaded from.
* @throws IOException if the ideal name could not be attached to the item.
*/
public abstract void recordLegacyName(P parent, I item, String legacyDirName) throws IOException;

/**
* Traces the creation of a new Item in a folder.
* Traces the creation of a new {@link Item} in a folder. Use
* {@link ChildNameGenerator#beforeCreateItem(AbstractFolder, String, String)} to get the instance.
*/
public static final class Trace implements Closeable {
/**
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/cloudbees/hudson/plugins/folder/ChildNameGeneratorTest.java
View file
Expand Up @@ -354,6 +354,7 @@ public static String mangle(String s) {

@SuppressWarnings({"unchecked", "rawtypes"})
public static class ComputedFolderImpl extends ComputedFolder<FreeStyleProject> {
// TODO refactor the ChildNameGeneratorTests to remove duplication of most of this class

private Set<String> fatalKids = new TreeSet<String>();

Expand Down

0 comments on commit e151a7b

Please sign in to comment.