|
I looked at this a little closer and the only converters that have a direct reference to the original ReflectionConverter are ThrowableConverter, RegexPatternConverter, and SelfStreamingInstanceChecker. The other ones I mention take the "reflectionProvider" in the constructor, not the "reflectionConverter". This patch changes XStream2 to "replace" ThrowableConverter, RegexPatternConverter, and SelfStreamingInstanceChecker with ones that delegate to RobustReflectionConverter. The only one I've seen fail is ThrowableConverter. Failure of RegexPatternConverter seems unlikely since Pattern is not generally subclasses. I'm not sure what SelfStreamingInstanceChecker actually does, so I can't say whether or not the likelihood of a failure in the default ReflectionConverter is likely to bubble up and cause it to have an exception itself. One more note. It seems like the right fix for this might be in XStream. It seems like they should be using the convertAnother method on the MarshallingContext/UnmarshallingContext, but those method can't be passed a HierarchicalStreamWriter or HierarchicalStreamReader. Not sure if the contexts have enough information for that not to matter. That must be why they maintain the direct reference to the default converter. You're right that using convertAnother would not be workable. An alternative change for XStream that might be helpful would be for them to add a "protected Converter createReflectionConverter()" on the XStream class. That would let XStream2 replace the default reflection converter with RobustReflectionConverter. Putting aside the discussion of what could be changed in XStream itself, does my patch for Hudson seem workable? Aside from the nastiness of having to know the priority associated with the original converters and the lack of the ability to simply replace those converters directly, it seems like a workable approach to me. Yes, both solutions are workable.. we already have a forked XStream, and I'm on the fence about whether to add another custom change in there. But I do like that createReflectionConverter (or maybe createDefaultConverter) idea, as it would make the full registry of converters a bit cleaner.. "replacing" ThrowableConverter, etc. actually leaves the original instances in the registry, though I guess they'd never be used. Going with createDefaultConverter(). http://github.com/hudson/xstream/commit/b52438619b90231ce6fc37b4d0053079a98567e4 r28141 | mindless | 2010-03-02 10:55:42 -0800 (Tue, 02 Mar 2010) | 8 lines Changed paths: M /trunk/hudson/main/core/pom.xml M /trunk/hudson/main/core/src/main/java/hudson/util/XStream2.java M /trunk/hudson/main/core/src/test/java/hudson/util/XStream2Test.java M /trunk/www/changelog.html [FIXED HUDSON-5769] Update to XStream 1.3.1-hudson-6 where I added protected Converter createDefaultConverter(). This allows a subclass to override the default converter (was hardcoded to ReflectionConverter) which is also given to the constructor of a few other converters, most notably ThrowableConverter. Override this method in XStream2 to provide a RobustReflectionConverter so missing fields in classes extending Throwable are handled properly. Code changed in hudson |
|||||||||||||||||||||||||||||||||||||
For what it's worth, I was able to work around this issue for the ThrowableConverter case by putting the following code into a plugin: