feat(Codestyle/SQL): require both id and guid in spawn UPDATEs (#27657)

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>

.agents/docs/sql-guidelines.md

@@ -4,7 +4,7 @@
 
 1. `cd data/sql/updates/pending_db_world/` (or `pending_db_auth` / `pending_db_characters`).
 2. `./create_sql.sh` generates an empty `rev_<timestamp>.sql` to write into.
-3. Conventions (linted): every `INSERT` preceded by a matching `DELETE` (idempotency); spawn `DELETE` (`creature`, `gameobject`) filters on both `id` and `guid` with `=`/`IN`/`BETWEEN`, never `OR`; no double semicolons; no multiple blank lines; InnoDB engine.
+3. Conventions (linted): every `INSERT` preceded by a matching `DELETE` (idempotency); spawn `DELETE` and `UPDATE` (`creature`, `gameobject`) target that table alone (no join, comma list or schema qualifier) and filter on both `id` and `guid` with `=`/`IN`/`BETWEEN`, never `OR`; no double semicolons; no multiple blank lines; InnoDB engine.
 
 Run the linter before claiming a change is done: `python apps/codestyle/codestyle-sql.py` (compares to origin/master).
 

.coderabbit.yml

@@ -102,6 +102,7 @@ reviews:
       instructions: |
         AzerothCore SQL update conventions (enforced by apps/codestyle/codestyle-sql.py):
         - Every INSERT must be preceded by a matching DELETE for idempotency, and that DELETE must include a WHERE clause scoped precisely to the intended rows. A predicate that is too broad will remove unrelated data, so confirm it matches exactly what the INSERT will re-add.
+        - Spawn tables (creature, gameobject): DELETE and UPDATE must target the table alone (no join, comma list or schema qualifier) and filter on both id and guid, with =, IN or BETWEEN (a mix is fine), never OR.
         - 4-space indent (no tabs), trailing newline, no double semicolons, no multiple blank lines.
         - Tables must use the InnoDB engine.
     - path: "data/sql/base/**"

apps/codestyle/codestyle-sql.py

@@ -25,7 +25,7 @@ results = {
     "Multiple blank lines check": "Passed",
     "Trailing whitespace check": "Passed",
     "SQL codestyle check": "Passed",
-    "INSERT & DELETE safety usage check": "Passed",
+    "INSERT, UPDATE & DELETE safety usage check": "Passed",
     "Missing semicolon check": "Passed",
     "Backtick check": "Passed",
     "Directory check": "Passed",
@@ -205,13 +205,13 @@ def insert_delete_safety_check(file: io, file_path: str) -> None:
                     f"❌ Entries from {table_name} should not be deleted! {file_path} at line {line_number}\nIf this error is intended, please notify a maintainer")
                 check_failed = True
 
-    if spawn_delete_filter_check(file, file_path):
+    if spawn_filter_check(file, file_path):
         check_failed = True
 
     # Handle the script error and update the result output
     if check_failed:
         error_handler = True
-        results["INSERT & DELETE safety usage check"] = "Failed"
+        results["INSERT, UPDATE & DELETE safety usage check"] = "Failed"
 
 # Strip a trailing "-- ..." line comment while ignoring any "--" that appears
 # inside a single- or double-quoted string literal (e.g. descriptions).
@@ -243,9 +243,19 @@ def open_paren_balance(text: str) -> int:
     without_strings = re.sub(r'"(?:\\.|[^"])*"', "", without_strings)
     return without_strings.count('(') - without_strings.count(')')
 
-DELETE_START = re.compile(r"\bDELETE\b", re.IGNORECASE)
-SPAWN_DELETE_START = re.compile(r"DELETE\s+FROM\s+(?:`(creature|gameobject)`|\b(creature|gameobject)\b)", re.IGNORECASE)
-# Only these bound a delete to known rows: `guid` > 0 or `id` != 5 match without limiting.
+SPAWN_STATEMENT_START = re.compile(r"\b(?:DELETE|UPDATE)\b", re.IGNORECASE)
+SPAWN_TABLE = r"(?:`(creature|gameobject)`|\b(creature|gameobject)\b)"
+# The target list, captured whole rather than matched table-first, so a spawn table named second
+# in a join or a comma list is seen too. Anchored and requiring FROM/SET, which keeps the
+# `ON DELETE CASCADE` and `ON UPDATE CURRENT_TIMESTAMP` of a DDL statement out.
+SPAWN_DELETE_TARGETS = re.compile(
+    r"^\s*DELETE\s+(?:(?:LOW_PRIORITY|QUICK|IGNORE)\s+)*(.*?)\bFROM\s+(.*?)(?:\bWHERE\b|$)",
+    re.IGNORECASE | re.DOTALL)
+SPAWN_UPDATE_TARGETS = re.compile(
+    r"^\s*UPDATE\s+(?:(?:LOW_PRIORITY|IGNORE)\s+)*(.*?)\bSET\b", re.IGNORECASE | re.DOTALL)
+# One table, optionally aliased. Anything else is a join, a comma list or a schema qualifier.
+PLAIN_SPAWN_TARGET = re.compile(rf"^\s*{SPAWN_TABLE}(?:\s+(?:AS\s+)?`?\w+`?)?\s*$", re.IGNORECASE)
+# Only these bound a statement to known rows: `guid` > 0 or `id` != 5 match without limiting.
 SPAWN_FILTER_OPERATORS = r"(?:=|\bIN\b|\bBETWEEN\b)"
 
 # The column token has to be bounded on both sides, otherwise `guid` would satisfy the `id`
@@ -254,6 +264,35 @@ def has_column_filter(statement: str, column: str) -> bool:
     pattern = rf"(?:`{column}`|(?<![\w@`]){column}(?![\w`]))\s*{SPAWN_FILTER_OPERATORS}"
     return re.search(pattern, statement, re.IGNORECASE) is not None
 
+SPAWN_TABLE_MENTION = re.compile(SPAWN_TABLE, re.IGNORECASE)
+SUBQUERY_START = re.compile(r"\(\s*(?:SELECT|WITH)\b", re.IGNORECASE)
+
+# A subquery bounds its own rows, not the ones the statement touches, so its predicates must not be
+# read as filters. Parentheses that merely group a predicate are kept, those are part of the filter.
+def strip_subqueries(text: str) -> str:
+    while True:
+        match = SUBQUERY_START.search(text)
+        if not match:
+            return text
+        depth = 0
+        for index in range(match.start(), len(text)):
+            if text[index] == '(':
+                depth += 1
+            elif text[index] == ')':
+                depth -= 1
+                if depth == 0:
+                    text = text[:match.start()] + " " + text[index + 1:]
+                    break
+        else:
+            return text  # unbalanced, so leave it whole rather than judge half a statement
+
+# Only the WHERE clause decides which rows are hit, so the `SET id` = ... of an UPDATE must not
+# count as a filter. Returns "" when there is no WHERE at all, which then reads as unfiltered.
+def where_clause(statement: str) -> str:
+    # A backticked `where` column would otherwise split the statement mid-SET
+    parts = re.split(r"(?<!`)\bWHERE\b(?!`)", strip_subqueries(statement), maxsplit = 1, flags = re.IGNORECASE)
+    return parts[1] if len(parts) > 1 else ""
+
 # Walk the line left to right dropping quoted literals and both comment styles, so a "--", a "/*"
 # or a ";" inside a string is not taken for a comment or a statement terminator. Returns the
 # sanitised text plus whether a block comment is left open for the following lines.
@@ -292,38 +331,58 @@ def strip_sql_noise(text: str, in_block_comment: bool) -> tuple:
         index += 1
     return ''.join(sanitized).strip(), in_block_comment
 
-# Spawns in `creature` and `gameobject` must be deleted by both `id` and `guid`: a guid-only delete
-# wipes whatever spawn owns that guid today, an id-only one wipes every spawn of that entry in the
-# world. Returns whether a violation was found; the caller owns the result state.
-def spawn_delete_filter_check(file: io, file_path: str) -> bool:
+# Spawns in `creature` and `gameobject` must be matched by both `id` and `guid`: a guid-only
+# statement hits whatever spawn owns that guid today, an id-only one hits every spawn of that entry
+# in the world. Returns whether a violation was found; the caller owns the result state.
+def spawn_filter_check(file: io, file_path: str) -> bool:
     file.seek(0)  # Reset file pointer to the beginning
     check_failed = False
     in_block_comment = False
     statement = ""
     statement_line = 0
 
-    def report(text: str, line_number: int, table: str) -> bool:
+    def report(clause: str, line_number: int, statement_name: str) -> bool:
         # A disjunction needs real boolean parsing to judge, so it is refused rather than guessed at
-        if re.search(r"\bOR\b", text, re.IGNORECASE):
-            print(f"❌ DELETE FROM `{table}` must not use OR. Use IN, or split it into one statement per "
+        if re.search(r"\bOR\b", clause, re.IGNORECASE):
+            print(f"❌ {statement_name} must not use OR. Use IN, or split it into one statement per "
                   f"spawn. {file_path} at line {line_number}\n"
                   f"If this error is intended, please notify a maintainer")
             return True
-        missing = [column for column in ("id", "guid") if not has_column_filter(text, column)]
+        missing = [column for column in ("id", "guid") if not has_column_filter(clause, column)]
         if not missing:
             return False
         columns = " and ".join(f"`{column}`" for column in missing)
-        print(f"❌ DELETE FROM `{table}` must filter on both `id` and `guid` (missing: {columns}). "
+        print(f"❌ {statement_name} must filter on both `id` and `guid` (missing: {columns}). "
+              f"{file_path} at line {line_number}\nIf this error is intended, please notify a maintainer")
+        return True
+
+    # Which rows a join or a comma list touches cannot be judged without resolving aliases, and a
+    # schema qualifier is wrong anyway since the database name is configurable, so the form is
+    # refused rather than guessed at.
+    def report_indirect_target(targets: str, line_number: int, table: str) -> bool:
+        print(f"❌ A statement touching `{table}` must name it as its only target. Joins, "
+              f"comma-separated targets and schema qualifiers are not supported. "
               f"{file_path} at line {line_number}\nIf this error is intended, please notify a maintainer")
         return True
 
     # Judged once the whole statement is accumulated, since DELETE, FROM and the table name can
     # each sit on their own line
-    def report_when_spawn_delete(text: str, line_number: int) -> bool:
-        table = SPAWN_DELETE_START.search(text)
-        if not table:
-            return False
-        return report(text, line_number, table.group(1) or table.group(2))
+    def report_when_spawn_statement(text: str, line_number: int) -> bool:
+        statement = strip_subqueries(text)
+        for pattern, keyword in ((SPAWN_DELETE_TARGETS, "DELETE FROM"), (SPAWN_UPDATE_TARGETS, "UPDATE")):
+            match = pattern.match(statement)
+            if not match:
+                continue
+            # A multi-table DELETE names its targets before FROM, so both halves have to be plain
+            targets = " ".join(group for group in match.groups() if group).strip()
+            table = SPAWN_TABLE_MENTION.search(targets)
+            if not table:
+                return False
+            name = table.group(1) or table.group(2)
+            if not PLAIN_SPAWN_TARGET.match(targets):
+                return report_indirect_target(targets, line_number, name)
+            return report(where_clause(statement), line_number, f"{keyword} `{name}`")
+        return False
 
     for line_number, line in enumerate(file, start = 1):
         text, in_block_comment = strip_sql_noise(line.strip(), in_block_comment)
@@ -336,7 +395,7 @@ def spawn_delete_filter_check(file: io, file_path: str) -> bool:
                 segment, terminator, rest = remainder.partition(';')
                 statement += " " + segment
             else:
-                match = DELETE_START.search(remainder)
+                match = SPAWN_STATEMENT_START.search(remainder)
                 if not match:
                     break
                 statement_line = line_number
@@ -344,13 +403,13 @@ def spawn_delete_filter_check(file: io, file_path: str) -> bool:
                 statement = segment
             if not terminator:
                 break
-            if report_when_spawn_delete(statement, statement_line):
+            if report_when_spawn_statement(statement, statement_line):
                 check_failed = True
             statement = ""
             remainder = rest.strip()
 
     # An unterminated statement is reported by semicolon_check, but still judge it here
-    if statement and report_when_spawn_delete(statement, statement_line):
+    if statement and report_when_spawn_statement(statement, statement_line):
         check_failed = True
 
     return check_failed