Use of ClassLoader-taking newFactory() variant breaks applications using default JDK XML implementation

[dfelliott]

The fix for #483 introduced a regression for applications using only the built-in JDK XML support.

Before this change, the code did this:

            xmlIn = XMLInputFactory.newInstance();

After this change, the code does this:

            // 05-Jul-2021, tatu: as per [dataformat-xml#483], specify ClassLoader
            xmlIn = XMLInputFactory.newFactory(XMLInputFactory.class.getName(),
                    getClass().getClassLoader());

A similar change applies to the XMLOutputFactory.

Unfortunately if you look at the JDK's implementation of these methods, the one taking zero arguments (old code) will fall back to the built-in implementation whereas the one accepting arguments (new code) does not perform the fallback.

My preference would be to revert this change, not just because it would fix this problem for me but because I believe the original code was actually correct and the new code is not. Why should only XML libraries on whatever ClassLoader is hosting Jackson be considered? Shouldn't the caller be able to control what library is used in the default constructor by controlling the context ClassLoader?

Meanwhile, a caller wishing to use the XML support of a specific ClassLoader, be it Jackson's ClassLoader or another, is already free to make the call to XMLInputFactory and XMLOutputFactory itself, passing in the specific objects to use. Or that caller could instead simply set their ContextClassLoader appropriately and the default constructor would do what they want.

[cowtowncoder]

Ok just to make sure I understand: what is the effect of missing fall-back for 2-argument case? Exception for missing implementation? Or something else?

I am also curious as to why someone would want to use JDK Stax implementation (it being inferior to Woodstox and Aalto in about every way), but that's more for my curiosity: it should be possible from impl perspective.

[dfelliott]

Exception for missing implementation.

[cowtowncoder]

Ok, so.

First of all, I don't like the idea of reverting the change since there was an issue it was meant to resolve and since the change has been in one full minor version (2.13).
At the same time, current behavior is not good either.

One thing that seems doable is catching of the exception and attempting secondary lookup with no-args method.
I think I'd like to go with that.

One immediate challenge here is testing, however; due to the way Stax implementation lookup works, if any implementation is in the classpath it will be found so test would specifically require use of a very bare-bones project.
Since this repo is not a multi-maven-module one it seems impossible to add a basic unit test -- unless I am missing some obvious way to achieve suitable set up.

@dfelliott Do you think you could come up with a PR here? I would be open to it being based off of 2.13 branch if it's just simple try-catch-reattempt change.

[cowtowncoder]

@dfelliott I added try-catch for FactoryConfigurationError -- was able to locally test that this is the exception you get.
Haven't been able to automate it to guard against regression but hopefully there won't be high risk of regression.