fix: register session listeners in framework extension configs (#709)#712
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
==========================================
+ Coverage 80.61% 80.68% +0.07%
==========================================
Files 99 99
Lines 8227 8197 -30
Branches 1124 1116 -8
==========================================
- Hits 6632 6614 -18
+ Misses 1270 1269 -1
+ Partials 325 314 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Regression tests for litestar-org#709. Lock the contract that the Litestar SQLAlchemyAsyncConfig subclass must register the base-class listener set (file-object, timestamp, cache) when create_session_maker() is called. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regression tests for litestar-org#709. Lock the contract that the Litestar SQLAlchemySyncConfig subclass must register the base-class listener set directly on the session_maker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regression tests for litestar-org#709. Lock the contract that both Starlette async and sync config subclasses must register the base-class listener set when create_session_maker() is called. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regression tests for litestar-org#709. Lock the contract that both Sanic async and sync config subclasses must register the base-class listener set when create_session_maker() is called. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regression tests for litestar-org#709. Lock the contract that both Flask async and sync config subclasses must register the base-class listener set when create_session_maker() is called. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…er wiring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ener wiring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ner wiring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… wiring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…wiring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…wiring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… wiring (litestar-org#709) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Starlette/Sanic/Flask listener tests call create_session_maker on a fresh config. When get_engine() runs for real, the aiosqlite dialect registers spurious event.listen calls that pollute the mock count. Pre-populating engine_instance with a MagicMock makes get_engine() short-circuit so only the listener-registration path contributes to the mock. This unblocks restoring the engine_instance caching pre-step in the extension overrides per the "minimal harm" design directive from litestar-org#709. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Preserves exact observable behavior of the previous override: engine_instance is populated during create_session_maker, not lazily on a later get_engine() call. This honors the "minimal harm" design directive from the litestar-org#709 reviewer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Preserves exact observable behavior of the previous overrides: engine_instance is populated during create_session_maker, not lazily on a later get_engine() call. Honors the "minimal harm" design directive from the litestar-org#709 reviewer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Preserves exact observable behavior of the previous overrides: engine_instance is populated during create_session_maker, not lazily on a later get_engine() call. Honors the "minimal harm" design directive from the litestar-org#709 reviewer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add per-call target assertions (every listener attached to the synthetic sync_maker, not the async session_maker) and event-name set assertion to match the reference pattern in test_config/test_async_config.py and the Litestar async listener test. Catches a regression where listeners are incorrectly attached to the async session_maker instead of the synthetic sync_maker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Flask/Sanic/Starlette listener tests were ~156 lines of near-identical code each. Extract the 6 assertions into _listener_contract.py and reduce each per-extension test file to ~45 lines of thin wrappers that pass the extension's config class into the shared helpers. Keeps the per-extension files for locality and per-framework skip markers, while eliminating ~400 lines of duplication. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FastAPI re-exports SQLAlchemyAsyncConfig / SQLAlchemySyncConfig from the Starlette extension at the package level, so the listener-wiring fix carries over for free. Add a dedicated regression test that delegates to the shared _listener_contract helpers — if someone later adds a FastAPI-specific override without calling super(), this test will fail loudly rather than silently dropping listeners like litestar-org#709. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6d862c9 to
71d2651
Compare
|
@hasansezertasan what did you have remaining on this? the change looks correct from what i can tell. |
Co-authored-by: Cody Fincher <204685+cofin@users.noreply.github.com> Signed-off-by: Hasan Sezer Taşan <13135006+hasansezertasan@users.noreply.github.com>
Co-authored-by: Cody Fincher <204685+cofin@users.noreply.github.com> Signed-off-by: Hasan Sezer Taşan <13135006+hasansezertasan@users.noreply.github.com>
…io.py Co-authored-by: Cody Fincher <204685+cofin@users.noreply.github.com> Signed-off-by: Hasan Sezer Taşan <13135006+hasansezertasan@users.noreply.github.com>
Pin mysql-connector-python exclude-newer to a TZ-qualified ISO timestamp so uv lock produces deterministic output across runners (was date-only, drifted by local TZ on every uv sync). Also drop the extra blank line ruff-format flagged in the listener contract helper.
Hey @cofin, thanks for your time. I converted this one because I had some stuff in mind but I can't recall right now 🫤. I'll do a final review of my work and mark it as "ready for review". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f169d58f00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When `routing_config` is set, `super().create_session_maker()` constructs a `RoutingAsync/SyncSessionMaker` that owns its own per-group engine pools (primary + replicas). Extension shutdown previously only disposed `engine_instance`, leaking replica connections across reload/shutdown. Add `dispose_session_maker` / `adispose_session_maker` helpers that no-op for non-routing makers and call `close_all()` for routing makers, then wire them into Starlette, Flask, Sanic, and Litestar shutdown paths (FastAPI inherits from Starlette). Includes unit coverage for both helpers. Refs codex review on litestar-org#712.
Starlette's run_in_threadpool is typed as Callable[[], T], so passing positional args fails pyright. Wrap dispose_session_maker call in a lambda to match the zero-arg signature, mirroring the existing pattern used at line 380.
Summary
Fixes #709 — framework extension configs (Litestar, Starlette, Sanic, Flask, async and sync each) override
create_session_makerwithout delegating tosuper(), silently skippingAsyncFileObjectListener/SyncFileObjectListener,touch_updated_timestamp, andAsyncCacheListener/SyncCacheListenerregistration. User-visible effect: file uploads viaFileObject/StoredObjectpersist metadata to the database but the actual file content is silently discarded.super().create_session_maker()so the middle-layer listener wiring runs. Framework-specific pre-steps (notably Starlette/Sanic/Flask'sengine_instancecaching) are preserved verbatim — the principle is add listener wiring, preserve every other observable behavior.tests/unit/test_extensions/_listener_contract.py). Flask, Sanic, Starlette, FastAPI, and Litestar each get 6 thin tests that call into the shared assertions, so future overrides cannot silently drop listeners again.Design notes
call_count == 0vs expected6), whereas an integration test would fail as a confusing silent missing-file, which is exactly the failure mode Bug: LitestarSQLAlchemyAsyncConfig.create_session_maker()does not register FileObject session listeners #709 reported.engine_instancepre-step preserved in Starlette/Sanic/Flask? Analysis shows it's technically redundant withget_engine()'s internal memoization, so the overrides could be deleted outright. But "minimal harm" mandates preserving exact call sequences in case downstream users have subclassed or monkey-patched in unexpected ways. The tests pre-populateconfig.engine_instance = MagicMock()to isolate listener-count assertions from the aiosqlite dialect's internalevent.listencalls._listener_contract.pyhelpers (~160 lines) instead of duplicating ~600 lines across four files. Each per-extension test file is now ~45 lines of thin wrappers that pass the extension's config class into the shared assertions. Litestar tests remain separate because they predate the extraction and live under their own subdirectory.super()delegation commits (green) → test-isolation commit → pre-step restoration commits → test strengthening and helper-extraction commits. Each commit is individually revertable.Side effects worth noting
session_makercaching is now enabled. The pre-fix Litestar async/sync overrides ended withreturn self.session_maker_class(**session_kws)— they built and returned a session maker but never assigned it toself.session_maker, so every call rebuilt it. After the fix,super().create_session_maker()routes through the middle layer (advanced_alchemy/config/{asyncio,sync}.py), which does cache viaself.session_maker = .... Theif self.session_maker: return self.session_makerguard at the top of the Litestar override now short-circuits on subsequent calls. This is a latent bug fix (nothing wants multiple session makers for one config), not a regression — flagged here so anyone trackingid(config.session_maker)stability across calls isn't surprised.self.session_maker = self.session_maker_class(**session_kws)in the pre-fix code, and the middle layer preserves the same caching semantics.Test Plan
tests/unit/suite — no regressions (pre-existing failures intest_repository.pyandtest_utils/test_fixtures.pyconfirmed unrelated by checking out base commit)SQLAlchemyAsyncConfig.create_session_maker()does not register FileObject session listeners #709 now produces"file_exists_on_disk": trueengine_instancepre-step) matches project preferenceDraft status
Drafting for reviewer discussion on whether to keep the conservative
engine_instancepre-step or delete the non-Litestar overrides outright (simpler, provably safe perget_engine()memoization analysis). CHANGELOG entries deferred to the release-bump process per project convention.Supersedes
Supersedes #711 — same commits, renamed branch (
feat/hasansezertasan/a→fix/extension-create-session-maker-listeners). The GitHub branch rename API closed the previous PR because cross-fork PRs don't preserve rename linkage.🤖 Generated with Claude Code