Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXED JENKINS-28843] Stored the unique value in the job configuration #1

Merged
merged 15 commits into from Jun 11, 2015
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
@@ -0,0 +1,4 @@
/target/
/.classpath
/.project
/.settings
34 changes: 32 additions & 2 deletions pom.xml
Expand Up @@ -17,16 +17,38 @@
<connection>scm:git:ssh://github.com/jenkinsci/unique-id-plugin.git</connection>
<developerConnection>scm:git:ssh://git@github.com/jenkinsci/unique-id-plugin.git</developerConnection>
<url>https://github.com/jenkinsci/unique-id-plugin</url>
<tag>unique-id-1.2</tag>
</scm>

<properties>
<maven.compiler.source>1.6</maven.compiler.source>
<maven.compiler.target>1.6</maven.compiler.target>
</properties>

<dependencies>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>cloudbees-folder</artifactId>
<version>4.0</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.jenkins-ci</groupId>
<artifactId>test-annotations</artifactId>
<version>1.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.12</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-core</artifactId>
<version>1.3</version>
<scope>test</scope>
</dependency>
</dependencies>

<!-- get every artifact through repo.jenkins-ci.org, which proxies all the artifacts that we need -->
Expand All @@ -48,7 +70,15 @@
<plugins>
<plugin>
<artifactId>maven-release-plugin</artifactId>
<version>2.4</version>
<version>2.5.1</version>
</plugin>
<plugin>
<groupId>org.jenkins-ci.tools</groupId>
<artifactId>maven-hpi-plugin</artifactId>
<extensions>true</extensions>
<configuration>
<compatibleSinceVersion>1.3</compatibleSinceVersion>
</configuration>
</plugin>
</plugins>
</build>
Expand Down
43 changes: 37 additions & 6 deletions src/main/java/org/jenkinsci/plugins/uniqueid/IdStore.java
@@ -1,16 +1,27 @@
package org.jenkinsci.plugins.uniqueid;

import hudson.ExtensionPoint;

import jenkins.model.Jenkins;

import java.io.UnsupportedEncodingException;
import java.util.UUID;

import javax.annotation.Nullable;

import org.apache.commons.codec.binary.Base64;

/**
* An abstraction to persistently store and retrieve unique id's
* for various Jenkins model objects.
*
* These keys are guaranteed to be unique with a Jenkins
* and immutable across the lifetime of the given object.
*
* Implementations should not store the ID inside any specific item configuration as it is
* common for users top copy items either through the UI or manually and this will cause the
* IDs to become non-unique.
*
*
* @param <T>
*/
Expand All @@ -26,17 +37,19 @@ public IdStore (Class<T> forType) {
* Creates an unique id for the given object.
* Subsequent calls are idempotent.
*
* @param object
* @param object the object to make the id for.
* @throws Exception if we could not store the unique ID for some reason.
*/
public abstract void make(T object);
public abstract void make(T object) throws Exception;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It causes a binary compatibility issue, but I doubt anybody uses the plugin's API actively. Let's bump the major version digit BTW

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - but the alternative is to make this return a boolean and have callers check.
Neither are good in my opinion - I could change this to be IOException (but that is specific to our implementation )


/**
* Get the id for this given object.
* @param object
* @return the id or null if none assigned.
* @return the id or {@code null} if none assigned.
* @throws Exception if we could not retrieve the unique ID for some reason.
*/
@Nullable
public abstract String get(T object);
public abstract String get(T object) throws Exception;

public boolean supports(Class clazz) {
return type.isAssignableFrom(clazz);
Expand All @@ -62,8 +75,9 @@ public static <C> IdStore<C> forClass(Class<C> clazz) {
* Convenience method which makes the id for the given object.
*
* @throws java.lang.IllegalArgumentException if the type is not supported.
* @throws Exception if we could not store the unique ID for some reason.
*/
public static void makeId(Object object) {
public static void makeId(Object object) throws IllegalArgumentException, Exception {
IdStore store = forClass(object.getClass());
if (store == null) {
throw new IllegalArgumentException("Unsupported type: " + object.getClass().getName());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more like an IllegalStateException to me i.e. something trying to make an ID for an object for which there's no IdStore impl.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous API was IllegalArgumeException - so I do not want to change all callers.

Expand All @@ -76,8 +90,9 @@ public static void makeId(Object object) {
* Convenience method which retrieves the id for the given object.
*
* @throws java.lang.IllegalArgumentException if the type is not supported.
* @throws Exception if we could not store the unique ID for some reason.
*/
public static String getId(Object object) {
public static String getId(Object object) throws IllegalArgumentException, Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a fallback to a legacy store is required here (as I've understood initial test suites)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - there should be no fallback at all.
The legacy store must be migrated at first run and then removed if it exists.

IdStore store = forClass(object.getClass());
if (store == null) {
throw new IllegalArgumentException("Unsupported type: " + object.getClass().getName());
Expand All @@ -86,4 +101,20 @@ public static String getId(Object object) {
}
}

/**
* Generates a new unique ID.
* Subclasses do not need to use this to create unique IDs and are free to create IDs by other methods.
* @return a string that should be unique against all jenkins instances.
*/
protected static String generateUniqueID() {
try {
return Base64.encodeBase64String(UUID.randomUUID().toString().getBytes("UTF-8")).substring(0, 30);
} catch (UnsupportedEncodingException e) {
// impossible condition
Error err = new InternalError("The JLS mandates UTF-8 yet it is not available on this JVM. Your JVM is broken.");
err.initCause(e);
throw err;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this just be throw new InternalError("...", e); ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in the ancient JDK that this compiles against :-(

}
}

}
@@ -1,37 +1,47 @@
package org.jenkinsci.plugins.uniqueid.impl;

import com.cloudbees.hudson.plugins.folder.Folder;
import com.cloudbees.hudson.plugins.folder.FolderProperty;
import com.cloudbees.hudson.plugins.folder.FolderPropertyDescriptor;
import hudson.Extension;
import hudson.model.Action;
import hudson.model.Actionable;
import org.jenkinsci.plugins.uniqueid.IdStore;
import hudson.util.DescribableList;

import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.logging.Level;
import java.util.Iterator;
import java.util.logging.Logger;

import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import com.cloudbees.hudson.plugins.folder.FolderProperty;
import com.cloudbees.hudson.plugins.folder.FolderPropertyDescriptor;
import com.cloudbees.hudson.plugins.folder.Folder;

/**
* Stores ids for folders as a {@link FolderIdProperty}
* @deprecated {@see PersistenceRootIdStore}
*/
@Extension(optional = true)
public class FolderIdStore extends IdStore<Folder> {
@Deprecated
@Restricted(NoExternalUse.class)
public class FolderIdStore extends LegacyIdStore<Folder> {
public FolderIdStore() {
super(Folder.class);
}

@Override
public void make(Folder folder) {
if (folder.getProperties().get(FolderIdProperty.class) == null) {
try {
folder.addProperty(new FolderIdProperty());
} catch (IOException e) {
LOGGER.log(Level.SEVERE, "Failed to add property",e);
public void remove(Folder folder) throws IOException {
DescribableList<FolderProperty<?>,FolderPropertyDescriptor> properties = folder.getProperties();

for (Iterator<FolderProperty<?>> itr = properties.iterator(); itr.hasNext(); ) {
FolderProperty<?> prop = itr.next();

if (prop instanceof FolderIdProperty) {
itr.remove();
}
}
folder.save();
}

@Override
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/org/jenkinsci/plugins/uniqueid/impl/Id.java
Expand Up @@ -2,15 +2,22 @@

import hudson.model.Action;
import hudson.model.Actionable;

import org.apache.commons.codec.binary.Base64;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.Nullable;

import java.util.UUID;
import java.util.logging.Logger;

/**
* An action which stores an id.
* @deprecated users should not use this as it ties the ID explicitly to the item and as will not work when copying items (for example).
*/
@Deprecated
@Restricted(NoExternalUse.class)
class Id implements Action {


Expand All @@ -36,7 +43,13 @@ public String getId() {
return id;
}


/**
* @deprecated Sub classes should not use this as it stores the ID in the actionable item.
* @return
*/
@Nullable
@Deprecated
protected static String getId(Actionable actionable) {
Id id = actionable.getAction(Id.class);
if (id != null) {
Expand Down
@@ -0,0 +1,113 @@
package org.jenkinsci.plugins.uniqueid.impl;

import hudson.Extension;
import hudson.init.InitMilestone;
import hudson.init.Initializer;
import hudson.model.Item;
import hudson.model.PersistenceRoot;
import hudson.model.Job;
import hudson.model.Run;
import hudson.util.RunList;

import jenkins.model.Jenkins;

import java.io.File;
import java.io.IOException;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

import javax.annotation.Nonnull;

import org.jenkinsci.plugins.uniqueid.implv2.PersistenceRootIdStore;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* Converts legacy UniqueIDs that are stored inside a Folder/Job/Run configuration to UniqueIDs that are stored alongside the Folder/Job/Run.
*
*/
@Restricted(NoExternalUse.class)
@Extension
public class IdStoreMigratorV1ToV2 {

private static Logger LOGGER = Logger.getLogger(IdStoreMigratorV1ToV2.class.getName());

/**
* Migrates any IDs stored in Folder/Job/Run configuration
* @throws IOException
*/

@Initializer(after=InitMilestone.JOB_LOADED, before=InitMilestone.COMPLETED, fatal=true)
public static void migrateIdStore() throws IOException {
Jenkins jenkins = Jenkins.getInstance();
if (jenkins == null) {
throw new IllegalStateException("Jenkins is null, so it is impossible to migrate the IDs");
}
File marker = new File(jenkins.getRootDir(), "unique-id-migration.txt");
if (marker.exists()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileBoolean

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next time...

LOGGER.log(Level.INFO, "Migration of IDStore already perfomed, so skipping migration.");
return;
}
LOGGER.log(Level.INFO, "Starting migration of IDs");

performMigration(jenkins);

LOGGER.log(Level.INFO, "Finished migration of IDs");
if (!marker.createNewFile()) {
throw new IOException("Failed to record the completion of the IDStore Migration. " +
"This will cause performance issues on subsequent startup. " +
"Please create an empty file at '" + marker.getCanonicalPath() + "'");
}
}

@SuppressWarnings("unchecked")
static void performMigration(@Nonnull Jenkins jenkins) {
List<Item> allItems = jenkins.getAllItems();

for (Item item : allItems) {
// can only be Folder or Job here (not a run) - and these both implement PersistenceRoot
if (item instanceof PersistenceRoot) {
migrate((PersistenceRoot) item);
}
else {
LOGGER.log(Level.WARNING, "Expected item of type Folder or Job which implement PersistenceRoot, but got a {0} so can not migrate the IdStore for this item",
item.getClass().getName());
}

if (item instanceof Job) {
// need to migrate the RunIDs if they exist.
Job<? extends Job, ? extends Run> job = (Job<? extends Job, ? extends Run>) item;
RunList<? extends Run> builds = job.getBuilds();
for (Run build : builds) {
migrate(build);
}
}
}
}

private static void migrate(PersistenceRoot pr) {
LOGGER.log(Level.FINE, "migrating {0}" , pr.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove .toString()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in followup #3

try {
String id = LegacyIdStore.getId(pr);
if (id != null) {
PersistenceRootIdStore.create(pr, id);
LegacyIdStore.removeId(pr);
}
} catch (IOException ex) {
// need to rethrow (but add some context first) otherwise the migration will continue to run
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what behavior is preferable. Probably it's better to continue the migration.
If we introduce new methods, probably it's better to avoid runtime exceptions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we continue that the user can end up with a broken analytics system... and mutilple Items that contain non-unique ID

// and it will not have migrated everything :-(
throw new IDStoreMigrationException("Failure whilst migrating " + pr.toString(), ex);
}
}

/**
* Exception to indicate a failure to migrate the IDStore.
*/
private static class IDStoreMigrationException extends RuntimeException {

public IDStoreMigrationException(String message, Throwable cause) {
super(message,cause);
}
}
}