Default String id storage to ObjectId, fix association id coercion, and deprecate the non-codec engine - #16297
Conversation
Make `grails.mongodb.stringIds.defaultStoredAs` default to `objectid`, so a
domain declaring `String id` persists `_id` as a BSON ObjectId while keeping
String ergonomics in application code. Set it to `string` to opt out.
The default is applied via the field initializer rather than only in the
config-reading constructors, so all four MongoMappingContext constructors
register entities with the same default.
Making it the default surfaced five latent defects in the storedAs path, all
of which reproduce today by setting defaultStoredAs: objectid explicitly:
- ToOneEncoder and OneToManyEncoder wrote association references using the
declared id type, so a reference pointed at an ObjectId _id with a BSON
String and matched nothing from $lookup or the raw driver.
- ToOneDecoder read by the predicted type and threw
BsonInvalidOperationException on documents whose stored type legitimately
differs (a non-hex assigned id falls back to BSON String even under
storedAs: ObjectId). It now reads the actual BSON type.
- OneToManyDecoder returned stored-type ids rather than the declared type.
- MongoQuery coerced only identity criteria, so a filter on a to-one
association sent a hex String against an ObjectId foreign key; bidirectional
one-to-many and hasOne lookups silently resolved to empty.
- MongoQuery also missed findAllById(hex), which builds Equals('id', ..)
rather than IdEquals and so bypassed id coercion entirely, returning no
results. findAllByIdInList was unaffected because the In handler covers it.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16297 +/- ##
==================================================
+ Coverage 55.0529% 55.3811% +0.3282%
- Complexity 20773 20919 +146
==================================================
Files 2110 2110
Lines 101368 101522 +154
Branches 18005 18058 +53
==================================================
+ Hits 55806 56224 +418
+ Misses 37517 37187 -330
- Partials 8045 8111 +66
🚀 New features to boost your workflow:
|
|
A concrete example of the association-reference bug from a real application running An activity._id = ObjectId("69d441c2ba1f8010688e2695")
activity.issue = "69d441c2ba1f8010688e2694" // BSON String
issue._id = ObjectId("69d441c2ba1f8010688e2694") // BSON ObjectIdThe reference and its target are different BSON types, so nothing outside GORM can join them: db.issue.find({_id: activity.issue}) // null
db.issue.find({_id: ObjectId(activity.issue)}) // found
db.activity.aggregate([
{ $lookup: { from: "issue", localField: "issue", foreignField: "_id", as: "iss" } }
]) // iss: [] for every rowGORM traversal is unaffected —
|
| "Falling back to the default ('objectid').", | ||
| value, MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS); | ||
| return null; | ||
| return DEFAULT_STRING_ID_STORED_AS; |
There was a problem hiding this comment.
COOL, previous behavior was wrong!
|
Understood. Since the previous behavior was incorrect, please ensure the updated logic correctly addresses the requirements for the MongoMappingContext. If you would like me to review the specific code changes or provide an alternative implementation based on the current diff, please let me know. |
borinquenkid
left a comment
borinquenkid
left a comment
There was a problem hiding this comment.
The idea is totally correct, it needs unit tests
MongoIdCoercionSpec covers both coercion directions and their fallback
branches directly: null key, null entity, a value already of the target type,
the non-hex natural key that must stay a String rather than produce a
{_id: null} filter, an ObjectId-id domain being left alone, and a round trip.
StringIdAssociationStorageSpec asserts the BSON that lands on disk rather than
only that traversal works — traversal passed the whole time the stored type
was wrong, which is why the mismatch went unnoticed. It covers to-one and
to-many reference storage, resolution through GORM, a bidirectional
one-to-many, findAllById and findAllByIdInList, and a reference written as a
BSON String so pre-existing data keeps decoding.
Each case was checked against reverted production code: reverting the encoders
fails the two reference-storage cases and findAllById, and reverting only
MongoQuery fails the bidirectional and findAllById cases.
|
Thanks — added in 87219f1.
Each case was checked against reverted production code so it fails without the fix:
The second check turned out to matter: with everything reverted the bidirectional case still passes, because references written as String and queried as String are self-consistent. It only fails in the mixed state that the encoder fix creates, which is the real-world one. |
|
AI review: Review FindingsThe latest update adds coverage in High: Legacy Mongo engine is incompatible with the new defaultReferences:
With 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:
|
…default The Decoupling section introduced storedAs: ObjectId as opt-in and stated that a bare String id writes _id as a BSON String, contradicting the Global Default section below it.
The bulk delete path collected native keys under Groovy truthiness, so an empty String id -- a valid BSON _id, reachable through the invalid-ObjectId fallback for assigned natural keys -- was dropped from the delete model and the document silently survived.
The bulk update path replaced an association object with its raw declared identifier, so updateAll(child: child) wrote a BSON String where normal persistence writes the target's stored _id type, and wrote a plain id where the mapping asks for a DBRef. Association queries and external clients could not match the updated relationship. It now coerces through the associated entity's mapping and emits a DBRef when the mapping declares one, matching ToOneEncoder.
The Mongo-specific preprocessing ran only for criteria at the current junction
level. The inherited BsonQuery negation handler dispatches nested criteria
itself, so an id or to-one association criterion inside not { } never reached
it and was sent as a hex String against an ObjectId; findAllByIdNot(hex) could
therefore fail to exclude the document it named.
The preprocessing moves into a helper that recurses through junctions, so
criteria nested inside not { }, and { } and or { } are coerced too.
The IN handler applied the storage type only when the queried property was the entity's own identity. getInListQueryValues unwraps association instances to their declared identifier, so a to-one association IN criterion sent hex Strings against ObjectId foreign keys and matched nothing -- affecting 'child in [childInstance]' and findAllByChildInList(..). It now resolves the entity whose identifier mapping governs the criterion, the same way the scalar path does, and coerces each unwrapped value against it.
MongoEntityPersister decided the stored _id type from the declared identifier type alone, so a String-id domain always wrote a BSON String. Once a bare String id resolves to storedAs: ObjectId, a document saved by this engine could not be found again, because the shared MongoQuery sends an ObjectId. Two places needed it, not one: storeEntry overwrites _id with the declared-type identifier immediately after generateIdentifier runs, and createDBObjectWithKey builds the by-key filter and passed the hex String straight through for a String identifier. Both now go through the id mapping, while the declared-type value is still what the domain sees.
|
Thanks @matrei ! All six addressed, one isolated commit each, every fix pinned by a test verified to fail without it.
Two notes where the investigation diverged from the report. The legacy engine needed two changes, not one. Negation. The
|
|
from the discussion in the weekly, we acknowledged that this functionality was added in 7.2.x and then this PR is changing the default in the major release. |
|
If @borinquenkid is ok with this change, I'm good to proceed. |
|
Round 2: Review FindingsThe 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 BlockersHigh: Mapping-engine updates and deletes still use the wrong
|
| 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:
./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.
The codec bulk update path wrote the encoded association reference back into the caller-provided map. That replaced their domain object with an ObjectId or DBRef after the call, and threw UnsupportedOperationException outright for an unmodifiable argument such as Collections.singletonMap or Map.of. The map is now copied before normalising. Pre-dates this branch -- the same line previously wrote the declared identifier back into the caller's map -- but the encoded value makes the mutation more surprising, and it is cheap to stop doing.
|
Round 2 addressed, two isolated commits.
Findings 1-3 are all the same shape: the mapping engine builds its flush-time update and delete filters from the declared identifier, and writes association references and DBRef Scoping affects the default only: an application that explicitly sets This also reverts the partial mapping-engine
Correct on the defect, and it is worth noting it is not a regression: the line before this branch was already
One note on the round-2 verification section: a run that exceeds a local timeout without printing a result is not evidence of passing. I mention it only because the same caveat appeared in round 1 and the conclusion happened to hold both times. |
MongoEntityPersister decided the stored _id type from the declared identifier type alone, so a String-id domain always wrote a BSON String. Once a bare String id resolves to storedAs: ObjectId, the shared MongoQuery sends an ObjectId and never matches those documents. Four places needed it. storeEntry overwrites _id with the declared-type identifier immediately after generateIdentifier runs, and is the single point where _id is written for this engine. createDBObjectWithKey builds the by-key filter. MongoSession builds the flush-time update filter and the single and batch delete keys, and its iterable delete filters on the literal _id field rather than the logical identity name, so the criterion preprocessing in MongoQuery does not recognise it. The declared-type value is still what the domain sees throughout.
…engine formulateDatabaseReference returned the association's declared identifier, and setEmbeddedCollectionKeys built its DBRef list the same way, so a String-id target stored as ObjectId was pointed at by a BSON String. The codec engine coerces this in ToOneEncoder. Left uncoerced, no $lookup, raw driver query or coerced association criterion could match the document being referenced -- and MongoQuery now coerces association criteria, so those queries searched for ObjectId against String foreign keys. Plain foreign keys, DBRef $id values and embedded collection references all go through the target entity's id mapping now.
…rop the engine scoping MongoSession.updateAll sent the caller's property map straight into $set, so a bulk association update wrote the domain object itself rather than a reference. It now extracts the target's identifier, coerces it to the type that target's _id is stored as, and emits a DBRef where the mapping asks for one -- matching both normal persistence and the codec engine's updateAll. Normalised into a copy, so the caller's map is untouched and may be immutable. With every non-codec path now going through the id mapping, the default no longer needs restricting to the codec engine, so that scoping is removed and MongoMappingContext applies the default on both engines again. MappingEngineStringIdStorageSpec covers this engine end to end: insert, point read, update, single delete, iterable delete, to-one reference storage, traversal, and bulk association update. It drives the session API rather than the GORM statics, which bind to whichever datastore registered the class last, and asserts up front that the session really is a MongoSession -- without that guard the cases would pass against the codec engine and prove nothing.
|
Took the other option: the non-codec engine is fixed rather than excluded, so the default applies to both. Three isolated commits, and the engine scoping from
Two things that made the coverage harder than it looks, worth recording since they cost me two false green runs:
|
The "mapping" engine has not been the default since GORM 6, is selected by an undocumented setting, and reaches MongoDB through an entirely separate persister hierarchy -- MongoSession, MongoEntityPersister and AbstractMongoObectEntityPersister -- that has to be kept in step with the codec one for every change to the storage layer. It carries no capability the codec engine lacks. That cost is not theoretical: making ObjectId the default for String ids required six separate fixes in this engine (id storage, the by-key filter, the flush-time update and delete filters, the iterable delete, association references and DBRef $id values, and bulk association updates), none of which had any test coverage. Before this branch nothing in the test suite exercised grails.mongodb.engine at all. Deprecated rather than removed, because MongoEntityPersister and AbstractMongoObectEntityPersister are public types that third parties may extend, and an application setting the engine today deserves a warning before its configuration stops working. Selecting it now logs at startup, and the setting is documented for the first time -- as deprecated, with the advice to remove it.
codeconsole
changed the title
|
Round 3: Review FindingsThe latest update fixes the previous mapping-engine ID, update/delete, association-reference, and immutable-map issues. The focused tests pass. However, several public Merge BlockersHigh:
|
Both updateAll implementations reflected the association value's identifier directly. A lazy GORM proxy keeps its id in the proxy handler rather than the reflected field, so reflecting one yields null and the bulk update writes an invalid reference. ToOneEncoder asks the proxy factory first on the normal persistence path; updateAll now does the same, on both engines.
Embedded extends ToOne, so the association normalization added for bulk updates also caught embedded properties. An embedded value is a subdocument with no identity of its own: normal persistence encodes it through the embedded path rather than ToOneEncoder, and reflecting an id from one yields null, so updateAll(address: new Address(..)) wrote a scalar or null where a subdocument belongs. Both engines now skip Embedded and leave the value for the normal encoding.
…on-codec engine Two association write paths still stored the declared identifier. The association indexer holds unidirectional one-to-many keys on the owning document, as plain ids or DBRefs, and the many-to-many path builds its own id list. For a target whose _id is stored as ObjectId, both left BSON Strings that raw joins, DBRef consumers and external clients cannot match. Both now go through the target entity's id mapping, matching formulateDatabaseReference and setEmbeddedCollectionKeys.
The association normalization handled only ToOne, so OneToMany and ManyToMany values went into $set as the domain objects themselves rather than the ids normal persistence stores -- inconsistent at best, and dependent on whatever the codec registry happens to do with a domain instance. Both engines now encode each element the way OneToManyEncoder does: the target's identifier in its stored _id type, wrapped in a DBRef where the mapping declares one, and proxy-safe. EmbeddedCollection is excluded for the same reason as Embedded -- those are subdocuments, not references.
The setting was described as having 'only supported value: codec' and then documented selecting 'mapping', which the implementation still supports with a deprecation warning. It now says codec is the default and recommended value and mapping remains available for compatibility but is deprecated.
|
Thanks @matrei All five addressed, one isolated commit each, every fix pinned by a test verified to fail without it.
Two of these were mine to own. For the collection paths, On the documentation: agreed, "only supported value" contradicted the paragraph documenting
One note on the verification section, same as last round: a run that exceeds a local timeout without printing a completion result is not evidence that it passed. The conclusion has held each time, but it is worth separating "the focused tests passed" from "the full task was inconclusive". |
|
Round 4 Review FindingsThe latest update resolves all five findings from the previous round: proxy-safe However, the new to-many branch in Merge BlockersHigh:
|
| 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.
…ions Basic extends ToMany, so a collection of simple values -- hasMany = [labels: String] -- entered the branch added for OneToMany and ManyToMany. A Basic association has no associated entity, so the reflector lookup dereferenced null and updateAll threw NullPointerException on both engines for a call that worked before this PR. The branch now names the two kinds it is for and requires a non-null associated entity. EmbeddedCollection no longer needs excluding: it extends Association directly, not ToMany, so it never reached here and the exclusion implied a case that cannot occur.
A bidirectional one-to-many keeps its foreign key on the inverse side, so there is no
field on the owning document to update -- OneToManyEncoder's shouldEncodeIds skips it for
that reason, and the decoder never reads such a field.
Left in the $set the value was written as raw subdocuments, and the owner then failed to
decode at all:
BsonInvalidOperationException: ReadBSONType can only be called when State is TYPE
Skipping the id encoding alone is not enough, because the property still reaches $set. It
is now rejected by name. Dropping it silently would be the same silent no-op this review
objected to elsewhere, and the operation is genuinely unexpressible: reassigning children
means updating their foreign key, which an update on the parent cannot do.
…e reads This engine keeps many-to-many ids under a suffixed key -- see setManyToMany and getManyToManyKeys -- so writing the plain association name left the real field untouched. The document ended up with both, the read path kept returning the old association, and the update was silently a no-op. Also adds the read-back assertion the review noted was missing: the unidirectional one-to-many cases only checked the raw document, so nothing covered the retrieveAllEntities conversion of coerced ObjectId keys.
ToOneEncoder writes a reference on the owner only when the foreign key is not held by the child -- its !isForeignKeyInChild() guard. The updateAll normalization had no equivalent check, so updateAll(nose: n) on a hasOne owner wrote an id field onto a document that normal persistence never gives one, the same corruption shape as the bidirectional one-to-many case. Found by an adversarial audit of the association-type space rather than by a reviewer: two independent analysts converged on it after the previous three defects here were all the same shape -- an association kind entering a branch meant for a different one.
Two divergences from normal persistence, both found by the same audit as the hasOne case. OneToManyEncoder drops null entries before wrapping a reference collection, so it never builds DBRef(collection, null); the bulk path kept them and would hand the driver a null id. It now filters exactly where the encoder does, and leaves a plain id list alone. The non-codec engine's setManyToMany stores plain identifiers under its suffixed key and getManyToManyKeys reads them back the same way -- it never writes DBRefs. Wrapping them in the bulk path produced an array the read path cannot interpret, so many-to-many values are now written as plain ids there.
|
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.
Basic. Correct — Bidirectional one-to-many. Skipping the id encoding alone was not enough — the property still reached Many-to-many key. Fixed, and both observations addressed: 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 It also caught two representation divergences:
|
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Review Findings - round 5
The latest update resolves all three findings from the previous round, and additionally rejects hasOne associations in updateAll with a clear exception. The focused tests and the complete grails-data-mongodb-core check pass. I have no remaining merge blockers; the items below are non-blocking suggestions.
Resolved Findings
updateAllon a basic collection no longer enters the id branch.Basicis excluded by restricting the branch toOneToMany/ManyToManywith a non-null associated entity, and both engines have alabels: ['y', 'z']test.updateAllon a bidirectional one-to-many now throwsUnsupportedOperationExceptioninstead of writing a field the decoder cannot read. Both engines test that nothing stray lands on the owner document, and the codec spec confirms the owner still decodes afterwards.- Mapping-engine
updateAllon a many-to-many now writes plain ids under<name>_$$manyToManyIds, the keygetManyToManyKeysreads, and removes the plain key. The new test asserts both the raw document and the read-back. hasOneinupdateAllis rejected in both engines, matching the!isForeignKeyInChild()guard inToOneEncoder.- The unidirectional one-to-many read-back through coerced keys on the mapping engine is now asserted, not just the raw document.
Suggestions (non-blocking)
Low: An identifier value passed for a to-one association fails with a reflector error
References:
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:388-390grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:405-409
Both engines reflect the identifier off whatever value is supplied. A caller who passes the id rather than the instance:
RefTicket.where { title == 'x' }.updateAll(project: project.id)gets:
java.lang.IllegalArgumentException: Cannot read field [private java.lang.String ...RefProject.id]
from object [6aa27b25977124bdfec620b9] of type [class java.lang.String]
On the codec engine this predates the PR, since updateAll already reflected the id there. On the mapping engine it is new: before this PR the raw value went through $set unchanged, which with String-stored ids happened to be correct. Since updateAll now knows the target's stored type anyway, a value that is not an instance of the associated class could simply be passed through MongoIdCoercion.coerceIdToStoredType instead of the reflector. That would make updateAll(project: id) and updateAll(project: instance) equivalent on both engines. Fine to defer to a follow-up.
Low: Test name does not match what it asserts, and the DBRef null-drop is untested
Reference:
grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdAssociationStorageSpec.groovy:398-411
"updateAll drops nulls from a to-many rather than encoding them" asserts raw.get('tags') == [ObjectId, null], i.e. the null is kept. The drop only happens for a reference: true mapping, and no test exercises that branch. I verified with a temporary probe that a reference: true to-many with [tag, null] lands as a single DBRef and reads back correctly, so the code is right; the test should be renamed to say the plain list keeps nulls, and a reference: true case added for the drop.
Low: Coverage gaps for paths that only one engine tests
hasOnerejection is tested on the codec engine only. The mapping engine throws the same exception (verified with a probe);MappingEngineStringIdStorageSpecshould have the matching case, since the two implementations are separate code.- Many-to-many
updateAllis tested on the mapping engine only. On the codec engine it shares the unidirectional branch, and a probe confirmed both the owning and inverse side write an ObjectId array under the plain name and read back, but a test inStringIdAssociationStorageSpecwould guard it.
Nit: Dead type check
References:
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:411grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoSession.java:430
ManyToMany extends ToMany directly, not OneToMany, so association instanceof OneToMany && !(association instanceof ManyToMany) never excludes anything. Harmless, but like the EmbeddedCollection note last round it implies a hierarchy that does not exist. association instanceof OneToMany && association.isBidirectional() is the whole condition.
Nit: Documentation
The new UnsupportedOperationException for hasOne and bidirectional one-to-many in updateAll is user-visible behavior. Before this PR those calls corrupted the owner document or failed with a driver error, so this is a fix rather than a feature, but the MongoDB guide has no mention of updateAll at all. A short note on which association kinds bulk updates support, and to update the inverse side for the others, would save users a round trip to the exception message.
Verification
Complete module check, including Checkstyle and CodeNarc on main sources:
./gradlew :grails-data-mongodb-core:check --no-daemon --continue
| Tests | Failures | Errors | Skipped |
|---|---|---|---|
| 723 | 0 | 0 | 45 |
BUILD SUCCESSFUL, no Checkstyle or CodeNarc violations.
The identifier-value behavior, the DBRef null drop, the codec many-to-many update on both sides, the bidirectional many-to-one update from the child side, and the mapping-engine hasOne rejection were each exercised with a temporary probe spec against the running datastores, which was removed afterwards.
I am happy to approve as is; the suggestions above can be follow-ups.
✅ All tests passed ✅🏷️ Commit: 8707742 Learn more about TestLens at testlens.app/docs. |
4 participants
Makes
grails.mongodb.stringIds.defaultStoredAsdefault toobjectid. A domain declaringString idnow persists_idas a BSON ObjectId while application code still sees the hexString. This is breaking for existing data, so it wants to land before 8.0.0.The default is applied in the field initializer rather than only in the config-reading constructors —
MongoMappingContexthas four constructors and only two read configuration, sonew MongoMappingContext("db")was registering entities with a different default.Latent bugs this surfaced
Every one of these reproduces on 8.0.x today by setting
defaultStoredAs: objectidexplicitly. They are not caused by the default change; making it the default is what drags them into the test suite.References written with the declared type.
ToOneEncoderandOneToManyEncoderwrote the id as declared, so a reference pointed at an ObjectId_idwith a BSON String:Decoders read by predicted type. A non-hex assigned id falls back to BSON String even under
storedAs: ObjectId, so predicting the type threw:They now switch on
bsonReader.currentBsonTypeand convert back to the declared type, which also handles collections written before astoredAschange.Queries coerced only identity criteria. A filter on a to-one association carries the associated entity's id but got no coercion, so bidirectional and
hasOnelookups sent a hex String against an ObjectId foreign key:The same gap applied to
INcriteria on an association (child in [childInstance],findAllByChildInList(..)) and to anything nested in a junction — the inherited negation handler dispatches nested criteria itself, sonot { eq 'id', hex }andfindAllByIdNot(hex)never reached the coercion and could fail to exclude the document they named.findAllByIdbypassed id coercion entirely, because a dynamic finder buildsEquals('id', ..)rather thanIdEquals:updateAllwrote the declared representation. The bulk path replaced an association with its raw declared id, bypassing the encoder — and normalized into the caller's own map, which mutated their argument and threw for an immutable one.An empty assigned String id could not be deleted. The bulk delete path collected keys under Groovy truthiness, so
id == ''— a valid BSON_id, reachable through the invalid-ObjectId fallback for natural keys — was dropped from the delete model and the document survived.The non-codec engine
The "mapping" engine reaches MongoDB through a separate hierarchy that used the declared identifier type throughout. Six paths needed the same treatment:
_idstorage (storeEntryoverwrites whatgenerateIdentifiersets, so both matter), the by-key filter, the flush-time update and delete filters, the iterable delete (which filters on the literal_idfield rather than the logical identity name), association references and DBRef$idvalues, and bulk association updates.Those are fixed, so the default applies to both engines. The engine is then deprecated: it has not been the default since GORM 6, the setting that selects it was undocumented until this PR, it offers no capability the codec engine lacks, and nothing in the test suite exercised it before this branch. Selecting it now logs a warning at startup. Deprecated rather than removed because
MongoEntityPersisterandAbstractMongoObectEntityPersisterare public types.Changes
MongoMappingContext—DEFAULT_STRING_ID_STORED_AS, applied via field initializer; unrecognized values fall back to the default rather than to a third silent behaviorMongoIdCoercion— addscoerceIdToDeclaredType, the inverse ofcoerceIdToStoredTypePersistentEntityCodec— coercion inToOneEncoder,OneToManyEncoder,ToOneDecoder,OneToManyDecoderMongoQuery— coerces to-one association and identity criteria, recursing through junctions;INcoerces after unwrappingMongoCodecSession—updateAllencodes associations into a copy of the caller's map; empty-id deleteMongoSession,MongoEntityPersister,AbstractMongoObectEntityPersister— the six non-codec paths, then deprecatedidGeneration.adoc,advancedConfig.adoc(including the engine setting, documented for the first time), and an upgrade note covering the data-compatibility breakTests
MongoIdCoercionSpec(12 cases, no live MongoDB) covers both coercion directions and their fallback branches.StringIdAssociationStorageSpecasserts the BSON that lands on disk rather than only that traversal works — traversal passed the whole time the stored type was wrong, which is why this went unnoticed.MappingEngineStringIdStorageSpeccovers the non-codec engine end to end, and asserts up front that the session really is aMongoSession: GORM statics bind to whichever datastore registered the class last, so without that guard the whole file would pass against the codec engine and prove nothing.Every case was checked against reverted production code so that it fails without its fix.
Adds coverage for the
defaultStoredAs: stringopt-out, which had none.LegacyVideois pinned tostoredAs: Stringso the legacy-breakage cases still reproduce, and specs asserting raw BSON reference types were updated.grails-data-mongodb-coreis at parity with8.0.xlocally;spring-data,spring-boot,embeddedandgrails-data-mongodball pass.