Repository navigation
Fix for when GORM id and version added to abstract base-class and not to domain class extending base-class - #16653
Conversation
### Changes - Both the GORM transformation and Grails fallback injector now place generated `id` and `version` fields only within the domain-class hierarchy. - Shared non-domain superclasses are left unchanged, so command objects no longer inherit unwanted persistence fields. - Existing domain inheritance and explicitly declared identity/version properties are preserved. - Added regression tests and documentation, including advice to perform a clean build after upgrading. ### Verification - Confirmed regressions fail before the fix and pass afterward. - Full tests passed in: - `grails-core` - `grails-datamapping-core` - `grails-datastore-core` - `grails-validation` - Broader simple-datastore/TCK and persistence suites passed. - Scoped code-style checks passed. - Documentation guide built successfully.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Thanks - the leak is real and the direction is right: a class outside grails-app/domain is not a domain class and should not acquire persistence properties. The furthest-relative injection dates to GRAILS-3209 (2010) and was written for domain hierarchies (its regression test used @Entity abstract class Parent); a plain Groovy parent only receives id/version because it is compiled in the same source set, while the same parent from a jar is left alone.
Three things before this can go in:
- Target branch. 8.0.0 has shipped, so 8.0.x is a patch line now, and this is a behavior change (see the compile break noted on the docs). Please retarget to 9.0.x and add an upgrade note there (
upgrading90x.adoc). - Two regressions in the new helper, inline on
AstUtils: an order-dependent duplicate identity when a plain class sits between two domain classes, and placement that depends on the memoizedisDomainClassname cache. - Docs: lead with a trait for sharing properties between a command object and a domain class (it works today without this change), and describe the plain-superclass rule in the GORM inheritance chapter.
I ran the four touched modules, the Hibernate 7 inheritance specs and grails-test-examples-gorm's DirtyCheckBindingSpec (an id bindable: true constraint on a plain @DirtyCheck base) on this head: all green.
| */ | ||
| static ClassNode getFurthestUnresolvedDomainParent(ClassNode classNode) { | ||
| ClassNode parent = classNode.getSuperClass() | ||
| while (parent != null && !parent.isResolved() && isDomainClass(parent)) { |
There was a problem hiding this comment.
This stops at the first plain parent, so a plain class between two domain classes breaks the one-identity rule and brings GRAILS-3209 back, depending on declaration order.
Probe: @Entity class Dog extends Mammal, abstract class Mammal extends Animal (plain), @Entity abstract class Animal. With Dog declared first, Dog and Animal both get id/version (8.0.x head: Animal only, in both orders). isDomainClass(Mammal) is false, so Dog receives the fields; Animal then inherits none and receives its own.
Could you return the furthest unresolved domain ancestor, walking past plain intermediates, and add that case in both orders to the where: tables here, in GormEntityTransformSpec and in DefaultDomainClassInjectorSpec? Something like:
static ClassNode getFurthestUnresolvedDomainParent(ClassNode classNode) {
ClassNode target = classNode
ClassNode parent = classNode.getSuperClass()
while (parent != null && !parent.isResolved() && parent.name != 'java.lang.Object') {
if (isDomainClass(parent)) {
target = parent
}
parent = parent.getSuperClass()
}
return target
}(with the uncached check from the other thread in place of isDomainClass). With both, the probe matches the current behaviour and the new specs stay green.
| * @param classNode the domain class being enhanced | ||
| * @return the domain class that should declare generated persistence properties | ||
| */ | ||
| static ClassNode getFurthestUnresolvedDomainParent(ClassNode classNode) { |
There was a problem hiding this comment.
isDomainClass(ClassNode) is @Memoized, and ClassNode equality is by name, so where the fields land now depends on what the compiling JVM has already seen for that name. Gradle keeps compiler-daemon statics across compilations, and test JVMs compile many same-named fixtures.
Probe in one JVM: compile abstract class ParentName { String name } + @Entity class X extends ParentName in one GroovyClassLoader, then @Entity class Kid extends ParentName; @Entity abstract class ParentName { } in another. ParentName and Kid both get id/version: the cached false wins over the annotation on the new node (8.0.x head: ParentName only).
DirtyCheckingTransformer has the same exposure, but a stale answer there costs a missed weave; here it costs a second identity. Could placement use an uncached check (entity annotations, GormEntity interface, module path), for example an uncached variant in AstUtils that this helper and DirtyCheckingTransformer share?
| loader.close() | ||
|
|
||
| where: | ||
| declarations << [ |
There was a problem hiding this comment.
Please add rows for a plain class between two domain classes, in both declaration orders (the AstUtils thread has the shape). That is the case that currently regresses, and the same rows belong in DefaultDomainClassInjectorSpec.
| When domain classes extend other domain classes, generated persistence properties remain on the domain superclass. | ||
| Properties explicitly declared on a superclass are still inherited normally. | ||
|
|
||
| After upgrading an application affected by persistence properties being added to a shared non-domain superclass, run a clean build to regenerate the affected classes. |
There was a problem hiding this comment.
This belongs in the upgrade notes rather than here, and it is more than a clean build. The change is visible to any plain base class compiled alongside its domain subclass:
@CompileStatic abstract class Base { boolean isNew() { id == null } }with@Entity class Foo extends Basecompiles on 8.0.x and fails with this change:[Static type checking] - The variable [id] is undeclared.(a dynamic base keeps working at runtime).@ToString,@EqualsAndHashCodeor a hand-writtenequalson the base no longer seeid/version.- A stale
.classof the base keeps the fields until a clean build.
Please add a numbered section to upgrading90x.adoc covering these and keep this chapter to the behaviour.
| In this example, both `name` and `age` will allow null values during validation. | ||
|
|
||
|
|
||
| ==== Sharing Properties With Domain Classes |
There was a problem hiding this comment.
Could this section lead with a trait? That is the idiomatic way to share properties between a command object and a domain class, and it works without this change: trait PersonDetails { String name } implemented by both Person and PersonCommand maps name on the entity, and the command validates name alone with no id/version. Then describe the shared superclass as what Grails does when one is used.
The GORM inheritance chapter (grails-data-hibernate7/docs/src/docs/asciidoc/domainClasses/inheritanceInGORM.adoc, and the hibernate5 copy) only covers abstract domain roots today. A sentence there on where generated id/version land when the parent is a plain class, and that @DirtyCheck on a plain base opts it into dirty checking but not into field placement, would close the loop.
1. Retargeted PR branch from `8.0.x` to `9.0.x` and verified the new base. No source files or branch history were changed. 2. Added the upgrade note to upgrading90x.adoc, covering: - Changed placement of generated `id` and `version` properties. - Removal of command-object workarounds. - Compatibility guidance and clean rebuild instructions. - Preserved the existing `9.0.x` notes and added the page to the guide’s table of contents. 3. Updated ancestor selection to walk past plain intermediate classes and place generated `id`/`version` on the furthest unresolved domain ancestor. - Used an uncached domain check to prevent same-named classes from earlier compilations affecting placement. - Added both declaration orders to all three requested specs, plus same-name compilation regressions. - Clarified the inheritance behavior in the documentation. 4. `DirtyCheckingTransformer` now uses the same uncached domain-class check as identity placement. - Added regressions for same-named parents compiled in separate class loaders, in both orders. - Strengthened entity tests to verify identity placement and actual dirty-property tracking. - Both `GormEntityTransformSpec` and `DefaultDomainClassInjectorSpec` cover a plain class between two domain classes. - Both declaration orders are tested. - Assertions verify that generated `id`/`version` remain on the domain root without duplicates on the child or plain intermediate. 5. Expanded section 5 of upgrading90x.adoc to cover static-compilation failures, effects on string representation and equality, dynamic property access, and stale `.class` files requiring a clean rebuild. - Removed the upgrade instruction from the command-object chapter, keeping it focused on behavior. 6. Made the command-object sharing section lead with a trait-based example, followed by shared-superclass behavior. - Updated both Hibernate 5 and Hibernate 7 inheritance guides to explain generated `id`/`version` placement and clarify that `@DirtyCheck` does not make a plain base a domain class. - Added tests for trait-based command validation and dirty-checked base-class field placement.
|
@ruthst00 I've tried to merge up from 7.x all the way to 10.x to make these PRs easier to run with the latest caching fixes. Can you please retarget this and let me know once feedback has been implemented? I'm wanting to do an 8.1 soon with the number of fixes / features we have and I feel this should make that cut. |
- grails-doc/src/en/guide/toc.yml - grails-doc/src/en/guide/upgrading/upgrading90x.adoc Resolved both conflicts locally against the latest `9.0.x`: - Preserved upstream’s full table of contents. - Preserved the reviewed GORM upgrade notes. - Staged the resolutions; no unresolved conflicts remain.
|
@jdaugherty PR branch has been retargeted to 9.0.x and all feedback implemented. As always, thanks very much for the constructive feedback :o) |
|
Once the test runs finish, I'll merge this. |
🚨 TestLens detected 3 failed tests 🚨Test SummaryTip Inspect all details with the TestLens CLI: CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-core-cli-legacy:test
CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-core:testCli
CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-shell-cli:test
🏷️ Commit: b1f8ad9 Test FailuresFileSystemInteractionImplSpec > files that do not exist yet are resolved like existing ones (:grails-core-cli-legacy:test in CI / Build Grails-Core (Windows JDK 25 shard 2))
FileSystemInteractionImplSpec > files that do not exist yet are resolved like existing ones (:grails-core:testCli in CI / Build Grails-Core (Windows JDK 25 shard 2))
FileSystemInteractionImplSpec > files that do not exist yet are resolved like existing ones (:grails-shell-cli:test in CI / Build Grails-Core (Windows JDK 25 shard 2))
Rerun ControlsNote Checks are currently running using the configuration below. Select tests to mute in this pull request: 🔲 FileSystemInteractionImplSpec > files that do not exist yet are resolved like existing ones Reuse successful test results: 🔲 ♻️ Only rerun the tests that failed or were muted before Click the checkbox to trigger a rerun: ☑️ Rerun jobs Learn more about TestLens at testlens.app/docs. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 9.0.x #16653 +/- ##
==================================================
+ Coverage 61.4500% 63.7827% +2.3327%
- Complexity 27010 28291 +1281
==================================================
Files 2260 2260
Lines 112192 112217 +25
Branches 20351 20370 +19
==================================================
+ Hits 68942 71575 +2633
+ Misses 34190 31493 -2697
- Partials 9060 9149 +89
🚀 New features to boost your workflow:
|
This branch has not been deployed
2 participants
Description
Root Cause
When using a common abstract base class for both entities and command objects (injecting
Validateable), the generated GORM code (idandversion) are injected into the abstract base class, and not into the entity object as expected.This results in a validation error on a command object extending an abstract-base class that is also used to extend a domain class, on the fields
idandversion, even though they should not be part of the command object.The Fix
idandversionfields only within the domain-class hierarchy.Verification
grails-coregrails-datamapping-coregrails-datastore-coregrails-validationFixes #15364
Generated with GPT 6 Astra via Cline API Provider in IntelliJ IDEA 2025.3.3
Contributor Checklist
Please review the following checklist before submitting your pull request. Pull requests that do not meet these requirements may be closed without review.
Issue and Scope
7.0.x): Bug fixes only. No new features or API changes.7.1.x): New features are welcome, but breaking existing APIs must be avoided.8.0.x): Reserved for major changes. Breaking API changes are permitted.Code Quality
./gradlew build --rerun-tasks../gradlew codeStyleand resolved any violations. See Code Style for details.Licensing and Attribution
Documentation