Use javax.transaction 1.2 dependencies only, while keeping [1.1,2) Import range

grgrzybek commented 3 years ago

There are many Maven artifacts providing javax.transaction package, most importantly:

org.apache.geronimo.specs/geronimo-jta_1.1_spec
org.jboss.spec.javax.transaction/artifactId=jboss-transaction-api_1.1_spec
org.jboss.spec.javax.transaction/artifactId=jboss-transaction-api_1.2_spec
javax.transaction/javax.transaction-api
jakarta.transaction/jakarta.transaction-api

Of course in multiple versions. Jakarta libraries provide either javax.transaction or jakarta.transaction packages.

IMO we need only one such dependency. At runtime, it can be e.g., Karaf that provides relevant packages.

grgrzybek commented 3 years ago

We still should assume that JDK8 is the most used JDK, but we have to be aware of what's happening in the recommended JDK (11 and soon, 17).

(See Add module java.transaction to export API javax.transaction for details)

JDK 8 has two packages in rt.jar:

javax.transaction (but only with 3 exceptions required by Corba)
javax.transaction.xa with 3 classes (XAException, XAResource and Xid)

JDK 11 has only one javax.transaction.xa package in javax.transaction.xa.jmod and the module is defined like this:

module java.transaction.xa {
    exports javax.transaction.xa;
}

jakarta.transaction-api-1.3.3-sources.jar and javax.transaction-api-1.3-sources.jar differ only by comment headers.

javax.transaction-api-1.2.jar contains (as expected) javax.transaction.xa. The exception classes in this version miss serialVersionUID.

Bundles for JTA 1.3 import javax.transaction.xa package without exporting them:

Export-Package: \
  javax.transaction;\
    uses:="javax.interceptor,javax.transaction.xa,javax.enterprise.util,javax.enterprise.context";\
    version="1.3.3"

Import-Package: \
  javax.enterprise.context,\
  javax.enterprise.util,\
  javax.interceptor;version="[1.2,1.2.98)",\
  javax.transaction;version="1.3.3",\
  javax.transaction.xa

Bundle for JTA 1.2 has:

Export-Package: \
  javax.transaction;\
    uses:="javax.enterprise.context,javax.enterprise.util,javax.transaction.xa,javax.interceptor";\
    version="1.2",
  javax.transaction.xa;\
    version="1.2"

Import-Package: \
  javax.enterprise.context,\
  javax.enterprise.util,\
  javax.interceptor;version="[1.2,1.2.98)",\
  javax.transaction;version="1.2",\
  javax.transaction.xa;version="1.2"

So the desired (IMO) state is:

OSGi runtime (like Karaf) should export javax.transaction.xa package from system bundle because it's part of JDK8 and JDK11 - this package is not a problem
OSGi runtime should not export full javax.transaction package from JDK8, because JDK8 doesn't have complete package. It should (in theory) not export this package from JDK11, simply because there isn't any...

grgrzybek commented 3 years ago

This situation is complicated because of DBCP2...

DBCP-445 and DBCP-454 introduced https://github.com/apache/commons-dbcp/commit/7363c4040d4d0bfc89761c904f4332c69420099c where the import declaration is:

javax.transaction.xa;version="1.1.0";partial=true;mandatory:=partial

Karaf, with KARAF-6715 tackled DBCP2 resolution problem by adding proper version 1.1 to javax.transaction/javax.transaction.xa packages already exported with partial=true; mandatory:=partial...

But I just realized that it's wrong or at least very confusing (both on DBCP2 and Karaf side)...

DBCP2 should NOT import javax.transaction.xa as partial (even if this is not a special attribute for Export-Package directive) because there's nothing partial about this package.
Karaf should NOT export javax.transaction.xa from system bundle as partial, because it's not partial (it's complete) both on JDK8 and JDK11
Karaf should NOT export javax.transaction at all on JDK11 from system bundle.

grgrzybek commented 3 years ago

My tests show few confusing scenarios...

a bundle with just Import-Package: javax.transaction won't resolve against clean Karaf, because the package is exported with partial=true; mandatory:=partial. Same for javax.transaction.xa package
DBCP2 won't resolve against clean Karaf, because it uses partial=true; mandatory:=partial only for javax.transaction.xa package (which is absurd)
It's not possible to resolve mvn:javax.transaction/javax.transaction-api/1.3 (even with required interceptor-api, cdi-api, el-api and javax.inject), because it imports javax.transaction.xa - it doesn't provide own version of this package and Karaf exports it with partial=true; mandatory:=partial
without boot delegating javax.transaction.* packages, with mvn:javax.transaction/javax.transaction-api/1.2, DBCP2 finally resolves (as well as my test bundle):
- javax.transaction.InvalidTransactionException is loaded from system bundle
- it starts with DBCP2 wiring and finds a provider of javax.transaction package
- then it checks javax.transaction-api wiring - even if this bundle contains this class, according to resolution rules, imported packages are checked first (nothing is found) and required packages are checked later - javax.transaction-api has ... Require-Bundle: system.bundle, so all system bundle's exported packages are added as javax.transaction-api required packages.
- javax.transaction.UserTransaction is loaded from javax.transaction-api bundle because javax.transaction package is exported by this bundle - even if required packages are checked, this interface is not found in system bundle, so the content of javax.transaction-api bundle is checked
with boot delegation of javax.transaction.* packages, org.apache.felix.framework.BundleWiringImpl#shouldBootDelegate() returns true quickly and system bundle is used for javax.transaction.InvalidTransactionException, but CNFE is thrown for UserTransaction and normal imports/requires are checked

After the above analysis, I see that everything works thanks to ... Require-Bundle: system.bundle of javax.transaction-api bundle.

grgrzybek commented 3 years ago

It's not so good with jakarta.transaction-api bundle. It can't be resolved on clean Karaf, as it imports javax.transaction.xa package (same as javax.transaction-api bundle), but without Require-Bundle: system.bundle, so it can't pass through the weird partial=true; mandatory:=partial markings on exported javax.transaction.xa package in Karaf's jre.properties.

grgrzybek commented 3 years ago

See: https://github.com/javaee/javax.transaction/commit/cafce67773652a48d37f4f18cd317cfbd6371802#diff-5b28dfac4685527f0aa99f6fde21966a5347c363d64baf1680fd1db0b0f14e73

grgrzybek commented 3 years ago

For completeness:

org.jboss.spec.javax.transaction/jboss-transaction-api_1.2_spec doesn't export javax.transaction.xa package
org.apache.geronimo.specs/geronimo-jta_1.2_spec is at 1.0-alpha-1 version, so I don't bother (though it exports proper packages)
I don't care about JTA 1.1

grgrzybek commented 3 years ago

@gnodet hello (cc: @jbonofre @ffang), I'd like to refer to https://github.com/apache/karaf/commit/d69f71a8fe550357b9d6addfe951c3fe1e1d7eee which you may not remember (2009!). I understand how the partial attribute works. It is shown in 3.13.1 "Require-Bundle" chapter of OSGi core (partial is just arbitrary name, the point is it is mandatory).

I'm just (see my above comments) trying to justify why Karaf still uses this attribute in javax.transaction and javax.transaction.xa exports from system bundle (etc/jre.properties). What I found is:

javax.transaction.xa package has to be exported from bundle 0 no matter what - simply because javax.sql.XAConnection interface uses javax.transaction.xa.XAResource interface.
however, javax.transaction.xa should not be exported as partial, because all JDKs (7, 8, 9+) provide full content of this package (namely 3 interfaces: XAResource, XAException, Xid)
If Karaf declares that it exports javax.transaction from system bundle, it may export it only as partial, because JDK 8 contains only 3 exception classes (needed by Corba), but JDK11 doesn't provide such package at all.
Also Karaf (by default) shouldn't boot delegate javax.transaction package, because it doesn't put JTA API Jar into lib directory (not a bundle)

So definitely my attempt to fix DBCP2 usage in Karaf (see: https://issues.apache.org/jira/browse/KARAF-6715), simply by adding version="1.1" is only a partial fix of general JTA usage in Karaf.

Karaf's transaction-api feature, with non-dependency:

mvn:javax.transaction/javax.transaction-api/1.2

is a good thing, but works only by accident, because this particular API bundle has:

Require-Bundle: system.bundle

Which ensures that javax.transaction.xa.XAResource is always loaded from JDK itself:

DBCP2 loads it from system bundle because it has Import-Package: javax.transaction.xa;mandatory:=partial;partial=true;version="1.1.0", so it always wires this package to system bundle (again, only because Karaf exports this package with this attribute in etc/jre.properties)
Bundles simply importing javax.transaction.xa package (without partial tricky attribute) wire to javax.transaction-api bundle, but this particular bundle is wired to system bundle via Require-Bundle, so it uses system bundle before checking own content

If mvn:javax.transaction/javax.transaction-api/1.2 didn't have (see https://github.com/javaee/javax.transaction/commit/cafce67773652a48d37f4f18cd317cfbd6371802#diff-5b28dfac4685527f0aa99f6fde21966a5347c363d64baf1680fd1db0b0f14e73) Require-Bundle: system.bundle, ~~DBCP2 would use different javax.transaction.xa.XAResource than bundles importing this interface from javax.transaction-api leading to CCE.~~ and if Karaf wouldn't boot delegate javax.transaction.xa, DBCP2 would not be resolved, because it'd have been exposed to javax.transaction.xa package through 2 different chains - from system bundle that can have partial attribute and through javax.transaction-api.

I created https://issues.apache.org/jira/browse/DBCP-571 to check how we could resolve this issue once and for all.

grgrzybek commented 3 years ago

Another interesting facts.

https://github.com/eclipse-ee4j/jta-api/commit/79d477b6b1503099af62c2b25fcb854f4b17a7e3 confirmed that api and spec submodules of jakarta.transaction-api can be versioned separately, which ensures me that while the specification version is 1.3, API version of the javax.transaction package should stay at 1.2

https://github.com/eclipse-ee4j/jta-api/commit/9256156ed6ec5d3bf3bdd5c66ca0071f78b381b1 moved the code to api submodule, but top-level osgi.bundle file was not touched. So api/pom.xml, while still having:

<_include>-${basedir}/osgi.bundle</_include>

doesn't actually use this file, so Require-Bundle: system.bundle trick that made javax.transaction-api work, doesn't work with jakarta.transaction-api. See original trick here: https://github.com/javaee/javax.transaction/commit/cafce67773652a48d37f4f18cd317cfbd6371802#diff-5b28dfac4685527f0aa99f6fde21966a5347c363d64baf1680fd1db0b0f14e73

grgrzybek commented 3 years ago

I've created two PRs:

grgrzybek commented 3 years ago

All tests pass. The changes are:

all bundles that need it, import javax.transction with range [1.1,2)
all bundles that need it, import javax.transaction.xa without version range
all bundles that need it, import javax.resource.* with range [1.6,2)
pax-transx-tm-api Karaf feature uses <bundle dependency="true">mvn:javax.transaction/javax.transaction-api/1.3</bundle> (the one without javax.transaction.xa). 1.3 is to check if the feature validates with JDK itself, but it is designed to work with Karaf's transaction-api feature.
pax-transx-tm-api Karaf feature uses <bundle dependency="true">mvn:javax.transaction/javax.transaction-api/1.2</bundle> (the one with javax.transaction.xa). 1.2 is needed because also Geronimo JCA spec is needed before eclipse-ee4j/jca-api#120 is fixed

ops4j / org.ops4j.pax.transx

Use javax.transaction 1.2 dependencies only, while keeping [1.1,2) Import range #33