matrei commented on PR #16297:
URL: https://github.com/apache/grails-core/pull/16297#issuecomment-5614755247
Round 4
## Review Findings
The latest update resolves all five findings from the previous round:
proxy-safe `updateAll`, embedded exclusion, mapping-engine unidirectional
collection and many-to-many reference coercion, collection-association encoding
in `updateAll`, and the engine documentation wording. The focused tests pass.
However, the new to-many branch in `updateAll` is broader than the normal
persistence path it mirrors. It catches association kinds that normal
persistence handles differently, and one of them is a regression of a common
operation.
## Merge Blockers
### High: `updateAll` on a basic collection now throws `NullPointerException`
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:394-410`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:414-436`
-
`grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/types/Basic.java:38`
`Basic` extends `ToMany`, so a collection of simple values such as:
```groovy
class Note {
String id
String title
List<String> labels = []
static hasMany = [labels: String]
}
```
now enters the new to-many branch. A `Basic` association has no associated
entity, so the reflector lookup dereferences `null`:
```groovy
Note.where { title == 'note' }.updateAll(labels: ['y', 'z'])
```
Codec engine:
```text
java.lang.NullPointerException: Cannot invoke
"org.grails.datastore.mapping.model.PersistentEntity.getReflector()" because
the return value of "...castToType(Object, java.lang.Class)" is null
```
Mapping engine:
```text
java.lang.NullPointerException: Cannot invoke
"org.grails.datastore.mapping.model.PersistentEntity.getName()" because
"persistentEntity" is null
```
Before this PR the same call sent the String list through `$set` and worked.
The branch should be restricted to `OneToMany` and `ManyToMany` with a non-null
associated entity, or skip any association whose associated entity is `null`.
Both engines need a test for a basic collection in `updateAll`.
### High: `updateAll` on a bidirectional one-to-many corrupts the owning
document in the codec engine
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:394-410`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:414-436`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy:524,600-602`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java:455,480`
Normal persistence only stores an id array for a to-many when it is
unidirectional or many-to-many. For a bidirectional one-to-many the foreign key
lives on the inverse side, and `OneToManyDecoder` does not consume a value for
that field, it initializes a lazy collection instead.
The new `updateAll` branch does not make that distinction. For:
```groovy
class Parent {
String id
Set<Child> children
static hasMany = [children: Child]
}
class Child {
String id
Parent parent
static belongsTo = [parent: Parent]
}
```
this call:
```groovy
Parent.where { name == 'bi-parent' }.updateAll(children: [child])
```
writes a `children` array onto the parent document. The decoder then hits
that unread field and every subsequent read of the parent fails:
```text
org.bson.BsonInvalidOperationException: ReadBSONType can only be called when
State is TYPE, not when State is VALUE.
at
org.grails.datastore.bson.codecs.BsonPersistentEntityCodec.decode(BsonPersistentEntityCodec.groovy:192)
```
The child's `parent` reference is not updated either, so the relationship
does not change. In the mapping engine the same call leaves a stray `children`
array and the relationship is likewise unchanged.
The bulk path should mirror the `shouldEncodeIds` rule in
`OneToManyEncoder`: skip bidirectional one-to-many properties, or reject them
with a clear exception, rather than writing a field normal persistence never
writes or reads.
### Medium: Mapping-engine `updateAll` on a many-to-many writes to the wrong
field
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:414-436`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java:388,414`
The mapping engine stores and reads many-to-many ids under a suffixed key:
```java
manyToMany.getName() + "_$$manyToManyIds"
```
The new `updateAll` branch writes the encoded ids under the plain
association name. After:
```groovy
((MongoSession) session).updateAll(criteria, [rights: [right2]])
```
the document contains both `rights_$$manyToManyIds` with the old id and
`rights` with the new id, and reading the entity back still returns the old
association. The update is silently a no-op.
The mapping engine is deprecated, but this commit specifically claims to
encode collection associations for it, and `MappingEngineStringIdStorageSpec`
has no many-to-many or to-many `updateAll` coverage. Either use the same key as
`setManyToMany` or document that many-to-many bulk updates are unsupported on
this engine.
## Observations
- The mapping engine's unidirectional one-to-many read path resolves coerced
ObjectId keys correctly, and `updateAll(tags: [...])` on it works. Neither is
covered by a test in `MappingEngineStringIdStorageSpec`, which only checks the
raw document. A read-back assertion would guard the `retrieveAllEntities`
conversion.
- The `EmbeddedCollection` exclusion in both to-many branches is redundant,
since `EmbeddedCollection` extends `Association` directly rather than `ToMany`.
Harmless, but the comment implies a case that cannot occur.
## Resolved Findings
- `updateAll` extracts ids from lazy proxies in both engines.
- Embedded to-one associations are excluded from id normalization and are
written as subdocuments.
- Mapping-engine unidirectional collection and many-to-many references are
stored in the target's `_id` type.
- Ordinary unidirectional and many-to-many collections in `updateAll` are
encoded as stored ids in the codec engine.
- Engine documentation wording is consistent with the implementation.
## Verification
The following focused tests passed:
```text
./gradlew :grails-data-mongodb-core:test \
--tests
'org.grails.datastore.gorm.mongo.bugs.MappingEngineStringIdStorageSpec' \
--tests
'org.grails.datastore.gorm.mongo.bugs.StringIdAssociationStorageSpec' \
--tests
'org.grails.datastore.gorm.mongo.bugs.StringIdWithObjectIdStorageSpec' \
--tests
'org.grails.datastore.gorm.mongo.bugs.StringIdDefaultStoredAsConfigSpec' \
--tests 'org.grails.datastore.gorm.mongo.bugs.MongoIdCoercionSpec' \
--no-daemon
```
The three findings above were reproduced with a temporary probe spec against
both engines, which was removed afterwards.
The complete `grails-data-mongodb-core` test task also passed:
| Tests | Failures | Errors | Skipped |
| --- | --- | --- | --- |
| 715 | 0 | 0 | 45 |
`./gradlew :grails-data-mongodb-core:check -x test` passed as well, with no
Checkstyle or CodeNarc violations.
The basic-collection regression and the bidirectional one-to-many corruption
should be fixed and covered with tests in both engines before approval. The
mapping-engine many-to-many field mismatch should be fixed or explicitly
documented as unsupported.
--
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]
