Skip to content

ignore single stmts with prefer-robust-stmts #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kodiakhq merged 2 commits into from
Jul 16, 2022
Merged

ignore single stmts with prefer-robust-stmts #231

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4c44fc
ignore single stmts with prefer-robust-stmts
chdsbd Jul 16, 2022
ac75134
Merge branch 'master' into cdignam/simpler-robust-stmts
kodiakhq[bot] Jul 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 69 additions & 19 deletions linter/src/rules/prefer_robust_stmts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ pub fn prefer_robust_stmts(tree: &[RawStmt], _pg_version: Option<Version>) -> Ve
let mut errs = vec![];
let mut inside_transaction = false;
let mut constraint_names: HashMap<String, Constraint> = HashMap::new();
// if we only have one statement in our file, Postgres will run that
// statement in an implicit transaction, so we don't need to worry about
// wrapping with `BEGIN;COMMIT;`.
if tree.len() == 1 {
return errs;
}
for raw_stmt in tree {
match &raw_stmt.stmt {
Stmt::TransactionStmt(stmt) => match stmt.kind {
Expand Down Expand Up @@ -105,8 +111,23 @@ pub fn prefer_robust_stmts(tree: &[RawStmt], _pg_version: Option<Version>) -> Ve

#[cfg(test)]
mod test_rules {
use crate::{check_sql, violations::RuleViolationKind};
use crate::{
check_sql,
errors::CheckSqlError,
violations::{RuleViolation, RuleViolationKind},
};
use insta::assert_debug_snapshot;
use squawk_parser::parse::parse_sql_query;

use super::prefer_robust_stmts;

fn lint_sql(sql: &str) -> Result<Vec<RuleViolation>, CheckSqlError> {
let mut sql = sql.to_owned();
// our check ignores single statement queries, so we add an extra statement to ensure we check that case
sql.push_str(";\nSELECT 1;");
let tree = parse_sql_query(&sql)?;
Ok(prefer_robust_stmts(&tree, None))
}

#[test]
/// If we drop the constraint before adding it, we don't need the IF EXISTS or a transaction.
Expand All @@ -115,14 +136,14 @@ mod test_rules {
ALTER TABLE "app_email" DROP CONSTRAINT IF EXISTS "email_uniq";
ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_idx";
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));
}
#[test]
fn drop_index() {
let sql = r#"
DROP INDEX CONCURRENTLY "email_idx";
"#;
let res = check_sql(sql, &[], None).unwrap();
let res = lint_sql(sql).unwrap();
assert_eq!(res.len(), 1);
assert_eq!(res[0].kind, RuleViolationKind::PreferRobustStmts);
}
Expand All @@ -131,7 +152,7 @@ DROP INDEX CONCURRENTLY "email_idx";
let sql = r#"
DROP INDEX CONCURRENTLY IF EXISTS "email_idx";
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));
}
#[test]
/// DROP CONSTRAINT and then ADD CONSTRAINT is safe. We can also safely run VALIDATE CONSTRAINT.
Expand All @@ -141,7 +162,7 @@ ALTER TABLE "app_email" DROP CONSTRAINT IF EXISTS "fk_user";
ALTER TABLE "app_email" ADD CONSTRAINT "fk_user" FOREIGN KEY ("user_id") REFERENCES "app_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "app_email" VALIDATE CONSTRAINT "fk_user";
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));
}
#[test]
/// We can only use the dropped constraint in one ADD CONSTRAINT statement.
Expand All @@ -152,7 +173,7 @@ ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_id
-- this second add constraint should error because it's not robust
ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_idx";
"#;
let res = check_sql(sql, &[], None).unwrap();
let res = lint_sql(sql).unwrap();
assert_eq!(res.len(), 1);
assert_eq!(res[0].kind, RuleViolationKind::PreferRobustStmts);
}
Expand All @@ -166,17 +187,17 @@ BEGIN;
ALTER TABLE "core_foo" ADD COLUMN "answer_id" integer NULL;
COMMIT;
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));

let sql = r#"
ALTER TABLE "core_foo" ADD COLUMN IF NOT EXISTS "answer_id" integer NULL;
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));

let sql = r#"
CREATE INDEX CONCURRENTLY IF NOT EXISTS "core_foo_idx" ON "core_foo" ("answer_id");
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));

let sql = r#"
BEGIN;
Expand All @@ -186,32 +207,32 @@ CREATE TABLE "core_bar" (
);
COMMIT;
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));

let sql = r#"
CREATE TABLE IF NOT EXISTS "core_bar" (
"id" serial NOT NULL PRIMARY KEY,
"bravo" text NOT NULL
);
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));

// select is fine, we're only interested in modifications to the tables
let sql = r#"
SELECT 1;
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));

// inserts are also okay
let sql = r#"
INSERT INTO tbl VALUES (a);
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));

let sql = r#"
ALTER TABLE "core_foo" DROP CONSTRAINT IF EXISTS "core_foo_idx";
"#;
assert_eq!(check_sql(sql, &[], None), Ok(vec![]));
assert_eq!(lint_sql(sql), Ok(vec![]));
}

#[test]
Expand All @@ -220,15 +241,44 @@ ALTER TABLE "core_foo" DROP CONSTRAINT IF EXISTS "core_foo_idx";
CREATE INDEX CONCURRENTLY ON "table_name" ("field_name");
"#;

assert_debug_snapshot!(check_sql(bad_sql, &[], None));
assert_debug_snapshot!(lint_sql(bad_sql));
}

fn violations(
res: Result<Vec<RuleViolation>, CheckSqlError>,
) -> Result<Vec<RuleViolationKind>, CheckSqlError> {
match res {
Ok(res) => Ok(res.into_iter().map(|x| x.kind).collect()),
Err(err) => Err(err),
}
}

#[test]
fn test_ignore_single_stmts() {
let bad_sql = r#"
CREATE INDEX CONCURRENTLY ON "table_name" ("field_name");
"#;
assert_eq!(check_sql(bad_sql, &[], None), Ok(vec![]));
let bad_sql = r#"
CREATE INDEX CONCURRENTLY ON "table_name" ("field_name");
CREATE INDEX CONCURRENTLY ON "table_name" ("field_name");
"#;

assert_eq!(
violations(check_sql(bad_sql, &[], None)),
Ok(vec![
RuleViolationKind::PreferRobustStmts,
RuleViolationKind::PreferRobustStmts
])
);
}

#[test]
fn test_prefer_robust_stmt_failure_cases() {
let sql = r#"
ALTER TABLE "core_foo" ADD COLUMN "answer_id" integer NULL;
"#;
assert_debug_snapshot!(check_sql(sql, &[], None), @r###"
assert_debug_snapshot!(lint_sql(sql), @r###"
Ok(
[
RuleViolation {
Expand All @@ -252,7 +302,7 @@ ALTER TABLE "core_foo" ADD COLUMN "answer_id" integer NULL;
let sql = r#"
CREATE INDEX CONCURRENTLY "core_foo_idx" ON "core_foo" ("answer_id");
"#;
assert_debug_snapshot!(check_sql(sql, &[], None), @r###"
assert_debug_snapshot!(lint_sql(sql), @r###"
Ok(
[
RuleViolation {
Expand All @@ -276,7 +326,7 @@ CREATE INDEX CONCURRENTLY "core_foo_idx" ON "core_foo" ("answer_id");
let sql = r#"
CREATE TABLE "core_bar" ( "id" serial NOT NULL PRIMARY KEY, "bravo" text NOT NULL);
"#;
assert_debug_snapshot!(check_sql(sql, &[], None), @r###"
assert_debug_snapshot!(lint_sql(sql), @r###"
Ok(
[
RuleViolation {
Expand All @@ -300,7 +350,7 @@ CREATE TABLE "core_bar" ( "id" serial NOT NULL PRIMARY KEY, "bravo" text NOT NUL
let sql = r#"
ALTER TABLE "core_foo" DROP CONSTRAINT "core_foo_idx";
"#;
assert_debug_snapshot!(check_sql(sql, &[], None), @r###"
assert_debug_snapshot!(lint_sql(sql), @r###"
Ok(
[
RuleViolation {
Expand Down