diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 2afe2a7f..3291207e 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -34,6 +34,7 @@ use rules::ban_drop_column; use rules::ban_drop_database; use rules::ban_drop_not_null; use rules::ban_drop_table; +use rules::ban_truncate_cascade; use rules::changing_column_type; use rules::constraint_missing_not_valid; use rules::disallow_unique_constraint; @@ -108,6 +109,8 @@ pub enum Rule { BanCreateDomainWithConstraint, #[serde(rename = "ban-alter-domain-with-add-constraint")] BanAlterDomainWithAddConstraint, + #[serde(rename = "ban-truncate-cascade")] + BanTruncateCascade, // xtask:new-rule:error-name } @@ -145,6 +148,7 @@ impl TryFrom<&str> for Rule { } "ban-create-domain-with-constraint" => Ok(Rule::BanCreateDomainWithConstraint), "ban-alter-domain-with-add-constraint" => Ok(Rule::BanAlterDomainWithAddConstraint), + "ban-truncate-cascade" => Ok(Rule::BanTruncateCascade), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {}", s)), } @@ -202,6 +206,7 @@ impl fmt::Display for Rule { Rule::BanCreateDomainWithConstraint => "ban-create-domain-with-constraint", Rule::UnusedIgnore => "unused-ignore", Rule::BanAlterDomainWithAddConstraint => "ban-alter-domain-with-add-constraint", + Rule::BanTruncateCascade => "ban-truncate-cascade", // xtask:new-rule:variant-to-name }; write!(f, "{}", val) @@ -345,6 +350,9 @@ impl Linter { if self.rules.contains(&Rule::TransactionNesting) { transaction_nesting(self, &file); } + if self.rules.contains(&Rule::BanTruncateCascade) { + ban_truncate_cascade(self, &file); + } // xtask:new-rule:rule-call // locate any ignores in the file diff --git a/crates/squawk_linter/src/rules/ban_truncate_cascade.rs b/crates/squawk_linter/src/rules/ban_truncate_cascade.rs new file mode 100644 index 00000000..fef4249c --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_truncate_cascade.rs @@ -0,0 +1,60 @@ +use squawk_syntax::{ + ast::{self, HasModuleItem}, + Parse, SourceFile, +}; + +use crate::{Linter, Rule, Violation}; + +pub(crate) fn ban_truncate_cascade(ctx: &mut Linter, parse: &Parse) { + let file = parse.tree(); + for item in file.items() { + match item { + ast::Item::Truncate(truncate) => { + if let Some(cascade) = truncate.cascade_token() { + // TODO: if we had knowledge about the entire schema, we + // could be more precise here and actually navigate the + // foreign keys. + ctx.report(Violation::new( + Rule::BanTruncateCascade, + format!("Using `CASCADE` will recursively truncate any tables that foreign key to the referenced tables! So if you had foreign keys setup as `a <- b <- c` and truncated `a`, then `b` & `c` would also be truncated!"), + cascade.text_range(), + "Remove the `CASCADE` and specify exactly which tables you want to truncate.".to_string(), + )); + } + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use insta::assert_debug_snapshot; + + use crate::{Linter, Rule}; + use squawk_syntax::SourceFile; + + #[test] + fn err() { + let sql = r#" + truncate a, b, c cascade; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanTruncateCascade]); + let errors = linter.lint(file, sql); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors); + } + + #[test] + fn ok() { + let sql = r#" + truncate a, b, c; + truncate a; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanTruncateCascade]); + let errors = linter.lint(file, sql); + assert_eq!(errors.len(), 0); + } +} diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index e217cd4e..7886c25d 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -11,6 +11,7 @@ pub(crate) mod ban_drop_column; pub(crate) mod ban_drop_database; pub(crate) mod ban_drop_not_null; pub(crate) mod ban_drop_table; +pub(crate) mod ban_truncate_cascade; pub(crate) mod changing_column_type; pub(crate) mod constraint_missing_not_valid; pub(crate) mod disallow_unique_constraint; @@ -40,6 +41,7 @@ pub(crate) use ban_drop_column::ban_drop_column; pub(crate) use ban_drop_database::ban_drop_database; pub(crate) use ban_drop_not_null::ban_drop_not_null; pub(crate) use ban_drop_table::ban_drop_table; +pub(crate) use ban_truncate_cascade::ban_truncate_cascade; pub(crate) use changing_column_type::changing_column_type; pub(crate) use constraint_missing_not_valid::constraint_missing_not_valid; pub(crate) use disallow_unique_constraint::disallow_unique_constraint; diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_truncate_cascade__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_truncate_cascade__test__err.snap new file mode 100644 index 00000000..0166dcaf --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_truncate_cascade__test__err.snap @@ -0,0 +1,14 @@ +--- +source: crates/squawk_linter/src/rules/ban_truncate_cascade.rs +expression: errors +--- +[ + Violation { + code: BanTruncateCascade, + message: "Using `CASCADE` will recursively truncate any tables that foreign key to the referenced tables! So if you had foreign keys setup as `a <- b <- c` and truncated `a`, then `b` & `c` would also be truncated!", + text_range: 26..33, + help: Some( + "Remove the `CASCADE` and specify exactly which tables you want to truncate.", + ), + }, +] diff --git a/crates/squawk_parser/src/grammar.rs b/crates/squawk_parser/src/grammar.rs index 5c83d8c4..dbf87856 100644 --- a/crates/squawk_parser/src/grammar.rs +++ b/crates/squawk_parser/src/grammar.rs @@ -9781,11 +9781,7 @@ fn lock_stmt(p: &mut Parser<'_>) -> CompletedMarker { p.bump(LOCK_KW); // [ TABLE ] p.eat(TABLE_KW); - // [ ONLY ] name [ * ] [, ...] - relation_name(p); - while !p.at(EOF) && p.eat(COMMA) { - relation_name(p); - } + table_list(p); // [ IN lockmode MODE ] if p.eat(IN_KW) { match (p.current(), p.nth(1)) { @@ -9828,6 +9824,16 @@ fn lock_stmt(p: &mut Parser<'_>) -> CompletedMarker { m.complete(p, LOCK_STMT) } +// [ ONLY ] name [ * ] [, ... ] +fn table_list(p: &mut Parser<'_>) { + let m = p.start(); + relation_name(p); + while !p.at(EOF) && p.eat(COMMA) { + relation_name(p); + } + m.complete(p, TABLE_LIST); +} + // [ WITH with_query [, ...] ] // MERGE INTO [ ONLY ] target_table_name [ * ] [ [ AS ] target_alias ] // USING data_source ON join_condition @@ -11098,12 +11104,7 @@ fn truncate_stmt(p: &mut Parser<'_>) -> CompletedMarker { let m = p.start(); p.bump(TRUNCATE_KW); p.eat(TABLE_KW); - while !p.at(EOF) { - relation_name(p); - if !p.eat(COMMA) { - break; - } - } + table_list(p); if p.eat(RESTART_KW) { p.expect(IDENTITY_KW); } diff --git a/crates/squawk_parser/src/snapshots/squawk_parser__test__lock_ok.snap b/crates/squawk_parser/src/snapshots/squawk_parser__test__lock_ok.snap index 8fa39914..878979e4 100644 --- a/crates/squawk_parser/src/snapshots/squawk_parser__test__lock_ok.snap +++ b/crates/squawk_parser/src/snapshots/squawk_parser__test__lock_ok.snap @@ -8,10 +8,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" SEMICOLON ";" WHITESPACE "\n\n" COMMENT "-- table_names" @@ -21,26 +22,27 @@ SOURCE_FILE WHITESPACE " " TABLE_KW "table" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" - COMMA "," - WHITESPACE " " - ONLY_KW "only" - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "b" - COMMA "," - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "c" - WHITESPACE " " - STAR "*" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" + COMMA "," + WHITESPACE " " + ONLY_KW "only" + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "b" + COMMA "," + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "c" + WHITESPACE " " + STAR "*" SEMICOLON ";" WHITESPACE "\n\n" COMMENT "-- lock_mode" @@ -48,10 +50,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -65,10 +68,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -82,10 +86,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -99,10 +104,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -118,10 +124,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -133,10 +140,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -152,10 +160,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -167,10 +176,11 @@ SOURCE_FILE LOCK_STMT LOCK_KW "lock" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE " " IN_KW "in" WHITESPACE " " @@ -188,26 +198,27 @@ SOURCE_FILE WHITESPACE " " TABLE_KW "table" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" - COMMA "," - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "a" - WHITESPACE " " - STAR "*" - COMMA "," - WHITESPACE " " - ONLY_KW "only" - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "c" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" + COMMA "," + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "a" + WHITESPACE " " + STAR "*" + COMMA "," + WHITESPACE " " + ONLY_KW "only" + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "c" WHITESPACE " " IN_KW "in" WHITESPACE " " diff --git a/crates/squawk_parser/src/snapshots/squawk_parser__test__truncate_ok.snap b/crates/squawk_parser/src/snapshots/squawk_parser__test__truncate_ok.snap index 11d06eb7..2ab0f221 100644 --- a/crates/squawk_parser/src/snapshots/squawk_parser__test__truncate_ok.snap +++ b/crates/squawk_parser/src/snapshots/squawk_parser__test__truncate_ok.snap @@ -10,12 +10,13 @@ SOURCE_FILE WHITESPACE " " TABLE_KW "table" WHITESPACE " " - ONLY_KW "only" - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + ONLY_KW "only" + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" WHITESPACE "\n" RESTART_KW "restart" WHITESPACE " " @@ -29,26 +30,27 @@ SOURCE_FILE TRUNCATE_STMT TRUNCATE_KW "TRUNCATE" WHITESPACE " " - ONLY_KW "only" - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "a" - COMMA "," - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "b" - WHITESPACE " " - STAR "*" - COMMA "," - WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "c" + TABLE_LIST + ONLY_KW "only" + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "a" + COMMA "," + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "b" + WHITESPACE " " + STAR "*" + COMMA "," + WHITESPACE " " + PATH + PATH_SEGMENT + NAME_REF + IDENT "c" SEMICOLON ";" WHITESPACE "\n\n" COMMENT "-- rest" @@ -56,19 +58,21 @@ SOURCE_FILE TRUNCATE_STMT TRUNCATE_KW "truncate" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "t" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "t" SEMICOLON ";" WHITESPACE "\n" TRUNCATE_STMT TRUNCATE_KW "truncate" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "a" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "a" WHITESPACE " " CONTINUE_KW "continue" WHITESPACE " " @@ -78,10 +82,11 @@ SOURCE_FILE TRUNCATE_STMT TRUNCATE_KW "truncate" WHITESPACE " " - PATH - PATH_SEGMENT - NAME_REF - IDENT "a" + TABLE_LIST + PATH + PATH_SEGMENT + NAME_REF + IDENT "a" WHITESPACE " " CONTINUE_KW "continue" WHITESPACE " " diff --git a/crates/squawk_parser/src/syntax_kind.rs b/crates/squawk_parser/src/syntax_kind.rs index e5d84383..7cbec73e 100644 --- a/crates/squawk_parser/src/syntax_kind.rs +++ b/crates/squawk_parser/src/syntax_kind.rs @@ -1430,6 +1430,7 @@ pub enum SyntaxKind { ADD_CONSTRAINT, ADD_COLUMN, ATTACH_PARTITION, + TABLE_LIST, SET_SCHEMA, SET_TABLESPACE, SET_WITHOUT_CLUSTER, diff --git a/crates/squawk_syntax/src/ast/nodes.rs b/crates/squawk_syntax/src/ast/nodes.rs index ef1f5b61..92f3fe34 100644 --- a/crates/squawk_syntax/src/ast/nodes.rs +++ b/crates/squawk_syntax/src/ast/nodes.rs @@ -3115,6 +3115,65 @@ impl AstNode for Rollback { } } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct TableList { + pub(crate) syntax: SyntaxNode, +} + +impl AstNode for TableList { + #[inline] + fn can_cast(kind: SyntaxKind) -> bool { + kind == SyntaxKind::TABLE_LIST + } + #[inline] + fn cast(syntax: SyntaxNode) -> Option { + if Self::can_cast(syntax.kind()) { + Some(Self { syntax }) + } else { + None + } + } + #[inline] + fn syntax(&self) -> &SyntaxNode { + &self.syntax + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Truncate { + pub(crate) syntax: SyntaxNode, +} + +impl Truncate { + #[inline] + pub fn cascade_token(&self) -> Option { + support::token(&self.syntax, SyntaxKind::CASCADE_KW) + } + #[inline] + pub fn table_list(&self) -> Option { + support::child(&self.syntax) + } +} + +impl AstNode for Truncate { + #[inline] + fn can_cast(kind: SyntaxKind) -> bool { + kind == SyntaxKind::TRUNCATE_STMT + } + #[inline] + fn cast(syntax: SyntaxNode) -> Option { + if Self::can_cast(syntax.kind()) { + Some(Self { syntax }) + } else { + None + } + } + #[inline] + fn syntax(&self) -> &SyntaxNode { + &self.syntax + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Item { AlterTable(AlterTable), @@ -3134,6 +3193,7 @@ pub enum Item { DropIndex(DropIndex), DropType(DropType), Select(Select), + Truncate(Truncate), } // impl ast::HasAttrs for Item {} // impl ast::HasDocComments for Item {} @@ -3159,7 +3219,8 @@ impl AstNode for Item { | SyntaxKind::ALTER_DOMAIN_STMT | SyntaxKind::ALTER_AGGREGATE_STMT | SyntaxKind::CREATE_AGGREGATE_STMT - | SyntaxKind::ROLLBACK_KW + | SyntaxKind::ROLLBACK_STMT + | SyntaxKind::TRUNCATE_STMT ) } #[inline] @@ -3182,6 +3243,7 @@ impl AstNode for Item { SyntaxKind::CREATE_AGGREGATE_STMT => Item::CreateAggregate(CreateAggregate { syntax }), SyntaxKind::DROP_AGGREGATE_STMT => Item::DropAggregate(DropAggregate { syntax }), SyntaxKind::ROLLBACK_STMT => Item::Rollback(Rollback { syntax }), + SyntaxKind::TRUNCATE_STMT => Item::Truncate(Truncate { syntax }), _ => return None, }; Some(res) @@ -3206,6 +3268,7 @@ impl AstNode for Item { Item::CreateAggregate(it) => &it.syntax, Item::DropAggregate(it) => &it.syntax, Item::Rollback(it) => &it.syntax, + Item::Truncate(it) => &it.syntax, } } } diff --git a/docs/docs/ban-truncate-cascade.md b/docs/docs/ban-truncate-cascade.md new file mode 100644 index 00000000..f25dbe4e --- /dev/null +++ b/docs/docs/ban-truncate-cascade.md @@ -0,0 +1,77 @@ +--- +id: ban-truncate-cascade +title: ban-truncate-cascade +--- + +## problem + +Using `TRUNCATE`'s `CASCADE` option will truncate any tables that are also foreign-keyed to the specified tables. + +So if you had tables with foreign-keys like: + +``` +a <- b <- c +``` + +and ran: + +```sql +truncate a cascade; +``` + +You'd end up with `a`, `b`, & `c` all being truncated! + +### runnable example + +Setup: + +```sql +create table a ( + a_id int primary key +); +create table b ( + b_id int primary key, + a_id int, + foreign key (a_id) references a(a_id) +); +create table c ( + c_id int primary key, + b_id int, + foreign key (b_id) references b(b_id) +); +insert into a (a_id) values (1), (2), (3); +insert into b (b_id, a_id) values (101, 1), (102, 2), (103, 3); +insert into c (c_id, b_id) values (1001, 101), (1002, 102), (1003, 103); +``` + +Then you run: + +```sql +truncate a cascade; +``` + +Which outputs: + +```text +NOTICE: truncate cascades to table "b" +NOTICE: truncate cascades to table "c" + +Query 1 OK: TRUNCATE TABLE +``` + +And now tables `a`, `b`, & `c` are empty! + +## solution + +Don't use the `CASCADE` option, instead manually specify the tables you want. + +So if you just wanted tables `a` and `b` from the example above: + +```sql +truncate a, b; +``` + +## links + +- https://www.postgresql.org/docs/current/sql-truncate.html +- `CASCADE`'s recursive nature [caused Linear's 2024-01-24 incident](https://linear.app/blog/linear-incident-on-jan-24th-2024). diff --git a/docs/sidebars.js b/docs/sidebars.js index 1262c68a..04644b6d 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -30,6 +30,7 @@ module.exports = { "transaction-nesting", "ban-create-domain-with-constraint", "ban-alter-domain-with-add-constraint", + "ban-truncate-cascade", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index fbb7ac51..ba7ffffc 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -188,6 +188,11 @@ const rules = [ tags: ["schema", "locking"], description: "Domains with constraints have poor support for online migrations", }, + { + name: "ban-truncate-cascade", + tags: ["backwards compatibility"], + description: "Truncate cascade will recursively truncate all related tables!", + }, // xtask:new-rule:rule-doc-meta ]