From 8586d2e5d82b50ed436e9c385377351e8d90a6a0 Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Wed, 1 Mar 2023 17:32:54 -0800 Subject: [PATCH 1/3] Switch to snapshot tests for adding_foreign_key_constraint These are used heavily elsewhere and are a little better at verifying the rule output than the hand-written assertions were. --- .../rules/adding_foreign_key_constraint.rs | 24 +++++++------------ ...est_rules__add_column_references_lock.snap | 23 ++++++++++++++++++ ...ules__add_foreign_key_constraint_lock.snap | 23 ++++++++++++++++++ ...ign_key_constraint_not_valid_validate.snap | 5 ++++ ...ate_table_with_foreign_key_constraint.snap | 5 ++++ 5 files changed, 64 insertions(+), 16 deletions(-) create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_column_references_lock.snap create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_lock.snap create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_not_valid_validate.snap create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_with_foreign_key_constraint.snap diff --git a/linter/src/rules/adding_foreign_key_constraint.rs b/linter/src/rules/adding_foreign_key_constraint.rs index 6fb9d16f..495c454e 100644 --- a/linter/src/rules/adding_foreign_key_constraint.rs +++ b/linter/src/rules/adding_foreign_key_constraint.rs @@ -73,6 +73,7 @@ mod test_rules { check_sql_with_rule, violations::{RuleViolation, RuleViolationKind}, }; + use insta::assert_debug_snapshot; fn lint_sql(sql: &str) -> Vec { check_sql_with_rule( @@ -100,9 +101,9 @@ CREATE TABLE email ( COMMIT; "#; - let violations = lint_sql(sql); - assert_eq!(violations.len(), 0); + assert_debug_snapshot!(lint_sql(sql)); } + #[test] fn test_add_foreign_key_constraint_not_valid_validate() { let sql = r#" @@ -113,9 +114,9 @@ ALTER TABLE "email" VALIDATE CONSTRAINT "fk_user"; COMMIT; "#; - let violations = lint_sql(sql); - assert_eq!(violations.len(), 0); + assert_debug_snapshot!(lint_sql(sql)); } + #[test] fn test_add_foreign_key_constraint_lock() { let sql = r#" @@ -125,13 +126,9 @@ ALTER TABLE "email" ADD CONSTRAINT "fk_user" FOREIGN KEY ("user_id") REFERENCES COMMIT; "#; - let violations = lint_sql(sql); - assert_eq!(violations.len(), 1); - assert_eq!( - violations[0].kind, - RuleViolationKind::AddingForeignKeyConstraint - ); + assert_debug_snapshot!(lint_sql(sql)); } + #[test] fn test_add_column_references_lock() { let sql = r#" @@ -140,11 +137,6 @@ ALTER TABLE "emails" ADD COLUMN "user_id" INT REFERENCES "user" ("id"); COMMIT; "#; - let violations = lint_sql(sql); - assert_eq!(violations.len(), 1); - assert_eq!( - violations[0].kind, - RuleViolationKind::AddingForeignKeyConstraint - ); + assert_debug_snapshot!(lint_sql(sql)); } } diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_column_references_lock.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_column_references_lock.snap new file mode 100644 index 00000000..4b7443d5 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_column_references_lock.snap @@ -0,0 +1,23 @@ +--- +source: linter/src/rules/adding_foreign_key_constraint.rs +expression: lint_sql(sql) +--- +[ + RuleViolation { + kind: AddingForeignKeyConstraint, + span: Span { + start: 7, + len: Some( + 71, + ), + }, + messages: [ + Note( + "Requires a table scan of the table you're altering and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to both tables while your table is scanned.", + ), + Help( + "Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.", + ), + ], + }, +] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_lock.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_lock.snap new file mode 100644 index 00000000..f313606e --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_lock.snap @@ -0,0 +1,23 @@ +--- +source: linter/src/rules/adding_foreign_key_constraint.rs +expression: lint_sql(sql) +--- +[ + RuleViolation { + kind: AddingForeignKeyConstraint, + span: Span { + start: 53, + len: Some( + 94, + ), + }, + messages: [ + Note( + "Requires a table scan of the table you're altering and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to both tables while your table is scanned.", + ), + Help( + "Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.", + ), + ], + }, +] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_not_valid_validate.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_not_valid_validate.snap new file mode 100644 index 00000000..d71ec493 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__add_foreign_key_constraint_not_valid_validate.snap @@ -0,0 +1,5 @@ +--- +source: linter/src/rules/adding_foreign_key_constraint.rs +expression: lint_sql(sql) +--- +[] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_with_foreign_key_constraint.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_with_foreign_key_constraint.snap new file mode 100644 index 00000000..d71ec493 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_with_foreign_key_constraint.snap @@ -0,0 +1,5 @@ +--- +source: linter/src/rules/adding_foreign_key_constraint.rs +expression: lint_sql(sql) +--- +[] From a807c1c195a0ff940e35054a6b6ac961cae0fc47 Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Thu, 2 Mar 2023 12:58:37 -0800 Subject: [PATCH 2/3] Add failing tests to verify the new behavior These cover a few cases that should not cause this rule to warn because the table is being created in the same transaction as the foreign key constraint. --- .../rules/adding_foreign_key_constraint.rs | 36 +++++++++++++++++++ ..._table_and_add_foreign_key_constraint.snap | 5 +++ ..._and_add_foreign_key_constraint_after.snap | 5 +++ ...aint_after_with_assume_in_transaction.snap | 5 +++ 4 files changed, 51 insertions(+) create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint.snap create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after.snap create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after_with_assume_in_transaction.snap diff --git a/linter/src/rules/adding_foreign_key_constraint.rs b/linter/src/rules/adding_foreign_key_constraint.rs index 495c454e..23bc884c 100644 --- a/linter/src/rules/adding_foreign_key_constraint.rs +++ b/linter/src/rules/adding_foreign_key_constraint.rs @@ -85,6 +85,16 @@ mod test_rules { .unwrap() } + fn lint_sql_assuming_in_transaction(sql: &str) -> Vec { + check_sql_with_rule( + sql, + &RuleViolationKind::AddingForeignKeyConstraint, + None, + true, + ) + .unwrap() + } + #[test] fn test_create_table_with_foreign_key_constraint() { let sql = r#" @@ -104,6 +114,32 @@ COMMIT; assert_debug_snapshot!(lint_sql(sql)); } + #[test] + fn test_create_table_and_add_foreign_key_constraint_after() { + let sql = r#" +BEGIN; +CREATE TABLE "email" ( + "user_id" INT +); +ALTER TABLE "email" ADD CONSTRAINT "fk_user" FOREIGN KEY ("user_id") REFERENCES "user" ("id"); +COMMIT; + "#; + + assert_debug_snapshot!(lint_sql(sql)); + } + + #[test] + fn test_create_table_and_add_foreign_key_constraint_after_with_assume_in_transaction() { + let sql = r#" +CREATE TABLE "email" ( + "user_id" INT +); +ALTER TABLE "email" ADD CONSTRAINT "fk_user" FOREIGN KEY ("user_id") REFERENCES "user" ("id"); + "#; + + assert_debug_snapshot!(lint_sql_assuming_in_transaction(sql)); + } + #[test] fn test_add_foreign_key_constraint_not_valid_validate() { let sql = r#" diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint.snap new file mode 100644 index 00000000..d71ec493 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint.snap @@ -0,0 +1,5 @@ +--- +source: linter/src/rules/adding_foreign_key_constraint.rs +expression: lint_sql(sql) +--- +[] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after.snap new file mode 100644 index 00000000..d71ec493 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after.snap @@ -0,0 +1,5 @@ +--- +source: linter/src/rules/adding_foreign_key_constraint.rs +expression: lint_sql(sql) +--- +[] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after_with_assume_in_transaction.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after_with_assume_in_transaction.snap new file mode 100644 index 00000000..26e37e34 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test_rules__create_table_and_add_foreign_key_constraint_after_with_assume_in_transaction.snap @@ -0,0 +1,5 @@ +--- +source: linter/src/rules/adding_foreign_key_constraint.rs +expression: lint_sql_assuming_in_transaction(sql) +--- +[] From ba7d66ec7ee1131f3dc1d8a466472648c1629d69 Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Thu, 2 Mar 2023 13:07:55 -0800 Subject: [PATCH 3/3] Change adding_foreign_key_constraint to ignore newly created tables This addresses #220, which claims that although an exclusive lock will be obtained on the referenced table, there are no rows in the referencing table so the table scan under the exclusive lock should be short or non-existent. --- linter/src/rules/adding_foreign_key_constraint.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/linter/src/rules/adding_foreign_key_constraint.rs b/linter/src/rules/adding_foreign_key_constraint.rs index 23bc884c..4993b1ca 100644 --- a/linter/src/rules/adding_foreign_key_constraint.rs +++ b/linter/src/rules/adding_foreign_key_constraint.rs @@ -1,4 +1,5 @@ use crate::{ + rules::utils::tables_created_in_transaction, versions::Version, violations::{RuleViolation, RuleViolationKind}, }; @@ -17,12 +18,16 @@ use squawk_parser::ast::{ pub fn adding_foreign_key_constraint( tree: &[RawStmt], _pg_version: Option, - _assume_in_transaction: bool, + assume_in_transaction: bool, ) -> Vec { let mut errs = vec![]; + let tables_created = tables_created_in_transaction(tree, assume_in_transaction); for raw_stmt in tree { match &raw_stmt.stmt { Stmt::AlterTableStmt(stmt) => { + if tables_created.contains(&stmt.relation.relname) { + continue; + } for cmd in &stmt.cmds { match cmd { AlterTableCmds::AlterTableCmd(ref command) => {