[FIXED JENKINS-31458] Ensuring that Descriptor.newInstance is called …

…even on nested objects.
jenkinsci · Nov 30, 2015 · dca8607 · dca8607
1 parent c6f66be
commit dca8607
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 6 deletions.
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;
 
@@ -560,7 +561,12 @@ public T newInstance(StaplerRequest req) throws FormException {
      * @since 1.145
      */
     public T newInstance(StaplerRequest req, JSONObject formData) throws FormException {
+        BindInterceptor oldInterceptor = req.getBindInterceptor();
         try {
+            if (!(oldInterceptor instanceof NewInstanceBindInterceptor)) {
+                req.setBindInterceptor(new NewInstanceBindInterceptor(oldInterceptor));
+            }
+
             Method m = getClass().getMethod("newInstance", StaplerRequest.class);
 
             if(!Modifier.isAbstract(m.getDeclaringClass().getModifiers())) {
@@ -584,9 +590,70 @@ public T newInstance(StaplerRequest req, JSONObject formData) throws FormExcepti
             throw new Error("Failed to instantiate "+clazz+" from "+formData,e);
         } catch (RuntimeException e) {
             throw new RuntimeException("Failed to instantiate "+clazz+" from "+formData,e);
+        } finally {
+            req.setBindInterceptor(oldInterceptor);
         }
     }
 
+    /**
+     * 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,Boolean> processed = new IdentityHashMap<>();
+
+        NewInstanceBindInterceptor(BindInterceptor oldInterceptor) {
+            LOGGER.log(Level.FINE, "new interceptor delegating to {0}", oldInterceptor);
+            this.oldInterceptor = oldInterceptor;
+        }
+
+        @Override
+        public Object instantiate(Class actualType, JSONObject json) {
+            if (Modifier.isAbstract(actualType.getModifiers())) {
+                LOGGER.log(Level.FINE, "ignoring abstract {0} {1}", new Object[] {actualType, json});
+                return oldInterceptor.instantiate(actualType, json);
+            }
+            if (!Describable.class.isAssignableFrom(actualType)) {
+                LOGGER.log(Level.FINE, "ignoring non-Describable {0} {1}", new Object[] {actualType, json});
+                return oldInterceptor.instantiate(actualType, json);
+            }
+            if (Boolean.TRUE.equals(processed.put(json, true))) {
+                LOGGER.log(Level.FINE, "already processed {0} {1}", new Object[] {actualType, json});
+                return oldInterceptor.instantiate(actualType, json);
+            }
+            LOGGER.log(Level.FINE, "switching to newInstance {0} {1}", new Object[] {actualType, json});
+            try {
+                return Jenkins.getActiveInstance().getDescriptor(actualType).newInstance(Stapler.getCurrentRequest(), json);
+            } catch (Exception x) {
+                LOGGER.log(Level.WARNING, "falling back to default instantiation " + actualType + " " + 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) {
+                LOGGER.log(Level.FINE, "potentially converting {0} {1} {2}", new Object[] {targetType, targetTypeErasure, jsonSource});
+                return instantiate(targetTypeErasure, (JSONObject) jsonSource);
+            } else {
+                LOGGER.log(Level.FINE, "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/ParametersDefinitionPropertyTest.java b/test/src/test/java/hudson/model/ParametersDefinitionPropertyTest.java
@@ -28,7 +28,6 @@
 import net.sf.json.JSONObject;
 import org.junit.Test;
 import static org.junit.Assert.*;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -40,7 +39,6 @@ public class ParametersDefinitionPropertyTest {
     @Rule
     public JenkinsRule r = new JenkinsRule();
 
-    @Ignore("TODO after 600b1f0 (#1888): NoStaplerConstructorException: There's no @DataBoundConstructor on any constructor of class hudson.model.ParametersDefinitionPropertyTest$KrazyParameterDefinition")
     @Issue("JENKINS-31458")
     @Test
     public void customNewInstance() throws Exception {