[JENKINS-21017] When unmarshalling into an existing object, reset mis…

…sing fields
jenkinsci · Feb 6, 2017 · c62f4f7 · c62f4f7
1 parent d0cdaa0
commit c62f4f7
Show file tree

Hide file tree

Showing 2 changed files with 295 additions and 0 deletions.
diff --git a/core/src/main/java/hudson/util/RobustReflectionConverter.java b/core/src/main/java/hudson/util/RobustReflectionConverter.java
@@ -271,8 +271,71 @@ public Object unmarshal(final HierarchicalStreamReader reader, final Unmarshalli
         return serializationMethodInvoker.callReadResolve(result);
     }
 
+    private static final class FieldExpectation {
+        private final Class definingClass;
+        private final String name;
+
+        public FieldExpectation(Class definingClass, String name) {
+            this.definingClass = definingClass;
+            this.name = name;
+        }
+
+        public Class getDefiningClass() {
+            return definingClass;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+
+            FieldExpectation that = (FieldExpectation) o;
+
+            if (definingClass != null ? !definingClass.equals(that.definingClass) : that.definingClass != null) {
+                return false;
+            }
+            return name.equals(that.name);
+        }
+
+        @Override
+        public int hashCode() {
+            int result = definingClass != null ? definingClass.hashCode() : 0;
+            result = 31 * result + name.hashCode();
+            return result;
+        }
+
+        @Override
+        public String toString() {
+            return "FieldExpectation{" +
+                    "definingClass=" + definingClass +
+                    ", name='" + name + '\'' +
+                    '}';
+        }
+
+
+    }
+
     public Object doUnmarshal(final Object result, final HierarchicalStreamReader reader, final UnmarshallingContext context) {
         final SeenFields seenFields = new SeenFields();
+        final boolean existingObject = context.currentObject() != null;
+        final Map<FieldExpectation, Object> expectedFields = existingObject ? new HashMap<FieldExpectation, Object>() : null;
+        final Object cleanInstance = existingObject ? reflectionProvider.newInstance(result.getClass()) : null;
+        if (existingObject) {
+            reflectionProvider.visitSerializableFields(cleanInstance, new ReflectionProvider.Visitor() {
+                @Override
+                public void visit(String name, Class type, Class definedIn, Object value) {
+                    expectedFields.put(new FieldExpectation(definedIn, name), value);
+                }
+            });
+        }
         Iterator it = reader.getAttributeNames();
         // Remember outermost Saveable encountered, for reporting below
         if (result instanceof Saveable && context.get("Saveable") == null)
@@ -301,6 +364,10 @@ public Object doUnmarshal(final Object result, final HierarchicalStreamReader re
                     }
                     reflectionProvider.writeField(result, attrName, value, classDefiningField);
                     seenFields.add(classDefiningField, attrName);
+                    if (existingObject) {
+                        expectedFields.remove(new FieldExpectation(
+                                classDefiningField == null ? result.getClass() : classDefiningField, attrName));
+                    }
                 }
             }
         }
@@ -342,6 +409,10 @@ public Object doUnmarshal(final Object result, final HierarchicalStreamReader re
                     LOGGER.warning("Cannot convert type " + value.getClass().getName() + " to type " + type.getName());
                     // behave as if we didn't see this element
                 } else {
+                    if (existingObject) {
+                        expectedFields.remove(new FieldExpectation(
+                                classDefiningField == null ? result.getClass() : classDefiningField, fieldName));
+                    }
                     if (fieldExistsInClass) {
                         reflectionProvider.writeField(result, fieldName, value, classDefiningField);
                         seenFields.add(classDefiningField, fieldName);
@@ -371,6 +442,12 @@ public Object doUnmarshal(final Object result, final HierarchicalStreamReader re
             OldDataMonitor.report((Saveable)result, (ArrayList<Throwable>)context.get("ReadError"));
             context.put("ReadError", null);
         }
+        if (existingObject) {
+            for (Map.Entry<FieldExpectation, Object> entry : expectedFields.entrySet()) {
+                reflectionProvider.writeField(result, entry.getKey().getName(), entry.getValue(),
+                        entry.getKey().getDefiningClass());
+            }
+        }
         return result;
     }
 

diff --git a/core/src/test/java/hudson/util/XStream2Test.java b/core/src/test/java/hudson/util/XStream2Test.java
@@ -23,6 +23,7 @@
  */
 package hudson.util;
 
+import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.*;
 
 import com.google.common.collect.ImmutableList;
@@ -32,11 +33,14 @@
 import hudson.model.Result;
 import hudson.model.Run;
 import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import jenkins.model.Jenkins;
 import org.apache.commons.io.FileUtils;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -296,4 +300,218 @@ public void trimVersion() {
         assertEquals("3.2.1", XStream2.trimVersion("3.2.1"));
         assertEquals("3.2-SNAPSHOT", XStream2.trimVersion("3.2-SNAPSHOT (private-09/23/2012 12:26-jhacker)"));
     }
+
+    @Issue("JENKINS-21017")
+    @Test
+    public void unmarshalToDefault_populated() {
+        String populatedXml = "<hudson.util.XStream2Test_-WithDefaults>\n"
+                + "  <stringDefaultValue>my string</stringDefaultValue>\n"
+                + "  <stringDefaultNull>not null</stringDefaultNull>\n"
+                + "  <arrayDefaultValue>\n"
+                + "    <string>1</string>\n"
+                + "    <string>2</string>\n"
+                + "    <string>3</string>\n"
+                + "  </arrayDefaultValue>\n"
+                + "  <arrayDefaultEmpty>\n"
+                + "    <string>1</string>\n"
+                + "    <string>2</string>\n"
+                + "    <string>3</string>\n"
+                + "  </arrayDefaultEmpty>\n"
+                + "  <arrayDefaultNull>\n"
+                + "    <string>1</string>\n"
+                + "    <string>2</string>\n"
+                + "    <string>3</string>\n"
+                + "  </arrayDefaultNull>\n"
+                + "  <listDefaultValue>\n"
+                + "    <string>1</string>\n"
+                + "    <string>2</string>\n"
+                + "    <string>3</string>\n"
+                + "  </listDefaultValue>\n"
+                + "  <listDefaultEmpty>\n"
+                + "    <string>1</string>\n"
+                + "    <string>2</string>\n"
+                + "    <string>3</string>\n"
+                + "  </listDefaultEmpty>\n"
+                + "  <listDefaultNull>\n"
+                + "    <string>1</string>\n"
+                + "    <string>2</string>\n"
+                + "    <string>3</string>\n"
+                + "  </listDefaultNull>\n"
+                + "</hudson.util.XStream2Test_-WithDefaults>";
+
+        WithDefaults existingInstance = new WithDefaults("foobar",
+                "foobar",
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                Arrays.asList("foobar", "barfoo", "fumanchu"),
+                Arrays.asList("foobar", "barfoo", "fumanchu"),
+                Arrays.asList("foobar", "barfoo", "fumanchu")
+        );
+
+        WithDefaults newInstance = new WithDefaults();
+
+        String xmlA = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(populatedXml, existingInstance));
+        String xmlB = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(populatedXml, newInstance));
+        String xmlC = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(populatedXml, null));
+
+        assertThat("Deserializing over an existing instance is the same as with no root", xmlA, is(xmlC));
+        assertThat("Deserializing over an new instance is the same as with no root", xmlB, is(xmlC));
+    }
+
+
+    @Issue("JENKINS-21017")
+    @Test
+    public void unmarshalToDefault_default() {
+        String defaultXml = "<hudson.util.XStream2Test_-WithDefaults>\n"
+                + "  <stringDefaultValue>defaultValue</stringDefaultValue>\n"
+                + "  <arrayDefaultValue>\n"
+                + "    <string>first</string>\n"
+                + "    <string>second</string>\n"
+                + "  </arrayDefaultValue>\n"
+                + "  <arrayDefaultEmpty/>\n"
+                + "  <listDefaultValue>\n"
+                + "    <string>first</string>\n"
+                + "    <string>second</string>\n"
+                + "  </listDefaultValue>\n"
+                + "  <listDefaultEmpty/>\n"
+                + "</hudson.util.XStream2Test_-WithDefaults>";
+
+        WithDefaults existingInstance = new WithDefaults("foobar",
+                "foobar",
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                Arrays.asList("foobar", "barfoo", "fumanchu"),
+                Arrays.asList("foobar", "barfoo", "fumanchu"),
+                Arrays.asList("foobar", "barfoo", "fumanchu")
+        );
+
+        WithDefaults newInstance = new WithDefaults();
+
+        String xmlA = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(defaultXml, existingInstance));
+        String xmlB = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(defaultXml, newInstance));
+        String xmlC = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(defaultXml, null));
+
+        assertThat("Deserializing over an existing instance is the same as with no root", xmlA, is(xmlC));
+        assertThat("Deserializing over an new instance is the same as with no root", xmlB, is(xmlC));
+    }
+
+
+    @Issue("JENKINS-21017")
+    @Test
+    public void unmarshalToDefault_empty() {
+        String emptyXml = "<hudson.util.XStream2Test_-WithDefaults/>";
+
+        WithDefaults existingInstance = new WithDefaults("foobar",
+                "foobar",
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                new String[]{"foobar", "barfoo", "fumanchu"},
+                Arrays.asList("foobar", "barfoo", "fumanchu"),
+                Arrays.asList("foobar", "barfoo", "fumanchu"),
+                Arrays.asList("foobar", "barfoo", "fumanchu")
+        );
+
+        WithDefaults newInstance = new WithDefaults();
+
+        String xmlA = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(emptyXml, existingInstance));
+        String xmlB = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(emptyXml, newInstance));
+        String xmlC = Jenkins.XSTREAM2.toXML(Jenkins.XSTREAM2.fromXML(emptyXml, null));
+
+        assertThat("Deserializing over an existing instance is the same as with no root", xmlA, is(xmlC));
+        assertThat("Deserializing over an new instance is the same as with no root", xmlB, is(xmlC));
+    }
+
+    public static class WithDefaults {
+        private String stringDefaultValue = "defaultValue";
+        private String stringDefaultNull;
+        private String[] arrayDefaultValue = { "first", "second" };
+        private String[] arrayDefaultEmpty = new String[0];
+        private String[] arrayDefaultNull;
+        private List<String> listDefaultValue = new ArrayList<>(Arrays.asList("first", "second"));
+        private List<String> listDefaultEmpty = new ArrayList<>();
+        private List<String> listDefaultNull;
+
+        public WithDefaults() {
+        }
+
+        public WithDefaults(String stringDefaultValue, String stringDefaultNull, String[] arrayDefaultValue,
+                            String[] arrayDefaultEmpty, String[] arrayDefaultNull,
+                            List<String> listDefaultValue, List<String> listDefaultEmpty,
+                            List<String> listDefaultNull) {
+            this.stringDefaultValue = stringDefaultValue;
+            this.stringDefaultNull = stringDefaultNull;
+            this.arrayDefaultValue = arrayDefaultValue == null ? null : arrayDefaultValue.clone();
+            this.arrayDefaultEmpty = arrayDefaultEmpty == null ? null : arrayDefaultEmpty.clone();
+            this.arrayDefaultNull = arrayDefaultNull == null ? null : arrayDefaultNull.clone();
+            this.listDefaultValue = listDefaultValue == null ? null : new ArrayList<>(listDefaultValue);
+            this.listDefaultEmpty = listDefaultEmpty == null ? null : new ArrayList<>(listDefaultEmpty);
+            this.listDefaultNull = listDefaultNull == null ? null : new ArrayList<>(listDefaultNull);
+        }
+
+        public String getStringDefaultValue() {
+            return stringDefaultValue;
+        }
+
+        public void setStringDefaultValue(String stringDefaultValue) {
+            this.stringDefaultValue = stringDefaultValue;
+        }
+
+        public String getStringDefaultNull() {
+            return stringDefaultNull;
+        }
+
+        public void setStringDefaultNull(String stringDefaultNull) {
+            this.stringDefaultNull = stringDefaultNull;
+        }
+
+        public String[] getArrayDefaultValue() {
+            return arrayDefaultValue;
+        }
+
+        public void setArrayDefaultValue(String[] arrayDefaultValue) {
+            this.arrayDefaultValue = arrayDefaultValue;
+        }
+
+        public String[] getArrayDefaultEmpty() {
+            return arrayDefaultEmpty;
+        }
+
+        public void setArrayDefaultEmpty(String[] arrayDefaultEmpty) {
+            this.arrayDefaultEmpty = arrayDefaultEmpty;
+        }
+
+        public String[] getArrayDefaultNull() {
+            return arrayDefaultNull;
+        }
+
+        public void setArrayDefaultNull(String[] arrayDefaultNull) {
+            this.arrayDefaultNull = arrayDefaultNull;
+        }
+
+        public List<String> getListDefaultValue() {
+            return listDefaultValue;
+        }
+
+        public void setListDefaultValue(List<String> listDefaultValue) {
+            this.listDefaultValue = listDefaultValue;
+        }
+
+        public List<String> getListDefaultEmpty() {
+            return listDefaultEmpty;
+        }
+
+        public void setListDefaultEmpty(List<String> listDefaultEmpty) {
+            this.listDefaultEmpty = listDefaultEmpty;
+        }
+
+        public List<String> getListDefaultNull() {
+            return listDefaultNull;
+        }
+
+        public void setListDefaultNull(List<String> listDefaultNull) {
+            this.listDefaultNull = listDefaultNull;
+        }
+    }
 }