linter: add rule ban-uncommitted-transaction (#699)

We have an existing rule to handle transaction nesting, this rule makes sure you commit your transactions!

rel: #697

Author: sbdchd

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

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<SourceFile>) {
+    let file = parse.tree();
+    let mut uncommitted_begin: Option<ast::Begin> = 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;
+");
+    }
+}

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;

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;
+```

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
     ],
   },

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
 ]