Expose read-only ManyToMany SQL metadata#134
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds read‑only ManyToMany SQL metadata: introduces Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sqliter/orm/m2m.py (1)
163-172:_from_col/_to_colare computed twice — consider deriving from the metadata objectThe column names are first computed manually (lines 163–165) and then re-derived by the same
_m2m_column_names+ swap logic inside_build_m2m_sql_metadata(lines 166–172). This is harmless but the two results must always stay in sync.Reversing the order (compute metadata first, then read columns from it) removes the duplication:
♻️ Proposed refactor
- self._from_col, self._to_col = _m2m_column_names(from_table, to_table) - if manager_options.swap_columns: - self._from_col, self._to_col = self._to_col, self._from_col - self._sql_metadata = _build_m2m_sql_metadata( + self._sql_metadata = _build_m2m_sql_metadata( source_table=from_table, target_table=to_table, junction_table=junction_table, symmetrical=manager_options.symmetrical, swap_columns=manager_options.swap_columns, ) + self._from_col = self._sql_metadata.from_column + self._to_col = self._sql_metadata.to_column🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/orm/m2m.py` around lines 163 - 172, The code computes _from_col/_to_col twice—once via _m2m_column_names and again inside _build_m2m_sql_metadata—so refactor to call _build_m2m_sql_metadata first to produce _sql_metadata and then set self._from_col and self._to_col from that metadata (instead of re-calling _m2m_column_names or re-applying manager_options.swap_columns). Update the constructor/initializer to remove the initial _m2m_column_names call and derive column names from the returned _sql_metadata fields to keep a single source of truth (referencing _build_m2m_sql_metadata, _sql_metadata, _from_col, _to_col, and manager_options.swap_columns).docs/guide/many-to-many.md (1)
96-109: PreferArticle.tagsoverArticle.__dict__["tags"]for consistencyBoth produce the same
ManyToManydescriptor object (since__get__withinstance=Nonereturnsself), butArticle.__dict__["tags"]bypasses the descriptor protocol and is unusual in user-facing docs. The equivalent section indocs/tui-demo/orm.mdusesArticle.tags.sql_metadatadirectly, which is cleaner and more consistent.♻️ Proposed fix
-desc = Article.__dict__["tags"] -meta = desc.sql_metadata -assert meta is not None +meta = Article.tags.sql_metadata +assert meta is not None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/many-to-many.md` around lines 96 - 109, The snippet uses Article.__dict__["tags"] which bypasses the descriptor protocol and is inconsistent with other docs; change it to use the descriptor directly (Article.tags) when obtaining sql_metadata (e.g., replace desc = Article.__dict__["tags"] with desc = Article.tags) and then continue using desc.sql_metadata (or meta = Article.tags.sql_metadata) so the example is consistent and clearer.
🤖 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/orm.md`:
- Around line 403-406: The sentence under the "## M2M SQL Metadata
Introspection" heading should include a comma before the coordinating
conjunction "so"; update the line "Inspect read-only SQL metadata for
many-to-many relationships so raw SQL can reuse SQLiter's naming conventions."
to read with a comma before "so" to improve readability.
---
Nitpick comments:
In `@docs/guide/many-to-many.md`:
- Around line 96-109: The snippet uses Article.__dict__["tags"] which bypasses
the descriptor protocol and is inconsistent with other docs; change it to use
the descriptor directly (Article.tags) when obtaining sql_metadata (e.g.,
replace desc = Article.__dict__["tags"] with desc = Article.tags) and then
continue using desc.sql_metadata (or meta = Article.tags.sql_metadata) so the
example is consistent and clearer.
In `@sqliter/orm/m2m.py`:
- Around line 163-172: The code computes _from_col/_to_col twice—once via
_m2m_column_names and again inside _build_m2m_sql_metadata—so refactor to call
_build_m2m_sql_metadata first to produce _sql_metadata and then set
self._from_col and self._to_col from that metadata (instead of re-calling
_m2m_column_names or re-applying manager_options.swap_columns). Update the
constructor/initializer to remove the initial _m2m_column_names call and derive
column names from the returned _sql_metadata fields to keep a single source of
truth (referencing _build_m2m_sql_metadata, _sql_metadata, _from_col, _to_col,
and manager_options.swap_columns).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sqliter/orm/m2m.py (2)
162-171:self._symmetricalis now a second source of truth.
self._symmetrical(line 162) always equalsself._sql_metadata.symmetrical— both resolve tobool(manager_options.symmetrical and self._self_ref). The five methods that useself._symmetrical(_fetch_related_pks,add,remove,clear,count) could instead referenceself._sql_metadata.symmetrical, eliminating the risk of these diverging in a future change.♻️ Suggested cleanup
- self._self_ref = from_table == to_table - self._symmetrical = bool(manager_options.symmetrical and self._self_ref) self._sql_metadata = _build_m2m_sql_metadata( source_table=from_table, target_table=to_table, junction_table=junction_table, symmetrical=manager_options.symmetrical, swap_columns=manager_options.swap_columns, ) self._from_col = self._sql_metadata.from_column self._to_col = self._sql_metadata.to_column + self._symmetrical = self._sql_metadata.symmetrical
self._self_refcan also be dropped since nothing outside__init__uses it after this refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/orm/m2m.py` around lines 162 - 171, self._symmetrical is redundant with self._sql_metadata.symmetrical; update the class to stop storing a second source of truth by removing the self._symmetrical assignment in __init__ and replacing all uses of self._symmetrical in _fetch_related_pks, add, remove, clear, and count with self._sql_metadata.symmetrical; also remove self._self_ref from the instance if it becomes unused after this change (and drop any code that only existed to set it in __init__).
680-699:ManyToMany.sql_metadatarecomputes on every access.Both
get_table_name()calls and theM2MSQLMetadataconstruction run on each property access. Sinceowner,to_model, and_junction_tableare all stable once the descriptor is registered, the result could be cached as_sql_metadataon the descriptor the first time it's computed (guard:_junction_table is not None). The same applies toReverseManyToMany.sql_metadata(lines 869–884), where all inputs are set in__init__and never change.This is negligible for infrequent introspection but could accumulate if called in a hot path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/orm/m2m.py` around lines 680 - 699, The sql_metadata property on ManyToMany (and similarly on ReverseManyToMany) recomputes each access; change it to memoize the result in a private attribute (e.g., _sql_metadata): if owner is None or to_model is a str return None as before, otherwise if _sql_metadata is None and _junction_table is not None compute owner_table/target_table via get_table_name(), call _build_m2m_sql_metadata with source_table, target_table, junction_table and m2m_info.symmetrical, store the returned M2MSQLMetadata in self._sql_metadata and return it; ensure ReverseManyToMany.sql_metadata uses the same caching (its inputs are fixed in __init__), and keep the existing guards around _junction_table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sqliter/orm/m2m.py`:
- Around line 162-171: self._symmetrical is redundant with
self._sql_metadata.symmetrical; update the class to stop storing a second source
of truth by removing the self._symmetrical assignment in __init__ and replacing
all uses of self._symmetrical in _fetch_related_pks, add, remove, clear, and
count with self._sql_metadata.symmetrical; also remove self._self_ref from the
instance if it becomes unused after this change (and drop any code that only
existed to set it in __init__).
- Around line 680-699: The sql_metadata property on ManyToMany (and similarly on
ReverseManyToMany) recomputes each access; change it to memoize the result in a
private attribute (e.g., _sql_metadata): if owner is None or to_model is a str
return None as before, otherwise if _sql_metadata is None and _junction_table is
not None compute owner_table/target_table via get_table_name(), call
_build_m2m_sql_metadata with source_table, target_table, junction_table and
m2m_info.symmetrical, store the returned M2MSQLMetadata in self._sql_metadata
and return it; ensure ReverseManyToMany.sql_metadata uses the same caching (its
inputs are fixed in __init__), and keep the existing guards around
_junction_table.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sqliter/orm/m2m.py (1)
679-701: Memoisation of descriptor-level metadata — note the interaction withresolve_forward_ref.The lazy-init + cache pattern is correct: the guards at lines 686-689 ensure
Noneis returned (without caching) while the descriptor is still partially resolved, and once the metadata is built it is cached for all future accesses.One thing to be aware of:
resolve_forward_ref(line 667) mutatesto_modeland_junction_tablebut does not clear_sql_metadata. This is fine today because_sql_metadatacan only be non-Noneafter both fields are resolved, so re-resolution would never encounter a stale cache. However, ifresolve_forward_refis ever called a second time with a different target, the stale cache would silently persist. A defensive one-liner (self._sql_metadata = None) insideresolve_forward_refwould close this off cheaply.Optional defensive reset in resolve_forward_ref
def resolve_forward_ref(self, model_class: type[Any]) -> None: """Resolve a string forward ref to a concrete model class.""" self.to_model = model_class self.m2m_info.to_model = model_class + self._sql_metadata = None # invalidate cached metadata if self._junction_table is None and self.owner is not None: self._junction_table = self._get_junction_table_name(self.owner)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/orm/m2m.py` around lines 679 - 701, The cached SQL metadata stored on the descriptor (_sql_metadata) can become stale if resolve_forward_ref mutates to_model or _junction_table; update resolve_forward_ref to defensively reset the cache by setting self._sql_metadata = None whenever it updates to_model or _junction_table so subsequent accesses to the sql_metadata property rebuild fresh metadata.
🤖 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/orm/m2m.py`:
- Line 633: The import of FrozenInstanceError should be made conditional to
preserve Python 3.9–3.10 compatibility: in the module that currently does "from
dataclasses import FrozenInstanceError" (e.g., tests referencing
FrozenInstanceError) check sys.version_info and import it only for >= (3,11),
otherwise assign a compatible fallback (e.g., FrozenInstanceError = Exception or
a small custom subclass of Exception); update the import site that references
FrozenInstanceError so all uses (tests or code) reference this conditional
symbol instead of assuming dataclasses provides it.
---
Nitpick comments:
In `@sqliter/orm/m2m.py`:
- Around line 679-701: The cached SQL metadata stored on the descriptor
(_sql_metadata) can become stale if resolve_forward_ref mutates to_model or
_junction_table; update resolve_forward_ref to defensively reset the cache by
setting self._sql_metadata = None whenever it updates to_model or
_junction_table so subsequent accesses to the sql_metadata property rebuild
fresh metadata.
There was a problem hiding this comment.
Pull request overview
Adds a public, read-only API for introspecting many-to-many SQL naming (junction table and FK column names) so application code can build occasional raw SQL without duplicating internal conventions.
Changes:
- Introduces
M2MSQLMetadataand exposessql_metadataon forward descriptors, reverse descriptors, instance managers, and prefetched M2M results. - Updates TUI ORM demo and docs to showcase metadata introspection.
- Adds comprehensive tests covering forward/reverse/manager/prefetch paths, self-references, caching, and unresolved forward refs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sqliter/orm/m2m.py |
Implements M2MSQLMetadata, builder helper, and sql_metadata properties across descriptor/manager/prefetch wrappers. |
tests/test_m2m.py |
Adds a new test suite validating metadata values, caching, immutability, and forward-ref edge cases. |
tests/test_prefetch_related.py |
Verifies prefetched M2M wrapper exposes sql_metadata via manager passthrough. |
sqliter/tui/demos/orm.py |
Adds a new demo for inspecting M2M SQL metadata in the TUI. |
docs/tui-demo/orm.md |
Documents the new TUI demo usage for M2M SQL metadata introspection. |
docs/guide/many-to-many.md |
Adds a guide section demonstrating using sql_metadata for raw SQL queries. |
docs/api-reference/many-to-many.md |
Documents the new sql_metadata surface area and the M2MSQLMetadata type. |
TODO.md |
Notes a medium-term typing direction for explicit reverse relationship declarations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/orm/m2m.py`:
- Line 158: Shorten the overly-long inline comment in sqliter/orm/m2m.py that
currently reads about "Column/metadata names derived from from_model/to_model
table names (with optional column swapping)" so it is under 80 characters;
locate the comment just above the logic that builds column/metadata names for
the many-to-many mapping (the block that refers to from_model/to_model table
names) and replace it with a concise version such as "Column/metadata names
derived from model table names (columns may swap)" to satisfy the E501 lint
rule.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
sqliter/tui/demos/orm.py (2)
289-293:Article.tags.sql_metadatacan returnNone; thecastmasks this.
ManyToMany.sql_metadatareturnsOptional[M2MSQLMetadata]. Usingcasthere suppresses the type checker but doesn't guard againstNoneat runtime. In this demo both models are concrete, so it's safe in practice, but an explicit guard would be more robust and would better illustrate the API for users reading the demo code.Suggested improvement
- descriptor_meta = cast("M2MSQLMetadata", Article.tags.sql_metadata) - output.write("Descriptor metadata (Article.tags):\n") + descriptor_meta = Article.tags.sql_metadata + if descriptor_meta is None: + output.write("Descriptor metadata not available\n") + db.close() + return output.getvalue() + output.write("Descriptor metadata (Article.tags):\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/tui/demos/orm.py` around lines 289 - 293, Article.tags.sql_metadata is Optional[M2MSQLMetadata] and the current cast hides potential None; update the demo to check for None before accessing descriptor_meta attributes: retrieve descriptor_meta from Article.tags.sql_metadata, if it's None write a clear message to output (e.g., "no SQL metadata available") or raise a helpful error, otherwise access descriptor_meta.junction_table, .from_column and .to_column; reference the symbols Article.tags.sql_metadata, M2MSQLMetadata and the descriptor_meta variable when making the change so the guard is applied before any attribute access.
6-13: Static analysis:Anyimport flagged as unused.Codacy reports
Anyis unused. Thecast("Any", ...)calls throughout the file use a string literal, so the symbol isn't technically referenced at runtime. SinceTYPE_CHECKINGwas just added to this line, it's likely a pre-existing issue surfaced by the line change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/tui/demos/orm.py` around lines 6 - 13, Update the typing usage so the Any import is actually referenced: in sqliter/tui/demos/orm.py change the string-literal casts like cast("Any", ...) to use the real Any symbol (cast(Any, ...)) or remove the unused Any import and keep the string casts; specifically edit the import line (from typing import TYPE_CHECKING, Any, Optional, cast) and all cast("Any", ...) occurrences so the Any symbol is used (or remove Any if you prefer the string-cast approach) to satisfy static analysis.sqliter/orm/m2m.py (1)
872-893: Duplicatedswap_columnscondition betweensql_metadataand__get__.The expression
self._from_model is self._to_model and not self._symmetricalappears identically on lines 890–891 and 937–938. If one is updated without the other, column orientation would silently diverge between the metadata and the manager.Consider extracting it to a cached property or private method:
Suggested refactor
+ `@property` + def _swap_columns(self) -> bool: + """Whether columns must be swapped for reverse orientation.""" + return self._from_model is self._to_model and not self._symmetrical + `@property` def sql_metadata(self) -> M2MSQLMetadata: ... self._sql_metadata = _build_m2m_sql_metadata( ... - swap_columns=self._from_model is self._to_model - and not self._symmetrical, + swap_columns=self._swap_columns, ) ... def __get__(self, ...): ... options=ManyToManyOptions( symmetrical=self._symmetrical, - swap_columns=self._from_model is self._to_model - and not self._symmetrical, + swap_columns=self._swap_columns, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/orm/m2m.py` around lines 872 - 893, The duplicated boolean expression "self._from_model is self._to_model and not self._symmetrical" used in the sql_metadata property and __get__ should be extracted to a single source of truth: add a private cached property or method (e.g. _should_swap_columns or swap_columns_cached) on the same class, compute and cache the expression there, then replace the inline expressions in sql_metadata (where _build_m2m_sql_metadata is called) and in __get__ to reference that new property/method so both places use the identical, single computed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sqliter/orm/m2m.py`:
- Around line 872-893: The duplicated boolean expression "self._from_model is
self._to_model and not self._symmetrical" used in the sql_metadata property and
__get__ should be extracted to a single source of truth: add a private cached
property or method (e.g. _should_swap_columns or swap_columns_cached) on the
same class, compute and cache the expression there, then replace the inline
expressions in sql_metadata (where _build_m2m_sql_metadata is called) and in
__get__ to reference that new property/method so both places use the identical,
single computed value.
In `@sqliter/tui/demos/orm.py`:
- Around line 289-293: Article.tags.sql_metadata is Optional[M2MSQLMetadata] and
the current cast hides potential None; update the demo to check for None before
accessing descriptor_meta attributes: retrieve descriptor_meta from
Article.tags.sql_metadata, if it's None write a clear message to output (e.g.,
"no SQL metadata available") or raise a helpful error, otherwise access
descriptor_meta.junction_table, .from_column and .to_column; reference the
symbols Article.tags.sql_metadata, M2MSQLMetadata and the descriptor_meta
variable when making the change so the guard is applied before any attribute
access.
- Around line 6-13: Update the typing usage so the Any import is actually
referenced: in sqliter/tui/demos/orm.py change the string-literal casts like
cast("Any", ...) to use the real Any symbol (cast(Any, ...)) or remove the
unused Any import and keep the string casts; specifically edit the import line
(from typing import TYPE_CHECKING, Any, Optional, cast) and all cast("Any", ...)
occurrences so the Any symbol is used (or remove Any if you prefer the
string-cast approach) to satisfy static analysis.
Summary
M2MSQLMetadatasql_metadataon forward descriptor, reverse descriptor, manager, and prefetched wrapperCloses #133
Summary by CodeRabbit
New Features
Documentation
TUI Demo
Tests