Merge pull request #3196 from jglick/unmarshal-JENKINS-21017

[JENKINS-21017] When unmarshalling into an existing object, reset missing fields
jenkinsci · Dec 29, 2017 · 16042bd · 16042bd
2 parents 0ac93de + ddb9ad6
commit 16042bd
Show file tree

Hide file tree

Showing 8 changed files with 372 additions and 16 deletions.
diff --git a/core/src/main/java/hudson/XmlFile.java b/core/src/main/java/hudson/XmlFile.java
@@ -159,10 +159,25 @@ public Object read() throws IOException {
      *      if the XML representation is completely new.
      */
     public Object unmarshal( Object o ) throws IOException {
+        return unmarshal(o, false);
+    }
+
+    /**
+     * Variant of {@link #unmarshal(Object)} applying {@link XStream2#unmarshal(HierarchicalStreamReader, Object, DataHolder, boolean)}.
+     * @since FIXME
+     */
+    public Object unmarshalNullingOut(Object o) throws IOException {
+        return unmarshal(o, true);
+    }
 
+    private Object unmarshal(Object o, boolean nullOut) throws IOException {
         try (InputStream in = new BufferedInputStream(Files.newInputStream(file.toPath()))) {
             // TODO: expose XStream the driver from XStream
-            return xs.unmarshal(DEFAULT_DRIVER.createReader(in), o);
+            if (nullOut) {
+                return ((XStream2) xs).unmarshal(DEFAULT_DRIVER.createReader(in), o, null, true);
+            } else {
+                return xs.unmarshal(DEFAULT_DRIVER.createReader(in), o);
+            }
         } catch (RuntimeException | Error e) {
             throw new IOException("Unable to read "+file,e);
         }

diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java
@@ -752,7 +752,7 @@ public void updateByXml(Source source) throws IOException {
             }
 
             // try to reflect the changes by reloading
-            Object o = new XmlFile(Items.XSTREAM, out.getTemporaryFile()).unmarshal(this);
+            Object o = new XmlFile(Items.XSTREAM, out.getTemporaryFile()).unmarshalNullingOut(this);
             if (o!=this) {
                 // ensure that we've got the same job type. extending this code to support updating
                 // to different job type requires destroying & creating a new job type

diff --git a/core/src/main/java/hudson/model/View.java b/core/src/main/java/hudson/model/View.java
@@ -1213,7 +1213,8 @@ public void updateByXml(Source source) throws IOException {
             // Do not allow overwriting view name as it might collide with another
             // view in same ViewGroup and might not satisfy Jenkins.checkGoodName.
             String oldname = name;
-            Object o = Jenkins.XSTREAM.unmarshal(new Xpp3Driver().createReader(in), this);
+            ViewGroup oldOwner = owner; // oddly, this field is not transient
+            Object o = Jenkins.XSTREAM2.unmarshal(new Xpp3Driver().createReader(in), this, null, true);
             if (!o.getClass().equals(getClass())) {
                 // ensure that we've got the same view type. extending this code to support updating
                 // to different view type requires destroying & creating a new view type
@@ -1222,6 +1223,7 @@ public void updateByXml(Source source) throws IOException {
                     "the view with the new view type.");
             }
             name = oldname;
+            owner = oldOwner;
         } catch (StreamException | ConversionException | Error e) {// mostly reflection errors
             throw new IOException("Unable to read",e);
         }

diff --git a/core/src/main/java/hudson/util/ReflectionUtils.java b/core/src/main/java/hudson/util/ReflectionUtils.java
@@ -37,6 +37,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import javax.annotation.CheckForNull;
 
 /**
  * Utility code for reflection.
@@ -205,15 +206,23 @@ public Annotation[] getDeclaredAnnotations() {
 
     /**
      * Given the primitive type, returns the VM default value for that type in a boxed form.
+     * @return null unless {@link Class#isPrimitive}
      */
-    public static Object getVmDefaultValueForPrimitiveType(Class<?> type) {
+    public static @CheckForNull Object getVmDefaultValueForPrimitiveType(Class<?> type) {
         return defaultPrimitiveValue.get(type);
     }
 
-    private static final Map<Class,Object> defaultPrimitiveValue = new HashMap<Class, Object>();
+    // TODO the version in org.kohsuke.stapler is incomplete
+    private static final Map<Class<?>, Object> defaultPrimitiveValue = new HashMap<>();
     static {
-        defaultPrimitiveValue.put(boolean.class,false);
-        defaultPrimitiveValue.put(int.class,0);
-        defaultPrimitiveValue.put(long.class,0L);
+        defaultPrimitiveValue.put(boolean.class, false);
+        defaultPrimitiveValue.put(char.class, '\0');
+        defaultPrimitiveValue.put(byte.class, (byte) 0);
+        defaultPrimitiveValue.put(short.class, (short) 0);
+        defaultPrimitiveValue.put(int.class, 0);
+        defaultPrimitiveValue.put(long.class, 0L);
+        defaultPrimitiveValue.put(float.class, (float) 0);
+        defaultPrimitiveValue.put(double.class, (double) 0);
+        defaultPrimitiveValue.put(void.class, null); // FWIW
     }
 }
diff --git a/core/src/main/java/hudson/util/XStream2.java b/core/src/main/java/hudson/util/XStream2.java
@@ -39,13 +39,16 @@
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
 import com.thoughtworks.xstream.converters.extended.DynamicProxyConverter;
 import com.thoughtworks.xstream.core.JVM;
+import com.thoughtworks.xstream.core.util.Fields;
 import com.thoughtworks.xstream.io.HierarchicalStreamDriver;
 import com.thoughtworks.xstream.io.HierarchicalStreamReader;
 import com.thoughtworks.xstream.io.HierarchicalStreamWriter;
+import com.thoughtworks.xstream.io.ReaderWrapper;
 import com.thoughtworks.xstream.mapper.CannotResolveClassException;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.PluginManager;
 import hudson.PluginWrapper;
+import hudson.XmlFile;
 import hudson.diagnosis.OldDataMonitor;
 import hudson.remoting.ClassFilter;
 import hudson.util.xstream.ImmutableSetConverter;
@@ -63,19 +66,26 @@
 import java.io.Writer;
 
 import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.nio.charset.Charset;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
-import org.kohsuke.accmod.Restricted;
-import org.kohsuke.accmod.restrictions.NoExternalUse;
+import javax.annotation.Nonnull;
 
 /**
  * {@link XStream} enhanced for additional Java5 support and improved robustness.
  * @author Kohsuke Kawaguchi
  */
 public class XStream2 extends XStream {
+
+    private static final Logger LOGGER = Logger.getLogger(XStream2.class.getName());
+
     private RobustReflectionConverter reflectionConverter;
     private final ThreadLocal<Boolean> oldData = new ThreadLocal<Boolean>();
     private final @CheckForNull ClassOwnership classOwnership;
@@ -104,14 +114,81 @@ public XStream2(HierarchicalStreamDriver hierarchicalStreamDriver) {
 
     @Override
     public Object unmarshal(HierarchicalStreamReader reader, Object root, DataHolder dataHolder) {
+        return unmarshal(reader, root, dataHolder, false);
+    }
+
+    /**
+     * Variant of {@link #unmarshal(HierarchicalStreamReader, Object, DataHolder)} that nulls out non-{@code transient} instance fields not defined in the source when unmarshaling into an existing object.
+     * <p>Typically useful when loading user-supplied XML files in place (non-null {@code root})
+     * where some reference-valued fields of the root object may have legitimate reasons for being null.
+     * Without this mode, it is impossible to clear such fields in an existing instance,
+     * since XStream has no notation for a null field value.
+     * Even for primitive-valued fields, it is useful to guarantee
+     * that unmarshaling will produce the same result as creating a new instance.
+     * <p>Do <em>not</em> use in cases where the root objects defines fields (typically {@code final})
+     * which it expects to be {@link Nonnull} unless you are prepared to restore default values for those fields.
+     * @param nullOut whether to perform this special behavior;
+     *                false to use the stock XStream behavior of leaving unmentioned {@code root} fields untouched
+     * @see XmlFile#unmarshalNullingOut
+     * @see <a href="https://issues.jenkins-ci.org/browse/JENKINS-21017">JENKINS-21017</a>
+     * @since FIXME
+     */
+    public Object unmarshal(HierarchicalStreamReader reader, Object root, DataHolder dataHolder, boolean nullOut) {
         // init() is too early to do this
         // defensive because some use of XStream happens before plugins are initialized.
         Jenkins h = Jenkins.getInstanceOrNull();
         if(h!=null && h.pluginManager!=null && h.pluginManager.uberClassLoader!=null) {
             setClassLoader(h.pluginManager.uberClassLoader);
         }
 
-        Object o = super.unmarshal(reader,root,dataHolder);
+        Object o;
+        if (root == null || !nullOut) {
+            o = super.unmarshal(reader, root, dataHolder);
+        } else {
+            Set<String> topLevelFields = new HashSet<>();
+            o = super.unmarshal(new ReaderWrapper(reader) {
+                int depth;
+                @Override
+                public void moveUp() {
+                    if (--depth == 0) {
+                        topLevelFields.add(getNodeName());
+                    }
+                    super.moveUp();
+                }
+                @Override
+                public void moveDown() {
+                    try {
+                        super.moveDown();
+                    } finally {
+                        depth++;
+                    }
+                }
+            }, root, dataHolder);
+            if (o == root && getConverterLookup().lookupConverterForType(o.getClass()) instanceof RobustReflectionConverter) {
+                getReflectionProvider().visitSerializableFields(o, (String name, Class type, Class definedIn, Object value) -> {
+                    if (topLevelFields.contains(name)) {
+                        return;
+                    }
+                    Field f = Fields.find(definedIn, name);
+                    Object v;
+                    if (type.isPrimitive()) {
+                        // oddly not in com.thoughtworks.xstream.core.util.Primitives
+                        v = ReflectionUtils.getVmDefaultValueForPrimitiveType(type);
+                        if (v.equals(value)) {
+                            return;
+                        }
+                    } else {
+                        if (value == null) {
+                            return;
+                        }
+                        v = null;
+                    }
+                    LOGGER.log(Level.FINE, "JENKINS-21017: nulling out {0} in {1}", new Object[] {f, o});
+                    Fields.write(f, o, v);
+                });
+            }
+        }
+
         if (oldData.get()!=null) {
             oldData.remove();
             if (o instanceof Saveable) OldDataMonitor.report((Saveable)o, "1.106");