codeconsole commented on PR #16297: URL: https://github.com/apache/grails-core/pull/16297#issuecomment-5615536589
All three addressed, plus three more of the same class that an audit turned up before you had to. One isolated commit each, every fix pinned by a test verified to fail without it. | commit | finding | |---|---| | `7deb7dc` | `updateAll` on a basic collection throws NPE | | `0dfbe06` | `updateAll` on a bidirectional one-to-many corrupts the owner | | `9528ba1` | mapping-engine many-to-many writes the wrong field | | `b1c52c2` | *(found by audit)* `updateAll` on a `hasOne` corrupts the owner | | `8707742` | *(found by audit)* null handling and many-to-many representation diverge from the encoders | **Basic.** Correct — `Basic extends ToMany` and has no associated entity. The branch now names the two kinds it is for and requires a non-null associated entity. You were also right that the `EmbeddedCollection` exclusion was redundant: it extends `Association` directly, so it never reached there. Removed. **Bidirectional one-to-many.** Skipping the id encoding alone was not enough — the property still reached `$set` and was written as raw subdocuments, so the owner still stopped decoding. It is now rejected by name. I chose rejecting over silently dropping because a silent drop is the same silent no-op you objected to for many-to-many, and the operation is genuinely unexpressible: reassigning children means updating their foreign key, which an update on the parent cannot do. `MongoQuery` already throws `UnsupportedOperationException` for join queries, so this follows that precedent. **Many-to-many key.** Fixed, and both observations addressed: `MappingEngineStringIdStorageSpec` now has many-to-many and to-many `updateAll` coverage, and a read-back assertion on the unidirectional one-to-many that exercises the `retrieveAllEntities` conversion rather than only the raw document. **On the two extra findings.** Three rounds running, every defect here has been the same shape: an association kind entering a branch meant for a different one. Rather than wait to find out what the fourth was, I ran an adversarial audit over the association-type space — four independent analysts reading the encoders and the type hierarchy, each finding then handed to a separate agent prompted to refute it. It found `hasOne` immediately, and four of the analysts converged on it independently. `ToOneEncoder` writes a reference on the owner only when `!isForeignKeyInChild()`; the normalization had no equivalent guard, so `updateAll(nose: n)` wrote an id field onto a document that normal persistence never gives one — the same corruption shape as the bidirectional case, in the to-one branch. It also caught two representation divergences: `OneToManyEncoder` drops nulls before wrapping a reference collection so it never builds `DBRef(collection, null)`, and `setManyToMany` stores plain identifiers under its suffixed key and never DBRefs, so wrapping them there produced an array `getManyToManyKeys` cannot read. `grails-data-mongodb-core` is at parity with `8.0.x` locally: the only failures are the two pre-existing `MongoDatastoreLifecycleSpec` cases, which also fail on an unmodified checkout here. checkstyle and codenarc clean. -- 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]
