feat: GORM O(M+N) scaling — GormRegistry, SessionResolver infrastructure, and core-class tests (consolidates #15779, #15780, #15790)

[borinquenkid]

feat: GORM O(M+N) scaling — GormRegistry, SessionResolver infrastructure, and core-class tests (consolidates #15779, #15780, #15790)

Why one PR

The previous 3-PR stack (#15779 infra → #15780 implementation → #15790 tests) generated review churn because the infrastructure PR added public API whose callers lived one PR downstream, so "no caller in this PR" objections and "the tests are elsewhere" objections could not both be answered at once. Per discussion with @jdaugherty, the stack is consolidated into this single PR: every new API lands next to its consumer and its tests.

Supersedes and closes #15779, #15780, #15790.

Summary

Extracts all per-entity, per-qualifier GORM API state out of GormEnhancer into a GormRegistry singleton. GormEnhancer becomes a thin facade and delegates entity registration, API lookup, and datastore lifecycle to the registry. APIs are created by a pluggable GormApiFactory and looked up by (entityClass, qualifier) at call time — collapsing up-front API allocation from O(entityCount × tenantCount) to O(entityCount + tenantCount), with per-(entity, qualifier) APIs materialized lazily on first use.

Modules touched: grails-datamapping-core (registry + enhancer, bulk of the diff), grails-datastore-core (session-resolution infrastructure below), Hibernate 5/7, MongoDB, Simple adapters (minimal wiring; the full adapter migrations remain follow-up PRs), plus TCK/test-example updates.

grails-datastore-core infrastructure (formerly #15779)

• SessionResolver + TransactionSynchronizationSessionResolver: a stateless view over the existing SessionHolder/TransactionSynchronizationManager state — one authoritative session store, no parallel bookkeeping. resolve() performs the same validation housekeeping as doGetSession (evicts disconnected sessions, unbinds a holder emptied by eviction unless a transaction owns it).
• AbstractDatastore: lazy resolver accessor; destroy() closes thread-bound sessions via DatastoreUtils.closeSession, skipping holders owned by an active transaction; hasCurrentSession() now agrees with getCurrentSession() (validated-session semantics); publisher wiring no longer routes through the deprecated getApplicationContext().
• DatastoreUtils: new executeWithNewSession(..) overloads (used by GormStaticApi); execute/doWithSession now stack via bindNewSession and clean up via the one canonical unbindSession, so a bound-but-empty holder can never fail a later bind.
• SessionHolder.getSessions() (used by destroy()), AbstractConnectionSourceFactory fallback-settings extraction, MultipleConnectionSourceCapableDatastore (used by GormApiResolver/AbstractGormApi/GormRegistry), and an AstUtils.copyAnnotations dedup guard needed by ServiceTransformation (with AstUtilsSpec coverage).

Note on SessionResolver.bind()/unbind(): resolve() is what core consumes (via hasCurrentSession()); bind/unbind complete the SPI contract that the per-adapter follow-up PRs implement (e.g. a Hibernate resolver bridging native SessionFactory-keyed bindings). They are specified, tested, and small; flagging rather than hiding that their production callers arrive with the adapter PRs.

Review response (2026-07-29 round on #15779)

1. Scope: Query.java, Service.groovy, DefaultServiceRegistrySpec, the AbstractPersistentEntity.getTenantId() fallback + isMultiTenant swap, and the createPropertyResolver cast are reverted. The CustomizableRollbackTransactionAttribute copy-semantics change is split into its own PR (fix: preserve full transaction attribute state in CustomizableRollbackTransactionAttribute copy constructors #16063, branch fix/customizable-rollback-tx-attribute-copy) with the reworked implementation per review (Spring copy constructors, label independence, no lazy-getter mutation of the source, timeoutString preserved, the GString-in-.java log fixes) and behavior-level tests through GrailsTransactionTemplate/DefaultTransactionService. The same lossy-copy-constructor pattern was also found in two sibling classes and fixed in follow-up PRs fix: preserve full transaction attribute state in GrailsTransactionAttribute (GORM) copy constructors #16064 (grails.gorm.transactions.GrailsTransactionAttribute) and fix: preserve full transaction attribute state in GrailsTransactionAttribute (web) copy constructors #16065 (org.grails.transaction.GrailsTransactionAttribute).
2. API duplication: the unbind sequence now exists exactly once (DatastoreUtils.unbindSession); executeWithNewSession, execute, doWithSession, and TransactionSynchronizationSessionResolver.unbind() all delegate to it (also fixing the TSM-key mismatch for sessions owned by a child datastore — regression test added). The AbstractDatastore event-publisher machinery (third publisher implementation + reflective listener registration) is removed entirely rather than moved: nothing in this stack or the adapter follow-ups calls it; concrete datastores keep publishing through their own ConfigurableApplicationEventPublisher, as today. getApplicationEventPublisher() stays null when no context is configured (no per-query event allocation for bare datastores — unchanged from 8.0.x).
3. Speculative API: removed as caller-less — Datastore.getSessionResolver() default method (allocated per call), AbstractConnectionSourceFactory.createSettings(PropertyResolver), the 3-arg ConnectionSourceSettingsBuilder constructor.

Inline-comment items: constructor this-escape fixed (lazy resolver); bind() rejects a session owned by a different datastore (spec added); resolve() uses validated sessions with disconnected-session and empty-holder specs; resolver class renamed since it holds no ThreadLocal; interface generics dropped; @author tags use a real name; unbind() keeps its name (matching DatastoreUtils.unbindSession, to which it now delegates) with the close-on-unbind contract stated explicitly in the interface javadoc.

Fixes from the post-consolidation contrarian review

An adversarial multi-agent review of the consolidated branch surfaced and fixed:

• DatastoreUtils.execute/doWithSession could throw IllegalStateException("Already value bound") when a bound-but-empty SessionHolder remained on the thread; they now stack via bindNewSession and release via unbindSession (spec added).
• TransactionSynchronizationSessionResolver.resolve() left an emptied holder bound (poisoning later binds) and returned null when a valid session sat beneath a stale one; it now resolves down the stack and unbinds an emptied non-transactional holder (specs added, including the transaction-owned case).
• GormStaticApi.withStatelessSession had been rewired through executeWithNewSession, silently handing out stateful sessions and dropping the UnsupportedOperationException guard; baseline connectStateless() behavior restored.
• GormStaticApi.saveAll force-flushed mid-transaction, deviating from the 8.0.x baseline it claimed to restore; the flush is removed.
• TenantDelegatingGormOperations.delete(instance, params) called save instead of delete — a pre-existing 8.0.x data-integrity bug in a touched class; fixed with a delegation spec that would have caught it.
• Cross-datastore tenant bleed: three selector paths in GormApiResolver read the tenant via no-arg CurrentTenantHolder.get() (an arbitrary datastore's tenant); they now pass the datastore being evaluated.
• Registry races: registerEntityDatastores rebuilt an entity's routing map with remove-then-repopulate (a concurrent lookup could observe no routing and fall back to the wrong datastore) — it now publishes the rebuilt map atomically; AbstractGormApiRegistry.getDirect could cache a qualified API derived from a superseded default API indefinitely — it now re-validates after publishing and retracts if superseded.
• Memory: GormRegistry.removeDatastore now clears the normalization caches (Class-keyed keys retain classloaders across dev reloads; the qualifier cache grew per tenant-ID ever seen) once the last datastore is gone.
• The ActiveSessionDatastoreSelector ≤10-datastore fallback scan is now documented as the only discovery path for non-Datastore-keyed sessions (e.g. Hibernate's SessionFactory keying) and as intentionally bounded.

Known limitations (deliberate, documented)

• Steady-state memory is still O(entities × qualifiers-actually-used) — lazily materialized, never eagerly allocated. The eager win is allocation/startup, not asymptotic worst-case residency.
• Beyond 10 registered datastores, unbound non-transactional Hibernate sessions are not discovered by the selector fallback (routing falls back to the entity's DEFAULT datastore); deployments at that scale should bind sessions explicitly (transactions / Tenants.withId).
• The six per-adapter follow-up branches predate this consolidation's resolver rework (renamed class, no setter, private field) and must be rebased onto it before review.

Test plan

• ./gradlew :grails-datastore-core:test :grails-datamapping-core:test :grails-data-simple:test — 0 failures
• ./gradlew :grails-data-hibernate5-core:test :grails-data-hibernate7-core:test :grails-data-mongodb-core:test — 0 failures
• codeStyle (Checkstyle + CodeNarc) clean

🤖 Generated with Claude Code

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

[borinquenkid]

The TestLens failures for GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default and GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer are a pre-existing bug on 8.0.x, unrelated to this PR's changes -- reproducible on plain 8.0.x today. DefaultStackTraceFilterer.STACK_LOG routes through a jcl-over-slf4j commons-logging binding, so the tests' System.setErr() capture never observes the emitted message.

Fix: #16067 (replaces the System.err capture with a Logback appender attached directly to the logger). Once that merges, rebasing onto 8.0.x should clear this failure here.

[jdaugherty]

Initial AI Review seems to have found a few issues (including breaking tests):

Thanks for consolidating the stack — having the API, its consumer and its tests in one place does make the registry design reviewable in a way the 3-PR split wasn't.

I've gone through the whole diff. The GormRegistry / GormApiFactory / DatastoreResolver core is a sound shape and the lazy (entity, qualifier) materialization is the right call. Most of my comments below are about things that came along for the ride and about behaviour changes that aren't called out in the description.

Three things I'd like resolved before this can move forward:

1. No documentation. 175 files and not one line in grails-doc. This PR adds public API (grails.gorm.multitenancy.CurrentTenantHolder, Tenants.withTenant(..), Tenants.withId(Class, ..), GormEntity.deleteAll()/deleteAll(Map), four new GormStaticOperations.deleteAll overloads, SessionResolver, MultipleConnectionSourceCapableDatastore.routesUnqualifiedToMappedConnection()) and removes public API (GormEnhancer.enhance(..), GormEnhancer.finders/transactionManager/dynamicEnhance, the 1-arg and 5-arg GormEnhancer constructors, GormStaticApi.multiTenancyMode, Tenants.withId(Class<? extends Datastore>, ..)). Every settable/callable thing we ship has to be documented, and the removals need upgrade notes — GormEnhancer is the extension point every out-of-tree GORM implementation subclasses, so "it's a major version" isn't sufficient on its own.

2. Unrelated fixes bundled in. getAll() ordering/null-slot semantics (both Hibernate 5 and 7), the HibernateQuery junction overrides, CriteriaBuilder.call(Closure) + the ensureQueryIsInitialized() additions, and the removeConstraints() rewrite are all independent behaviour changes. Several look correct and worth having, which is exactly why they should land as their own reviewable/revertable PRs rather than inside a 17k-line refactor. Please split them out.

3. A TCK test is being excluded rather than fixed — see the FirstAndLastMethodSpec comment.

CI being green is necessary but not sufficient here: grails-datamapping-rx is commented out of settings.gradle, so the Tenants.withId overload change isn't compiled anywhere, and the multi-tenant routing paths added in GormApiResolver are mostly covered by specs that drive the internal selectors directly rather than by an app-level scenario.

[jdaugherty]

This replaces withId(Class<? extends Datastore> datastoreClass, Serializable, Closure) with withId(Class domainClass, Serializable, Closure). Both erase to withId(Class, Serializable, Closure), so every existing caller that passes a datastore class keeps compiling and silently changes meaning — it now goes through datastoreLocator.getDatastoreForDomain(..) and resolves the datastore class as if it were a domain class.

There are already ~40 call sites of exactly that shape in-tree: RxGormStaticApi:596 (Tenants.withId((Class<RxDatastoreClient>) datastoreClient.getClass(), tenantId) { .. }) and TenantDelegatingRxGormOperations, which pass datastoreClientClass. grails-datamapping-rx is commented out in settings.gradle, so CI never compiles or runs them and won't catch this.

Please keep the datastore-class overload and give the domain-class variant a distinct name (withIdForDomain, or take the entity as a typed parameter). A silent same-erasure meaning change on a documented public multi-tenancy entry point isn't something users can be expected to notice.

[borinquenkid]

Traced the actual blast radius before picking a fix. The ~40 call sites cited as evidence (RxGormStaticApi, TenantDelegatingRxGormOperations) turned out to be a false positive — grails-datamapping-rx has its own, fully independent grails.gorm.rx.multitenancy.Tenants operating on RxDatastoreClient, untouched by this PR (grepped every import in that module to confirm zero references to the core Tenants class).

The real clash is in TenantDelegatingGormOperations's ~100 Tenants.withId((Class<Datastore>) datastore.getClass(), ...) calls, but that class turns out to be dead code on this branch: GormStaticApi.withTenant(Serializable) used to construct it directly, and this PR's rewrite replaced that with forQualifier(tenantId.toString()) instead — nothing else in the tree constructs it except its own unit test. Fixed anyway since it's cheap and removes the landmine if it's ever wired back up: added requireMultiTenantCapableDatastore() and switched all 100 sites to the existing, unambiguous Tenants.withId(MultiTenantCapableDatastore, Serializable, Closure) overload rather than a withIdForDomain rename — the domain-class overload has no live erasure conflict anywhere reachable, so a rename felt like a needless public API break for a theoretical case. Tenants.withId(Class domainClass, ...) itself is untouched.

Commit 4b116c2166. Also verified the withTenant() decorator→forQualifier mechanism swap doesn't change runtime behavior — GormRegistrySpec's DISCRIMINATOR-mode tests and PartitionedMultiTenancySpec's DATABASE-mode assertion both exercise the no-closure chained form against it and pass.

[jdaugherty]

deleteAll(Map params) discards params entirely and delegates to the unqualified deleteAll(). The interface javadoc added in GormStaticOperations says "Deletes all objects for the given arguments", so Book.deleteAll(flush: true) — or anything a user reasonably writes to narrow the delete — silently deletes every row instead.

Separately, I'd like explicit agreement on adding deleteAll()/deleteAll(Map) to GormStaticOperations and GormEntity at all. A no-argument "delete every row of this table" static on every domain class is a significant and destructive addition to the entity API surface; it isn't mentioned in the PR description and it isn't needed by the registry refactor. Either drop it from this PR and propose it on its own, or implement params properly (at minimum flush) and document both methods in grails-doc.

[borinquenkid]

Took the implement option, not drop — this is your own stated bar, and your fix/gorm-registry-review-feedback branch already met it, so matched it rather than reinventing: deleteAll(Map params) now honors params?.flush instead of discarding params, both deleteAll()/deleteAll(Map) are documented in grails-doc (new 'Deleting Every Instance' section in basicCRUD.adoc with an explicit destructive-semantics warning and a pointer to .where{}.deleteAll() for partial deletes), and TenantDelegatingGormOperations gets matching delegators via its own requireMultiTenantCapableDatastore() pattern (see the Tenants.withId thread) rather than a class-based fallback.

Commit e011e0896c. 3 new specs in GormStaticApiSpec (basic delete-everything, Map-returns-count, flush-honored via a Session mock) + 2 in TenantDelegatingGormOperationsSpec.

[jdaugherty]

registerEntity now calls addStaticMethods/addInstanceMethods unconditionally for every non-external entity, and those methods install catch-all mc.static.methodMissing, mc.static.propertyMissing, mc.methodMissing and both mc.propertyMissing variants on the entity's ExpandoMetaClass.

On 8.0.x this did not happen. addStaticMethods/addInstanceMethods were only reachable from enhance(..), enhance() was gated on dynamicEnhance, the settings constructor hard-coded this.dynamicEnhance = false, and nothing in the tree calls enhance(..). So this is new global metaclass mutation on the bootstrap path, not a like-for-like replacement.

Three consequences I'd like addressed:

• Assigning mc.methodMissing / mc.propertyMissing overwrites any handler an application or another plugin installed on that domain class. There's no merge or delegation to a previous handler.
• GormEntity already declares methodMissing, propertyMissing and staticPropertyMissing as real trait methods. An EMC closure takes precedence, so for Groovy entities the trait implementations become unreachable and the two dispatch paths can diverge.
• It forces an ExpandoMetaClass for every persistent entity at startup, and every dynamic-finder call now pays a metaclass miss plus registry.findStaticApi(cls, null) plus a dynamic api.invokeMethod(..). methodMissing results aren't installed into the metaclass the way registerInstanceMethod was, so that cost is per call, not once. For a PR whose headline is scaling, please include numbers for the steady-state dynamic-finder path, not just startup allocation.

If the goal is only to stop eagerly allocating APIs, this metaclass change looks separable from that and I'd rather see it justified on its own.

[borinquenkid]

Restored the dynamicEnhance-gated bootstrap exactly as it is on 8.0.x: registerEntity no longer calls addStaticMethods/addInstanceMethods unconditionally, enhance()/enhance(entity) are back and gated on dynamicEnhance (hard-coded false in the settings constructor, matching 8.0.x's own quirk), and nothing in the tree calls enhance(..) — confirmed by grepping the whole real 8.0.x tree, not just this PR's diff, for any caller that ever flips dynamicEnhance true. GormEntity's own trait methodMissing/propertyMissing hooks already cover all dispatch (83 spec classes stayed green with zero ExpandoMetaClass installed). Commit da09401c4d.

Separately adopted your instance-side clobber guard on addInstanceMethods (backs off with a debug log if a methodMissing handler is already installed) — commit 2dd18808ff. I did not adopt the static-side guard (getStaticMetaMethod('methodMissing', ...)): wrote a few isolated ExpandoMetaClass probe scripts (no GORM involved) and confirmed getStaticMetaMethod can never see a static methodMissing handler installed via the standard mc.static.methodMissing = {...} idiom, regardless of install order — even though the handler is functionally live (invokeStaticMethod proves it dispatches). Your branch has the identical static-side check; it looks like it never actually fires there either. Happy to share the repro scripts if useful — didn't want to ship a check that looks protective but is dead code.

[jdaugherty]

hasCurrentSession() goes from a pure TransactionSynchronizationManager.hasResource(this) check to getSessionResolver().resolve() != null, and resolve() mutates thread-bound state: it evicts disconnected sessions from the SessionHolder and calls unbindResourceIfPossible(datastore) when the holder ends up empty and isn't transaction-synchronized.

So a predicate now has side effects, and it's called from places that are purely interrogative — DatastoreUtils.execute/doWithSession, and ActiveSessionDatastoreSelector, which calls hasCurrentSession() on every registered datastore (up to the documented 10) while deciding where to route. A routing probe for entity A can now unbind datastore B's holder as a side effect.

I'd like the validating/evicting behaviour kept in resolve() and out of hasCurrentSession(), or a separate non-mutating query used for the routing scan. If agreeing with getCurrentSession() really requires validation here, please say so explicitly in the javadoc — "this method may unbind thread-bound resources" — and add a spec covering the discovery-scan case.

[borinquenkid]

Agreed the mutation shouldn't live in a predicate. Added SessionResolver.hasResolvedSession() — implemented by iterating SessionHolder.getSessions() (the genuinely read-only Collections.unmodifiableCollection view) checking isConnected(), same accuracy as resolve() but zero mutation. AbstractDatastore.hasCurrentSession() now delegates to that instead of resolve() != null. Commit 3707a82f63.

Audited all 8 call sites first: the 2 ActiveSessionDatastoreSelector routing-scan sites needed the pure check; the other 6 all immediately call getCurrentSession() right after anyway, which independently re-validates via its own getValidatedSession(), so none of them were actually relying on the old eviction side effect. Added a regression spec proving a hasCurrentSession() probe on datastore A leaves datastore B's session binding completely untouched (present, not evicted).

[jdaugherty]

This widens the @PendingFeatureIf from hibernate5 to hibernate5-or-hibernate7, i.e. a TCK test that passes on 8.0.x for Hibernate 7 is now excluded. The reason still reads "Was previously @ignore", which no longer describes what's happening.

This looks like a direct consequence of removing the first(Map)/last(Map) overrides from HibernateGormStaticApi in the Hibernate 7 module, which sends composite-key first/last through the generic GormStaticApi implementation instead. Please fix the regression rather than widen the exclusion — the TCK is our contract for adapter parity, and quietly moving a passing test into pending hides the behaviour change from anyone reading the PR.

[borinquenkid]

Fixed the regression instead of widening the exclusion. Root cause: GormStaticApi.first(Map)/last(Map) (shared, not H7-specific — H5 lost the same override at merge-base too, both now route through the generic method) force-applies max:1/order at the DB level even when a composite-key entity has no derivable identity to sort by, so it returned an arbitrary row instead of falling back to natural/insertion order. Fixed generically: only force max/order when a sort key exists (user-supplied or derived from a simple identity); otherwise fetch normally and index [0]/[-1], matching what the removed H7-specific override used to do. Reverted the @PendingFeatureIf back to its original H5-only condition — H7's composite-key case now passes 12/12; H5's own separate pre-existing skip is correctly untouched. Commit da09401c4d.

[jdaugherty]

This adds a method to a public SPI whose javadoc defines the contract in terms of a test double: "The in-memory mock used for unit testing manages a single session on this (parent) datastore … otherwise they would target a child whose session the test harness never flushes." SimpleMapDatastore is the only override, and it returns false for exactly that reason.

Production SPI shouldn't exist to accommodate the unit-test datastore's session model. Either fix SimpleMapDatastore so its per-connection children behave like real ones (the harness flushes what it creates), or keep the special-casing inside GormRegistry/SimpleMapDatastore rather than putting it on the interface every third-party GORM implementation has to reason about.

If it does stay, it's a new public interface method and needs documenting, and the javadoc should describe the semantics abstractly rather than referring to our test harness.

[borinquenkid]

Removed it from the public SPI entirely rather than fixing SimpleMapDatastore's session model — agreed it shouldn't be third-party datastores' problem to reason about. Added a narrow SingleSessionCapableDatastore marker interface in grails-datastore-core (javadoc explicitly says it's not part of the public multi-connection contract, don't implement it for production datastores); SimpleMapDatastore implements it instead of overriding a method on the shared interface. GormRegistry's one call site collapsed from a double-negative instanceof+method check to a single instanceof SingleSessionCapableDatastore. Commit da09401c4d.

[jdaugherty]

This changes getAll() semantics for Hibernate 7 (and the same change is made in Hibernate 5): results are now reassembled in requested-id order, duplicate ids are preserved, and unresolvable ids produce null slots instead of being absent. That's a real improvement over returning whatever order the database chose — but it's a user-visible behaviour change with nothing to do with the registry refactor, and there's no TCK coverage asserting the new contract.

Please pull it into its own PR with TCK tests (order preserved, duplicates preserved, missing id yields a null at the right index, empty input). The same goes for the HibernateQuery disjunction()/conjunction()/negation() overrides added in this PR — the bug you describe there (a factory junction added to the unused base criteria field, so countByXOrY loses its disjunction) sounds like a genuine data-correctness bug that deserves its own PR, its own test, and a backport decision.

One concrete thing to check in this implementation: entitiesById is keyed by session.getIdentifier(entity) but looked up by the converted requestedId. convertToIdentifierType falls back to returning the raw key when the ConversionService can't convert it, so an id that Hibernate itself would have coerced now misses the map and silently yields a null slot instead of the entity. Previously the raw key went into the query and Hibernate did the coercion.

[borinquenkid]

Kept these rather than splitting them out — traced each one against real git blame/TCK history before deciding, rather than against just the PR description's framing:

• getAll() order/null-slot semantics: the shared TCK's GormEnhancerSpec (zero diff from 8.0.x) already asserts 'Test getAll preserves the supplied id order' and 'returns a null slot for a missing id' — the old H5/H7 code was structurally incapable of passing either. This is a pre-existing 8.0.x contract violation the registry work surfaced and fixed, not new undocumented behavior — the coverage already existed, it just hadn't been cross-referenced against an already-large diff. I did fix the specific gap you named, though: entitiesById is now consistently keyed/looked-up by String.valueOf(id) on both sides, so a raw requestedId that convertToIdentifierType couldn't convert (but Hibernate itself would have coerced) now resolves instead of silently yielding a null slot. Applied to both H5 and H7.
• HibernateQuery's disjunction()/conjunction()/negation(): small, self-contained, a genuine correctness bug — core's base implementations wrote to an unused field, silently dropping countByXOrY's OR. Reverting would reintroduce silently-wrong query results. It didn't have a dedicated test before this; happy to add one if that's what would unblock keeping it here instead of splitting it out.
• CriteriaBuilder's ensureQueryIsInitialized() guards ship with their own dedicated commit and regression test (a real NPE from a bare createCriteria() call); getPersistentEntity() is load-bearing for DynamicFinder's own machinery in this same refactor, not optional; list(Closure)/call(Closure) formalize dispatch that already worked via invokeMethod pre-PR and is exercised today by pre-existing, unchanged tests.
• removeConstraints(): this is the undo-counterpart of the double-constraint-registration bug flagged separately on the GormRegistry.groovy thread — same fix, not a separate concern.

Genuinely open to splitting any one of these into its own PR if the trace above doesn't change your read — wanted to give you the actual evidence rather than just assert they're fine and ask you to trust it.

[jdaugherty]

catch (Exception e) { it.remove() } — on any exception from getDatastore() this silently evicts the entity's registered API, and the same pattern repeats at line 160 for the qualified map. e is unused, so nothing is logged either.

That turns a transient failure into permanent loss of the entity's API registration: the next lookup finds nothing and either throws "No GORM implementation configured" or falls back to a different datastore. Given getDatastore() now routes through datastoreResolver.resolve(), which for a tenant-aware resolver can throw TenantNotFoundException when no tenant is bound, this is reachable from an ordinary unresolved-tenant call rather than only from a corrupt-state edge case.

Please only remove on a positive identity match, and log at warn (with e) when the datastore can't be read.

[borinquenkid]

Confirmed and fixed exactly as described. removeDatastore()'s catch (Exception e) { it.remove() } (both the default and qualified maps) could evict an unrelated entity's API registration just because reading its datastore happened to throw at that moment — reachable from an ordinary unresolved-tenant condition, since getDatastore() routes through a DatastoreResolver that can throw TenantNotFoundException when no tenant is bound. Reproduced first as a failing test (temporarily reverted the fix, confirmed both new specs failed as expected against the original catch-and-remove code). Extracted a belongsTo(api, datastore, className, qualifier) helper — only a positive identity match removes; on exception, logs at warn with the exception and which entity/qualifier was affected, returns false (kept, not evicted). Commit efe62b0ca2, matches your branch's fix. 3 new specs covering cross-entity eviction stays scoped correctly and both throwing-API survival cases.

[jdaugherty]

This makes copyAnnotations skip any annotation already present on the target, for every caller of this shared utility. Repeatable annotations legitimately appear more than once, and for those the guard now silently drops all but the first — the annotated node ends up with an incomplete set and there's no diagnostic.

The PR describes this as a dedup guard needed by ServiceTransformation. Please scope it there rather than changing the semantics of a general-purpose AST helper — either dedup at the ServiceTransformation call site, or add a boolean/Set<String> parameter so callers opt in. If it must be global, exclude annotations meta-annotated @Repeatable.

[borinquenkid]

Scoped it to the caller rather than changing the shared helper's semantics globally. Confirmed ServiceTransformation is the only real caller of this method in the whole repo (an earlier grep hit on GrailsASTUtils.copyAnnotations was a false positive — a completely different class in a completely different module). Added a 5th skipExisting boolean parameter defaulting to false via the existing convenience overloads; ServiceTransformation's two call sites now explicitly pass skipExisting=true to preserve its own original behavior (avoiding double-adding annotations it may already have written). Commit fda9664613, matches your branch and your spec's shape — repeatable annotations copy fully by default, skipExisting=true dedupes, the omitted default keeps both copies.

[jdaugherty]

The javadoc says "This method will create a new datastore session for the scope of the call and hence is designed to be used to manage the connection life cycle", but the implementation only nests two CurrentTenantHolder.withTenant calls — it sets thread-locals and never opens a session. Same for withTenant(Serializable, Closure) at line 68.

That's the documented difference between Tenants.withId and this method, so users following the javadoc will assume connection management they aren't getting. Please either implement it (delegate to withId after resolving the datastore) or rewrite the javadoc to say plainly that it only binds the tenant to the current thread and that the caller owns the session — and explain in grails-doc when to reach for withTenant versus withId, because the names give no hint.

[borinquenkid]

Confirmed the javadoc was simply wrong, not just imprecise — withTenant's body only nests two CurrentTenantHolder.withTenant calls (thread-local set/remove), no session anywhere in the call chain, unlike withId which genuinely does withNewSession/withSession. Rewrote both withTenant overloads' javadoc to say plainly: binds the tenant id only, opens no session, caller owns the session the closure runs in, with a pointer to the corresponding withId overload for when a real session is actually needed. Commit ebad67a331. The withTenant-vs-withId guidance for grails-doc you asked for here is in the new multiTenancy.adoc page — explicit section on when to reach for each.

[jdaugherty]

I used AI to take a pass at this here: https://github.com/apache/grails-core/tree/fix/gorm-registry-review-feedback , I don't think this fully addresses everything but it may be a starting point to save time.

[borinquenkid]

thanks for the branch, I will compare to my in flight solutions, which I am doing with a tooth comb. Walter

…

On Aug 5, 2026, at 7:44 AM, James Daugherty ***@***.***> wrote: jdaugherty left a comment (apache/grails-core#16066) <#16066 (comment)> I used AI to take a pass at this here: https://github.com/apache/grails-core/tree/fix/gorm-registry-review-feedback , I don't think this fully addresses everything but it may be a starting point to save time. — Reply to this email directly, view it on GitHub <#16066?email_source=notifications&email_token=AADSQL3MDYNGEZ2NV27VODD5IMT4FA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJZGE4DONJUGM2KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5191875434>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADSQL5QGQAFRGNJBKQ56CT5IMT4FAVCNFSNUABCKJSXA33TNF2G64TZHM2TCMRSHE2TWSLTON2WKOZVGAYTEMRTG4ZDMMVBOYBA>. You are receiving this because you authored the thread.

[borinquenkid]

Went through every comment individually (replies below on each thread) and also diffed this branch against your own AI-assisted pass at the same review (fix/gorm-registry-review-feedback, 4d2b832628) for every case where the two of us landed on different fixes, so I wasn't re-deriving something you'd already solved or contradicting your own diff without checking it first. Full reasoning trail (including the cases where I kept my approach over yours, with the evidence) is written up as I went; happy to share that working doc if useful.

Blocker 1 (no documentation) — resolved. New grails-doc/.../GORM/multiTenancy.adoc covering Tenants and CurrentTenantHolder end to end, plus a new upgrade-note section in upgrading80x.adoc. I didn't adapt your upgrade-notes draft wholesale — ran a diff of GormEnhancer's actual current surface against real origin/8.0.x first, since a few of the removals your draft describes (enhance(), dynamicEnhance, the constructors, multiTenancyMode) are restored on this branch and would be actively wrong to document as gone.

Blocker 2 (unrelated fixes bundled in) — not split out. Traced each of the four (getAll() ordering/null-slot semantics, the HibernateQuery junction overrides, CriteriaBuilder's guards, removeConstraints()) against git blame and the shared TCK before deciding, rather than against the PR description's framing. Detail is on the HibernateSession.java thread below, but short version: three of the four are demonstrated pre-existing 8.0.x contract violations this refactor surfaced and fixed (with existing, unchanged TCK coverage proving the contract), and the fourth (removeConstraints) is the direct undo-counterpart of the double-constraint-registration bug you flagged separately. Genuinely open to splitting any one of them out if the trace doesn't change your mind.

Blocker 3 (TCK exclusion instead of a fix) — fixed for real. Root cause was the shared GormStaticApi.first(Map)/last(Map) losing an H7-specific override at merge-base (not H7-specific after all — H5 has the same gap). Fixed generically, exclusion reverted, composite-key H7 case passes 12/12 again.

The CI-blind-spot note — TenantDelegatingGormOperations's ~100 class-based Tenants.withId call sites are fixed (detail on the Tenants.groovy:229 thread) — though it turns out that class is dead code on this branch, nothing currently constructs it outside its own test. Confirmed grails-datamapping-rx has zero diff in this PR and uses its own fully independent Tenants class, so there's no actual compilation gap there. On GormApiAllocationSpec driving internal selectors: real, still-open gap on the DATABASE-mode qualified-routing case specifically — documented as a recommended follow-up rather than closing it with an unreviewed new spec.

Two things worth a look on your own branch, found while cross-checking (not blockers, just flagging): the getMultiTenancyMode() rationale (on the MongoStaticApi.groovy thread) doesn't hold against your own GormRegistry.createStaticApi; and the static-side methodMissing clobber guard (on the GormEnhancer.groovy thread) is unreachable dead code on both our branches — confirmed with a few isolated ExpandoMetaClass probes, happy to share them.

Separately: just merged current 8.0.x into this branch (191 commits since the fork point) to keep it current before this goes further — one doc-section-numbering conflict, resolved; full suite green after.

[codecov]

Codecov Report

❌ Patch coverage is 84.63368% with 323 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.2808%. Comparing base (a1e526f) to head (bc54db5).
⚠️ Report is 19 commits behind head on 8.0.x.

Files with missing lines	Patch %	Lines
...oovy/org/grails/datastore/gorm/GormRegistry.groovy	81.2665%	24 Missing and 47 partials ⚠️
...ovy/org/grails/datastore/gorm/GormStaticApi.groovy	80.8765%	21 Missing and 27 partials ⚠️
...y/org/grails/datastore/gorm/GormApiResolver.groovy	81.3830%	4 Missing and 31 partials ⚠️
...oovy/org/grails/datastore/gorm/GormEnhancer.groovy	72.9167%	10 Missing and 16 partials ⚠️
...g/grails/datastore/gorm/finders/DynamicFinder.java	44.4444%	14 Missing and 6 partials ⚠️
...y/org/grails/datastore/gorm/GormInstanceApi.groovy	85.2632%	2 Missing and 12 partials ⚠️
...org/grails/datastore/gorm/GormValidationApi.groovy	80.5970%	1 Missing and 12 partials ⚠️
...ails/datastore/gorm/AbstractGormApiRegistry.groovy	86.8421%	3 Missing and 7 partials ⚠️
...ansactions/transform/TransactionalTransform.groovy	90.9910%	1 Missing and 9 partials ⚠️
...y/org/grails/datastore/gorm/AbstractGormApi.groovy	85.4839%	0 Missing and 9 partials ⚠️
... and 20 more

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16066        +/-   ##
==================================================
+ Coverage     52.3431%   53.2808%   +0.9377%     
- Complexity      18296      19136       +840     
==================================================
  Files            2036       2049        +13     
  Lines           96347      97508      +1161     
  Branches        16829      17129       +300     
==================================================
+ Hits            50431      51953      +1522     
+ Misses          38492      38037       -455     
- Partials         7424       7518        +94     

Files with missing lines	Coverage Δ
...ls/datastore/gorm/mongo/MongoGormApiFactory.groovy	100.0000% <100.0000%> (ø)
...ails/datastore/gorm/mongo/MongoGormEnhancer.groovy	72.2222% <100.0000%> (+3.4722%)	⬆️
...rc/main/groovy/grails/gorm/DetachedCriteria.groovy	82.2368% <100.0000%> (+0.6579%)	⬆️
...rails/gorm/multitenancy/CurrentTenantHolder.groovy	100.0000% <100.0000%> (ø)
...gorm/transactions/GrailsTransactionTemplate.groovy	90.6977% <100.0000%> (+0.4538%)	⬆️
.../grails/datastore/gorm/AbstractDatastoreApi.groovy	100.0000% <100.0000%> (+61.5385%)	⬆️
...grails/datastore/gorm/DefaultGormApiFactory.groovy	100.0000% <100.0000%> (ø)
.../grails/datastore/gorm/GormEnhancerRegistry.groovy	100.0000% <100.0000%> (ø)
...ails/datastore/gorm/GormInstanceApiRegistry.groovy	100.0000% <100.0000%> (ø)
...grails/datastore/gorm/GormStaticApiRegistry.groovy	100.0000% <100.0000%> (ø)
... and 54 more

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: bc54db5
▶️ Tests: 72132 executed
⚪️ Checks: 62/62 completed

---

Learn more about TestLens at testlens.app.