matrei commented on PR #16297:
URL: https://github.com/apache/grails-core/pull/16297#issuecomment-5537409198
Round 2:
## Review Findings
The latest update adds production fixes for the codec engine and for
mapping-engine insert/point-read behavior. The focused tests pass, but the
mapping engine still has inconsistent update/delete and association write
paths. There is also a regression in codec bulk updates.
## Merge Blockers
### High: Mapping-engine updates and deletes still use the wrong `_id` type
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoEntityPersister.java:331-340,415-421`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:141-160,181-195`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:286-295`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java:802-808`
The mapping-engine insert and point-read paths now use `MongoIdCoercion`,
but the flush path still constructs pending update and delete filters from the
declared identifier without coercion:
```java
final Document id = new Document(MongoConstants.MONGO_ID_FIELD, nativeKey);
```
and:
```java
final Object k = delete.getNativeKey();
```
For a bare `String id` using the new ObjectId default, MongoDB stores `_id`
as an `ObjectId`, while these filters use the String value. As a result, after
reloading a mapping-engine entity, `save()` can fail to update it and
`delete()` can leave it in the database.
The iterable delete path also builds a query using literal `_id`, while
`resolveIdCriterionTarget()` only recognizes the logical identity name `id`, so
that path does not receive the new ID coercion either.
The new `MappingEngineStringIdStorageSpec` only covers insert and point
retrieval at `:59-95`; it does not cover update, single delete, iterable
delete, or batch delete.
### High: Mapping-engine association references still use declared String IDs
References:
-
`grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/engine/NativeEntryEntityPersister.java:1001-1057`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java:197-207,328-334,434-449`
The mapping engine still passes association IDs unchanged to
`formulateDatabaseReference()` and to the association indexer. For a String-id
target stored as ObjectId:
- Plain association foreign keys are stored as BSON Strings.
- DBRef `$id` values are stored as BSON Strings.
- Embedded and unidirectional collection references have the same mismatch.
`MongoQuery` now coerces association criteria to ObjectId, so mapping-engine
association queries can search for ObjectIds against String foreign keys and
return no results. This affects direct to-one queries, reverse one-to-many
lookups, DBRefs, and association `IN`/negated criteria.
The new `StringIdAssociationStorageSpec` exercises the default codec engine
and does not exercise `MongoSession` or the mapping-engine persister.
### High: Mapping-engine `updateAll` still does not encode association values
Reference:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:346-354`
The mapping-engine bulk update path sends the caller's property map directly
inside `$set`. It does not:
- Extract a to-one association's identifier.
- Coerce that identifier to the target entity's `storedAs` type.
- Create a DBRef when the association uses `reference: true`.
Therefore, with `engine: mapping`, an operation such as:
```groovy
Parent.where { ... }.updateAll(child: child)
```
can write an incompatible domain object or an association representation
that cannot be queried or decoded consistently.
Please add mapping-engine coverage for to-one associations, DBRefs, and bulk
association updates.
### Medium: Codec `updateAll` mutates the caller's property map and rejects
immutable maps
Reference:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:345-374`
The codec bulk-update implementation normalizes association values by
calling `properties.put(...)` on the caller-provided map. This changes the
caller's data:
```groovy
def updates = [project: project]
criteria.updateAll(updates)
```
After the call, `updates.project` is an ObjectId or DBRef rather than the
original domain object.
It also fails for immutable maps:
```groovy
Map updates = Collections.singletonMap('project', project)
criteria.updateAll(updates)
```
This throws `UnsupportedOperationException` before the update is sent. Copy
the map before normalizing association values.
## Previous Findings Status
| Finding | Status |
| --- | --- |
| Default incompatible with mapping engine | **Partially fixed**: insert and
point read fixed; update, delete, and association paths remain inconsistent |
| To-one association `IN` criteria | **Fixed for codec-backed documents;
still inconsistent for mapping-engine data because association writes remain
uncoerced** |
| Negated scalar/association criteria | **Fixed for codec-backed documents;
still inconsistent for mapping-engine data because association writes remain
uncoerced** |
| Codec `updateAll` association representation | **Representation fixed for
mutable maps; map mutation and immutable-map regression introduced** |
| Empty assigned String ID deletion | **Fixed** at
`MongoCodecSession.groovy:204-207` |
| Contradictory identity-generation documentation | **Fixed** at
`idGeneration.adoc:131-171` |
## Verification
The following focused command 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' \
--no-daemon
```
The existing MongoDB association, negation, batch update/delete, and
`hasOne` tests also passed. The complete `grails-data-mongodb-core` test task
was run, but the local process exceeded the ten-minute timeout before Gradle
printed a final completion result; no failures were visible in the captured
output.
The remaining mapping-engine issues should be fixed and covered with update,
delete, association, DBRef, and bulk-update tests before merging.
--
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]
