Skip to content

Commit

Permalink
Ignore unique constraint on drop constraint (#161)
Browse files Browse the repository at this point in the history 
Ignore rule failures on `DROP CONSTRAINT...` for _disallowed-unique-constraint_ and _adding-serial-primary-key-field_. 

When running `squawk` against the recommended usage,

```
CREATE UNIQUE INDEX CONCURRENTLY dist_id_temp_idx ON distributors (dist_id);
ALTER TABLE distributors DROP CONSTRAINT distributors_pkey,
    ADD CONSTRAINT distributors_pkey PRIMARY KEY USING INDEX dist_id_temp_idx;
```

we still get a warning.
  • Loading branch information
@qoelet
qoelet authored Oct 29, 2021
1 parent dda7ae7 commit 24a9337
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 61 deletions.
2 changes: 1 addition & 1 deletion linter/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ lazy_static! {
func: prefer_robust_stmts,
messages: vec![
ViolationMessage::Help(
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statment supports it.".into()
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.".into()
),
]
},
Expand Down
33 changes: 20 additions & 13 deletions linter/src/rules/adding_primary_key_constraint.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::violations::{RuleViolation, RuleViolationKind};
use squawk_parser::ast::{
AlterTableCmds, AlterTableDef, ColumnDefConstraint, ConstrType, RootStmt, Stmt,
AlterTableCmds, AlterTableDef, AlterTableType, ColumnDefConstraint, ConstrType, RootStmt, Stmt,
};

#[must_use]
Expand All @@ -10,10 +10,26 @@ pub fn adding_primary_key_constraint(tree: &[RootStmt]) -> Vec<RuleViolation> {
match &raw_stmt.stmt {
Stmt::AlterTableStmt(stmt) => {
for AlterTableCmds::AlterTableCmd(cmd) in &stmt.cmds {
match &cmd.def {
Some(AlterTableDef::ColumnDef(def)) => {
match (&cmd.def, &cmd.subtype) {
(
Some(AlterTableDef::Constraint(constraint)),
AlterTableType::AddConstraint,
) => {
if constraint.contype == ConstrType::Primary
&& constraint.indexname.is_none()
{
errs.push(RuleViolation::new(
RuleViolationKind::AddingSerialPrimaryKeyField,
raw_stmt.into(),
None,
));
}
}
(Some(AlterTableDef::ColumnDef(def)), _) => {
for ColumnDefConstraint::Constraint(constraint) in &def.constraints {
if constraint.contype == ConstrType::Primary {
if constraint.contype == ConstrType::Primary
&& constraint.indexname.is_none()
{
errs.push(RuleViolation::new(
RuleViolationKind::AddingSerialPrimaryKeyField,
raw_stmt.into(),
Expand All @@ -22,15 +38,6 @@ pub fn adding_primary_key_constraint(tree: &[RootStmt]) -> Vec<RuleViolation> {
}
}
}
Some(AlterTableDef::Constraint(constraint)) => {
if constraint.contype == ConstrType::Primary {
errs.push(RuleViolation::new(
RuleViolationKind::AddingSerialPrimaryKeyField,
raw_stmt.into(),
None,
));
}
}
_ => continue,
}
}
Expand Down
20 changes: 15 additions & 5 deletions linter/src/rules/disallow_unique_constraint.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::rules::utils::tables_created_in_transaction;
use crate::violations::{RuleViolation, RuleViolationKind};
use squawk_parser::ast::{AlterTableCmds, AlterTableDef, ConstrType, RelationKind, RootStmt, Stmt};
use squawk_parser::ast::{
AlterTableCmds, AlterTableDef, AlterTableType, ConstrType, RelationKind, RootStmt, Stmt,
};

#[must_use]
pub fn disallow_unique_constraint(tree: &[RootStmt]) -> Vec<RuleViolation> {
Expand All @@ -12,8 +14,11 @@ pub fn disallow_unique_constraint(tree: &[RootStmt]) -> Vec<RuleViolation> {
let RelationKind::RangeVar(range) = &stmt.relation;
let tbl_name = &range.relname;
for AlterTableCmds::AlterTableCmd(cmd) in &stmt.cmds {
match &cmd.def {
Some(AlterTableDef::Constraint(constraint)) => {
match (&cmd.def, &cmd.subtype) {
(
Some(AlterTableDef::Constraint(constraint)),
AlterTableType::AddConstraint,
) => {
if !tables_created.contains(tbl_name)
&& constraint.contype == ConstrType::Unique
&& constraint.indexname.is_none()
Expand Down Expand Up @@ -54,6 +59,10 @@ mod test_rules {
fn test_adding_unique_constraint() {
let bad_sql = r#"
ALTER TABLE table_name ADD CONSTRAINT field_name_constraint UNIQUE (field_name);
"#;

let ignored_sql = r#"
ALTER TABLE table_name DROP CONSTRAINT field_name_constraint;
"#;

let ok_sql = r#"
Expand All @@ -63,15 +72,16 @@ ADD CONSTRAINT distributors_pkey PRIMARY KEY USING INDEX dist_id_temp_idx;
"#;

assert_debug_snapshot!(check_sql(bad_sql, &["prefer-robust-stmts".into()]));
assert_debug_snapshot!(check_sql(ignored_sql, &["prefer-robust-stmts".into()]));
assert_debug_snapshot!(check_sql(ok_sql, &["prefer-robust-stmts".into()]));
}

/// Creating a UNQIUE constraint from an existing index should be considered
/// Creating a UNIQUE constraint from an existing index should be considered
/// safe
#[test]
fn test_unique_constraint_ok() {
let sql = r#"
CREATE UNIQUE INDEX CONCURRENTLY "legacy_questiongrouppg_mongo_id_1f8f47d9_uniq_idx"
CREATE UNIQUE INDEX CONCURRENTLY "legacy_questiongrouppg_mongo_id_1f8f47d9_uniq_idx"
ON "legacy_questiongrouppg" ("mongo_id");
ALTER TABLE "legacy_questiongrouppg" ADD CONSTRAINT "legacy_questiongrouppg_mongo_id_1f8f47d9_uniq" UNIQUE USING INDEX "legacy_questiongrouppg_mongo_id_1f8f47d9_uniq_idx";
"#;
Expand Down
8 changes: 4 additions & 4 deletions linter/src/rules/prefer_robust_stmts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ ALTER TABLE "core_foo" ADD COLUMN "answer_id" integer NULL;
},
messages: [
Help(
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statment supports it.",
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.",
),
],
},
Expand All @@ -240,7 +240,7 @@ CREATE INDEX CONCURRENTLY "core_foo_idx" ON "core_foo" ("answer_id");
},
messages: [
Help(
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statment supports it.",
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.",
),
],
},
Expand All @@ -264,7 +264,7 @@ CREATE TABLE "core_bar" ( "id" serial NOT NULL PRIMARY KEY, "bravo" text NOT NUL
},
messages: [
Help(
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statment supports it.",
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.",
),
],
},
Expand All @@ -288,7 +288,7 @@ ALTER TABLE "core_foo" DROP CONSTRAINT "core_foo_idx";
},
messages: [
Help(
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statment supports it.",
"Consider wrapping in a transaction or adding a IF NOT EXISTS clause if the statement supports it.",
),
],
},
Expand Down
20 changes: 1 addition & 19 deletions ...squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key-2.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,5 @@ source: linter/src/rules/adding_primary_key_constraint.rs
expression: "check_sql(ok_sql, &[\"prefer-robust-stmts\".into()])"
---
Ok(
[
RuleViolation {
kind: AddingSerialPrimaryKeyField,
span: Span {
start: 0,
len: Some(
75,
),
},
messages: [
Note(
"Adding a PRIMARY KEY constraint results in locks and table rewrites",
),
Help(
"Add the PRIMARY KEY constraint USING an index.",
),
],
},
],
[],
)
20 changes: 1 addition & 19 deletions ...wk_linter__rules__disallow_unique_constraint__test_rules__adding_unique_constraint-2.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,5 @@ source: linter/src/rules/disallow_unique_constraint.rs
expression: "check_sql(ok_sql, &[\"prefer-robust-stmts\".into()])"
---
Ok(
[
RuleViolation {
kind: AddingSerialPrimaryKeyField,
span: Span {
start: 77,
len: Some(
134,
),
},
messages: [
Note(
"Adding a PRIMARY KEY constraint results in locks and table rewrites",
),
Help(
"Add the PRIMARY KEY constraint USING an index.",
),
],
},
],
[],
)
7 changes: 7 additions & 0 deletions ...wk_linter__rules__disallow_unique_constraint__test_rules__adding_unique_constraint-3.snap
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
source: linter/src/rules/disallow_unique_constraint.rs
expression: "check_sql(ignored_sql, &[\"prefer-robust-stmts\".into()])"
---
Ok(
[],
)

0 comments on commit 24a9337

Please sign in to comment.