-
-
Notifications
You must be signed in to change notification settings - Fork 229
Add classloader parameter to XmlFactory #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
if (cl != null) { | ||
xmlIn = XMLInputFactory.newFactory(XMLInputFactory.class.getName(), cl); | ||
} else { | ||
xmlIn = XMLInputFactory.newInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is needed, it maintains compatibility fully, but I don't think the fallback functionality actually works for jackson, given jenkinsci/azure-storage-plugin#194 shows that the default jdk implementation doesn't work here
java.lang.UnsupportedOperationException: Cannot create XMLStreamReader or XMLEventReader from a org.codehaus.stax2.io.Stax2ByteArraySource
at java.xml/com.sun.xml.internal.stream.XMLInputFactoryImpl.jaxpSourcetoXMLInputSource(XMLInputFactoryImpl.java:302)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fallback functionality does work for the majority of Jackson users, where the thread's context class loader happens to include the Woodstox implementation. But it doesn't work for Jenkins plugins when Woodstox is removed from Jenkins core, because in that situation the plugin's class loader has access to Woodstox but core's class loader (which is the thread's context class loader) does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Jackson simply default to passing in its own class loader, rather than using the thread CCL? Would seem to make it work the way you expect without being told. Such a change might be incompatible in weird cases however. Can it detect that it is getting an incompatible implementation from the JRE and automatically switch to its own CCL if so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Jackson simply default to passing in its own class loader, rather than using the thread CCL? Would seem to make it work the way you expect without being told. Such a change might be incompatible in weird cases however.
Agreed on both counts.
Can it detect that it is getting an incompatible implementation from the JRE and automatically switch to its own CCL if so?
We could check to see if it is the known-incompatible com.sun.xml.internal.stream.XMLInputFactoryImpl
I suppose. But we probably couldn't do that with instanceof
because it's an implementation-specific class and might not be present in all Java implementations. So you'd likely have to do string comparison on .getClass().getName()
which isn't the most elegant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given https://github.com/FasterXML/jackson-dataformat-xml/#maven-dependency and
jackson-dataformat-xml/pom.xml
Lines 78 to 96 in 8b48a3f
<!-- and a Stax impl is needed: SJSXP (from JDK 1.6) might work, but always | |
has odd issues. Let's default to Woodstox: caller can upgrade to Aalto | |
(needs to block this dep) | |
--> | |
<!-- 09-May-2016, tatu: With Jackson 2.8, let's make this compile-dep to make it | |
less likely users accidentally try to use Sjsxp from JDK, which leads to probs | |
--> | |
<!-- 16-Jul-2016, tatu: For 2.10, need Woodstox 6.x to get module info --> | |
<dependency> | |
<groupId>com.fasterxml.woodstox</groupId> | |
<artifactId>woodstox-core</artifactId> | |
<version>6.2.6</version> | |
<exclusions> | |
<exclusion> | |
<groupId>javax.xml.stream</groupId> | |
<artifactId>stax-api</artifactId> | |
</exclusion> | |
</exclusions> | |
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like “strongly encouraging” use of Woodstox from the same class loader as Jackson would be reasonable.
If we wanted to "strongly encourage" use of the same class loader as Jackson, this would do the trick (tested, works):
diff --git a/src/main/java/com/fasterxml/jackson/dataformat/xml/XmlFactory.java b/src/main/java/com/fasterxml/jackson/dataformat/xml/XmlFactory.java
index f9391d8d..97a8831a 100644
--- a/src/main/java/com/fasterxml/jackson/dataformat/xml/XmlFactory.java
+++ b/src/main/java/com/fasterxml/jackson/dataformat/xml/XmlFactory.java
@@ -110,14 +110,22 @@ public class XmlFactory extends JsonFactory
_xmlGeneratorFeatures = xgFeatures;
_cfgNameForTextElement = nameForTextElem;
if (xmlIn == null) {
- xmlIn = XMLInputFactory.newInstance();
+ try {
+ xmlIn = XMLInputFactory.newFactory(XMLInputFactory.class.getName(), XmlFactory.class.getClassLoader());
+ } catch (FactoryConfigurationError e) {
+ xmlIn = XMLInputFactory.newInstance();
+ }
// as per [dataformat-xml#190], disable external entity expansion by default
xmlIn.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE);
// and ditto wrt [dataformat-xml#211], SUPPORT_DTD
xmlIn.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.FALSE);
}
if (xmlOut == null) {
- xmlOut = XMLOutputFactory.newInstance();
+ try {
+ xmlOut = XMLOutputFactory.newFactory(XMLOutputFactory.class.getName(), XmlFactory.class.getClassLoader());
+ } catch (FactoryConfigurationError e) {
+ xmlOut = XMLOutputFactory.newInstance();
+ }
}
_initFactories(xmlIn, xmlOut);
_xmlInputFactory = xmlIn;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cowtowncoder Did you have an opinion about the approach described here: "strongly encouraging", as Jesse put it, the use of these classes from XmlFactory
's class loader rather than the thread's current context class loader? I tested that in the Jenkins use case this would avoid SJSXP completely in favor of Woodstox. This seems more consistent to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@basil I wish I had an opinion here: I can see why it'd make sense but feel I may not have the full picture of possible downsides. My only concern would be that of breaking someone's existing usage somewhere due to changing resolution (I also realize it is obv impossible to know 100% sure in advance, and like Jesse said, there may be weird edge cases).
How about this: I'll create a new issue for proposed change (linking back to this PR) and we can discuss that change there? If anyone has a link/links to article describing the usual logic to determine between context classloader vs classes own, in context of libraries, that'd be great.
{ | ||
this(oc, DEFAULT_XML_PARSER_FEATURE_FLAGS, DEFAULT_XML_GENERATOR_FEATURE_FLAGS, | ||
xmlIn, xmlOut, null); | ||
xmlIn, xmlOut, null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be plumbing through cl
to the protected
method rather than passing in null
.
} | ||
|
||
public XmlFactory(ObjectCodec oc, XMLInputFactory xmlIn, XMLOutputFactory xmlOut) | ||
public XmlFactory(ObjectCodec oc, XMLInputFactory xmlIn, XMLOutputFactory xmlOut, ClassLoader cl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically speaking this removes a public API; not sure if this was intentional. Maybe you wanted to preserve the existing public XmlFactory(ObjectCodec, XMLInputFactory, XMLOutputFactory)
constructor and add a new one?
{ | ||
super(oc); | ||
_xmlParserFeatures = xpFeatures; | ||
_xmlGeneratorFeatures = xgFeatures; | ||
_cfgNameForTextElement = nameForTextElem; | ||
if (xmlIn == null) { | ||
xmlIn = XMLInputFactory.newInstance(); | ||
if (cl != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, I think we'd want to do the same thing for XMLOutputFactory.newInstance
a few lines below on line 128.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is an incompatible signature change here.
Please test whether #481 fixes the issue in Jenkins. |
Yes it does. Tested with the following diff to
|
Shouldn’t your diff be passing a class loader @basil? |
One quick note: any new configurability settings should go via Other than that I guess I better read the linked-to issues to have an opinion other than "do we really need to add classloader stuff" :) |
Ok... I see things get a bit hairy through the stack, SPI and so on. That makes sense I guess. There are couple of sort of orthogonal concerns, approaches and I am open to considering various ones. First: although Woodstox definitely is the recommended Stax implementation (along with Aalto), it might be possible to resolve the specific issue of not being able to use Second: although I am fine adding Third: changing of the default logic sounds acceptable too (we have RC1 of 2.13.0 to sanity check changes), so even if a new option is exposed (to opt-in to old behavior, I assume, if that is rarely needed). Fourth: downstream frameworks may want to explicitly construct and pass I hope above makes sense? Apologies if I missed something obvious. Timeline-wise, I am hoping to get 2.13.0-rc1 out in 2-3 weeks, so timing here is quite good. |
Just noticed there is #481, as an alternative. Will check it out. EDIT: ended up writing #482 based on that, which should avoid the specific problem with non-stax2 implementations. |
I think in general if you're using an SPI to resolve classes a classloader option should be exposed, e.g. see https://www.jenkins.io/doc/developer/plugin-development/dependencies-and-class-loading/#context-class-loaders but given there's a work around of constructing your own instance it may not be needed. I think I'll close this, and it can be revived if required. @jglick may have an opinion on it Thanks everyone for your work on this :) |
Allow setting a custom classloader rather than relying on the thread context classloader for resolving the
XMLInputFactory
What do you think?
Background:
Azure/azure-sdk-for-java#22764
jenkinsci/jackson2-api-plugin#82 (comment)