Repository navigation
Merge 8.0.x into 8.1.x - #16345
Merged
Merged
Merge 8.0.x into 8.1.x#16345
Conversation
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.
…ions Basic extends ToMany, so a collection of simple values -- hasMany = [labels: String] -- entered the branch added for OneToMany and ManyToMany. A Basic association has no associated entity, so the reflector lookup dereferenced null and updateAll threw NullPointerException on both engines for a call that worked before this PR. The branch now names the two kinds it is for and requires a non-null associated entity. EmbeddedCollection no longer needs excluding: it extends Association directly, not ToMany, so it never reached here and the exclusion implied a case that cannot occur.
A bidirectional one-to-many keeps its foreign key on the inverse side, so there is no
field on the owning document to update -- OneToManyEncoder's shouldEncodeIds skips it for
that reason, and the decoder never reads such a field.
Left in the $set the value was written as raw subdocuments, and the owner then failed to
decode at all:
BsonInvalidOperationException: ReadBSONType can only be called when State is TYPE
Skipping the id encoding alone is not enough, because the property still reaches $set. It
is now rejected by name. Dropping it silently would be the same silent no-op this review
objected to elsewhere, and the operation is genuinely unexpressible: reassigning children
means updating their foreign key, which an update on the parent cannot do.
…e reads This engine keeps many-to-many ids under a suffixed key -- see setManyToMany and getManyToManyKeys -- so writing the plain association name left the real field untouched. The document ended up with both, the read path kept returning the old association, and the update was silently a no-op. Also adds the read-back assertion the review noted was missing: the unidirectional one-to-many cases only checked the raw document, so nothing covered the retrieveAllEntities conversion of coerced ObjectId keys.
ToOneEncoder writes a reference on the owner only when the foreign key is not held by the child -- its !isForeignKeyInChild() guard. The updateAll normalization had no equivalent check, so updateAll(nose: n) on a hasOne owner wrote an id field onto a document that normal persistence never gives one, the same corruption shape as the bidirectional one-to-many case. Found by an adversarial audit of the association-type space rather than by a reviewer: two independent analysts converged on it after the previous three defects here were all the same shape -- an association kind entering a branch meant for a different one.
Two divergences from normal persistence, both found by the same audit as the hasOne case. OneToManyEncoder drops null entries before wrapping a reference collection, so it never builds DBRef(collection, null); the bulk path kept them and would hand the driver a null id. It now filters exactly where the encoder does, and leaves a plain id list alone. The non-codec engine's setManyToMany stores plain identifiers under its suffixed key and getManyToManyKeys reads them back the same way -- it never writes DBRefs. Wrapping them in the bulk path produced an array the read path cannot interpret, so many-to-many values are now written as plain ids there.
An ordinary shutdown stops the embedded server twice: the lifecycle bean stops it when the application context closes, and the shutdown hook the initializer registers stops it again as the JVM exits. The flapdoodle backend already treats the second stop as nothing to do, but the in-memory backend called MongoServer.shutdownNow() every time. A second shutdownNow() can wait forever. It closes the clients mongo-java-server still tracks and waits, with no timeout, for them to close. A client that connected just as the first shutdown began can be registered on a Netty event loop after that loop has closed its channels; the loop then terminates with the client still tracked, and closing it through that loop never completes. Waiting on that from the shutdown hook, the JVM never exits. This module's own test worker hung that way on CI until GitHub cancelled the job at six hours, six times on 8.0.x between 2026-08-26 and 2026-09-11. Stopping a server that is already stopped now returns at once, and stop() and restart() are synchronized, since the context's shutdown hook and the initializer's run at the same time on the way out. A new feature runs an application in a JVM of its own that lets a client connect, stops the server as its context closes, and returns from main. Without this change that JVM never exited in 8 of 8 runs, so the feature fails after 30 seconds rather than hanging; with it the JVM exits in about a second. The application is written in Java because, started through Groovy and Spock, the same steps almost never hang even unfixed. The existing feature that stops a server twice now waits a bounded time for the second stop, so a regression fails instead of hanging the build.
Each feature closed its application context in cleanup, but a context that was never refreshed is not active, and closing a context that is not active does nothing. The four mongods the specification started were left running for the JVM shutdown hook to stop as the test worker exited, and when that hook hung they were still running when CI killed the job. Stop each server through the lifecycle bean the initializer registers, as EmbeddedMongoInitializerSpec and EmbeddedReplicaSetSpec already do.
mongodb-test-config.gradle skips its test tasks when a job selects another datastore with -PonlyHibernate5Tests, -PonlyHibernate7Tests, -PonlyRedisTests and the rest, but not with -PonlyNeo4jTests. The three Neo4j jobs therefore ran every MongoDB suite that config covers, which the five MongoDB jobs already run, and four of the six hangs of the embedded MongoDB tests on 8.0.x cancelled a Neo4j job. grails-data-mongodb-core applies mongodb-forked-test-config.gradle, whose list has the same gap, but the data TCK config already keeps that module out of the Neo4j jobs, so its list is left as it is.
A test JVM that never exits keeps its job running until GitHub cancels it at six hours, long after every other job has reported. Give each test job a timeout of twice the slowest run that finished on its own, passed or failed, over the 160 CI runs from 2026-09-03 to 2026-09-13, rounded up to the half hour: functional 150 min (slowest 63) security 150 min (slowest 71) securityConfig 90 min (slowest 39) casSecurityConfig 90 min (slowest 35) redis 180 min (slowest 86) mongodbFunctional 150 min (slowest 70) hibernate5Functional 120 min (slowest 58) hibernate7Functional 210 min (slowest 104, a failure; the slowest pass took 86) neo4jFunctional 180 min (slowest 90) The build jobs are left without one: the macOS build has passed after more than five hours.
hibernate5-test-config.gradle, hibernate7-test-config.gradle and redis-test-config.gradle skip their test tasks for every other datastore's -Ponly flag except -PonlyNeo4jTests, as mongodb-test-config.gradle did. The three Neo4j jobs therefore also ran nineteen Hibernate 5 and Hibernate 7 test tasks - the plugin, Spring Boot and database migration suites - which the Hibernate jobs already run. The core modules were already kept out by the data TCK config, and grails-redis by test-config.gradle, so the Redis list changes nothing today; it now names the Neo4j jobs itself instead of relying on another config to.
Its class comment blamed a server thread that is not a daemon: a Gradle test worker, it said, cannot exit while one runs. The worker exits through System.exit, which does not wait for such threads. It waits for every JVM shutdown hook to finish, and a hook stopping a server that hangs is what kept builds whose tests had all passed from finishing.
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.
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.
…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
✅ All tests passed ✅Test SummaryCI / Functional Tests (Java 25, indy=false, shard 1) > :grails-test-examples-app1:integrationTest
🏷️ Commit: 916fefe Test FailuresAsyncPromiseSpec > async task with timeout completes within time limit > async task with timeout completes within time limit [delay: 100, timeout: 500, elapsedMin: 100, status: completed, result: Completed in time, #0] (:grails-test-examples-app1:integrationTest in CI / Functional Tests (Java 25, indy=false, shard 1) | Attempt 1/2)AsyncPromiseSpec > multi-stage process reports all stages (:grails-test-examples-app1:integrationTest in CI / Functional Tests (Java 25, indy=false, shard 1) | Attempt 1/2)Learn more about TestLens at testlens.app/docs. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.1.x #16345 +/- ##
==================================================
+ Coverage 56.2213% 56.5461% +0.3248%
- Complexity 21879 22027 +148
==================================================
Files 2155 2155
Lines 103146 103298 +152
Branches 18276 18328 +52
==================================================
+ Hits 57990 58411 +421
+ Misses 37037 36706 -331
- Partials 8119 8181 +62
🚀 New features to boost your workflow:
|
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.0.xinto8.1.x, not a squash. Git resolved it with theortstrategy and no conflicts, and no file was changed on both lines since the last merge (#16343).projectVersionremains8.1.0-SNAPSHOT.Carries the MongoDB
updateAllassociation work, the embedded MongoDB shutdown-hang fix (#16338), and the CI job timeout and suite-scoping changes for the Neo4j and Redis jobs.