jdaugherty commented on code in PR #15583:
URL: https://github.com/apache/grails-core/pull/15583#discussion_r3141992071
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy:
##########
@@ -286,6 +286,44 @@ 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}).
+ *
+ * <p>Exercised end-to-end by {@code StringIdWithObjectIdStorageSpec}:
Review Comment:
Listing Spock test method names verbatim in javadoc (`"with storedAs
ObjectId, updates persist…"`, etc.) is brittle: those names tend to get renamed
during PR cycles, and there is no compile-time link to flag the doc as stale
when that happens. A short semantic pointer (e.g. `see
StringIdWithObjectIdStorageSpec for end-to-end coverage of update/delete
filters under storedAs`) carries the same value without the rot risk. Same
comment applies to the parallel javadoc in
`MongoCodecEntityPersister#coerceIdToStoredType` and the inline comment block
above the new `In` handler in `MongoQuery`.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java:
##########
@@ -133,6 +137,20 @@ public class MongoMappingContext extends
DocumentMappingContext {
private CodecRegistry codecRegistry;
private Map<Class, Boolean> 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;
Review Comment:
`stringIdDefaultStoredAs` has a public setter but is neither `final` nor
`volatile`. In practice the field is set in the constructor before
`initialize(classes)`, but `MongoDocumentMappingFactory.createIdentity` reads
it later for every entity — and any caller that uses the public setter from a
different thread than the constructor thread has no guaranteed visibility.
Either make it `final` and drop the setter (constructor-only assignment is
JMM-safe via the publication rules), or mark it `volatile`. Same applies to the
accessor pair if you keep the setter.
--
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]
