Keep collection properties dirty-checked after reassignment; track iterator-based removals

[codeconsole]

Mongo Collection Property Dirty Tracking

On interception-based stores (MongoDB), two everyday mutation patterns silently escape dirty checking — save() reports success and persists nothing:

// 1. The defensive re-init — true for an EMPTY tracked list, because empty collections are falsy
if (!schedule.shares) {
    schedule.shares = []              // replaces the tracked wrapper with a plain ArrayList
}                                     // and [] == [] means the assignment isn't even flagged
schedule.shares.add(newShare)         // invisible: plain list, nothing marks the entity dirty
schedule.save(flush: true)            // writes nothing

// 2. Groovy's closure-based removal — removes via iterator().remove()
schedule.shares.removeAll { it.userId == userId }   // DirtyCheckingCollection doesn't override iterator()
schedule.save(flush: true)                          // writes nothing

Hibernate is unaffected — its flush-time snapshot comparison catches everything. Mongo relies exclusively on the DirtyChecking* wrappers, so anything that escapes them is lost. Hit in production: a schedule-sharing feature showed "shared" while the document kept shares: [].

The fix (interception only — no snapshots, no flush-time diffing)

1. Wrappers track every mutation path. iterator()/listIterator() now return dirty-marking iterators (covers removeAll(Closure), retainAll(Closure), removeIf), plus the missing direct overrides: retainAll(Collection), List.sort, List.replaceAll. Same approach as Hibernate's PersistentCollection.

2. Generated setters keep tracking across reassignment. Collection/List/Set/Map-typed properties assign through DirtyCheckingSupport.rewrap:

// generated setter, before:
void setShares(List shares) { markDirty("shares", shares); this.shares = shares }
// after:
void setShares(List shares) { markDirty("shares", shares); this.shares = (List) DirtyCheckingSupport.rewrap(this, "shares", this.shares, shares) }

rewrap wraps the new value only when the value being replaced was itself a tracked wrapper — otherwise it returns the raw value after one instanceof. Never-tracked properties (transient instances, Hibernate entities) behave exactly as before, and non-collection properties compile to identical bytecode.

3. Replacement wrappers are flagged isAssigned() (default method on DirtyCheckableCollection, so binary-compatible). PersistentEntityCodec then takes the full-rewrite path instead of per-element diffing — a replacement's layout need not match the stored array. Without the flag, a same-size replacement holding clean elements emitted no update at all.

Tests

Each escape is reproduced by a spec that fails without the fix:

• DirtyCheckingCollectionSpec — 8 wrapper mutation paths that bypassed tracking
• DirtyCheckCollectionReassignmentSpec — reassignment loses tracking (List/Set/Map); never-tracked values stay untouched
• EmbeddedCollectionDirtyTrackingSpec — end-to-end against MongoDB, replicating the production shape (an auto-timestamped entity: the lastUpdated write during flush resets the explicit-save dirty marker, so persistence depends entirely on the wrappers)

[codecov]

Codecov Report

❌ Patch coverage is 96.27329% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.1221%. Comparing base (0980623) to head (f11e855).

Files with missing lines	Patch %	Lines
...mapping/dirty/checking/DirtyCheckingSupport.groovy	93.0233%	0 Missing and 3 partials ⚠️
...ping/dirty/checking/DirtyCheckingCollection.groovy	89.4737%	2 Missing ⚠️
...re/mapping/dirty/checking/DirtyCheckingList.groovy	97.5000%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16282        +/-   ##
==================================================
+ Coverage     55.0529%   55.1221%   +0.0693%     
- Complexity      20773      20864        +91     
==================================================
  Files            2110       2111         +1     
  Lines          101368     101520       +152     
  Branches        18005      18025        +20     
==================================================
+ Hits            55806      55960       +154     
+ Misses          37517      37514         -3     
- Partials         8045       8046         +1     

Files with missing lines	Coverage Δ
...g/mongo/engine/codecs/PersistentEntityCodec.groovy	74.4807% <100.0000%> (+0.6712%)	⬆️
...ails/compiler/gorm/DirtyCheckingTransformer.groovy	78.9720% <100.0000%> (+0.8166%)	⬆️
...pping/dirty/checking/DirtyCheckableCollection.java	100.0000% <100.0000%> (ø)
...ore/mapping/dirty/checking/DirtyCheckingMap.groovy	75.0000% <100.0000%> (+35.0000%)	⬆️
...ore/mapping/dirty/checking/DirtyCheckingSet.groovy	100.0000% <100.0000%> (ø)
...pping/dirty/checking/DirtyCheckingSortedSet.groovy	100.0000% <100.0000%> (+100.0000%)	⬆️
...re/mapping/dirty/checking/DirtyCheckingList.groovy	84.3137% <97.5000%> (+42.6471%)	⬆️
...ping/dirty/checking/DirtyCheckingCollection.groovy	76.9231% <89.4737%> (+17.8322%)	⬆️
...mapping/dirty/checking/DirtyCheckingSupport.groovy	82.7160% <93.0233%> (+11.6634%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jdaugherty]

Ran the two unit specs locally (both green) and read through the wrapper, transformer and codec changes. The approach is sound and the Mongo fix is real. Three things before this goes in:

1. Neo4j regression (inline on rewrap): reassigning a hasMany now permanently replaces the Neo4j-aware collection with a generic wrapper, so later in-place removals stop deleting relationships. Reproduced on this branch against the embedded harness; the same spec passes on the merge-base.
2. Map wrapper still leaks (inline on DirtyCheckingMap): the @Delegate-generated default methods and the three views bypass tracking.
3. Docs: grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc documents the exact caveat this PR removes ("if you override the collection with a non-dirty checking aware type it can disable dirty checking and prevent the property from being updated"). That paragraph needs to describe the new behaviour, plus whatever gaps remain after the above (Map views, subList, hand-written setters).

[jdaugherty]

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]:

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.

[codeconsole]

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.

[jdaugherty]

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.

[codeconsole]

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.

[codeconsole]

Following up on this thread, since it is where the "tracks every mutation path" claim was litigated: reviewing the compiled classes for the latest round turned up two more @Delegate-generated escapes of exactly this kind, now closed in a2ac5178cf.

• The Java 21 SequencedCollection mutators — addFirst, addLast, removeFirst, removeLast — were generated straight through to the target on DirtyCheckingList and DirtyCheckingSortedSet, with no markDirty.
• DirtyCheckingSortedSet handed out headSet, tailSet and subSet as raw live views, the same hole subList was overridden for.

Those views (plus reversed) now come back as tracking wrappers, and are constructed as assigned, because a view stored back onto a property is a wholesale replacement rather than something to diff element by element. The docs sentence is narrowed to List and SortedSet properties, since a plain Set/Map wrapper does not expose the sequenced API at all.

Map.Entry.setValue during iteration is still the one Map path left untracked, and remains listed as a gap.

[jdaugherty]

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.

[codeconsole]

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.

[jdaugherty]

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.

[codeconsole]

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."

[codeconsole]

The Mongo CI failures (BasicCollectionsSpec Currency/BigDecimal maps, MapOfDomainsSpec) were caused by the new Map views: Groovy's Map == Map extension compares self.keySet().equals(other.keySet()), and the tracking view fell back to identity equality. Fixed in 3ba7fb58d2 by delegating equals/hashCode on the wrappers to their target — the contract AbstractPersistentCollection already implements — with a spec pinning content equality both ways plus the three views.

[sbglasius]

One edge in DirtyCheckingSupport.rewrap worth noting. It is not a regression from this PR, so feel free to defer it.

rewrap returns early for a value that is already a DirtyCheckableCollection:

if (newValue instanceof DirtyCheckableCollection) {
    return newValue
}

So assigning one entity's tracked collection onto another entity's property stores the source wrapper, whose parent/property still point at the source entity:

a.shares = b.shares    // a.shares is now b's DirtyCheckingList
a.shares.add(share)    // marks b dirty for 'shares' — a stays clean

The wrong-parent binding pre-dates this PR (the old generated setter stored that wrapper verbatim), so nothing regresses here. What is new is the assigned consequence: a value taking this early return keeps assigned = false, so PersistentEntityCodec.encodeEmbeddedCollectionUpdate takes the per-element update path for what is in fact a wholesale replacement — while the same assignment written as a.shares = b.shares.toList() correctly gets assigned = true and falls through to the full re-encode.

Re-wrapping in that branch when the existing wrapper's parent/property do not match the target (instead of returning it as-is) would cover both.

[codeconsole]

Thanks — taken rather than deferred, in f11e85549b.

rewrap now re-binds a borrowed wrapper: it re-wraps that wrapper's raw target against the assigning entity with assigned = true. Three constraints shaped it:

• Store-specific wrappers stay untouched. Your wording taken literally (re-wrap on parent/property mismatch) would convert a Neo4jList belonging to another entity into a generic wrapper — the regression @jdaugherty caught earlier, since Neo4jList extends DirtyCheckingList. The exact-class check now guards the incoming value as well as the replaced one.
• Unwrap, never nest. Wrapping the foreign wrapper itself would leave its parent.markDirty firing underneath, marking both entities on every mutation. It unwraps in a loop, because BasicCollectionTypeEncoder:78 builds a DirtyCheckingMap with no already-wrapped guard, so a wrapper can arrive nested inside another.
• Owner compared by identity, not equals — two rows equal by business key are not the same owner, and on an association equals can initialise a proxy.

Two things worth flagging back:

Your repro passes as-is today. The codec condition is a three-way AND, and the add grows the list, so hasChangedSize() is true and the full re-encode runs anyway. I wrote that spec first and it was green pre-fix, so I replaced it. The shape that does fail is the one where the two collections are equal in content: the assignment is then equality-suppressed, so A is never flagged, the update carries only lastUpdated, and the addition is silently lost while B absorbs it. That is the new Mongo spec, verified red before the change and green after.

It also fires when the property was untracked. new Entity(shares: other.shares) had the same defect and could not self-heal — the insert's write-back assigns the field directly and never reaches rewrap — so the re-binding no longer depends on what the property held before. Assigning a plain collection to an untracked property still introduces no tracking.

Not addressed here, deliberately: wrap() keeps the same owner-blind early return, so a foreign wrapper reaching it through another path is still handed back as-is. It is a public method that has returned the same instance since 2.0 and is on the encode path, so it felt like its own change rather than a rider on this one — happy to do it here if you would rather it were one PR.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: f11e855
▶️ Tests: 26149 executed
⚪️ Checks: 89/89 completed

---

Learn more about TestLens at testlens.app/docs.

addressed

[matrei]

AI Review Findings

The three findings from the first round and the borrowed-wrapper edge from the second are all resolved, and the fixes hold up: I ran DirtyCheckingCollectionSpec, DirtyCheckCollectionReassignmentSpec, EmbeddedCollectionDirtyTrackingSpec (Mongo) and HasManyReassignDirtyCheckingSpec (Neo4j) locally, all green, and checked the compiled wrappers with javap to confirm no @Delegate-generated mutator is left calling straight through on any of the five classes.

One finding below I would like either fixed or stated in the docs before this goes in, because it is the PR's own bug on a shape one mapping away from the one in the description. The rest are non-blocking.

Resolved Findings

• Neo4j regression: rewrap gates on the exact generic wrapper classes, so Neo4jList/Neo4jSet and the PersistentCollection types are left for the store to re-wrap. HasManyReassignDirtyCheckingSpec runs the reassign, save, remove, save, reload sequence and passes.
• Map wrapper: the @Delegate-generated default methods are overridden and entrySet()/keySet()/values() come back as tracking views. javap on DirtyCheckingMap shows only forEach still delegated, which is read-only. The same check on DirtyCheckingList/DirtyCheckingSortedSet shows the SequencedCollection mutators and range views covered as of a2ac5178cf.
• SortedSet is in REWRAPPABLE_TYPE_NAMES, and both wrap() and rewrap() produce DirtyCheckingSortedSet ahead of the Set check.
• Hand-written setters are called out in the docs as untracked.
• Borrowed wrappers are re-bound to the assigning entity by wrapping the raw target, with owner compared by identity and the store-specific types excluded on the incoming side too. The Mongo spec uses the equal-content shape, which is the one that actually fails without the fix.
• The Map == Map CI failures are fixed by content equals/hashCode on the wrappers, matching AbstractPersistentCollection.

Findings

Medium: A loaded hasMany on Mongo still loses the falsy empty re-init

References:

• grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy:192 (the isGenericWrapper(oldValue) gate)
• grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy:268
• grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy:551-565 (to-many decode)
• grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc:39

The description's first escape is reproduced with shares as an embedded collection. Declare the same property as a unidirectional hasMany instead and the exact same code still loses the add on this branch. Mongo decodes a stored members: [] into an empty PersistentList, whose class is not one of the five generic wrappers, so rewrap stores the raw []; markDirty suppresses the assignment because ArrayList.equals(emptyPersistentList) is true; the add then goes to a plain list nobody tracks.

Probe against the Mongo harness, List<ProbeMember> members; static hasMany = [members: ProbeMember], entity saved with members: []:

board = ProbeBoard.get(id); board.trackChanges()   // members: PersistentList (empty)
if (!board.members) board.members = []             // members: java.util.ArrayList
board.members.add(new ProbeMember(userId: 'u1'))
board.save(flush: true)

	class held after re-init	document after save	reload
this branch	java.util.ArrayList	members: [], version: 1	0 members

This is not a regression: the generated setter stored the raw list before this PR as well. But it is the bug in the title, on the mapping most applications would reach for before embedded, and the instanceof DirtyCheckableCollection gate from the first push did cover it; the exact-class gate that fixed Neo4j dropped it. The docs sentence at line 39 reads as if reassignment is covered generally, and the one-to-many caveat at line 53 only appears inside the borrowed-collection paragraph.

Two ways out:

1. Accept the three datastore-core classes PersistentList, PersistentSet and PersistentSortedSet as exact classes in the old-value gate alongside the generic wrappers. Neo4jPersistentList/Neo4jPersistentSet/Neo4jPersistentSortedSet are subclasses, so the Neo4j path is unchanged. The resulting state, a hasMany property holding a generic DirtyCheckingList, is already what MongoCodecEntityPersister writes back after any save, and the probe's second case (reassign after an in-session save, then add) round-trips through exactly that state today. It would need a Mongo spec on the hasMany shape next to the embedded one.
2. Leave the behaviour and add a bullet to the gap list at line 56: a one-to-many or many-to-many loaded from the database is a PersistentCollection, is not re-wrapped on reassignment, and needs markDirty after an in-place mutation that follows a reassignment.

I would take the first, since it is the production shape with a different mapping keyword.

Low: Wrapper equals is not reflexive when the target has identity equality

Reference:

• grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollection.groovy:82

target.equals(other) is fine when the target defines content equality (ArrayList, HashSet, TreeSet, every Map), but the new values() view and any DirtyCheckingCollection produced by wrap() for a non-List, non-Set target wrap an AbstractCollection, which inherits Object.equals. The wrapper then fails to equal itself. Checked against the compiled classes:

map.values().equals(map.values())      = false
[map.values()].contains(map.values())  = false
new DirtyCheckingCollection(new ArrayDeque(['x']), o, 'p').equals(self) = false

Groovy == still returns true because it checks identity first, so nothing in the specs sees it, but List.contains, indexOf and HashSet membership go through equals. Short-circuiting on identity keeps the delegation and restores the contract:

boolean equals(Object other) {
    other.is(this) || target.equals(other)
}

Low: BasicCollectionTypeEncoder nests a new DirtyCheckingMap on every save (follow-up)

References:

• grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/BasicCollectionTypeEncoder.groovy:76-78
• grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy:255

The write-back for a map property wraps whatever the property currently holds, with no already-wrapped guard, so an entity saved N times in a session holds a map N wrappers deep. The unwrap loop in genericWrapperTarget works around this for rewrap, and the comment there names the cause. Pre-existing, so fine to defer, but worth a follow-up now that each level also hands out a fresh view wrapper per entrySet() call, since the cost of the nesting is no longer just an extra markDirty. The collection branch a few lines up already goes through DirtyCheckingSupport.wrap, which has the guard; the map branch only needs the same instanceof check.

Nit: The explicit iterator() overrides in the three subclasses are redundant

References:

• grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingList.groovy:83
• grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSet.groovy:45
• grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSortedSet.groovy:45

@Delegate skips methods the owner already has, including inherited ones. javap on DirtyCheckingSet shows no removeIf, retainAll or removeAll of its own, so the ones inherited from DirtyCheckingCollection are what run, and the same holds for iterator(). The three super.iterator() overrides and their "route through" comments can go. Harmless if kept.

Nit: The docs list addFirst/addLast for SortedSet properties

Reference:

• grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc:43-44

SortedSet.addFirst/addLast throw UnsupportedOperationException by contract, which is why DirtyCheckingSortedSet does not override them (its own comment at line 50 says so). The sentence should attach those two to List properties only; the removals and views apply to both.