Add multi-segment with_count support and ORM demos#137
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR extends with_count() to support nested relationship paths (e.g., "author__books" or "articles__tags") requiring the terminal segment to be to-many; adds a frozen dataclass WithCountJoinNode, a resolver for path segments, forward/reverse/M2M join builders, and a cache for join metadata per path. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / API
participant Resolver as Path Resolver
participant Cache as Join Cache
participant FKBuilder as FK Join Builder
participant M2MBuilder as M2M Join Builder
participant SQLBuilder as SQL Builder
Client->>Resolver: resolve path "author__books"
activate Resolver
Resolver->>Resolver: split & classify segments
Resolver-->>Client: [("author", forward_fk, desc), ("books", reverse_fk, desc)]
deactivate Resolver
Client->>Cache: check cached join nodes for prefix "author"
activate Cache
Cache-->>Client: miss
deactivate Cache
Client->>FKBuilder: build forward-FK join for "author"
activate FKBuilder
FKBuilder->>Cache: store WithCountJoinNode(alias_A, model_A)
FKBuilder-->>Client: alias_A
deactivate FKBuilder
Client->>FKBuilder: build reverse-FK join for "books" from alias_A
activate FKBuilder
FKBuilder->>Cache: store WithCountJoinNode(alias_B, model_B)
FKBuilder-->>Client: alias_B
deactivate FKBuilder
Client->>SQLBuilder: build COUNT target using terminal alias_B
activate SQLBuilder
SQLBuilder->>Cache: retrieve join nodes for SQL fragments
SQLBuilder-->>Client: SQL COUNT(...) + JOIN fragments
deactivate SQLBuilder
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 |
![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
🤖 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/query/query.py`:
- Around line 614-632: The reverse-FK JOIN currently hardcodes fk_column =
f"{descriptor.fk_field}_id" in _build_reverse_fk_with_count_join which breaks
when a ForeignKey defines a custom DB column; change it to derive the actual
column name from the FK descriptor (e.g., use descriptor.column or
descriptor.db_column or the descriptor-provided attribute that holds the DB
column name) and use that value for fk_column (with a safe fallback to
f"{descriptor.fk_field}_id" if the descriptor lacks the attribute), then build
join_clause using that derived fk_column instead of the hardcoded pattern.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TODO.md (1)
31-34:⚠️ Potential issue | 🟡 MinorStale TODO — this item has been implemented by this PR.
The
:fire:item describing multi-segmentwith_count()support (mixed reverse FK + M2M, distinct semantics, LEFT JOIN zero-row preservation, join plan reuse) is exactly what this PR delivers. It should be removed to keep the backlog accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TODO.md` around lines 31 - 34, Remove the stale TODO bullet about extending with_count() from TODO.md since this PR implements it; specifically delete the line starting with "🔥 Extend `with_count()` to support full multi-segment relationship paths..." so the backlog no longer lists the already-implemented with_count() multi-segment/reverse FK/M2M distinct/LEFT JOIN/join-planning item.
🧹 Nitpick comments (2)
sqliter/query/query.py (2)
699-701: Redundant terminal-alias lookup —parent_aliasalready holds the value.After the
forloop,parent_aliasis assigned the last node's.alias. The two-step derivation viaresolved_path[-1][0]→_with_count_join_nodes[terminal_path]repeats that same value needlessly.♻️ Proposed simplification
- terminal_path = resolved_path[-1][0] - terminal_alias = self._with_count_join_nodes[terminal_path].alias - return f'{terminal_alias}."pk"' + return f'{parent_alias}."pk"'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/query/query.py` around lines 699 - 701, The terminal-alias lookup is redundant: use the already-assigned parent_alias instead of re-deriving terminal_alias via resolved_path[-1][0] and self._with_count_join_nodes; update the return expression in the method that computes the terminal pk (the block using terminal_path/terminal_alias/resolved_path and parent_alias) to return parent_alias."pk" (or equivalent string assembly using parent_alias) so you avoid the duplicate lookup through _with_count_join_nodes.
123-129: Remove the unusedis_to_manyfield fromWithCountJoinNode.All three join builders (
_build_forward_fk_with_count_join,_build_reverse_fk_with_count_join,_build_m2m_with_count_join) populate this field, but it is never read anywhere in the codebase. The relationship type information is already conveyed by thekindfield returned from_resolve_with_count_path, making this field redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/query/query.py` around lines 123 - 129, Remove the redundant is_to_many attribute from the WithCountJoinNode dataclass and eliminate any assignments to it in the join builders; specifically update class WithCountJoinNode to only include alias and model_class, and modify _build_forward_fk_with_count_join, _build_reverse_fk_with_count_join, and _build_m2m_with_count_join to stop populating is_to_many (use the existing kind value from _resolve_with_count_path where relationship type is needed). Ensure any construction sites of WithCountJoinNode are updated to the new two-argument signature and that no other code references is_to_many.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@TODO.md`:
- Around line 31-34: Remove the stale TODO bullet about extending with_count()
from TODO.md since this PR implements it; specifically delete the line starting
with "🔥 Extend `with_count()` to support full multi-segment relationship
paths..." so the backlog no longer lists the already-implemented with_count()
multi-segment/reverse FK/M2M distinct/LEFT JOIN/join-planning item.
---
Nitpick comments:
In `@sqliter/query/query.py`:
- Around line 699-701: The terminal-alias lookup is redundant: use the
already-assigned parent_alias instead of re-deriving terminal_alias via
resolved_path[-1][0] and self._with_count_join_nodes; update the return
expression in the method that computes the terminal pk (the block using
terminal_path/terminal_alias/resolved_path and parent_alias) to return
parent_alias."pk" (or equivalent string assembly using parent_alias) so you
avoid the duplicate lookup through _with_count_join_nodes.
- Around line 123-129: Remove the redundant is_to_many attribute from the
WithCountJoinNode dataclass and eliminate any assignments to it in the join
builders; specifically update class WithCountJoinNode to only include alias and
model_class, and modify _build_forward_fk_with_count_join,
_build_reverse_fk_with_count_join, and _build_m2m_with_count_join to stop
populating is_to_many (use the existing kind value from _resolve_with_count_path
where relationship type is needed). Ensure any construction sites of
WithCountJoinNode are updated to the new two-argument signature and that no
other code references is_to_many.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sqliter/query/query.py (1)
502-588: Add a guard for unresolved forward-FKto_modelstring references.The M2M branches (lines 561–577) explicitly raise
InvalidProjectionErrorwhento_modelis still a string, but theforward_fkbranch (lines 556–558) silently does a barecast. If a FK descriptor'sto_modelhasn't been resolved yet, subsequent calls to_resolve_with_count_segmentwill callcurrent_model.__name__on a plainstrobject and raiseAttributeErrorrather than the expectedInvalidProjectionError.🛡️ Suggested guard (mirrors the existing M2M pattern)
if kind == "forward_fk": + if isinstance(descriptor.to_model, str): + msg = ( + "Cannot resolve model class for forward FK relationship " + f"'{segment}' in path '{path}'." + ) + raise InvalidProjectionError(msg) current_model = cast("type[BaseDBModel]", descriptor.to_model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sqliter/query/query.py` around lines 502 - 588, The forward-FK branch in _resolve_with_count_path uses cast on a ForeignKey descriptor.to_model without guarding against unresolved string references; add a check mirroring the M2M branches: if kind == "forward_fk" and isinstance(descriptor.to_model, str) then raise InvalidProjectionError with a message like "Cannot resolve SQL metadata for relationship '<path>'" (use the same path variable or similar context), otherwise set current_model = cast("type[BaseDBModel]", descriptor.to_model); this prevents AttributeError when descriptor.to_model is still a string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@sqliter/query/query.py`:
- Around line 612-636: The JOIN clause may produce unquoted aliases/identifiers
which can break SQL when aliases or column names need quoting; in
_build_reverse_fk_with_count_join ensure the join alias and parent alias are
quoted consistently and that fk_column (resolved in the fk_descriptor lookup) is
safely used as a quoted identifier in the join_clause construction (refer to
join_clause, fk_column, _next_projection_alias, and WithCountJoinNode) so the
generated SQL always quotes table aliases and column names.
---
Nitpick comments:
In `@sqliter/query/query.py`:
- Around line 502-588: The forward-FK branch in _resolve_with_count_path uses
cast on a ForeignKey descriptor.to_model without guarding against unresolved
string references; add a check mirroring the M2M branches: if kind ==
"forward_fk" and isinstance(descriptor.to_model, str) then raise
InvalidProjectionError with a message like "Cannot resolve SQL metadata for
relationship '<path>'" (use the same path variable or similar context),
otherwise set current_model = cast("type[BaseDBModel]", descriptor.to_model);
this prevents AttributeError when descriptor.to_model is still a string.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_aggregates.py (1)
590-603: Redundant manual restore on Line 603.
monkeypatchautomatically undoessetattrcalls at test teardown, so the explicit restore on line 603 is unnecessary. It's harmless but adds noise.Suggested removal
with pytest.raises( InvalidProjectionError, match="Cannot resolve SQL metadata" ): relation_db.select(BookAgg).with_count("author__books") - - monkeypatch.setattr(BookAgg.author, "to_model", original_to_model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_aggregates.py` around lines 590 - 603, The test test_with_count_rejects_unresolved_forward_fk_target uses monkeypatch.setattr on BookAgg.author to simulate a missing to_model; remove the redundant manual restore at the end (the monkeypatch.setattr(BookAgg.author, "to_model", original_to_model) call) because pytest's monkeypatch fixture automatically reverts changes at teardown—just keep the monkeypatch.setattr call and the assertion block, and delete the explicit restoration line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_aggregates.py`:
- Around line 590-603: The test
test_with_count_rejects_unresolved_forward_fk_target uses monkeypatch.setattr on
BookAgg.author to simulate a missing to_model; remove the redundant manual
restore at the end (the monkeypatch.setattr(BookAgg.author, "to_model",
original_to_model) call) because pytest's monkeypatch fixture automatically
reverts changes at teardown—just keep the monkeypatch.setattr call and the
assertion block, and delete the explicit restoration line.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_aggregates.py (1)
629-643: Consider reaching this error path through the public API instead of calling_build_m2m_with_count_joindirectly.The test exercises an internal method directly, coupling it to the method's name and exact signature. If the method is renamed or merged as part of a future refactor, this test will break with no public-API regression to catch the gap.
The same error path (missing M2M metadata) could be triggered via
with_countusing a monkeypatched descriptor on a proper model field, consistent with howtest_with_count_rejects_unresolved_forward_fk_targettests a similar code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_aggregates.py` around lines 629 - 643, Replace the direct call to the internal _build_m2m_with_count_join in test_build_m2m_with_count_join_rejects_missing_metadata with a public-API invocation: create the same ManyToMany descriptor (ManyToMany(TagAgg)), attach/monkeypatch it onto the appropriate model field on AuthorAgg (or the model instance used by relation_db.select(AuthorAgg)), then call query.with_count("tags") (or the public with_count entrypoint used elsewhere, e.g. as in test_with_count_rejects_unresolved_forward_fk_target) and assert it raises InvalidProjectionError with "Cannot resolve SQL metadata"; this keeps the test exercising the same missing-M2M-metadata path but via the public API rather than calling _build_m2m_with_count_join directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_aggregates.py`:
- Around line 217-240: Add an inline comment before the raw-SQL insert that
explains why we bypass the ORM write path (we need to pin author_ref directly so
the test validates only the read/join side of BookCustomColAgg.with_count on
AuthorAgg), and wrap the DB use in a try/finally so db.close() is always called:
ensure the block that opens conn/cursor, executes the raw INSERTs using
BookCustomColAgg.get_table_name(), commits, runs the
db.select(...).with_count("custom_books",
alias="usage").order("name").fetch_dicts() assertions, and then calls db.close()
in a finally clause to guarantee cleanup even if the assertions fail.
---
Nitpick comments:
In `@tests/test_aggregates.py`:
- Around line 629-643: Replace the direct call to the internal
_build_m2m_with_count_join in
test_build_m2m_with_count_join_rejects_missing_metadata with a public-API
invocation: create the same ManyToMany descriptor (ManyToMany(TagAgg)),
attach/monkeypatch it onto the appropriate model field on AuthorAgg (or the
model instance used by relation_db.select(AuthorAgg)), then call
query.with_count("tags") (or the public with_count entrypoint used elsewhere,
e.g. as in test_with_count_rejects_unresolved_forward_fk_target) and assert it
raises InvalidProjectionError with "Cannot resolve SQL metadata"; this keeps the
test exercising the same missing-M2M-metadata path but via the public API rather
than calling _build_m2m_with_count_join directly.
Owner
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
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
Behavior notes
Validation
Summary by CodeRabbit
New Features
Documentation
Demos
Tests
Bug Fixes