Skip to content

Commit

Permalink
Merge pull request #1701 from jglick/backport-JENKINS-26781
Browse files Browse the repository at this point in the history 
[JENKINS-26781] Backport to 1.609
  • Loading branch information
@olivergondza
olivergondza committed May 14, 2015
2 parents aadc991 + c15bd27 commit 0f0047a
Show file tree
Hide file tree
Showing 13 changed files with 275 additions and 18 deletions.
1 change: 1 addition & 0 deletions core/src/main/java/hudson/DescriptorExtensionList.java
View file
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/hudson/model/ComputerSet.java
View file
Expand Up @@ -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);

Expand Down
79 changes: 69 additions & 10 deletions core/src/main/java/hudson/model/Descriptor.java
View file
Expand Up @@ -74,6 +74,8 @@
import java.beans.Introspector;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* Metadata about a configurable instance.
Expand Down Expand Up @@ -909,14 +911,35 @@ List<T> newInstancesFromHeteroList(StaplerRequest req, Object formData,
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) {
// Only applies when Descriptor.getId is overridden.
// Note that kind is only supported here,
// *not* inside the StaplerRequest.bindJSON which is normally called by newInstance
// (since Descriptor.newInstance is not itself available to Stapler).
// If you merely override getId for some reason, but use @DataBoundConstructor on your Describable,
// there is no problem; but you can only rely on newInstance being called at top level.
d = findById(descriptors, kind);
}
if (d == null) {
kind = jo.optString("$class");
if (kind != null) { // else we will fall through to the warning
// This is the normal case.
d = findByDescribableClassName(descriptors, kind);
if (d == null) {
// Deprecated system where stapler-class was the Descriptor class name (rather than Describable class name).
d = findByClassName(descriptors, kind);
}
}
}
Descriptor<T> d = find(descriptors, kind);
if (d != null) {
items.add(d.newInstance(req, jo));
} else {
LOGGER.log(Level.WARNING, "Received unexpected form data element: {0}", jo);
}
}
}
Expand All @@ -925,22 +948,58 @@ List<T> newInstancesFromHeteroList(StaplerRequest req, Object formData,
}

/**
* 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}
*/
public static @CheckForNull <T extends Descriptor> T find(Collection<? extends T> list, String className) {
@Restricted(NoExternalUse.class) // backported from 1.610
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}
*/
@Restricted(NoExternalUse.class) // backported from 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);
}
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/hudson/model/Items.java
View file
Expand Up @@ -148,6 +148,9 @@ public static List<TopLevelItemDescriptor> all(Authentication a, ItemGroup c) {
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);
}
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/hudson/tools/ToolLocationNodeProperty.java
View file
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/hudson/util/DescriptorList.java
View file
Expand Up @@ -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);
Expand Down
28 changes: 20 additions & 8 deletions core/src/main/resources/lib/form/class-entry.jelly
View file
Expand Up @@ -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>
Expand All @@ -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}" />
<input type="hidden" name="stapler-class" value="${clazz}" /> <!-- Legacy: Remove once plugins have been staged onto $class -->
<j:choose>
<j:when test="${attrs.descriptor != null and attrs.descriptor.id != clazz}">
<input type="hidden" name="kind" value="${attrs.descriptor.id}" />
</j:when>
<j:otherwise>
<input type="hidden" name="$class" value="${clazz}" />
</j:otherwise>
</j:choose>
</f:invisibleEntry>
</j:jelly>
</j:jelly>
156 changes: 156 additions & 0 deletions test/src/test/java/hudson/model/DescriptorTest.java
View file
Expand Up @@ -24,14 +24,27 @@

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.Arrays;
import java.util.List;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import static org.junit.Assert.*;
import org.junit.Ignore;
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.DataBoundConstructor;
import org.kohsuke.stapler.StaplerRequest;

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

public @Rule JenkinsRule rule = new JenkinsRule();
Expand All @@ -51,4 +64,147 @@ public class DescriptorTest {
}
}

@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");

@Issue("JENKINS-28110")
@Test public void nestedDescribableOverridingId() throws Exception {
FreeStyleProject p = rule.createFreeStyleProject("p");
p.getBuildersList().add(new B1(Arrays.asList(new D1(), new D2())));
rule.configRoundtrip(p);
rule.assertLogContains("[D 1, D 2]", rule.buildAndAssertSuccess(p));
}
public static abstract class D extends AbstractDescribableImpl<D> {
@Override public String toString() {return getDescriptor().getDisplayName();}
}
public static class D1 extends D {
@DataBoundConstructor public D1() {}
@TestExtension("nestedDescribableOverridingId") public static class DescriptorImpl extends Descriptor<D> {
@Override public String getDisplayName() {return "D 1";}
@Override public String getId() {return "D1-id";}
}
}
public static class D2 extends D {
@DataBoundConstructor public D2() {}
@TestExtension("nestedDescribableOverridingId") public static class DescriptorImpl extends Descriptor<D> {
@Override public String getDisplayName() {return "D 2";}
@Override public String getId() {return "D2-id";}
}
}
public static class B1 extends Builder {
public final List<D> ds;
@DataBoundConstructor public B1(List<D> ds) {
this.ds = ds;
}
@Override public boolean perform(AbstractBuild<?,?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
listener.getLogger().println(ds);
return true;
}
@TestExtension("nestedDescribableOverridingId") public static class DescriptorImpl extends Descriptor<Builder> {
@Override public String getDisplayName() {return "B1";}
}
}

@Ignore("never worked: TypePair.convertJSON looks for @DataBoundConstructor on D3 (Stapler does not grok Descriptor)")
@Issue("JENKINS-28110")
@Test public void nestedDescribableSharingClass() throws Exception {
FreeStyleProject p = rule.createFreeStyleProject("p");
p.getBuildersList().add(new B2(Arrays.asList(new D3("d3a"), new D3("d3b"))));
rule.configRoundtrip(p);
rule.assertLogContains("[d3a, d3b]", rule.buildAndAssertSuccess(p));
}
public static class D3 implements Describable<D3> {
private final String id;
D3(String id) {
this.id = id;
}
@Override public String toString() {
return id;
}
@Override public Descriptor<D3> getDescriptor() {
return Jenkins.getInstance().getDescriptorByName(id);
}
}
public static class D3D extends Descriptor<D3> {
private final String id;
public D3D(String id) {
super(D3.class);
this.id = id;
}
@Override public String getId() {
return id;
}
@Override public D3 newInstance(StaplerRequest req, JSONObject formData) throws Descriptor.FormException {
return new D3(id);
}
@Override public String getDisplayName() {
return id;
}
}
@TestExtension("nestedDescribableSharingClass") public static final Descriptor<D3> d3a = new D3D("d3a");
@TestExtension("nestedDescribableSharingClass") public static final Descriptor<D3> d3b = new D3D("d3b");
public static class B2 extends Builder {
public final List<D3> ds;
@DataBoundConstructor public B2(List<D3> ds) {
this.ds = ds;
}
@Override public boolean perform(AbstractBuild<?,?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
listener.getLogger().println(ds);
return true;
}
@TestExtension("nestedDescribableSharingClass") public static class DescriptorImpl extends Descriptor<Builder> {
@Override public String getDisplayName() {return "B2";}
}
}

}
7 changes: 7 additions & 0 deletions test/src/test/resources/hudson/model/DescriptorTest/B1/config.jelly
View file
@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:block>
<f:repeatableHeteroProperty field="ds"/>
</f:block>
</j:jelly>
7 changes: 7 additions & 0 deletions test/src/test/resources/hudson/model/DescriptorTest/B2/config.jelly
View file
@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:block>
<f:repeatableHeteroProperty field="ds"/>
</f:block>
</j:jelly>
3 changes: 3 additions & 0 deletions test/src/test/resources/hudson/model/DescriptorTest/D1/config.jelly
View file
@@ -0,0 +1,3 @@
<?xml version="1.0" encoding="UTF-8"?>
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core"/>
3 changes: 3 additions & 0 deletions test/src/test/resources/hudson/model/DescriptorTest/D2/config.jelly
View file
@@ -0,0 +1,3 @@
<?xml version="1.0" encoding="UTF-8"?>
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core"/>

0 comments on commit 0f0047a

Please sign in to comment.