Copilot commented on code in PR #15583:
URL: https://github.com/apache/grails-core/pull/15583#discussion_r3123886439
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java:
##########
@@ -165,7 +183,28 @@ public MongoMappingContext(String defaultDatabaseName,
Closure defaultMapping, C
*/
@Deprecated
public MongoMappingContext(PropertyResolver configuration, Class...
classes) {
- this(getDefaultDatabaseName(configuration),
configuration.getProperty(MongoSettings.SETTING_DEFAULT_MAPPING, Closure.class,
null), classes);
+ super(getDefaultDatabaseName(configuration),
configuration.getProperty(MongoSettings.SETTING_DEFAULT_MAPPING, Closure.class,
null));
+ // Must run BEFORE initialize(classes) so that
MongoDocumentMappingFactory.createIdentity
+ // (invoked during entity registration) can read the global default.
+ String storedAsDefault =
configuration.getProperty(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS,
String.class, null);
+ this.stringIdDefaultStoredAs = parseStoredAs(storedAsDefault);
+ initialize(classes);
+ }
+
+ private static Class<?> parseStoredAs(String value) {
+ if (value == null) return null;
+ switch (value.toLowerCase()) {
+ case "objectid":
+ case "object_id":
+ return ObjectId.class;
+ case "string":
+ return String.class;
+ default:
+ log.warn("Unrecognized value '{}' for {}; accepted values are
'objectid' or 'string'. " +
Review Comment:
`parseStoredAs` accepts both `objectid` and `object_id`, but the warning
message says the only accepted values are 'objectid' or 'string'. This is
misleading when users supply `object_id` (they won't get a warning, but the
message implies it would be invalid). Consider either removing the `object_id`
alias or updating the warning/doc text to reflect all accepted values.
```suggestion
log.warn("Unrecognized value '{}' for {}; accepted values
are 'objectid', 'object_id', or 'string'. " +
```
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoCodecEntityPersister.groovy:
##########
@@ -175,6 +178,32 @@ class MongoCodecEntityPersister extends
ThirdPartyCacheEntityPersister<Object> {
new Document(AbstractMongoObectEntityPersister.MONGO_ID_FIELD, key)
}
+ /**
+ * Coerce an identifier value to the {@code storedAs} type declared in the
entity's id
+ * mapping, so that point-lookup queries target the BSON type actually on
disk.
+ * See {@code IdentityMapping#getStoredAs()}.
+ */
+ protected Object coerceIdToStoredType(Object key, PersistentEntity entity)
{
+ if (key == null) return key
+ Class<?> storedAs = null
+ try {
+ storedAs = entity?.mapping?.identifier?.storedAs
+ } catch (Throwable ignored) {
+ return key
+ }
+ if (storedAs == null || storedAs.isInstance(key)) return key
+ try {
+ def converted = mappingContext.conversionService.convert(key,
storedAs)
+ // Symmetry with IdentityEncoder's non-hex fallback: a null return
(vs a throw)
+ // means the converter rejected the value — e.g. a natural-key
String being
+ // converted to ObjectId. Keep the original so the query matches
the document
+ // the encoder actually wrote.
+ return converted != null ? converted : key
+ } catch (Throwable ignored) {
+ return key
+ }
+ }
Review Comment:
`coerceIdToStoredType` converts to any non-null `storedAs`. Since the
encoder currently only honors storedAs for String/ObjectId, using another
storedAs type that Spring can convert to (e.g. UUID) will make `retrieveEntity`
/ `retrieveAllEntities` query using a BSON type that was never written,
guaranteeing misses. Either limit coercion to supported storedAs types
(String/ObjectId) or update `IdentityEncoder` to encode ids using the same
ConversionService for arbitrary storedAs so persistence and retrieval are
symmetric.
##########
grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc:
##########
@@ -67,4 +67,64 @@ p.id = "Fred"
p.insert()
// to update
p.save()
-----
\ No newline at end of file
+----
+
+
+==== Decoupling the Declared Type from the Storage Type
+
+When a domain declares `String id`, GORM by default writes `_id` as a BSON
`String`. This is convenient in application code — HTTP controllers return
clean JSON, URLs embed ids directly, and you never need `new ObjectId(...)` at
call sites — but it gives up the native `ObjectId` benefits at the storage
layer (12-byte index entries vs. 24-byte hex strings, embedded creation
timestamp, etc.).
+
+You can get both using the `storedAs` mapping option. The domain sees a
`String id` (hex form), but `_id` on disk is a BSON `ObjectId`:
+
+[source,groovy]
+----
+import org.bson.types.ObjectId
+
+class Person {
+ String id
+
+ static mapping = {
+ id storedAs: ObjectId
+ }
+}
+----
+
+With `storedAs: ObjectId`:
+
+* `_id` is written as BSON `ObjectId` (native index size, sort order, creation
timestamp)
+* `person.id` in application code is the 24-char hex string (clean JSON, no
conversion at boundaries)
+* `Person.get(hexString)`, `findAllByIdInList([hexA, hexB])`, updates, and
deletes all coerce the declared-type value to the storage type automatically
+* Existing documents that already have BSON `ObjectId` `_id` values continue
to load without a data migration
+
+This is especially useful when migrating a legacy `ObjectId id` domain to
`String id` for ergonomic reasons, because neither a data migration nor any
change in application call sites is required.
+
+===== Global Default
+
+You can opt in globally with an `application.yml` setting, which applies to
every domain declaring `String id` that does not specify its own `storedAs`:
+
+[source,yaml]
+----
+grails:
+ mongodb:
+ stringIdsDefaultStoredAs: objectid
+----
+
+Valid values are `string` (current default, BSON String) and `objectid` (BSON
ObjectId). Unrecognized values fall back to the default with a warning logged
at startup.
+
+Per-domain `storedAs` always wins over the global default. Domains using a
natural string key (slug, email, UUID) should opt out explicitly:
+
+[source,groovy]
+----
+class UserProfile {
+ String id // e.g. "[email protected]"
+
+ static mapping = {
+ id generator: 'assigned', storedAs: String
+ }
+}
+----
+
+===== Caveats
+
+* `storedAs` is currently honored only for converting between `String` and
`ObjectId`. Other combinations are accepted but behave as if `storedAs` were
unset.
Review Comment:
This caveat says non-String/ObjectId `storedAs` values "behave as if
`storedAs` were unset", but the current implementation coerces ids in
query/update/delete paths for any storedAs type that Spring can convert to.
This can create surprising behavior (and even mismatches with
`IdentityEncoder`) for types like UUID. Either tighten the implementation to
only honor String↔ObjectId (so this doc stays true) or update the documentation
to describe exactly what combinations are supported and how mismatches are
handled.
```suggestion
* `storedAs` is primarily intended for converting between `String` and
`ObjectId`. Other `storedAs` values are not treated uniformly as "unset": if
Spring can convert the id type, query/update/delete paths may still coerce
values to the declared `storedAs` type. This can differ from identity encoding
and persistence behavior, so non-`String`/`ObjectId` combinations (for example
`UUID`) should be avoided unless you have verified the full read/write/query
lifecycle for your mapping.
```
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java:
##########
@@ -138,11 +138,78 @@ public void handle(EmbeddedQueryEncoder queryEncoder,
IdEquals criterion, Docume
Object value = criterion.getValue();
MappingContext mappingContext = entity.getMappingContext();
PersistentProperty identity = entity.getIdentity();
- Object converted =
mappingContext.getConversionService().convert(value, identity.getType());
+ // Prefer the configured storage type ('storedAs' on the id
mapping) so query BSON
+ // matches what's actually on disk. Falls back to the declared
Java type otherwise.
+ Class<?> targetType = null;
+ try {
+ if (entity.getMapping() != null &&
entity.getMapping().getIdentifier() != null) {
+ targetType =
entity.getMapping().getIdentifier().getStoredAs();
+ }
+ } catch (Throwable ignored) {
+ // defensive: some mapping implementations may not expose
storedAs yet
+ }
+ if (targetType == null) {
+ targetType = identity.getType();
+ }
+ Object converted =
mappingContext.getConversionService().convert(value, targetType);
+ // Symmetry with IdentityEncoder's non-hex fallback: if the
converter returns
+ // null for a non-null input (e.g. a natural-key String being
converted to
+ // ObjectId), keep the original value so the query targets
what the encoder
+ // actually wrote rather than {_id: null}.
+ if (converted == null && value != null) {
+ converted = value;
+ }
query.put(MongoEntityPersister.MONGO_ID_FIELD, converted);
}
});
+ // Override the In handler so that criteria targeting the identity
(findAllByIdInList,
+ // Domain.createCriteria().list { 'in'('id', [...]) }, etc.) honor
'storedAs' the same way
+ // IdEquals does. Without this, a domain declaring storedAs: ObjectId
would send BSON Strings
+ // in {_id: {$in: [...]}} and miss all stored ObjectId documents.
+ queryHandlers.put(In.class, new QueryHandler<In>() {
+ public void handle(EmbeddedQueryEncoder queryEncoder, In in,
Document query, PersistentEntity entity) {
+ Document inQuery = new Document();
+ List<Object> values = getInListQueryValues(entity, in);
+
+ Class<?> storedAs = null;
+ try {
+ PersistentProperty identityProp = entity.getIdentity();
+ if (identityProp != null &&
identityProp.getName().equals(in.getProperty()) &&
+ entity.getMapping() != null &&
entity.getMapping().getIdentifier() != null) {
+ storedAs =
entity.getMapping().getIdentifier().getStoredAs();
+ }
+ } catch (Throwable ignored) {
+ // defensive: mapping implementations without storedAs
support fall through
+ }
+ if (storedAs != null) {
+ MappingContext mappingContext = entity.getMappingContext();
+ List<Object> coerced = new ArrayList<>(values.size());
+ for (Object v : values) {
+ if (v == null || storedAs.isInstance(v)) {
+ coerced.add(v);
+ } else {
+ Object c;
+ try {
+ c =
mappingContext.getConversionService().convert(v, storedAs);
+ } catch (Throwable ignored) {
+ c = v;
+ }
+ // A null return (vs a throw) means the converter
rejected the value —
+ // e.g. a natural-key String being converted to
ObjectId. Fall back to
+ // the original so the $in list matches what the
encoder actually wrote.
+ coerced.add(c != null ? c : v);
+ }
+ }
+ values = coerced;
+ }
Review Comment:
The overridden `In` handler applies `storedAs` coercion for any non-null
storedAs. This can desynchronize query behavior from how ids are actually
encoded (IdentityEncoder only handles String/ObjectId storedAs), leading to
`$in` queries that can never match when storedAs is set to another convertible
type (e.g. UUID). Recommend gating coercion to supported storedAs values
(String/ObjectId) or making encoding use the same conversion logic so the
stored BSON type and query BSON type are always aligned.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:
##########
@@ -286,6 +286,36 @@ class MongoCodecSession extends AbstractMongoSession {
return entityWrites
}
+ /**
+ * If the entity's id mapping declares {@code storedAs} and it differs
from the in-memory
+ * native key type, coerce the key so that update/delete filters target
BSON values of
+ * the correct type (otherwise {@code {_id: "<hex>"}} sent as a BSON
String would never
+ * match an {@code _id: ObjectId(...)} document on disk, and the write
would silently miss,
+ * surfacing as a misleading {@link OptimisticLockingException}).
+ */
+ protected Object coerceIdToStoredType(Object nativeKey, PersistentEntity
entity) {
+ if (nativeKey == null) return null
+ Class<?> storedAs = null
+ try {
+ storedAs = entity?.mapping?.identifier?.storedAs
+ } catch (Throwable ignored) {
+ return nativeKey
+ }
+ if (storedAs == null) return nativeKey
+ if (storedAs.isInstance(nativeKey)) return nativeKey
+ try {
+ def converted =
mappingContext.conversionService.convert(nativeKey, storedAs)
+ // Symmetry with IdentityEncoder's non-hex fallback: a null return
(vs a throw)
+ // means the converter rejected the value — e.g. a natural-key
String being
+ // converted to ObjectId. Keep the original so update/delete
filters match the
+ // document the encoder actually wrote (BSON String), rather than
targeting
+ // {_id: null} and surfacing as a misleading
OptimisticLockingException.
+ return converted != null ? converted : nativeKey
+ } catch (Throwable ignored) {
+ return nativeKey
+ }
+ }
Review Comment:
`coerceIdToStoredType` converts to whatever
`entity.mapping.identifier.storedAs` is. Because `IdentityEncoder` only changes
the BSON encoding for storedAs=String/ObjectId, setting storedAs to another
type that Spring can convert to (e.g. UUID) will cause update/delete filters to
target a different BSON type than what was written, resulting in silent misses
and potential `OptimisticLockingException`s. Consider restricting coercion to
the same supported storedAs types as the encoder, or enhancing the encoder to
perform the same conversion so writes and filters stay consistent.
##########
grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy:
##########
@@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.mongo.bugs
+
+import grails.persistence.Entity
+import org.bson.types.ObjectId
+import org.grails.datastore.mapping.core.DatastoreUtils
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.datastore.mapping.mongo.config.MongoMappingContext
+import org.grails.datastore.mapping.mongo.config.MongoSettings
+import
org.grails.datastore.mapping.mongo.connections.MongoConnectionSourceSettingsBuilder
+import spock.lang.Specification
+
+/**
+ * Exercises the global config {@code
grails.mongodb.stringIds.defaultStoredAs}.
+ *
+ * Uses {@link MongoMappingContext} directly (no live MongoDB) — this spec is
purely about
+ * how the mapping layer resolves {@code storedAs} from config. The plumbing
into the
+ * encoder/decoder/query paths is exercised by {@link
StringIdWithObjectIdStorageSpec}.
+ */
Review Comment:
The config key referenced here (`grails.mongodb.stringIds.defaultStoredAs`)
doesn't match the actual setting introduced in this PR
(`grails.mongodb.stringIdsDefaultStoredAs`, per
`MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS`). This is likely to
confuse future readers since it appears in both this class-level doc and later
test names.
##########
grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy:
##########
@@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.mongo.bugs
+
+import grails.persistence.Entity
+import org.bson.types.ObjectId
+import org.grails.datastore.mapping.core.DatastoreUtils
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.datastore.mapping.mongo.config.MongoMappingContext
+import org.grails.datastore.mapping.mongo.config.MongoSettings
+import
org.grails.datastore.mapping.mongo.connections.MongoConnectionSourceSettingsBuilder
+import spock.lang.Specification
+
+/**
+ * Exercises the global config {@code
grails.mongodb.stringIds.defaultStoredAs}.
+ *
+ * Uses {@link MongoMappingContext} directly (no live MongoDB) — this spec is
purely about
+ * how the mapping layer resolves {@code storedAs} from config. The plumbing
into the
+ * encoder/decoder/query paths is exercised by {@link
StringIdWithObjectIdStorageSpec}.
+ */
+class StringIdDefaultStoredAsConfigSpec extends Specification {
+
+ private MongoMappingContext contextFor(Map<String, ?> props, Class...
domainClasses) {
+ def settings = new
MongoConnectionSourceSettingsBuilder(DatastoreUtils.createPropertyResolver(props)).build()
+ new MongoMappingContext(settings, domainClasses)
+ }
+
+ void "without the global config, String id has no storedAs (default
behavior unchanged)"() {
+ given:
+ MongoMappingContext ctx = contextFor([:], PlainStringIdDomain)
+
+ when:
+ PersistentEntity entity =
ctx.getPersistentEntity(PlainStringIdDomain.name)
+
+ then:
+ entity != null
+ entity.mapping.identifier.storedAs == null
+ }
+
+ void "with grails.mongodb.stringIds.defaultStoredAs=objectid, String id
picks up ObjectId storedAs"() {
+ given:
+ MongoMappingContext ctx = contextFor(
+ [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS):
'objectid'],
+ PlainStringIdDomain
+ )
+
Review Comment:
This test name string also refers to
`grails.mongodb.stringIds.defaultStoredAs`, but the implemented setting is
`grails.mongodb.stringIdsDefaultStoredAs`. Aligning the description with the
real key will make failures easier to interpret.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java:
##########
@@ -138,11 +138,78 @@ public void handle(EmbeddedQueryEncoder queryEncoder,
IdEquals criterion, Docume
Object value = criterion.getValue();
MappingContext mappingContext = entity.getMappingContext();
PersistentProperty identity = entity.getIdentity();
- Object converted =
mappingContext.getConversionService().convert(value, identity.getType());
+ // Prefer the configured storage type ('storedAs' on the id
mapping) so query BSON
+ // matches what's actually on disk. Falls back to the declared
Java type otherwise.
+ Class<?> targetType = null;
+ try {
+ if (entity.getMapping() != null &&
entity.getMapping().getIdentifier() != null) {
+ targetType =
entity.getMapping().getIdentifier().getStoredAs();
+ }
+ } catch (Throwable ignored) {
+ // defensive: some mapping implementations may not expose
storedAs yet
+ }
+ if (targetType == null) {
+ targetType = identity.getType();
+ }
+ Object converted =
mappingContext.getConversionService().convert(value, targetType);
+ // Symmetry with IdentityEncoder's non-hex fallback: if the
converter returns
+ // null for a non-null input (e.g. a natural-key String being
converted to
+ // ObjectId), keep the original value so the query targets
what the encoder
+ // actually wrote rather than {_id: null}.
+ if (converted == null && value != null) {
+ converted = value;
+ }
Review Comment:
The IdEquals handler coerces to `identifier.storedAs` for any non-null
storedAs. Since `IdentityEncoder` only actively encodes `storedAs` for
String/ObjectId, a user setting `storedAs` to another type that Spring can
convert to (e.g. UUID) will make point lookups query using that BSON type while
writes still store a String, causing guaranteed misses. Consider restricting
this coercion to the same supported storedAs types as the encoder
(String/ObjectId), or updating the encoder to use the same ConversionService
for arbitrary storedAs so read/write/query remain symmetric.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy:
##########
@@ -95,4 +95,18 @@ interface MongoSettings extends Settings {
String SETTING_ENGINE = 'grails.mongodb.engine'
+ /**
+ * Global default storage type for {@code String id} fields when no
per-domain
+ * {@code id storedAs: ...} mapping is declared. Accepted values are the
names (or hex
+ * aliases) {@code 'string'} (default, current behavior) and {@code
'objectid'}.
Review Comment:
Javadoc mentions "hex aliases" for the accepted values, but the only
non-canonical alias currently handled is `object_id` (underscore), not a hex
alias. Suggest rewording to avoid confusion (and to align with the docs/log
message).
```suggestion
* {@code id storedAs: ...} mapping is declared. Accepted values are the
names (or
* aliases) {@code 'string'} (default, current behavior), {@code
'objectid'}, and
* {@code 'object_id'}.
```
--
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]
