matrei commented on PR #16282:
URL: https://github.com/apache/grails-core/pull/16282#issuecomment-5623305502
## 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: []`:
```groovy
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:
```text
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:
```groovy
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.
--
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]
