Hibernate 7 - Step 1#15654
Conversation
Copy all source, test examples, BOMs, and build config from the hibernate5 namespace to hibernate7 so that the real hibernate7 PR can be reviewed as a true delta rather than a sea of new files. Modules added: - grails-hibernate7-bom (copy of grails-hibernate5-bom) - grails-data-hibernate7-core, spring-orm, grails-plugin, dbmigration, spring-boot, docs - grails-test-examples/hibernate7 (12 projects mirroring hibernate5) - gradle/hibernate7-test-config.gradle (skipHibernate7Tests flag) Build infrastructure: - publish-root-config.gradle: register hibernate7 modules for publishing - SbomPlugin.groovy: add LGPL exemptions for hibernate5 artifacts used by hibernate7 staging modules - settings.gradle: include all hibernate7 projects
|
This PR is #1 for the hibernate PR. Basically, I cloned the hibernate5 projects and then created this PR as if they were the hibernate 7 artifacts. This then allows the actual hibernate PR to be meaningful since we can better see what changed. It's not perfect, but it did cut out about 400+ files to review this way. |
|
I went ahead and renamed |
matrei
left a comment
There was a problem hiding this comment.
Thank you @jdaugherty !
There are some TCK tests that are failing because they only have
@IgnoreIf({ System.getProperty('hibernate5.gorm.suite') })
and not
@IgnoreIf({ System.getProperty('hibernate7.gorm.suite') })
|
Per discussion, I've pulled forward the addAllDomainClasses, the common base spec for mongo, & the package renames from tests -> specs |
06fe15d to
f18465a
Compare
|
Should commit 06fe15d be on this branch? |
|
@matrei i had removed it from this branch since it's to fix the datastore changes in the other branch. |
|
@matrei Actually, it looks like it got readded somehow, let me try to take it off again |
6783934 to
4c5c563
Compare
|
@matrei I believe I have removed this now. |
|
I think we should revert any changes that are not directly related to Hibernate 7 compatibility. Any other changes that we still want to keep should be split out into separate, focused PRs targeting the appropriate version branch. If we, as a project, discuss and agree that we want to add PMD and Jacoco, those should also be introduced in separate PRs. That’s my two cents. |
sbglasius
left a comment
There was a problem hiding this comment.
This is an approval with a "Why?" and a caveat.
Why are there so many unrelated changes. It makes it impossible to get through all files.
The caveat is, that I have assumed, that all files in grails-data-hibernate7 are a plain copy of that in grails-data-hibernate5
Looking through each and every single file in this humongous PR would take more than the couple of hours that I have already spent on it.
There are none of my comments that requires code changes, they are more there out of frustration.
You can accept my approval if you feel it's valid.
| Class<?> transformerClass = getClass().getClassLoader().loadClass("org.grails.async.transform.internal.DefaultDelegateAsyncTransactionalMethodTransformer"); | ||
| return (DelegateAsyncTransactionalMethodTransformer) transformerClass.getDeclaredConstructor().newInstance(); | ||
| } catch (Throwable e) { | ||
| } catch (Exception ignored) { |
There was a problem hiding this comment.
One explanation is that it's bad coding style to catch Throwables. And ignored is an IntelliJ thing 🤔
| id 'org.apache.grails.buildsrc.sbom' | ||
| id 'org.apache.grails.gradle.grails-code-style' | ||
| id 'org.apache.grails.gradle.grails-code-analysis' | ||
| id 'org.apache.grails.gradle.grails-jacoco' |
There was a problem hiding this comment.
I'm commenting on this once: Why did this have to go into this PR. It's all noise making it almost impossible to review.
There was a problem hiding this comment.
Acknowledging - this one is still in PR-A. The two added plugin lines (grails-code-analysis and grails-jacoco) are part of the build/test infrastructure that @borinquenkid argued should stay on stage-hibernate7 so Step 2 (#15568) doesn't have to inherit the same scaffolding. If you'd prefer the analysis/jacoco adoption extracted to its own PR, I can do that. Leaving open for your call.
|
|
||
| import grails.persistence.support.PersistenceContextInterceptorExecutor | ||
| import grails.async.decorator.PromiseDecorator | ||
| import grails.persistence.support.PersistenceContextInterceptorExecutor |
There was a problem hiding this comment.
Why the reformat in this PR.
| import java.util.concurrent.atomic.AtomicBoolean | ||
| import java.util.function.Consumer | ||
|
|
||
| import groovy.transform.CompileStatic |
There was a problem hiding this comment.
Import order noise unrelated to this PR changes.
|
|
||
| class MongoCascadeSpec extends GrailsDataTckSpec<GrailsDataMongoTckManager> { | ||
| class MongoCascadeSpec extends MongoDatastoreSpec { |
There was a problem hiding this comment.
Was a refactor of GrailsDataTckSpec<GrailsDataMongoTckManager> to MongoDatastoreSpec needed? If yes, ok, if no, then it's just another source of noise.
There was a problem hiding this comment.
Extracted to #15685 (PR-E: MongoDatastoreSpec base class + mongo package rename) - the MongoDatastoreSpec refactor plus the prerequisite mongo tests/ -> specs/ rename was reverted from PR-A in ba235da. Your question 'was the refactor needed?' can be debated atomically there. Keeping this thread open until #15685 is resolved.
| import org.grails.datastore.mapping.query.Query | ||
| import org.slf4j.LoggerFactory |
There was a problem hiding this comment.
Acknowledging - the import re-order in this file is from the mongo styling commit (954f89e Pull forward styling changes in mongo) which wasn't extracted. The two factual changes left are: groovy.util.logging.Slf4j moves into the third-party-imports block, and MultiTenancySettings/SystemPropertyTenantResolver swap positions within the org.grails group. Let me know if you want this extracted; otherwise leaving as-is to avoid spawning another PR.
|
|
||
| import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec | ||
|
|
||
| abstract class MongoDatastoreSpec extends GrailsDataTckSpec<GrailsDataMongoTckManager> { |
There was a problem hiding this comment.
This refactor should have been done in a later PR. Adding to the size of the PR.
| void 'Test the list method returns a PagedResultList with pagination arguments'() { | ||
| given: 'A bunch of people' | ||
| createPeople() | ||
|
|
||
| when: 'A detached criteria instance is created and the list method used with the max parameter' | ||
| def criteria = new DetachedCriteria(Person) | ||
| criteria.with { | ||
| eq('lastName', 'Simpson') | ||
| eq 'lastName', 'Simpson' |
There was a problem hiding this comment.
This is an unneeded change for this PR. Removing parentheses should have been done in a clean-up PR.
Resolve add/add conflict in grails-data-hibernate5/dbmigration/src/test/resources/logging.properties by keeping the 8.0.x version (only difference was a trailing newline). Assisted-by: claude-code:claude-4.7-opus
…phql The hibernate7 stage branch removed the hibernate5Version project property from gradle.properties (commit ac0f947) in favor of letting BOM platforms resolve hibernate versions. After the merge from 8.0.x brought the renamed grails-test-examples/graphql/* apps into this branch, their build files still referenced "$hibernate5Version" directly and broke configuration across every CI job (validate-deps, code style, code analysis, rat-audit, codeql, all build + functional + mongo + hibernate5/7 test matrices, groovy-joint build). Drop the explicit version from the hibernate-core-jakarta and hibernate-ehcache coordinates and let `implementation platform(project(':grails-bom'))` (already declared in every affected file) supply the version, matching the pattern used in grails-data-hibernate5/core/build.gradle and the other test-example modules under grails-test-examples/. Assisted-by: claude-code:claude-4.7-opus
|
@matrei The missing @IgnoreIf({ System.getProperty('hibernate7.gorm.suite') }) checks on the TCK tests were actually fixed by commit 5ac6e2c and should all be passing now. Regarding the "unrelated changes" making this PR too large to review (cc @sbglasius): The size of this PR is almost entirely due to the 1:1 file clone of the hibernate5 namespaces into hibernate7 The styling and analysis changes are actually minuscule and isolated:
Since Step 1 is just the baseline clone, pulling these build/infrastructure changes forward keeps Step 2 clean. Given this, are we good to merge? |
|
You have putten way more effort into this, than I have. If this is a requirement for part 2 (the big change) then go ahead and merge. |
…doc change Per @matrei and @sbglasius review feedback on PR #15654: 1. Delete unused logback.groovy files in dbmigration and graphql plugin modules. Logback removed support for the Groovy configurator in 1.2.9 (referenced in grails-doc/src/en/guide/conf/config/logging.adoc:27), so these files have been silently ignored since the Logback 1.3.x upgrade. No build.gradle in this repository declares a logback-groovy-classic dependency. The remaining logback configuration uses logback.xml and logback-spring.xml, which is the documented and supported format. Files removed (symmetric across h5 and h7): - grails-data-hibernate5/dbmigration/grails-app/conf/logback.groovy - grails-data-hibernate5/dbmigration/src/test/resources/logback.groovy - grails-data-hibernate7/dbmigration/grails-app/conf/logback.groovy - grails-data-hibernate7/dbmigration/src/test/resources/logback.groovy - grails-data-hibernate7/dbmigration-core/src/test/resources/logback.groovy - grails-data-graphql/plugin/grails-app/conf/logback.groovy 2. Revert the unrelated package-rename change in grails-data-docs/guide-developer/src/main/docs/stepByStep.adoc. The one-line change from "import grails.gorm.tests.*" to "import grails.gorm.specs.*" was unrelated to the Hibernate 7 clone and flagged as noise by both reviewers. Note on @matrei's TCK @IgnoreIf concern: commit 5ac6e2c already added the missing System.getProperty('hibernate7.gorm.suite') checks to FirstAndLastMethodSpec, NullValueEqualSpec, OptimisticLockingSpec, and ValidationSpec. A scan of grails-datamapping-tck confirms no remaining files reference hibernate5.gorm.suite without also referencing hibernate7.gorm.suite. The larger pull-forward refactors flagged as noise (async cleanup, mongo base TCK refactor, TCK style cleanups) will be extracted into separate PRs targeting 8.0.x so they can be reviewed and debated on their own merits without holding up the Step 1 clone. Assisted-by: claude-code:claude-4.7-opus
This commit reverts the portions of this branch that have been split out into standalone PRs against 8.0.x. The goal is to shrink the PR-A review surface to focus on the actual hibernate5 -> hibernate7 clone and let the unrelated cleanups be reviewed on their own merits. Once PRs B/C/D/E land on 8.0.x and 8.0.x is merged into this branch, the reverted changes will arrive through the merge so the final state of stage-hibernate7 is unchanged - only the diff visible on this PR is reduced. Reverted content: Async defensive cleanup (PR #15682, PR-B) 9 files in grails-async/. Reverts e3cad41. Null-safety guards, @OverRide annotations, catch(Exception ignored) over catch(Throwable), constant-on-left equality, varargs. addAllDomainClasses helper + adoption (PR #15683, PR-C) Helper method on GrailsDataTckManager plus all callers across the data mapping test suites. Reverts ed0916d plus all orphan callers added by subsequent commits (50 specs) so the branch compiles without the helper method. MongoDatastoreSpec base class + mongo package rename (PR #15685, PR-E) Reverts c8cb43f (MongoDatastoreSpec introduction + ~107 spec refactors) and the mongo portion of 01c27f9 (8 file renames grails.gorm.tests -> grails.gorm.specs plus 18 import-update modifications in non-renamed mongo specs). DetachedCriteriaSpec command-chain syntax (PR #15684, PR-D) 18 `eq(...)` and `like(...)` parens restorations in the TCK DetachedCriteriaSpec. Reverts the parens hunk of ee4ab14 while leaving the rest of that commit (setupSpec addition, import reorder, and the other 35 TCK file changes) intact. NOT reverted (intentionally kept in PR-A): - The hibernate5 -> hibernate7 baseline clone (~100k lines, the actual work being reviewed) - 89 hibernate5 and 90 hibernate7 grails.gorm.tests -> grails.gorm.specs package renames (non-mongo portion of 01c27f9, since these touch the cloned tests and reverting would require re-doing both sides) - 18 grails-datamapping-core-test package renames (non-mongo portion of 01c27f9) - All other "Pull forward..." commits (styling, build config, test additions, etc.) which the reviewers have not specifically objected to Assisted-by: claude-code:claude-4.7-opus
|
@sbglasius @matrei @borinquenkid @jdaugherty I tried to address all the comment concerns by carving out pieces of the Step 1 PR into separate PRs Merge order
Merge PR-B #15682 (#15682) (chore/async-defensive-cleanup) PR-C #15683 (#15683) (refactor/tck-add-all-domain-classes) PR-D #15684 (#15684) (chore/tck-detached-criteria-style)
|
|
@jamesfredley We should carve out the bom-split, jacoco, and the code-analysis changes as well revert the |
|
@matrei I will carve these out, thank you for reviewing it again. |
|
We definitely should carve out the package name change, but then we should discuss if we want to keep it or not. The naming convention for Spock are specs not tests. My opinion is that you don't keep things that do not follow convention. |
|
Follow-up to the merge-order comment above. Per @matrei's review, I've carved the remaining non-clone changes out of this PR into focused, single-topic PRs so each can be reviewed - and accepted or declined - on its own. Worth restating: this Step 1 PR was never just "copy hibernate5 to hibernate7" - it also carried several independent improvements, and bundling them is what made it hard to review. Full carve-out stackAlready split (B-E):
Newly split (F-I), covering @matrei's request to carve out the bom-split, jacoco, and code-analysis, and to revert
Where this leaves usEach topic is now independently reviewable. I don't expect all of these to land - PMD/Jacoco need project agreement (@matrei), and the For whichever of F-I we agree to keep, I'll shrink PR-A the same way as B-E: a revert commit on the stage branch, so once 8.0.x is merged back the final state is unchanged and only PR-A's review surface shrinks. I'm holding the PR-A reductions for F-I until we settle which ones we're keeping, to avoid reverting something we then decide to retain. Updated merge order
|
) Continues shrinking the PR-A review surface by reverting the portions of this branch split into standalone PRs against 8.0.x, matching the established B/C/D/E revert pattern. Once these land on 8.0.x and 8.0.x is merged back into this branch, the reverted changes return through the merge, so the final state of stage-hibernate7 is unchanged - only the diff visible on PR-A is reduced. Reverted content: grails-code-analysis convention plugin (PR #15686, PR-G) GrailsCodeAnalysisPlugin/Extension, GrailsViolationAggregationPlugin (+specs), the codeanalysis workflow, the pmd ruleset resource, the codenarcFix improvements and config relocation in GrailsCodeStylePlugin, spotbugs/pmd build deps, and the grails-code-analysis apply-line across all modules (incl. the h7 clones). grails-jacoco convention plugin (PR #15687, PR-H) GrailsJacocoPlugin (+spec), the coverage workflow, and the grails-jacoco apply-line across all modules (incl. the h7 clones). NOT reverted (intentionally kept): - The codenarc violation fixes (f18465a) - reverting would re-introduce style violations; they are plugin-independent. - All hibernate7 BOM/clone content (the actual PR-A work). Verified: ./gradlew help configures the root, grails-gradle, and grails-forge builds; :build-logic-root:build-logic:test passes. Assisted-by: claude-code:claude-4.7-opus
…R-F #15689) Continues shrinking the PR-A review surface, matching the established B/C/D/E revert pattern. Once these land on 8.0.x and 8.0.x is merged back into this branch, the reverted changes return through the merge, so the final state of stage-hibernate7 is unchanged - only the diff visible on PR-A is reduced. Reverted content: grails.gorm.tests -> grails.gorm.specs package rename (PR #15688, PR-I) Renames the test package back to grails.gorm.tests (the 8.0.x convention) across the three affected test trees: hibernate5 core (90 files), hibernate7 core (90 files), and grails-datamapping-core-test (18 files). The mongo portion was already reverted in the prior B/C/D/E revert. Hibernate 5 Micronaut BOM split (PR #15689, PR-F) Removes grails-hibernate5-micronaut-bom, its sample app (micronaut-hibernate5), and all h5-micronaut references in settings.gradle, dependencies.gradle, publish-root-config.gradle, validateMicronautBom, the doc-generation task, and the Micronaut config/upgrade guides. The generic grails-micronaut-bom is retained. NOT reverted (intentionally kept - the actual PR-A work): - The hibernate5 -> hibernate7 baseline clone - grails-hibernate7-micronaut-bom, its sample app, and all h7 references (settings/dependencies/publish/plugin/doc-gen/guides) Verified: ./gradlew help configures cleanly; compileTestGroovy passes for grails-datamapping-core-test, grails-data-hibernate5-core, and grails-data-hibernate7-core (the three renamed trees); grails-hibernate7 -micronaut-bom still publishes. Assisted-by: claude-code:claude-4.7-opus
Carve-out complete - detailed merge orderAll the changes that @matrei asked to be reviewed independently have now been carved out of this PR into focused, single-topic PRs. This comment is the authoritative merge order and a conservation check confirming nothing was lost. The full stack
Dependencies
How this PR was reducedEach carved topic was reverted on this branch (
The reverts only shrink this PR's visible diff. Once a carved PR lands on Recommended merge sequence
Hibernate 7 mirror caveat (important)The carved PRs are based on
These were removed here so this PR compiles cleanly without the carved plugins; re-applying them to the h7 clone is part of finalizing this PR after the merge-back, and keeps the h7 modules a faithful mirror of h5. Conservation checkEvery file the reverts removed from this branch is present in its standalone PR (verified): all Copilot reviewAll Copilot feedback across B-I has been addressed, replied to, and resolved (zero unresolved Copilot threads remain on any of them). |
Resolve conflicts in gradle.properties (take gradleToolingApiVersion 9.5.1 from 8.0.x; keep hibernate5Version removed per Hibernate 7 step 1) and grails-bom/micronaut/build.gradle (adopt updated comment documenting the tools.jackson exclusion added on 8.0.x). Assisted-by: claude-code:claude-4.8-opus
Commit ba235da reverted the addAllDomainClasses helper from GrailsDataTckManager and its callers, but six grails-data-mongodb specs were missed, leaving them calling a method that no longer exists. This caused initializationError failures in :grails-data-mongodb-core:test (MissingMethodException: GrailsDataMongoTckManager.addAllDomainClasses). Restore these specs to use manager.domainClasses.addAll([...]) (identical to the 8.0.x baseline and behaviorally equivalent to the removed helper), so the branch compiles without the helper as intended. The helper and its callers will return together when PR-C (#15683) lands on 8.0.x and is merged back. Assisted-by: claude-code:claude-4.8-opus
✅ All tests passed ✅🏷️ Commit: 3cf0cc7 Learn more about TestLens at testlens.app. |
creating this request by cloning the existing hibernate five modules so that we can easily review the hibernate seven changes between hibernate five and seven
Copy all source, test examples, BOMs, and build config from the
hibernate5 namespace to hibernate7 so that the real hibernate7 PR
can be reviewed as a true delta rather than a sea of new files.
Modules added:
Build infrastructure: