diff --git a/.agents/docs/sql-guidelines.md b/.agents/docs/sql-guidelines.md index f4b2a492e0..5ae1f9e011 100644 --- a/.agents/docs/sql-guidelines.md +++ b/.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_.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). diff --git a/.coderabbit.yml b/.coderabbit.yml index 90efecd69e..c25b087967 100644 --- a/.coderabbit.yml +++ b/.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/**" diff --git a/apps/codestyle/codestyle-sql.py b/apps/codestyle/codestyle-sql.py index 5833e65c27..9d75505d90 100644 --- a/apps/codestyle/codestyle-sql.py +++ b/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}`|(? 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"(? 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