(1/3) feat: add SessionResolver infrastructure and extend core datastore APIs

[borinquenkid]

📖 Reading order: 1 of 3 — this GormRegistry O(M+N) scaling change is split across three PRs; please review them in order:

1. (1/3) (1/3) feat: add SessionResolver infrastructure and extend core datastore APIs #15779 — SessionResolver infrastructure & core datastore API extensions
2. (2/3) (2/3) feat: GORM O(M+N) scaling — GormRegistry, GormEnhancer, and core API refactor #15780 — GormRegistry implementation (core production code)
3. (3/3) (3/3) test: add core-class unit tests for the GormRegistry O(M+N) rewrite #15790 — core-class unit tests

You are reading (1/3).

---

Summary

Prerequisite for the GormRegistry O(M+N) scaling stack. Introduces low-level infrastructure in grails-datastore-core that the GormRegistry layer depends on:

• Add SessionResolver and ThreadLocalSessionResolver for thread-safe session lookup decoupled from a specific Datastore reference
• Extend AbstractDatastore, Datastore, and DatastoreUtils with hooks for registry lifecycle events (register/deregister on open/close)
• Add MappingContext and AbstractMappingContext extensions needed by GormRegistry for entity lookup
• Minor additions to AbstractPersistentEntity, ClassUtils, DirtyCheckingSupport, ConnectionSourceSettingsBuilder, CustomizableRollbackTransactionAttribute, AstUtils

This PR carries no GORM API changes and has no dependency on any GormRegistry class — it can be reviewed and merged independently.

---

Relationship to the upstream fix

This stack builds on top of fix/gorm-api-registration-scaling, which solves the same O(M×N) problem through a different strategy. The two approaches are deliberately complementary:

Upstream fix strategy — incremental optimization (4 files, ~815 lines)

The partner's fix adds apiQualifiers() to GormEnhancer to split tenant qualifiers into a canonical eager set and a lazy-on-demand set, then introduces an internal GormEnhancerRegistry that allocates per-qualifier API providers via computeIfAbsent. This collapses startup allocation from O(entityCount × tenantCount) to O(entityCount + tenantCount) while keeping GormEnhancer as the sole stateholder. Two follow-up commits fix TSM consistency bugs that surfaced from the lazy-allocation: a stale ConnectionHolder in GrailsHibernateTemplate.executeWithNewSession (DATABASE mode) and stale cached APIs after addTenantForSchema re-creates a child datastore (SCHEMA mode).

Footprint: all changes contained within GormEnhancer.groovy and GrailsHibernateTemplate.java (H5 + H7). No new public types.

This stack strategy — registry-first architecture (251 files across 9 PRs)

This stack extracts all per-entity, per-qualifier API state from GormEnhancer into a dedicated GormRegistry singleton. GormEnhancer becomes stateless — it delegates entity registration, API lookup, and datastore lifecycle to the registry. APIs are created at entity-registration time by a pluggable GormApiFactory (one implementation per datastore type), then looked up O(1) by (entityClass, qualifier) at call time.

Key differences from the upstream fix:

Dimension	Upstream fix	This stack
Complexity	O(M+N) via lazy computeIfAbsent	O(M+N) via eager factory allocation at registration time
New public types	None	GormRegistry, GormApiFactory, SessionResolver
Stale-entry eviction	Explicit eviction heuristic in registerEntity	Not needed — registry owns the authoritative copy
Lifecycle	No change	Datastore open/close fires register/deregister events on the registry
Pluggability	Single allocation path	Each datastore registers its own GormApiFactory
Scope	GormEnhancer + H5/H7 template	All datastore adapter modules

Conflict resolution: our GormRegistry supersedes the upstream GormEnhancerRegistry. When rebasing this stack onto the upstream fix, conflicts in GormEnhancer.groovy are always resolved in favour of our version — the registry approach provides a strict superset of the optimization.

---

Review response

See this comment for the full writeup responding to @jdaugherty's review. Short version: SessionResolver no longer maintains independent state (it's now a stateless view over the same SessionHolder/TSM store DatastoreUtils already uses), the public API surface is narrower (Datastore.getSessionResolver() is a default method, MappingContext.initialize() is back to protected), several unrelated behavioral changes were reverted out of this PR, and every touched class has new/updated test coverage for the gaps called out as untested.

---

Test plan

• ./gradlew :grails-datastore-core:test passes
• New/updated specs: AbstractDatastoreSpec, SessionResolverIntegrationSpec, ThreadLocalSessionResolverSpec, DatastoreUtilsSpec, AbstractConnectionSourceFactorySpec, CustomizableRollbackTransactionAttributeSpec
• Full sweep: grails-datastore-core, grails-datamapping-core, grails-data-mongodb-core, grails-data-simple, grails-data-hibernate5-core, grails-data-hibernate7-core — BUILD SUCCESSFUL, 0 failures
• codeStyle (Checkstyle/CodeNarc) clean on grails-datastore-core and grails-data-mongodb-core

Stack

This is the prereq for:

• feat/gorm-registry-core-impl — GormRegistry implementation
• feat/gorm-registry-core-tests — GormRegistry test suite
• Per-adapter wiring PRs: H5, MongoDB, Neo4j, Simple, misc, H7

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Introduces foundational datastore-layer infrastructure needed for the upcoming GormRegistry O(M+N) scaling work by adding session-resolution abstractions, expanding mapping-context lifecycle hooks, and tightening several low-level behaviors across datastore + Hibernate adapters.

Changes:

• Add SessionResolver + default ThreadLocalSessionResolver, and wire session lookup into Datastore/AbstractDatastore and DatastoreUtils.
• Extend MappingContext/AbstractMappingContext initialization and multi-tenancy configurability; adjust key mapping-context implementations accordingly.
• Refine supporting utilities and behaviors (query pre-event source, dirty-checking semantics, transaction attribute copying, Hibernate template TSM handling) and add/adjust targeted tests.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/services/DefaultServiceRegistrySpec.groovy	Adjust test service stub to include a datastore property.
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/core/ThreadLocalSessionResolverSpec.groovy	New unit spec covering basic bind/resolve/unbind for ThreadLocalSessionResolver.
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/core/SessionResolverIntegrationSpec.groovy	New spec exercising resolver availability through an AbstractDatastore subclass.
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/core/AbstractDatastoreSpec.groovy	New spec validating event publisher behavior and session creation event publication.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/CustomizableRollbackTransactionAttribute.java	Improve copying semantics (incl. qualifier/rollbackRules/connection) and fix trace logging strings.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/services/Service.groovy	Minor formatting-only adjustment.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/ClassUtils.java	Add getIntegerFromMap helper.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy	Avoid duplicating annotations when copying to a target node.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/query/Query.java	Update PreQueryEvent creation to use (source, query) constructor.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/MappingContext.java	Add initialize(ConnectionSourceSettings) and setMultiTenancyMode(...) hooks.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/AbstractPersistentEntity.java	Lazily resolve tenantId property and fix multi-tenant check call site.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/AbstractMappingContext.java	Make initialize public and add setter for multi-tenancy mode.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/keyvalue/mapping/config/KeyValueMappingContext.java	Switch to GormMappingConfigurationStrategy and route through updated initialization.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/document/config/DocumentMappingContext.java	Align initialize visibility with new MappingContext contract.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy	Expand dirty-checking to detect changes within initialized collections and DirtyCheckableCollection.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/ThreadLocalSessionResolver.groovy	New default session resolver implementation.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/SessionResolver.groovy	New session-resolver SPI.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/DatastoreUtils.java	Prefer resolver-based session lookup; add execute-with-new-session helpers; adjust session binding behavior and a generic array cast.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/Datastore.java	Add getSessionResolver() to the datastore contract.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/connections/ConnectionSourceSettingsBuilder.groovy	Add constructor supporting fallback configuration.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/connections/AbstractConnectionSourceFactory.java	Add createSettings(PropertyResolver) convenience.
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java	Add default event publisher + listener wiring, session resolver field, destroy-time unbind, and updated “current session” logic.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormEnhancerAllQualifiersSpec.groovy	Expand tests around qualifier allocation, lazy API creation, and close semantics.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEnhancer.groovy	Add enhancer registry, eager-vs-lazy API qualifier separation, lazy API initialization, and safer close semantics.
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java	Align initialize visibility with new mapping-context contract.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/GrailsHibernateTemplateSpec.groovy	Add regression coverage for TSM restoration when DS is bound without SF.
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/connections/GormApiAllocationSpec.groovy	New spec verifying lazy API allocation behavior across multi-tenancy modes.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java	Unbind/rebind DataSource holders independently of SessionFactory holders in executeWithNewSession.
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/GrailsHibernateTemplateSpec.groovy	New regression test for TSM restoration in Hibernate 5 template.
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/GormApiAllocationSpec.groovy	New spec verifying lazy API allocation behavior across multi-tenancy modes (Hibernate 5).
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java	Same TSM unbind/rebind fix as Hibernate 7 template.
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateDatastore.java	Trivial whitespace removal.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.1080%. Comparing base (ef09bcd) to head (ce346f0).
⚠️ Report is 122 commits behind head on 8.0.x.

Files with missing lines	Patch %	Lines
...ails/datastore/mapping/core/AbstractDatastore.java	88.6364%	1 Missing and 4 partials ⚠️
...ions/CustomizableRollbackTransactionAttribute.java	79.1667%	3 Missing and 2 partials ⚠️
...ore/mapping/core/ThreadLocalSessionResolver.groovy	88.8889%	0 Missing and 2 partials ⚠️
.../grails/datastore/mapping/core/DatastoreUtils.java	92.3077%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #15779        +/-   ##
==================================================
+ Coverage     49.4833%   51.1080%   +1.6247%     
- Complexity      16883      17459       +576     
==================================================
  Files            1998       2014        +16     
  Lines           93672      94310       +638     
  Branches        16400      16482        +82     
==================================================
+ Hits            46352      48200      +1848     
+ Misses          40169      38899      -1270     
- Partials         7151       7211        +60     

Files with missing lines	Coverage Δ
...y/org/grails/datastore/mapping/core/Datastore.java	100.0000% <100.0000%> (ø)
...e/connections/AbstractConnectionSourceFactory.java	95.6522% <100.0000%> (+27.2311%)	⬆️
...connections/ConnectionSourceSettingsBuilder.groovy	100.0000% <100.0000%> (ø)
...astore/mapping/model/AbstractPersistentEntity.java	76.5027% <100.0000%> (+0.9346%)	⬆️
...oovy/org/grails/datastore/mapping/query/Query.java	83.2184% <100.0000%> (+2.0690%)	⬆️
.../datastore/mapping/transactions/SessionHolder.java	82.3529% <100.0000%> (+9.6257%)	⬆️
.../grails/datastore/mapping/core/DatastoreUtils.java	65.5319% <92.3077%> (+3.2001%)	⬆️
...ore/mapping/core/ThreadLocalSessionResolver.groovy	88.8889% <88.8889%> (ø)
...ails/datastore/mapping/core/AbstractDatastore.java	78.9474% <88.6364%> (+6.3446%)	⬆️
...ions/CustomizableRollbackTransactionAttribute.java	81.3559% <79.1667%> (+25.3559%)	⬆️

... and 111 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.

[jdaugherty]

Thanks for the work on this. I do not think the PR is ready to merge in its current form.

The primary concern is architectural: SessionResolver introduces a second independently mutable source of truth alongside TransactionSynchronizationManager and SessionHolder, then gives it precedence during lookup. This can cause work to execute on one session while transaction commit or rollback operates on another, and it breaks nested and executeWithNewSession semantics. I recommend keeping SessionHolder as the authoritative session stack and composing the resolver as an adapter over it.

The event-publisher changes have a similar parallel-state problem. Concrete datastores publish through their own publisher fields, while the new listener API registers against the superclass publisher. Using a datastore-owned Spring multicaster with an optional outbound publisher delegate would provide one consistent path without custom dispatch or reflection.

The PR also widens public interfaces with mutable lifecycle methods and includes several unrelated behavioral changes, including key-value JPA mapping, tenant metadata, dirty checking, AST annotation copying, and transaction-attribute copying. These should be removed or split into focused PRs with dedicated tests.

The module tests pass, but they do not cover the transaction precedence, nested-session restoration, concrete datastore publisher, or cleanup scenarios raised in the inline comments. My recommendation is to address the session and event architecture first, narrow the public API changes, and separate the unrelated behavior changes before proceeding.

[jdaugherty]

This placeholder comment ("In a production scenario, we’d need to...") signals the design is incomplete and should not ship as the default core implementation. A single value also cannot support nested scopes: bind(A); bind(B); unbind() leaves no current session instead of restoring A, while the existing SessionHolder already has push/pop semantics. Qualified bindings have the same replacement problem, and plain unbind() unexpectedly clears every qualified binding as well. Prefer composing the default resolver over TSM/SessionHolder; if an independent context is genuinely required, it needs scoped stack semantics and automatic cleanup tied to session close.

[jdaugherty]

The independent ThreadLocal source has been replaced with the shared SessionHolder, but nested resolver cleanup is still incomplete. bind() pushes onto the holder while unbind() removes the entire holder, so bind(A); bind(B); unbind() loses A instead of restoring it. I am leaving this thread open and adding a current-line comment.

[borinquenkid]

Replied on the current-line thread you split this into — see the comment on ThreadLocalSessionResolver.groovy re: bind()/unbind(). Fixed in 48c9c49.

[borinquenkid]

@jdaugherty Thanks for the thorough review — pushed a commit (23f6b48) addressing all of it.

Session/event architecture (your primary concern): SessionResolver/ThreadLocalSessionResolver no longer maintain independent ThreadLocal state. They're now a thin, stateless view over the same SessionHolder/TransactionSynchronizationManager store DatastoreUtils already uses, so resolve() can't disagree with the transactional session. doGetSession() no longer short-circuits through the resolver before transaction-sync registration and session validation — that shortcut was bypassing both, plus the allowCreate contract, as you flagged. hasCurrentSession() collapses to a single check now that both sides read the same state. Nested scopes fall out of SessionHolder's existing stack for free, so I dropped the unfinished, unused qualifier resolve(String)/bind(String, S) surface rather than redesigning a feature with zero callers anywhere in the codebase.

Event publisher: replaced the hand-rolled dispatch with one composing SimpleApplicationEventMulticaster. addApplicationListener() now routes through getApplicationEventPublisher() (virtual) instead of the raw field, so it reaches whatever publisher a subclass (Mongo/Hibernate/Neo4j) actually publishes through — fixes the disconnect you called out without touching those modules. Also fixed the triple-assignment and the setApplicationContext(null)-discards-a-custom-publisher bug.

API surface: Datastore.getSessionResolver() is now a default method — safe now that the resolver holds no private state to lose. MappingContext.initialize() is back to protected, not promoted onto the public interface; nothing needed the promotion (checked #15780/#15790, zero callers).

Split out of this PR (reverted, all untested and unrelated to SessionResolver): the KeyValueMappingContext JPA-strategy swap, the DirtyCheckingSupport O(elements)/transitive-dirty change, and the AstUtils annotation-dedup change. Also dropped setMultiTenancyMode/getIntegerFromMap — zero callers.

Other fixes: bindSession()/bindSession(creator) fail fast again on a double-bind (bindNewSession already provides the stacking behavior for executeWithNewSession, so nothing lost); CustomizableRollbackTransactionAttribute deep-copies rollback rules instead of aliasing the source's list, and now copies labels too; AbstractConnectionSourceFactory.createSettings() composes the same fallback-settings path create() uses, so it picks up the injected TenantResolver/custom types; @PreDestroy now closes every session it finds instead of dropping the reference; deduplicated the executeWithNewSession void-overload.

Every touched class has new/updated Spock coverage for exactly the gaps you called out as untested — transaction precedence, nested-session restoration, concrete-datastore publisher wiring, @PreDestroy cleanup. Full sweep across grails-datastore-core, grails-datamapping-core, grails-data-mongodb-core, grails-data-simple, grails-data-hibernate5-core, and grails-data-hibernate7-core: BUILD SUCCESSFUL, 0 failures.

I checked the blast radius against #15780/#15790 before touching the resolver: neither depends on anything here beyond Datastore#hasCurrentSession() (used in GormApiResolver.select()'s fallback), and that's now strictly more correct than before, not just differently shaped.

[jdaugherty]

bind() pushes onto the existing SessionHolder, but unbind() removes the entire holder. This still breaks nested scope restoration: after bind(A); bind(B); unbind(), resolve() returns null instead of A, and both sessions are detached without either being closed. The new nested test currently asserts this destructive behavior. Please remove/close only the top session and leave the outer binding intact.

[borinquenkid]

Fixed in 48c9c49. unbind() no longer calls unbindResourceIfPossible() unconditionally — it now pops and closes only the top session:

void unbind() {
    SessionHolder holder = (SessionHolder) TransactionSynchronizationManager.getResource(datastore)
    if (holder == null) {
        return
    }
    Session session = holder.getSession()
    if (session != null) {
        holder.removeSession(session)
    }
    if (holder.isEmpty()) {
        TransactionSynchronizationManager.unbindResourceIfPossible(datastore)
    }
    if (session != null) {
        DatastoreUtils.closeSessionOrRegisterDeferredClose(session, datastore)
    }
}

Same removeSession()/isEmpty()/closeSessionOrRegisterDeferredClose() sequence DatastoreUtils.executeWithNewSession already used, so it's consistent with the rest of the session-binding code rather than a new pattern.

The nested test now asserts restoration instead of the destructive behavior:

when:
resolver.bind(first)
resolver.bind(second)

then: "resolve() returns the most recently bound (top-of-stack) session"
resolver.resolve() == second

when:
resolver.unbind()

then: "unbind() closes and pops only the top session, restoring the outer binding"
1 * second.disconnect()
0 * first.disconnect()
resolver.resolve() == first

when:
resolver.unbind()

then: "unbinding the last remaining session closes it and clears the binding entirely"
1 * first.disconnect()
resolver.resolve() == null

bind(A); bind(B); unbind() now leaves resolve() == A, with only B closed — verified this actually passes locally, not just trusting the commit message.

[jdaugherty]

hasCurrentSession() uses the replaceable sessionResolver, but getCurrentSession() still delegates to DatastoreUtils.doGetSession(), which reads only TSM/SessionHolder. A custom resolver can therefore make this return true while getCurrentSession() throws or returns a different session. This also breaks DatastoreUtils.execute(), which branches here and then calls getCurrentSession(). Either remove the alternate resolver contract/setter or make both methods use the same authoritative resolution path.

[borinquenkid]

Fixed in 48c9c49. sessionResolver is now final, set once in the constructor to new ThreadLocalSessionResolver<>(this) — the alternate-resolver contract/setter you flagged (setSessionResolver()) is gone entirely; confirmed zero remaining references to it anywhere in the repo.

ThreadLocalSessionResolver no longer maintains independent state — its resolve() reads the exact same TransactionSynchronizationManager.getResource(datastore)/SessionHolder state that DatastoreUtils.doGetSession() (which getCurrentSession() delegates to) also reads. So hasCurrentSession() and getCurrentSession() are now guaranteed to agree — there's no longer a second source of truth for either of them, or for DatastoreUtils.execute()'s branch, to disagree over.

[jdaugherty]

This still silently loses listeners for any valid ApplicationEventPublisher that does not expose the unrelated addApplicationListener method; logging does not satisfy this API contract. The new test explicitly accepts the drop with only noExceptionThrown(). Please retain listeners in a datastore-owned multicaster and use the configured publisher as an outbound delegate, or narrow the accepted publisher type so registration cannot silently succeed without delivery.

[borinquenkid]

Fixed in 48c9c49, along the second option you suggested (narrow what can silently succeed, rather than retain-and-delegate): the reflective fallback now throws IllegalStateException instead of logging and swallowing when the publisher exposes no addApplicationListener(ApplicationListener) method, so a caller can no longer register a listener that will never fire without finding out immediately.

else if (publisher != null) {
    try {
        Method method = publisher.getClass().getMethod("addApplicationListener", ApplicationListener.class);
        method.invoke(publisher, listener);
    }
    catch (Exception e) {
        throw new IllegalStateException("Could not register application listener [" + listener + "] with publisher [" +
                publisher + "]: it does not expose an addApplicationListener(ApplicationListener) method", e);
    }
}

The test was rewritten to match — it no longer accepts the drop with noExceptionThrown(); it now asserts the throw:

void "addApplicationListener fails loudly rather than silently dropping the listener when the publisher exposes no addApplicationListener method"() {
    ...
    when:
    datastore.addApplicationListener(listener)

    then: "the caller finds out immediately that the listener will never receive events, instead of it being silently dropped"
    thrown(IllegalStateException)
}

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: ce346f0
▶️ Tests: 53244 executed
⚪️ Checks: 59/59 completed

---

Learn more about TestLens at testlens.app.