feat: add JOIN support to QuerySet in orm.py#62
Conversation
|
👋 Hi @SatishKumar620! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds chainable JOIN support to QuerySet in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QuerySet
participant SQLBuilder
participant Database
Client->>QuerySet: join(table, on, join_type)
QuerySet->>QuerySet: validate join_type, table, ON (lhs = rhs), store in _joins
Client->>QuerySet: filter(...).all() / .count()
QuerySet->>SQLBuilder: _build_select_sql() / _build_from_with_joins_sql() (include _joins)
SQLBuilder->>Database: execute SQL (with JOIN clauses)
Database-->>SQLBuilder: rows / count
SQLBuilder-->>QuerySet: return results
QuerySet-->>Client: return rows / count
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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 `@src/libs/orm.py`:
- Around line 207-248: The count() and exists() methods currently ignore
self._joins causing wrong results; update count() to reuse the same FROM+JOIN
assembly as _build_select_sql (or call _build_select_sql to get the FROM/JOIN
fragment) so that self._joins are appended to the COUNT query, and update
exists() similarly to include joins when constructing its SELECT ... LIMIT 1;
reference the methods count, exists, _build_select_sql, join and the _joins
attribute to locate and modify the code so JOIN tuples are incorporated into the
SQL for both count() and exists().
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e0ea1ad4-c10d-4fd9-b424-92af1defa7eb
📒 Files selected for processing (1)
src/libs/orm.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libs/orm.py (1)
207-248: Well-implemented JOIN validation with thorough security controls.The validation logic correctly:
- Restricts
join_typeto a safe whitelist- Validates the table identifier
- Parses and validates both sides of the ON clause
One consideration:
update()anddelete()methods don't include_joins. If a user chains.join(...).filter(joined_column=x).update(...)where the filter references a joined column, the resulting SQL will fail because the UPDATE won't have the JOIN clause. Consider either:
- Raising an error if
update()/delete()is called on a QuerySet with non-empty_joins- Documenting this limitation in the
join()docstring💡 Optional: Prevent update/delete on joined QuerySets
async def update(self, **kwargs: Any) -> None: """Update all matching rows with the supplied field=value pairs.""" if not kwargs: return + if self._joins: + raise ValueError("update() is not supported on QuerySets with JOINs.") table = self._model.table_name # ... rest of methodasync def delete(self) -> None: """Delete all matching rows.""" + if self._joins: + raise ValueError("delete() is not supported on QuerySets with JOINs.") table = self._model.table_name # ... rest of method🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/orm.py` around lines 207 - 248, The QuerySet.join allows joining but update() and delete() ignore _joins, causing SQL errors when users call .join(...).filter(...).update(...); add a guard at the start of QuerySet.update and QuerySet.delete that checks if self._joins is non-empty and raises a clear exception (e.g., ValueError or RuntimeError) explaining that update/delete on joined QuerySets is not supported and suggesting to perform the update via the base table or use a subquery; reference the QuerySet.update, QuerySet.delete and QuerySet.join symbols and ensure the error message mentions joins so callers understand the limitation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/libs/orm.py`:
- Around line 207-248: The QuerySet.join allows joining but update() and
delete() ignore _joins, causing SQL errors when users call
.join(...).filter(...).update(...); add a guard at the start of QuerySet.update
and QuerySet.delete that checks if self._joins is non-empty and raises a clear
exception (e.g., ValueError or RuntimeError) explaining that update/delete on
joined QuerySets is not supported and suggesting to perform the update via the
base table or use a subquery; reference the QuerySet.update, QuerySet.delete and
QuerySet.join symbols and ensure the error message mentions joins so callers
understand the limitation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 238247c4-16ba-463c-97dd-b955b774dad6
📒 Files selected for processing (1)
src/libs/orm.py
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 33696 🏁 Script executed: Length of output: 23095 🏁 Script executed: Length of output: 15662
✅ What's Well Done
❌ Blockers Before Merge1. No unit tests for
|
| Check | Status |
|---|---|
| JOIN implementation correctness | ✅ |
| Security / SQL injection safety | ✅ |
Unit tests for join() |
❌ Missing |
| Docstring coverage ≥ 80% | ❌ Currently 75% |
| Human peer review | ❌ Pending |
handlers/bugs.py migration (PR #44 scope) |
ℹ️ Intentionally out of scope |
| No merge conflicts | ✅ |
The core implementation is solid. The main items to address before merging are: adding JOIN unit tests and bringing docstring coverage above 80%.
|
@coderabbitai Please assess the PR's completeness and merge readiness |
🧩 Analysis chain🏁 Script executed: Length of output: 48030
✅ Updated Merge Readiness Assessment
🔲 One Remaining GateThe PR still shows "Changes Requested" — that is from my original review on the first commit (which flagged the ✅ Actions performedReview triggered.
|
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 `@tests/test_orm.py`:
- Line 579: The test uses a normal Python string for the regex in
pytest.raises(match=...) which includes backslashes (e.g. in the assertion
around update\(\) is not supported) and triggers Ruff W605; change the match
arguments in the pytest.raises calls (the ones referencing the update() pattern)
to raw string literals (prefix with r) so the backslashes are treated as regex
escapes rather than invalid Python escape sequences (update\(\) → r"update\(\)
is not supported" in the pytest.raises calls).
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 35d92498-4f86-4789-8a7c-d729e5a196d9
📒 Files selected for processing (2)
src/libs/orm.pytests/test_orm.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai Please assess the PR's completeness and merge readiness |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11286
✅ PR
|
| Check | Status |
|---|---|
| JOIN implementation correctness | ✅ |
| Security / SQL injection safety | ✅ |
Unit tests for join() (TestJoin — 10 tests) |
✅ |
count() includes _joins |
✅ |
update() / delete() JOIN guards |
✅ |
Tests for update/delete guards (TestUpdateDeleteJoinGuards) |
✅ |
Raw string regex in pytest.raises (Ruff W605) |
✅ Fixed in latest commit |
| No merge conflicts | ✅ |
| CI (CodeRabbit) | ✅ SUCCESS |
| Human peer review / approval | ❌ Still pending |
🔲 Only Remaining Gate
The PR still shows "Changes Requested" — this is a GitHub UI artifact from my earlier review on the first commit (the count() issue), which was fixed but GitHub does not auto-dismiss review states. The code itself is clean.
The only actual remaining requirement is a human team member review and approval. Once a maintainer approves, this PR is ready to merge.
|
@coderabbitai re-review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_orm.py (1)
579-587:⚠️ Potential issue | 🟠 MajorEscape literal parentheses in
pytest.raises(..., match=...)regex patterns.At Line 579 and Line 586,
matchuses regex patterns that treat()as an empty group, not literal parentheses. This can make the assertion miss the intended exception message.🔧 Proposed fix
- with pytest.raises(ValueError, match=r"update() is not supported"): + with pytest.raises(ValueError, match=r"update\(\) is not supported"): @@ - with pytest.raises(ValueError, match=r"delete() is not supported"): + with pytest.raises(ValueError, match=r"delete\(\) is not supported"):#!/bin/bash # Verify regex behavior used by pytest.raises(match=...) python - <<'PY' import re cases = [ (r"update() is not supported", "update() is not supported on QuerySets with active JOINs."), (r"update\(\) is not supported", "update() is not supported on QuerySets with active JOINs."), (r"delete() is not supported", "delete() is not supported on QuerySets with active JOINs."), (r"delete\(\) is not supported", "delete() is not supported on QuerySets with active JOINs."), ] for pat, msg in cases: print(f"{pat!r} -> {bool(re.search(pat, msg))}") PYExpected: unescaped patterns evaluate to
False, escaped patterns toTrue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_orm.py` around lines 579 - 587, The regex used in the pytest.raises match strings treats "()" as a group; update the two raises calls (the one asserting update() and the one asserting delete()) to escape the literal parentheses so the pattern matches the exception text; replace "update() is not supported" with r"update\(\) is not supported" and "delete() is not supported" with r"delete\(\) is not supported" in the pytest.raises(...) calls that wrap the self.qs.join(...).update(...) and self.qs.join(...).delete(...) assertions.
🧹 Nitpick comments (1)
tests/test_orm.py (1)
523-533: Add an unsafe RHS ON-clause test to fully mirror production validation.The suite currently checks unsafe ON LHS, but ORM validation also enforces safety on RHS. Add a dedicated RHS-negative case to close that gap.
✅ Suggested test addition
class TestJoin: @@ def test_join_unsafe_on_lhs_raises(self): with pytest.raises(ValueError): self.qs.join( "domains", on="bugs.domain_id; DROP TABLE bugs-- = domains.id" ) + + def test_join_unsafe_on_rhs_raises(self): + with pytest.raises(ValueError): + self.qs.join( + "domains", + on="bugs.domain_id = domains.id; DROP TABLE domains--" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_orm.py` around lines 523 - 533, Add a new negative test in tests/test_orm.py (next to test_join_unsafe_on_lhs_raises) named something like test_join_unsafe_on_rhs_raises that calls self.qs.join("domains", on="bugs.domain_id = domains.id; DROP TABLE domains--") and asserts it raises ValueError using pytest.raises(ValueError); this mirrors the LHS unsafe test and ensures ORM validation blocks unsafe payloads on the RHS as well, using the existing self.qs.join call-site and pytest.raises pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_orm.py`:
- Around line 579-587: The regex used in the pytest.raises match strings treats
"()" as a group; update the two raises calls (the one asserting update() and the
one asserting delete()) to escape the literal parentheses so the pattern matches
the exception text; replace "update() is not supported" with r"update\(\) is not
supported" and "delete() is not supported" with r"delete\(\) is not supported"
in the pytest.raises(...) calls that wrap the self.qs.join(...).update(...) and
self.qs.join(...).delete(...) assertions.
---
Nitpick comments:
In `@tests/test_orm.py`:
- Around line 523-533: Add a new negative test in tests/test_orm.py (next to
test_join_unsafe_on_lhs_raises) named something like
test_join_unsafe_on_rhs_raises that calls self.qs.join("domains",
on="bugs.domain_id = domains.id; DROP TABLE domains--") and asserts it raises
ValueError using pytest.raises(ValueError); this mirrors the LHS unsafe test and
ensures ORM validation blocks unsafe payloads on the RHS as well, using the
existing self.qs.join call-site and pytest.raises pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 89b8a5b6-0c5e-459c-b5e2-d31a0ac9f03b
📒 Files selected for processing (1)
tests/test_orm.py
|
LGTM Minor SuggestionDocument ON clause limitation in The |
|
@VedantAnand17 Sure |
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 `@src/libs/orm.py`:
- Around line 239-250: The code currently strips spaces into on_stripped for
validation but then stores the original on, allowing injection like "a = b OR
1"; change validation to operate on the original structure and store a
normalized, safe ON expression: parse on into lhs and rhs using a strict split
on "=" after trimming (e.g., lhs_raw, rhs_raw = on.split("=", 1)), reject any
extra tokens or operators (no "OR", "AND", additional "="), run
_validate_identifier on the trimmed lhs_raw and rhs_raw, then append a
normalized string (e.g., f"{lhs} = {rhs}" with validated identifiers) to
qs._joins instead of the original on so the _joins list only ever contains the
validated safe clause.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c70c4979-50c9-4a6a-93f3-1b3c3fc6cf65
📒 Files selected for processing (1)
src/libs/orm.py
|
@coderabbitai resume |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_orm.py (1)
543-543: Prefix unused variable with underscore.Static analysis correctly flags
join_tableas unused. Use_or_join_tableto indicate it's intentionally ignored.🔧 Proposed fix
- _, join_table, on_clause = qs._joins[0] + _, _join_table, on_clause = qs._joins[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_orm.py` at line 543, The assignment "_, join_table, on_clause = qs._joins[0]" declares join_table but it's unused; rename it to indicate intentional ignore (e.g. "_join_table" or just "_" ) so static analysis won't flag it—update that tuple unpack in tests to "_, _join_table, on_clause = qs._joins[0]" (or "_, _, on_clause = qs._joins[0]") while leaving qs._joins and on_clause unchanged.
🤖 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_orm.py`:
- Line 543: The assignment "_, join_table, on_clause = qs._joins[0]" declares
join_table but it's unused; rename it to indicate intentional ignore (e.g.
"_join_table" or just "_" ) so static analysis won't flag it—update that tuple
unpack in tests to "_, _join_table, on_clause = qs._joins[0]" (or "_, _,
on_clause = qs._joins[0]") while leaving qs._joins and on_clause unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2f9e3a74-8b3e-4c2c-97ed-38ad606ffcef
📒 Files selected for processing (2)
src/libs/orm.pytests/test_orm.py
✅ Actions performedReviews resumed. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_orm.py (1)
537-544: Strengthen canonicalization coverage with non-canonical input.
test_join_on_clause_canonical_form_storedcurrently passes an already-canonical ON clause, so it does not actually verify normalization behavior.🔧 Proposed test improvement
def test_join_on_clause_canonical_form_stored(self): """Whitespace-folding bypass prevention — canonical ON clause is stored.""" qs = self.qs.join( "domains", - on="bugs.domain_id = domains.id" + on=" bugs.domain_id = domains.id " ) _, _, on_clause = qs._joins[0] assert on_clause == "bugs.domain_id = domains.id" assert " " not in on_clause🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_orm.py` around lines 537 - 544, Replace the already-canonical ON clause in test_join_on_clause_canonical_form_stored with a non-canonical variant that exercises whitespace-folding (for example extra spaces around tokens like " bugs . domain_id = domains.id " or "bugs. domain_id=domains.id") then call qs.join as before and assert that the stored on_clause (from qs._joins[0][2]) equals the canonical form "bugs.domain_id = domains.id"; this ensures qs.join canonicalizes/normalizes the input rather than accepting already-normalized input.
🤖 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_orm.py`:
- Around line 604-611: The regex in the pytest.raises match strings treats
parentheses as grouping; update the match patterns used in the failing tests to
escape literal parentheses so they match the exception messages. Specifically,
change the match for the update test used with pytest.raises(...) to use a
pattern like r"update\(\) is not supported" and similarly change the delete test
match to r"delete\(\) is not supported" so pytest.raises correctly matches the
messages thrown by the code paths in the tests.
---
Nitpick comments:
In `@tests/test_orm.py`:
- Around line 537-544: Replace the already-canonical ON clause in
test_join_on_clause_canonical_form_stored with a non-canonical variant that
exercises whitespace-folding (for example extra spaces around tokens like " bugs
. domain_id = domains.id " or "bugs. domain_id=domains.id") then call
qs.join as before and assert that the stored on_clause (from qs._joins[0][2])
equals the canonical form "bugs.domain_id = domains.id"; this ensures qs.join
canonicalizes/normalizes the input rather than accepting already-normalized
input.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 14e59f54-2555-4a4f-8d78-976c35a29be9
📒 Files selected for processing (1)
tests/test_orm.py
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai Please assess the PR's completeness and merge readiness |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 35370
✅ PR
|
| Fix | Commit |
|---|---|
count() includes _joins |
commit 2 |
update()/delete() JOIN guards + tests |
commit 3 |
Raw string regex in pytest.raises |
commit 4 |
ON-clause limitation documented in join() docstring |
commit 5 |
_build_from_with_joins_sql() helper extracted |
commit 6/7 |
| Unsafe RHS ON-clause negative test | commit 7 |
| Canonical ON clause stored (whitespace-bypass prevention) | commit 8 |
Unused join_table variable renamed to _ |
commit 9 |
Escaped parentheses in pytest.raises match patterns |
commit 10 ✅ |
📊 Current Status
| Check | Status |
|---|---|
| JOIN implementation correctness | ✅ |
| Security / SQL injection safety | ✅ |
Unit tests (TestJoin, TestUpdateDeleteJoinGuards) |
✅ |
count() includes _joins |
✅ |
update()/delete() JOIN guards |
✅ |
_build_from_with_joins_sql() helper (deduplication) |
✅ |
| ON-clause limitation documented | ✅ |
| CI (CodeRabbit) | ✅ SUCCESS |
| No merge conflicts | ✅ |
| Formal human maintainer approval | ❌ Still pending |
⚠️ One Remaining Nitpick (Non-blocking)
From the last automated review: test_join_on_clause_canonical_form_stored currently passes an already-canonical string, so it doesn't exercise the normalization path. This is non-blocking but easy to tighten:
def test_join_on_clause_canonical_form_stored(self):
"""Whitespace-folding bypass prevention — canonical ON clause is stored."""
qs = self.qs.join(
"domains",
on=" bugs.domain_id = domains.id " # non-canonical input
)
_, _, on_clause = qs._joins[0]
assert on_clause == "bugs.domain_id = domains.id"
assert " " not in on_clause🔲 Only Remaining Gate
The "Changes Requested" state in the GitHub UI is a historic artifact from my earlier review on commit e047bd6f — that item (escaped parentheses) was fixed in commit 10. The code and tests are clean. The only actual gate remaining is a formal maintainer approval in the GitHub review UI.
|
@coderabbitai Please approve it then on review |
|
✅ Actions performedComments resolved and changes approved. |
|
@Nachiket-Roy Review please |
|
@VedantAnand17 approve the pr please |
owasp-blt
bot
added
has-peer-review
|
@DonnieBLT review it |
|
🎉 PR merged! Thanks for your contribution, @SatishKumar620! Your work is now part of the project. Keep contributing to OWASP BLT-Pool and help make the web a safer place! 🛡️ Visit pool.owaspblt.org to explore the mentor pool and connect with contributors. 📊 Monthly LeaderboardHi @SatishKumar620! Here's how you rank for March 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run 🔍 Reviewer LeaderboardTop reviewers for March 2026 (across the OWASP-BLT org):
Reviews earn +5 points each in the monthly leaderboard (first two reviewers per PR). Thank you to everyone who helps review PRs! 🙏 |
4 participants
Summary
Closes #61
Required to unblock PR #44 which cannot be merged until
all raw SQL JOIN queries in bugs.py are replaced with ORM calls.
Changes to
src/libs/orm.pyNew
.join()chainable methodAdded a
.join(table, on, join_type)method toQuerySet: