Use older libraries and exclude some

[dr0i]

See #334.

[dr0i]

There is the issue to test OAIPMH still open, I find it not so easy to test this without doing an actual httpUrlConnection which we agreed on to not do in the library. I updated #325 to point to the encoding issue to test that also.

[dr0i]

Got the branch built by using proper version of xalan library. Requesting your review again @fsteeg.

[fsteeg]

As discussed in the standup, I tried keeping the Xalan dependency at 2.7.1, keeping the exclusions. This got me a NoClassDefFoundError org.apache.xml.serializer.TreeWalker. Searching that, I found a solution of setting a system property: System.setProperty("javax.xml.transform.TransformerFactory", "com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl"). This fixes that TreeWalker error, but also, when removing the exclusions, the original error. I then moved the dependency up to 2.7.2, all still looks good for me. Pushing that here.

[dr0i]

Seems to work.
Pro: it uses the newest xalan version.
Con: we have more dependencies. Also, your solution is somwhere commented as a "workaround" (I don't know what's the implication).

What way is the better one?

[fsteeg]

Con: we have more dependencies.

Why more dependencies? You mean setting the property?

Also, your solution is somwhere commented as a "workaround"

As far as I understand, it's a workaround to set it yourself as a user of their project, and their solution was to do it in their own code: maxjiang153/Openfire@3d88aaf (in that sense, it would be a workaround if a metafacture user had to set it in their own code).

What way is the better one?

I think it's better to use the newer Xalan lib, which should avoid other issues.

[dr0i]

Why more dependencies? You mean setting the property?

I mean now there are no exclusions of libraries. So there are more dependencies. Or does it not work in this way? Do exclusions none-the-less load dependencies and set them into the classpath? If that's the case, dump my idea. Otherwise, I so often experienced problems with libraries on the classpath having the same name but coming in different versions that it's sometimes real hard to find the problem. So, avoiding unnecessary dependencies is better in my experience.

Re workaround:
yes I think you are right.

[fsteeg]

I mean now there are no exclusions of libraries. So there are more dependencies. Or does it not work in this way?

I think the problem is always a conflict of transitive dependencies. So by excluding them, we actually say: don't use the ones pulled in by Xalan itself here. They are still dependencies of Xalan, so they need to come from some other place. Where is actually undefined at this point, so while often a good solution to fix a problem, I think exclusions actually increase the intransparency. Without exclusions, it's clear the library uses the dependencies as it declares them itself (in its pom etc.).

[dr0i]

come from some other place. Where is actually undefined at this point,

It comes with Java Class Library.

The dependencies in the metafacture-biblio module have impact on other modules, like e.g. org.metafacture.metamorph.test.MetamorphTestCaseTest, so I still think using as less dependencies makes the whole more stable. Also transparency.

For the complex of XML handling in java see also https://stackoverflow.com/questions/11952289/serializing-supplementary-unicode-characters-into-xml-documents-with-java. It's quite baffling or say: complex to see what uses when.

[fsteeg]

It comes with Java Class Library.

The xml-apis sound like that, but xalan.serializer and xercesImpl too?

In general however, I don't think it's less dependencies, it's just a question who pulls them. Xalan has these three dependencies that are excluded, it needs to get them somewhere, right? But I don't really see where. Running ./gradlew :metafacture-runner:dependencies only shows them as the Xalan dependencies. So I don't know. Both ways are a little weird, running with 2.7.0 + exclusions instead of 2.7.2 or setting an internal implementation class in a system property. Since both contain weirdness (exclusions, property) I'd favor the higher version.

[dr0i]

The package java.xml as part of the Java Class Library comes with copies of xalan and xerces, see e.g. in https://stackoverflow.com/questions/11952289/serializing-supplementary-unicode-characters-into-xml-documents-with-java. :

With the default JDK (1.6 and 1.7), TransformerFactory returns a com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl, which uses com.sun.org.apache.xml.internal.serializer.ToXMLStream ...

It's worthwhile to read that section.

[fsteeg]

The package java.xml as part of the Java Class Library comes with copies of xalan and xerces

Right, now I understand. Then how about this: we add back the exclusions, but use 2.7.2 + property.

So we have both the latest version and reduced XML dependency copies (pushed in 3ecdb47).

[dr0i]

but use 2.7.2 + property

would love to, but then tests in other modules fail (see note here #335 (comment))

[fsteeg]

would love to, but then tests in other modules fail (see note here #335 (comment))

Right, sorry, only ran the Flux test. We could set the property in the build for metamorph-test: ee0e624

[dr0i]

We could set the property in the build for metamorph-test

Hm, but you then change a test which was supposed to work as it is. Probably then one would have to set this property in all the productive apps to also work, and this would be a "break of the api".

[fsteeg]

Hm, but you then change a test which was supposed to work as it is. Probably then one would have to set this property in all the productive apps to also work, and this would be a "break of the api".

Hm, yes, that is true. So 2.7.2 + exclusions is no option. I'd still prefer 2.7.2 + property, since that keeps the workaround very local, to OaiPmhOpener only, while the rest of the biblio package and the rest of metafacture can use the newer 2.7.2. But if you prefer 2.7.0 + exclusion, that's fine with me too.

[dr0i]

Disucussed offline:
Both approaches have their weiredness. As Xalan > 2.7.0 somehow interferes with other java native modules I tend to not use it atm. But we should keep this in the back of our minds and may come back to the "System-Property-solution" if the need arises.