jdaugherty commented on code in PR #16653:
URL: https://github.com/apache/grails-core/pull/16653#discussion_r4294836385
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy:
##########
@@ -633,6 +633,23 @@ class AstUtils {
return null
}
+ /**
+ * Finds the highest uncompiled domain class in the contiguous domain
hierarchy.
+ * Plain superclasses may be shared with non-persistent classes and must
not receive
+ * generated persistence properties.
+ *
+ * @param classNode the domain class being enhanced
+ * @return the domain class that should declare generated persistence
properties
+ */
+ static ClassNode getFurthestUnresolvedDomainParent(ClassNode classNode) {
+ ClassNode parent = classNode.getSuperClass()
+ while (parent != null && !parent.isResolved() &&
isDomainClass(parent)) {
Review 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:
```groovy
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.
##########
grails-datamapping-core/src/test/groovy/org/grails/compiler/gorm/GormEntityTransformSpec.groovy:
##########
@@ -36,6 +36,67 @@ import
org.grails.datastore.mapping.dirty.checking.DirtyCheckable
*/
class GormEntityTransformSpec extends Specification{
+ void 'generated fields do not leak into a shared non-domain superclass'() {
+ given:
+ def loader = new GroovyClassLoader()
+ loader.parseClass("""
+ import grails.gorm.annotation.Entity
+
+ $declarations
+
+ abstract class SharedDetails {
+ String name
+ }
+
+ class PlainCommand extends SharedDetails { }
+ """)
+
+ when:
+ def root = loader.loadClass('RootEntity')
+ def child = loader.loadClass('ChildEntity')
+ def command =
loader.loadClass('PlainCommand').getDeclaredConstructor().newInstance()
+
+ then:
+ root.getDeclaredField('id').type == Long
+ root.getDeclaredField('version').type == Long
+ child.getMethod('getId').declaringClass == root
+ child.getMethod('getVersion').declaringClass == root
+ !child.declaredFields*.name.contains('id')
+ !child.declaredFields*.name.contains('version')
+ command.hasProperty('id') == null
+ command.hasProperty('version') == null
+
+ cleanup:
+ loader.close()
+
+ where:
+ declarations << [
Review 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`.
##########
grails-doc/src/en/guide/theWebLayer/controllers/commandObjects.adoc:
##########
@@ -59,6 +59,39 @@ class AuthorSearchCommand implements
grails.validation.Validateable {
In this example, both `name` and `age` will allow null values during
validation.
+==== Sharing Properties With Domain Classes
Review 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.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy:
##########
@@ -633,6 +633,23 @@ class AstUtils {
return null
}
+ /**
+ * Finds the highest uncompiled domain class in the contiguous domain
hierarchy.
+ * Plain superclasses may be shared with non-persistent classes and must
not receive
+ * generated persistence properties.
+ *
+ * @param classNode the domain class being enhanced
+ * @return the domain class that should declare generated persistence
properties
+ */
+ static ClassNode getFurthestUnresolvedDomainParent(ClassNode classNode) {
Review 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?
##########
grails-doc/src/en/guide/theWebLayer/controllers/commandObjects.adoc:
##########
@@ -59,6 +59,39 @@ class AuthorSearchCommand implements
grails.validation.Validateable {
In this example, both `name` and `age` will allow null values during
validation.
+==== Sharing Properties With Domain Classes
+
+A command object and a domain class can extend the same non-domain superclass
to share properties.
+Keep that superclass in `src/main/groovy`, outside `grails-app/domain`, and do
not annotate it as an entity.
+
+[source,groovy]
+----
+abstract class PersonDetails {
+ String name
+}
+
[email protected]
+class Person extends PersonDetails {
+ static constraints = {
+ name blank: false
+ }
+}
+
+class PersonCommand extends PersonDetails implements
grails.validation.Validateable {
+ static constraints = {
+ name blank: false
+ }
+}
+----
+
+Grails adds the generated `id` and `version` properties to the domain class,
not to the shared non-domain superclass.
+Consequently, `PersonCommand` validates `name` without acquiring persistence
properties from `Person`.
+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.
Review 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 Base` compiles 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`, `@EqualsAndHashCode` or a hand-written `equals` on the base
no longer see `id`/`version`.
- A stale `.class` of 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
