borinquenkid opened a new pull request, #16066:
URL: https://github.com/apache/grails-core/pull/16066
# 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 (#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 #16064 (`grails.gorm.transactions.GrailsTransactionAttribute`)
and #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
- [x] `./gradlew :grails-datastore-core:test :grails-datamapping-core:test
:grails-data-simple:test` — 0 failures
- [x] `./gradlew :grails-data-hibernate5-core:test
:grails-data-hibernate7-core:test :grails-data-mongodb-core:test` — 0 failures
- [x] `codeStyle` (Checkstyle + CodeNarc) clean
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
