fix warnings for generated columns (#397)

fixes #396

reverts 0917fe4, which broke the build

Author: chdsbd

CHANGELOG.md

@@ -7,7 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-## v1.4.1
+## v1.5.0 - 2025-01-09
+
+### Added
+
+- Added warning to `adding-field-with-default` to warn about generated columns (#397).
+
+### Fixed
+
+- Fixed bug in `adding-required-field` where generated columns were incorrectly flagged (#397).
+
+## v1.4.1 - 2024-10-10
 
 ### Fixed
 

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "squawk"
-version = "1.4.0"
+version = "1.5.0"
 authors = ["Steve Dignam <[email protected]>"]
 edition = "2018"
 license = "GPL-3.0"

cli/Cargo.toml

@@ -481,10 +481,10 @@ fn get_sql_file_content(violation: &ViolationContent) -> Result<String, std::io:
 
     let violation_content = if violation_count > 0 {
         format!(
-            r#"
+            r"
 ```
 {}
-```"#,
+```",
             violations_text.trim_matches('\n')
         )
     } else {
@@ -494,7 +494,7 @@ fn get_sql_file_content(violation: &ViolationContent) -> Result<String, std::io:
     let violations_emoji = get_violations_emoji(violation_count);
 
     Ok(format!(
-        r#"
+        r"
 <h3><code>{filename}</code></h3>
 
 ```sql
@@ -506,7 +506,7 @@ fn get_sql_file_content(violation: &ViolationContent) -> Result<String, std::io:
 {violation_content}
     
 ---
-    "#,
+    ",
         violations_emoji = violations_emoji,
         filename = violation.filename,
         sql = sql,
@@ -521,7 +521,7 @@ pub fn get_comment_body(files: &[ViolationContent], version: &str) -> String {
     let violations_emoji = get_violations_emoji(violations_count);
 
     format!(
-        r#"
+        r"
 # Squawk Report
 
 ### **{violations_emoji} {violation_count}** violations across **{file_count}** file(s)
@@ -532,7 +532,7 @@ pub fn get_comment_body(files: &[ViolationContent], version: &str) -> String {
 [📚 More info on rules](https://github.com/sbdchd/squawk#rules)
 
 ⚡️ Powered by [`Squawk`](https://github.com/sbdchd/squawk) ({version}), a linter for PostgreSQL, focused on migrations
-"#,
+",
         violations_emoji = violations_emoji,
         violation_count = violations_count,
         file_count = files.len(),

cli/src/reporter.rs

@@ -45,6 +45,40 @@ We add our column as nullable, set a default for new rows, backfill our column (
 See ["How not valid constraints work"](constraint-missing-not-valid.md#how-not-valid-validate-works) for more information on adding constraints as `NOT VALID`.
 
 
+### Adding a generated column
+
+Adding a generated column in Postgres will cause a table rewrite, locking the table while the update completes.
+
+Instead of using a generated column, use a trigger.
+
+
+Instead of:
+
+```sql
+ALTER TABLE "core_recipe" ADD COLUMN "foo" integer GENERATED ALWAAYS as ("bar" + "baz") STORED;
+```
+
+Use:
+
+```sql
+ALTER TABLE "core_recipe" ADD COLUMN "foo" integer;
+
+-- backfill column in batches
+
+-- add a trigger to match the generated column behavior
+CREATE FUNCTION foo_update_trigger()
+RETURNS trigger AS '
+BEGIN
+NEW.foo := NEW.bar + NEW.baz;
+RETURN NEW;
+END' LANGUAGE 'plpgsql';
+
+CREATE TRIGGER update_foo_column
+BEFORE INSERT ON core_recipe
+FOR EACH ROW
+EXECUTE PROCEDURE foo_update_trigger();
+```
+
 ## solution for alembic and sqlalchemy
 
 Instead of:

docs/docs/adding-field-with-default.md

@@ -18,7 +18,7 @@
           {
             squawk = final.rustPlatform.buildRustPackage {
               pname = "squawk";
-              version = "1.4.0";
+              version = "1.5.0";
 
               cargoLock = {
                 lockFile = ./Cargo.lock;

flake.nix

@@ -2,7 +2,7 @@ use std::collections::HashSet;
 
 use crate::{
     versions::Version,
-    violations::{RuleViolation, RuleViolationKind},
+    violations::{RuleViolation, RuleViolationKind, ViolationMessage},
 };
 
 use serde_json::{json, Value};
@@ -73,6 +73,20 @@ pub fn adding_field_with_default(
                                         None,
                                     ));
                                 }
+                                if constraint.contype == ConstrType::Generated {
+                                    errs.push(RuleViolation::new(
+                                        RuleViolationKind::AddingFieldWithDefault,
+                                        raw_stmt.into(),
+                                        Some(vec![
+                                            ViolationMessage::Note(
+                                            "Adding a generated column requires a table rewrite with an ACCESS EXCLUSIVE lock.".into(),
+                                        ),
+                                        ViolationMessage::Help(
+                                            "Add the column as nullable, backfill existing rows, and add a trigger to update the column on write instead.".into(),
+                                        ),
+                                        ]),
+                                    ));
+                                }
                             }
                         }
                         _ => continue,
@@ -240,4 +254,14 @@ alter table account_metadata add column blah integer default 2 + 2;
         let pg_version_11 = Some(Version::from_str("11.0.0").unwrap());
         assert_debug_snapshot!(lint_sql(ok_sql, pg_version_11));
     }
+
+    #[test]
+    fn test_generated_stored() {
+        let bad_sql = r"
+        ALTER TABLE foo
+    ADD COLUMN bar numeric GENERATED ALWAYS AS (bar + baz) STORED;
+        ";
+        let pg_version_11 = Some(Version::from_str("11.0.0").unwrap());
+        assert_debug_snapshot!(lint_sql(bad_sql, pg_version_11));
+    }
 }

linter/src/rules/adding_field_with_default.rs

@@ -4,6 +4,14 @@ use crate::violations::{RuleViolation, RuleViolationKind};
 use squawk_parser::ast::{
     AlterTableCmds, AlterTableDef, AlterTableType, ColumnDefConstraint, ConstrType, RawStmt, Stmt,
 };
+fn has_generated_constraint(constraints: &[ColumnDefConstraint]) -> bool {
+    for ColumnDefConstraint::Constraint(constraint) in constraints {
+        if constraint.contype == ConstrType::Generated {
+            return true;
+        }
+    }
+    false
+}
 
 fn has_not_null_and_no_default_constraint(constraints: &[ColumnDefConstraint]) -> bool {
     let mut has_not_null = false;
@@ -33,6 +41,9 @@ pub fn adding_required_field(
                 for AlterTableCmds::AlterTableCmd(cmd) in &stmt.cmds {
                     if cmd.subtype == AlterTableType::AddColumn {
                         if let Some(AlterTableDef::ColumnDef(column_def)) = &cmd.def {
+                            if has_generated_constraint(&column_def.constraints) {
+                                continue;
+                            }
                             if has_not_null_and_no_default_constraint(&column_def.constraints) {
                                 errs.push(RuleViolation::new(
                                     RuleViolationKind::AddingRequiredField,
@@ -86,4 +97,20 @@ ALTER TABLE "recipe" ADD COLUMN "public" boolean NOT NULL;
   "#;
         assert_debug_snapshot!(lint_sql(bad_sql));
     }
+    #[test]
+    fn test_generated_stored_not_null() {
+        let ok_sql = r"
+ ALTER TABLE foo
+    ADD COLUMN bar numeric GENERATED ALWAYS AS (bar + baz) STORED NOT NULL;
+        ";
+        assert_debug_snapshot!(lint_sql(ok_sql));
+    }
+    #[test]
+    fn test_generated_stored() {
+        let ok_sql = r"
+ ALTER TABLE foo
+    ADD COLUMN bar numeric GENERATED ALWAYS AS (bar + baz) STORED ;
+        ";
+        assert_debug_snapshot!(lint_sql(ok_sql));
+    }
 }

linter/src/rules/adding_required_field.rs

@@ -0,0 +1,23 @@
+---
+source: linter/src/rules/adding_field_with_default.rs
+expression: "lint_sql(bad_sql, pg_version_11)"
+---
+[
+    RuleViolation {
+        kind: AddingFieldWithDefault,
+        span: Span {
+            start: 0,
+            len: Some(
+                90,
+            ),
+        },
+        messages: [
+            Note(
+                "Adding a generated column requires a table rewrite with an ACCESS EXCLUSIVE lock.",
+            ),
+            Help(
+                "Add the column as nullable, backfill existing rows, and add a trigger to update the column on write instead.",
+            ),
+        ],
+    },
+]

linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test_rules__generated_stored.snap

@@ -0,0 +1,5 @@
+---
+source: linter/src/rules/adding_required_field.rs
+expression: lint_sql(ok_sql)
+---
+[]