Skip to content
Permalink
Browse files

[JENKINS-26781] Merging #1563.

  • Loading branch information...
jglick committed Apr 15, 2015
2 parents bc8c0df + f3070d9 commit b688047877cf6b735ee17cd3d22436f5b4219b03
@@ -55,6 +55,9 @@
<!-- Record your changes in the trunk here. -->
<div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
<ul class=image>
<li class=bug>
Since 1.598 overrides of <code>Descriptor.getId</code> were not correctly handled by form binding, breaking at least the CloudBees Templates plugin.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-26781">issue 26781</a>)
<li class=bug>
Archiving of large artifacts. Tar implementation cannot handle files having a size >8GB.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-10629">issue 10629</a>)
@@ -109,6 +109,7 @@ protected DescriptorExtensionList(Jenkins jenkins, Class<T> describableType) {
*
* @param fqcn
* Fully qualified name of the descriptor, not the describable.
* @deprecated {@link Descriptor#getId} is supposed to be used for new code, not the descriptor class name.
*/
public D find(String fqcn) {
return Descriptor.find(this,fqcn);
@@ -290,6 +290,7 @@ public synchronized void doDoCreateItem( StaplerRequest req, StaplerResponse rsp
JSONObject formData = req.getSubmittedForm();
formData.put("name", fixedName);

// TODO type is probably NodeDescriptor.id but confirm
Node result = NodeDescriptor.all().find(type).newInstance(req, formData);
app.addNode(result);

@@ -909,14 +909,23 @@ private URL getStaticHelpUrl(Klass<?> c, String suffix) {
if (formData!=null) {
for (Object o : JSONArray.fromObject(formData)) {
JSONObject jo = (JSONObject)o;
String kind = jo.optString("$class", null);
if (kind == null) {
// Legacy: Remove once plugins have been staged onto $class
kind = jo.getString("kind");
Descriptor<T> d = null;
// 'kind' and '$class' are mutually exclusive (see class-entry.jelly), but to be more lenient on the reader side,
// we check them both anyway. 'kind' (which maps to ID) is more unique than '$class', which can have multiple matching
// Descriptors, so we prefer 'kind' if it's present.
String kind = jo.optString("kind", null);
if (kind != null) {
d = findById(descriptors, kind);
}
if (d == null) {
kind = jo.getString("$class");
d = findByDescribableClassName(descriptors, kind);
if (d == null) d = findByClassName(descriptors, kind);
}
Descriptor<T> d = find(descriptors, kind);
if (d != null) {
items.add(d.newInstance(req, jo));
} else {
LOGGER.warning("Received unexpected formData for descriptor " + kind);
}
}
}
@@ -925,22 +934,58 @@ private URL getStaticHelpUrl(Klass<?> c, String suffix) {
}

/**
* Finds a descriptor from a collection by its class name.
* Finds a descriptor from a collection by its ID.
* @param id should match {@link #getId}
* @since 1.610
*/
public static @CheckForNull <T extends Descriptor> T find(Collection<? extends T> list, String className) {
public static @CheckForNull <T extends Descriptor> T findById(Collection<? extends T> list, String id) {
for (T d : list) {
if(d.getId().equals(id))
return d;
}
return null;
}

/**
* Finds a descriptor from a collection by the class name of the {@link Descriptor}.
* This is useless as of the introduction of {@link #getId} and so only very old compatibility code needs it.
*/
private static @CheckForNull <T extends Descriptor> T findByClassName(Collection<? extends T> list, String className) {
for (T d : list) {
if(d.getClass().getName().equals(className))
return d;
}
// Since we introduced Descriptor.getId(), it is a preferred method of identifying descriptor by a string.
// To make that migration easier without breaking compatibility, let's also match up with the id.
return null;
}

/**
* Finds a descriptor from a collection by the class name of the {@link Describable} it describes.
* @param className should match {@link Class#getName} of a {@link #clazz}
* @since 1.610
*/
public static @CheckForNull <T extends Descriptor> T findByDescribableClassName(Collection<? extends T> list, String className) {
for (T d : list) {
if(d.getId().equals(className))
if(d.clazz.getName().equals(className))
return d;
}
return null;
}

/**
* Finds a descriptor from a collection by its class name or ID.
* @deprecated choose between {@link #findById} or {@link #findByDescribableClassName}
*/
public static @CheckForNull <T extends Descriptor> T find(Collection<? extends T> list, String string) {
T d = findByClassName(list, string);
if (d != null) {
return d;
}
return findById(list, string);
}

/**
* @deprecated choose between {@link #findById} or {@link #findByDescribableClassName}
*/
public static @CheckForNull Descriptor find(String className) {
return find(ExtensionList.lookup(Descriptor.class),className);
}
@@ -148,6 +148,9 @@ public static boolean currentlyUpdatingByXml() {
return result;
}

/**
* @deprecated Underspecified what the parameter is. {@link Descriptor#getId}? A {@link Describable} class name?
*/
public static TopLevelItemDescriptor getDescriptor(String fqcn) {
return Descriptor.find(all(), fqcn);
}
@@ -122,15 +122,16 @@ protected DescriptorImpl() {
}

protected Downloadable createDownloadable() {
return new Downloadable(getId());
return new Downloadable(getDownloadableId());
}

/**
* This ID needs to be unique, and needs to match the ID token in the JSON update file.
* <p>
* By default we use the fully-qualified class name of the {@link DownloadFromUrlInstaller} subtype.
* @since 1.610
*/
public String getId() {
public String getDownloadableId() {
return clazz.getName().replace('$','.');
}

@@ -144,7 +145,7 @@ public String getId() {
* @return never null.
*/
public List<? extends Installable> getInstallables() throws IOException {
JSONObject d = Downloadable.get(getId()).getData();
JSONObject d = Downloadable.get(getDownloadableId()).getData();
if(d==null) return Collections.emptyList();
return Arrays.asList(((InstallableList)JSONObject.toBean(d,InstallableList.class)).list);
}
@@ -167,6 +167,7 @@ public String getHome() {
return home;
}

@SuppressWarnings("deprecation") // TODO this was mistakenly made to be the ToolDescriptor class name, rather than .id as you would expect; now baked into serial form
public ToolDescriptor getType() {
if (descriptor == null) {
descriptor = (ToolDescriptor) Descriptor.find(type);
@@ -196,6 +196,7 @@ public void load(Class<? extends Describable> c) {

/**
* Finds the descriptor that has the matching fully-qualified class name.
* @deprecated Underspecified what the parameter is. {@link Descriptor#getId}? A {@link Describable} class name?
*/
public Descriptor<T> find(String fqcn) {
return Descriptor.find(this,fqcn);
@@ -26,6 +26,16 @@ THE SOFTWARE.
xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<st:documentation>
Invisible &lt;f:entry> type for embedding a descriptor's $class field.

Most of the time a Descriptor has an unique class name that we can use to instantiate the right Describable
class, so we use the '$class' to represent that to clarify the intent.

In some other times, such as templates, there are multiple Descriptors with the same Descriptor.clazz
but different IDs, and in that case we put 'kind' to indicate that. In this case, to avoid confusing
readers we do not put non-unique '$class'.

See Descriptor.newInstancesFromHeteroList for how the reader side is handled.

<st:attribute name="clazz">
The describable class that we are instantiating via structured form submission.
</st:attribute>
@@ -36,12 +46,14 @@ THE SOFTWARE.
</st:documentation>
<j:set var="clazz" value="${attrs.clazz ?: attrs.descriptor.clazz.name}" />
<f:invisibleEntry>
<!-- Legacy: Remove once plugins have been staged onto $class -->
<input type="hidden" name="stapler-class" value="${clazz}" />
<!-- Legacy: Remove once plugins have been staged onto $class -->
<j:if test="${attrs.descriptor != null}">
<input type="hidden" name="kind" value="${attrs.descriptor.id}" />
</j:if>
<input type="hidden" name="$class" value="${clazz}" />
<j:choose>
<j:when test="${descriptor != null and descriptor.id != clazz}">
<input type="hidden" name="kind" value="${attrs.descriptor.id}" />
</j:when>
<j:otherwise>
<input type="hidden" name="stapler-class" value="${clazz}" /> <!-- Legacy: Remove once plugins have been staged onto $class -->
<input type="hidden" name="$class" value="${clazz}" />
</j:otherwise>
</j:choose>
</f:invisibleEntry>
</j:jelly>
</j:jelly>
@@ -24,14 +24,24 @@

package hudson.model;

import hudson.Launcher;
import hudson.model.Descriptor.PropertyType;
import hudson.tasks.BuildStepDescriptor;
import hudson.tasks.Builder;
import hudson.tasks.Shell;
import java.io.IOException;
import java.util.List;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import static org.junit.Assert.*;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.stapler.StaplerRequest;

@SuppressWarnings({"unchecked", "rawtypes"})
public class DescriptorTest {

public @Rule JenkinsRule rule = new JenkinsRule();
@@ -51,4 +61,57 @@
}
}

@Issue("JENKINS-26781")
@Test public void overriddenId() throws Exception {
FreeStyleProject p = rule.createFreeStyleProject();
p.getBuildersList().add(new BuilderImpl("builder-a"));
rule.configRoundtrip(p);
List<Builder> builders = p.getBuildersList();
assertEquals(1, builders.size());
assertEquals(BuilderImpl.class, builders.get(0).getClass());
assertEquals("builder-a", ((BuilderImpl) builders.get(0)).id);
rule.assertLogContains("running builder-a", rule.buildAndAssertSuccess(p));
p.getBuildersList().replace(new BuilderImpl("builder-b"));
rule.configRoundtrip(p);
builders = p.getBuildersList();
assertEquals(1, builders.size());
assertEquals(BuilderImpl.class, builders.get(0).getClass());
assertEquals("builder-b", ((BuilderImpl) builders.get(0)).id);
rule.assertLogContains("running builder-b", rule.buildAndAssertSuccess(p));
}
private static final class BuilderImpl extends Builder {
private final String id;
BuilderImpl(String id) {
this.id = id;
}
@Override public boolean perform(AbstractBuild<?,?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
listener.getLogger().println("running " + getDescriptor().getId());
return true;
}
@Override public Descriptor<Builder> getDescriptor() {
return (Descriptor<Builder>) Jenkins.getInstance().getDescriptorByName(id);
}
}
private static final class DescriptorImpl extends BuildStepDescriptor<Builder> {
private final String id;
DescriptorImpl(String id) {
super(BuilderImpl.class);
this.id = id;
}
@Override public String getId() {
return id;
}
@Override public Builder newInstance(StaplerRequest req, JSONObject formData) throws Descriptor.FormException {
return new BuilderImpl(id);
}
@Override public String getDisplayName() {
return id;
}
@Override public boolean isApplicable(Class<? extends AbstractProject> jobType) {
return true;
}
}
@TestExtension("overriddenId") public static final BuildStepDescriptor<Builder> builderA = new DescriptorImpl("builder-a");
@TestExtension("overriddenId") public static final BuildStepDescriptor<Builder> builderB = new DescriptorImpl("builder-b");

}

0 comments on commit b688047

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