Native gorm mongodb String to ObjectId conversion#15583
Native gorm mongodb String to ObjectId conversion#15583codeconsole wants to merge 8 commits intoapache:7.1.xfrom
Conversation
`ObjectId.class.isAssignableFrom(...)` and `String.class.isAssignableFrom(...)` are flagged by the CodeNarc UnnecessaryDotClass rule. Drop the redundant `.class` so the linter passes.
f7c69c1 to
949ccc1
Compare
|
Yes, to maintain consistency between save and query paths, the query conversion should also fall back to the original String when ObjectId conversion fails for invalid hex. Currently, the IdentityEncoder falls back gracefully, but IdEquals in MongoQuery directly calls convert() without try-catch, which could throw an exception. Wrapping it in try-catch like the coerce methods would align the behavior. |
…; keep original so the filter matches the BSON String the encoder wrote.
| "3.2.0" | true | ||
| "3.1.0" | true | ||
| "3.3.0" | true | ||
| "7.1.0" | false |
There was a problem hiding this comment.
We should fix this with an arbitrary large value instead of just setting to true.
There was a problem hiding this comment.
I've updated this in the 7.1 branch after seeing it fail on multiple reviews. You should be able to update your base branch now.
jdaugherty
left a comment
There was a problem hiding this comment.
@borinquenkid I know you've recently worked on mongo. Would you mind taking a look at this PR?
@copilot can you please review this PR too?
There was a problem hiding this comment.
Pull request overview
Adds first-class support in GORM MongoDB for persisting String id domains with _id stored as BSON ObjectId, plus a global default to opt into this behavior without per-domain mapping.
Changes:
- Introduces
storedAson identifier mapping (IdentityMapping#getStoredAs,Property.storedAs) and wires it throughMappingFactory. - Applies
storedAscoercion across MongoDB encoder/query/persister/session paths and adds a global default (grails.mongodb.stringIdsDefaultStoredAs). - Adds/updates docs and adds new integration/unit specs covering the previously asymmetric read/query/write behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/MappingFactory.java | Exposes storedAs dynamically via identity mapping, with composite-key null-safety. |
| grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/IdentityMapping.java | Adds default getStoredAs() hook for backends to honor storage-type coercion. |
| grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/Property.groovy | Adds storedAs to mapped form so backends can persist using a different native type. |
| grails-datamapping-support/src/test/groovy/org/grails/datastore/mapping/core/grailsversion/GrailsVersionSpec.groovy | Updates version expectation for 7.1.x baseline. |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy | Defines new global setting key for default String-id storage type. |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/AbstractMongoConnectionSourceSettings.groovy | Adds settings field for stringIdsDefaultStoredAs. |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java | Parses/applies global default storedAs for String-id domains during identity creation. |
| grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/IdentityEncoder.groovy | Encodes ids as BSON ObjectId (or String) based on storedAs, with non-hex fallback. |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java | Coerces id values for IdEquals and identity In queries to match stored BSON type. |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoCodecEntityPersister.groovy | Coerces ids for point lookup and $in batch retrieval (get, getAll). |
| grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy | Coerces update/delete filters’ native keys to match stored BSON type. |
| grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy | Adds regression/integration coverage for scan vs point-lookup vs write asymmetry and fixes. |
| grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy | Adds tests validating global default + per-domain override semantics. |
| grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc | Documents storedAs and global default, plus caveats. |
| grails-data-mongodb/docs/src/docs/asciidoc/gettingStarted/advancedConfig.adoc | Adds pointer to the new global config setting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
borinquenkid
left a comment
There was a problem hiding this comment.
Please increase the test coverage on the noted parts. If the coverage was missed in the review, respond inline
- Rename config key to nested form: grails.mongodb.stringIds.defaultStoredAs (Spring-style hierarchical namespace). Introduce StringIdSettings nested under AbstractMongoConnectionSourceSettings so ConfigurationBuilder binds at the nested path. - Add javadoc/comment cross-references from each id-coercion site (MongoCodecEntityPersister, MongoCodecSession, MongoQuery.IdEquals, MongoQuery.In) to the specific StringIdWithObjectIdStorageSpec feature methods that exercise them end-to-end. - Update idGeneration.adoc and advancedConfig.adoc to the nested key form.
PersistentEntityCodec.encodeUpdate silently dropped the recursive encode of non-DirtyCheckable embedded values (there was a "TODO: Support non-dirty checkable objects?" in the else branch). Result: when a caller assigned a new embedded instance to a previously-null property and then save(flush:true)'d, the parent's $set never included the embedded sub-document, so nothing landed in Mongo even though save() returned a non-null persisted instance. When we are encoding an embedded update and the value lacks dirty tracking, fall back to encoding every persistent property; the caller's encodeEmbeddedUpdate then $set's them onto "<assocName>.<prop>" paths exactly as the DirtyCheckable path does. Adds SingleEmbeddedAssignNullToNonNullSpec covering three variants: POGO embedded (the regression), @entity embedded, and a top-level scalar save control on the parent.
✅ All tests passed ✅🏷️ Commit: 69b8add Learn more about TestLens at testlens.app. |
GORM MongoDB:
storedAsIdentifier CoercionThe Problem
Before this change, GORM MongoDB had a silent asymmetry in how it handled identifier types:
String iddeclared +_id: ObjectIdon disk.list())get(hex))null— query sends{_id: "<hex>"}as BSON Stringsave())OptimisticLockingExceptiongetAll([hex]),findAllByIdInList,in('id', [...]))$inlist sent as BSON StringsThe reader was forgiving, but the query and write paths weren't. This made "declare
String idon a domain that previously usedObjectId id" a silent landmine — scans looked fine, everything else quietly broke.The ergonomic consequence: teams were forced to either keep
ObjectId id(which serializes to{"timestamp":..., "date":...}in JSON and requiresnew ObjectId(...)at every call site) or do a full data migration of every_idvalue in storage.The Fix
Two new knobs that let a domain keep
String idergonomics while persisting_idas a BSON ObjectId — no data migration required.Per-domain mapping option
Global config default
Per-domain
storedAsalways wins over the global default. Natural-key domains opt out withid storedAs: String.What Changed
10 production files across three modules, all coerce between declared type and storage type:
IdentityMapping.javadefault Class<?> getStoredAs() { return null; }Property.groovyClass<?> storedAsfieldMappingFactory.javacreateDefaultIdentityMappingoverloads exposestoredAsdynamically (composite-key-safe)MongoSettings.groovySETTING_STRING_IDS_DEFAULT_STORED_ASAbstractMongoConnectionSourceSettings.groovyString stringIdsDefaultStoredAsfieldMongoMappingContext.javastoredAs; warns on unrecognized valuesIdentityEncoder.groovyObjectId.isValidguard for natural keys)MongoQuery.javaIdEquals+Inhandlers coerce to storage type (fixesget(hex),findAllByIdInList, criteriain('id', ...))MongoCodecEntityPersister.groovyretrieveEntityandretrieveAllEntities(fixesgetandgetAll)MongoCodecSession.groovynativeKeyin update/delete filters (fixes saves and deletes)Why It's Better
String id(clean JSON, nonew ObjectId(...)dance, no[object Object]bugs at HTTP boundaries). Storage keeps 12-byte BSON ObjectId_id(half the index size of hex strings, embedded creation timestamp, native Mongo sort order)._id: ObjectId(...)continue to load via the existing decoder fallback; new writes produce BSON ObjectId too — everything stays consistent on disk.get(hex),findAllByIdInList, saves, deletes, and criteria in-lists all now match stored ObjectIds when the domain opts in. The misleadingOptimisticLockingExceptionon save (which previously pointed developers at concurrency rather than the real id-type mismatch) no longer fires.null/string— nothing changes for existing apps unless they explicitly opt in.generator: 'assigned'withstoredAs: ObjectIdand a non-hex value (e.g. a slug) falls back to BSON String rather than throwingIllegalArgumentExceptiondeep in the BSON pipeline.Testing
23 tests across 2 new specs, all against a real MongoDB testcontainer or real
MongoMappingContextinit:StringIdWithObjectIdStorageSpec(18 tests) — documents the original bugs, then proves each code path (read, point lookup, save, batchgetAll,findAllByIdInList, criteriain('id', ...), delete, update of legacy ObjectId-_iddocs, non-hex natural-key fallback, JSON serialization demo).StringIdDefaultStoredAsConfigSpec(5 tests) — global config plumbing: default off, global on, per-domain override, declared-type filter (ObjectId-id domains ignore the String-id global), unrecognized-value safe fallback.Full
:grails-data-mongodb-core:test+:grails-data-hibernate5-core:testgreen. Checkstyle clean on all three modified modules. Three rounds of automated code review completed, each round surfaced real bugs, all fixed.Documentation
objectMapping/idGeneration.adoc— new section "Decoupling the Declared Type from the Storage Type" + "Global Default" + "Caveats".gettingStarted/advancedConfig.adoc— one-line pointer to the new setting.