Exclude hibernate-ehcache

[jamesfredley]

resolves hibernate-core and hibernate-core-jakarta being on the classpath

#951

[matrei]

Couple of thoughts about this:

1. org.hiberate:hibernate-ehcache is, what I can see, not a dependency of neither grails-datastore-gorm-hibernate5 or grails-plugin and can be removed as such.
2. org.hibernate:hibernate-ehcache is a testRuntimeOnly dependency of grails-plugin as tests explicitly sets the hibernate:cache:region.factory_class to classes from org.hibernate:hibernate-ehcache in configuration.
3. org.hibernate:hibernate-ehcache is a runtimeOnly dependency of examples-grails-hibernate as it explicitly sets the hibernate:cache:region.factory_class to org.hibernate.cache.ehcache.EhCacheRegionFactory in application.yml.
4. Making org.jboss.spec.javax.transaction:jboss-transaction-api_1.3_spec a dependency of grails-plugin should not be needed.

I believe it is the responsibility of the consumer of org.hibernate:hibernate-ehcache to make sure it is on the classpath and that an incompatible version of org.hibernate:hibernate-core is excluded from it, if it causes conflicts.

@jamesfredley If you agree with these thoughts, would you like me to add them to this PR?

[jamesfredley]

@matrei

org.hiberate:hibernate-ehcache is, what I can see, not a dependency of neither grails-datastore-gorm-hibernate5 or grails-plugin and can be removed as such.

This is true and it was removed around Sept of 2024, but then broke a number of tests in other Grails projects which depended upon it being provided by the hibernate5 plugin historically. So it is a trade off of a breaking change for Grails apps using ehcache during Grails 7 upgrade vs removing a dependency that is not directly used by the plugin. ehache does not exist for hibernate6, so when gorm-hibernate6 is ready, jcache will be part of the transition.

Making org.jboss.spec.javax.transaction:jboss-transaction-api_1.3_spec a dependency of grails-plugin should not be needed.

If we remove org.hiberate:hibernate-ehcache as a grails-plugin dependency, there are cases like #955 (comment) where the end user will need to include it, like the following:

implementation "org.hibernate:hibernate-ehcache:$hibernateVersion", {
    exclude group:'org.hibernate', module:'hibernate-core'
}
// required for org.hibernate:hibernate-ehcache to work with org.hibernate:hibernate-core-jakarta
implementation "org.jboss.spec.javax.transaction:jboss-transaction-api_1.3_spec:$jbossTransactionApiVersion", {
    exclude group:"jakarta.enterprise", module:"jakarta.enterprise.cdi-api"
}

A quick start to the list of locations that will need to have org.hibernate:hibernate-ehcache added:

• https://github.com/search?q=org%3Agrails+org.hibernate.cache.ehcache+language%3AYAML&type=code&l=YAML

[matrei]

So it is a trade off of a breaking change for Grails apps using ehcache during Grails 7 upgrade vs removing a dependency that is not directly used by the plugin

I’d prefer a breaking change in a major version update rather than continuing with the wrong approach. Why should every Grails app using org.grails.plugins:hibernate5 include this dependency, even if it’s not needed? This "kitchen-sink" approach adds unnecessary complexity and makes it harder for users to understand the libraries they depend on. I believe a cleaner, more intentional dependency structure benefits everyone in the long run.

If we remove org.hiberate:hibernate-ehcache as a grails-plugin dependency, there are cases like #955 (comment) where the end user will need to include it, like the following

I looked through the issues related to this PR but couldn’t find a project demonstrating the need for org.jboss.spec.javax.transaction:jboss-transaction-api_1.3_spec.

It seems that BookControllerUnitSpec works when adding testRuntimeOnly 'org.hibernate:hibernate-ehcache' to the grails-plugin project. What am I missing?

[jamesfredley]

@matrei Ok, please proceed with the changes.

It seems that BookControllerUnitSpec works when adding testRuntimeOnly 'org.hibernate:hibernate-ehcache' to the grails-plugin project. What am I missing?

That is excellent news, as I did not like adding that javax dependency.

I'll be glad to work on updating the other Grails projects after this breaking change.

[jdaugherty]

Can we update the upgrade guide for Grails 7 to include these instructions since it is a breaking change?

[matrei]

Can we update the upgrade guide for Grails 7 to include these instructions since it is a breaking change?

grails/grails-doc#951

[jamesfredley]

Add the following to document the right approach and avoid conflicts from the javax version?

, {
    exclude group:'org.hibernate', module:'hibernate-core'
}

[jamesfredley]

Add the following to document the right approach and avoid conflicts from the javax version?

, {
    exclude group:'org.hibernate', module:'hibernate-core'
}

[jamesfredley]

Add the following to document the right approach and avoid conflicts from the javax version?

, {
    exclude group:'org.hibernate', module:'hibernate-core'
}

[jamesfredley]

@matrei The documentation updates on grails/grails-doc#951 look great. What do you think about the 3 small changes I commented on the code?

[matrei]

@jamesfredley Well, that failed spectacularly😄 . I'll check what's needed to make it run again.

[jamesfredley]

@matrei It is the same error that I say initially.

The following does resolve it, although I still think it is suboptimal since it includes 19 javax.transaction.* classes.

// required for org.hibernate:hibernate-ehcache to work with org.hibernate:hibernate-core-jakarta
implementation "org.jboss.spec.javax.transaction:jboss-transaction-api_1.3_spec:$jbossTransactionApiVersion", {
    exclude group:"jakarta.enterprise", module:"jakarta.enterprise.cdi-api"
}

[matrei]

The following does resolve it, although I still think it is suboptimal since it includes 19 javax.transaction.* classes

@jamesfredley Yes, you are right. The question is where we put the responsibility to add this. I would like to say on the application as we have already concluded hibernate-ehcache is an application concern. But it can be added as a runtimeOnly dependency and I think the exclusion of jakarta.enterprise.cdi-api is not needed as it is marked as provided.

[jamesfredley]

@matrei I agree it is an application concern and am onboard with reducing the scope when we can. For this project we can add it as needed, for the 3 runtimeOnly/testRuntimeOnly locations in the projects.

[matrei]

I agree it is an application concern and am onboard with reducing the scope when we can. For this project we can add it as needed, for the 3 runtimeOnly/testRuntimeOnly locations in the projects.

@jamesfredley Excellent! Changes made, please review.

[jdaugherty]

I worry about this change. The gorm documentation (under Getting Started) recommends ehcache be included. Caching is a critical component of hibernate and we are definitely making this harder to include because of the extra gradle workarounds that are needed. We need to be sure to update the gorm docs for these workarounds so people understand this is only until we have hibernate 6.

[matrei]

The gorm documentation (under Getting Started) recommends ehcache be included.

Yes, exactly! The documentation does not say that hibernate-ehcache is provided transitively. It says to include it explicitly.

We need to be sure to update the gorm docs for these workarounds so people understand this is only until we have hibernate 6.

Yes, it looks very outdated and needs to be updated.

[jamesfredley]

I will work on PRs for the following documentation updates:

grails/grails-doc#953
grails/grails-core#14008
grails/grails-data-mapping#1876

[jamesfredley]

I will also try to preemptively address the following, which will break with this change.

https://github.com/search?q=org%3Agrails+org.hibernate.cache.ehcache+language%3AYAML&type=code&l=YAML