[JENKINS-31458] Noting merge of #1936.

jenkinsci · Dec 5, 2015 · b8d4d2b · b8d4d2b
2 parents 7c9ba70 + a154270
commit b8d4d2b
Show file tree

Hide file tree

Showing 7 changed files with 280 additions and 9 deletions.
diff --git a/changelog.html b/changelog.html
@@ -62,6 +62,9 @@
     Bytecode Compatibility Transformer computes the common super class without loading classes.
     Fixes the <code>ClassCircularityError</code> exception in Ruby Runtime Plugin. 
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-31019">issue 31019</a>)
+  <li class="major bug">
+    Extended Choice parameter definitions could not be saved since 1.637.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-31458">issue 31458</a>)
 </ul>
 </div><!--=TRUNK-END=-->
 <h3><a name=v1.639>What's new in 1.639</a> (2015/11/29)</h3>

diff --git a/core/src/main/java/hudson/model/Descriptor.java b/core/src/main/java/hudson/model/Descriptor.java
@@ -72,6 +72,7 @@
 import java.lang.reflect.Field;
 import java.lang.reflect.ParameterizedType;
 import java.beans.Introspector;
+import java.util.IdentityHashMap;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 
@@ -559,7 +560,7 @@ public T newInstance(StaplerRequest req) throws FormException {
      *      Signals a problem in the submitted form.
      * @since 1.145
      */
-    public T newInstance(StaplerRequest req, JSONObject formData) throws FormException {
+    public T newInstance(@CheckForNull StaplerRequest req, @Nonnull JSONObject formData) throws FormException {
         try {
             Method m = getClass().getMethod("newInstance", StaplerRequest.class);
 
@@ -574,7 +575,20 @@ public T newInstance(StaplerRequest req, JSONObject formData) throws FormExcepti
                 }
 
                 // new behavior as of 1.206
-                return verifyNewInstance(req.bindJSON(clazz,formData));
+                BindInterceptor oldInterceptor = req.getBindInterceptor();
+                try {
+                    NewInstanceBindInterceptor interceptor;
+                    if ((oldInterceptor instanceof NewInstanceBindInterceptor)) {
+                        interceptor = (NewInstanceBindInterceptor) oldInterceptor;
+                    } else {
+                        interceptor = new NewInstanceBindInterceptor(oldInterceptor);
+                        req.setBindInterceptor(interceptor);
+                    }
+                    interceptor.processed.put(formData, null);
+                    return verifyNewInstance(req.bindJSON(clazz, formData));
+                } finally {
+                    req.setBindInterceptor(oldInterceptor);
+                }
             }
         } catch (NoSuchMethodException e) {
             throw new AssertionError(e); // impossible
@@ -587,6 +601,78 @@ public T newInstance(StaplerRequest req, JSONObject formData) throws FormExcepti
         }
     }
 
+    /**
+     * Ensures that calls to {@link StaplerRequest#bindJSON(Class, JSONObject)} from {@link #newInstance(StaplerRequest, JSONObject)} recurse properly.
+     * {@code doConfigSubmit}-like methods will wind up calling {@code newInstance} directly
+     * or via {@link #newInstancesFromHeteroList(StaplerRequest, Object, Collection)},
+     * which consult any custom {@code newInstance} overrides for top-level {@link Describable} objects.
+     * But for nested describable objects Stapler would know nothing about {@code newInstance} without this trick.
+     */
+    @SuppressWarnings({"unchecked", "rawtypes"})
+    private static class NewInstanceBindInterceptor extends BindInterceptor {
+
+        private final BindInterceptor oldInterceptor;
+        private final Map<JSONObject,Void> processed = new IdentityHashMap<>();
+
+        NewInstanceBindInterceptor(BindInterceptor oldInterceptor) {
+            LOGGER.log(Level.FINER, "new interceptor delegating to {0}", oldInterceptor);
+            this.oldInterceptor = oldInterceptor;
+        }
+
+        private boolean isApplicable(Class type, JSONObject json) {
+            if (Modifier.isAbstract(type.getModifiers())) {
+                LOGGER.log(Level.FINER, "ignoring abstract {0} {1}", new Object[] {type.getName(), json});
+                return false;
+            }
+            if (!Describable.class.isAssignableFrom(type)) {
+                LOGGER.log(Level.FINER, "ignoring non-Describable {0} {1}", new Object[] {type.getName(), json});
+                return false;
+            }
+            if (processed.containsKey(json)) {
+                LOGGER.log(Level.FINER, "already processed {0} {1}", new Object[] {type.getName(), json});
+                return false;
+            }
+            return true;
+        }
+
+        @Override
+        public Object instantiate(Class actualType, JSONObject json) {
+            if (isApplicable(actualType, json)) {
+                LOGGER.log(Level.FINE, "switching to newInstance {0} {1}", new Object[] {actualType.getName(), json});
+                try {
+                    return Jenkins.getActiveInstance().getDescriptor(actualType).newInstance(Stapler.getCurrentRequest(), json);
+                } catch (Exception x) {
+                    LOGGER.log(Level.WARNING, "falling back to default instantiation " + actualType.getName() + " " + json, x);
+                    // If nested objects are not using newInstance, bindJSON will wind up throwing the same exception anyway,
+                    // so logging above will result in a duplicated stack trace.
+                    // However if they *are* then this is the only way to find errors in that newInstance.
+                    // Normally oldInterceptor.instantiate will just return DEFAULT, not actually do anything,
+                    // so we cannot try calling the default instantiation and then decide which problem to report.
+                }
+            }
+            return oldInterceptor.instantiate(actualType, json);
+        }
+
+        @Override
+        public Object onConvert(Type targetType, Class targetTypeErasure, Object jsonSource) {
+            if (jsonSource instanceof JSONObject) {
+                JSONObject json = (JSONObject) jsonSource;
+                if (isApplicable(targetTypeErasure, json)) {
+                    LOGGER.log(Level.FINE, "switching to newInstance {0} {1}", new Object[] {targetTypeErasure.getName(), json});
+                    try {
+                        return Jenkins.getActiveInstance().getDescriptor(targetTypeErasure).newInstance(Stapler.getCurrentRequest(), json);
+                    } catch (Exception x) {
+                        LOGGER.log(Level.WARNING, "falling back to default instantiation " + targetTypeErasure.getName() + " " + json, x);
+                    }
+                }
+            } else {
+                LOGGER.log(Level.FINER, "ignoring non-object {0}", jsonSource);
+            }
+            return oldInterceptor.onConvert(targetType, targetTypeErasure, jsonSource);
+        }
+
+    }
+
     /**
      * Look out for a typical error a plugin developer makes.
      * See http://hudson.361315.n4.nabble.com/Help-Hint-needed-Post-build-action-doesn-t-stay-activated-td2308833.html

diff --git a/core/src/main/java/hudson/model/Node.java b/core/src/main/java/hudson/model/Node.java
@@ -57,6 +57,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
@@ -470,10 +471,13 @@ public Node reconfigure(final StaplerRequest req, JSONObject form) throws FormEx
         if (form==null)     return null;
 
         final JSONObject jsonForProperties = form.optJSONObject("nodeProperties");
-        BindInterceptor old = req.setBindListener(new BindInterceptor() {
+        final AtomicReference<BindInterceptor> old = new AtomicReference<>();
+        old.set(req.setBindListener(new BindInterceptor() {
             @Override
             public Object onConvert(Type targetType, Class targetTypeErasure, Object jsonSource) {
-                if (jsonForProperties!=jsonSource)  return DEFAULT;
+                if (jsonForProperties != jsonSource) {
+                    return old.get().onConvert(targetType, targetTypeErasure, jsonSource);
+                }
 
                 try {
                     DescribableList<NodeProperty<?>, NodePropertyDescriptor> tmp = new DescribableList<NodeProperty<?>, NodePropertyDescriptor>(Saveable.NOOP,getNodeProperties().toList());
@@ -485,12 +489,12 @@ public Object onConvert(Type targetType, Class targetTypeErasure, Object jsonSou
                     throw new IllegalArgumentException(e);
                 }
             }
-        });
+        }));
 
         try {
             return getDescriptor().newInstance(req, form);
         } finally {
-            req.setBindListener(old);
+            req.setBindListener(old.get());
         }
     }
 

diff --git a/test/src/test/java/hudson/model/JobPropertyTest.java b/test/src/test/java/hudson/model/JobPropertyTest.java
@@ -23,15 +23,19 @@
  */
 package hudson.model;
 
-import static org.junit.Assert.assertNotSame;
-import static org.junit.Assert.assertSame;
+import static org.junit.Assert.*;
 
 import com.gargoylesoftware.htmlunit.WebAssert;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import hudson.matrix.MatrixProject;
 import hudson.maven.MavenModuleSet;
 import hudson.model.Descriptor.FormException;
+import java.util.logging.ConsoleHandler;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import net.sf.json.JSONObject;
+import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -42,6 +46,15 @@
 
 public class JobPropertyTest {
 
+    private static final Logger logger = Logger.getLogger(Descriptor.class.getName());
+    @BeforeClass
+    public static void logging() {
+        logger.setLevel(Level.ALL);
+        Handler handler = new ConsoleHandler();
+        handler.setLevel(Level.ALL);
+        logger.addHandler(handler);
+    }
+
     @Rule
     public JenkinsRule j = new JenkinsRule();
 
@@ -104,6 +117,11 @@ public void configRoundtrip() throws Exception {
         JobPropertyWithConfigImpl after = p.getProperty(JobPropertyWithConfigImpl.class);
         assertNotSame(after,before);
         j.assertEqualDataBoundBeans(before, after);
+        p.removeProperty(after);
+        JobPropertyWithConfigImpl empty = new JobPropertyWithConfigImpl("");
+        p.addProperty(empty);
+        j.configRoundtrip((Item)p);
+        assertNull(p.getProperty(JobPropertyWithConfigImpl.class));
     }
 
     public static class JobPropertyWithConfigImpl extends JobProperty<Job<?,?>> {
@@ -115,7 +133,13 @@ public JobPropertyWithConfigImpl(String name) {
         }
 
         @TestExtension("configRoundtrip")
-        public static class DescriptorImpl extends JobPropertyDescriptor {}
+        public static class DescriptorImpl extends JobPropertyDescriptor {
+            @Override
+            public JobProperty<?> newInstance(StaplerRequest req, JSONObject formData) throws FormException {
+                JobPropertyWithConfigImpl prop = (JobPropertyWithConfigImpl) super.newInstance(req, formData);
+                return prop.name.isEmpty() ? null : prop;
+            }
+        }
     }
 
     @Test

diff --git a/test/src/test/java/hudson/model/ParametersDefinitionPropertyTest.java b/test/src/test/java/hudson/model/ParametersDefinitionPropertyTest.java
@@ -0,0 +1,102 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2015 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package hudson.model;
+
+import java.util.Locale;
+import java.util.logging.ConsoleHandler;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import net.sf.json.JSONObject;
+import org.junit.Test;
+import static org.junit.Assert.*;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.TestExtension;
+import org.kohsuke.stapler.StaplerRequest;
+
+public class ParametersDefinitionPropertyTest {
+
+    private static final Logger logger = Logger.getLogger(Descriptor.class.getName());
+    @BeforeClass
+    public static void logging() {
+        logger.setLevel(Level.ALL);
+        Handler handler = new ConsoleHandler();
+        handler.setLevel(Level.ALL);
+        logger.addHandler(handler);
+    }
+
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
+
+    @Issue("JENKINS-31458")
+    @Test
+    public void customNewInstance() throws Exception {
+        KrazyParameterDefinition kpd = new KrazyParameterDefinition("kpd", "desc", "KrAzY");
+        FreeStyleProject p = r.createFreeStyleProject();
+        ParametersDefinitionProperty pdp = new ParametersDefinitionProperty(kpd);
+        p.addProperty(pdp);
+        r.configRoundtrip(p);
+        pdp = p.getProperty(ParametersDefinitionProperty.class);
+        kpd = (KrazyParameterDefinition) pdp.getParameterDefinition("kpd");
+        assertEquals("desc", kpd.getDescription());
+        assertEquals("krazy", kpd.field);
+    }
+
+    public static class KrazyParameterDefinition extends ParameterDefinition {
+
+        public final String field;
+
+        // not @DataBoundConstructor
+        public KrazyParameterDefinition(String name, String description, String field) {
+            super(name, description);
+            this.field = field;
+        }
+
+        @Override
+        public ParameterValue createValue(StaplerRequest req, JSONObject jo) {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public ParameterValue createValue(StaplerRequest req) {
+            throw new UnsupportedOperationException();
+        }
+
+        @TestExtension("customNewInstance")
+        public static class DescriptorImpl extends ParameterDescriptor {
+
+            @Override
+            public ParameterDefinition newInstance(StaplerRequest req, JSONObject formData) throws FormException {
+                return new KrazyParameterDefinition(formData.getString("name"), formData.getString("description"), formData.getString("field").toLowerCase(Locale.ENGLISH));
+            }
+
+        }
+
+    }
+
+}
diff --git a/test/src/test/java/hudson/slaves/NodePropertyTest.java b/test/src/test/java/hudson/slaves/NodePropertyTest.java
@@ -9,9 +9,15 @@
 import com.gargoylesoftware.htmlunit.html.DomNodeUtil;
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlLabel;
+import hudson.model.Descriptor;
 import hudson.model.Descriptor.FormException;
 import hudson.model.Slave;
+import java.util.logging.ConsoleHandler;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import net.sf.json.JSONObject;
+import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -24,6 +30,15 @@
  */
 public class NodePropertyTest {
 
+    private static final Logger logger = Logger.getLogger(Descriptor.class.getName());
+    @BeforeClass
+    public static void logging() {
+        logger.setLevel(Level.ALL);
+        Handler handler = new ConsoleHandler();
+        handler.setLevel(Level.ALL);
+        logger.addHandler(handler);
+    }
+
     @Rule
     public JenkinsRule j = new JenkinsRule();