Add E009 update-from-without-join (T-SQL implicit-join Cartesian risk) (#47)

* Lift strip_strings_and_comments helper into base.py

Moves the SQL string/comment stripper that S001 introduced into a
package-shared helper exposed from sql_guard.rules.base. structural.py
now imports it; the next rule (E009) and any future paren-aware rule
get the same behaviour without duplicating the regexes.

Behaviour-preserving refactor only. structural test suite stays at
23 passing.

* Add E009 update-from-without-join rule

T-SQL accepts UPDATE customers SET status = o.status FROM customers c,
orders o WHERE c.customer_id = o.customer_id. The comma form is a
legacy implicit join: get the WHERE wrong (or omit it) and every row
in the target table is updated against the Cartesian product. Silent
data corruption, no syntax error.

Reported as severity=error rather than warning (the sister rule S001
covers SELECT FROM comma-joins as a warning) because the failure mode
here is a write that touches every row of a table, not a read.

Implementation mirrors the post-fix S001: walk from each UPDATE
keyword to the next FROM, then forward through the FROM clause
tracking parenthesis depth, stopping at WHERE / GROUP BY / ORDER BY /
HAVING / LIMIT / explicit JOIN / UNION / EXCEPT / INTERSECT, flagging
the first depth-0 comma. Uses the strip_strings_and_comments helper
lifted to base.py in the previous commit. Comma followed by LATERAL
is recognised as a legitimate Snowflake/Postgres lateral join.

Hand-tested against 12 cases (4 should-flag, 8 should-pass) including
the issue example, multi-line layout, three-way comma joins, Postgres
single-table UPDATE FROM, LATERAL, LEFT JOIN, and CTE-with-UPDATE.

Registry counts updated: 40 total (was 39), 11 errors (was 10).

Closes #42.

* Add E009 update-from-without-join tests

Adds an E009 line to the errors.sql fixture (T-SQL UPDATE FROM with a
comma-separated table list, the issue example shape) and seven test
methods on TestErrorRules:

- test_e009_update_from_implicit_join: fixture-based, asserts at
  least one E009 finding with cartesian / comma-separated wording
- test_e009_explicit_join_ok: the rule message's recommended fix
  pattern (UPDATE ... FROM a INNER JOIN b ON ...) must not flag
- test_e009_postgres_single_from_table_ok: canonical Postgres
  UPDATE ... FROM single-table form
- test_e009_lateral_after_comma_ok: , LATERAL is a real
  Snowflake/Postgres lateral join, not flagged
- test_e009_update_without_from_ok: plain UPDATE with no FROM
- test_e009_three_table_comma_join_flagged
- test_e009_multiline_comma_join_flagged

test_rules.py 49 pass, full suite 226 pass / 1 skipped.

* Document E009 in CHANGELOG under [Unreleased] Added

* style: pre-commit auto-fixes

[pre-commit.ci] auto-applied fixes from configured hooks

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: Pawansingh3889

CHANGELOG.md

@@ -22,6 +22,18 @@ a deprecation window (see `GOVERNANCE.md` § Scope discipline).
   Contributed by [@mvanhorn](https://github.com/mvanhorn)
   ([#39](https://github.com/Pawansingh3889/sql-guard/pull/39)).
   Resolves #3.
+- **E009 `update-from-without-join`** (error) - flags
+  `UPDATE ... FROM a, b WHERE ...` and similar comma-separated FROM
+  clauses on UPDATE statements. T-SQL accepts this legacy implicit
+  join: get the WHERE wrong (or omit it) and every row in the target
+  table is updated against the Cartesian product. Severity is error
+  rather than warning (the sister rule S001 covers SELECT FROM
+  comma-joins as a warning) because the failure mode is a write that
+  touches every row of a table. Walk-from-UPDATE-keyword,
+  paren-depth-aware scan reusing the `strip_strings_and_comments`
+  helper from `base.py`. `, LATERAL ...` is recognised as a
+  legitimate Snowflake/Postgres lateral join and not flagged.
+  Resolves #42.
 
 ### Fixed
 

sql_guard/rules/__init__.py

@@ -11,6 +11,7 @@
     GrantRevoke,
     InsertWithoutColumns,
     StringConcatInWhere,
+    UpdateFromImplicitJoin,
     UpdateWithoutWhere,
 )
 from sql_guard.rules.warnings import (
@@ -74,7 +75,7 @@
 ]
 
 ALL_RULES: list[Rule] = [
-    # Errors (E001-E008)
+    # Errors (E001-E009)
     DeleteWithoutWhere(),
     DropWithoutIfExists(),
     GrantRevoke(),
@@ -83,6 +84,7 @@
     UpdateWithoutWhere(),
     AlterAddNotNullNoDefault(),
     DropColumn(),
+    UpdateFromImplicitJoin(),
     # Warnings (W001-W023 with gaps)
     SelectStar(),
     MissingLimit(),

sql_guard/rules/base.py

@@ -6,6 +6,26 @@
 from dataclasses import dataclass
 
 
+_SQL_STRING = re.compile(r"'(?:[^']|'')*'")
+_SQL_LINE_COMMENT = re.compile(r"--[^\n]*")
+_SQL_BLOCK_COMMENT = re.compile(r"/\*.*?\*/", re.DOTALL)
+
+
+def strip_strings_and_comments(text: str) -> str:
+    """Replace SQL string literals and comments with empty equivalents.
+
+    Single-quoted strings (with `''` escapes), `--` line comments, and
+    `/* ... */` block comments are all removed (strings collapse to ``''``,
+    comments to empty). Useful before paren-depth or keyword scanning so
+    commas, parentheses, or keywords inside literals/comments do not
+    affect the scan.
+    """
+    text = _SQL_STRING.sub("''", text)
+    text = _SQL_LINE_COMMENT.sub("", text)
+    text = _SQL_BLOCK_COMMENT.sub("", text)
+    return text
+
+
 @dataclass
 class Finding:
     """A single issue found by a rule."""

sql_guard/rules/errors.py

@@ -1,8 +1,10 @@
-"""Error rules (E001-E008) -- these block commits."""
+"""Error rules (E001-E009) -- these block commits."""
 
 from __future__ import annotations
 
-from sql_guard.rules.base import Finding, Rule
+import re
+
+from sql_guard.rules.base import Finding, Rule, strip_strings_and_comments
 
 
 class DeleteWithoutWhere(Rule):
@@ -223,3 +225,118 @@ def check_statement(self, statement: str, start_line: int, file: str) -> Finding
                 suggestion="Stop reading the column for one release, then drop in a follow-up",
             )
         return None
+
+
+class UpdateFromImplicitJoin(Rule):
+    """E009: ``UPDATE ... FROM`` with comma-separated tables.
+
+    T-SQL accepts ``UPDATE customers SET status = o.status FROM customers c,
+    orders o WHERE c.customer_id = o.customer_id``. The comma form is a
+    legacy implicit join: get the WHERE wrong (or omit it) and every row in
+    the target table is updated against the Cartesian product. Silent data
+    corruption, no syntax error.
+
+    Reported as **error** rather than warning (the sister rule S001 covers
+    `SELECT FROM` comma-joins as a warning) because the failure mode here
+    is a write that touches every row of a table, not a read.
+
+    The scan walks from each ``UPDATE`` keyword to the next ``FROM``, then
+    forward through the ``FROM`` clause tracking parenthesis depth, stops
+    at ``WHERE`` / ``GROUP BY`` / ``ORDER BY`` / ``HAVING`` / ``LIMIT`` /
+    explicit ``JOIN`` / ``UNION`` / ``EXCEPT`` / ``INTERSECT``, and flags
+    the first depth-0 comma. Strings and comments are stripped first.
+    Comma followed by ``LATERAL`` is recognised as a legitimate
+    Snowflake/Postgres lateral join and not flagged.
+
+    Suppress with an inline ``-- noqa: E009`` comment on the same line, or
+    use the project-wide ``-- sql-guard: disable=E009`` directive.
+    """
+
+    id = "E009"
+    name = "update-from-without-join"
+    severity = "error"
+    description = "UPDATE ... FROM with comma-separated tables silently creates a Cartesian product"
+    multiline = True
+
+    _update_pattern = re.compile(r"\bUPDATE\b", re.IGNORECASE)
+    _from_pattern = re.compile(r"\bFROM\b", re.IGNORECASE)
+    _stop_pattern = re.compile(
+        r"\b("
+        r"WHERE|"
+        r"GROUP\s+BY|ORDER\s+BY|HAVING|"
+        r"LIMIT|FETCH|OFFSET|"
+        r"INNER\s+JOIN|"
+        r"LEFT\s+(?:OUTER\s+)?JOIN|"
+        r"RIGHT\s+(?:OUTER\s+)?JOIN|"
+        r"FULL\s+(?:OUTER\s+)?JOIN|"
+        r"CROSS\s+JOIN|"
+        r"NATURAL\s+(?:INNER\s+|LEFT\s+|RIGHT\s+|FULL\s+)?JOIN|"
+        r"JOIN|"
+        r"UNION|EXCEPT|INTERSECT"
+        r")\b",
+        re.IGNORECASE,
+    )
+    _lateral_pattern = re.compile(r"\s*LATERAL\b", re.IGNORECASE)
+
+    def check_statement(self, statement: str, start_line: int, file: str) -> Finding | None:
+        cleaned = strip_strings_and_comments(statement)
+        n = len(cleaned)
+
+        update_match = self._update_pattern.search(cleaned)
+        if not update_match:
+            return None
+
+        # The FROM clause must come after the UPDATE keyword. A FROM that
+        # appears before UPDATE (e.g. inside a CTE) does not belong to this
+        # UPDATE.
+        from_match = self._from_pattern.search(cleaned, update_match.end())
+        if not from_match:
+            return None
+
+        depth = 0
+        i = from_match.end()
+        while i < n:
+            ch = cleaned[i]
+
+            if ch == "(":
+                depth += 1
+                i += 1
+                continue
+            if ch == ")":
+                depth -= 1
+                if depth < 0:
+                    break
+                i += 1
+                continue
+
+            if depth != 0:
+                i += 1
+                continue
+
+            stop = self._stop_pattern.match(cleaned, i)
+            if stop:
+                break
+            if ch == ";":
+                break
+            if ch == ",":
+                if self._lateral_pattern.match(cleaned, i + 1):
+                    i += 1
+                    continue
+                line_offset = statement[:i].count("\n")
+                return Finding(
+                    rule_id=self.id,
+                    severity=self.severity,
+                    file=file,
+                    line=start_line + line_offset,
+                    message=(
+                        "UPDATE ... FROM with comma-separated tables -- "
+                        "silent Cartesian product risks corrupting every row"
+                    ),
+                    suggestion=(
+                        "Use UPDATE ... FROM a INNER JOIN b ON a.id = b.id "
+                        "(explicit JOIN with ON clause)"
+                    ),
+                )
+
+            i += 1
+        return None

sql_guard/rules/structural.py

@@ -12,7 +12,7 @@
 
 import re
 
-from sql_guard.rules.base import Finding, Rule
+from sql_guard.rules.base import Finding, Rule, strip_strings_and_comments
 
 try:
     import sqlparse
@@ -23,24 +23,6 @@
     HAS_SQLPARSE = False
 
 
-_SQL_STRING = re.compile(r"'(?:[^']|'')*'")
-_SQL_LINE_COMMENT = re.compile(r"--[^\n]*")
-_SQL_BLOCK_COMMENT = re.compile(r"/\*.*?\*/", re.DOTALL)
-
-
-def _strip_strings_and_comments(text: str) -> str:
-    """Replace string literals and comments with empty equivalents.
-
-    Preserves character positions roughly (replaces strings with ``''`` and
-    comments with empty), so commas inside literals or comments do not
-    trigger comma-join detection but the surrounding structure is unchanged.
-    """
-    text = _SQL_STRING.sub("''", text)
-    text = _SQL_LINE_COMMENT.sub("", text)
-    text = _SQL_BLOCK_COMMENT.sub("", text)
-    return text
-
-
 class ImplicitCrossJoin(Rule):
     """S001: Implicit cross join via comma-separated tables in FROM.
 
@@ -89,7 +71,7 @@ class ImplicitCrossJoin(Rule):
     _lateral_pattern = re.compile(r"\s*LATERAL\b", re.IGNORECASE)
 
     def check_statement(self, statement: str, start_line: int, file: str) -> Finding | None:
-        cleaned = _strip_strings_and_comments(statement)
+        cleaned = strip_strings_and_comments(statement)
         n = len(cleaned)
 
         for from_match in self._from_pattern.finditer(cleaned):

tests/fixtures/errors.sql

@@ -17,3 +17,6 @@ INSERT INTO orders VALUES (1, 'test', 100);
 
 -- E006: UPDATE without WHERE
 UPDATE orders SET status = 'cancelled';
+
+-- E009: UPDATE FROM with comma-separated tables (T-SQL legacy implicit join)
+UPDATE customers SET status = o.status FROM customers c, orders o WHERE c.customer_id = o.customer_id;

tests/test_rules.py

@@ -18,12 +18,12 @@
 
 class TestRuleRegistry:
     def test_all_rules_loaded(self) -> None:
-        assert len(ALL_RULES) == 39
+        assert len(ALL_RULES) == 40
 
-    def test_10_errors(self) -> None:
-        # 8 E-series + 2 T-series (T002 xp-cmdshell, T004 deprecated-outer-join).
+    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) == 10
+        assert len(errors) == 11
 
     def test_29_warnings(self) -> None:
         # 23 W-series + 3 S-series + 3 T-series (T001 with-nolock,
@@ -91,6 +91,82 @@ def test_e006_update_with_where_ok(self, tmp_path) -> None:
         e006 = [f for f in result.findings if f.rule_id == "E006"]
         assert not e006
 
+    def test_e009_update_from_implicit_join(self) -> None:
+        findings = check([str(FIXTURES / "errors.sql")])
+        e009 = [f for f in findings.findings if f.rule_id == "E009"]
+        assert len(e009) >= 1
+        assert "UPDATE" in e009[0].message
+        assert (
+            "cartesian" in e009[0].message.lower() or "comma-separated" in e009[0].message.lower()
+        )
+
+    def test_e009_explicit_join_ok(self, tmp_path) -> None:
+        # The recommended fix from the rule message must NOT trigger E009.
+        sql = tmp_path / "safe_update_from.sql"
+        sql.write_text(
+            "UPDATE c SET c.status = o.status "
+            "FROM customers c INNER JOIN orders o "
+            "ON c.customer_id = o.customer_id;\n"
+        )
+        result = check([str(sql)])
+        e009 = [f for f in result.findings if f.rule_id == "E009"]
+        assert not e009
+
+    def test_e009_postgres_single_from_table_ok(self, tmp_path) -> None:
+        # Postgres UPDATE ... FROM with a single table is the canonical
+        # form and must not flag.
+        sql = tmp_path / "postgres_update.sql"
+        sql.write_text(
+            "UPDATE customers SET status = o.status "
+            "FROM orders o WHERE customers.id = o.customer_id;\n"
+        )
+        result = check([str(sql)])
+        e009 = [f for f in result.findings if f.rule_id == "E009"]
+        assert not e009
+
+    def test_e009_lateral_after_comma_ok(self, tmp_path) -> None:
+        # `, LATERAL ...` is a real Snowflake / Postgres lateral join.
+        sql = tmp_path / "lateral_update.sql"
+        sql.write_text(
+            "UPDATE c SET tag = sub.tag FROM customers c, "
+            "LATERAL (SELECT tag FROM tags WHERE customer_id = c.id LIMIT 1) sub;\n"
+        )
+        result = check([str(sql)])
+        e009 = [f for f in result.findings if f.rule_id == "E009"]
+        assert not e009
+
+    def test_e009_update_without_from_ok(self, tmp_path) -> None:
+        # No FROM clause at all is a plain single-table UPDATE.
+        sql = tmp_path / "plain_update.sql"
+        sql.write_text("UPDATE customers SET status = 'active' WHERE id = 1;\n")
+        result = check([str(sql)])
+        e009 = [f for f in result.findings if f.rule_id == "E009"]
+        assert not e009
+
+    def test_e009_three_table_comma_join_flagged(self, tmp_path) -> None:
+        sql = tmp_path / "three_table.sql"
+        sql.write_text(
+            "UPDATE c SET c.label = p.label "
+            "FROM customers c, orders o, products p "
+            "WHERE c.id = o.customer_id AND o.product_id = p.id;\n"
+        )
+        result = check([str(sql)])
+        e009 = [f for f in result.findings if f.rule_id == "E009"]
+        assert len(e009) == 1
+
+    def test_e009_multiline_comma_join_flagged(self, tmp_path) -> None:
+        sql = tmp_path / "multiline.sql"
+        sql.write_text(
+            "UPDATE customers\n"
+            "SET status = o.status\n"
+            "FROM customers c,\n"
+            "     orders o\n"
+            "WHERE c.customer_id = o.customer_id;\n"
+        )
+        result = check([str(sql)])
+        e009 = [f for f in result.findings if f.rule_id == "E009"]
+        assert len(e009) == 1
+
 
 # ---------------------------------------------------------------------------
 # Warning rules