[hibernate-dev] Javassist dependency conflict in the ORM modules for WildFly

[Scott Marlow]

On Thu, Oct 27, 2016 at 10:09 AM, Gunnar Morling <gunnar at hibernate.org> wrote:
> 2016-10-27 15:55 GMT+02:00 Scott Marlow <smarlow at redhat.com>:
>>
>> I remember now why I didn't remove the JPADependencyProcessor
>> injection of Javassist in
>> https://github.com/wildfly/wildfly/pull/8474, it would cause a failure
>> in applications that embeds a copy of the ORM jars but the application
>> doesn't have the Javassisst jars on its classpath.  Currently,
>> JPADependencyProcessor ensures that Javassist is available to such an
>> application.
>
>
> Why would one support such use case? If a user is adding their own ORM in
> the app, I'd expect them to do it completely, with all the dependencies of
> *that version*. I mean, how would you even know whether the Javassist
> version you are adding is the right one?

I think that exporting the Javassist classes from ORM was considered a
bad idea back then in 2011.  Now, we don't want to lose application
compatibility.

>
> And what happens if the user *is* providing Javassist with the app? Tbh.
> allowing the user to add their own ORM in the app seems even more a
> justification to not inject Javassist.

Sure, lets get to the point where we can allow the user to have their
own Javassist.

>
>> I could easily work around the above in the
>>
>> wildfly/testsuite/compat/src/test/java/org/jboss/as/test/compat/jpa/hibernate/HibernateJarsInDeploymentTestCase.java
>> test, however, this would break application compatibility, which is
>> more important.  This goes back to around 2011 when we first started
>> auto injecting the Javassist classes to applications that are using
>> Hibernate as a JPA persistence provider.  The next time where we would
>> be allowed to break application compatibility, would be on an EAP
>> .zero release.
>>
>> If https://github.com/wildfly/wildfly/pull/8474 ever gets merged into
>> WildFly, perhaps we could enhance JPADependencyProcessor with a
>> persistence unit check for a "don't inject javassist" hint.  Or maybe
>> that could help the ORM testsuite now, in which case we could create
>> an independent pull request for the "don't inject javassist" hint
>> handling.
>
>
> Would that hint be another "knob" to be set by the user? If so, I wouldn't
> like it too much, it's just such a technical detail we shouldn't bother the
> user with.

Yes, it would be another knob, however, it is likely to only be used
by the Hibernate ORM testsuite, as I haven't heard of any user request
for this.

>
> Another idea: can you examine the ORM module you are about to add and check
> wether it exports Javassist and if so, not auto-inject Javassist? Not sure
> whether it's doable in terms of capabilities of the module API.

I'm not 100%, we would likely need to explore what is available from
the application deployment
org.jboss.as.server.deployment.module.ModuleSpecification at the early
deployment phase that JPADependencyProcessor is called in.
ModuleSpecification seems to have getSystemDependencies() +
getUserDependencies() + getLocalDependencies() calls, but I'm not sure
how (deployment) expensive it would be to sequentially walk through
each module dependency and look for javassist classes.

>
>>
>>
>> On Thu, Oct 27, 2016 at 9:40 AM, Gunnar Morling <gunnar at hibernate.org>
>> wrote:
>> >
>> >
>> > 2016-10-27 14:59 GMT+02:00 Scott Marlow <smarlow at redhat.com>:
>> >>
>> >> On Thu, Oct 27, 2016 at 4:49 AM, Gunnar Morling <gunnar at hibernate.org>
>> >> wrote:
>> >> > Hi,
>> >>
>> >> Hi,
>> >>
>> >> >
>> >> > 2016-10-27 4:27 GMT+02:00 Scott Marlow <smarlow at redhat.com>:
>> >> >>
>> >> >>
>> >> >> > Unless I am mistaken, Gunnar is suggesting that the Hibernate ORM
>> >> >> > module
>> >> >> > (the WF module) export Javassist.  Not the application.
>> >> >
>> >> >
>> >> > Right, Hibernate ORM's module should be the one exposing it, not the
>> >> > application nor JipiJapa.
>> >>
>> >> JipiJapa has zero to do with this, we will create a pr later today to
>> >> remove the unneeded dependencies, which has nothing to do with this
>> >> conversation.
>> >
>> >
>> > Yes, there is this superfluous dependency, thanks for removing it.
>> >
>> > But the other issue is hat in JPADependencyProcessor (that's what I
>> > meant
>> > when referring to "JipiJapa", sorry if that's not correct), a dependency
>> > to
>> > Javassist is injected. This shouldn't be the case for the reasons I've
>> > pointed out. Also with the PR
>> > https://github.com/wildfly/wildfly/pull/8474
>> > you mentioned this seems to be the case.
>> >>
>> >>
>> >> >
>> >> > I've done the following changes locally:
>> >> >
>> >> > 1) Altered JPADependencyProcessor to not add the Javassist dependency
>> >> > to
>> >> > the
>> >> > deployment
>> >> > 2) Altered module.xml of jipijapa-hibernate5 to not depend on
>> >> > Javassist
>> >>
>> >> I did the same locally, which is an unused dependency.  I removed some
>> >> other unused dependencies other as well.  Gail and I will talk later
>> >> about removing these unused dependencies.
>> >>
>> >> > 3) Added a new module for the latest Javassist
>> >> > 4) Altered module.xml of ORM itself to depend on that new module
>> >> > *and*
>> >> > re-export it
>> >> >
>> >> > With all this in place, the tests in Hibernate Search pass
>> >> > successfully.
>> >> >
>> >> > Scott, do you think we can try another PR with that? Again, it
>> >> > doesn't
>> >>
>> >> https://github.com/wildfly/wildfly/pull/8474 is still open, no need
>> >> for a new PR when we already have an open one, with one exception, as
>> >> I didn't actually remove the export of Javassist to the application
>> >> classpath.  I forget why.  I'll try that locally and run the WildFly
>> >> testsuite to see if there is a failure.
>> >
>> >
>
>