Rothamsted / knetbuilder

KnetBuilder data integration platform for building knowledge graphs. Previously known as ondex.
https://knetminer.com
MIT License
12 stars 11 forks source link

Remove all System.setProperty() involving XML components and alike #17

Closed marco-brandizi closed 4 years ago

marco-brandizi commented 5 years ago

A lot of Ondex code behaves like this:

System.setProperty(
  "javax.xml.stream.XMLInputFactory", "com.ctc.wstx.stax.WstxInputFactory"
);

XMLInputFactory2 xmlif = (XMLInputFactory2) XMLInputFactory2.newInstance();

That's bad practice, since it forces the whole JVM hosting this code to use a particular XML component. This has already happened to conflict with web servers, both Jetty and Tomcat.

I'm already working on fixing the problem, by adopting this other approach:

System.setProperty(
  "ondex.javax.xml.stream.XMLInputFactory", "com.ctc.wstx.stax.WstxInputFactory"
);

XMLInputFactory2 xmlif = (XMLInputFactory2) XMLInputFactory2.newFactory (
     "ondex.javax.xml.stream.XMLInputFactory", this.getClass ().getClassLoader ()
);

newFactory( propName, loader ) takes the class to be used as factory from the property name parameter. By using an Ondex-specific property name for that, which is not the name that methods like newFactory() or newInstance() lookup by default, we can avoid to interfere with the rest of the JVM.

Note that direct instantiation of WstxInputFactory doesn't always work in a straightforward way, since some plugins/modules instantiate factories like this dynamically, without having a declared dependency on, in this case, the Woodstox library (they get it indirectly, from containers like OVTK2 or the workflow engine).

Possible improvement

Due to issues with dependency management in Ondex, I'm changing the existing code by duplicating lines like the ones above. Ideally, they should be factorised into a component provider, which would set the ondex.* properties once (upon class loading) and would return components like XMLInputFactory2, XMLOutputFactory2 etc.