[JENKINS-26781] Merging #1563.

changelog.html

@@ -55,6 +55,9 @@ Upcoming changes</a>
 <!-- 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>)

core/src/main/java/hudson/DescriptorExtensionList.java

@@ -109,6 +109,7 @@ public class DescriptorExtensionList<T extends Describable<T>, D extends Descrip
      *
      * @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);

core/src/main/java/hudson/model/ComputerSet.java

@@ -290,6 +290,7 @@ public final class ComputerSet extends AbstractModelObject implements Describabl
         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);
 

core/src/main/java/hudson/model/Descriptor.java

@@ -909,14 +909,23 @@ public abstract class Descriptor<T extends Describable<T>> implements Saveable {
         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 @@ public abstract class Descriptor<T extends Describable<T>> implements Saveable {
     }
 
     /**
-     * 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);
     }

core/src/main/java/hudson/model/Items.java

@@ -148,6 +148,9 @@ public class Items {
         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);
     }

core/src/main/java/hudson/tools/DownloadFromUrlInstaller.java

@@ -122,15 +122,16 @@ public abstract class DownloadFromUrlInstaller extends ToolInstaller {
         }
 
         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 abstract class DownloadFromUrlInstaller extends ToolInstaller {
          * @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);
         }

core/src/main/java/hudson/tools/ToolLocationNodeProperty.java

@@ -167,6 +167,7 @@ public class ToolLocationNodeProperty extends NodeProperty<Node> {
             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);

core/src/main/java/hudson/util/DescriptorList.java

@@ -196,6 +196,7 @@ public final class DescriptorList<T extends Describable<T>> extends AbstractList
 
     /**
      * 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);

core/src/main/resources/lib/form/class-entry.jelly

@@ -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>
\ No newline at end of file
+</j:jelly>

test/src/test/java/hudson/model/DescriptorTest.java

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