borinquenkid commented on PR #15779: URL: https://github.com/apache/grails-core/pull/15779#issuecomment-4932216356
@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. -- 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]
