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 |
jdaugherty
changed the title
matrei
left a comment
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 |
jdaugherty
force-pushed
the
8.0.x-stage-hibernate7
branch
from
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 |
jdaugherty
force-pushed
the
8.0.x-stage-hibernate7
branch
from
6783934 to
4c5c563
Compare
|
@matrei I believe I have removed this now. |
|
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
The hibernate7Functional job in gradle.yml retained unpinned action references (actions/checkout@v6, actions/setup-java@v4, gradle/actions/setup-gradle@v5.0.0) that are not on the ASF GitHub Actions allow-list. Apache's org policy rejects non-allowlisted actions, causing the entire 'CI' workflow (gradle.yml) to end in startup_failure so none of its jobs ever ran. Every other job in this file was already updated to ASF-approved, SHA-pinned versions when 8.0.x was merged in; this aligns the PR-specific hibernate7Functional job with its hibernate5Functional twin (matching pins plus the cache-provider: basic option), allowing the workflow to start. Assisted-by: claude-code:claude-4.8-opus
✅ All tests passed ✅🏷️ Commit: 06ddb63 Learn more about TestLens at testlens.app. |
|
I'm trying to understand what's been done here and what's been agreed to. Originally, I was under the impression we were going to split off changes into a merge chain. My assumption then was we would revert as we went, but the opposite has been done here. I think we need to discuss more on this before merging as a result. I'm not confident we have actually reverted what we've said without a more detailed review. |
|
The micronaut-hibernate5-bom changes look to have been reverted, which is the wrong course of action. We have to split our boms so they set the right values when using a specific version of hibernate. I'm -1 on merging this until that's been restored. |
@matrei the entire point of this PR originally was to clone & split the boms. Are you suggesting we should do these as 2 PRs? If so, why did we introduce the hibernate 7 bom in this PR? |
|
I'm copying @jamesfredley's comment here so I can gather the state of these PRs & update the purposes:
Will update as I work through these PRs |
5 participants
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: