Repository navigation
Merge 8.1.x into 9.0.x - #16355
Merged
Merged
Merge 8.1.x into 9.0.x#16355
Conversation
…erator-based removals Interception-based stores (MongoDB) rely exclusively on the DirtyChecking* wrappers — there is no flush-time snapshot comparison — so mutations that escape them are silently lost: save() sees a clean entity and persists nothing. Two real-world escapes: 1. Reassignment through a generated setter stored the raw value, so 'entity.items = []' over a tracked list replaced the wrapper with a plain untracked ArrayList. The common defensive re-init 'if (!entity.items) entity.items = []' triggers this on every load (an empty tracked collection is falsy in Groovy), and because [] == [] the equality-suppressed markDirty never flagged the assignment either. The in-place add() that followed was lost. 2. DirtyCheckingCollection never overrode iterator(), so every iterator-based removal — including Groovy's removeAll(Closure) and retainAll(Closure) and Java's removeIf — bypassed tracking, along with retainAll(Collection), List.sort and List.replaceAll. The fix stays interception-only (no snapshots, no flush-time diffing): - Wrappers override iterator()/listIterator() with dirty-marking iterators plus the missing direct mutators — the same approach as Hibernate's PersistentCollection. - Generated setters for Collection/List/Set/Map-typed properties assign through DirtyCheckingSupport.rewrap, which wraps the incoming value ONLY when the value being replaced was itself a tracked wrapper. A never-tracked property (transient instance, or a store like Hibernate with its own dirty checking) stores the raw value as before, and non-collection properties compile to identical bytecode. - A replacement wrapper is flagged isAssigned() so PersistentEntityCodec takes the full-rewrite path rather than per-element diffing — a replacement's layout need not match the stored array (a same-size replacement of clean elements previously emitted no update at all once wrapped). Specs reproduce each escape before the fix: DirtyCheckingCollectionSpec (wrapper mutation paths), DirtyCheckCollectionReassignmentSpec (setter reassignment), and EmbeddedCollectionDirtyTrackingSpec (end-to-end against MongoDB, replicating the production shape where an auto-timestamped entity dropped an embedded-collection add).
- SortedSet wrapper: construction, assigned flag, iterator-based removal - ListIterator navigation methods (hasPrevious/previous/nextIndex/previousIndex) - DirtyCheckingMap assigned flag on both constructors - DirtyCheckableCollection.isAssigned() interface default (kept false for implementations that do not override it, e.g. PersistentCollection) - rewrap: tracked-wrapper passthrough, bare-Collection fallback, and the defensive non-collection tail
Make `grails.mongodb.stringIds.defaultStoredAs` default to `objectid`, so a
domain declaring `String id` persists `_id` as a BSON ObjectId while keeping
String ergonomics in application code. Set it to `string` to opt out.
The default is applied via the field initializer rather than only in the
config-reading constructors, so all four MongoMappingContext constructors
register entities with the same default.
Making it the default surfaced five latent defects in the storedAs path, all
of which reproduce today by setting defaultStoredAs: objectid explicitly:
- ToOneEncoder and OneToManyEncoder wrote association references using the
declared id type, so a reference pointed at an ObjectId _id with a BSON
String and matched nothing from $lookup or the raw driver.
- ToOneDecoder read by the predicted type and threw
BsonInvalidOperationException on documents whose stored type legitimately
differs (a non-hex assigned id falls back to BSON String even under
storedAs: ObjectId). It now reads the actual BSON type.
- OneToManyDecoder returned stored-type ids rather than the declared type.
- MongoQuery coerced only identity criteria, so a filter on a to-one
association sent a hex String against an ObjectId foreign key; bidirectional
one-to-many and hasOne lookups silently resolved to empty.
- MongoQuery also missed findAllById(hex), which builds Equals('id', ..)
rather than IdEquals and so bypassed id coercion entirely, returning no
results. findAllByIdInList was unaffected because the In handler covers it.
MongoIdCoercionSpec covers both coercion directions and their fallback
branches directly: null key, null entity, a value already of the target type,
the non-hex natural key that must stay a String rather than produce a
{_id: null} filter, an ObjectId-id domain being left alone, and a round trip.
StringIdAssociationStorageSpec asserts the BSON that lands on disk rather than
only that traversal works — traversal passed the whole time the stored type
was wrong, which is why the mismatch went unnoticed. It covers to-one and
to-many reference storage, resolution through GORM, a bidirectional
one-to-many, findAllById and findAllByIdInList, and a reference written as a
BSON String so pre-existing data keeps decoding.
Each case was checked against reverted production code: reverting the encoders
fails the two reference-storage cases and findAllById, and reverting only
MongoQuery fails the bidirectional and findAllById cases.
…default The Decoupling section introduced storedAs: ObjectId as opt-in and stated that a bare String id writes _id as a BSON String, contradicting the Global Default section below it.
The bulk delete path collected native keys under Groovy truthiness, so an empty String id -- a valid BSON _id, reachable through the invalid-ObjectId fallback for assigned natural keys -- was dropped from the delete model and the document silently survived.
The bulk update path replaced an association object with its raw declared identifier, so updateAll(child: child) wrote a BSON String where normal persistence writes the target's stored _id type, and wrote a plain id where the mapping asks for a DBRef. Association queries and external clients could not match the updated relationship. It now coerces through the associated entity's mapping and emits a DBRef when the mapping declares one, matching ToOneEncoder.
The Mongo-specific preprocessing ran only for criteria at the current junction
level. The inherited BsonQuery negation handler dispatches nested criteria
itself, so an id or to-one association criterion inside not { } never reached
it and was sent as a hex String against an ObjectId; findAllByIdNot(hex) could
therefore fail to exclude the document it named.
The preprocessing moves into a helper that recurses through junctions, so
criteria nested inside not { }, and { } and or { } are coerced too.
The IN handler applied the storage type only when the queried property was the entity's own identity. getInListQueryValues unwraps association instances to their declared identifier, so a to-one association IN criterion sent hex Strings against ObjectId foreign keys and matched nothing -- affecting 'child in [childInstance]' and findAllByChildInList(..). It now resolves the entity whose identifier mapping governs the criterion, the same way the scalar path does, and coerces each unwrapped value against it.
MongoEntityPersister decided the stored _id type from the declared identifier type alone, so a String-id domain always wrote a BSON String. Once a bare String id resolves to storedAs: ObjectId, a document saved by this engine could not be found again, because the shared MongoQuery sends an ObjectId. Two places needed it, not one: storeEntry overwrites _id with the declared-type identifier immediately after generateIdentifier runs, and createDBObjectWithKey builds the by-key filter and passed the hex String straight through for a String identifier. Both now go through the id mapping, while the declared-type value is still what the domain sees.
The non-codec ("mapping") engine builds its flush-time update and delete
filters from the declared identifier type, and writes association references
and DBRef $id values the same way. A default it does not implement end to end
would have it store documents it could not then update, delete or join.
The default is therefore scoped to the codec engine, which is the default
engine and the one every path in this PR covers. The mapping engine keeps the
pre-8.0.0 behaviour exactly.
Scoping affects the default only. An application that explicitly sets
grails.mongodb.stringIds.defaultStoredAs still gets what it asked for on
either engine, with the same caveats it has today.
This also reverts the partial mapping-engine _id coercion from an earlier
commit on this branch: with the default no longer reaching that engine, the
insert and point-read paths no longer need it, and fixing those two while
update, delete and association writes stayed uncoerced left that engine
half-converted.
The codec bulk update path wrote the encoded association reference back into the caller-provided map. That replaced their domain object with an ObjectId or DBRef after the call, and threw UnsupportedOperationException outright for an unmodifiable argument such as Collections.singletonMap or Map.of. The map is now copied before normalising. Pre-dates this branch -- the same line previously wrote the declared identifier back into the caller's map -- but the encoded value makes the mutation more surprising, and it is cheap to stop doing.
MongoEntityPersister decided the stored _id type from the declared identifier type alone, so a String-id domain always wrote a BSON String. Once a bare String id resolves to storedAs: ObjectId, the shared MongoQuery sends an ObjectId and never matches those documents. Four places needed it. storeEntry overwrites _id with the declared-type identifier immediately after generateIdentifier runs, and is the single point where _id is written for this engine. createDBObjectWithKey builds the by-key filter. MongoSession builds the flush-time update filter and the single and batch delete keys, and its iterable delete filters on the literal _id field rather than the logical identity name, so the criterion preprocessing in MongoQuery does not recognise it. The declared-type value is still what the domain sees throughout.
…engine formulateDatabaseReference returned the association's declared identifier, and setEmbeddedCollectionKeys built its DBRef list the same way, so a String-id target stored as ObjectId was pointed at by a BSON String. The codec engine coerces this in ToOneEncoder. Left uncoerced, no $lookup, raw driver query or coerced association criterion could match the document being referenced -- and MongoQuery now coerces association criteria, so those queries searched for ObjectId against String foreign keys. Plain foreign keys, DBRef $id values and embedded collection references all go through the target entity's id mapping now.
…rop the engine scoping MongoSession.updateAll sent the caller's property map straight into $set, so a bulk association update wrote the domain object itself rather than a reference. It now extracts the target's identifier, coerces it to the type that target's _id is stored as, and emits a DBRef where the mapping asks for one -- matching both normal persistence and the codec engine's updateAll. Normalised into a copy, so the caller's map is untouched and may be immutable. With every non-codec path now going through the id mapping, the default no longer needs restricting to the codec engine, so that scoping is removed and MongoMappingContext applies the default on both engines again. MappingEngineStringIdStorageSpec covers this engine end to end: insert, point read, update, single delete, iterable delete, to-one reference storage, traversal, and bulk association update. It drives the session API rather than the GORM statics, which bind to whichever datastore registered the class last, and asserts up front that the session really is a MongoSession -- without that guard the cases would pass against the codec engine and prove nothing.
The "mapping" engine has not been the default since GORM 6, is selected by an undocumented setting, and reaches MongoDB through an entirely separate persister hierarchy -- MongoSession, MongoEntityPersister and AbstractMongoObectEntityPersister -- that has to be kept in step with the codec one for every change to the storage layer. It carries no capability the codec engine lacks. That cost is not theoretical: making ObjectId the default for String ids required six separate fixes in this engine (id storage, the by-key filter, the flush-time update and delete filters, the iterable delete, association references and DBRef $id values, and bulk association updates), none of which had any test coverage. Before this branch nothing in the test suite exercised grails.mongodb.engine at all. Deprecated rather than removed, because MongoEntityPersister and AbstractMongoObectEntityPersister are public types that third parties may extend, and an application setting the engine today deserves a warning before its configuration stops working. Selecting it now logs at startup, and the setting is documented for the first time -- as deprecated, with the advice to remove it.
Both updateAll implementations reflected the association value's identifier directly. A lazy GORM proxy keeps its id in the proxy handler rather than the reflected field, so reflecting one yields null and the bulk update writes an invalid reference. ToOneEncoder asks the proxy factory first on the normal persistence path; updateAll now does the same, on both engines.
Embedded extends ToOne, so the association normalization added for bulk updates also caught embedded properties. An embedded value is a subdocument with no identity of its own: normal persistence encodes it through the embedded path rather than ToOneEncoder, and reflecting an id from one yields null, so updateAll(address: new Address(..)) wrote a scalar or null where a subdocument belongs. Both engines now skip Embedded and leave the value for the normal encoding.
…on-codec engine Two association write paths still stored the declared identifier. The association indexer holds unidirectional one-to-many keys on the owning document, as plain ids or DBRefs, and the many-to-many path builds its own id list. For a target whose _id is stored as ObjectId, both left BSON Strings that raw joins, DBRef consumers and external clients cannot match. Both now go through the target entity's id mapping, matching formulateDatabaseReference and setEmbeddedCollectionKeys.
The association normalization handled only ToOne, so OneToMany and ManyToMany values went into $set as the domain objects themselves rather than the ids normal persistence stores -- inconsistent at best, and dependent on whatever the codec registry happens to do with a domain instance. Both engines now encode each element the way OneToManyEncoder does: the target's identifier in its stored _id type, wrapped in a DBRef where the mapping declares one, and proxy-safe. EmbeddedCollection is excluded for the same reason as Embedded -- those are subdocuments, not references.
The setting was described as having 'only supported value: codec' and then documented selecting 'mapping', which the implementation still supports with a deprecation warning. It now says codec is the default and recommended value and mapping remains available for compatibility but is deprecated.
grails-redis applied test-config.gradle as well as redis-test-config.gradle, and a test task runs only when every config applied to it lets it. test-config.gradle skips its suites under -PonlyRedisTests, and redis-test-config.gradle skips them under -PonlyCoreTests and every other selection flag, so between the two the module's test and integrationTest tasks ran in no CI job: the Redis jobs reported only grails-test-examples-redis. Apply redis-test-config.gradle alone, as the Hibernate and MongoDB modules apply only their own config. It already forwards -PredisContainerVersion, and with it alone the tests pass against Redis 7.4 and 8.0: 4 unit and 31 integration features, besides the two RedisTagLibSpec features the spec itself skips.
Move the collection and core.Session imports into the existing org.grails
block in alphabetical order, matching the rest of the file.
The PersistentCollection subclass case now also asserts hasChanged('shares'),
mirroring the wrapper-side subclass case: the store persister relies on the
assignment being flagged to re-wrap the raw value on save.
…-producer-wiring Preserve generated Groovy compiler script producer dependencies
…at/gorm-count-returns-long-8.0.x # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
…rns-long-8.0.x Return Long from count() instead of silently narrowing to Integer
…ollection-tracking-8.0.x Keep collection properties dirty-checked after reassignment; track iterator-based removals
ManyToMany extends ToMany directly, not OneToMany, so !(association instanceof ManyToMany) after an instanceof OneToMany check never excluded anything and implied a hierarchy that does not exist. Also moves the collection-branch comment back beside the branch it describes; it had ended up above the bidirectional guard.
Both engines reflected the identifier off whatever value was supplied, so updateAll(project: project.id) failed with an IllegalArgumentException from the reflector. On the codec engine that predates this branch. On the non-codec engine it was new: before the association normalization, the raw value went through $set unchanged, which with String-stored ids happened to be right. A value is now resolved in order: a proxy through the proxy factory, an instance of the associated class through the reflector, and anything else taken as the identifier itself. It is then coerced to the target's stored type as before, so updateAll(project: project) and updateAll(project: project.id) write the same value, for to-one and to-many alike.
The MongoDB guide did not mention updateAll. The new UnsupportedOperationException for hasOne and bidirectional one-to-many is user-visible -- a fix, since those calls previously corrupted the owner document or failed with a driver error, but worth stating up front. The section covers how association values are written, that an instance or an identifier can be passed, which association kinds can be bulk-updated and why, and to update the side holding the foreign key for the rest.
… drop The test called "updateAll drops nulls from a to-many" asserted the opposite: a plain id list keeps its nulls, which is correct, since OneToManyEncoder only drops them when wrapping DBRefs. It is renamed to say so, and a reference: true case now covers the drop itself -- the null is removed, the remaining entry is a DBRef carrying the stored ObjectId, and it reads back.
It was tested only on the non-codec engine. On the codec engine a many-to-many shares the unidirectional branch, so nothing guarded it as its own association kind. The case asserts the ObjectId array lands under the plain name, as OneToManyEncoder writes it, and reads back.
The rejection was tested only on the codec engine, but the two engines implement it in separate code, so a regression in one would not show in the other's spec.
Merge 8.0.x into 8.1.x
…default-8.0.x Default String id storage to ObjectId, fix association id coercion, and deprecate the non-codec engine
Exercise the bounded double-stop regression ten times on separate ports and clarify that both the child JVM and Spock reproductions depend on timing. Enable SLF4J diagnostics in the child process and document the idempotent stop contract, with stopped-state assertions for both backends. Keep Spring Security test tasks out of Neo4j jobs, matching the other test configs. Verify that all 27 security test tasks across 12 modules are skipped with onlyNeo4jTests and remain enabled with onlySpringSecurityTests. The earlier replica set cleanup commit's reference to EmbeddedReplicaSetSpec was mistaken; the existing cleanup example is EmbeddedMongoInitializerSpec.
…hutdown-hang-8.0.x Keep the embedded MongoDB tests from hanging CI jobs for six hours
Merge 8.0.x into 8.1.x
One conflict, on the functional job's timeout-minutes in gradle.yml. 8.1.x set every test job to twice its slowest observed run; 9.0.x had already capped this one job at 80 minutes as a backstop to the Groovy 6 worker teardown hang, above the 45 and 70 minute limits its own run step enforces. Kept the 9.0.x cap; every other job takes the 8.1.x timeout.
On Groovy 6 the @Delegate-generated reversed() replaced the hand-written override on DirtyCheckingList and DirtyCheckingSortedSet, so the live reverse-ordered view came back as a plain java.util.ReverseOrderListView that wrote through to the target without marking the parent dirty. Three DirtyCheckingCollectionSpec cases failed on that, which took down the Coverage and SiteMesh 2 jobs. Exclude reversed() from the delegate so the override stands. The other SequencedCollection overrides (addFirst, removeFirst and the rest) were never displaced and are unchanged.
🚨 TestLens detected 1 failed test 🚨Here is what you can do:
Test SummaryGroovy Snapshot Canary Build / Build Grails (shard 2) > :grails-test-examples-scaffolding:integrationTest
🏷️ Commit: efc506b Test FailuresUserControllerSpec > User list (:grails-test-examples-scaffolding:integrationTest in Groovy Snapshot Canary Build / Build Grails (shard 2) | Attempt 2/3)UserControllerSpec > User list (:grails-test-examples-scaffolding:integrationTest in Groovy Snapshot Canary Build / Build Grails (shard 2) | Attempt 1/3)Rerun ControlsNote Checks are currently running using the configuration below. Select tests to mute in this pull request: 🔲 UserControllerSpec > User list Reuse successful test results: 🔲 ♻️ Only rerun the tests that failed or were muted before Click the checkbox to trigger a rerun: ☑️ Rerun jobs Learn more about TestLens at testlens.app/docs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release-line merge of
8.1.xinto9.0.x, not a squash.projectVersionremains9.0.0-SNAPSHOT.One conflict, on the
functionaljob'stimeout-minutesin.github/workflows/gradle.yml. 8.1.x set every test job to twice its slowest observed run (#16345), which would have given this job 150 minutes; 9.0.x had already capped it at 80 as a backstop to the Groovy 6 worker-teardown hang, above the 45 and 70 minute limits its own run step enforces withtimeout. Kept the 9.0.x cap and its comment; every other test job takes the 8.1.x timeout.The other two files both lines touched,
gradle/hibernate5-test-config.gradleandgradle/mongodb-test-config.gradle, merged cleanly: 8.1.x addedonlyNeo4jTeststo the skip list, 9.0.x's Spock Groovy-version-check flags are untouched.Carries from 8.1.x: the MongoDB
updateAllassociation work, the embedded MongoDB shutdown-hang fix (#16338), the dirty-checking collection wrapper changes ingrails-datastore-coreandgrails-datamapping-core, and gson views and graphql coverage work.One follow-up commit makes the merged tree pass: the dirty-checking collection wrappers keep their tracking
reversed()override. On Groovy 6 the@Delegate-generatedreversed()replaced the hand-written override onDirtyCheckingListandDirtyCheckingSortedSet(the compiled class carried both, and the generated one won), so the live reverse-ordered view came back as a plainjava.util.ReverseOrderListViewthat wrote through to the target without marking the parent dirty. ThreeDirtyCheckingCollectionSpeccases failed on that, taking down the Coverage and SiteMesh 2 jobs.reversed()is now excluded from the delegate. The other SequencedCollection overrides were never displaced.The same two-line change is worth carrying back to 8.0.x, where the wrappers live, so the lines do not diverge. It is inert on Groovy 5.