jdaugherty commented on code in PR #16282:
URL: https://github.com/apache/grails-core/pull/16282#discussion_r3964422752
##########
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:
`PersistentCollection` and the Neo4j store's own wrappers
(`Neo4jList`/`Neo4jSet`/`Neo4jSortedSet`,
`Neo4jPersistentList`/`Neo4jPersistentSet`) all implement
`DirtyCheckableCollection`, so this also fires when one of *those* is the value
being replaced, and it installs a plain `DirtyChecking*` wrapper in their
place. `Neo4jEntityPersister.createDirtyCheckableAwareCollection` then takes
its "already dirty-checkable" branch and never re-wraps into a Neo4j
collection. Before this change the raw replacement went through the other
branch and came back as a `Neo4jList`/`Neo4jSet` on the first save.
Net effect: once a `hasMany` property has been reassigned, in-place removals
on that instance stop deleting relationships for the rest of its life (inserts
still happen because `hasChanged()` is true, and they are MERGEs, so adds look
fine). Orphan removal in `GraphAdapter.adaptGraphUponRemove` is lost the same
way.
Repro against the embedded harness, unidirectional `List songs` / `hasMany =
[songs: Song]`:
```groovy
p = Playlist.get(id); p.trackChanges() // songs: Neo4jPersistentList
p.songs = [a, b, c]; p.save(flush: true) // songs: DirtyCheckingList, and
still is after save
p.songs.remove(a); p.save(flush: true) // hasChanged('songs') == true,
but no RelationshipPendingDelete
```
| | relationships after reassign+save | after remove+save | reload |
|---|---|---|---|
| this branch | 3 | **3** | `[a, b, c]` |
| merge-base | 3 | 2 | `[b, c]` |
Two ways out, either works for me: (a) have the Neo4j persister treat an
`isAssigned()` wrapper like a raw collection, which is what the flag means, by
wrapping it (or its `target`) in `Neo4jList`/`Neo4jSet` and registering the
inserts; or (b) only rewrap when the old value is one of the generic
`DirtyChecking*` classes and leave store-specific collections to the store.
Either way this needs a Neo4j spec covering reassign, save, remove, save.
##########
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:
`SortedSet` is a supported GORM collection type and `DirtyCheckingSortedSet`
exists for it, but it isn't in this set, so a `SortedSet`-typed property
reassigned over a tracked value loses tracking exactly the way `List`/`Set`
did. Adding `SortedSet.name` here plus a `newValue instanceof SortedSet` branch
in `rewrap` ahead of the `Set` check keeps the generated cast valid and stops a
`TreeSet` assigned to a `Set` property from losing its `SortedSet` API behind
the wrapper. `wrap()` has the same blind spot, so worth fixing both together.
##########
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:
The Map wrapper still has the same class of escape this PR closes for
collections. Checked the compiled class: `@Delegate` generates `putIfAbsent`,
`merge`, `compute`/`computeIfAbsent`/`computeIfPresent`, `replaceAll`,
`replace` and `remove(key, value)` as straight calls on `target` with no
`markDirty`, and `entrySet()`/`keySet()`/`values()` hand out the raw views. So
Groovy's `Map.removeAll(Closure)` / `retainAll(Closure)` (they iterate
`entrySet()`), `keySet().remove(k)` and `values().removeIf { }` are all
invisible to tracking, exactly like `List.removeAll(Closure)` was.
`DirtyCheckingList.subList()` is a raw view too.
Since the description says the wrappers now track every mutation path,
either cover these (override the default methods, return tracking views for the
three collection views) or call them out as known gaps in the docs update.
##########
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:
Only generated setters get the rewrap. A domain class with its own `void
setShares(List shares)` goes through `weaveIntoExistingSetter`, which only
prepends `markDirty`, so the original bug is still there for hand-written
setters. Fine to leave for a follow-up, but it should be stated in the docs
update so nobody assumes it's covered.
--
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]
