Skip to content

fix: register session listeners in framework extension configs (#709)#711

Closed
hasansezertasan wants to merge 18 commits intolitestar-org:mainfrom
hasansezertasan:feat/hasansezertasan/a
Closed

fix: register session listeners in framework extension configs (#709)#711
hasansezertasan wants to merge 18 commits intolitestar-org:mainfrom
hasansezertasan:feat/hasansezertasan/a

Conversation

@hasansezertasan
Copy link
Copy Markdown
Contributor

Summary

Fixes #709 — framework extension configs (Litestar, Starlette, Sanic, Flask, async and sync each) override create_session_maker without delegating to super(), silently skipping AsyncFileObjectListener / SyncFileObjectListener, touch_updated_timestamp, and AsyncCacheListener / SyncCacheListener registration. User-visible effect: file uploads via FileObject / StoredObject persist metadata to the database but the actual file content is silently discarded.

  • Fix: Each override now delegates to super().create_session_maker() so the middle-layer listener wiring runs. Framework-specific pre-steps (notably Starlette/Sanic/Flask's engine_instance caching) are preserved verbatim — the principle is add listener wiring, preserve every other observable behavior.
  • Scope: 8 source files (4 extensions × async + sync). FastAPI inherits from Starlette so gets the fix for free (no override exists there).
  • Tests: 24 new unit tests in 5 files mirror the existing mocking pattern in tests/unit/test_config/test_async_config.py, locking the listener-registration contract per extension so future overrides cannot silently drop it again.

Design notes

  • Why mock-based unit tests and not integration tests? The base class's integration tests already cover end-to-end listener behavior. The bug is that listeners weren't registered at all in subclasses — a unit-test question. Mocks catch exactly that failure mode loudly and specifically (call_count == 0 vs expected 6), whereas an integration test would fail as a confusing silent missing-file, which is exactly the failure mode Bug: Litestar SQLAlchemyAsyncConfig.create_session_maker() does not register FileObject session listeners #709 reported.
  • Why engine_instance pre-step preserved in Starlette/Sanic/Flask? Analysis shows it's technically redundant with get_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-populate config.engine_instance = MagicMock() to isolate listener-count assertions from the aiosqlite dialect's internal event.listen calls.
  • Atomic commits: 18 small commits tell the full story: 5 failing-test commits (red) → 8 super() delegation commits (green) → 1 test-isolation commit → 3 pre-step restoration commits → 1 test strengthening commit. Each commit is individually revertable.

Test Plan

Draft status

Drafting for reviewer discussion on:

  1. Whether to keep the conservative engine_instance pre-step or delete the non-Litestar overrides outright (simpler, provably safe per get_engine() memoization analysis).
  2. Whether to add CHANGELOG entries or leave them to the release-bump process.

🤖 Generated with Claude Code

hasansezertasan and others added 18 commits April 10, 2026 14:18
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>
@hasansezertasan
Copy link
Copy Markdown
Contributor Author

Superseded by #712 — same commits, renamed branch (feat/hasansezertasan/afix/extension-create-session-maker-listeners). This PR was auto-closed by GitHub when the source branch was renamed because cross-fork PRs don't preserve rename linkage. Please review #712 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Litestar SQLAlchemyAsyncConfig.create_session_maker() does not register FileObject session listeners

1 participant