diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 0e6f6472..1194e049 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -39,6 +39,7 @@ use rules::ban_drop_database; use rules::ban_drop_not_null; use rules::ban_drop_table; use rules::ban_truncate_cascade; +use rules::ban_uncommitted_transaction; use rules::changing_column_type; use rules::constraint_missing_not_valid; use rules::disallow_unique_constraint; @@ -88,6 +89,7 @@ pub enum Rule { BanAlterDomainWithAddConstraint, BanTruncateCascade, RequireTimeoutSettings, + BanUncommittedTransaction, // xtask:new-rule:error-name } @@ -129,6 +131,7 @@ impl TryFrom<&str> for Rule { "ban-alter-domain-with-add-constraint" => Ok(Rule::BanAlterDomainWithAddConstraint), "ban-truncate-cascade" => Ok(Rule::BanTruncateCascade), "require-timeout-settings" => Ok(Rule::RequireTimeoutSettings), + "ban-uncommitted-transaction" => Ok(Rule::BanUncommittedTransaction), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -190,6 +193,7 @@ impl fmt::Display for Rule { Rule::BanAlterDomainWithAddConstraint => "ban-alter-domain-with-add-constraint", Rule::BanTruncateCascade => "ban-truncate-cascade", Rule::RequireTimeoutSettings => "require-timeout-settings", + Rule::BanUncommittedTransaction => "ban-uncommitted-transaction", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -407,6 +411,9 @@ impl Linter { if self.rules.contains(&Rule::RequireTimeoutSettings) { require_timeout_settings(self, file); } + if self.rules.contains(&Rule::BanUncommittedTransaction) { + ban_uncommitted_transaction(self, file); + } // xtask:new-rule:rule-call // locate any ignores in the file diff --git a/crates/squawk_linter/src/rules/ban_uncommitted_transaction.rs b/crates/squawk_linter/src/rules/ban_uncommitted_transaction.rs new file mode 100644 index 00000000..a1007d56 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_uncommitted_transaction.rs @@ -0,0 +1,274 @@ +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +use crate::{Edit, Fix, Linter, Rule, Violation}; + +pub(crate) fn ban_uncommitted_transaction(ctx: &mut Linter, parse: &Parse) { + let file = parse.tree(); + let mut uncommitted_begin: Option = None; + + for stmt in file.stmts() { + match stmt { + ast::Stmt::Begin(begin) => { + uncommitted_begin = Some(begin); + } + ast::Stmt::Commit(_) | ast::Stmt::Rollback(_) => { + uncommitted_begin = None; + } + _ => (), + } + } + + if let Some(begin) = uncommitted_begin { + let end_pos = file.syntax().text_range().end(); + let fix = Fix::new("Add COMMIT", vec![Edit::insert("\nCOMMIT;\n", end_pos)]); + + ctx.report( + Violation::for_node( + Rule::BanUncommittedTransaction, + "Transaction never committed or rolled back.".to_string(), + begin.syntax(), + ) + .help("Add a `COMMIT` or `ROLLBACK` statement to complete the transaction.") + .fix(Some(fix)), + ); + } +} + +#[cfg(test)] +mod test { + use insta::{assert_debug_snapshot, assert_snapshot}; + + use crate::{Linter, Rule, test_utils::fix_sql}; + use squawk_syntax::SourceFile; + + #[test] + fn uncommitted_transaction_err() { + let sql = r#" +BEGIN; +CREATE TABLE users (id bigint); + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors, @r#" + [ + Violation { + code: BanUncommittedTransaction, + message: "Transaction never committed or rolled back.", + text_range: 1..6, + help: Some( + "Add a `COMMIT` or `ROLLBACK` statement to complete the transaction.", + ), + fix: Some( + Fix { + title: "Add COMMIT", + edits: [ + Edit { + text_range: 48..48, + text: Some( + "\nCOMMIT;\n", + ), + }, + ], + }, + ), + }, + ] + "#); + } + + #[test] + fn committed_transaction_ok() { + let sql = r#" +BEGIN; +CREATE TABLE users (id bigint); +COMMIT; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_eq!(errors.len(), 0); + } + + #[test] + fn rolled_back_transaction_ok() { + let sql = r#" +BEGIN; +CREATE TABLE users (id bigint); +ROLLBACK; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_eq!(errors.len(), 0); + } + + #[test] + fn no_transaction_ok() { + let sql = r#" +CREATE TABLE users (id bigint); + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_eq!(errors.len(), 0); + } + + #[test] + fn multiple_transactions_last_uncommitted_err() { + let sql = r#" +BEGIN; +CREATE TABLE users (id bigint); +COMMIT; + +BEGIN; +CREATE TABLE posts (id bigint); + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors, @r#" + [ + Violation { + code: BanUncommittedTransaction, + message: "Transaction never committed or rolled back.", + text_range: 49..54, + help: Some( + "Add a `COMMIT` or `ROLLBACK` statement to complete the transaction.", + ), + fix: Some( + Fix { + title: "Add COMMIT", + edits: [ + Edit { + text_range: 96..96, + text: Some( + "\nCOMMIT;\n", + ), + }, + ], + }, + ), + }, + ] + "#); + } + + #[test] + fn start_transaction_uncommitted_err() { + let sql = r#" +START TRANSACTION; +CREATE TABLE users (id bigint); + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors, @r#" + [ + Violation { + code: BanUncommittedTransaction, + message: "Transaction never committed or rolled back.", + text_range: 1..18, + help: Some( + "Add a `COMMIT` or `ROLLBACK` statement to complete the transaction.", + ), + fix: Some( + Fix { + title: "Add COMMIT", + edits: [ + Edit { + text_range: 60..60, + text: Some( + "\nCOMMIT;\n", + ), + }, + ], + }, + ), + }, + ] + "#); + } + + #[test] + fn nested_begin_only_last_uncommitted_err() { + let sql = r#" +BEGIN; +BEGIN; +COMMIT; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_eq!(errors.len(), 0); + } + + #[test] + fn begin_work_uncommitted_err() { + let sql = r#" +BEGIN WORK; +CREATE TABLE users (id bigint); + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::BanUncommittedTransaction]); + let errors = linter.lint(&file, sql); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors, @r#" + [ + Violation { + code: BanUncommittedTransaction, + message: "Transaction never committed or rolled back.", + text_range: 1..11, + help: Some( + "Add a `COMMIT` or `ROLLBACK` statement to complete the transaction.", + ), + fix: Some( + Fix { + title: "Add COMMIT", + edits: [ + Edit { + text_range: 53..53, + text: Some( + "\nCOMMIT;\n", + ), + }, + ], + }, + ), + }, + ] + "#); + } + + #[test] + fn fix_adds_commit() { + let sql = r#" +BEGIN; +CREATE TABLE users (id bigint); + "#; + let fixed = fix_sql(sql, Rule::BanUncommittedTransaction); + assert_snapshot!(fixed, @r" + BEGIN; + CREATE TABLE users (id bigint); + + COMMIT; + "); + } + + #[test] + fn fix_adds_commit_to_start_transaction() { + let sql = r#"START TRANSACTION; +CREATE TABLE posts (id bigint);"#; + let fixed = fix_sql(sql, Rule::BanUncommittedTransaction); + assert_snapshot!(fixed, @r"START TRANSACTION; +CREATE TABLE posts (id bigint); +COMMIT; +"); + } +} diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index 7df1f834..1bb4e019 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -12,6 +12,7 @@ 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 ban_uncommitted_transaction; pub(crate) mod changing_column_type; pub(crate) mod constraint_missing_not_valid; pub(crate) mod disallow_unique_constraint; @@ -43,6 +44,7 @@ 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 ban_uncommitted_transaction::ban_uncommitted_transaction; 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/docs/docs/ban-uncommitted-transaction.md b/docs/docs/ban-uncommitted-transaction.md new file mode 100644 index 00000000..8e006752 --- /dev/null +++ b/docs/docs/ban-uncommitted-transaction.md @@ -0,0 +1,24 @@ +--- +id: ban-uncommitted-transaction +title: ban-uncommitted-transaction +--- + +## problem + +If you start a transaction and forget to add a `commit` statement, then the migration will run successfully, but it [won't have actually applied the changes](https://github.com/sbdchd/squawk/issues/697#issue-3570671498). + +```sql +begin; +create table users (id bigint); +-- missing `commit`! +``` + +## solution + +Add a `commit` statement to complete your transaction! + +```sql +begin; +create table users (id bigint); +commit; +``` diff --git a/docs/sidebars.js b/docs/sidebars.js index 7924200d..873997b8 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -33,6 +33,7 @@ module.exports = { "ban-truncate-cascade", "syntax-error", "require-timeout-settings", + "ban-uncommitted-transaction", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index ff524933..251ebec1 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -198,6 +198,11 @@ const rules = [ tags: ["locking"], description: "Require lock and statement timeouts", }, + { + name: "ban-uncommitted-transaction", + tags: ["schema"], + description: "Ensure all transactions are committed", + }, // xtask:new-rule:rule-doc-meta ]