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 "__"

          mdillon created issue -
          Alan Harder made changes -
          Description Original: 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.
          New: {noformat}
          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.
          {noformat}
          Alan Harder made changes -
          Assignee New: Alan Harder [ mindless ]

          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).
          Alan Harder made changes -
          Status Original: Open [ 1 ] New: In Progress [ 3 ]

          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.

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

              Created:
              Updated:
              Resolved: