NodePropertyTest.invisibleProperty demonstrates that the fallback for...

NodePropertyTest.invisibleProperty demonstrates that the fallback for onConvert must be onConvert, not instantiate, of the delegate.

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

@@ -610,47 +610,60 @@ public abstract class Descriptor<T extends Describable<T>> implements Saveable {
         private final Map<JSONObject,Boolean> processed = new IdentityHashMap<>();
 
         NewInstanceBindInterceptor(BindInterceptor oldInterceptor) {
-            LOGGER.log(Level.FINE, "new interceptor delegating to {0}", oldInterceptor);
+            LOGGER.log(Level.FINER, "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);
+        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(actualType)) {
-                LOGGER.log(Level.FINE, "ignoring non-Describable {0} {1}", new Object[] {actualType, json});
-                return oldInterceptor.instantiate(actualType, json);
+            if (!Describable.class.isAssignableFrom(type)) {
+                LOGGER.log(Level.FINER, "ignoring non-Describable {0} {1}", new Object[] {type.getName(), json});
+                return false;
             }
             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.FINER, "already processed {0} {1}", new Object[] {type.getName(), json});
+                return false;
             }
-            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);
+            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) {
-                LOGGER.log(Level.FINE, "potentially converting {0} {1} {2}", new Object[] {targetType, targetTypeErasure, jsonSource});
-                return instantiate(targetTypeErasure, (JSONObject) jsonSource);
+                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.FINE, "ignoring non-object {0}", jsonSource);
-                return oldInterceptor.onConvert(targetType, targetTypeErasure, jsonSource);
+                LOGGER.log(Level.FINER, "ignoring non-object {0}", jsonSource);
             }
+            return oldInterceptor.onConvert(targetType, targetTypeErasure, jsonSource);
         }
 
     }

test/src/test/java/hudson/model/ParametersDefinitionPropertyTest.java

@@ -25,9 +25,14 @@
 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;
@@ -36,6 +41,15 @@ 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();
 

test/src/test/java/hudson/slaves/NodePropertyTest.java

@@ -9,9 +9,15 @@ import static org.junit.Assert.assertTrue;
 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 @@ import org.kohsuke.stapler.StaplerRequest;
  */
 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();