matrei commented on PR #16297:
URL: https://github.com/apache/grails-core/pull/16297#issuecomment-5530650276
AI review:
## Review Findings
The latest update adds coverage in `87219f1`, but it does not change the
production implementation. The following issues from the previous review remain.
### High: Legacy Mongo engine is incompatible with the new default
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java:151,413-417`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoEntityPersister.java:183-191`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java:145-188`
`MongoMappingContext` now resolves a bare `String id` to `storedAs:
ObjectId`, but the non-codec/mapping engine still generates and writes String
`_id` values. The shared `MongoQuery` converts ID criteria to `ObjectId`.
With `grails.mongodb.engine: mapping`, a saved domain can therefore contain
a BSON String `_id` while query-based operations search for an ObjectId. This
affects `findById`, ID criteria, `findAllByIdInList`, and bulk criteria
operations. The behavior is inconsistent because some direct persistence paths
still use the String representation.
The new default needs to be implemented by both engines, or the default and
query coercion need to be restricted to the codec engine.
### High: `IN` queries on to-one associations do not coerce IDs
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java:171-189,734-758`
-
`grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/query/BsonQuery.java:766-785`
The generic preprocessing passes the entire `In` collection to
`coerceIdToStoredType()`:
```java
pc.setValue(MongoIdCoercion.coerceIdToStoredType(raw, idTarget));
```
The `In` handler then reads its separate `values` collection and only
applies storage coercion when the queried property is the entity's own
identity. Association instances are unwrapped to their declared IDs without
converting them to the target entity's storage type.
For example:
```groovy
Parent.where {
child in [childInstance]
}.list()
```
When `Child` declares `String id` and stores `_id` as ObjectId, the query
sends a String association ID against an ObjectId foreign key. This also
affects `findAllByChildInList(...)` and DBRef associations. The association
`IN` handler should coerce each unwrapped ID using the associated entity's
mapping.
### High: Negated scalar ID and association criteria bypass coercion
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java:734-768`
-
`grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/query/BsonQuery.java:286-307`
The Mongo-specific coercion runs only for the current junction. The
inherited `BsonQuery` negation handler invokes nested criterion handlers
directly, so nested `Equals` and association criteria never pass through the
Mongo-specific preprocessing.
For example:
```groovy
Domain.where {
not {
eq 'id', hex
}
}.list()
```
And dynamic finders such as `findAllByIdNot(hex)` can generate a negated
String predicate against an ObjectId `_id`, meaning the document being excluded
may still be returned. Coercion needs to be applied recursively inside
negations.
### High: `updateAll` writes to-one association IDs in the declared
representation
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:347-356`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy:673-689`
The normal codec persistence path converts association IDs to the target
entity's storage type. The bulk update path instead replaces an association
object with its raw declared ID:
```groovy
properties.put(associationName,
association.associatedEntity.reflector.getIdentifier(value))
```
For a String-id target, `updateAll(child: child)` can write a BSON String
where normal persistence writes an ObjectId. For DBRef mappings it can also
write a plain ID instead of a DBRef. Subsequent association queries and
external MongoDB clients will not reliably see the updated relationship.
Bulk association updates should use the same storage-type and DBRef
representation as normal association encoding.
### Medium: Empty assigned String IDs cannot be deleted
Reference:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:202-214`
The invalid-ObjectId fallback allows assigned natural keys to be stored as
BSON Strings. An empty String is a valid BSON `_id`, but the delete path uses
Groovy truthiness:
```groovy
if (k) {
nativeKeys << k
}
```
For `id == ''`, the delete model is skipped and the document remains. This
should check `k != null` instead.
### Medium: Identity-generation documentation contradicts the new default
Reference:
-
`grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc:127-131,157-171`
The earlier section still says that a bare `String id` defaults to BSON
String storage and presents `storedAs: ObjectId` as opt-in. The later section
says ObjectId is the Grails 8 default.
The earlier explanation should be updated to describe `storedAs: String` or
`defaultStoredAs: string` as the opt-out.
## What the Latest Update Covers
- Adds unit coverage for `MongoIdCoercion` and both coercion directions.
- Adds raw BSON coverage for to-one and to-many association storage.
- Adds coverage for normal association traversal, bidirectional association
lookup, `findAllById`, and `findAllByIdInList`.
- Confirms that BSON String association references can still be decoded.
These tests pass, but they do not cover the legacy engine, association `IN`
criteria with String/ObjectId IDs, negated scalar ID criteria, bulk association
updates, or empty assigned String IDs.
## Verification
The following focused command passed:
```text
./gradlew :grails-data-mongodb-core:test \
--tests 'org.grails.datastore.gorm.mongo.bugs.MongoIdCoercionSpec' \
--tests
'org.grails.datastore.gorm.mongo.bugs.StringIdAssociationStorageSpec' \
--no-daemon
```
The existing association, negation, batch update/delete, and `hasOne` specs
also passed. The complete `grails-data-mongodb-core` test task was started but
exceeded the local six-minute timeout; no failure was observed in the captured
output.
--
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]
