diff --git a/core/src/main/java/hudson/XmlFile.java b/core/src/main/java/hudson/XmlFile.java index 16644262b9..bde1ac12a4 100644 --- a/core/src/main/java/hudson/XmlFile.java +++ b/core/src/main/java/hudson/XmlFile.java @@ -159,10 +159,25 @@ public final class XmlFile { * 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 index 4c026d5f31..e06a3f1988 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -752,7 +752,7 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet } // 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 index b38c2b96d1..83a3682503 100644 --- a/core/src/main/java/hudson/model/View.java +++ b/core/src/main/java/hudson/model/View.java @@ -1214,7 +1214,7 @@ public abstract class View extends AbstractModelObject implements AccessControll // view in same ViewGroup and might not satisfy Jenkins.checkGoodName. String oldname = name; ViewGroup oldOwner = owner; // oddly, this field is not transient - Object o = Jenkins.XSTREAM.unmarshal(new Xpp3Driver().createReader(in), this); + 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 diff --git a/core/src/main/java/hudson/util/XStream2.java b/core/src/main/java/hudson/util/XStream2.java index de64a7e82e..9d4ef9b99b 100644 --- a/core/src/main/java/hudson/util/XStream2.java +++ b/core/src/main/java/hudson/util/XStream2.java @@ -48,6 +48,7 @@ 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; @@ -75,6 +76,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; /** * {@link XStream} enhanced for additional Java5 support and improved robustness. @@ -112,6 +114,26 @@ public class XStream2 extends XStream { @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. + *

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. + *

Do not 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 JENKINS-21017 + * @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(); @@ -120,8 +142,8 @@ public class XStream2 extends XStream { } Object o; - if (root == null) { - o = super.unmarshal(reader, null, dataHolder); + if (root == null || !nullOut) { + o = super.unmarshal(reader, root, dataHolder); } else { Set topLevelFields = new HashSet<>(); o = super.unmarshal(new ReaderWrapper(reader) { diff --git a/core/src/test/java/hudson/util/XStream2Test.java b/core/src/test/java/hudson/util/XStream2Test.java index ac1eb1c01a..f5a038ae79 100644 --- a/core/src/test/java/hudson/util/XStream2Test.java +++ b/core/src/test/java/hudson/util/XStream2Test.java @@ -29,11 +29,11 @@ import static org.junit.Assert.*; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.thoughtworks.xstream.XStreamException; +import com.thoughtworks.xstream.io.xml.Xpp3Driver; import com.thoughtworks.xstream.security.ForbiddenClassException; -import hudson.XmlFile; import hudson.model.Result; import hudson.model.Run; -import java.io.File; +import java.io.StringReader; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -42,10 +42,8 @@ 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; -import org.jvnet.hudson.test.Issue; /** * Tests for XML serialization of java objects. @@ -352,9 +350,9 @@ public class XStream2Test { 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)); + String xmlA = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(populatedXml, existingInstance)); + String xmlB = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(populatedXml, newInstance)); + String xmlC = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(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)); @@ -390,9 +388,9 @@ public class XStream2Test { 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)); + String xmlA = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(defaultXml, existingInstance)); + String xmlB = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(defaultXml, newInstance)); + String xmlC = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(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)); @@ -416,14 +414,21 @@ public class XStream2Test { 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)); + Object reloaded = fromXMLNullingOut(emptyXml, existingInstance); + assertSame(existingInstance, reloaded); + String xmlA = Jenkins.XSTREAM2.toXML(reloaded); + String xmlB = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(emptyXml, newInstance)); + String xmlC = Jenkins.XSTREAM2.toXML(fromXMLNullingOut(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)); } + private Object fromXMLNullingOut(String xml, Object root) { + // Currently not offering a public convenience API for this: + return Jenkins.XSTREAM2.unmarshal(new Xpp3Driver().createReader(new StringReader(xml)), root, null, true); + } + public static class WithDefaults { private String stringDefaultValue = "defaultValue"; private String stringDefaultNull;