Skip to content

refactor: one CRUD path for sync and async engines - #39

Closed
iklobato wants to merge 3 commits into
chore/remove-unimported-modulesfrom
refactor/single-crud-path
Closed

iklobato wants to merge 3 commits into
chore/remove-unimported-modulesfrom
refactor/single-crud-path

Conversation

@iklobato

Copy link
Copy Markdown
Owner

Stacked on #38. Merge that one first; GitHub will then point this PR at dev.

What

Three commits:

  1. One CRUD implementation. RestEndpoint had every CRUD method twice: list, retrieve, create, update, destroy, and again as _list_async ... _destroy_async, copied line by line with await added. update alone was 107 lines, twice. The logic now lives once in _list, _retrieve, _create, _update and _destroy, which take an open sync Session. The sync methods open a session and call them. The _*_async helpers call the same code through AsyncSession.run_sync. paginate_async does the same with paginate. rest.py goes from 807 to 613 lines.
  2. Sync CRUD no longer runs on the event loop thread. The route handlers are async def but called endpoint.list(), create() and the rest directly, so with a sync Engine one slow query stalled every other request on that worker. They now go through Starlette's thread pool.
  3. Meta.cache works on async engines. Cached GETs and cache invalidation only existed on the sync path. An app on an AsyncEngine with cache = Cache(...) never read or wrote the cache, with no warning.

What does not change

Names and signatures. The _*_async helpers and paginate_async stay, because the README, docs/advanced/async.md, docs/api-reference/pagination.md and three examples tell users to call them.

Behaviour changes to know about

  • With a sync engine on a normal pool, CRUD and the Redis cache calls now run in a worker thread.
  • Engines on StaticPool or SingletonThreadPool still run in place. Those are the in-memory SQLite pools: SingletonThreadPool gives each thread its own empty database, and StaticPool shares one connection that is not safe across threads. The install snippet in the docs and most examples use them, and a plain thread pool would have broken them.
  • Async apps with Meta.cache start using Redis. Before this they silently did not.

Both are in CHANGELOG.md under a new [Unreleased] section. I left the old "Unreleased 0.1.24" section as it was.

Checks

All run on my machine. The test workflow does not run for PRs into dev.

  • pytest: 335 passed, 1 xfailed, 2 xpassed. That is the 327 from before plus 8 new ones.
  • The 8 new tests fail when I break the code on purpose: paginate_async (2), thread used for sync CRUD on a pooled engine and on StaticPool (2), async cache hit, miss, invalidation on POST, no invalidation on GET (4).
  • The five test_async_* files and test_background_tasks (34 tests) pass through the new single path on aiosqlite: insert with refresh, update with rowcount, delete with RETURNING, both paginators, async queryset, reflection.
  • Coverage on lightapi/: 82% before, 84% now (2,251 statements, 357 missed).
  • ruff check, ruff format --check, mypy: clean.

Not checked: Postgres with asyncpg. I have no Postgres here, so run_sync is only proven on aiosqlite.

iklo added 3 commits September 18, 2026 10:14
RestEndpoint carried every CRUD method twice: list, retrieve, create,
update and destroy, and again as _list_async ... _destroy_async, copied
line by line with `await` added. update alone was 107 lines, twice. Any
fix had to be made in both copies.

The logic now lives once in _list, _retrieve, _create, _update and
_destroy, which take an open sync Session. The public methods are thin:

- list/retrieve/create/update/destroy open a sync session
  (get_sync_session) and call the core.
- the _*_async helpers call the same core through
  AsyncSession.run_sync, which is SQLAlchemy's supported way to run
  sync ORM code on an async driver.

Names and signatures do not change. The _*_async helpers and
paginate_async stay because the README, docs/advanced/async.md and
three examples tell users to call them. paginate_async now delegates to
paginate the same way; it had no test, so it gets one.

Checked on aiosqlite: the five test_async_* files and
test_background_tasks pass through the new path (insert with refresh,
update with rowcount, delete with RETURNING, both paginators, async
queryset, reflection). Not checked on asyncpg: no Postgres here.

Tests: 329 passed (327 + 2 new), 1 xfailed, 2 xpassed.
With a sync Engine the route handlers are `async def` but called
endpoint.list(), create(), update() and the rest directly, so every
database round trip ran on the event loop thread. One slow query stalled
all other requests on that worker. The Redis cache calls had the same
problem.

The sync branches now go through session.run_blocking, which uses
Starlette's run_in_threadpool. Cache invalidation goes the same way and
is skipped for GET, which never invalidates.

Engines on StaticPool or SingletonThreadPool keep running in place.
Those are the in-memory SQLite pools: SingletonThreadPool gives every
thread its own empty database, and StaticPool shares one connection that
is not safe across threads. The docs install snippet and most examples
use them, and they would have broken under a plain thread pool.

tests/test_sync_offload.py pins both cases by comparing the thread that
runs the queryset with the event loop thread. It fails without this
change.

Tests: 331 passed, 1 xfailed, 2 xpassed.
maybe_cached and the cache invalidation lived only in the sync branches
of the route handlers. An app on an AsyncEngine with `cache = Cache(...)`
never read, wrote or invalidated anything, with no warning.

cache_helper now splits the work into _cached_response and
_store_response. maybe_cached (sync) and the new maybe_cached_async
share them, so the rules exist once. The Redis client is blocking, so the
async path sends those calls to a worker thread and skips the hop when
the endpoint has no cache configured.

invalidate_cache_after_write replaces maybe_invalidate_cache and is
called for both engine kinds, which removes one `if not is_async` from
each handler.

tests/test_cache_async.py covers hit, miss with the stored key, body and
ttl, invalidation on POST, and no invalidation on GET. Three of the four
fail without this change.

Tests: 335 passed, 1 xfailed, 2 xpassed.
@iklobato

Copy link
Copy Markdown
Owner Author

Consolidated into #44 for a single review pass (one PR covering all five refactors plus the validation work, instead of six stacked ones). Closing this one, no commits were rewritten, they're all still in #44's history.

@iklobato iklobato closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant