4:39 p.m.
On Thu, Oct 27, 2016 at 10:09 AM, Gunnar Morling <gunnar(a)hibernate.org> wrote:
2016-10-27 15:55 GMT+02:00 Scott Marlow <smarlow(a)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(a)hibernate.org>
> wrote:
> >
> >
> > 2016-10-27 14:59 GMT+02:00 Scott Marlow <smarlow(a)redhat.com>:
> >>
> >> On Thu, Oct 27, 2016 at 4:49 AM, Gunnar Morling
<gunnar(a)hibernate.org>
> >> wrote:
> >> > Hi,
> >>
> >> Hi,
> >>
> >> >
> >> > 2016-10-27 4:27 GMT+02:00 Scott Marlow <smarlow(a)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.
> >
> >