[#HUDSON-5768] XStream round-trip fails for classes with fields containing "__" - Hudson JIRA

Hudson

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

Created: 25/Feb/10 02:18 PM Updated: 28/Feb/10 11:25 PM Resolved: 28/Feb/10 10:51 PM

Return to search

Component/s:

core

Affects Version/s:

None

Fix Version/s:

None

File Attachments:

XStream2Test.java.diff (1 kB)

Description

« Hide

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.

Sort Order:

[ Permalink | « Hide ]

mindless added a comment - 27/Feb/10 03:57 PM

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).

[ Permalink | « Hide ]

mdillon added a comment - 27/Feb/10 04:09 PM

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.

[ Permalink | « Hide ]

mindless added a comment - 27/Feb/10 04:23 PM - 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.

[ Permalink | « Hide ]

mindless added a comment - 27/Feb/10 04:40 PM

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.

[ Permalink | « Hide ]

mdillon added a comment - 27/Feb/10 04:48 PM - 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.

[ Permalink | « Hide ]

mindless added a comment - 27/Feb/10 04:49 PM

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.

[ Permalink | « Hide ]

scm_issue_link added a comment - 28/Feb/10 10:51 PM

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://hudson-ci.org/commit/28071
Log:
[FIXED HUDSON-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.

[ Permalink | « Hide ]

mindless added a comment - 28/Feb/10 11:25 PM

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