Add classloader parameter to XmlFactory

[timja]

Allow setting a custom classloader rather than relying on the thread context classloader for resolving the XMLInputFactory

What do you think?

Background:

https://github.com/Azure/azure-sdk-for-java/issues/22764 https://github.com/jenkinsci/jackson2-api-plugin/pull/82#issuecomment-873264276

[jglick]

Please test whether #481 fixes the issue in Jenkins.

[basil]

Please test whether #481 fixes the issue in Jenkins.

Yes it does. Tested with the following diff to jackson2-api-plugin:

diff --git a/pom.xml b/pom.xml
index 4eba2d1..31130c2 100644
--- a/pom.xml
+++ b/pom.xml
@@ -67,9 +67,9 @@
     <revision>2.12.4</revision>
     <changelist>-SNAPSHOT</changelist>
     <java.level>8</java.level>
-    <jenkins.version>2.222.4</jenkins.version>
+    <jenkins.version>2.300</jenkins.version>
     <jackson.version>2.12.3</jackson.version>
-    <jackson-databind.version>${jackson.version}</jackson-databind.version>
+    <jackson-databind.version>2.13.0-rc1-SNAPSHOT</jackson-databind.version>
   </properties>

   <repositories>
@@ -89,8 +89,8 @@
     <dependencies>
       <dependency>
         <groupId>io.jenkins.tools.bom</groupId>
-        <artifactId>bom-2.222.x</artifactId>
-        <version>807.v6d348e44c987</version>
+        <artifactId>bom-2.289.x</artifactId>
+        <version>887.vae9c8ac09ff7</version>
         <scope>import</scope>
         <type>pom</type>
       </dependency>
@@ -131,13 +131,13 @@
     <dependency>
       <groupId>com.fasterxml.jackson.core</groupId>
       <artifactId>jackson-core</artifactId>
-      <version>${jackson.version}</version>
+      <version>2.13.0-rc1-SNAPSHOT</version>
     </dependency>

     <dependency>
       <groupId>com.fasterxml.jackson.core</groupId>
       <artifactId>jackson-annotations</artifactId>
-      <version>${jackson.version}</version>
+      <version>2.13.0-rc1-SNAPSHOT</version>
     </dependency>

     <dependency>
@@ -149,7 +149,7 @@
     <dependency>
       <groupId>com.fasterxml.jackson.dataformat</groupId>
       <artifactId>jackson-dataformat-xml</artifactId>
-      <version>${jackson.version}</version>
+      <version>2.13.0-rc1-SNAPSHOT</version>
     </dependency>

     <dependency>
diff --git a/src/test/java/com/fasterxml/jackson/dataformat/xml/XmlMapperTest.java b/src/test/java/com/fasterxml/jackson/dataformat/xml/XmlMapperTest.java
index a864ecd..825d5a4 100644
--- a/src/test/java/com/fasterxml/jackson/dataformat/xml/XmlMapperTest.java
+++ b/src/test/java/com/fasterxml/jackson/dataformat/xml/XmlMapperTest.java
@@ -10,8 +10,6 @@ import org.jvnet.hudson.test.RealJenkinsRule;

 import java.nio.charset.StandardCharsets;

-import javax.xml.stream.XMLInputFactory;
-
 public class XmlMapperTest {

     @Rule public RealJenkinsRule rr = new RealJenkinsRule();
@@ -22,11 +20,7 @@ public class XmlMapperTest {
     }

     private static void _smokes(JenkinsRule r) throws Throwable {
-        XMLInputFactory inputFactory =
-                XMLInputFactory.newFactory(
-                        XMLInputFactory.class.getName(), XmlFactory.class.getClassLoader());
-        XmlFactory factory = new XmlFactory(inputFactory);
-        XmlMapper mapper = new XmlMapper(factory);
+        XmlMapper mapper = new XmlMapper();
         String content = "<foo><bar><id>123</id></bar></foo>";
         Foo foo = mapper.readValue(content.getBytes(StandardCharsets.UTF_8), Foo.class);
         assertNotNull(foo.getBar());

[timja]

Shouldn’t your diff be passing a class loader @basil?

[basil]

Shouldn’t your diff be passing a class loader @basil?

No, I don't think so. Recall that I was testing #481, not this PR. :-)

[cowtowncoder]

One quick note: any new configurability settings should go via XmlFactoryBuilder and not through XmlFactory constructor overloads. That's more of a technicality but should remove the need to add new constructor overloads.

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" :)

[cowtowncoder]

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 Stax2ByteArraySource as input for SJSXP. This would likely be something I could even backport into older Jackson module version(s) (at least 2.12, possibly 2.11). I am not sure if that would be good or bad in the sense that there may be further issues but wanted to mention it first.

Second: although I am fine adding ClassLoader argument to XmlFactoryBuilder -- and changing the logic to make XmlFactoryBuilder create instances so it need not pass CL at all -- I wonder how many valid options there are. If this comes down to just 2 choices (context class loader vs... one XmlFactoryBuilder is loaded from?), we could probably have true/false setting instead, to reduce likelihood of issues.

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). This would still be needed for common case of implicit or direct construction of XmlFactory (i.e. one that does not go through XmlFactoryBuilder) anyway.

Fourth: downstream frameworks may want to explicitly construct and pass XMLInputFactory and XMLOutputFactory, going forward. That won't solve all issues but may help avoid one error mode.

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.

[cowtowncoder]

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. It may still be the case that default stax factory instantiation should use different ClassLoader, but I am bit wary of changing that -- feel free to convince me that change would still be useful.

[timja]

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