codeconsole commented on code in PR #16282:
URL: https://github.com/apache/grails-core/pull/16282#discussion_r3973849533
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy:
##########
@@ -130,4 +130,53 @@ class DirtyCheckingSupport {
}
return new DirtyCheckingCollection(coll, parent, property)
}
+
+ /**
+ * Re-establishes change tracking when a tracked collection or map value
is replaced
+ * through a generated dirty-checking setter.
+ *
+ * <p>Interception-based stores (MongoDB et al.) install the
DirtyChecking* wrappers when an
+ * entity is decoded and rely on them exclusively — there is no flush-time
snapshot
+ * comparison. The generated setter used to store whatever raw value it
was handed, so
+ * reassigning a collection property replaced the tracked wrapper with a
plain, untracked
+ * collection and every later in-place mutation became invisible to change
tracking. The
+ * common defensive re-init {@code if (!entity.items) entity.items = []}
triggered this on
+ * every load (an empty tracked collection is falsy in Groovy), and
because the new empty
+ * collection equals the old one the assignment itself was never flagged
either.
+ *
+ * <p>Tracking is only re-established, never introduced: when the value
being replaced is
+ * not a tracked wrapper — a transient instance, or a store like Hibernate
that performs its
+ * own snapshot-based dirty checking and never installs these wrappers —
the new value is
+ * returned untouched, keeping this a no-op for those cases.
+ *
+ * @param parent The dirty-checkable owner
+ * @param property The property being assigned
+ * @param oldValue The value being replaced
+ * @param newValue The value being assigned
+ * @return The value to store: {@code newValue}, wrapped if it replaces a
tracked value
+ */
+ static Object rewrap(DirtyCheckable parent, String property, Object
oldValue, Object newValue) {
Review Comment:
Took option (b), with one adjustment your repro made necessary: `Neo4jList
extends DirtyCheckingList`, so an `instanceof` check against the generic
classes would still have caught the Neo4j wrappers. `rewrap` now gates on the
**exact class** of the replaced value being one of the five generic wrappers
(`isGenericWrapper`). A store-specific wrapper or `PersistentCollection` being
replaced stores the raw value, and the store's persister re-wraps it on save
exactly as at merge-base.
Added `HasManyReassignDirtyCheckingSpec` in grails-data-neo4j (embedded
harness) running your exact reassign → save → remove → save → reload sequence —
verified it fails with the previous `instanceof` gate and passes with the
exact-class one. There's also a unit spec pinning that a `DirtyCheckingList`
subclass as the old value is left alone. In `8581d5604f`.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingMap.groovy:
##########
@@ -34,12 +34,23 @@ class DirtyCheckingMap implements Map,
DirtyCheckableCollection {
final DirtyCheckable parent
final String property
final int originalSize
+ final boolean assigned
Review Comment:
Covered rather than documented away, in `8581d5604f`: overrides for the
`@Delegate`-generated default methods (`putIfAbsent`, `merge`,
`compute`/`computeIfAbsent`/`computeIfPresent`, `replace`,
`replace(k,old,new)`, `replaceAll`, `remove(k,v)`), and
`entrySet()`/`keySet()`/`values()` now return tracking wrappers — which is what
catches Groovy's `Map.removeAll(Closure)`/`retainAll(Closure)` (they iterate
`entrySet()`), `keySet().remove(k)` and `values().removeIf { }`.
`DirtyCheckingList.subList()` returns a tracking view as well, with a spec per
path.
The one Map path left untracked is `Map.Entry.setValue` during iteration;
that's now called out explicitly in the docs update.
##########
grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckingTransformer.groovy:
##########
@@ -94,6 +97,13 @@ class DirtyCheckingTransformer implements
CompilationUnitAware {
public static final ClassNode DIRTY_CHECKED_PROPERTY_CLASS_NODE =
ClassHelper.make(DirtyCheckedProperty)
public static final ClassNode DIRTY_CHECK_CLASS_NODE =
ClassHelper.make(DirtyCheck)
public static final AnnotationNode DIRTY_CHECKED_PROPERTY_ANNOTATION_NODE
= new AnnotationNode(DIRTY_CHECKED_PROPERTY_CLASS_NODE)
+ private static final ClassNode DIRTY_CHECKING_SUPPORT_CLASS_NODE =
ClassHelper.make(DirtyCheckingSupport)
+ // Interface-typed collection properties whose generated setter
re-establishes change
+ // tracking via DirtyCheckingSupport.rewrap. Restricted to the exact
interfaces the
+ // DirtyChecking* wrappers implement so the cast in the generated setter
is always valid.
+ private static final Set<String> REWRAPPABLE_TYPE_NAMES = [
Review Comment:
Done in `8581d5604f`: `SortedSet.name` added here, and both `wrap()` and
`rewrap()` now produce `DirtyCheckingSortedSet` ahead of the `Set` check — so a
`TreeSet` keeps its `SortedSet` API behind the wrapper on `Set`-declared
properties too (the `wrap()` blind spot you pointed out). Specs cover the
SortedSet-typed reassignment and `wrap(TreeSet)` returning the SortedSet
wrapper.
##########
grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckingTransformer.groovy:
##########
@@ -364,7 +374,21 @@ class DirtyCheckingTransformer implements
CompilationUnitAware {
final BlockStatement setterBody = new BlockStatement()
MethodCallExpression markDirtyMethodCall =
createMarkDirtyMethodCall(markDirtyMethodNode, propertyName, setterParameter)
setterBody.addStatement(stmt(markDirtyMethodCall))
- setterBody.addStatement(assignS(propX(varX('this'), fieldName),
varX(setterParameter)))
+ // Collection/Map-typed properties assign through
DirtyCheckingSupport.rewrap so a
+ // value that replaces a tracked wrapper (installed by an
interception-based store's
+ // decoder) stays tracked. Without this, `entity.items = []` over
a tracked list
+ // stored a plain untracked collection and every later in-place
mutation was
+ // invisible to change tracking. rewrap is a no-op when the old
value was untracked,
+ // so stores with their own dirty checking (Hibernate) are
unaffected.
+ Expression assignedValue
+ if (REWRAPPABLE_TYPE_NAMES.contains(returnType.name)) {
Review Comment:
Left for a follow-up as you suggested, and now stated explicitly in the docs
update: the rewritten caveat in `objectMapping/dirtyChecking.adoc` lists
hand-written setters first among the paths that still require an explicit
`markDirty(propertyName)` — "a hand-written setter … stores the value it is
given without re-wrapping, so a collection assigned through it loses tracking
until the next load."
--
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]
