diff --git a/CHANGELOG.md b/CHANGELOG.md index 39aaed0..bec65de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,17 @@ a deprecation window (see `GOVERNANCE.md` ยง Scope discipline). the explicit-column variant (`SELECT col1, col2 INTO target FROM source`) is allowed through here and covered by the contracts pack at C001/C003 if a column type drifts. Resolves #43. +- **W024 `select-distinct-suspicious`** (warning) - fires when a + statement contains both top-level `SELECT DISTINCT` and any `JOIN` + keyword. The pattern is a common band-aid for a missing join + condition or a missing `GROUP BY`: developers reach for `DISTINCT` + to swallow row duplication caused by an over-wide JOIN cardinality + rather than fixing the join. Standalone `SELECT DISTINCT col FROM t` + (no join) is left alone, and aggregate-DISTINCT forms like + `COUNT(DISTINCT col)` are not flagged because the regex anchors on + `SELECT` directly preceding `DISTINCT`. String literals and comments + are stripped first so a mention of "SELECT DISTINCT" inside a string + or comment cannot fire the rule. Resolves #40. ### Fixed diff --git a/README.md b/README.md index 6ff7848..54ea4e7 100644 --- a/README.md +++ b/README.md @@ -238,6 +238,7 @@ sql-sop list-rules # show every registered rule | W020 | `truncate-table` | `TRUNCATE TABLE staging;` -- bypasses triggers, resets identity | | W022 | `cross-join-explicit` | `FROM products CROSS JOIN regions` -- Cartesian product, confirm intent | | W023 | `scalar-udf-in-where` | `WHERE dbo.fn_X(col) = 1` -- row-by-row predicate evaluation | +| W024 | `select-distinct-suspicious` | `SELECT DISTINCT a, b FROM x JOIN y ON ...` -- DISTINCT often masks a missing join condition or GROUP BY | ### Structural (v0.3.0+, sqlparse-based) diff --git a/sql_guard/rules/__init__.py b/sql_guard/rules/__init__.py index f546170..a7bdc6a 100644 --- a/sql_guard/rules/__init__.py +++ b/sql_guard/rules/__init__.py @@ -33,6 +33,7 @@ OrAcrossColumns, OrderByWithoutLimit, ScalarUdfInWhere, + SelectDistinctSuspicious, SelectStar, SubqueryCouldBeJoin, TruncateTable, @@ -106,6 +107,7 @@ LeadingWildcardLike(), OrAcrossColumns(), CountDistinctUnbounded(), + SelectDistinctSuspicious(), TruncateTable(), HavingWithoutGroupBy(), CrossJoinExplicit(), diff --git a/sql_guard/rules/warnings.py b/sql_guard/rules/warnings.py index 88608e8..132b87f 100644 --- a/sql_guard/rules/warnings.py +++ b/sql_guard/rules/warnings.py @@ -4,7 +4,7 @@ import re -from sql_guard.rules.base import Finding, Rule +from sql_guard.rules.base import Finding, Rule, strip_strings_and_comments class SelectStar(Rule): @@ -510,6 +510,56 @@ def check_line(self, line: str, line_number: int, file: str) -> Finding | None: return None +class SelectDistinctSuspicious(Rule): + """W024: ``SELECT DISTINCT`` paired with ``JOIN`` is often a band-aid. + + Developers reach for ``SELECT DISTINCT`` to deduplicate rows when a + JOIN cardinality is wider than they expected, often because a join + condition is missing or because what they actually want is a + ``GROUP BY``. The result reads more rows than necessary and hides + the underlying join-condition bug. + + Fires when a statement contains both top-level ``SELECT DISTINCT`` + and any ``JOIN`` keyword. Standalone ``SELECT DISTINCT col FROM t`` + (no join) is fine -- the smell is the cross-join blow-up pattern, + not legitimate single-table uniqueness. + + ``COUNT(DISTINCT col)`` and other aggregate-DISTINCT forms are not + flagged: the regex anchors on ``SELECT`` directly preceding + ``DISTINCT``, so ``SELECT COUNT(DISTINCT x) FROM t JOIN u`` does + not match. + """ + + id = "W024" + name = "select-distinct-suspicious" + severity = "warning" + description = "SELECT DISTINCT combined with JOIN often masks a missing join condition" + multiline = True + + _select_distinct = Rule._compile(r"\bSELECT\s+DISTINCT\b") + _join = Rule._compile(r"\bJOIN\b") + + def check_statement(self, statement: str, start_line: int, file: str) -> Finding | None: + # Strip strings and comments so DISTINCT or JOIN inside literal + # text or comments cannot trigger a false positive. + stripped = strip_strings_and_comments(statement) + if self._select_distinct.search(stripped) and self._join.search(stripped): + return Finding( + rule_id=self.id, + severity=self.severity, + file=file, + line=start_line, + message=( + "SELECT DISTINCT with JOIN often masks a missing join condition or grouping" + ), + suggestion=( + "Verify the JOIN condition is correct and DISTINCT is genuinely needed; " + "consider GROUP BY if you are hiding row duplication" + ), + ) + return None + + class CountDistinctUnbounded(Rule): """W019: ``COUNT(DISTINCT col)`` on an unfiltered table. diff --git a/tests/test_new_rules.py b/tests/test_new_rules.py index 182c250..b5d31ce 100644 --- a/tests/test_new_rules.py +++ b/tests/test_new_rules.py @@ -10,6 +10,7 @@ LeadingWildcardLike, OrAcrossColumns, ScalarUdfInWhere, + SelectDistinctSuspicious, TruncateTable, ) @@ -387,3 +388,85 @@ def test_w022_does_not_flag_cross_join_inside_trailing_comment(): ) is None ) + + +# W024 select-distinct-suspicious --------------------------------------------- + + +def test_w024_flags_distinct_with_inner_join(): + rule = SelectDistinctSuspicious() + finding = _stmt( + rule, + "SELECT DISTINCT c.id, c.name FROM customers c JOIN orders o ON c.id = o.customer_id;", + ) + assert finding is not None + assert finding.rule_id == "W024" + assert finding.severity == "warning" + + +def test_w024_flags_distinct_with_left_join(): + rule = SelectDistinctSuspicious() + sql = "SELECT DISTINCT a.id FROM a LEFT JOIN b ON a.id = b.a_id;" + assert _stmt(rule, sql) is not None + + +def test_w024_flags_distinct_with_join_multiline(): + rule = SelectDistinctSuspicious() + sql = ( + "SELECT DISTINCT\n c.id, c.name\nFROM customers c\nJOIN orders o ON c.id = o.customer_id;" + ) + assert _stmt(rule, sql) is not None + + +def test_w024_case_insensitive(): + rule = SelectDistinctSuspicious() + assert _stmt(rule, "select distinct a from x join y on x.id = y.id;") is not None + + +def test_w024_does_not_flag_distinct_alone(): + # Single-table DISTINCT is fine -- no JOIN cardinality blow-up to mask. + rule = SelectDistinctSuspicious() + assert _stmt(rule, "SELECT DISTINCT country FROM customers;") is None + + +def test_w024_does_not_flag_count_distinct_with_join(): + # Aggregate-DISTINCT is a different pattern; the regex anchors on + # "SELECT DISTINCT" directly, not "COUNT(DISTINCT ...)". + rule = SelectDistinctSuspicious() + sql = "SELECT COUNT(DISTINCT c.id) FROM customers c JOIN orders o ON c.id = o.customer_id;" + assert _stmt(rule, sql) is None + + +def test_w024_does_not_flag_sum_distinct_with_join(): + rule = SelectDistinctSuspicious() + sql = "SELECT SUM(DISTINCT amount) FROM payments p JOIN customers c ON p.cust_id = c.id;" + assert _stmt(rule, sql) is None + + +def test_w024_does_not_flag_join_without_distinct(): + rule = SelectDistinctSuspicious() + assert _stmt(rule, "SELECT a.id FROM a JOIN b ON a.id = b.a_id;") is None + + +def test_w024_does_not_flag_distinct_inside_string_literal(): + rule = SelectDistinctSuspicious() + sql = "INSERT INTO log(msg) SELECT 'SELECT DISTINCT x JOIN y' FROM t JOIN u ON t.id = u.id;" + # Outer SELECT does not have DISTINCT; the literal mentions it. + assert _stmt(rule, sql) is None + + +def test_w024_does_not_flag_distinct_inside_comment(): + rule = SelectDistinctSuspicious() + sql = "-- SELECT DISTINCT x FROM y JOIN z\nSELECT id FROM t;" + assert _stmt(rule, sql) is None + + +def test_w024_message_mentions_join_or_grouping(): + rule = SelectDistinctSuspicious() + finding = _stmt( + rule, + "SELECT DISTINCT a FROM x JOIN y ON x.id = y.id;", + ) + assert finding is not None + msg = finding.message.lower() + assert "join" in msg or "grouping" in msg diff --git a/tests/test_rules.py b/tests/test_rules.py index 1e9397c..824a02c 100644 --- a/tests/test_rules.py +++ b/tests/test_rules.py @@ -18,19 +18,19 @@ class TestRuleRegistry: def test_all_rules_loaded(self) -> None: - assert len(ALL_RULES) == 41 + assert len(ALL_RULES) == 42 def test_11_errors(self) -> None: # 9 E-series + 2 T-series (T002 xp-cmdshell, T004 deprecated-outer-join). errors = [r for r in ALL_RULES if r.severity == "error"] assert len(errors) == 11 - def test_30_warnings(self) -> None: - # 23 W-series + 3 S-series + 4 T-series (T001 with-nolock, + def test_31_warnings(self) -> None: + # 24 W-series + 3 S-series + 4 T-series (T001 with-nolock, # T003 cursor-declaration, T005 create-index-without-online, # T006 select-into-without-typed-fields). warnings = [r for r in ALL_RULES if r.severity == "warning"] - assert len(warnings) == 30 + assert len(warnings) == 31 def test_unique_ids(self) -> None: ids = [r.id for r in ALL_RULES]