diff --git a/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/IdentityEncoder.groovy b/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/IdentityEncoder.groovy index 48b86eb44f4..b7f1aaf9d1d 100644 --- a/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/IdentityEncoder.groovy +++ b/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/IdentityEncoder.groovy @@ -19,12 +19,16 @@ package org.grails.datastore.bson.codecs.encoders +import java.util.concurrent.ConcurrentHashMap + import groovy.transform.CompileStatic import org.bson.BsonWriter import org.bson.codecs.EncoderContext import org.bson.codecs.configuration.CodecRegistry import org.bson.types.ObjectId +import org.slf4j.Logger +import org.slf4j.LoggerFactory import org.grails.datastore.bson.codecs.PropertyEncoder import org.grails.datastore.mapping.engine.EntityAccess @@ -37,10 +41,36 @@ import org.grails.datastore.mapping.model.types.Identity @CompileStatic class IdentityEncoder implements PropertyEncoder { + private static final Logger log = LoggerFactory.getLogger(IdentityEncoder) + + // One warn per (owning entity class) so a misconfigured 'storedAs: ObjectId' on + // natural-key data is debuggable in the field without flooding logs at write rate. + private static final Set warnedNonHexEntities = ConcurrentHashMap.newKeySet() + @Override void encode(BsonWriter writer, Identity property, Object id, EntityAccess parentAccess, EncoderContext encoderContext, CodecRegistry codecRegistry) { writer.writeName(getIdentifierName(property)) + Class storedAs = resolveStoredAs(property) + if (storedAs != null && id != null) { + if (ObjectId.isAssignableFrom(storedAs) && !(id instanceof ObjectId)) { + String hex = id.toString() + // Guard against natural-key strings accidentally paired with storedAs: ObjectId. + // new ObjectId() throws IllegalArgumentException, which would surface + // deep inside the BSON write pipeline. Fall through to writeString for consistency + // with the converter-based paths (MongoCodecSession, MongoCodecEntityPersister). + if (ObjectId.isValid(hex)) { + writer.writeObjectId(new ObjectId(hex)) + return + } + warnNonHexFallback(property, hex) + } + if (String.isAssignableFrom(storedAs) && !(id instanceof String)) { + writer.writeString(id.toString()) + return + } + } + if (id instanceof ObjectId) { writer.writeObjectId(id) } else if (id instanceof Number) { @@ -51,6 +81,24 @@ class IdentityEncoder implements PropertyEncoder { } + private static Class resolveStoredAs(Identity property) { + try { + return property?.owner?.mapping?.identifier?.storedAs + } catch (Exception ignored) { + return null + } + } + + private static void warnNonHexFallback(Identity property, String hex) { + if (!log.warnEnabled) return + String entityName = property?.owner?.javaClass?.name ?: 'unknown' + if (warnedNonHexEntities.add(entityName)) { + log.warn( + "Identity for {} is mapped storedAs: ObjectId but value '{}' is not a valid 24-char hex ObjectId; persisting as BSON String. Use storedAs: String for natural-key domains. (warned once per entity class)", + entityName, hex) + } + } + protected String getIdentifierName(Identity property) { String[] identifierName = property.getOwner().mapping.identifier?.identifierName if (identifierName != null) { diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy index be03e0adb59..29ae2d06635 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy @@ -62,6 +62,7 @@ import org.grails.datastore.mapping.model.types.Association import org.grails.datastore.mapping.model.types.ToOne import org.grails.datastore.mapping.mongo.engine.MongoCodecEntityPersister import org.grails.datastore.mapping.mongo.engine.MongoEntityPersister +import org.grails.datastore.mapping.mongo.engine.MongoIdCoercion import org.grails.datastore.mapping.mongo.engine.codecs.PersistentEntityCodec import org.grails.datastore.mapping.mongo.query.MongoQuery import org.grails.datastore.mapping.query.Query @@ -152,7 +153,7 @@ class MongoCodecSession extends AbstractMongoSession { DirtyCheckable changedObject = (DirtyCheckable) update.getNativeEntry() PersistentEntityCodec codec = (PersistentEntityCodec) datastore.codecRegistry.get(changedObject.getClass()) - final Object nativeKey = update.nativeKey + final Object nativeKey = coerceIdToStoredType(update.nativeKey, persistentEntity) final Document id = new Document(MongoEntityPersister.MONGO_ID_FIELD, nativeKey) EntityAccess entityAccess = update.entityAccess @@ -200,7 +201,7 @@ class MongoCodecSession extends AbstractMongoSession { if (delete.vetoed) continue - final Object k = delete.nativeKey + final Object k = coerceIdToStoredType(delete.nativeKey, persistentEntity) if (k) { nativeKeys << k final List cascadeOperations = delete.cascadeOperations @@ -286,6 +287,25 @@ 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: ""}} 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}). + * + *

Exercised end-to-end by {@code StringIdWithObjectIdStorageSpec}: + *

    + *
  • "with storedAs ObjectId, updates persist (no phantom OptimisticLockingException)" — happy path on update filter
    • + *
  • "with storedAs ObjectId, update of a non-hex id document lands on the right row" — null-return fallback on update filter
    • + *
  • "with storedAs ObjectId, delete of a non-hex id document removes the row" — null-return fallback on delete filter
    • + *
  • "with storedAs ObjectId, legacy documents written directly as BSON ObjectId are fully accessible" — update path against legacy BSON ObjectId _id
    • + *
+ */ + protected Object coerceIdToStoredType(Object nativeKey, PersistentEntity entity) { + MongoIdCoercion.coerceIdToStoredType(nativeKey, entity) + } + @Override protected Transaction beginTransactionInternal() { return new SessionOnlyTransaction(getNativeInterface(), this) diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java index b2f82582a05..d37f38de3c1 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java @@ -46,6 +46,8 @@ import org.bson.types.Decimal128; import org.bson.types.ObjectId; import org.bson.types.Symbol; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.ConverterRegistry; @@ -101,6 +103,8 @@ */ @SuppressWarnings("rawtypes") public class MongoMappingContext extends DocumentMappingContext { + + private static final Logger log = LoggerFactory.getLogger(MongoMappingContext.class); private static final String DECIMAL_TYPE_CLASS_NAME = "org.bson.types.Decimal128"; /** * Java types supported as mongo property types. @@ -134,6 +138,21 @@ public class MongoMappingContext extends DocumentMappingContext { private CodecRegistry codecRegistry; private Map hasCodecCache = new HashMap<>(); + /** + * Global default storage type for {@code String id} fields that don't declare an explicit + * {@code id storedAs: ...} in their mapping. Null means "no default — use the declared + * Java type" (current GORM behavior). See {@link MongoSettings#SETTING_STRING_IDS_DEFAULT_STORED_AS}. + */ + private Class stringIdDefaultStoredAs; + + public Class getStringIdDefaultStoredAs() { + return stringIdDefaultStoredAs; + } + + public void setStringIdDefaultStoredAs(Class stringIdDefaultStoredAs) { + this.stringIdDefaultStoredAs = stringIdDefaultStoredAs; + } + public MongoMappingContext(String defaultDatabaseName) { this(defaultDatabaseName, null); } @@ -165,7 +184,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'. " + + "Falling back to default behavior (no coercion).", + value, MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS); + return null; + } } /** @@ -176,6 +216,10 @@ public MongoMappingContext(PropertyResolver configuration, Class... classes) { */ public MongoMappingContext(AbstractMongoConnectionSourceSettings settings, Class... classes) { super(settings.getDatabase(), settings); + // Must run BEFORE initialize(classes) so that MongoDocumentMappingFactory.createIdentity + // (invoked during entity registration) can read the global default. + String storedAsDefault = settings.getStringIds() != null ? settings.getStringIds().getDefaultStoredAs() : null; + this.stringIdDefaultStoredAs = parseStoredAs(storedAsDefault); initialize(classes); } @@ -333,7 +377,14 @@ protected Class getEntityMappedFormType() { @Override public Identity createIdentity(PersistentEntity owner, MappingContext context, PropertyDescriptor pd) { Identity identity = super.createIdentity(owner, context, pd); - identity.getMapping().getMappedForm().setTargetName(MongoConstants.MONGO_ID_FIELD); + MongoAttribute mappedForm = identity.getMapping().getMappedForm(); + mappedForm.setTargetName(MongoConstants.MONGO_ID_FIELD); + // Apply the global default storedAs for String-id domains that don't declare their own. + if (mappedForm.getStoredAs() == null && + stringIdDefaultStoredAs != null && + String.class.equals(pd.getPropertyType())) { + mappedForm.setStoredAs(stringIdDefaultStoredAs); + } return identity; } diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy index 39684d3e9d1..4a2bbf15fbd 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy +++ b/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'}. + * + *

When set to {@code 'objectid'}, every domain that declares {@code String id} + * without an explicit {@code storedAs} will persist {@code _id} as a BSON ObjectId, + * while keeping the {@code String} ergonomics in application code. Domains that use + * natural string keys (slug, email, UUID) should opt out per-domain via + * {@code static mapping = { id storedAs: String }}. + * + * @since 7.1.1 + */ + String SETTING_STRING_IDS_DEFAULT_STORED_AS = 'grails.mongodb.stringIds.defaultStoredAs' } diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/AbstractMongoConnectionSourceSettings.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/AbstractMongoConnectionSourceSettings.groovy index 1ec305b2d56..025f4be9250 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/AbstractMongoConnectionSourceSettings.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/AbstractMongoConnectionSourceSettings.groovy @@ -87,6 +87,17 @@ abstract class AbstractMongoConnectionSourceSettings extends ConnectionSourceSet */ boolean decimalType = true + /** + * Nested settings for domains with {@code String id}. Holds the global default + * {@code defaultStoredAs} switch plus any future string-id configuration. + * + *

Exposes the property path {@code grails.mongodb.stringIds.defaultStoredAs}, + * aligned with the Spring-style hierarchical namespace convention. + * + * @since 7.1.1 + */ + StringIdSettings stringIds = new StringIdSettings() + /** * The collection name to use to resolve connections when using {@link MongoConnectionSources} */ diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/StringIdSettings.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/StringIdSettings.groovy new file mode 100644 index 00000000000..6a9ac5280b3 --- /dev/null +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/StringIdSettings.groovy @@ -0,0 +1,47 @@ +/* + * 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.mapping.mongo.connections + +import groovy.transform.CompileStatic +import groovy.transform.builder.Builder +import groovy.transform.builder.SimpleStrategy + +/** + * Settings grouped under the {@code grails.mongodb.stringIds} namespace. The + * {@code @Builder(SimpleStrategy)} annotation lets {@code ConfigurationBuilder} + * recurse into this class so its fields bind at the nested property path. + * + * @since 7.1.1 + */ +@Builder(builderStrategy = SimpleStrategy, prefix = '') +@CompileStatic +class StringIdSettings { + + /** + * Global default storage type for {@code String id} fields that don't declare an + * explicit {@code id storedAs: ...} in their mapping. Accepted values are + * {@code 'string'} (default, current behavior) or {@code 'objectid'}. + * + *

Corresponds to the property path + * {@code grails.mongodb.stringIds.defaultStoredAs}. + */ + String defaultStoredAs + +} diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoCodecEntityPersister.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoCodecEntityPersister.groovy index 6de40c221a3..7178b6b0029 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoCodecEntityPersister.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoCodecEntityPersister.groovy @@ -128,8 +128,11 @@ class MongoCodecEntityPersister extends ThirdPartyCacheEntityPersister { // don't bother with query if list of keys is empty return [] } else { + // Coerce each key to the storage type so {_id: {$in: [...]}} uses BSON values + // that actually match on disk when 'storedAs' differs from the declared type. + def coercedIds = idList.collect { coerceIdToStoredType(it, pe) } createQuery() - .in(pe.identity.name, idList) + .in(pe.identity.name, coercedIds) .list() } @@ -153,7 +156,7 @@ class MongoCodecEntityPersister extends ThirdPartyCacheEntityPersister { return null } else { MongoCollection mongoCollection = getMongoCollection(pe) - Document idQuery = createIdQuery(key) + Document idQuery = createIdQuery(coerceIdToStoredType(key, pe)) o = mongoCollection .withDocumentClass(persistentEntity.javaClass) .withCodecRegistry(mongoDatastore.codecRegistry) @@ -175,6 +178,24 @@ class MongoCodecEntityPersister extends ThirdPartyCacheEntityPersister { 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()}. + * + *

Exercised end-to-end (via {@code retrieveEntity}/{@code retrieveAllEntities}) by + * {@code StringIdWithObjectIdStorageSpec}, specifically: + *

    + *
  • "with storedAs ObjectId, point lookup by hex string works" — happy path, String → ObjectId
    • + *
  • "with storedAs ObjectId, batch getAll resolves all ids (coerces each key in the in-list filter)"
    • + *
  • "with storedAs ObjectId, point lookup of a non-hex id matches the BSON String the encoder wrote" — null-return fallback
    • + *
  • "with storedAs ObjectId, legacy documents written directly as BSON ObjectId are fully accessible"
    • + *
+ */ + protected Object coerceIdToStoredType(Object key, PersistentEntity entity) { + MongoIdCoercion.coerceIdToStoredType(key, entity) + } + @Override protected Serializable persistEntity(PersistentEntity entity, Object obj, boolean isInsert) { diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoIdCoercion.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoIdCoercion.java new file mode 100644 index 00000000000..9fd867999fd --- /dev/null +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoIdCoercion.java @@ -0,0 +1,93 @@ +/* + * 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.mapping.mongo.engine; + +import org.springframework.core.convert.ConversionService; + +import org.grails.datastore.mapping.model.ClassMapping; +import org.grails.datastore.mapping.model.IdentityMapping; +import org.grails.datastore.mapping.model.PersistentEntity; + +/** + * Centralizes coercion of an identifier value to the {@code storedAs} type declared + * on a {@link PersistentEntity}'s id mapping. + * + *

Used at every boundary where in-memory ids cross into BSON: point lookups + * ({@code MongoCodecEntityPersister#retrieveEntity}), batch lookups + * ({@code MongoCodecEntityPersister#retrieveAllEntities}), update/delete filters + * ({@code MongoCodecSession}), and query handlers ({@code MongoQuery}'s + * {@code IdEquals} and {@code In}). Keeping the rules in one place prevents the + * four call sites from diverging — the null-return fallback for converter + * rejection (e.g. non-hex String → ObjectId) is the kind of subtlety that breaks + * silently when duplicated. + * + * @since 7.1.1 + */ +public final class MongoIdCoercion { + + private MongoIdCoercion() { + } + + /** + * Read the {@code storedAs} class from the entity's id mapping, or {@code null} + * if the mapping doesn't declare one (or the mapping implementation predates + * {@link IdentityMapping#getStoredAs()}). + */ + public static Class resolveStoredAs(PersistentEntity entity) { + if (entity == null) return null; + try { + ClassMapping mapping = entity.getMapping(); + if (mapping == null) return null; + IdentityMapping identifier = mapping.getIdentifier(); + return identifier != null ? identifier.getStoredAs() : null; + } + catch (Exception ignored) { + return null; + } + } + + /** + * Coerce {@code key} to the entity's {@code storedAs} type so query/update/delete + * filters target BSON values that match what the encoder actually wrote on disk. + * + *

Returns the original {@code key} when: + *

    + *
  • {@code key} is {@code null}, or already an instance of the storedAs type,
    • + *
  • the entity declares no {@code storedAs},
    • + *
  • the converter rejects the value by returning {@code null} (e.g. a non-hex + * natural-key String against {@code storedAs: ObjectId}) — kept symmetric with + * {@code IdentityEncoder}'s non-hex fallback so filters target the BSON String + * the encoder wrote rather than {@code {_id: null}},
    • + *
  • the converter throws.
    • + *
+ */ + public static Object coerceIdToStoredType(Object key, PersistentEntity entity) { + if (key == null) return null; + Class storedAs = resolveStoredAs(entity); + if (storedAs == null || storedAs.isInstance(key)) return key; + try { + ConversionService cs = entity.getMappingContext().getConversionService(); + Object converted = cs.convert(key, storedAs); + return converted != null ? converted : key; + } + catch (Exception ignored) { + return key; + } + } +} diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy index 24fbd6a9bcd..df651ad7519 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy @@ -361,7 +361,40 @@ class PersistentEntityCodec extends BsonPersistentEntityCodec { } } else { - // TODO: Support non-dirty checkable objects? + // Non-DirtyCheckable values: no per-property change history available, + // so when the caller is encoding this as an embedded update (null→non-null + // transition on a single-valued embedded field), encode every persistent + // property. Without this, the parent's $set on the embedded path stays + // empty and the sub-document is silently dropped. + if (embedded) { + def sets = new BsonDocument() + BsonWriter writer = new BsonDocumentWriter(sets) + writer.writeStartDocument() + if (!entity.isRoot()) { + sets.put(MongoConstants.MONGO_CLASS_FIELD, new BsonString(entity.discriminator)) + } + for (propertyName in entity.persistentPropertyNames) { + def prop = entity.getPropertyByName(propertyName) + if (prop == null) continue + Object v = access.getProperty(prop.name) + if (v == null) continue + if (prop instanceof Embedded) { + encodeEmbeddedUpdate(sets, new Document(), (Association) prop, v) + } + else if (prop instanceof EmbeddedCollection) { + encodeEmbeddedCollectionUpdate(access, sets, new Document(), (Association) prop, v) + } + else { + def propKind = prop.getClass().superclass + PropertyEncoder propertyEncoder = getPropertyEncoder((Class) propKind) + propertyEncoder?.encode(writer, prop, v, access, encoderContext, codecRegistry) + } + } + writer.writeEndDocument() + if (!sets.isEmpty()) { + update.put(MONGO_SET_OPERATOR, sets) + } + } } return update diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java index e0e8eebf94a..75522c20284 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java @@ -62,7 +62,6 @@ import org.grails.datastore.mapping.engine.EntityPersister; import org.grails.datastore.mapping.engine.types.CustomTypeMarshaller; import org.grails.datastore.mapping.model.EmbeddedPersistentEntity; -import org.grails.datastore.mapping.model.MappingContext; import org.grails.datastore.mapping.model.PersistentEntity; import org.grails.datastore.mapping.model.PersistentProperty; import org.grails.datastore.mapping.model.types.Association; @@ -77,6 +76,7 @@ import org.grails.datastore.mapping.mongo.config.MongoCollection; import org.grails.datastore.mapping.mongo.engine.MongoCodecEntityPersister; import org.grails.datastore.mapping.mongo.engine.MongoEntityPersister; +import org.grails.datastore.mapping.mongo.engine.MongoIdCoercion; import org.grails.datastore.mapping.mongo.engine.codecs.PersistentEntityCodec; import org.grails.datastore.mapping.query.AssociationQuery; import org.grails.datastore.mapping.query.Query; @@ -134,15 +134,61 @@ public class MongoQuery extends BsonQuery implements QueryArgumentsAware { static { queryHandlers.put(IdEquals.class, new QueryHandler() { + // Exercised end-to-end by StringIdWithObjectIdStorageSpec: + // - "with storedAs ObjectId, point lookup by hex string works" (happy path) + // - "with storedAs ObjectId, point lookup of a non-hex id matches the + // BSON String the encoder wrote" (null-return fallback for natural keys) + // - "with storedAs ObjectId, updates persist (no phantom OptimisticLockingException)" + // and related update/delete specs (which also hit IdEquals via the session filter) public void handle(EmbeddedQueryEncoder queryEncoder, IdEquals criterion, Document query, PersistentEntity entity) { 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 storedAs = MongoIdCoercion.resolveStoredAs(entity); + Class targetType = storedAs != null ? storedAs : entity.getIdentity().getType(); + Object converted = entity.getMappingContext().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. + // + // Exercised end-to-end by StringIdWithObjectIdStorageSpec: + // - "with storedAs ObjectId, findAllByIdInList resolves all ids" (happy path via dynamic finder) + // - "with storedAs ObjectId, criteria in('id', [...]) resolves all ids" (happy path via createCriteria) + // - "with storedAs ObjectId, batch getAll with non-hex ids falls back to BSON String in the in-list" + // (null-return fallback for natural keys) + queryHandlers.put(In.class, new QueryHandler() { + public void handle(EmbeddedQueryEncoder queryEncoder, In in, Document query, PersistentEntity entity) { + Document inQuery = new Document(); + List values = getInListQueryValues(entity, in); + + PersistentProperty identityProp = entity.getIdentity(); + boolean isIdInList = identityProp != null && identityProp.getName().equals(in.getProperty()); + if (isIdInList && MongoIdCoercion.resolveStoredAs(entity) != null) { + List coerced = new ArrayList<>(values.size()); + for (Object v : values) { + coerced.add(MongoIdCoercion.coerceIdToStoredType(v, entity)); + } + values = coerced; + } + + inQuery.put(IN_OPERATOR, values); + String propertyName = getPropertyName(entity, in); + query.put(propertyName, inQuery); + } + }); + queryHandlers.put(AssociationQuery.class, new QueryHandler() { public void handle(EmbeddedQueryEncoder queryEncoder, AssociationQuery criterion, Document query, PersistentEntity entity) { Association association = criterion.getAssociation(); diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/SingleEmbeddedAssignNullToNonNullSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/SingleEmbeddedAssignNullToNonNullSpec.groovy new file mode 100644 index 00000000000..51cbc240280 --- /dev/null +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/SingleEmbeddedAssignNullToNonNullSpec.groovy @@ -0,0 +1,146 @@ +/* + * 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 + +import grails.persistence.Entity +import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import org.bson.types.ObjectId + +/** + * Reproduces a bug in GORM-MongoDB where assigning a new instance to a + * previously-null single-valued embedded property does not persist on + * {@code save(flush: true)}: dirty tracking fails to flag the transition + * from {@code null} to an embedded value, so the resulting {@code $set} + * never includes the embedded sub-document. + * + * Top-level scalar fields on the same entity save fine — the bug is + * specific to single-valued embedded references going from null to non-null. + */ +class SingleEmbeddedAssignNullToNonNullSpec extends GrailsDataTckSpec { + + void setupSpec() { + manager.domainClasses.addAll([FramePogo, CropPogo, FrameEntity, CropEntity]) + } + + void "POGO embedded value: null→non-null assignment persists"() { + given: "a Frame with no embedded Crop" + FramePogo frame = new FramePogo(name: 'frame-a') + frame.save(flush: true, failOnError: true) + ObjectId id = frame.id + manager.session.clear() + + expect: "the persisted document has no crop field" + FramePogo.get(id).crop == null + + when: "we reload, set a new Crop value, and save" + FramePogo reloaded = FramePogo.get(id) + reloaded.crop = new CropPogo(status: 'CROPPED', ratio: 0.75d) + reloaded.save(flush: true, failOnError: true) + manager.session.clear() + + then: "the embedded value is now persisted" + FramePogo persisted = FramePogo.get(id) + persisted.crop != null + persisted.crop.status == 'CROPPED' + persisted.crop.ratio == 0.75d + } + + void "POGO embedded: top-level scalar save works (control)"() { + given: + FramePogo frame = new FramePogo(name: 'frame-b') + frame.save(flush: true, failOnError: true) + ObjectId id = frame.id + manager.session.clear() + + when: + FramePogo reloaded = FramePogo.get(id) + reloaded.name = 'frame-b-renamed' + reloaded.save(flush: true, failOnError: true) + manager.session.clear() + + then: + FramePogo.get(id).name == 'frame-b-renamed' + } + + void "@Entity embedded value: null→non-null assignment persists"() { + given: + FrameEntity frame = new FrameEntity(name: 'frame-c') + frame.save(flush: true, failOnError: true) + ObjectId id = frame.id + manager.session.clear() + + expect: + FrameEntity.get(id).crop == null + + when: + FrameEntity reloaded = FrameEntity.get(id) + reloaded.crop = new CropEntity(status: 'CROPPED', ratio: 0.75d) + reloaded.save(flush: true, failOnError: true) + manager.session.clear() + + then: + FrameEntity persisted = FrameEntity.get(id) + persisted.crop != null + persisted.crop.status == 'CROPPED' + persisted.crop.ratio == 0.75d + } +} + +@Entity +class FramePogo { + static mapWith = 'mongo' + static embedded = ['crop'] + static constraints = { + crop nullable: true + } + + ObjectId id + String name + CropPogo crop +} + +class CropPogo { + String status + Double ratio +} + +@Entity +class FrameEntity { + static mapWith = 'mongo' + static embedded = ['crop'] + static constraints = { + crop nullable: true + } + + ObjectId id + String name + CropEntity crop +} + +@Entity +class CropEntity { + ObjectId id + String status + Double ratio + static constraints = { + status nullable: true + ratio nullable: true + } +} diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy new file mode 100644 index 00000000000..9fb34367e88 --- /dev/null +++ b/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 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 the global default set to objectid, String id picks up ObjectId storedAs"() { + given: + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'objectid'], + PlainStringIdDomain + ) + + when: + PersistentEntity entity = ctx.getPersistentEntity(PlainStringIdDomain.name) + + then: + entity.mapping.identifier.storedAs == ObjectId + } + + void "explicit per-domain storedAs wins over the global default"() { + given: 'global says objectid, but the domain explicitly declares storedAs: String' + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'objectid'], + ExplicitStringStoredAsDomain + ) + + when: + PersistentEntity entity = ctx.getPersistentEntity(ExplicitStringStoredAsDomain.name) + + then: + entity.mapping.identifier.storedAs == String + } + + void "global default only applies to String-id domains, not ObjectId-id ones"() { + given: + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'objectid'], + ObjectIdDeclaredDomain + ) + + when: + PersistentEntity entity = ctx.getPersistentEntity(ObjectIdDeclaredDomain.name) + + then: 'domain declared ObjectId id; the global default for String-ids does not apply' + entity.mapping.identifier.storedAs == null + } + + void "unrecognized storedAs value parses as null (safe fallback, no boot failure)"() { + given: + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'banana'], + PlainStringIdDomain + ) + + when: + PersistentEntity entity = ctx.getPersistentEntity(PlainStringIdDomain.name) + + then: + entity.mapping.identifier.storedAs == null + } +} + +@Entity +class PlainStringIdDomain { + String id + String name +} + +@Entity +class ExplicitStringStoredAsDomain { + String id + String name + static mapping = { + id storedAs: String + } +} + +@Entity +class ObjectIdDeclaredDomain { + ObjectId id + String name +} diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy new file mode 100644 index 00000000000..a6cd8663d2b --- /dev/null +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy @@ -0,0 +1,432 @@ +/* + * 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 groovy.json.JsonOutput +import groovy.json.JsonSlurper +import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import org.bson.Document +import org.bson.types.ObjectId +import org.grails.datastore.mapping.core.OptimisticLockingException + +/** + * Reproduces a decoder/encoder asymmetry in GORM MongoDB when a domain class + * declares {@code String id} but the underlying {@code _id} is stored as a BSON + * {@code ObjectId} (e.g. legacy data from when the domain used {@code ObjectId id}). + * + *
    + *
  • {@code IdentityDecoder} has a fallback: if the declared type is String + * but the BSON is ObjectId, it still decodes the document (via Spring's + * {@code ConversionService}). Scan-style reads work.
    • + *
  • {@code MongoQuery.IdEquals} and {@code IdentityEncoder} do not + * have this forgiveness: point lookups send {@code {_id: "hex"}} as a + * BSON String, which does not match {@code _id: ObjectId("hex")} in + * storage; updates write BSON String and miss for the same reason.
    • + *
+ * + * The net effect is that a domain can silently "read" its legacy documents but + * silently fail to {@code get(id)}, update, or delete them. + */ +class StringIdWithObjectIdStorageSpec extends GrailsDataTckSpec { + + void setupSpec() { + manager.domainClasses.addAll([LegacyVideo, ObjectIdVideo, StoredAsVideo, AssignedNonHexVideo, VersionedStoredAsVideo]) + } + + void "scan read decodes a BSON ObjectId _id into a String-typed id field"() { + given: + ObjectId legacyId = new ObjectId() + rawCollection().insertOne(new Document('_id', legacyId).append('title', 'Legacy')) + + when: + manager.session.clear() + List all = LegacyVideo.list() + + then: 'decoder fallback coerces ObjectId -> String' + all.size() == 1 + all[0].id == legacyId.toHexString() + all[0].title == 'Legacy' + } + + void "point lookup by hex string returns null because the query sends BSON String"() { + given: + ObjectId legacyId = new ObjectId() + rawCollection().insertOne(new Document('_id', legacyId).append('title', 'Legacy')) + + when: + manager.session.clear() + LegacyVideo found = LegacyVideo.get(legacyId.toHexString()) + + then: 'MongoQuery.IdEquals builds {_id: ""} as BSON String, which does not match the ObjectId _id' + found == null + } + + void "update-through-read throws a misleading OptimisticLockingException because the update filter misses the legacy doc"() { + given: + ObjectId legacyId = new ObjectId() + rawCollection().insertOne(new Document('_id', legacyId).append('title', 'Original')) + + when: 'load via scan (works), mutate, save' + manager.session.clear() + LegacyVideo v = LegacyVideo.list().first() + v.title = 'Updated' + v.save(flush: true) + + then: '''the update targets {_id: ""} as BSON String which matches zero docs; + GORM interprets the zero-match as a concurrency conflict and throws + OptimisticLockingException, even though nothing else touched the data. + This error message is misleading — the real cause is the id-type asymmetry.''' + OptimisticLockingException ex = thrown() + ex.message.contains('updated by another user') + + and: 'the legacy ObjectId-_id document is unchanged' + manager.session.clear() + Document raw = rawCollection().find(new Document('_id', legacyId)).first() + raw.getString('title') == 'Original' + } + + void "reflective JSON serialization of an ObjectId-id domain emits the id as a nested object, not a hex string"() { + given: 'a domain with ObjectId id saved via GORM' + ObjectIdVideo v = new ObjectIdVideo(title: 'Symposium').save(flush: true, failOnError: true) + + when: '''we serialize it the way a controller would when no ObjectId marshaller + is registered — reflective bean walk over every property, which is exactly + what Grails\' default JSON converter falls back to for unknown types''' + String json = JsonOutput.toJson([id: v.id, title: v.title]) + Map parsed = new JsonSlurper().parseText(json) as Map + + then: '''the id field is an object with internal ObjectId fields (timestamp/date) + rather than the hex string — this is the root cause of client-side bugs + like data-video-id="[object Object]" when HTML datasets or URLs are built + from the parsed value''' + parsed.id instanceof Map + parsed.id.containsKey('timestamp') + !(parsed.id instanceof String) + + and: 'compare to what a String id would serialize as — a plain hex string' + LegacyVideo wouldBeFine = new LegacyVideo(title: 'OK').save(flush: true, failOnError: true) + Map parsedString = new JsonSlurper().parseText(JsonOutput.toJson([id: wouldBeFine.id, title: wouldBeFine.title])) as Map + parsedString.id instanceof String + parsedString.id.length() == 24 // hex ObjectId generated by GORM for String-id domains + } + + // ---------------------------------------------------------------------- + // Cases covering `id storedAs: ObjectId` — the feature that makes the + // three failure modes above go away without requiring a data migration. + // ---------------------------------------------------------------------- + + void "with storedAs ObjectId, a String-id domain writes BSON ObjectId to _id"() { + given: + StoredAsVideo v = new StoredAsVideo(title: 'Symposium').save(flush: true, failOnError: true) + String hex = v.id + + when: 'peek at the raw document via the driver' + Document raw = storedAsRawCollection().find(new Document('_id', new ObjectId(hex))).first() + + then: '''_id is BSON ObjectId, not BSON String. Query by ObjectId finds it; + query by String would not.''' + raw != null + raw.get('_id') instanceof ObjectId + raw.get('_id').toString() == hex + storedAsRawCollection().find(new Document('_id', hex)).first() == null + } + + void "with storedAs ObjectId, point lookup by hex string works"() { + given: + StoredAsVideo v = new StoredAsVideo(title: 'Symposium').save(flush: true, failOnError: true) + String hex = v.id + + when: + manager.session.clear() + StoredAsVideo found = StoredAsVideo.get(hex) + + then: 'MongoQuery.IdEquals converts to the storedAs type, matches BSON ObjectId' + found != null + found.id == hex + found.title == 'Symposium' + } + + void "with storedAs ObjectId, updates persist (no phantom OptimisticLockingException)"() { + given: + StoredAsVideo v = new StoredAsVideo(title: 'Original').save(flush: true, failOnError: true) + String hex = v.id + + when: + manager.session.clear() + StoredAsVideo reloaded = StoredAsVideo.get(hex) + reloaded.title = 'Updated' + reloaded.save(flush: true, failOnError: true) + manager.session.clear() + + and: + Document raw = storedAsRawCollection().find(new Document('_id', new ObjectId(hex))).first() + + then: + raw != null + raw.getString('title') == 'Updated' + } + + void "with storedAs ObjectId AND versioning on, updates persist (proves the OptimisticLockingException path is genuinely fixed)"() { + // The headline bug is a misleading OptimisticLockingException on save: when the + // update filter sends the id as the wrong BSON type, zero documents match, and a + // versioned domain interprets that as a concurrency conflict. StoredAsVideo + // declares 'version false' so it can't actually exercise the OLE check — this + // parallel domain keeps versioning on so the regression test fails (OLE thrown) + // if the storedAs id-coercion is ever broken on the update path. + given: + VersionedStoredAsVideo v = new VersionedStoredAsVideo(title: 'Original').save(flush: true, failOnError: true) + String hex = v.id + Long initialVersion = v.version + + when: + manager.session.clear() + VersionedStoredAsVideo reloaded = VersionedStoredAsVideo.get(hex) + reloaded.title = 'Updated' + reloaded.save(flush: true, failOnError: true) + manager.session.clear() + + and: + Document raw = manager.mongoClient.getDatabase('test') + .getCollection('versionedStoredAsVideo') + .find(new Document('_id', new ObjectId(hex))).first() + + then: 'no OLE was thrown, the document was updated, and version incremented' + raw != null + raw.getString('title') == 'Updated' + raw.getLong('version') == initialVersion + 1 + } + + void "with storedAs ObjectId, batch getAll resolves all ids (coerces each key in the in-list filter)"() { + given: + StoredAsVideo a = new StoredAsVideo(title: 'A').save(flush: true, failOnError: true) + StoredAsVideo b = new StoredAsVideo(title: 'B').save(flush: true, failOnError: true) + StoredAsVideo c = new StoredAsVideo(title: 'C').save(flush: true, failOnError: true) + + when: + manager.session.clear() + List found = StoredAsVideo.getAll([a.id, b.id, c.id]) + + then: 'regression test for in-list handler: batch queries must coerce each key to BSON ObjectId' + found.size() == 3 + found*.title.sort() == ['A', 'B', 'C'] + } + + void "with storedAs ObjectId, findAllByIdInList resolves all ids"() { + given: + StoredAsVideo a = new StoredAsVideo(title: 'A').save(flush: true, failOnError: true) + StoredAsVideo b = new StoredAsVideo(title: 'B').save(flush: true, failOnError: true) + + when: + manager.session.clear() + List found = StoredAsVideo.findAllByIdInList([a.id, b.id]) + + then: 'regression test: dynamic-finder in-list must coerce each id to BSON ObjectId' + found.size() == 2 + found*.title.sort() == ['A', 'B'] + } + + void "with storedAs ObjectId, criteria in('id', [...]) resolves all ids"() { + given: + StoredAsVideo a = new StoredAsVideo(title: 'A').save(flush: true, failOnError: true) + StoredAsVideo b = new StoredAsVideo(title: 'B').save(flush: true, failOnError: true) + + when: + manager.session.clear() + List found = StoredAsVideo.createCriteria().list { + 'in' 'id', [a.id, b.id] + } + + then: 'regression test: criteria in-list on id must coerce to BSON ObjectId' + found.size() == 2 + found*.title.sort() == ['A', 'B'] + } + + void "with storedAs ObjectId, encoding a non-hex id falls back to BSON String instead of throwing"() { + given: '''a domain using an assigned natural key (not a valid ObjectId hex). + This is a misconfiguration — the user should not be combining storedAs: ObjectId + with natural keys — but the library should degrade predictably rather than + throwing IllegalArgumentException deep inside the BSON write pipeline.''' + AssignedNonHexVideo v = new AssignedNonHexVideo(id: 'my-slug', title: 'Slug-Keyed').save(flush: true) + + expect: 'save did not throw; a doc was written with BSON String _id (the fallback path)' + v != null + !v.hasErrors() + + when: + Document raw = manager.mongoClient.getDatabase('test').getCollection('assignedNonHexVideo') + .find(new Document('_id', 'my-slug')).first() + + then: + raw != null + raw.getString('title') == 'Slug-Keyed' + } + + void "with storedAs ObjectId, point lookup of a non-hex id matches the BSON String the encoder wrote"() { + given: '''save path falls back to BSON String for non-hex; the read path must mirror + that fallback — otherwise the ConversionService returns null for invalid hex + and the query targets {_id: null}, stranding the document.''' + new AssignedNonHexVideo(id: 'my-slug', title: 'Slug-Keyed').save(flush: true, failOnError: true) + + when: + manager.session.clear() + AssignedNonHexVideo found = AssignedNonHexVideo.get('my-slug') + + then: 'regression: String→ObjectId converter returns null for non-hex; IdEquals handler keeps original' + found != null + found.id == 'my-slug' + found.title == 'Slug-Keyed' + } + + void "with storedAs ObjectId, update of a non-hex id document lands on the right row"() { + given: + new AssignedNonHexVideo(id: 'my-slug', title: 'Original').save(flush: true, failOnError: true) + + when: + manager.session.clear() + AssignedNonHexVideo reloaded = AssignedNonHexVideo.get('my-slug') + reloaded.title = 'Updated' + reloaded.save(flush: true, failOnError: true) + + and: + Document raw = manager.mongoClient.getDatabase('test').getCollection('assignedNonHexVideo') + .find(new Document('_id', 'my-slug')).first() + + then: 'regression: without the null-return fallback the update filter targets {_id: null} and silently misses' + raw != null + raw.getString('title') == 'Updated' + } + + void "with storedAs ObjectId, batch getAll with non-hex ids falls back to BSON String in the in-list"() { + given: + new AssignedNonHexVideo(id: 'slug-a', title: 'A').save(flush: true, failOnError: true) + new AssignedNonHexVideo(id: 'slug-b', title: 'B').save(flush: true, failOnError: true) + + when: + manager.session.clear() + List found = AssignedNonHexVideo.getAll(['slug-a', 'slug-b']) + + then: 'regression: In handler converts each value to ObjectId (returns null for non-hex); fallback keeps original' + found.size() == 2 + found*.title.sort() == ['A', 'B'] + } + + void "with storedAs ObjectId, delete of a non-hex id document removes the row"() { + given: + new AssignedNonHexVideo(id: 'my-slug', title: 'Delete Me').save(flush: true, failOnError: true) + + when: + manager.session.clear() + AssignedNonHexVideo reloaded = AssignedNonHexVideo.get('my-slug') + reloaded.delete(flush: true) + + and: + Document raw = manager.mongoClient.getDatabase('test').getCollection('assignedNonHexVideo') + .find(new Document('_id', 'my-slug')).first() + + then: 'regression: delete filter must target {_id: "my-slug"} (BSON String), not {_id: null}' + raw == null + } + + void "with storedAs ObjectId, legacy documents written directly as BSON ObjectId are fully accessible"() { + given: 'a document inserted outside GORM with _id as BSON ObjectId (simulates legacy data)' + ObjectId legacyId = new ObjectId() + storedAsRawCollection().insertOne(new Document('_id', legacyId).append('title', 'Legacy')) + + when: 'point lookup' + manager.session.clear() + StoredAsVideo found = StoredAsVideo.get(legacyId.toHexString()) + + then: 'works — no migration needed' + found != null + found.id == legacyId.toHexString() + + when: 'update' + found.title = 'Updated' + found.save(flush: true, failOnError: true) + manager.session.clear() + + and: + Document raw = storedAsRawCollection().find(new Document('_id', legacyId)).first() + + then: 'update lands on the legacy ObjectId-_id doc' + raw.getString('title') == 'Updated' + } + + private com.mongodb.client.MongoCollection rawCollection() { + manager.mongoClient + .getDatabase('test') + .getCollection('legacyVideo') + } + + private com.mongodb.client.MongoCollection storedAsRawCollection() { + manager.mongoClient + .getDatabase('test') + .getCollection('storedAsVideo') + } +} + +@Entity +class LegacyVideo { + String id + String title +} + +@Entity +class ObjectIdVideo { + ObjectId id + String title +} + +@Entity +class StoredAsVideo { + String id + String title + + static mapping = { + id storedAs: ObjectId + version false // orthogonal to storedAs; avoids version-field noise in raw-insert tests + } +} + +@Entity +class AssignedNonHexVideo { + String id + String title + + static mapping = { + id generator: 'assigned', storedAs: ObjectId + version false + } +} + +// Mirrors StoredAsVideo but keeps versioning on. The 'updates persist (no phantom +// OptimisticLockingException)' regression can only be genuinely exercised when a +// version field is present — without it, GORM never raises OLE regardless of +// whether the update filter matches. +@Entity +class VersionedStoredAsVideo { + String id + String title + + static mapping = { + id storedAs: ObjectId + } +} diff --git a/grails-data-mongodb/docs/src/docs/asciidoc/gettingStarted/advancedConfig.adoc b/grails-data-mongodb/docs/src/docs/asciidoc/gettingStarted/advancedConfig.adoc index 1ee849e6d54..c333acf4fdc 100644 --- a/grails-data-mongodb/docs/src/docs/asciidoc/gettingStarted/advancedConfig.adoc +++ b/grails-data-mongodb/docs/src/docs/asciidoc/gettingStarted/advancedConfig.adoc @@ -122,3 +122,5 @@ grails.mongodb.default.mapping = { ---- The `*` method is used to indicate that the setting applies to all properties. + +You can also set a global default for the storage type of `String id` fields via `grails.mongodb.stringIds.defaultStoredAs` (values `string` or `objectid`). See <> for details. diff --git a/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc b/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc index be51d4c20e7..0d4f0a9a80b 100644 --- a/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc +++ b/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc @@ -67,4 +67,65 @@ 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: + stringIds: + defaultStoredAs: 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. "jsmith@example.com" + + 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. +* Combining `generator: 'assigned'` with `storedAs: ObjectId` requires that the assigned value be a valid 24-char ObjectId hex string. Invalid values fall back to writing BSON `String` on the assumption that the domain is using a natural key despite the `storedAs` setting — consider declaring `storedAs: String` (or omitting it) in that case. \ No newline at end of file diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/Property.groovy b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/Property.groovy index 64dae27a7c2..3f5bbde9dec 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/Property.groovy +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/Property.groovy @@ -79,6 +79,20 @@ class Property implements Cloneable { * @param generator name or class */ String generator + /** + * Override the storage type used to persist this property independent of its declared Java type. + * + *

The primary use case is on identifiers: a domain may declare {@code String id} for + * ergonomic reasons (clean JSON, no native-type dance in controllers) while the underlying + * document stores {@code _id} as a native type such as {@code ObjectId}. Backends that + * support this (currently MongoDB GORM) coerce between the declared Groovy type and the + * native storage type on write, read, and query. + * + *

A {@code null} value means "use the declared Java type" (default / current behavior). + * + * @return The native storage class, or {@code null} to use the declared property type. + */ + Class storedAs /** * @return The maximum size */ diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/IdentityMapping.java b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/IdentityMapping.java index 14b9f65289c..baa937f6b81 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/IdentityMapping.java +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/IdentityMapping.java @@ -38,4 +38,19 @@ public interface IdentityMapping extends PropertyMapping { * @return The type of value generated used */ ValueGenerator getGenerator(); + + /** + * The native storage type for this identifier, which may differ from the declared Java type. + * + *

When non-{@code null}, the backend is expected to coerce identifier values between + * the declared type and this type at write, read, and query time. Currently honored by + * MongoDB GORM to support patterns like "declare {@code String id}, store BSON + * {@code ObjectId}" without requiring per-call conversion in application code. + * + * @return the storage type, or {@code null} to use the declared property type. + * @since 7.1.1 + */ + default Class getStoredAs() { + return null; + } } diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/MappingFactory.java b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/MappingFactory.java index 9fd9754a340..8cbfd319699 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/MappingFactory.java +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/MappingFactory.java @@ -551,12 +551,23 @@ public ValueGenerator getGenerator() { return ValueGenerator.AUTO; } + @Override + public Class getStoredAs() { + // Read from the underlying property so backend-specific hooks (e.g. Mongo's + // global default applied in createIdentity) are observable, even for domains + // without an explicit `static mapping` block. + Property mappedForm = getMappedForm(); + return mappedForm != null ? mappedForm.getStoredAs() : null; + } + public ClassMapping getClassMapping() { return classMapping; } public Property getMappedForm() { - return classMapping.getEntity().getIdentity().getMapping().getMappedForm(); + // Null-safe for composite-key entities, which have null getIdentity(). + PersistentProperty identity = classMapping.getEntity().getIdentity(); + return identity != null ? identity.getMapping().getMappedForm() : null; } }; } @@ -580,6 +591,14 @@ public ValueGenerator getGenerator() { return generator != null ? ValueGenerator.valueOf(generator) : ValueGenerator.AUTO; } + @Override + public Class getStoredAs() { + // Read dynamically from the underlying property so downstream hooks (e.g. Mongo's + // global default applied in createIdentity) remain observable even when they run + // after this IdentityMapping has been constructed. + return property != null ? property.getStoredAs() : null; + } + public ClassMapping getClassMapping() { return classMapping; }