diff --git a/CHANGELOG.md b/CHANGELOG.md index f91477a4..6627c9e2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Cargo.lock b/Cargo.lock index c3d96ce5..06a0dceb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "addr2line" @@ -440,6 +440,12 @@ dependencies = [ "percent-encoding 2.1.0", ] +[[package]] +name = "fs_extra" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c" + [[package]] name = "fuchsia-cprng" version = "0.1.1" @@ -753,10 +759,12 @@ dependencies = [ [[package]] name = "libpg_query-sys" version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b71cba258ea83cb94ab1709c23fe74fd3ade56100fe0aba279449197cf6d2efa" +source = "git+https://github.com/chdsbd/libpg_query-sys.git?rev=f4584dcbcb8c1f3bee550477257e84a846fdd92d#f4584dcbcb8c1f3bee550477257e84a846fdd92d" dependencies = [ "bindgen", + "cc", + "fs_extra", + "glob", "make-cmd", ] @@ -1578,7 +1586,7 @@ checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" [[package]] name = "squawk" -version = "1.4.0" +version = "1.5.0" dependencies = [ "atty", "base64 0.12.3", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 9b3050d5..4c2f5321 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "squawk" -version = "1.4.0" +version = "1.5.0" authors = ["Steve Dignam "] edition = "2018" license = "GPL-3.0" diff --git a/cli/src/reporter.rs b/cli/src/reporter.rs index 27244848..c8894b96 100644 --- a/cli/src/reporter.rs +++ b/cli/src/reporter.rs @@ -481,10 +481,10 @@ fn get_sql_file_content(violation: &ViolationContent) -> Result 0 { format!( - r#" + r" ``` {} -```"#, +```", violations_text.trim_matches('\n') ) } else { @@ -494,7 +494,7 @@ fn get_sql_file_content(violation: &ViolationContent) -> Result{filename} ```sql @@ -506,7 +506,7 @@ fn get_sql_file_content(violation: &ViolationContent) -> Result 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(), diff --git a/docs/docs/adding-field-with-default.md b/docs/docs/adding-field-with-default.md index ac39c022..8f97d386 100644 --- a/docs/docs/adding-field-with-default.md +++ b/docs/docs/adding-field-with-default.md @@ -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: diff --git a/flake.nix b/flake.nix index bf90cc34..cafadfd6 100644 --- a/flake.nix +++ b/flake.nix @@ -18,7 +18,7 @@ { squawk = final.rustPlatform.buildRustPackage { pname = "squawk"; - version = "1.4.0"; + version = "1.5.0"; cargoLock = { lockFile = ./Cargo.lock; diff --git a/linter/src/rules/adding_field_with_default.rs b/linter/src/rules/adding_field_with_default.rs index 651ff2a4..468c1fd6 100644 --- a/linter/src/rules/adding_field_with_default.rs +++ b/linter/src/rules/adding_field_with_default.rs @@ -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)); + } } diff --git a/linter/src/rules/adding_required_field.rs b/linter/src/rules/adding_required_field.rs index 78c42adf..1748c7bf 100644 --- a/linter/src/rules/adding_required_field.rs +++ b/linter/src/rules/adding_required_field.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)); + } } diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test_rules__generated_stored.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test_rules__generated_stored.snap new file mode 100644 index 00000000..fd39b05c --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test_rules__generated_stored.snap @@ -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.", + ), + ], + }, +] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_required_field__test_rules__generated_stored.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_required_field__test_rules__generated_stored.snap new file mode 100644 index 00000000..86aef5e3 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_required_field__test_rules__generated_stored.snap @@ -0,0 +1,5 @@ +--- +source: linter/src/rules/adding_required_field.rs +expression: lint_sql(ok_sql) +--- +[] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_required_field__test_rules__generated_stored_not_null.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_required_field__test_rules__generated_stored_not_null.snap new file mode 100644 index 00000000..86aef5e3 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_required_field__test_rules__generated_stored_not_null.snap @@ -0,0 +1,5 @@ +--- +source: linter/src/rules/adding_required_field.rs +expression: lint_sql(ok_sql) +--- +[] diff --git a/package.json b/package.json index db4d24e6..449fb0e2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "squawk-cli", - "version": "1.4.0", + "version": "1.5.0", "description": "linter for PostgreSQL, focused on migrations", "repository": "git@github.com:sbdchd/squawk.git", "author": "Steve Dignam ", diff --git a/parser/Cargo.toml b/parser/Cargo.toml index c93b6667..e99d2d50 100644 --- a/parser/Cargo.toml +++ b/parser/Cargo.toml @@ -15,7 +15,7 @@ keywords = ["postgres", "sql", "parser"] serde_json = "1.0" serde = { version = "1.0", features = ["derive"] } serde_repr = "0.1" -libpg_query-sys = "0.4.0" +libpg_query-sys = { git = "https://github.com/chdsbd/libpg_query-sys.git", rev = "f4584dcbcb8c1f3bee550477257e84a846fdd92d" } [dev-dependencies] insta = "0.16.0" diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 899e4f4d..b140484c 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,3 +1,3 @@ [toolchain] targets = ["x86_64-unknown-linux-gnu", "x86_64-apple-darwin", "aarch64-apple-darwin"] -channel = "1.80.1" +channel = "1.84.0"