Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-5768

XStream round-trip fails for classes with fields containing "__"

    • Icon: Bug Bug
    • Resolution: Done
    • Icon: Minor Minor
    • core
    • None

      If a class has "__" in a field name, Hudson is not able to do correct round-trip marshaling and unmarshaling. This seems to be related to the fact that XStream2.useXStream11XmlFriendlyMapper() is overridden to return "true". If that flag is changed back to false, the round-trip works correctly (but the testCompatibilityFromString() test in hudson.util.SecretTest starts failing unless "$" is replaced with "_-" instead of "-" in the test).
      
      I believe this failure is happening because Hudson's XStream instance has both an XmlFriendlyReplacer and and XmlFriendlyMapper in the pipeline. This makes it so that the Mapper.realMember() call in RobustReflectionConverter strips underscores from an element name that has already had the underscores stripped before being returned by reader.getNodeName().
      
      In addition to this problem, we were futher impacted by this issue because the field in question was insider of a Throwable object. It turns out that the ThrowableConverter in XStream uses the internal ReflectionConverter directly, so that means that RobustReflectionConverter is not used when drilling into a Throwable. I'll create a separate JIRA ticket for this part of problem.
      
      The attached patch adds a failing test to XStream2Test. I'm not sure what the proper way to fix this would be.
      

          [JENKINS-5768] XStream round-trip fails for classes with fields containing "__"

          Alan Harder added a comment -
          Thanks for the great analysis.. reading the javadoc of XStream11XmlFiendlyMapper and looking at existing serialized files, it does look like only XmlFriendlyReplacer is doing the work for us (I see "_-" and "__" replacements.. no _DOLLAR_ as mentioned in the Mapper javadoc).
          As for the SecretTest failure after your suggested change, I think it's ok to change "-" to "_-" in that test, as it's just trying to make XML readable for that Foo inner class and we know "_-" is what Hudson writes today for inner classes.
          So, I'm in favor of removing the useXStream11XmlFriendlyMapper() in XStream2, but I'll have to check with Kohsuke to see if there is some historical reason why this is in there (perhaps people using Hudson for a very long time may have xml files with that different style of encoding).
          

          Alan Harder added a comment - Thanks for the great analysis.. reading the javadoc of XStream11XmlFiendlyMapper and looking at existing serialized files, it does look like only XmlFriendlyReplacer is doing the work for us (I see "_-" and "__" replacements.. no _DOLLAR_ as mentioned in the Mapper javadoc). As for the SecretTest failure after your suggested change, I think it's ok to change "-" to "_-" in that test, as it's just trying to make XML readable for that Foo inner class and we know "_-" is what Hudson writes today for inner classes. So, I'm in favor of removing the useXStream11XmlFriendlyMapper() in XStream2, but I'll have to check with Kohsuke to see if there is some historical reason why this is in there (perhaps people using Hudson for a very long time may have xml files with that different style of encoding).

          mdillon added a comment -

          Let's just say that tracing this was not very fun

          The change to override that method was made in cs3313 in May 2007. I would expect some people still have XML files from that time, but I don't know exactly what this was supposed to fix. There were no tests created at the time of the change.

          mdillon added a comment - Let's just say that tracing this was not very fun The change to override that method was made in cs3313 in May 2007. I would expect some people still have XML files from that time, but I don't know exactly what this was supposed to fix. There were no tests created at the time of the change.

          Alan Harder added a comment - - edited

          Ok, so that mapper was added in r3313, shortly after switch from XStream 1.1.3 to 1.2.1. Not sure when XmlFriendlyReplacer came into play, but prior to Hudson 1.106 perhaps the "-" and "_DOLLAR_" encoding were used.. I'll try it out.

          Alan Harder added a comment - - edited Ok, so that mapper was added in r3313, shortly after switch from XStream 1.1.3 to 1.2.1. Not sure when XmlFriendlyReplacer came into play, but prior to Hudson 1.106 perhaps the "-" and "_DOLLAR_" encoding were used.. I'll try it out.

          Alan Harder added a comment -
          Yes, Hudson 1.105 writes just "-" for inner classes instead of "_-".  Coincidentally, I'm working on a management screen related to updating old deprecated data in xml files (branches/old-data-monitor in svn).. this issue may fit into this work.
          Here's what I'm thinking:
          1. remove useXStream11XmlFriendlyMapper() in XStream2
          2. add code in RobustReflectionConverter to catch unknown-field errors and give it another try with the "-" mapping.. if this makes it work, report this file to the OldDataMonitor so that manage screen can help in re-saving this file in the new format.
          

          Alan Harder added a comment - Yes, Hudson 1.105 writes just "-" for inner classes instead of "_-". Coincidentally, I'm working on a management screen related to updating old deprecated data in xml files (branches/old-data-monitor in svn).. this issue may fit into this work. Here's what I'm thinking: 1. remove useXStream11XmlFriendlyMapper() in XStream2 2. add code in RobustReflectionConverter to catch unknown-field errors and give it another try with the "-" mapping.. if this makes it work, report this file to the OldDataMonitor so that manage screen can help in re-saving this file in the new format.

          mdillon added a comment - - edited

          That sounds right. I don't think the old format can be confused with a member name since "-" is not allowed as a Java identifier in the case that the tag is being interpreted under the new scheme. The _DOLLAR_ stuff seems even less likely to cause a problem. I guess you could have a problem if a non-Java JVM language had a field in an object that had "-" as part of the name and that got serialized as part of a Hudson object, but that seems way far-fetched.

          mdillon added a comment - - edited That sounds right. I don't think the old format can be confused with a member name since "-" is not allowed as a Java identifier in the case that the tag is being interpreted under the new scheme. The _DOLLAR_ stuff seems even less likely to cause a problem. I guess you could have a problem if a non-Java JVM language had a field in an object that had "-" as part of the name and that got serialized as part of a Hudson object, but that seems way far-fetched.

          Alan Harder added a comment -

          Hm, well my first test case didn't route through RobustReflectionConverter at all.. I'll still see if I can find a solution along these lines, but it's not as simple as I'd hoped.

          Alan Harder added a comment - Hm, well my first test case didn't route through RobustReflectionConverter at all.. I'll still see if I can find a solution along these lines, but it's not as simple as I'd hoped.

          Code changed in hudson
          User: : mindless
          Path:
          trunk/hudson/main/core/src/main/java/hudson/util/XStream2.java
          trunk/hudson/main/core/src/test/java/hudson/util/SecretTest.java
          trunk/hudson/main/core/src/test/java/hudson/util/XStream2Test.java
          trunk/www/changelog.html
          http://jenkins-ci.org/commit/28071
          Log:
          [FIXED JENKINS-5768] Avoid double decoding that broke deserialization of fields with
          double underscore, by replacing XStream11XmlFriendlyMapper with a custom mapper that
          only does "-" to "$" decoding that is needed for compatibility with old xml data
          written by Hudson 1.105 or older.

          SCM/JIRA link daemon added a comment - Code changed in hudson User: : mindless Path: trunk/hudson/main/core/src/main/java/hudson/util/XStream2.java trunk/hudson/main/core/src/test/java/hudson/util/SecretTest.java trunk/hudson/main/core/src/test/java/hudson/util/XStream2Test.java trunk/www/changelog.html http://jenkins-ci.org/commit/28071 Log: [FIXED JENKINS-5768] Avoid double decoding that broke deserialization of fields with double underscore, by replacing XStream11XmlFriendlyMapper with a custom mapper that only does "-" to "$" decoding that is needed for compatibility with old xml data written by Hudson 1.105 or older.

          Alan Harder added a comment -

          Hopefully this resolves the issue and maintains compatibility.. I added the reporting of detected old data in branches/old-data-monitor in r28073.

          Alan Harder added a comment - Hopefully this resolves the issue and maintains compatibility.. I added the reporting of detected old data in branches/old-data-monitor in r28073.

            mindless Alan Harder
            md5 Mike Dillon
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

              Created:
              Updated:
              Resolved: