jdaugherty commented on code in PR #15583:
URL: https://github.com/apache/grails-core/pull/15583#discussion_r3141979599
##########
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<>();
+ /**
Review Comment:
Add a new line between the comment and the other variable?
##########
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.
##########
grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/IdentityEncoder.groovy:
##########
@@ -41,6 +41,25 @@ class IdentityEncoder implements PropertyEncoder<Identity> {
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(<non-hex>) 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)) {
Review Comment:
From AI:
When `storedAs: ObjectId` is set but the value is not valid hex, the encoder
silently writes a BSON String. The doc caveat acknowledges this and points
users at `storedAs: String`, but at runtime there is no signal — a
misconfigured `id generator: assigned, storedAs: ObjectId` domain just quietly
persists Strings. Consider a one-shot `log.warn` (e.g. cached per `(entity,
id-shape)` to avoid log spam) so this is debuggable in the field. Today the
only way to discover the misconfiguration is to peek at raw BSON.
##########
grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/IdentityEncoder.groovy:
##########
@@ -51,6 +70,14 @@ class IdentityEncoder implements PropertyEncoder<Identity> {
}
+ private static Class<?> resolveStoredAs(Identity property) {
+ try {
+ return property?.owner?.mapping?.identifier?.storedAs
+ } catch (Throwable ignored) {
Review Comment:
`catch (Throwable ignored)` is broader than it needs to be: it swallows
`OutOfMemoryError`, `StackOverflowError`, etc. Why can't you use Exception
ignored?
Same pattern repeats in `MongoCodecSession#coerceIdToStoredType`,
`MongoCodecEntityPersister#coerceIdToStoredType`, and the two `try` blocks in
`MongoQuery` (`IdEquals` and the new `In` handler)
##########
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}:
+ * <ul>
+ * <li>"with storedAs ObjectId, updates persist (no phantom
OptimisticLockingException)" — happy path on update filter</li>
+ * <li>"with storedAs ObjectId, update of a non-hex id document lands on
the right row" — null-return fallback on update filter</li>
+ * <li>"with storedAs ObjectId, delete of a non-hex id document removes
the row" — null-return fallback on delete filter</li>
+ * <li>"with storedAs ObjectId, legacy documents written directly as
BSON ObjectId are fully accessible" — update path against legacy BSON ObjectId
_id</li>
+ * </ul>
+ */
+ protected Object coerceIdToStoredType(Object nativeKey, PersistentEntity
entity) {
Review Comment:
This looks identical to `MongoCodecEntityPersister#coerceIdToStoredType` &
the same logic seems to exist in `MongoQuery`'s `IdEquals`/`In` handlers. Lets
extract it into a static helper so the logic doesn't diverge?
##########
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) {
Review Comment:
This is from AI:
(1) non-DirtyCheckable embedded with a discriminator (subclass) — the new
branch writes `_class` only at the embedded root, but nested embedded objects
via `Embedded`/`EmbeddedCollection` recurse through the same `else` branch and
may need the same treatment
(2) confirm that the existing DirtyCheckable path is genuinely never hit for
POGOs that have been touched by `markDirty` upstream — otherwise this branch
can fire double-encodes.
##########
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:
##########
@@ -176,6 +215,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);
Review Comment:
Asymmetry to flag: the deprecated `(PropertyResolver, Class...)` constructor
and this `(AbstractMongoConnectionSourceSettings, Class...)` constructor both
read `stringIdDefaultStoredAs` from settings, but the public
`MongoMappingContext(String defaultDatabaseName, Closure defaultMapping,
Class... classes)` constructor does not — so anyone wiring the context manually
with that signature silently loses the global default. Either factor a private
`applyStoredAsFromConfig(...)` helper and invoke it from all constructors that
have the relevant inputs, or document on the bare constructor that it does not
honor `grails.mongodb.stringIdsDefaultStoredAs`. Today this is the kind of
inconsistency that gets discovered only when a test using the bare constructor
passes locally and the production datastore — using the settings constructor —
diverges.
##########
grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy:
##########
@@ -0,0 +1,388 @@
+/*
+ * 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}).
+ *
+ * <ul>
+ * <li>{@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.</li>
+ * <li>{@code MongoQuery.IdEquals} and {@code IdentityEncoder} do
<em>not</em>
+ * 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.</li>
+ * </ul>
+ *
+ * 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<GrailsDataMongoTckManager> {
+
+ void setupSpec() {
+ manager.domainClasses.addAll([LegacyVideo, ObjectIdVideo,
StoredAsVideo, AssignedNonHexVideo])
+ }
+
+ 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<LegacyVideo> 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: "<hex>"} 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: "<hex>"} 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, 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<StoredAsVideo> 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<StoredAsVideo> 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<StoredAsVideo> 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<AssignedNonHexVideo> 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<Document> rawCollection() {
+ manager.mongoClient
+ .getDatabase('test')
+ .getCollection('legacyVideo')
+ }
+
+ private com.mongodb.client.MongoCollection<Document>
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
Review Comment:
Both `StoredAsVideo` and `AssignedNonHexVideo` declare `version false`. The
headline regression for this PR — `with storedAs ObjectId, updates persist (no
phantom OptimisticLockingException)` — therefore cannot actually exercise the
optimistic-locking path it claims to cover, because there is no `version`
field. Worth adding a parallel domain that keeps versioning on (the default) so
the original failure mode (where a missed update filter throws
`OptimisticLockingException`) is genuinely reproduced and proven fixed.
Otherwise this regression test will pass even if a future change accidentally
reintroduces the bug under versioned domains, which is the more common case in
real apps.
--
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]
