Conversation
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Replace the sequential per-record insert loop (which committed after each row) with a shared cursor approach matching the sync implementation. _insert_single_record_async handles SQL building and execution on the shared cursor; a single _maybe_commit() and cache invalidation follow. Adds three tests covering the FK, non-FK IntegrityError, and generic sqlite3.Error rollback paths. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Add lookup_cache() and store_cache() to QueryBuilder, encapsulating the cache_bypassed guard, key generation, and cache_get/set calls that were previously duplicated verbatim in both _fetch_result and _fetch_projection_result across the sync and async query paths. Both paths now call the shared helpers, removing ~30 lines of duplicate cache logic. The three public accessors (cache_bypassed, query_cache_ttl, make_cache_key) that existed solely to expose internals to the async layer are removed as they are no longer needed. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
#5 - Add optional db_context parameter to _create_instance_from_data / create_instance_from_data so callers can supply the binding directly. AsyncSqliterDB._create_instance_from_data now delegates in a single call instead of delegating then overwriting the db_context attribute. #6 - Remove the redundant _in_transaction getter/setter property pair. in_transaction is now a single direct-read property; the two write sites in __aenter__/__aexit__ call _sync.set_in_transaction() explicitly. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
- Improve AsyncSqliterDB and AsyncQueryBuilder class docstrings from internal implementation notes to user-facing descriptions - Add async mention to pyproject.toml project description Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Add a 15th TUI demo category 'Async Support' with 9 runnable demos covering: connection, context manager, CRUD, bulk insert, queries, FK lazy loading (with explicit wrong/correct patterns), FK eager loading, reverse relationships, and transactions with rollback. Key implementation details: - Demos run via a ThreadPoolExecutor to avoid conflicting with Textual's running event loop - Display code is sanitised to strip infra boilerplate (_ASYNC_AVAILABLE guard, _run_async call, file cleanup) before showing to the user - asyncio.run() unavailable when aiosqlite is not installed; each demo returns a helpful install message instead Also adds docs/tui-demo/async.md mirroring the existing tui-demo page style, with code snippets, What Happens sections, gotcha admonitions, and a full sync-vs-async quick-reference table. Updates mkdocs.yml nav and the tui-demo index page. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Add a 'mypy and Static Type Checking' section to the async guide explaining the AsyncForeignKey typing trade-off (typed as T for eager loading ergonomics, but lazy-loaded runtime value is AsyncLazyLoader[T]) and the reverse accessor limitation (installed via setattr, so __getattribute__ returns object under strict mypy). Update the TUI async demos to show the required cast() workarounds in the FK lazy and reverse demos, with multi-line explanatory comments. Update docs/tui-demo/async.md to mirror the cast patterns with matching !!! note admonitions linking back to the guide. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
… icons Remove the '⚡' icon from the Async Support demo category so all 15 categories are consistent (all use icon=""). Add a TODO item to revisit adding suitable icons to all categories in one pass. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
📝 WalkthroughWalkthroughAdds an optional asynchronous API under ChangesAsync + Core Refactor
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant AsyncDB as AsyncSqliterDB
participant Builder as AsyncQueryBuilder
participant Conn as aiosqlite.Connection
participant SQLite as SQLite
App->>AsyncDB: await AsyncSqliterDB.create(db_file)
AsyncDB->>Conn: await connect()
Conn->>SQLite: open DB
SQLite-->>Conn: ready
Conn-->>AsyncDB: connected
AsyncDB-->>App: AsyncSqliterDB instance
App->>AsyncDB: await insert(model)
AsyncDB->>AsyncDB: build InsertPlan (sync helper)
AsyncDB->>Conn: await execute(sql, values)
Conn->>SQLite: write
SQLite-->>Conn: result
Conn-->>AsyncDB: cursor/result
AsyncDB->>AsyncDB: invalidate cache
AsyncDB-->>App: model with pk
App->>Builder: Builder = AsyncDB.select(Model)
Builder->>AsyncDB: build_execution_plan()
Builder->>Conn: await execute(select_sql)
Conn->>SQLite: read rows
SQLite-->>Conn: rows
Conn-->>Builder: rows
Builder->>Builder: convert rows -> models
alt prefetch requested
Builder->>AsyncDB: await prefetch queries
AsyncDB->>Conn: await execute(prefetch_sql)
end
Builder-->>App: list of models
App->>AsyncDB: await AsyncDB.close()
AsyncDB->>AsyncDB: maybe_commit()
AsyncDB->>Conn: await close()
Conn->>SQLite: close
SQLite-->>Conn: closed
Conn-->>AsyncDB: closed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 834 |
| Duplication | 91 |
🟢 Coverage 100.00% diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (a27ead8) Report Missing Report Missing Report Missing Head commit (2667dd3) 6508 (+1639) 6508 (+1639) 100.00% (+0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#149) 1932 1932 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Use isinstance() instead of type() for bulk_insert type checking (both sync and async), and move AsyncLazyLoader to TYPE_CHECKING block since it's only used in a string annotation. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Update bulk_insert mixed-model tests to expect TypeError instead of ValueError after isinstance refactor. Add full coverage for async_demos.py including execution, ImportError fallback, and guard clause paths. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Fix TestImportErrorFallback to use monkeypatch for all sys.modules cleanup, and also save/restore the parent package's async_demos attribute (importlib.import_module updates it, which patch() uses for resolution). Replace AsyncSqliterDB.get patch with full mock using SimpleNamespace return values to avoid dangling aiosqlite connections. Use real function in test_leading_blank_lines_removed instead of fragile MagicMock. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/test_asyncio_orm.py (1)
1002-1005:⚠️ Potential issue | 🟡 MinorCapture the real manager before seeding
_prefetch_cache.Once
_prefetch_cache["tags"]exists,article.tagsreturnsAsyncPrefetchedM2MResult, notAsyncManyToManyManager. That makes this test cover a wrapper-to-wrapper path instead of the intended in-place refresh against the real manager.💡 Suggested fix
- cached_items = [tag1] - article.__dict__["_prefetch_cache"] = {"tags": cached_items} manager = cast("AsyncManyToManyManager[Tag]", article.tags) + cached_items = [tag1] + article.__dict__["_prefetch_cache"] = {"tags": cached_items} prefetched = AsyncPrefetchedM2MResult(cached_items, manager)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_asyncio_orm.py` around lines 1002 - 1005, Capture the real AsyncManyToManyManager from article.tags before you seed article.__dict__["_prefetch_cache"]; currently you set _prefetch_cache first so article.tags returns an AsyncPrefetchedM2MResult instead of the real manager, making the test exercise wrapper-to-wrapper paths. Change the order: obtain manager = cast("AsyncManyToManyManager[Tag]", article.tags) first, then set article.__dict__["_prefetch_cache"] = {"tags": cached_items}, and finally create AsyncPrefetchedM2MResult(cached_items, manager) so the refresh runs against the real manager.
🧹 Nitpick comments (2)
tests/test_asyncio_core.py (1)
1327-1341: Cache-preservation assertion locks in the__aexit__fix.Asserting
len(db._sync._cache) > 0after the async context exit directly regression-tests the earlier concern about__aexit__wiping caches. Reaching into_sync._cacheis fine for a white-box test; if you want to keep the assertion less fragile,get_cache_stats()["hits"] + ...or a public cache introspection accessor would achieve the same coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_asyncio_core.py` around lines 1327 - 1341, The test test_async_context_manager_preserves_cache directly inspects the private _sync._cache after exiting the async context which locks in the __aexit__ behavior; change the assertion to use public cache introspection instead (e.g. call a public accessor or cache stats on the AsyncSqliterDB such as db.get_cache_stats() and assert that hits/entries > 0 or that the total entries count is > 0) so the test verifies cache preservation without coupling to the internal _sync._cache implementation; update the assertion accordingly in the test body.sqliter/sqliter.py (1)
1010-1053: Transaction scope helpers look good;set_in_transaction(value=True)is a blunt setter.
enter_transaction_scope/exit_transaction_scope/reset_transaction_scopecorrectly implement nested-transaction semantics and the outer-only finalize pattern used by both__enter__/__exit__here andAsyncSqliterDB.__aenter__/__aexit__.One subtlety:
set_in_transaction(value=True)hard-resets_transaction_depthto1, which would discard a pre-existing deeper nesting if ever called while nested. It isn't called from any nesting-aware path in this file, but please ensure external callers only use it as a top-level flag toggle (and preferenter_transaction_scope/exit_transaction_scopefor anything that can be re-entered).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/sqliter.py` around lines 1010 - 1053, set_in_transaction currently force-resets _transaction_depth to 1 when called with value=True which discards any existing nesting; change set_in_transaction so it does not overwrite a deeper nesting (e.g., when value is True, leave _transaction_depth unchanged if it's already >0, otherwise set to 1; when value is False, set depth to 0) and update the set_in_transaction docstring to note it is a blunt top-level toggle and callers that may enter/exit nested scopes should use enter_transaction_scope/exit_transaction_scope instead; refer to set_in_transaction, enter_transaction_scope, exit_transaction_scope, and reset_transaction_scope when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sqliter/asyncio/db.py`:
- Around line 377-400: reset_database currently calls await conn.commit()
unconditionally which can prematurely flush an enclosing async with db:
transaction; change reset_database to be scope-aware by using the existing
helper (call self._maybe_commit() or check self.in_transaction) instead of
unconditionally committing so it only commits when not in an open transaction,
or alternatively update the reset_database docstring to state it always commits
immediately; reference reset_database, _reset_database, _maybe_commit,
self.in_transaction and create(reset=True) (and note __aexit__ is the enclosing
commit/rollback) so reviewers can locate and apply the fix.
In `@sqliter/asyncio/orm/m2m.py`:
- Around line 215-223: _the _get_instance_pk method currently only treats None
as unsaved which is inconsistent with the repo convention that pk==0 is unsaved;
change the guard in _get_instance_pk (which reads self._instance.pk) to reject
any falsy PK (e.g., use "if not pk") instead of only None, and raise
ManyToManyIntegrityError with the same message when pk is falsy so callers that
skip _check_context() cannot proceed with an unsaved instance; keep returning
int(pk) for valid PKs.
In `@sqliter/sqliter.py`:
- Around line 1217-1223: The change constructs INSERT SQL that emits literal
NULL for None values by building placeholders with "NULL" and excluding those
values from the bound tuple, which alters the SQL shape and relies on sql_data
iteration order; either confirm this behavioral change is intentional or revert
to the previous approach that always emits "?" placeholders and includes None in
the values tuple to keep placeholder count stable — update the logic around
fields, placeholders and values (the sql_data comprehension that creates
placeholders and the values tuple construction) accordingly so downstream
tooling and tests see consistent SQL shape.
In `@tests/test_asyncio_core.py`:
- Around line 865-889: The test patches builtins.__import__ to raise ImportError
for both "sqliter.orm.m2m" and "sqliter.orm.registry", but
AsyncSqliterDB._create_m2m_junction_tables imports ModelRegistry first and
returns on that ImportError, and ExampleModel has no ManyToMany fields so the
m2m import path is never exercised; to fix, either (A) change the test to supply
a model with a ManyToMany descriptor (e.g., create a test model with a
ManyToMany field) so the loop in _create_m2m_junction_tables reaches the
junction-build path and then patch only "sqliter.orm.m2m" (not
"sqliter.orm.registry"), or (B) if you only want to assert registry import is
ignored, remove the dead patch for "sqliter.orm.m2m" and update the test
name/description to reflect it only covers ModelRegistry import handling;
reference AsyncSqliterDB._create_m2m_junction_tables, ModelRegistry,
_build_m2m_junction_sql, ExampleModel, and the fake_import helper when making
the change.
---
Duplicate comments:
In `@tests/test_asyncio_orm.py`:
- Around line 1002-1005: Capture the real AsyncManyToManyManager from
article.tags before you seed article.__dict__["_prefetch_cache"]; currently you
set _prefetch_cache first so article.tags returns an AsyncPrefetchedM2MResult
instead of the real manager, making the test exercise wrapper-to-wrapper paths.
Change the order: obtain manager = cast("AsyncManyToManyManager[Tag]",
article.tags) first, then set article.__dict__["_prefetch_cache"] = {"tags":
cached_items}, and finally create AsyncPrefetchedM2MResult(cached_items,
manager) so the refresh runs against the real manager.
---
Nitpick comments:
In `@sqliter/sqliter.py`:
- Around line 1010-1053: set_in_transaction currently force-resets
_transaction_depth to 1 when called with value=True which discards any existing
nesting; change set_in_transaction so it does not overwrite a deeper nesting
(e.g., when value is True, leave _transaction_depth unchanged if it's already
>0, otherwise set to 1; when value is False, set depth to 0) and update the
set_in_transaction docstring to note it is a blunt top-level toggle and callers
that may enter/exit nested scopes should use
enter_transaction_scope/exit_transaction_scope instead; refer to
set_in_transaction, enter_transaction_scope, exit_transaction_scope, and
reset_transaction_scope when making these changes.
In `@tests/test_asyncio_core.py`:
- Around line 1327-1341: The test test_async_context_manager_preserves_cache
directly inspects the private _sync._cache after exiting the async context which
locks in the __aexit__ behavior; change the assertion to use public cache
introspection instead (e.g. call a public accessor or cache stats on the
AsyncSqliterDB such as db.get_cache_stats() and assert that hits/entries > 0 or
that the total entries count is > 0) so the test verifies cache preservation
without coupling to the internal _sync._cache implementation; update the
assertion accordingly in the test body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6962ed98-eaa6-4870-94aa-fe1936335020
📒 Files selected for processing (13)
docs/tui-demo/async.mdsqliter/asyncio/db.pysqliter/asyncio/orm/m2m.pysqliter/sqliter.pytests/test_asyncio_core.pytests/test_asyncio_orm.pytests/test_context_manager.pytests/test_drop_table.pytests/test_foreign_keys.pytests/test_m2m.pytests/test_sqliter.pytests/test_transaction_rollback.pytests/test_unique.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_context_manager.py
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sqliter/sqliter.py (1)
922-962:⚠️ Potential issue | 🟡 MinorUpdate type annotation to accept arbitrary-length tuples.
The parameter annotations use
tuple[str], which denotes a fixed-length, one-element tuple. However,_build_index_sql()explicitly accepts and processes arbitrary-length tuples (such as("slug", "name")), and the implementation iterates over all elements regardless of length. This mismatch causes type checkers and IDEs to incorrectly flag valid composite indexes as errors.Both
_create_indexes()andcreate_indexes()should usetuple[str, ...]to match the actual supported behaviour.Suggested fix
- indexes: list[Union[str, tuple[str]]], + indexes: list[str | tuple[str, ...]],Apply this change to both method signatures (lines 925 and 952).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/sqliter.py` around lines 922 - 962, Update the type annotations for index tuple parameters to accept arbitrary-length tuples: change the annotations in _create_indexes() and create_indexes() from tuple[str] to tuple[str, ...] so composite indexes like ("slug","name") are correctly typed; update both method signatures (the index parameter in _create_indexes and create_indexes) to use tuple[str, ...].
🧹 Nitpick comments (1)
docs/tui-demo/async.md (1)
99-101: Minor prose polish for grammar/punctuation in flagged lines.A few sentences are readable but trip grammar tooling (comma placement/articles). Quick copy edits here will remove noisy LanguageTool warnings without changing meaning.
Also applies to: 344-346, 414-415, 474-477, 546-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tui-demo/async.md` around lines 99 - 101, Polish the prose in the flagged sentences that mention `async with db:`, `0.21.0`, and `await db.close()` to correct comma placement and article usage: reword to something like "Prefer `async with db:` when you need atomicity. Starting with `0.21.0`, the context manager no longer closes the connection automatically; call `await db.close()` explicitly when the async database instance is no longer needed." Apply the same minor copy edits to the other occurrences (around lines 344–346, 414–415, 474–477, 546–547) so punctuation and articles are consistent and tooling warnings are resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tui-demo/async.md`:
- Around line 91-97: Multiple identical section headings ("### What Happens" and
"### When to Use") trigger MD024; update each repeated heading to be unique by
scoping them with context (e.g., "### What Happens (Async Context Manager)" and
"### When to Use (Async Context Manager)") and apply the same disambiguation to
the other occurrences of those headings so every "What Happens" and "When to
Use" heading is distinct.
- Line 24: The markdown uses fenced code blocks using the triple-backtick marker
(e.g., "```python") which conflicts with the project's markdownlint rule MD046;
either convert those fenced blocks to indented code blocks or disable/relax
MD046 for this document. Locate every occurrence of the fenced block marker (the
"```python" and matching "```" tokens) in async.md (several occurrences noted)
and replace each fenced block with an indented block (4-space indent for each
code line) to satisfy MD046, or add a file-level markdownlint disable directive
(e.g., a comment to disable MD046) near the top of the file if fenced blocks are
required for syntax highlighting. Ensure consistency across all occurrences (the
instances around the noted fenced blocks must all be handled).
In `@sqliter/asyncio/orm/m2m.py`:
- Around line 203-205: The current _rollback_if_needed method only rolls back
when db.auto_commit is true, leaving implicit SQLite transactions open when
auto_commit is False; change the condition in _rollback_if_needed (method name)
to roll back whenever there is a connection and the DB is not in a tracked
transaction (i.e., if db.conn is not None and not db.in_transaction) so that
failed M2M INSERT/DELETEs are always rolled back; keep the await
db.conn.rollback() call but remove the db.auto_commit gating and optionally
catch/log rollback errors if needed.
In `@sqliter/sqliter.py`:
- Around line 1010-1069: The connection close path currently doesn't clear
transaction bookkeeping, so ensure close() resets _transaction_depth,
_rollback_requested, and _in_transaction by calling the existing
reset_transaction_scope() (or explicitly setting those fields) inside the
SqliterDB.close() implementation; locate the close() method and add a call to
reset_transaction_scope() before or after finalizing the underlying connection
so subsequent reopenings don't inherit stale state from
set_in_transaction/enter_transaction_scope.
- Around line 1364-1371: After creating the new instance from insert data you
must also update the original supplied instance with the DB-assigned PK and mark
it as saved so callers that reuse the original object don’t remain unsaved;
locate the block that calls self._create_instance_from_data (and the surrounding
insert handling in bulk_insert flow), set the original instance's pk to
cursor.lastrowid and flip whatever saved/dirty flag the ORM uses (the same state
mutation performed for newly created instances), and apply the identical change
in _insert_single_record() so single-record inserts also update the supplied
instance's pk.
---
Outside diff comments:
In `@sqliter/sqliter.py`:
- Around line 922-962: Update the type annotations for index tuple parameters to
accept arbitrary-length tuples: change the annotations in _create_indexes() and
create_indexes() from tuple[str] to tuple[str, ...] so composite indexes like
("slug","name") are correctly typed; update both method signatures (the index
parameter in _create_indexes and create_indexes) to use tuple[str, ...].
---
Nitpick comments:
In `@docs/tui-demo/async.md`:
- Around line 99-101: Polish the prose in the flagged sentences that mention
`async with db:`, `0.21.0`, and `await db.close()` to correct comma placement
and article usage: reword to something like "Prefer `async with db:` when you
need atomicity. Starting with `0.21.0`, the context manager no longer closes the
connection automatically; call `await db.close()` explicitly when the async
database instance is no longer needed." Apply the same minor copy edits to the
other occurrences (around lines 344–346, 414–415, 474–477, 546–547) so
punctuation and articles are consistent and tooling warnings are resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96709153-1a03-4987-a223-d018957aa13f
📒 Files selected for processing (13)
docs/tui-demo/async.mdsqliter/asyncio/db.pysqliter/asyncio/orm/m2m.pysqliter/sqliter.pytests/test_asyncio_core.pytests/test_asyncio_orm.pytests/test_context_manager.pytests/test_drop_table.pytests/test_foreign_keys.pytests/test_m2m.pytests/test_sqliter.pytests/test_transaction_rollback.pytests/test_unique.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_drop_table.py
- tests/test_foreign_keys.py
- tests/test_unique.py
- tests/test_transaction_rollback.py
- tests/test_asyncio_orm.py
- sqliter/asyncio/db.py
…file Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sqliter/sqliter.py (1)
836-863:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-validate configured indexes before creating the table.
create_table()commits the table at Line 845, then only afterwards buildsMeta.indexes/Meta.unique_indexes. If_build_index_sql()raisesInvalidIndexError, the call fails with the table already persisted, which leaves schema setup half-applied.Suggested fix
+ regular_index_sqls = [ + self._build_index_sql(model_class, index, unique=False) + for index in getattr(model_class.Meta, "indexes", []) + ] + unique_index_sqls = [ + self._build_index_sql(model_class, index, unique=True) + for index in getattr(model_class.Meta, "unique_indexes", []) + ] + try: conn = self.connect() cursor = conn.cursor() self._execute(cursor, create_table_sql) self._maybe_commit() @@ - if hasattr(model_class.Meta, "indexes"): - self._create_indexes( - model_class, model_class.Meta.indexes, unique=False - ) + for index_sql in regular_index_sqls: + self._execute_sql(index_sql) @@ - if hasattr(model_class.Meta, "unique_indexes"): - self._create_indexes( - model_class, model_class.Meta.unique_indexes, unique=True - ) + for index_sql in unique_index_sqls: + self._execute_sql(index_sql)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sqliter/sqliter.py` around lines 836 - 863, Pre-validate any configured indexes on the model before executing and committing the CREATE TABLE so index validation errors surface before the table is persisted: before calling self.connect()/self._execute(create_table_sql) in create_table(), iterate over model_class.Meta.indexes and model_class.Meta.unique_indexes (if present) and call the same index-building/validation path (i.e. use the internal _build_index_sql or the logic used by _create_indexes) for each index to raise InvalidIndexError early; only after all index SQL is successfully built continue to execute create_table_sql, then proceed with creating FK indexes and calling _create_indexes as before.
♻️ Duplicate comments (1)
tests/test_asyncio_core.py (1)
261-279:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
ModelRegistryaround these temporary ORM models.Like the earlier dynamic-model case in this file, both tests register
sqliter.orm.BaseDBModelsubclasses and leave the global registry mutated afterwards. That can leak metadata into later async ORM tests.Also applies to: 397-421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_asyncio_core.py` around lines 261 - 279, The test defines a temporary BaseDBModel subclass (SavedAsyncORMModel) but mutates the global ModelRegistry and does not restore it, leaking registrations into other tests; wrap the temporary model usage by saving the current ModelRegistry state (e.g. copy the registry), run create_table/insert/close, and then restore the saved registry (or use an existing ModelRegistry context helper if available) in a finally block so the global registry is returned to its original state; do the same fix for the other test that registers temporary ORM models.
🧹 Nitpick comments (1)
pyproject.toml (1)
91-93: ⚡ Quick winPin
markdownlint-cli2version for reproducible docs linting.The current command
npx markdownlint-cli2 ...resolves the latest version at runtime, risking CI/local drift. The project has no Node.js lockfile or version specification file (no package.json, .node-version, etc.). Either pin the version explicitly (e.g.npx --yes markdownlint-cli2@<pinned_version> ...) or establish a project-managed Node lockfile workflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 91 - 93, Update the md.cmd entry to pin the markdownlint-cli2 package to a specific, supported version instead of resolving latest at runtime; replace the current md.cmd value that invokes `npx markdownlint-cli2 "docs/**/*.md" "README.md"` with an explicit pin such as `npx --yes markdownlint-cli2@<chosen_version> "docs/**/*.md" "README.md"` (choose and document the exact <chosen_version>), keeping md.help unchanged; this ensures reproducible linting in CI/local environments when using the md.cmd configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guide/data-operations.md`:
- Around line 302-305: Fix the typo in the markdown link label: change the link
label "tansactions" to "transactions" in the sentence referencing the
transactions guide (the inline link currently written as
[tansactions](transactions.md)) so the link text correctly reads "transactions".
In `@docs/guide/foreign-keys.md`:
- Line 198: Update the table row string that currently reads "Both approaches
have same write performance" to include the missing article so it reads "Both
approaches have the same write performance"; locate the row containing the
header "**Web APIs (write-heavy)**" and modify that cell text accordingly.
In `@docs/tui-demo/connection.md`:
- Around line 165-170: The "Context Manager" row in the summary table currently
lists the benefit as "Auto cleanup", which is incorrect for v0.21.0 because
context managers no longer auto-close connections; update the table entry for
"**Context Manager**" to reflect accurate behavior (e.g., "Scoped transaction
handling" or "Deterministic resource management/transaction boundaries") and
adjust the corresponding "Best For" or "Cons" phrasing if needed to mention that
explicit close is required; search for the "**Context Manager**" table row in
connection.md and replace the benefit text and any related note to align with
v0.21.0 semantics.
In `@pyproject.toml`:
- Around line 153-159: Remove the file-level S608 suppression entries in
pyproject.toml for "sqliter/asyncio/db.py" and "sqliter/query/query.py" and
instead add targeted inline suppressions (# noqa: S608) on the exact statements
that build raw SQL in the modules (e.g., the SQL construction sites inside the
query builder functions/methods in sqliter/query/query.py and the SQL
assembly/execute paths in sqliter/asyncio/db.py); keep TRY301 unchanged, run the
linter to confirm only the intended lines are suppressed, and ensure surrounding
code includes a short comment stating why S608 is safe for that specific line.
In `@tests/test_sqliter.py`:
- Around line 213-231: The test registers a temporary ORM subclass
(SavedORMModel) but doesn't restore the global ModelRegistry, leaking metadata;
update the test to snapshot the current ModelRegistry before defining
SavedORMModel and restore it after the test (in a finally block) so global state
is unchanged — e.g. capture the registry (ModelRegistry or its internal mapping)
before creating SavedORMModel, run the create_table/insert assertions, then
restore the saved registry back to ModelRegistry in the finally block to ensure
no leak.
---
Outside diff comments:
In `@sqliter/sqliter.py`:
- Around line 836-863: Pre-validate any configured indexes on the model before
executing and committing the CREATE TABLE so index validation errors surface
before the table is persisted: before calling
self.connect()/self._execute(create_table_sql) in create_table(), iterate over
model_class.Meta.indexes and model_class.Meta.unique_indexes (if present) and
call the same index-building/validation path (i.e. use the internal
_build_index_sql or the logic used by _create_indexes) for each index to raise
InvalidIndexError early; only after all index SQL is successfully built continue
to execute create_table_sql, then proceed with creating FK indexes and calling
_create_indexes as before.
---
Duplicate comments:
In `@tests/test_asyncio_core.py`:
- Around line 261-279: The test defines a temporary BaseDBModel subclass
(SavedAsyncORMModel) but mutates the global ModelRegistry and does not restore
it, leaking registrations into other tests; wrap the temporary model usage by
saving the current ModelRegistry state (e.g. copy the registry), run
create_table/insert/close, and then restore the saved registry (or use an
existing ModelRegistry context helper if available) in a finally block so the
global registry is returned to its original state; do the same fix for the other
test that registers temporary ORM models.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 91-93: Update the md.cmd entry to pin the markdownlint-cli2
package to a specific, supported version instead of resolving latest at runtime;
replace the current md.cmd value that invokes `npx markdownlint-cli2
"docs/**/*.md" "README.md"` with an explicit pin such as `npx --yes
markdownlint-cli2@<chosen_version> "docs/**/*.md" "README.md"` (choose and
document the exact <chosen_version>), keeping md.help unchanged; this ensures
reproducible linting in CI/local environments when using the md.cmd
configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a58b46f-29a9-4b74-9d28-0b808790c30e
📒 Files selected for processing (33)
.markdownlint.yamldocs/api-reference/many-to-many.mddocs/api-reference/sqliterdb.mddocs/guide/data-operations.mddocs/guide/fields.mddocs/guide/foreign-keys.mddocs/guide/guide.mddocs/guide/models.mddocs/guide/properties.mddocs/installation.mddocs/tui-demo/async.mddocs/tui-demo/connection.mddocs/tui-demo/constraints.mddocs/tui-demo/crud.mddocs/tui-demo/errors.mddocs/tui-demo/field-selection.mddocs/tui-demo/filters.mddocs/tui-demo/index.mddocs/tui-demo/ordering.mddocs/tui-demo/orm.mddocs/tui-demo/results.mddocs/tui-demo/string-filters.mddocs/tui-demo/transactions.mdpyproject.tomlsqliter/asyncio/db.pysqliter/asyncio/orm/m2m.pysqliter/model/model.pysqliter/sqliter.pytests/test_asyncio_core.pytests/test_asyncio_orm.pytests/test_bulk_insert.pytests/test_context_manager.pytests/test_sqliter.py
✅ Files skipped from review due to trivial changes (16)
- docs/tui-demo/ordering.md
- docs/tui-demo/constraints.md
- docs/tui-demo/filters.md
- .markdownlint.yaml
- docs/tui-demo/field-selection.md
- docs/guide/models.md
- docs/tui-demo/results.md
- docs/tui-demo/errors.md
- docs/tui-demo/orm.md
- docs/tui-demo/string-filters.md
- docs/guide/properties.md
- docs/installation.md
- docs/tui-demo/index.md
- docs/api-reference/sqliterdb.md
- docs/api-reference/many-to-many.md
- docs/guide/guide.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_context_manager.py
- sqliter/asyncio/db.py
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sqliter/asyncio/db.py (1)
808-827:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
reset_transaction_scope()is skipped whenconn is None, poisoning future transactions.
reset_transaction_scope()lives insideif self.conn is not None and should_finalize:, so it is only called when both conditions hold. However, ifclose()is called inside theasync with db:block and the block raises an exception, the sequence is:
close()setsconn = Noneand callsreset_transaction_scope()→_rollback_requested = False.- The exception propagates out to
__aexit__.exit_transaction_scope(had_error=True)sets_rollback_requested = Trueand returns(True, True).conn is None→ theifblock (and itsfinally) is skipped →reset_transaction_scope()is never called._rollback_requestedstaysTrue.- The next clean
async with db:block is silently rolled back becauseexit_transaction_scope(had_error=False)returns(True, True)while_rollback_requestedis still set.Move the
reset_transaction_scope()call so it always fires whenshould_finalizeisTrue, independent of the connection state.🛠️ Proposed fix
async def __aexit__( self, exc_type: Optional[type[BaseException]], exc_value: Optional[BaseException], traceback: Optional[TracebackType], ) -> None: """Exit the async transaction context.""" should_finalize, should_rollback = self._sync.exit_transaction_scope( had_error=exc_type is not None ) if self.conn is not None and should_finalize: try: if should_rollback: await self.conn.rollback() else: await self.conn.commit() finally: - self._sync.reset_transaction_scope() + pass + + if should_finalize: + self._sync.reset_transaction_scope()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sqliter/asyncio/db.py` around lines 808 - 827, The issue is that reset_transaction_scope() is only called when self.conn is not None, so if conn is None and should_finalize is True the transaction scope isn't reset and future transactions are poisoned; in __aexit__ (method __aexit__ on the async context manager) change the flow so that when should_finalize returned from self._sync.exit_transaction_scope(...) is True you always call self._sync.reset_transaction_scope() regardless of self.conn, i.e. perform commit/rollback on self.conn if present but ensure reset_transaction_scope() runs in a finally or after the conn check so it executes even when conn is None; keep using should_rollback to decide commit vs rollback when conn exists, but guarantee reset_transaction_scope() is invoked whenever should_finalize is True.
🧹 Nitpick comments (1)
sqliter/query/query.py (1)
918-927: 💤 Low valueReuse
_collect_prefetch_parent_pkshere.Lines 918‑925 reimplement exactly the dedup/truthy‑pk logic now extracted into the new static helper
_collect_prefetch_parent_pks(Lines 985‑995). Calling the helper keeps the two prefetch paths in lockstep should the rules change (e.g. handling ofpk == 0per the existing convention).♻️ Proposed change
- seen_pks: set[Any] = set() - parent_pks: list[Any] = [] - for inst in parent_instances: - pk = getattr(inst, "pk", None) - if not pk or pk in seen_pks: - continue - seen_pks.add(pk) - parent_pks.append(pk) - if not parent_pks: - return + parent_pks = self._collect_prefetch_parent_pks(parent_instances) + if not parent_pks: + return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sqliter/query/query.py` around lines 918 - 927, The block that builds parent_pks by iterating parent_instances reimplements the dedup/truthy‑pk logic; replace that loop with a call to the existing static helper _collect_prefetch_parent_pks(parent_instances) and use its return to set parent_pks (and early return if it returns empty). Update any local variables (seen_pks/parent_pks) to be removed and ensure the code uses the helper’s output so both prefetch paths stay consistent with _collect_prefetch_parent_pks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tui-demo/connection.md`:
- Around line 113-116: Replace the GitHub-style callout token `> [!WARNING]`
used above the "Breaking change in `0.21.0`" paragraph with the MkDocs Material
admonition syntax `!!! warning` to match the admonition at line 160; ensure the
admonition block contains the same text ("Breaking change in `0.21.0`: the
context manager no longer closes the connection automatically. Call `db.close()`
explicitly when you are done.") and formatting so it renders as a styled warning
box in MkDocs Material.
In `@sqliter/query/query.py`:
- Around line 2313-2347: The async override of _execute_query must return the
same 3-tuple (results, column_names, selected_fields) as the sync implementation
so callers can forward plan.selected_fields into convert_fetched_result; update
the async _execute_query override to call build_execution_plan and return
plan.selected_fields as the third element, then update the two call sites that
unpack its result (currently doing result, column_names = ...) to unpack result,
column_names, selected_fields and pass selected_fields=selected_fields into
self._query.convert_fetched_result(...). Alternatively (but pick one), make
QueryBuilder.__init__ always include "pk" in self._fields so the existing
fallback in _convert_row_to_model is correct; choose the async-side fix for
minimal change.
---
Outside diff comments:
In `@sqliter/asyncio/db.py`:
- Around line 808-827: The issue is that reset_transaction_scope() is only
called when self.conn is not None, so if conn is None and should_finalize is
True the transaction scope isn't reset and future transactions are poisoned; in
__aexit__ (method __aexit__ on the async context manager) change the flow so
that when should_finalize returned from self._sync.exit_transaction_scope(...)
is True you always call self._sync.reset_transaction_scope() regardless of
self.conn, i.e. perform commit/rollback on self.conn if present but ensure
reset_transaction_scope() runs in a finally or after the conn check so it
executes even when conn is None; keep using should_rollback to decide commit vs
rollback when conn exists, but guarantee reset_transaction_scope() is invoked
whenever should_finalize is True.
---
Nitpick comments:
In `@sqliter/query/query.py`:
- Around line 918-927: The block that builds parent_pks by iterating
parent_instances reimplements the dedup/truthy‑pk logic; replace that loop with
a call to the existing static helper
_collect_prefetch_parent_pks(parent_instances) and use its return to set
parent_pks (and early return if it returns empty). Update any local variables
(seen_pks/parent_pks) to be removed and ensure the code uses the helper’s output
so both prefetch paths stay consistent with _collect_prefetch_parent_pks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0c820ad-e977-421f-9909-44f6cefc8b57
📒 Files selected for processing (11)
TODO.mddocs/guide/data-operations.mddocs/guide/foreign-keys.mddocs/tui-demo/connection.mdpyproject.tomlsqliter/asyncio/db.pysqliter/query/query.pysqliter/sqliter.pytests/test_asyncio_core.pytests/test_indexes.pytests/test_sqliter.py
✅ Files skipped from review due to trivial changes (5)
- docs/guide/foreign-keys.md
- TODO.md
- docs/guide/data-operations.md
- tests/test_indexes.py
- tests/test_sqliter.py
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- tests/test_asyncio_core.py
- sqliter/sqliter.py
Comment on lines
+113
to
+116
| > [!WARNING] | ||
| > | ||
| > Breaking change in `0.21.0`: the context manager no longer closes the | ||
| > connection automatically. Call `db.close()` explicitly when you are done. |
There was a problem hiding this comment.
Inconsistent admonition syntax with line 160.
Lines 113-116 use GitHub-style callout syntax (> [!WARNING]), whilst line 160 uses MkDocs Material admonition syntax (!!! warning). If this project uses MkDocs Material for documentation, the GitHub syntax will render as a plain blockquote rather than a styled warning box.
📝 Proposed fix to align with MkDocs Material syntax
-> [!WARNING]
->
-> Breaking change in `0.21.0`: the context manager no longer closes the
-> connection automatically. Call `db.close()` explicitly when you are done.
+!!! warning "Breaking change in 0.21.0"
+
+ The context manager no longer closes the connection automatically. Call `db.close()` explicitly when you are done.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tui-demo/connection.md` around lines 113 - 116, Replace the GitHub-style
callout token `> [!WARNING]` used above the "Breaking change in `0.21.0`"
paragraph with the MkDocs Material admonition syntax `!!! warning` to match the
admonition at line 160; ensure the admonition block contains the same text
("Breaking change in `0.21.0`: the context manager no longer closes the
connection automatically. Call `db.close()` explicitly when you are done.") and
formatting so it renders as a styled warning box in MkDocs Material.
Owner
Author
There was a problem hiding this comment.
there is a hook google_style_notes.py that converts google-style admonition to mkdocs style so this can stay.
There was a problem hiding this comment.
@seapagan, thanks for the clarification! That makes sense — with the google_style_notes.py hook handling the conversion, the > [!WARNING] syntax will render correctly in MkDocs Material. I'll note this for future reviews.
(¬､¬)
🐰💭✨
✏️ Learnings added
Learnt from: seapagan
Repo: seapagan/sqliter-py PR: 149
File: docs/tui-demo/connection.md:113-116
Timestamp: 2026-05-05T20:01:31.889Z
Learning: In seapagan/sqliter-py, GitHub/Google-style admonition syntax (`> [!WARNING]`, `> [!NOTE]`, etc.) is intentionally used in Markdown documentation files. A MkDocs hook (`google_style_notes.py`) automatically converts these to MkDocs Material admonition syntax at build time, so reviewers should NOT flag these as incorrect syntax or suggest replacing them with `!!! warning` / `!!! note` style blocks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: seapagan
Repo: seapagan/sqliter-py PR: 149
File: docs/tui-demo/async.md:24-24
Timestamp: 2026-04-23T19:46:01.267Z
Learning: In seapagan/sqliter-py Markdown documentation, mixed Markdown code block styles are intentional: use indented code blocks for callout/admonition content and fenced code blocks for syntax-highlighted snippets. Because `.markdownlint.yaml` disables MD046 (code-block-style), reviewers should not flag issues solely due to mixing indented vs fenced code block styles within this project.
Learnt from: seapagan
Repo: seapagan/sqliter-py PR: 149
File: docs/tui-demo/async.md:91-97
Timestamp: 2026-04-23T19:46:34.918Z
Learning: Do not flag or suggest fixing duplicate Markdown headings (markdownlint rule MD024: "no-duplicate-heading") in this repository’s Markdown docs. MD024 is intentionally disabled via .markdownlint.yaml, so repeated headings (e.g., "### What Happens" / "### When to Use" in different sections within the same file) are a deliberate documentation style choice and should not be treated as a review defect.
Comment on lines
+2313
to
+2347
| def _execute_query( | ||
| self, | ||
| *, | ||
| fetch_one: bool = False, | ||
| count_only: bool = False, | ||
| ) -> tuple[ | ||
| list[tuple[Any, ...]] | tuple[Any, ...], | ||
| list[tuple[str, str, type[BaseDBModel]]], | ||
| Optional[tuple[str, ...]], | ||
| ]: | ||
| """Execute the constructed SQL query. | ||
|
|
||
| Args: | ||
| fetch_one: If True, fetch only one result. | ||
| count_only: If True, return only the count of results. | ||
|
|
||
| Returns: | ||
| A tuple containing: | ||
| - Query results (list of tuples or single tuple) | ||
| - Column metadata (list of (alias, field_name, model_class) tuples) | ||
| Empty list for non-JOIN queries (backward compatible). | ||
|
|
||
| Raises: | ||
| RecordFetchError: If there's an error executing the query. | ||
| """ | ||
| plan = self.build_execution_plan(count_only=count_only) | ||
| try: | ||
| conn = self.db.connect() | ||
| cursor = conn.cursor() | ||
| self.db._execute(cursor, sql, values) # noqa: SLF001 | ||
| self.db.execute_cursor(cursor, plan.sql, plan.values) | ||
| results = cursor.fetchall() if not fetch_one else cursor.fetchone() | ||
| except sqlite3.Error as exc: | ||
| raise RecordFetchError(self.table_name) from exc | ||
| else: | ||
| return (results, []) # Empty column_names for backward compat | ||
| return (results, plan.column_names, plan.selected_fields) |
There was a problem hiding this comment.
Async _execute_query override is now out of sync with this 3‑tuple return shape.
build_execution_plan and _execute_query here now expose plan.selected_fields as the third return value, and _fetch_result (Lines 2855‑2869) forwards it to convert_fetched_result(..., selected_fields=...) so non‑JOIN rows are decoded against the SQL‑level selection (which always includes pk via _selected_fields_for_execution).
The async subclass at sqliter/asyncio/query.py:175-201 overrides _execute_query to return only (results, plan.column_names) and its callers at sqliter/asyncio/query.py:207 and :465 unpack two values, then call self._query.convert_fetched_result(...) without passing selected_fields. In _convert_row_to_model (Lines 2458‑2496) the fallback is tuple(self._fields), but QueryBuilder.__init__ (Lines 166‑208) does not append "pk" to fields — only the chained .fields() method does. So an async call like db.select(Model, fields=["name"]) will execute a SELECT that includes pk (from _selected_fields_for_execution) but decode rows as if only name were selected, silently dropping pk from instances and breaking lazy FK / prefetch identity downstream.
Either propagate selected_fields from the async override, or normalise self._fields to always include pk at construction time so the fallback in _convert_row_to_model is safe for both paths.
🔧 Suggested async-side fix (apply in sqliter/asyncio/query.py)
- async def _execute_query(
- self,
- *,
- fetch_one: bool = False,
- count_only: bool = False,
- ) -> tuple[
- list[tuple[Any, ...]] | tuple[Any, ...] | None,
- list[tuple[str, str, type[BaseDBModel]]],
- ]:
+ async def _execute_query(
+ self,
+ *,
+ fetch_one: bool = False,
+ count_only: bool = False,
+ ) -> tuple[
+ list[tuple[Any, ...]] | tuple[Any, ...] | None,
+ list[tuple[str, str, type[BaseDBModel]]],
+ Optional[tuple[str, ...]],
+ ]:
"""Execute the constructed SQL query."""
plan = self._query.build_execution_plan(count_only=count_only)
...
- else:
- return (cast("Any", results), plan.column_names)
+ else:
+ return (
+ cast("Any", results),
+ plan.column_names,
+ plan.selected_fields,
+ )…and update the two unpack sites (result, column_names = ... → result, column_names, selected_fields = ...) to thread selected_fields=selected_fields into self._query.convert_fetched_result(...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sqliter/query/query.py` around lines 2313 - 2347, The async override of
_execute_query must return the same 3-tuple (results, column_names,
selected_fields) as the sync implementation so callers can forward
plan.selected_fields into convert_fetched_result; update the async
_execute_query override to call build_execution_plan and return
plan.selected_fields as the third element, then update the two call sites that
unpack its result (currently doing result, column_names = ...) to unpack result,
column_names, selected_fields and pass selected_fields=selected_fields into
self._query.convert_fetched_result(...). Alternatively (but pick one), make
QueryBuilder.__init__ always include "pk" in self._fields so the existing
fallback in _convert_row_to_model is correct; choose the async-side fix for
minimal change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch adds first-class async support to SQLiter under
sqliter.asyncioandsqliter.asyncio.orm.It includes:
AsyncSqliterDBandAsyncQueryBuilderAsyncBaseDBModel,AsyncForeignKey, async reverse relationships, and async many-to-many managersbulk_inserttransaction handlingAPI notes
Async ORM intentionally differs from sync ORM for lazy FK loading:
book.author.nameauthor = await book.author.fetch()Reverse relationships and many-to-many accessors keep the same object-oriented shape, but terminal and write operations are awaited.
Follow-up notes
A small docs note remains about current mypy limitations around dynamic async relationship typing. The runtime API is implemented and covered; the note is there to set correct expectations for static typing ergonomics.
Summary by CodeRabbit
New Features
Documentation
Behaviour Change
Tests
Tooling
Style