matrei commented on PR #16297:
URL: https://github.com/apache/grails-core/pull/16297#issuecomment-5566587272
Round 3:
## Review Findings
The latest update fixes the previous mapping-engine ID, update/delete,
association-reference, and immutable-map issues. The focused tests pass.
However, several public `updateAll` and mapping-engine association paths remain
incomplete.
## Merge Blockers
### High: `updateAll` is not proxy-safe
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:355-374`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:378-394`
-
`grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/proxy/JavassistProxyFactory.java:60-64`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy:663-676`
Both `updateAll` implementations obtain association IDs through the target
entity reflector:
```groovy
associatedEntity.reflector.getIdentifier(value)
```
and:
```java
getMappingContext().getEntityReflector(associatedEntity).getIdentifier(value)
```
A lazy GORM proxy stores its ID in the proxy handler, not in the reflected
domain field. The normal codec path already handles this with
`proxyFactory.getIdentifier(value)`, but `updateAll` does not.
Example:
```groovy
RefProject project = new RefProject(name: 'Target').save(flush: true)
RefProject proxy = RefProject.load(project.id)
RefTicket.where { title == 'Proxy update' }
.updateAll(project: proxy)
```
The update can extract `null` instead of the proxy key and write an invalid
association value. This affects both codec and mapping engines.
### High: Mapping-engine unidirectional collection and many-to-many
references remain uncoerced
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java:449-464`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java:392-411`
The mapping-engine association indexer now coerces embedded collection keys
and ordinary to-one references, but these paths still store declared String IDs
directly:
```java
dbRefs.add(new DBRef(getCollectionName(association.getAssociatedEntity()),
foreignKey));
```
```java
dbRefs.add(foreignKey);
```
The mapping-engine many-to-many path also stores:
```java
ids.add(entityAccess.getIdentifier(o));
```
For a target with a String ID stored as ObjectId, the target `_id` is BSON
ObjectId while collection entries or DBRef `$id` values remain BSON Strings.
Raw MongoDB joins, DBRef consumers, and external clients will not match those
references.
### High: `updateAll` incorrectly treats embedded to-one associations as ID
references
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:351-374`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:378-394`
-
`grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/EmbeddedPersistentEntity.java:31-39`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy:260-276`
`Embedded` extends `ToOne`, so the new association normalization also
catches embedded properties.
For:
```groovy
class Address {
String city
}
class Person {
String id
Address address
static embedded = ['address']
}
```
This call:
```groovy
Person.where { ... }
.updateAll(address: new Address(city: 'after'))
```
tries to extract an ID from the embedded object and writes that scalar
value, or `null`, instead of encoding a BSON subdocument. Normal persistence
has separate embedded encoding paths and does not send embedded values through
`ToOneEncoder`.
Embedded associations should be excluded from ID-reference normalization and
encoded using the normal embedded mapping logic.
### High: `updateAll` does not encode collection associations
References:
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:351-398`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:378-410`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy:619-644`
The new normalization handles only `ToOne` associations. `OneToMany`,
`ManyToMany`, and `EmbeddedCollection` properties are sent through `$set`
unchanged.
Example:
```groovy
Parent.where { ... }
.updateAll(children: [child])
```
Normal persistence stores association IDs, or DBRefs when configured. The
bulk-update path instead sends domain objects or embedded representations
directly. Depending on the engine and codec registry, this can fail or create a
representation inconsistent with normal persistence.
If collection properties are part of the public `updateAll(Map)` contract,
they need the same storage-aware encoding as normal persistence.
### Medium: Engine documentation wording is contradictory
References:
-
`grails-data-mongodb/docs/src/docs/asciidoc/gettingStarted/advancedConfig.adoc:128-142`
-
`grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy:96-104`
The documentation says:
> The default, and only supported value, is `codec`.
It then documents selecting `mapping`, and the implementation continues to
support it with a deprecation warning.
The wording should say that `codec` is the default and recommended engine,
while `mapping` remains available for compatibility but is deprecated.
## Resolved Findings
- Mapping-engine insert and point-read `_id` representation.
- Mapping-engine update and delete filter coercion.
- Mapping-engine iterable delete coercion.
- Mapping-engine ordinary to-one reference and DBRef ID coercion.
- Codec and mapping-engine ordinary to-one `updateAll`.
- Codec `updateAll` caller-map mutation and immutable-map handling.
- Association `IN` coercion for ordinary to-one associations.
- Nested negation and junction coercion.
- Empty assigned String ID deletion.
- Identity-generation documentation contradiction.
- Non-codec engine deprecation warning and documentation addition.
## 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' \
--no-daemon
```
The complete `grails-data-mongodb-core` test task was run previously with
the expanded changes and the affected tests passed, but the local process
exceeded the timeout before a final Gradle completion result.
The remaining proxy, embedded, collection-association, and mapping-engine
collection-reference paths should be addressed and covered with tests before
approval.
--
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]
