From 0ae786810f450af9423ad310c4813cad4e787d5a Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Mon, 15 Sep 2025 21:45:59 -0400 Subject: [PATCH 1/2] squawk_linter: fix adding-field-with-default to allow more cases we now allow: - current_timestamp - array expressions - const & non-volatile binary expressions --- Cargo.lock | 16 ++-- .../src/rules/adding_field_with_default.rs | 81 ++++++++++++++++--- ...default__test__default_empty_array_ok.snap | 5 ++ ...st__default_func_current_timestamp_ok.snap | 5 ++ ...lt__test__default_with_const_bin_expr.snap | 5 ++ 5 files changed, 93 insertions(+), 19 deletions(-) create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_empty_array_ok.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_func_current_timestamp_ok.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_with_const_bin_expr.snap diff --git a/Cargo.lock b/Cargo.lock index 192859ae..e0c45040 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1861,7 +1861,7 @@ dependencies = [ [[package]] name = "squawk_github" -version = "2.25.0" +version = "2.25.1" dependencies = [ "jsonwebtoken", "log", @@ -1872,14 +1872,14 @@ dependencies = [ [[package]] name = "squawk_lexer" -version = "2.25.0" +version = "2.25.1" dependencies = [ "insta", ] [[package]] name = "squawk_linter" -version = "2.25.0" +version = "2.25.1" dependencies = [ "enum-iterator", "insta", @@ -1893,7 +1893,7 @@ dependencies = [ [[package]] name = "squawk_parser" -version = "2.25.0" +version = "2.25.1" dependencies = [ "camino", "dir-test", @@ -1906,7 +1906,7 @@ dependencies = [ [[package]] name = "squawk_server" -version = "2.25.0" +version = "2.25.1" dependencies = [ "anyhow", "insta", @@ -1924,7 +1924,7 @@ dependencies = [ [[package]] name = "squawk_syntax" -version = "2.25.0" +version = "2.25.1" dependencies = [ "camino", "dir-test", @@ -1936,7 +1936,7 @@ dependencies = [ [[package]] name = "squawk_wasm" -version = "2.25.0" +version = "2.25.1" dependencies = [ "console_error_panic_hook", "console_log", @@ -2800,7 +2800,7 @@ checksum = "32ac00cd3f8ec9c1d33fb3e7958a82df6989c42d747bd326c822b1d625283547" [[package]] name = "xtask" -version = "2.25.0" +version = "2.25.1" dependencies = [ "anyhow", "camino", diff --git a/crates/squawk_linter/src/rules/adding_field_with_default.rs b/crates/squawk_linter/src/rules/adding_field_with_default.rs index e837bfab..dfb64536 100644 --- a/crates/squawk_linter/src/rules/adding_field_with_default.rs +++ b/crates/squawk_linter/src/rules/adding_field_with_default.rs @@ -2,19 +2,11 @@ use lazy_static::lazy_static; use std::collections::HashSet; use squawk_syntax::ast::AstNode; -use squawk_syntax::{Parse, SourceFile}; +use squawk_syntax::{Parse, SourceFile, SyntaxKind}; use squawk_syntax::{ast, identifier::Identifier}; use crate::{Linter, Rule, Version, Violation}; -fn is_const_expr(expr: &ast::Expr) -> bool { - match expr { - ast::Expr::Literal(_) => true, - ast::Expr::CastExpr(cast) => matches!(cast.expr(), Some(ast::Expr::Literal(_))), - _ => false, - } -} - lazy_static! { static ref NON_VOLATILE_FUNCS: HashSet = { NON_VOLATILE_BUILT_IN_FUNCTIONS @@ -26,8 +18,18 @@ lazy_static! { }; } -fn is_non_volatile(expr: &ast::Expr) -> bool { +fn is_non_volatile_or_const(expr: &ast::Expr) -> bool { match expr { + ast::Expr::Literal(_) => true, + ast::Expr::ArrayExpr(_) => true, + ast::Expr::BinExpr(bin_expr) => { + if let Some(lhs) = bin_expr.lhs() { + if let Some(rhs) = bin_expr.rhs() { + return is_non_volatile_or_const(&lhs) && is_non_volatile_or_const(&rhs); + } + } + false + } ast::Expr::CallExpr(call_expr) => { if let Some(arglist) = call_expr.arg_list() { let no_args = arglist.args().count() == 0; @@ -45,6 +47,24 @@ fn is_non_volatile(expr: &ast::Expr) -> bool { false } } + // array[]::t[] is non-volatile. We don't check for a plain array expr + // since postgres will reject it as a default unless it's cast to a type. + ast::Expr::CastExpr(cast_expr) => { + if let Some(inner_expr) = cast_expr.expr() { + is_non_volatile_or_const(&inner_expr) + } else { + false + } + } + // current_timestamp is the same as calling now() + ast::Expr::NameRef(name_ref) => { + if let Some(child) = name_ref.syntax().first_child_or_token() { + if child.kind() == SyntaxKind::CURRENT_TIMESTAMP_KW { + return true; + } + } + false + } _ => false, } } @@ -69,7 +89,7 @@ pub(crate) fn adding_field_with_default(ctx: &mut Linter, parse: &Parse Version::new(11, None, None) - && (is_const_expr(&expr) || is_non_volatile(&expr)) + && is_non_volatile_or_const(&expr) { continue; } @@ -181,6 +201,33 @@ ALTER TABLE "core_recipe" ADD COLUMN "foo" boolean DEFAULT true; assert_debug_snapshot!(errors); } + #[test] + fn default_empty_array_ok() { + let sql = r#" +alter table t add column a double precision[] default array[]::double precision[]; + +alter table t add column b bigint[] default cast(array[] as bigint[]); + +alter table t add column c text[] default array['foo', 'bar']::text[]; + "#; + + let errors = lint(sql, Rule::AddingFieldWithDefault); + assert!(errors.is_empty()); + assert_debug_snapshot!(errors); + } + + #[test] + fn default_with_const_bin_expr() { + let sql = r#" +ALTER TABLE assessments +ADD COLUMN statistics_last_updated_at timestamptz NOT NULL DEFAULT now() - interval '100 years'; + "#; + + let errors = lint(sql, Rule::AddingFieldWithDefault); + assert!(errors.is_empty()); + assert_debug_snapshot!(errors); + } + #[test] fn default_str_ok() { let sql = r#" @@ -240,6 +287,7 @@ ALTER TABLE "core_recipe" ADD COLUMN "foo" timestamptz DEFAULT now(123); assert!(!errors.is_empty()); assert_debug_snapshot!(errors); } + #[test] fn default_func_now_ok() { let sql = r#" @@ -252,6 +300,17 @@ ALTER TABLE "core_recipe" ADD COLUMN "foo" timestamptz DEFAULT now(); assert_debug_snapshot!(errors); } + #[test] + fn default_func_current_timestamp_ok() { + let sql = r#" +alter table t add column c timestamptz default current_timestamp; + "#; + + let errors = lint(sql, Rule::AddingFieldWithDefault); + assert!(errors.is_empty()); + assert_debug_snapshot!(errors); + } + #[test] fn add_numbers_ok() { // This should be okay, but we don't handle expressions like this at the moment. diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_empty_array_ok.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_empty_array_ok.snap new file mode 100644 index 00000000..5563b6de --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_empty_array_ok.snap @@ -0,0 +1,5 @@ +--- +source: crates/squawk_linter/src/rules/adding_field_with_default.rs +expression: errors +--- +[] diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_func_current_timestamp_ok.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_func_current_timestamp_ok.snap new file mode 100644 index 00000000..5563b6de --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_func_current_timestamp_ok.snap @@ -0,0 +1,5 @@ +--- +source: crates/squawk_linter/src/rules/adding_field_with_default.rs +expression: errors +--- +[] diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_with_const_bin_expr.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_with_const_bin_expr.snap new file mode 100644 index 00000000..5563b6de --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__default_with_const_bin_expr.snap @@ -0,0 +1,5 @@ +--- +source: crates/squawk_linter/src/rules/adding_field_with_default.rs +expression: errors +--- +[] From 7ca30b6d12f29a49dbda0645144164326ba8b68b Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Mon, 15 Sep 2025 21:50:09 -0400 Subject: [PATCH 2/2] fix --- .../src/rules/adding_field_with_default.rs | 2 +- ...ing_field_with_default__test__add_numbers_ok.snap | 12 +----------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/crates/squawk_linter/src/rules/adding_field_with_default.rs b/crates/squawk_linter/src/rules/adding_field_with_default.rs index dfb64536..2bd61656 100644 --- a/crates/squawk_linter/src/rules/adding_field_with_default.rs +++ b/crates/squawk_linter/src/rules/adding_field_with_default.rs @@ -313,12 +313,12 @@ alter table t add column c timestamptz default current_timestamp; #[test] fn add_numbers_ok() { - // This should be okay, but we don't handle expressions like this at the moment. let sql = r#" alter table account_metadata add column blah integer default 2 + 2; "#; let errors = lint(sql, Rule::AddingFieldWithDefault); + assert!(errors.is_empty()); assert_debug_snapshot!(errors); } diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__add_numbers_ok.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__add_numbers_ok.snap index 17c224ea..5563b6de 100644 --- a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__add_numbers_ok.snap +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_field_with_default__test__add_numbers_ok.snap @@ -2,14 +2,4 @@ source: crates/squawk_linter/src/rules/adding_field_with_default.rs expression: errors --- -[ - Violation { - code: AddingFieldWithDefault, - message: "Adding a generated column requires a table rewrite with an `ACCESS EXCLUSIVE` lock. In Postgres versions 11+, non-VOLATILE DEFAULTs can be added without a rewrite.", - text_range: 62..67, - help: Some( - "Add the column as nullable, backfill existing rows, and add a trigger to update the column on write instead.", - ), - fix: None, - }, -] +[]