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

[Scott Marlow]

I updated https://github.com/wildfly/wildfly/pull/8474 to make a push
for applying the pull request and also mentioned the desire to update
JPADependencyProcessor to not inject Javassist into applications.
Perhaps others from the WildFly team will disagree with me and make an
exception to break applications with Hibernate ORM embedded in a small
way, that has a tiny (one line change) workaround needed. :-)

On Thu, Oct 27, 2016 at 10:39 AM, Scott Marlow <smarlow at redhat.com> wrote:
> 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.
>>> >
>>> >
>>
>>