[JENKINS-28110] [JENKINS-28093] Merging #1680.

jenkinsci · Apr 29, 2015 · ca3b1db · ca3b1db
2 parents 6dd379d + 8e0cbbc
commit ca3b1db
Show file tree

Hide file tree

Showing 10 changed files with 139 additions and 9 deletions.
diff --git a/changelog.html b/changelog.html
@@ -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>
+    <code>Descriptor.getId</code> fix in 1.610 introduced regressions affecting at least the Performance and NodeJS plugins.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-28093">issue 28093</a> and <a href="https://issues.jenkins-ci.org/browse/JENKINS-28110">issue 28110</a>)
   <li class=bug>
     Under rare conditions Executor.getProgress() can throw a Division by zero exception.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-28115">issue 28115</a>)

diff --git a/core/src/main/java/hudson/model/Descriptor.java b/core/src/main/java/hudson/model/Descriptor.java
@@ -918,17 +918,29 @@ List<T> newInstancesFromHeteroList(StaplerRequest req, Object formData,
                 // 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.getString("$class");
-                  d = findByDescribableClassName(descriptors, kind);
-                  if (d == null) d = findByClassName(descriptors, kind);
+                  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);
+                      }
+                  }
                 }
                 if (d != null) {
                     items.add(d.newInstance(req, jo));
                 } else {
-                    LOGGER.warning("Received unexpected formData for descriptor " + kind);
+                    LOGGER.log(Level.WARNING, "Received unexpected form data element: {0}", jo);
                 }
             }
         }

diff --git a/core/src/main/java/hudson/tools/DownloadFromUrlInstaller.java b/core/src/main/java/hudson/tools/DownloadFromUrlInstaller.java
@@ -122,16 +122,15 @@ protected DescriptorImpl() {
         }
 
         protected Downloadable createDownloadable() {
-            return new Downloadable(getDownloadableId());
+            return new Downloadable(getId());
         }
 
         /**
          * 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 getDownloadableId() {
+        public String getId() {
             return clazz.getName().replace('$','.');
         }
 
@@ -145,7 +144,7 @@ public String getDownloadableId() {
          * @return never null.
          */
         public List<? extends Installable> getInstallables() throws IOException {
-            JSONObject d = Downloadable.get(getDownloadableId()).getData();
+            JSONObject d = Downloadable.get(getId()).getData();
             if(d==null)     return Collections.emptyList();
             return Arrays.asList(((InstallableList)JSONObject.toBean(d,InstallableList.class)).list);
         }

diff --git a/core/src/main/resources/lib/form/class-entry.jelly b/core/src/main/resources/lib/form/class-entry.jelly
@@ -46,12 +46,12 @@ THE SOFTWARE.
   </st:documentation>
   <j:set var="clazz" value="${attrs.clazz ?: attrs.descriptor.clazz.name}" />
   <f:invisibleEntry>
+    <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="stapler-class" value="${clazz}" />  <!-- Legacy: Remove once plugins have been staged onto $class -->
         <input type="hidden" name="$class" value="${clazz}" />
       </j:otherwise>
     </j:choose>

diff --git a/test/src/test/java/hudson/model/DescriptorTest.java b/test/src/test/java/hudson/model/DescriptorTest.java
@@ -30,15 +30,18 @@
 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"})
@@ -114,4 +117,94 @@ private static final class DescriptorImpl extends BuildStepDescriptor<Builder> {
     @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";}
+        }
+    }
+
 }
diff --git a/test/src/test/resources/hudson/model/DescriptorTest/B1/config.jelly b/test/src/test/resources/hudson/model/DescriptorTest/B1/config.jelly
@@ -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>
diff --git a/test/src/test/resources/hudson/model/DescriptorTest/B2/config.jelly b/test/src/test/resources/hudson/model/DescriptorTest/B2/config.jelly
@@ -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>
diff --git a/test/src/test/resources/hudson/model/DescriptorTest/D1/config.jelly b/test/src/test/resources/hudson/model/DescriptorTest/D1/config.jelly
@@ -0,0 +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core"/>
diff --git a/test/src/test/resources/hudson/model/DescriptorTest/D2/config.jelly b/test/src/test/resources/hudson/model/DescriptorTest/D2/config.jelly
@@ -0,0 +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core"/>
diff --git a/test/src/test/resources/hudson/model/DescriptorTest/D3/config.jelly b/test/src/test/resources/hudson/model/DescriptorTest/D3/config.jelly
@@ -0,0 +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core"/>