Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add classloader parameter to XmlFactory #480

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/main/java/com/fasterxml/jackson/dataformat/xml/XmlFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,36 +81,44 @@ public class XmlFactory extends JsonFactory
* and this reuse only works within context of a single
* factory instance.
*/
public XmlFactory() { this(null, null, null); }
public XmlFactory() { this(null, null, null, null); }

public XmlFactory(ClassLoader cl) {
this(null, null, null, cl);
}

public XmlFactory(ObjectCodec oc) {
this(oc, null, null);
this(oc, null, null, null);
}

public XmlFactory(XMLInputFactory xmlIn) {
this(null, xmlIn, null);
this(null, xmlIn, null, null);
}

public XmlFactory(XMLInputFactory xmlIn, XMLOutputFactory xmlOut) {
this(null, xmlIn, xmlOut);
this(null, xmlIn, xmlOut, null);
}

public XmlFactory(ObjectCodec oc, XMLInputFactory xmlIn, XMLOutputFactory xmlOut)
public XmlFactory(ObjectCodec oc, XMLInputFactory xmlIn, XMLOutputFactory xmlOut, ClassLoader cl)
Copy link

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?

{
this(oc, DEFAULT_XML_PARSER_FEATURE_FLAGS, DEFAULT_XML_GENERATOR_FEATURE_FLAGS,
xmlIn, xmlOut, null);
xmlIn, xmlOut, null, null);
Copy link

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.

}

protected XmlFactory(ObjectCodec oc, int xpFeatures, int xgFeatures,
XMLInputFactory xmlIn, XMLOutputFactory xmlOut,
String nameForTextElem)
String nameForTextElem, ClassLoader cl)
{
super(oc);
_xmlParserFeatures = xpFeatures;
_xmlGeneratorFeatures = xgFeatures;
_cfgNameForTextElement = nameForTextElem;
if (xmlIn == null) {
xmlIn = XMLInputFactory.newInstance();
if (cl != null) {
Copy link

@basil basil Jul 3, 2021 •
edited
Loading

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.

xmlIn = XMLInputFactory.newFactory(XMLInputFactory.class.getName(), cl);
} else {
xmlIn = XMLInputFactory.newInstance();
Copy link
Author

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)

Copy link

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.

Copy link

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?

Copy link

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.

Copy link

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>
it seems like “strongly encouraging” use of Woodstox from the same class loader as Jackson would be reasonable.

Copy link

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;

Copy link

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.

Copy link
Member

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.

}
// 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
Expand Down