sbglasius commented on PR #16282:
URL: https://github.com/apache/grails-core/pull/16282#issuecomment-5614977852
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`:
```groovy
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:
```groovy
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.
For what it is worth, I also checked the `@Delegate` interaction on the
subclasses, since `iterator()` is re-declared in
`DirtyCheckingList`/`Set`/`SortedSet` but `retainAll`/`removeIf` are not:
`@Delegate` does not shadow methods inherited from the superclass, so the new
`retainAll`/`removeIf` tracking does fire on all three (verified against the
compiled classes from this branch).
--
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]
