Skip to content

feat: add W022 cross-join-explicit rule#31

Merged
Pawansingh3889 merged 2 commits into
Pawansingh3889:mainfrom
vibeyclaw:feature/w022-cross-join-explicit
May 2, 2026
Merged

feat: add W022 cross-join-explicit rule#31
Pawansingh3889 merged 2 commits into
Pawansingh3889:mainfrom
vibeyclaw:feature/w022-cross-join-explicit

Conversation

@vibeyclaw

Copy link
Copy Markdown
Contributor

Summary

Implements the W022 rule requested in #8.

Adds a new warning rule that detects explicit CROSS JOIN usage, which produces a Cartesian product and is rarely intentional outside of calendar-grid or lookup-table generation patterns.

Changes

  • sql_guard/rules/warnings.py: Added CrossJoinExplicit class (W022) with a single-line regex check
  • sql_guard/rules/__init__.py: Registered CrossJoinExplicit in the rule registry and ALL_RULES list
  • tests/test_new_rules.py: Added 5 unit tests covering detection, case-insensitivity, and pass cases

Behavior

Flags:

SELECT * FROM products p CROSS JOIN regions r;
-- W022: Explicit CROSS JOIN -- Cartesian product, confirm this is intentional

Passes:

SELECT * FROM orders o JOIN customers c ON o.customer_id = c.id;
SELECT * FROM orders o LEFT JOIN customers c ON o.customer_id = c.id;

Suppress with noqa:

SELECT * FROM calendar_dates CROSS JOIN (SELECT 1 AS n UNION ALL SELECT 2); -- noqa: W022

Closes #8

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your first PR on sql-sop! Quick checks:

  • pytest -q and ruff check . pass locally
  • Every new rule has both a "fires on bad SQL" test AND a
    "does NOT fire on safe SQL" test
  • Rule IDs (E001, W001, …) are public API — don't renumber
    or rename existing ones without the deprecation process in
    GOVERNANCE.md
  • CHANGELOG.md [Unreleased] entry for any user-facing change

First-PR-wins on the linked issue applies for 7 days. The
maintainer will review within 14 days of your last response.

@Pawansingh3889

Copy link
Copy Markdown
Owner

Thanks @vibeyclaw, appreciate the fast turnaround on #8. The code is small, the tests pass locally, and the structure mirrors W019's shape correctly.

A few real things to address before this lands:

False positives in string literals and comments. The regex \bCROSS\s+JOIN\b fires on these:

  • SELECT 'use CROSS JOIN here' AS hint FROM products;
  • -- This used to do CROSS JOIN before refactor
  • /* CROSS JOIN was removed in v2 */

I reproduced all three locally. Each returns a Finding when it shouldn't. W020 (truncate-table) has a test_w020_does_not_flag_truncate_in_string precedent for handling exactly this. Either pre-process the line to strip string literals and comments before matching, or take a look at how W020 keeps it clean and mirror that.

E402 ruff error blocking CI. from sql_guard.rules.warnings import CrossJoinExplicit is on line 201, mid-file. Ruff's E402 Module level import not at top of file fires there. Move the import to the top of tests/test_new_rules.py alongside the other rule imports. That's the failing CI 3.11 check.

Missing CHANGELOG entry. The pr-sop / PR governance check failure is from pr-sop requiring an [Unreleased] entry for any user-facing change. Add a bullet under ## [Unreleased] then ### Added:

  • W022 cross-join-explicit - warns on explicit CROSS JOIN usage which produces a Cartesian product.

Worth knowing but not blocking: the rule won't catch CROSS JOIN split across two physical lines. Fine to leave for a follow-up since single-line covers nearly all real-world usage and matches the existing per-line rule pattern.

Test coverage suggestion: once the false-positive fix lands, add the three negative cases (string literal, line comment, block comment) to your test set so the regression surface is covered. The W020 tests are a good template.

Ping me here once these are addressed and I'll re-review. Genuinely good first PR, the implementation is clean and the test structure is right.

Pawansingh3889 added a commit that referenced this pull request Apr 27, 2026
Single-file CLI that prints copy-paste-ready snippets for every file
that needs an edit when adding a new rule:
- the rule class for sql_guard/rules/{warnings,errors,structural}.py
- the registration in sql_guard/rules/__init__.py
- the fixture line
- the two tests (fires + does-not-fire)
- the README rule table row
- the CHANGELOG entry under [Unreleased]
- a reminder to bump README Key Numbers

Output mirrors the W019 (CountDistinctUnbounded) shape that
CONTRIBUTING.md and the v0.7 milestone reference.

Includes an explicit reminder to put the test-file import at the top
of tests/test_new_rules.py to avoid the E402 ruff error that's been
catching first-time contributors (most recently #31).

Severity and target file are inferred from the rule code prefix:
  E -> error, errors.py
  W -> warning, warnings.py
  T -> warning, warnings.py
  S -> warning, structural.py
  P -> error, python_source.py
@Pawansingh3889 Pawansingh3889 force-pushed the feature/w022-cross-join-explicit branch from a9d3ea0 to e5e22e9 Compare May 2, 2026 10:31
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

vibeyclaw and others added 2 commits May 2, 2026 12:13
Adds a new warning rule W022 that detects explicit CROSS JOIN usage,
which produces a Cartesian product and is rarely intentional.

- Fires on any explicit CROSS JOIN statement
- Single-line rule using regex check
- Users can suppress with inline '-- noqa: W022' comment
- Adds 5 unit tests covering: detection, case-insensitivity, pass cases

Closes Pawansingh3889#8
@Pawansingh3889 Pawansingh3889 force-pushed the feature/w022-cross-join-explicit branch from e5e22e9 to f7ebd34 Compare May 2, 2026 11:15
@Pawansingh3889

Copy link
Copy Markdown
Owner

Took this over the line. Rebased on top of #33 and #32 which both landed in the last hour, so the count assertions are now 38/28. While I was in the rule, tightened the regex to strip trailing -- line comments before matching, since -- avoid CROSS JOIN here was tripping it as a false positive. Added a regression test for the comment case. Suggestion text now points at -- sql-guard: disable=W022 (the project-wide convention) rather than the bare noqa form.

CHANGELOG / README rule-table row / key-numbers updated. Squashing once CI settles. Thanks for the rule, the calendar-grid edge case is exactly the framing this needed.

@Pawansingh3889 Pawansingh3889 merged commit defd347 into Pawansingh3889:main May 2, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants