From c62f4f753d16210b67ec381b9e5d1d60145594d8 Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Mon, 6 Feb 2017 14:25:30 +0000 Subject: [PATCH 1/3] [JENKINS-21017] When unmarshalling into an existing object, reset missing fields --- .../util/RobustReflectionConverter.java | 77 +++++++ .../test/java/hudson/util/XStream2Test.java | 218 ++++++++++++++++++ 2 files changed, 295 insertions(+) diff --git a/core/src/main/java/hudson/util/RobustReflectionConverter.java b/core/src/main/java/hudson/util/RobustReflectionConverter.java index 3ec65ec994..a65c7f4e17 100644 --- a/core/src/main/java/hudson/util/RobustReflectionConverter.java +++ b/core/src/main/java/hudson/util/RobustReflectionConverter.java @@ -271,8 +271,71 @@ public class RobustReflectionConverter implements Converter { 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 expectedFields = existingObject ? new HashMap() : 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 class RobustReflectionConverter implements Converter { } 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 class RobustReflectionConverter implements Converter { 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 class RobustReflectionConverter implements Converter { OldDataMonitor.report((Saveable)result, (ArrayList)context.get("ReadError")); context.put("ReadError", null); } + if (existingObject) { + for (Map.Entry 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 index aeb781f672..5f65fac491 100644 --- 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.XmlFile; 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 class XStream2Test { 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 = "\n" + + " my string\n" + + " not null\n" + + " \n" + + " 1\n" + + " 2\n" + + " 3\n" + + " \n" + + " \n" + + " 1\n" + + " 2\n" + + " 3\n" + + " \n" + + " \n" + + " 1\n" + + " 2\n" + + " 3\n" + + " \n" + + " \n" + + " 1\n" + + " 2\n" + + " 3\n" + + " \n" + + " \n" + + " 1\n" + + " 2\n" + + " 3\n" + + " \n" + + " \n" + + " 1\n" + + " 2\n" + + " 3\n" + + " \n" + + ""; + + 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 = "\n" + + " defaultValue\n" + + " \n" + + " first\n" + + " second\n" + + " \n" + + " \n" + + " \n" + + " first\n" + + " second\n" + + " \n" + + " \n" + + ""; + + 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 = ""; + + 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 listDefaultValue = new ArrayList<>(Arrays.asList("first", "second")); + private List listDefaultEmpty = new ArrayList<>(); + private List listDefaultNull; + + public WithDefaults() { + } + + public WithDefaults(String stringDefaultValue, String stringDefaultNull, String[] arrayDefaultValue, + String[] arrayDefaultEmpty, String[] arrayDefaultNull, + List listDefaultValue, List listDefaultEmpty, + List 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 getListDefaultValue() { + return listDefaultValue; + } + + public void setListDefaultValue(List listDefaultValue) { + this.listDefaultValue = listDefaultValue; + } + + public List getListDefaultEmpty() { + return listDefaultEmpty; + } + + public void setListDefaultEmpty(List listDefaultEmpty) { + this.listDefaultEmpty = listDefaultEmpty; + } + + public List getListDefaultNull() { + return listDefaultNull; + } + + public void setListDefaultNull(List listDefaultNull) { + this.listDefaultNull = listDefaultNull; + } + } } From 51b0f2f5d31cd8cbec5a11fb070b02cb3a077fe3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 13 Dec 2017 17:17:16 -0500 Subject: [PATCH 2/3] Redesigned fix to run from XStream2.unmarshal. --- core/src/main/java/hudson/model/View.java | 2 + .../java/hudson/util/ReflectionUtils.java | 19 +++-- .../util/RobustReflectionConverter.java | 77 ------------------- core/src/main/java/hudson/util/XStream2.java | 61 ++++++++++++++- .../hudson/model/AbstractProjectTest.java | 12 +++ test/src/test/java/hudson/model/ViewTest.java | 20 ++++- 6 files changed, 105 insertions(+), 86 deletions(-) diff --git a/core/src/main/java/hudson/model/View.java b/core/src/main/java/hudson/model/View.java index fa3914af44..b38c2b96d1 100644 --- a/core/src/main/java/hudson/model/View.java +++ b/core/src/main/java/hudson/model/View.java @@ -1213,6 +1213,7 @@ public abstract class View extends AbstractModelObject implements AccessControll // 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; + ViewGroup oldOwner = owner; // oddly, this field is not transient Object o = Jenkins.XSTREAM.unmarshal(new Xpp3Driver().createReader(in), this); if (!o.getClass().equals(getClass())) { // ensure that we've got the same view type. extending this code to support updating @@ -1222,6 +1223,7 @@ public abstract class View extends AbstractModelObject implements AccessControll "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 index f491b59d87..faa6afb99a 100644 --- 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.AbstractList; 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 class ReflectionUtils extends org.springframework.util.ReflectionUtils { /** * 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 defaultPrimitiveValue = new HashMap(); + // TODO the version in org.kohsuke.stapler is incomplete + private static final Map, 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/RobustReflectionConverter.java b/core/src/main/java/hudson/util/RobustReflectionConverter.java index a65c7f4e17..3ec65ec994 100644 --- a/core/src/main/java/hudson/util/RobustReflectionConverter.java +++ b/core/src/main/java/hudson/util/RobustReflectionConverter.java @@ -271,71 +271,8 @@ public class RobustReflectionConverter implements Converter { 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 expectedFields = existingObject ? new HashMap() : 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) @@ -364,10 +301,6 @@ public class RobustReflectionConverter implements Converter { } reflectionProvider.writeField(result, attrName, value, classDefiningField); seenFields.add(classDefiningField, attrName); - if (existingObject) { - expectedFields.remove(new FieldExpectation( - classDefiningField == null ? result.getClass() : classDefiningField, attrName)); - } } } } @@ -409,10 +342,6 @@ public class RobustReflectionConverter implements Converter { 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); @@ -442,12 +371,6 @@ public class RobustReflectionConverter implements Converter { OldDataMonitor.report((Saveable)result, (ArrayList)context.get("ReadError")); context.put("ReadError", null); } - if (existingObject) { - for (Map.Entry entry : expectedFields.entrySet()) { - reflectionProvider.writeField(result, entry.getKey().getName(), entry.getValue(), - entry.getKey().getDefiningClass()); - } - } return result; } diff --git a/core/src/main/java/hudson/util/XStream2.java b/core/src/main/java/hudson/util/XStream2.java index 6b17761cb6..de64a7e82e 100644 --- a/core/src/main/java/hudson/util/XStream2.java +++ b/core/src/main/java/hudson/util/XStream2.java @@ -39,9 +39,11 @@ import com.thoughtworks.xstream.converters.SingleValueConverterWrapper; 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; @@ -63,19 +65,25 @@ import java.io.OutputStreamWriter; 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; /** * {@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 oldData = new ThreadLocal(); private final @CheckForNull ClassOwnership classOwnership; @@ -111,7 +119,54 @@ public class XStream2 extends XStream { setClassLoader(h.pluginManager.uberClassLoader); } - Object o = super.unmarshal(reader,root,dataHolder); + Object o; + if (root == null) { + o = super.unmarshal(reader, null, dataHolder); + } else { + Set 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"); diff --git a/test/src/test/java/hudson/model/AbstractProjectTest.java b/test/src/test/java/hudson/model/AbstractProjectTest.java index 308fc12993..c6fddb6a97 100644 --- a/test/src/test/java/hudson/model/AbstractProjectTest.java +++ b/test/src/test/java/hudson/model/AbstractProjectTest.java @@ -569,6 +569,18 @@ public class AbstractProjectTest { return con; } + @Issue("JENKINS-21017") + @Test public void doConfigDotXmlReset() throws Exception { + j.jenkins.setCrumbIssuer(null); + FreeStyleProject p = j.createFreeStyleProject(); + p.setAssignedLabel(Label.get("whatever")); + assertEquals("whatever", p.getAssignedLabelString()); + assertThat(p.getConfigFile().asString(), containsString("whatever")); + assertEquals(200, postConfigDotXml(p, "").getResponseCode()); + assertNull(p.getAssignedLabelString()); // did not work + assertThat(p.getConfigFile().asString(), not(containsString(""))); // actually did work anyway + } + @Test @Issue("JENKINS-27549") public void loadingWithNPEOnTriggerStart() throws Exception { diff --git a/test/src/test/java/hudson/model/ViewTest.java b/test/src/test/java/hudson/model/ViewTest.java index 7a70d87df2..f3bd6f70a6 100644 --- a/test/src/test/java/hudson/model/ViewTest.java +++ b/test/src/test/java/hudson/model/ViewTest.java @@ -62,7 +62,7 @@ import java.util.logging.LogRecord; import jenkins.model.ProjectNamingStrategy; import jenkins.security.NotReallyRoleSensitiveCallable; -import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import org.junit.Ignore; import org.junit.Rule; @@ -243,6 +243,24 @@ public class ViewTest { assertTrue(xml, xml.contains("two")); } + @Issue("JENKINS-21017") + @Test public void doConfigDotXmlReset() throws Exception { + ListView view = listView("v"); + view.description = "one"; + WebClient wc = j.createWebClient(); + String xml = wc.goToXml("view/v/config.xml").getWebResponse().getContentAsString(); + assertThat(xml, containsString("one")); + xml = xml.replace("one", ""); + WebRequest req = new WebRequest(wc.createCrumbedUrl("view/v/config.xml"), HttpMethod.POST); + req.setRequestBody(xml); + req.setEncodingType(null); + wc.getPage(req); + assertEquals(null, view.getDescription()); // did not work + xml = new XmlFile(Jenkins.XSTREAM, new File(j.jenkins.getRootDir(), "config.xml")).asString(); + assertThat(xml, not(containsString(""))); // did not work + assertEquals(j.jenkins, view.getOwner()); + } + @Test public void testGetQueueItems() throws IOException, Exception{ ListView view1 = listView("view1"); From ddb9ad68b4e74deb85f916d8d9ebe0ba5c044517 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 14 Dec 2017 13:16:42 -0500 Subject: [PATCH 3/3] Making nullOut behavior opt-in, so limiting to AbstractItem and View (and in particular not Jenkins). --- core/src/main/java/hudson/XmlFile.java | 17 +++++++++- .../main/java/hudson/model/AbstractItem.java | 2 +- core/src/main/java/hudson/model/View.java | 2 +- core/src/main/java/hudson/util/XStream2.java | 26 ++++++++++++++-- .../test/java/hudson/util/XStream2Test.java | 31 +++++++++++-------- 5 files changed, 60 insertions(+), 18 deletions(-) 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;