matrei commented on PR #16282:
URL: https://github.com/apache/grails-core/pull/16282#issuecomment-5653905827
## AI Review Findings (round 2)
Head `2dade74084`, base `8.0.x` (PR base `0980623481`; `origin/8.0.x` has
moved on by 33 commits since, none touching the PR's files, and a trial merge
is clean).
All five items from the previous round are taken, one commit each, and each
fix does what the author's comment says. What I ran on the head commit:
| Suite | Result |
|---|---|
| `grails-datastore-core` `dirty.checking.*` (29 tests) | green |
| `grails-datamapping-core` `DirtyCheckCollectionReassignmentSpec` (17) |
green |
| Mongo, Docker: `HasManyReassignDirtyTrackingSpec`,
`EmbeddedCollectionDirtyTrackingSpec`, `OneToManySpec`, `ManyToManySpec`,
`EmbeddedCollectionSpec` (8 classes, 18 tests) | green |
| Neo4j, Docker: `HasManyReassignDirtyCheckingSpec` | green |
| Simple-map TCK in `grails-datamapping-core-test`: `OneToManySpec`,
`CircularOneToManySpec`, `CircularManyToManySpec`, `ManyToManySpec`,
`InheritanceWithOneToManySpec`, `CircularCascadeSpec` (13 tests) | green |
| `checkstyleMain` + `codenarcMain` on `grails-datastore-core` | clean |
| `checkstyleMain` + `codenarcMain` on `grails-data-mongodb-bson` | **1
CodeNarc violation** |
One blocking item, and it is the one CI already flags: the "Code Style /
Core Projects" job fails on this head. The rest is non-blocking.
## Resolved Findings
- `hasMany` re-init on Mongo (`0ae70cef89`): `wasTracked` accepts
`PersistentList`/`PersistentSet`/`PersistentSortedSet` by exact class, so the
Neo4j and Rx subclasses still fall through to their stores.
`HasManyReassignDirtyTrackingSpec` reproduces the shape from the last round
with `members: []` actually stored, and passes. The gate reads only
`getClass()`, so an uninitialised lazy association is not forced.
- Reflexive `equals` (`f799282454`): identity short-circuit in both
wrappers, pinned by `[wrapper].contains(wrapper)` on a `values()` view and on a
`DirtyCheckingCollection` over an `ArrayDeque`.
- Map nesting on save (`a7f7edb0d6`): `DirtyCheckingSupport.wrap(Map)`
carries the same already-wrapped guard as the collection form, and the
encoder's map branch goes through it. The other two `wrap` call sites pass a
`List` or a `Collection`, one of them under `@CompileDynamic`, so the new
overload is unambiguous at every caller.
- Redundant `iterator()` overrides (`2dade74084`): removed on all three
subclasses; the iterator-removal and `removeAll(Closure)` cases in
`DirtyCheckingCollectionSpec` still pass, so the inherited override is the one
doing the work.
- Docs: `addFirst`/`addLast` are attached to `List` properties only.
## Findings
### Blocking: Unused import fails the code-style job
Reference:
-
`grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/BasicCollectionTypeEncoder.groovy:32`
`a7f7edb0d6` replaced the only use of `DirtyCheckingMap` in this file with
`DirtyCheckingSupport.wrap`, but left the import behind. CodeNarc's
`UnusedImport` rule rejects it:
```text
BasicCollectionTypeEncoder.groovy UnusedImport line 32
The [org.grails.datastore.mapping.dirty.checking.DirtyCheckingMap] import is
never referenced
```
That is the "Code Style / Core Projects" failure TestLens reports on
`2dade74084`. Reproduces locally with:
```bash
./gradlew :grails-data-mongodb-bson:codenarcMain
```
Dropping the import line is the whole fix.
### Low: The exact-class rule for `PersistentList` has no unit-level pin
References:
-
`grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy:292-305`
-
`grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/dirty/checking/DirtyCheckCollectionReassignmentSpec.groovy:188`
The new gate is covered only by the Mongo container spec, and only for the
`List` shape. The reassignment spec already has the mirror case for the wrapper
side, a `StoreSpecificList` subclass of `DirtyCheckingList` that must not be
rewrapped. The same pair for the association side would pin all three exact
classes and the subclass exclusion without a container, and would catch a
future refactor that relaxes the check to `instanceof PersistentCollection`
(which is exactly the change that regressed Neo4j in the first push):
```groovy
def 'a plain collection replacing a datastore PersistentList is rewrapped'()
{
given:
def entity = entityClass.newInstance()
entity.shares = new PersistentList([], String, Mock(Session))
entity.trackChanges()
when:
entity.shares = []
entity.shares.add('a')
then:
entity.shares instanceof DirtyCheckingList
((DirtyCheckableCollection) entity.shares).isAssigned()
entity.hasChanged('shares')
}
def 'a store-specific PersistentList subclass is left to its store'() {
given:
def entity = entityClass.newInstance()
entity.shares = new StoreSpecificPersistentList([], String,
Mock(Session)) // extends PersistentList
entity.trackChanges()
when:
entity.shares = ['a']
then:
!(entity.shares instanceof DirtyCheckableCollection)
}
```
The setter's `markDirty` equality check does initialise the
`PersistentList`, but `loadInverseChildKeys` returns before consulting the
session when the key set is empty, so a Spock mock is enough for these shapes.
Same two cases for `PersistentSet` and `PersistentSortedSet` if you want the
full matrix; the `where:` block form keeps it short.
### Nit: The borrowed-collection paragraph now reads as contradicting the
one above it
Reference:
-
`grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc:54-55`
Line 41 now says the reassignment behaviour "holds for ... one-to-many and
many-to-many associations alike". Thirteen lines later the borrowed-collection
paragraph says "A one-to-many or many-to-many is held in a
`PersistentCollection` instead and is left to the store". Both are true, but
only because the second sentence is scoped to the borrowing case, and the
reader has to work that out. Leading the sentence with the scope fixes it:
"When *borrowed* from another entity, a one-to-many or many-to-many is held in
a `PersistentCollection` and is stored as-is, ...".
## Verified as correct
- **Only the generated setter reaches `rewrap`.**
`FieldEntityAccess.setProperty` writes through `FieldWriter.write`, which is
`field.set`, so the key-value persister's post-persist
`entityAccess.setProperty(name, associatedObjects)` in
`NativeEntryEntityPersister` never runs the setter, and the newly accepted
`PersistentList` old value has no effect on that path. The simple-map TCK
one-to-many and many-to-many specs confirm it.
- **Mongo update path for a rewrapped `hasMany`.**
`DirtyCheckingCollection.hasChanged()` is `parent.hasChanged(property) ||
hasChangedElements()`. After the re-init the `add` marks the property,
`MongoCodecEntityPersister` persists the children and records the ids, and
`OneToManyEncoder` writes them. A same-size replacement with different members
is flagged by the setter's `markDirty` equality check. An equal-content
replacement writes nothing, which is the correct outcome.
- **Borrowed `PersistentList`.** `boardA.members = boardB.members` still
stores the `PersistentCollection` verbatim, since `genericWrapperTarget`
returns null for it. That is the pre-PR behaviour and what the docs paragraph
describes.
- **`wrap(Map)` and `@CompileStatic`.** In `BasicCollectionTypeEncoder` the
map branch is guarded by `value instanceof Map`, so static overload resolution
picks the `Map` form; the `Collection` branch picks the collection form. No
call site passes an untyped or possibly-null value.
- **Style on the changed datastore-core files.** Checkstyle and CodeNarc are
clean on `grails-datastore-core`.
--
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]
