Skip to content

Commit 3f62115

Browse files
authored
add exception for prefer-robust-stmts if DROP CONSTRAINT is used (#99)
Adding the constraint without a transaction or IF EXISTS is safe since we DROP CONSTRAINT before adding the constraint. ``` ALTER TABLE "app_email" DROP CONSTRAINT IF EXISTS "email_uniq"; ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_uniq_idx"; ```
1 parent 3f1d8b2 commit 3f62115

File tree

1 file changed

+68
-2
lines changed

1 file changed

+68
-2
lines changed

linter/src/rules/prefer_robust_stmts.rs

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,14 @@
1+
use std::collections::HashMap;
2+
13
use crate::violations::{RuleViolation, RuleViolationKind};
2-
use squawk_parser::ast::{AlterTableCmds, RootStmt, Stmt, TransactionStmtKind};
4+
use squawk_parser::ast::{
5+
AlterTableCmds, AlterTableDef, AlterTableType, RootStmt, Stmt, TransactionStmtKind,
6+
};
7+
#[derive(PartialEq)]
8+
enum Constraint {
9+
Dropped,
10+
Added,
11+
}
312

413
/// If a migration is running in a transaction, then we skip the statements
514
/// because if it fails part way through, it will revert.
@@ -11,6 +20,7 @@ use squawk_parser::ast::{AlterTableCmds, RootStmt, Stmt, TransactionStmtKind};
1120
pub fn prefer_robust_stmts(tree: &[RootStmt]) -> Vec<RuleViolation> {
1221
let mut errs = vec![];
1322
let mut inside_transaction = false;
23+
let mut constraint_names: HashMap<String, Constraint> = HashMap::new();
1424
for RootStmt::RawStmt(raw_stmt) in tree {
1525
match &raw_stmt.stmt {
1626
Stmt::TransactionStmt(stmt) => match stmt.kind {
@@ -20,6 +30,28 @@ pub fn prefer_robust_stmts(tree: &[RootStmt]) -> Vec<RuleViolation> {
2030
},
2131
Stmt::AlterTableStmt(stmt) => {
2232
for AlterTableCmds::AlterTableCmd(cmd) in &stmt.cmds {
33+
if let Some(constraint_name) = &cmd.name {
34+
if cmd.subtype == AlterTableType::DropConstraint {
35+
constraint_names.insert(constraint_name.clone(), Constraint::Dropped);
36+
}
37+
if (cmd.subtype == AlterTableType::AddConstraint
38+
|| cmd.subtype == AlterTableType::ValidateConstraint)
39+
&& constraint_names.contains_key(constraint_name)
40+
{
41+
continue;
42+
}
43+
}
44+
45+
if let Some(AlterTableDef::Constraint(constraint)) = &cmd.def {
46+
if let Some(constraint_name) = &constraint.conname {
47+
if let Some(constraint) = constraint_names.get_mut(constraint_name) {
48+
if *constraint == Constraint::Dropped {
49+
*constraint = Constraint::Added;
50+
continue;
51+
}
52+
}
53+
}
54+
}
2355
if cmd.missing_ok || inside_transaction {
2456
continue;
2557
}
@@ -52,8 +84,42 @@ pub fn prefer_robust_stmts(tree: &[RootStmt]) -> Vec<RuleViolation> {
5284

5385
#[cfg(test)]
5486
mod test_rules {
55-
use crate::check_sql;
87+
use crate::{check_sql, violations::RuleViolationKind};
5688
use insta::assert_debug_snapshot;
89+
90+
#[test]
91+
/// If we drop the constraint before adding it, we don't need the IF EXISTS or a transaction.
92+
fn drop_before_add() {
93+
let sql = r#"
94+
ALTER TABLE "app_email" DROP CONSTRAINT IF EXISTS "email_uniq";
95+
ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_idx";
96+
"#;
97+
assert_eq!(check_sql(sql, &[]), Ok(vec![]));
98+
}
99+
#[test]
100+
/// DROP CONSTRAINT and then ADD CONSTRAINT is safe. We can also safely run VALIDATE CONSTRAINT.
101+
fn drop_before_add_foreign_key() {
102+
let sql = r#"
103+
ALTER TABLE "app_email" DROP CONSTRAINT IF EXISTS "fk_user";
104+
ALTER TABLE "app_email" ADD CONSTRAINT "fk_user" FOREIGN KEY ("user_id") REFERENCES "app_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
105+
ALTER TABLE "app_email" VALIDATE CONSTRAINT "fk_user";
106+
"#;
107+
assert_eq!(check_sql(sql, &[]), Ok(vec![]));
108+
}
109+
#[test]
110+
/// We can only use the dropped constraint in one ADD CONSTRAINT statement.
111+
fn double_add_after_drop() {
112+
let sql = r#"
113+
ALTER TABLE "app_email" DROP CONSTRAINT IF EXISTS "email_uniq";
114+
ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_idx";
115+
-- this second add constraint should error because it's not robust
116+
ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_idx";
117+
"#;
118+
let res = check_sql(sql, &[]).unwrap();
119+
assert_eq!(res.len(), 1);
120+
assert_eq!(res[0].kind, RuleViolationKind::PreferRobustStmts);
121+
}
122+
57123
/// If the statement is in a transaction, or it has a guard like IF NOT
58124
/// EXISTS, then it is considered valid by the `prefer-robust-stmt` rule.
59125
#[test]

0 commit comments

Comments
 (0)