From c5152c646de8134c6ca1cf45aef9ca8716a36542 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Thu, 30 Oct 2025 20:59:26 -0400 Subject: [PATCH 1/3] linter: add rule ban-uncommitted-transaction We have an existing rule to handle transaction nesting, this rule makes sure you commit your transactions! rel: https://github.com/sbdchd/squawk/issues/697 --- crates/squawk_linter/src/lib.rs | 7 + .../src/rules/ban_uncommitted_transaction.rs | 274 ++++++++++++++++++ crates/squawk_linter/src/rules/mod.rs | 2 + docs/docs/ban-uncommitted-transaction.md | 24 ++ docs/sidebars.js | 1 + docs/src/pages/index.js | 5 + 6 files changed, 313 insertions(+) create mode 100644 crates/squawk_linter/src/rules/ban_uncommitted_transaction.rs create mode 100644 docs/docs/ban-uncommitted-transaction.md diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 0e6f6472..b2273434 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -54,6 +54,7 @@ use rules::require_concurrent_index_creation; use rules::require_concurrent_index_deletion; use rules::require_timeout_settings; use rules::transaction_nesting; +use rules::ban_uncommitted_transaction; // xtask:new-rule:rule-import #[derive(Debug, PartialEq, Clone, Copy, Hash, Eq, Sequence)] @@ -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..ba796794 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -27,6 +27,7 @@ pub(crate) mod require_concurrent_index_creation; pub(crate) mod require_concurrent_index_deletion; pub(crate) mod require_timeout_settings; pub(crate) mod transaction_nesting; +pub(crate) mod ban_uncommitted_transaction; // xtask:new-rule:mod-decl pub(crate) use adding_field_with_default::adding_field_with_default; @@ -58,4 +59,5 @@ pub(crate) use require_concurrent_index_creation::require_concurrent_index_creat pub(crate) use require_concurrent_index_deletion::require_concurrent_index_deletion; pub(crate) use require_timeout_settings::require_timeout_settings; pub(crate) use transaction_nesting::transaction_nesting; +pub(crate) use ban_uncommitted_transaction::ban_uncommitted_transaction; // xtask:new-rule:export 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..52bac18b 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-transactions", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index ff524933..fb3766ce 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-transactions", + tags: ["schema"], + description: "Ensure all transactions are committed", + }, // xtask:new-rule:rule-doc-meta ] From 3e0d6b9761f64c7f9ee0748c550c12297426af15 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Thu, 30 Oct 2025 21:04:09 -0400 Subject: [PATCH 2/3] fix rename --- docs/sidebars.js | 2 +- docs/src/pages/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/sidebars.js b/docs/sidebars.js index 52bac18b..873997b8 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -33,7 +33,7 @@ module.exports = { "ban-truncate-cascade", "syntax-error", "require-timeout-settings", - "ban-uncommitted-transactions", + "ban-uncommitted-transaction", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index fb3766ce..251ebec1 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -199,7 +199,7 @@ const rules = [ description: "Require lock and statement timeouts", }, { - name: "ban-uncommitted-transactions", + name: "ban-uncommitted-transaction", tags: ["schema"], description: "Ensure all transactions are committed", }, From 9f35b89f528f5b2976b1acf5fd4ada2506f836ff Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Thu, 30 Oct 2025 21:04:43 -0400 Subject: [PATCH 3/3] fix --- crates/squawk_linter/src/lib.rs | 4 ++-- crates/squawk_linter/src/rules/mod.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index b2273434..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; @@ -54,7 +55,6 @@ use rules::require_concurrent_index_creation; use rules::require_concurrent_index_deletion; use rules::require_timeout_settings; use rules::transaction_nesting; -use rules::ban_uncommitted_transaction; // xtask:new-rule:rule-import #[derive(Debug, PartialEq, Clone, Copy, Hash, Eq, Sequence)] @@ -412,7 +412,7 @@ impl Linter { require_timeout_settings(self, file); } if self.rules.contains(&Rule::BanUncommittedTransaction) { - ban_uncommitted_transaction(self, &file); + ban_uncommitted_transaction(self, file); } // xtask:new-rule:rule-call diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index ba796794..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; @@ -27,7 +28,6 @@ pub(crate) mod require_concurrent_index_creation; pub(crate) mod require_concurrent_index_deletion; pub(crate) mod require_timeout_settings; pub(crate) mod transaction_nesting; -pub(crate) mod ban_uncommitted_transaction; // xtask:new-rule:mod-decl pub(crate) use adding_field_with_default::adding_field_with_default; @@ -44,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; @@ -59,5 +60,4 @@ pub(crate) use require_concurrent_index_creation::require_concurrent_index_creat pub(crate) use require_concurrent_index_deletion::require_concurrent_index_deletion; pub(crate) use require_timeout_settings::require_timeout_settings; pub(crate) use transaction_nesting::transaction_nesting; -pub(crate) use ban_uncommitted_transaction::ban_uncommitted_transaction; // xtask:new-rule:export