From 36d87b66a6a1abe234e137d705f901529a42ba81 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Thu, 1 Oct 2026 11:33:40 +1000 Subject: [PATCH 01/21] docs: describe new ban-drop rules --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c3e2520..8c4867b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- linter: ban-drop-view, ban-drop-function, ban-drop-type, ban-drop-default rules +- linter: opt-in ban-drop-trigger rule + ## v2.67.0 - 2026-10-04 ### Added From 1548e50198129134f86ad16db7e2a255073a4506 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Thu, 1 Oct 2026 14:51:54 +1000 Subject: [PATCH 02/21] feat(linter): add backwards compatibility rules for schema changes --- crates/squawk_linter/src/ignore.rs | 2 +- crates/squawk_linter/src/lib.rs | 152 +++++++++++++++++- .../rules/ban_alter_generated_expression.rs | 51 ++++++ .../src/rules/ban_alter_identity.rs | 70 ++++++++ .../src/rules/ban_disable_trigger.rs | 43 +++++ .../src/rules/ban_drop_constraint.rs | 38 +++++ .../src/rules/ban_drop_domain.rs | 35 ++++ .../squawk_linter/src/rules/ban_drop_index.rs | 37 +++++ .../src/rules/ban_drop_policy.rs | 47 ++++++ .../src/rules/ban_drop_schema.rs | 35 ++++ .../src/rules/ban_drop_sequence.rs | 35 ++++ .../src/rules/ban_replace_view_function.rs | 40 +++++ .../src/rules/ban_replica_identity.rs | 38 +++++ crates/squawk_linter/src/rules/ban_revoke.rs | 52 ++++++ .../src/rules/ban_set_default.rs | 40 +++++ .../squawk_linter/src/rules/ban_set_schema.rs | 110 +++++++++++++ crates/squawk_linter/src/rules/mod.rs | 30 ++++ .../src/rules/renaming_object.rs | 143 ++++++++++++++++ ...alter_generated_expression__test__err.snap | 12 ++ ..._rules__ban_alter_identity__test__err.snap | 16 ++ ...rules__ban_disable_trigger__test__err.snap | 20 +++ ...rules__ban_drop_constraint__test__err.snap | 8 + ...er__rules__ban_drop_domain__test__err.snap | 8 + ...ter__rules__ban_drop_index__test__err.snap | 12 ++ ...er__rules__ban_drop_policy__test__err.snap | 12 ++ ...er__rules__ban_drop_schema__test__err.snap | 8 + ...__rules__ban_drop_sequence__test__err.snap | 8 + ..._ban_replace_view_function__test__err.snap | 16 ++ ...ules__ban_replica_identity__test__err.snap | 8 + ..._linter__rules__ban_revoke__test__err.snap | 12 ++ ...er__rules__ban_set_default__test__err.snap | 8 + ...ter__rules__ban_set_schema__test__err.snap | 32 ++++ ...er__rules__renaming_object__test__err.snap | 44 +++++ 33 files changed, 1219 insertions(+), 3 deletions(-) create mode 100644 crates/squawk_linter/src/rules/ban_alter_generated_expression.rs create mode 100644 crates/squawk_linter/src/rules/ban_alter_identity.rs create mode 100644 crates/squawk_linter/src/rules/ban_disable_trigger.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_constraint.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_domain.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_index.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_policy.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_schema.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_sequence.rs create mode 100644 crates/squawk_linter/src/rules/ban_replace_view_function.rs create mode 100644 crates/squawk_linter/src/rules/ban_replica_identity.rs create mode 100644 crates/squawk_linter/src/rules/ban_revoke.rs create mode 100644 crates/squawk_linter/src/rules/ban_set_default.rs create mode 100644 crates/squawk_linter/src/rules/ban_set_schema.rs create mode 100644 crates/squawk_linter/src/rules/renaming_object.rs create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_identity__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_constraint__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_domain__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_schema__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_sequence__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_schema__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__renaming_object__test__err.snap diff --git a/crates/squawk_linter/src/ignore.rs b/crates/squawk_linter/src/ignore.rs index b1d80c1b..1346b09e 100644 --- a/crates/squawk_linter/src/ignore.rs +++ b/crates/squawk_linter/src/ignore.rs @@ -278,7 +278,7 @@ alter table t drop column c cascade; alter table t add column c char; ALTER TABLE foo --- squawk-ignore adding-field-with-default,prefer-robust-stmts +-- squawk-ignore adding-field-with-default,prefer-robust-stmts,ban-alter-generated-expression ADD COLUMN bar numeric GENERATED ALWAYS AS (bar + baz) STORED; diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 16c004c1..0b2e7df8 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -68,6 +68,12 @@ use rules::require_enum_value_ordering; use rules::require_table_schema; use rules::require_timeout_settings; use rules::transaction_nesting; +use rules::{ + ban_alter_generated_expression, ban_alter_identity, ban_disable_trigger, ban_drop_constraint, + ban_drop_domain, ban_drop_index, ban_drop_policy, ban_drop_schema, ban_drop_sequence, + ban_replace_view_function, ban_replica_identity, ban_revoke, ban_set_default, ban_set_schema, + renaming_object, +}; // xtask:new-rule:rule-import #[derive(Debug, PartialEq, Clone, Copy, Hash, Eq, Sequence)] @@ -117,6 +123,21 @@ pub enum Rule { BanDropType, BanDropDefault, BanDropTrigger, + BanDropSchema, + BanDropSequence, + BanDropDomain, + BanDropConstraint, + RenamingObject, + BanSetSchema, + BanAlterIdentity, + BanAlterGeneratedExpression, + BanDropIndex, + BanSetDefault, + BanDisableTrigger, + BanReplicaIdentity, + BanDropPolicy, + BanRevoke, + BanReplaceViewFunction, // xtask:new-rule:error-name } @@ -127,7 +148,19 @@ impl Rule { // require-timeout-settings is an alias, see `Rule::expands_to` matches!( self, - Rule::RequireTableSchema | Rule::RequireTimeoutSettings | Rule::BanDropTrigger + Rule::RequireTableSchema + | Rule::RequireTimeoutSettings + | Rule::BanDropTrigger + | Rule::BanDropConstraint + | Rule::BanAlterIdentity + | Rule::BanAlterGeneratedExpression + | Rule::BanDropIndex + | Rule::BanSetDefault + | Rule::BanDisableTrigger + | Rule::BanReplicaIdentity + | Rule::BanDropPolicy + | Rule::BanRevoke + | Rule::BanReplaceViewFunction ) } @@ -195,6 +228,21 @@ impl TryFrom<&str> for Rule { "ban-drop-type" => Ok(Rule::BanDropType), "ban-drop-default" => Ok(Rule::BanDropDefault), "ban-drop-trigger" => Ok(Rule::BanDropTrigger), + "ban-drop-schema" => Ok(Rule::BanDropSchema), + "ban-drop-sequence" => Ok(Rule::BanDropSequence), + "ban-drop-domain" => Ok(Rule::BanDropDomain), + "ban-drop-constraint" => Ok(Rule::BanDropConstraint), + "renaming-object" => Ok(Rule::RenamingObject), + "ban-set-schema" => Ok(Rule::BanSetSchema), + "ban-alter-identity" => Ok(Rule::BanAlterIdentity), + "ban-alter-generated-expression" => Ok(Rule::BanAlterGeneratedExpression), + "ban-drop-index" => Ok(Rule::BanDropIndex), + "ban-set-default" => Ok(Rule::BanSetDefault), + "ban-disable-trigger" => Ok(Rule::BanDisableTrigger), + "ban-replica-identity" => Ok(Rule::BanReplicaIdentity), + "ban-drop-policy" => Ok(Rule::BanDropPolicy), + "ban-revoke" => Ok(Rule::BanRevoke), + "ban-replace-view-function" => Ok(Rule::BanReplaceViewFunction), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -271,6 +319,21 @@ impl fmt::Display for Rule { Rule::BanDropType => "ban-drop-type", Rule::BanDropDefault => "ban-drop-default", Rule::BanDropTrigger => "ban-drop-trigger", + Rule::BanDropSchema => "ban-drop-schema", + Rule::BanDropSequence => "ban-drop-sequence", + Rule::BanDropDomain => "ban-drop-domain", + Rule::BanDropConstraint => "ban-drop-constraint", + Rule::RenamingObject => "renaming-object", + Rule::BanSetSchema => "ban-set-schema", + Rule::BanAlterIdentity => "ban-alter-identity", + Rule::BanAlterGeneratedExpression => "ban-alter-generated-expression", + Rule::BanDropIndex => "ban-drop-index", + Rule::BanSetDefault => "ban-set-default", + Rule::BanDisableTrigger => "ban-disable-trigger", + Rule::BanReplicaIdentity => "ban-replica-identity", + Rule::BanDropPolicy => "ban-drop-policy", + Rule::BanRevoke => "ban-revoke", + Rule::BanReplaceViewFunction => "ban-replace-view-function", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -540,6 +603,34 @@ impl Linter { if self.rules.contains(&Rule::BanDropTrigger) { ban_drop_trigger(self, file); } + for (rule, check) in [ + ( + Rule::BanDropSchema, + ban_drop_schema + as fn(&mut Linter, &squawk_syntax::Parse), + ), + (Rule::BanDropSequence, ban_drop_sequence), + (Rule::BanDropDomain, ban_drop_domain), + (Rule::BanDropConstraint, ban_drop_constraint), + (Rule::RenamingObject, renaming_object), + (Rule::BanSetSchema, ban_set_schema), + (Rule::BanAlterIdentity, ban_alter_identity), + ( + Rule::BanAlterGeneratedExpression, + ban_alter_generated_expression, + ), + (Rule::BanDropIndex, ban_drop_index), + (Rule::BanSetDefault, ban_set_default), + (Rule::BanDisableTrigger, ban_disable_trigger), + (Rule::BanReplicaIdentity, ban_replica_identity), + (Rule::BanDropPolicy, ban_drop_policy), + (Rule::BanRevoke, ban_revoke), + (Rule::BanReplaceViewFunction, ban_replace_view_function), + ] { + if self.rules.contains(&rule) { + check(self, file); + } + } // xtask:new-rule:rule-call // locate any ignores in the file @@ -635,11 +726,68 @@ mod tests { let linter = Linter::with_rules(&[], &[]); assert!(!linter.rules.contains(&Rule::RequireTableSchema)); assert!(!linter.rules.contains(&Rule::BanDropTrigger)); + for rule in [ + Rule::BanDropConstraint, + Rule::BanAlterIdentity, + Rule::BanAlterGeneratedExpression, + Rule::BanDropIndex, + Rule::BanSetDefault, + Rule::BanDisableTrigger, + Rule::BanReplicaIdentity, + Rule::BanDropPolicy, + Rule::BanRevoke, + Rule::BanReplaceViewFunction, + ] { + assert!(!linter.rules.contains(&rule)); + } + } + + #[test] + fn new_opt_in_rules_only_report_when_included() { + for (rule, sql) in [ + (Rule::BanDropConstraint, "ALTER TABLE t DROP CONSTRAINT c;"), + ( + Rule::BanAlterIdentity, + "ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY;", + ), + ( + Rule::BanAlterGeneratedExpression, + "ALTER TABLE t ALTER COLUMN c DROP EXPRESSION;", + ), + ] { + let parse = SourceFile::parse(sql); + assert!(parse.errors().is_empty()); + assert!( + !Linter::with_default_rules() + .lint(&parse, sql) + .iter() + .any(|violation| violation.code == rule) + ); + assert!( + Linter::with_rules(&[rule], &[]) + .lint(&parse, sql) + .iter() + .any(|violation| violation.code == rule) + ); + } } #[test] fn with_rules_opt_in_enabled_via_include() { - for rule in [Rule::RequireTableSchema, Rule::BanDropTrigger] { + for rule in [ + Rule::RequireTableSchema, + Rule::BanDropTrigger, + Rule::BanDropConstraint, + Rule::BanAlterIdentity, + Rule::BanAlterGeneratedExpression, + Rule::BanDropIndex, + Rule::BanSetDefault, + Rule::BanDisableTrigger, + Rule::BanReplicaIdentity, + Rule::BanDropPolicy, + Rule::BanRevoke, + Rule::BanReplaceViewFunction, + ] { let linter = Linter::with_rules(&[rule], &[]); assert!(linter.rules.contains(&rule)); } diff --git a/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs new file mode 100644 index 00000000..1c937646 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs @@ -0,0 +1,51 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_alter_generated_expression(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + match action { + ast::AlterTableAction::AlterColumn(column) => { + if let Some(ast::AlterColumnOption::DropExpression(node)) = column.option() + { + ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Changing a generated column may break inserts from existing clients.".into(), node.syntax())); + } + } + ast::AlterTableAction::AddColumn(column) => { + for constraint in column.constraints() { + if let ast::Constraint::GeneratedConstraint(node) = constraint { + ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Changing a generated column may break inserts from existing clients.".into(), node.syntax())); + } + } + } + _ => (), + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;"; + assert_snapshot!(lint_errors(sql, Rule::BanAlterGeneratedExpression)); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ADD COLUMN c int;", + Rule::BanAlterGeneratedExpression, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_alter_identity.rs b/crates/squawk_linter/src/rules/ban_alter_identity.rs new file mode 100644 index 00000000..84dc73ed --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_alter_identity.rs @@ -0,0 +1,70 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_alter_identity(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + if let ast::AlterTableAction::AlterColumn(column) = action { + if let Some(option) = column.option() { + match option { + ast::AlterColumnOption::AddGenerated(node) => { + ctx.report(Violation::for_node(Rule::BanAlterIdentity, "Changing column identity may break inserts from existing clients.".into(), node.syntax())); + } + ast::AlterColumnOption::DropIdentity(node) => { + ctx.report(Violation::for_node(Rule::BanAlterIdentity, "Changing column identity may break inserts from existing clients.".into(), node.syntax())); + } + ast::AlterColumnOption::SetGenerated(node) + if matches!( + node.generated_when(), + Some(ast::GeneratedWhen::GeneratedAlways(_)) + ) => + { + ctx.report(Violation::for_node(Rule::BanAlterIdentity, "Changing column identity may break inserts from existing clients.".into(), node.syntax())); + } + ast::AlterColumnOption::SetGeneratedOptions(options) => { + for option in options.set_generated_options() { + if let ast::SetGeneratedOption::SetGenerated(node) = option { + if matches!( + node.generated_when(), + Some(ast::GeneratedWhen::GeneratedAlways(_)) + ) { + ctx.report(Violation::for_node(Rule::BanAlterIdentity, "Changing column identity may break inserts from existing clients.".into(), node.syntax())); + } + } + } + } + _ => (), + } + } + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY; ALTER TABLE t ALTER COLUMN id DROP IDENTITY; ALTER TABLE t ALTER COLUMN id SET GENERATED ALWAYS;"; + let errors = lint_errors(sql, Rule::BanAlterIdentity); + assert_eq!(errors.matches("warning[ban-alter-identity]").count(), 3); + assert_snapshot!(errors); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ALTER COLUMN id SET DEFAULT 1; ALTER TABLE t ALTER COLUMN id SET GENERATED BY DEFAULT;", + Rule::BanAlterIdentity, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_disable_trigger.rs b/crates/squawk_linter/src/rules/ban_disable_trigger.rs new file mode 100644 index 00000000..55a90722 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_disable_trigger.rs @@ -0,0 +1,43 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_disable_trigger(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + if matches!( + action, + ast::AlterTableAction::DisableTrigger(_) + | ast::AlterTableAction::DisableRule(_) + | ast::AlterTableAction::DisableRls(_) + | ast::AlterTableAction::ForceRls(_) + ) { + ctx.report(Violation::for_node(Rule::BanDisableTrigger, "Disabling a trigger, rule, or row level security may silently change behaviour for existing clients.".into(), action.syntax())); + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVEL SECURITY;"; + let errors = lint_errors(sql, Rule::BanDisableTrigger); + assert_eq!(errors.matches("warning[ban-disable-trigger]").count(), 4); + assert_snapshot!(errors); + } + #[test] + fn ok() { + lint_ok("ALTER TABLE t ENABLE TRIGGER trg;", Rule::BanDisableTrigger); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_constraint.rs b/crates/squawk_linter/src/rules/ban_drop_constraint.rs new file mode 100644 index 00000000..2e058da4 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_constraint.rs @@ -0,0 +1,38 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_constraint(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + if let ast::AlterTableAction::DropConstraint(node) = action { + ctx.report(Violation::for_node(Rule::BanDropConstraint, "Dropping a constraint may remove a guarantee that existing clients assume.".into(), node.syntax())); + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t DROP CONSTRAINT IF EXISTS c;"; + assert_snapshot!(lint_errors(sql, Rule::BanDropConstraint)); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0);", + Rule::BanDropConstraint, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_domain.rs b/crates/squawk_linter/src/rules/ban_drop_domain.rs new file mode 100644 index 00000000..ac3da242 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_domain.rs @@ -0,0 +1,35 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_domain(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::DropDomain(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropDomain, + "Dropping a domain may break existing clients.".into(), + node.syntax(), + )); + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "DROP DOMAIN IF EXISTS d CASCADE;"; + assert_snapshot!(lint_errors(sql, Rule::BanDropDomain)); + } + #[test] + fn ok() { + lint_ok("CREATE DOMAIN d AS integer;", Rule::BanDropDomain); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_index.rs b/crates/squawk_linter/src/rules/ban_drop_index.rs new file mode 100644 index 00000000..cb80db81 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_index.rs @@ -0,0 +1,37 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_index(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::DropIndex(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropIndex, + "Dropping an index may remove a guarantee or change query plans for existing clients.".into(), + node.syntax(), + )); + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "DROP INDEX CONCURRENTLY IF EXISTS i; DROP INDEX i;"; + let errors = lint_errors(sql, Rule::BanDropIndex); + assert_eq!(errors.matches("warning[ban-drop-index]").count(), 2); + assert_snapshot!(errors); + } + #[test] + fn ok() { + lint_ok("CREATE INDEX i ON t (id);", Rule::BanDropIndex); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_policy.rs b/crates/squawk_linter/src/rules/ban_drop_policy.rs new file mode 100644 index 00000000..91da9365 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_policy.rs @@ -0,0 +1,47 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_policy(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::DropPolicy(node) => { + ctx.report(Violation::for_node( + Rule::BanDropPolicy, + "Dropping a policy or rule may silently change behaviour for existing clients." + .into(), + node.syntax(), + )); + } + ast::Stmt::DropRule(node) => { + ctx.report(Violation::for_node( + Rule::BanDropPolicy, + "Dropping a policy or rule may silently change behaviour for existing clients." + .into(), + node.syntax(), + )); + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "DROP POLICY IF EXISTS p ON t; DROP RULE IF EXISTS r ON t;"; + assert_snapshot!(lint_errors(sql, Rule::BanDropPolicy)); + } + #[test] + fn ok() { + lint_ok("CREATE POLICY p ON t USING (true);", Rule::BanDropPolicy); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_schema.rs b/crates/squawk_linter/src/rules/ban_drop_schema.rs new file mode 100644 index 00000000..1db2fd08 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_schema.rs @@ -0,0 +1,35 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_schema(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::DropSchema(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropSchema, + "Dropping a schema may break existing clients.".into(), + node.syntax(), + )); + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "DROP SCHEMA IF EXISTS s CASCADE;"; + assert_snapshot!(lint_errors(sql, Rule::BanDropSchema)); + } + #[test] + fn ok() { + lint_ok("CREATE SCHEMA s;", Rule::BanDropSchema); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_sequence.rs b/crates/squawk_linter/src/rules/ban_drop_sequence.rs new file mode 100644 index 00000000..f282d054 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_sequence.rs @@ -0,0 +1,35 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_sequence(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::DropSequence(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropSequence, + "Dropping a sequence may break existing clients.".into(), + node.syntax(), + )); + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "DROP SEQUENCE IF EXISTS s CASCADE;"; + assert_snapshot!(lint_errors(sql, Rule::BanDropSequence)); + } + #[test] + fn ok() { + lint_ok("CREATE SEQUENCE s;", Rule::BanDropSequence); + } +} diff --git a/crates/squawk_linter/src/rules/ban_replace_view_function.rs b/crates/squawk_linter/src/rules/ban_replace_view_function.rs new file mode 100644 index 00000000..c8301583 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_replace_view_function.rs @@ -0,0 +1,40 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_replace_view_function(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::CreateView(node) if node.or_replace().is_some() => { + ctx.report(Violation::for_node(Rule::BanReplaceViewFunction, "Replacing a view, function, or procedure may silently change behaviour for existing clients.".into(), node.syntax())); + } + ast::Stmt::CreateFunction(node) if node.or_replace().is_some() => { + ctx.report(Violation::for_node(Rule::BanReplaceViewFunction, "Replacing a view, function, or procedure may silently change behaviour for existing clients.".into(), node.syntax())); + } + ast::Stmt::CreateProcedure(node) if node.or_replace().is_some() => { + ctx.report(Violation::for_node(Rule::BanReplaceViewFunction, "Replacing a view, function, or procedure may silently change behaviour for existing clients.".into(), node.syntax())); + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "CREATE OR REPLACE VIEW v AS SELECT 1 AS id; CREATE OR REPLACE FUNCTION f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$; CREATE OR REPLACE PROCEDURE p() LANGUAGE sql AS $$ SELECT 1 $$;"; + assert_snapshot!(lint_errors(sql, Rule::BanReplaceViewFunction)); + } + #[test] + fn ok() { + lint_ok("CREATE VIEW v AS SELECT 1;", Rule::BanReplaceViewFunction); + } +} diff --git a/crates/squawk_linter/src/rules/ban_replica_identity.rs b/crates/squawk_linter/src/rules/ban_replica_identity.rs new file mode 100644 index 00000000..c81f3f3a --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_replica_identity.rs @@ -0,0 +1,38 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_replica_identity(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + if let ast::AlterTableAction::ReplicaIdentity(node) = action { + ctx.report(Violation::for_node(Rule::BanReplicaIdentity, "Changing replica identity may silently change replication for existing clients.".into(), node.syntax())); + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t REPLICA IDENTITY FULL;"; + assert_snapshot!(lint_errors(sql, Rule::BanReplicaIdentity)); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ENABLE ROW LEVEL SECURITY;", + Rule::BanReplicaIdentity, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_revoke.rs b/crates/squawk_linter/src/rules/ban_revoke.rs new file mode 100644 index 00000000..bf2cec06 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_revoke.rs @@ -0,0 +1,52 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_revoke(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::Revoke(node) => { + ctx.report(Violation::for_node( + Rule::BanRevoke, + "Revoking privileges may break existing clients.".into(), + node.syntax(), + )); + } + ast::Stmt::AlterDefaultPrivileges(node) => { + if matches!( + node.action(), + Some(ast::AlterDefaultPrivilegesAction::RevokeDefaultPrivileges( + _ + )) + ) { + ctx.report(Violation::for_node( + Rule::BanRevoke, + "Revoking privileges may break existing clients.".into(), + node.syntax(), + )); + } + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "REVOKE SELECT ON t FROM app; ALTER DEFAULT PRIVILEGES REVOKE SELECT ON TABLES FROM app;"; + assert_snapshot!(lint_errors(sql, Rule::BanRevoke)); + } + #[test] + fn ok() { + lint_ok("GRANT SELECT ON t TO app;", Rule::BanRevoke); + } +} diff --git a/crates/squawk_linter/src/rules/ban_set_default.rs b/crates/squawk_linter/src/rules/ban_set_default.rs new file mode 100644 index 00000000..bf85aa25 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_set_default.rs @@ -0,0 +1,40 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_set_default(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + if let ast::AlterTableAction::AlterColumn(column) = action { + if let Some(ast::AlterColumnOption::SetDefault(node)) = column.option() { + ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); + } + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t ALTER COLUMN c SET DEFAULT 1;"; + assert_snapshot!(lint_errors(sql, Rule::BanSetDefault)); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ALTER COLUMN c DROP DEFAULT;", + Rule::BanSetDefault, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_set_schema.rs b/crates/squawk_linter/src/rules/ban_set_schema.rs new file mode 100644 index 00000000..60bdbd58 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_set_schema.rs @@ -0,0 +1,110 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_set_schema(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::AlterTable(node) => { + for action in node.actions() { + if let ast::AlterTableAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterView(node) => { + for action in node.action().into_iter() { + if let ast::AlterViewAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterMaterializedView(node) => { + for action in node.action() { + if let ast::AlterMaterializedViewAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterFunction(node) => { + for action in node.action().into_iter() { + if let ast::AlterFunctionAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterType(node) => { + for action in node.action().into_iter() { + if let ast::AlterTypeAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterSequence(node) => { + for action in node.actions() { + if let ast::AlterSequenceAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterDomain(node) => { + for action in node.action().into_iter() { + if let ast::AlterDomainAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t SET SCHEMA s; ALTER VIEW v SET SCHEMA s; ALTER MATERIALIZED VIEW mv SET SCHEMA s; ALTER FUNCTION f() SET SCHEMA s; ALTER TYPE typ SET SCHEMA s; ALTER SEQUENCE seq SET SCHEMA s; ALTER DOMAIN d SET SCHEMA s;"; + let errors = lint_errors(sql, Rule::BanSetSchema); + assert_eq!(errors.matches("warning[ban-set-schema]").count(), 7); + assert_snapshot!(errors); + } + #[test] + fn ok() { + lint_ok("ALTER TABLE t OWNER TO app;", Rule::BanSetSchema); + } +} diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index e62d5272..03381b44 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -4,19 +4,33 @@ pub(crate) mod adding_not_null_field; pub(crate) mod adding_primary_key_constraint; pub(crate) mod adding_required_field; pub(crate) mod ban_alter_domain_with_add_constraint; +pub(crate) mod ban_alter_generated_expression; +pub(crate) mod ban_alter_identity; pub(crate) mod ban_char_field; pub(crate) mod ban_concurrent_index_creation_in_transaction; pub(crate) mod ban_create_domain_with_constraint; +pub(crate) mod ban_disable_trigger; pub(crate) mod ban_drop_column; +pub(crate) mod ban_drop_constraint; pub(crate) mod ban_drop_database; pub(crate) mod ban_drop_default; +pub(crate) mod ban_drop_domain; pub(crate) mod ban_drop_function; +pub(crate) mod ban_drop_index; pub(crate) mod ban_drop_not_null; +pub(crate) mod ban_drop_policy; +pub(crate) mod ban_drop_schema; +pub(crate) mod ban_drop_sequence; pub(crate) mod ban_drop_table; pub(crate) mod ban_drop_trigger; pub(crate) mod ban_drop_type; pub(crate) mod ban_drop_view; pub(crate) mod ban_duplicate_column_assignments; +pub(crate) mod ban_replace_view_function; +pub(crate) mod ban_replica_identity; +pub(crate) mod ban_revoke; +pub(crate) mod ban_set_default; +pub(crate) mod ban_set_schema; pub(crate) mod ban_truncate_cascade; pub(crate) mod ban_uncommitted_transaction; pub(crate) mod changing_column_type; @@ -31,6 +45,7 @@ pub(crate) mod prefer_robust_stmts; pub(crate) mod prefer_text_field; pub(crate) mod prefer_timestamptz; pub(crate) mod renaming_column; +pub(crate) mod renaming_object; pub(crate) mod renaming_table; pub(crate) mod require_concurrent_index_creation; pub(crate) mod require_concurrent_index_deletion; @@ -48,19 +63,33 @@ pub(crate) use adding_not_null_field::adding_not_null_field; pub(crate) use adding_primary_key_constraint::adding_primary_key_constraint; pub(crate) use adding_required_field::adding_required_field; pub(crate) use ban_alter_domain_with_add_constraint::ban_alter_domain_with_add_constraint; +pub(crate) use ban_alter_generated_expression::ban_alter_generated_expression; +pub(crate) use ban_alter_identity::ban_alter_identity; pub(crate) use ban_char_field::ban_char_field; pub(crate) use ban_concurrent_index_creation_in_transaction::ban_concurrent_index_creation_in_transaction; pub(crate) use ban_create_domain_with_constraint::ban_create_domain_with_constraint; +pub(crate) use ban_disable_trigger::ban_disable_trigger; pub(crate) use ban_drop_column::ban_drop_column; +pub(crate) use ban_drop_constraint::ban_drop_constraint; pub(crate) use ban_drop_database::ban_drop_database; pub(crate) use ban_drop_default::ban_drop_default; +pub(crate) use ban_drop_domain::ban_drop_domain; pub(crate) use ban_drop_function::ban_drop_function; +pub(crate) use ban_drop_index::ban_drop_index; pub(crate) use ban_drop_not_null::ban_drop_not_null; +pub(crate) use ban_drop_policy::ban_drop_policy; +pub(crate) use ban_drop_schema::ban_drop_schema; +pub(crate) use ban_drop_sequence::ban_drop_sequence; pub(crate) use ban_drop_table::ban_drop_table; pub(crate) use ban_drop_trigger::ban_drop_trigger; pub(crate) use ban_drop_type::ban_drop_type; pub(crate) use ban_drop_view::ban_drop_view; pub(crate) use ban_duplicate_column_assignments::ban_duplicate_column_assignments; +pub(crate) use ban_replace_view_function::ban_replace_view_function; +pub(crate) use ban_replica_identity::ban_replica_identity; +pub(crate) use ban_revoke::ban_revoke; +pub(crate) use ban_set_default::ban_set_default; +pub(crate) use ban_set_schema::ban_set_schema; 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; @@ -75,6 +104,7 @@ pub(crate) use prefer_robust_stmts::prefer_robust_stmts; pub(crate) use prefer_text_field::prefer_text_field; pub(crate) use prefer_timestamptz::prefer_timestamptz; pub(crate) use renaming_column::renaming_column; +pub(crate) use renaming_object::renaming_object; pub(crate) use renaming_table::renaming_table; pub(crate) use require_concurrent_index_creation::require_concurrent_index_creation; pub(crate) use require_concurrent_index_deletion::require_concurrent_index_deletion; diff --git a/crates/squawk_linter/src/rules/renaming_object.rs b/crates/squawk_linter/src/rules/renaming_object.rs new file mode 100644 index 00000000..a3bafef4 --- /dev/null +++ b/crates/squawk_linter/src/rules/renaming_object.rs @@ -0,0 +1,143 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn renaming_object(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::AlterView(node) => { + for action in node.action().into_iter() { + if let ast::AlterViewAction::ViewRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a view may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterMaterializedView(node) => { + for action in node.action() { + if let ast::AlterMaterializedViewAction::ViewRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a materialized view may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterFunction(node) => { + for action in node.action().into_iter() { + if let ast::AlterFunctionAction::FunctionRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a function may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterProcedure(node) => { + for action in node.action().into_iter() { + if let ast::AlterProcedureAction::ProcedureRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a procedure may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterType(node) => { + for action in node.action().into_iter() { + match action { + ast::AlterTypeAction::TypeRenameTo(node) => { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a type may break existing clients.".into(), + node.syntax(), + )) + } + ast::AlterTypeAction::RenameValue(node) => ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a type value may break existing clients.".into(), + node.syntax(), + )), + _ => (), + } + } + } + ast::Stmt::AlterSequence(node) => { + for action in node.actions() { + if let ast::AlterSequenceAction::SequenceRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a sequence may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterSchema(node) => { + for action in node.action().into_iter() { + if let ast::AlterSchemaAction::SchemaRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterDomain(node) => { + for action in node.action().into_iter() { + if let ast::AlterDomainAction::DomainRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a domain may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterIndex(node) => { + for action in node.action().into_iter() { + if let ast::AlterIndexAction::IndexRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming an index may break existing clients.".into(), + node.syntax(), + )); + } + } + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER VIEW v RENAME TO v2; ALTER MATERIALIZED VIEW mv RENAME TO mv2; ALTER FUNCTION f() RENAME TO f2; ALTER PROCEDURE p() RENAME TO p2; ALTER TYPE typ RENAME TO typ2; ALTER SEQUENCE seq RENAME TO seq2; ALTER SCHEMA s RENAME TO s2; ALTER DOMAIN d RENAME TO d2; ALTER INDEX i RENAME TO i2; ALTER TYPE typ RENAME VALUE 'a' TO 'b';"; + let errors = lint_errors(sql, Rule::RenamingObject); + assert_eq!(errors.matches("warning[renaming-object]").count(), 10); + assert_snapshot!(errors); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t RENAME TO t2; ALTER TABLE t RENAME COLUMN c TO d;", + Rule::RenamingObject, + ); + } +} diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap new file mode 100644 index 00000000..4eec1cce --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_alter_generated_expression.rs +expression: "lint_errors(sql, Rule::BanAlterGeneratedExpression)" +--- +warning[ban-alter-generated-expression]: Changing a generated column may break inserts from existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; + ╰╴ ━━━━━━━━━━━━━━━ +warning[ban-alter-generated-expression]: Changing a generated column may break inserts from existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_identity__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_identity__test__err.snap new file mode 100644 index 00000000..fb162586 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_identity__test__err.snap @@ -0,0 +1,16 @@ +--- +source: crates/squawk_linter/src/rules/ban_alter_identity.rs +expression: errors +--- +warning[ban-alter-identity]: Changing column identity may break inserts from existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY; ALTER TABLE t ALTER COLUMN id DROP IDENTITY; ALTER TABLE t ALTER COLUMN… + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-alter-identity]: Changing column identity may break inserts from existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY; ALTER TABLE t ALTER COLUMN id DROP IDENTITY; ALTER TABLE t ALTER COLUMN… + ╰╴ ━━━━━━━━━━━━━ +warning[ban-alter-identity]: Changing column identity may break inserts from existing clients. + ╭▸ +1 │ …R COLUMN id DROP IDENTITY; ALTER TABLE t ALTER COLUMN id SET GENERATED ALWAYS; + ╰╴ ━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap new file mode 100644 index 00000000..742e6ba5 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap @@ -0,0 +1,20 @@ +--- +source: crates/squawk_linter/src/rules/ban_disable_trigger.rs +expression: "lint_errors(sql, Rule::BanDisableTrigger)" +--- +warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. + ╭▸ +1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… + ╰╴ ━━━━━━━━━━━━━━━━━━━ +warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. + ╭▸ +1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… + ╰╴ ━━━━━━━━━━━━━━ +warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. + ╭▸ +1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. + ╭▸ +1 │ …LTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVEL SECURITY; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_constraint__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_constraint__test__err.snap new file mode 100644 index 00000000..7adc6f82 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_constraint__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_constraint.rs +expression: "lint_errors(sql, Rule::BanDropConstraint)" +--- +warning[ban-drop-constraint]: Dropping a constraint may remove a guarantee that existing clients assume. + ╭▸ +1 │ ALTER TABLE t DROP CONSTRAINT IF EXISTS c; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_domain__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_domain__test__err.snap new file mode 100644 index 00000000..71c78c62 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_domain__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_domain.rs +expression: "lint_errors(sql, Rule::BanDropDomain)" +--- +warning[ban-drop-domain]: Dropping a domain may break existing clients. + ╭▸ +1 │ DROP DOMAIN IF EXISTS d CASCADE; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap new file mode 100644 index 00000000..49fbcc83 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_index.rs +expression: errors +--- +warning[ban-drop-index]: Dropping an index may remove a guarantee or change query plans for existing clients. + ╭▸ +1 │ DROP INDEX CONCURRENTLY IF EXISTS i; DROP INDEX i; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-drop-index]: Dropping an index may remove a guarantee or change query plans for existing clients. + ╭▸ +1 │ DROP INDEX CONCURRENTLY IF EXISTS i; DROP INDEX i; + ╰╴ ━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap new file mode 100644 index 00000000..05775b45 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_policy.rs +expression: "lint_errors(sql, Rule::BanDropPolicy)" +--- +warning[ban-drop-policy]: Dropping a policy or rule may silently change behaviour for existing clients. + ╭▸ +1 │ DROP POLICY IF EXISTS p ON t; DROP RULE IF EXISTS r ON t; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-drop-policy]: Dropping a policy or rule may silently change behaviour for existing clients. + ╭▸ +1 │ DROP POLICY IF EXISTS p ON t; DROP RULE IF EXISTS r ON t; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_schema__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_schema__test__err.snap new file mode 100644 index 00000000..a185c1ac --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_schema__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_schema.rs +expression: "lint_errors(sql, Rule::BanDropSchema)" +--- +warning[ban-drop-schema]: Dropping a schema may break existing clients. + ╭▸ +1 │ DROP SCHEMA IF EXISTS s CASCADE; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_sequence__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_sequence__test__err.snap new file mode 100644 index 00000000..510c377c --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_sequence__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_sequence.rs +expression: "lint_errors(sql, Rule::BanDropSequence)" +--- +warning[ban-drop-sequence]: Dropping a sequence may break existing clients. + ╭▸ +1 │ DROP SEQUENCE IF EXISTS s CASCADE; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap new file mode 100644 index 00000000..fbf6ae32 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap @@ -0,0 +1,16 @@ +--- +source: crates/squawk_linter/src/rules/ban_replace_view_function.rs +expression: "lint_errors(sql, Rule::BanReplaceViewFunction)" +--- +warning[ban-replace-view-function]: Replacing a view, function, or procedure may silently change behaviour for existing clients. + ╭▸ +1 │ CREATE OR REPLACE VIEW v AS SELECT 1 AS id; CREATE OR REPLACE FUNCTION f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$; CREATE OR REPLAC… + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-replace-view-function]: Replacing a view, function, or procedure may silently change behaviour for existing clients. + ╭▸ +1 │ CREATE OR REPLACE VIEW v AS SELECT 1 AS id; CREATE OR REPLACE FUNCTION f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$; CREATE OR REPLAC… + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-replace-view-function]: Replacing a view, function, or procedure may silently change behaviour for existing clients. + ╭▸ +1 │ …nt LANGUAGE sql AS $$ SELECT 1 $$; CREATE OR REPLACE PROCEDURE p() LANGUAGE sql AS $$ SELECT 1 $$; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap new file mode 100644 index 00000000..a098d6a7 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_replica_identity.rs +expression: "lint_errors(sql, Rule::BanReplicaIdentity)" +--- +warning[ban-replica-identity]: Changing replica identity may silently change replication for existing clients. + ╭▸ +1 │ ALTER TABLE t REPLICA IDENTITY FULL; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap new file mode 100644 index 00000000..211d5195 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_revoke.rs +expression: "lint_errors(sql, Rule::BanRevoke)" +--- +warning[ban-revoke]: Revoking privileges may break existing clients. + ╭▸ +1 │ REVOKE SELECT ON t FROM app; ALTER DEFAULT PRIVILEGES REVOKE SELECT ON TABLES FROM app; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-revoke]: Revoking privileges may break existing clients. + ╭▸ +1 │ REVOKE SELECT ON t FROM app; ALTER DEFAULT PRIVILEGES REVOKE SELECT ON TABLES FROM app; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap new file mode 100644 index 00000000..2ee52a65 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_set_default.rs +expression: "lint_errors(sql, Rule::BanSetDefault)" +--- +warning[ban-set-default]: Setting a column default may silently change values written by existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN c SET DEFAULT 1; + ╰╴ ━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_schema__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_schema__test__err.snap new file mode 100644 index 00000000..bd430de0 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_schema__test__err.snap @@ -0,0 +1,32 @@ +--- +source: crates/squawk_linter/src/rules/ban_set_schema.rs +expression: "lint_errors(sql, Rule::BanSetSchema)" +--- +warning[ban-set-schema]: Moving an object to another schema may break existing clients. + ╭▸ +1 │ ALTER TABLE t SET SCHEMA s; ALTER VIEW v SET SCHEMA s; ALTER MATERIALIZED VIEW mv SET SCHEMA s; ALTER FUNCTION f() SET SCHEMA s; ALTER … + ╰╴ ━━━━━━━━━━━━ +warning[ban-set-schema]: Moving an object to another schema may break existing clients. + ╭▸ +1 │ ALTER TABLE t SET SCHEMA s; ALTER VIEW v SET SCHEMA s; ALTER MATERIALIZED VIEW mv SET SCHEMA s; ALTER FUNCTION f() SET SCHEMA s; ALTER … + ╰╴ ━━━━━━━━━━━━ +warning[ban-set-schema]: Moving an object to another schema may break existing clients. + ╭▸ +1 │ ALTER TABLE t SET SCHEMA s; ALTER VIEW v SET SCHEMA s; ALTER MATERIALIZED VIEW mv SET SCHEMA s; ALTER FUNCTION f() SET SCHEMA s; ALTER … + ╰╴ ━━━━━━━━━━━━ +warning[ban-set-schema]: Moving an object to another schema may break existing clients. + ╭▸ +1 │ ALTER TABLE t SET SCHEMA s; ALTER VIEW v SET SCHEMA s; ALTER MATERIALIZED VIEW mv SET SCHEMA s; ALTER FUNCTION f() SET SCHEMA s; ALTER … + ╰╴ ━━━━━━━━━━━━ +warning[ban-set-schema]: Moving an object to another schema may break existing clients. + ╭▸ +1 │ …ET SCHEMA s; ALTER FUNCTION f() SET SCHEMA s; ALTER TYPE typ SET SCHEMA s; ALTER SEQUENCE seq SET SCHEMA s; ALTER DOMAIN d SET SCHEMA … + ╰╴ ━━━━━━━━━━━━ +warning[ban-set-schema]: Moving an object to another schema may break existing clients. + ╭▸ +1 │ …ET SCHEMA s; ALTER TYPE typ SET SCHEMA s; ALTER SEQUENCE seq SET SCHEMA s; ALTER DOMAIN d SET SCHEMA s; + ╰╴ ━━━━━━━━━━━━ +warning[ban-set-schema]: Moving an object to another schema may break existing clients. + ╭▸ +1 │ …ET SCHEMA s; ALTER SEQUENCE seq SET SCHEMA s; ALTER DOMAIN d SET SCHEMA s; + ╰╴ ━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__renaming_object__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__renaming_object__test__err.snap new file mode 100644 index 00000000..349aa1c3 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__renaming_object__test__err.snap @@ -0,0 +1,44 @@ +--- +source: crates/squawk_linter/src/rules/renaming_object.rs +expression: "lint_errors(sql, Rule::RenamingObject)" +--- +warning[renaming-object]: Renaming a view may break existing clients. + ╭▸ +1 │ ALTER VIEW v RENAME TO v2; ALTER MATERIALIZED VIEW mv RENAME TO mv2; ALTER FUNCTION f() RENAME TO f2; ALTER PROCEDURE p() RENAME TO p2;… + ╰╴ ━━━━━━━━━━━━ +warning[renaming-object]: Renaming a materialized view may break existing clients. + ╭▸ +1 │ ALTER VIEW v RENAME TO v2; ALTER MATERIALIZED VIEW mv RENAME TO mv2; ALTER FUNCTION f() RENAME TO f2; ALTER PROCEDURE p() RENAME TO p2;… + ╰╴ ━━━━━━━━━━━━━ +warning[renaming-object]: Renaming a function may break existing clients. + ╭▸ +1 │ ALTER VIEW v RENAME TO v2; ALTER MATERIALIZED VIEW mv RENAME TO mv2; ALTER FUNCTION f() RENAME TO f2; ALTER PROCEDURE p() RENAME TO p2;… + ╰╴ ━━━━━━━━━━━━ +warning[renaming-object]: Renaming a procedure may break existing clients. + ╭▸ +1 │ …TO mv2; ALTER FUNCTION f() RENAME TO f2; ALTER PROCEDURE p() RENAME TO p2; ALTER TYPE typ RENAME TO typ2; ALTER SEQUENCE seq RENAME TO… + ╰╴ ━━━━━━━━━━━━ +warning[renaming-object]: Renaming a type may break existing clients. + ╭▸ +1 │ …AME TO f2; ALTER PROCEDURE p() RENAME TO p2; ALTER TYPE typ RENAME TO typ2; ALTER SEQUENCE seq RENAME TO seq2; ALTER SCHEMA s RENAME T… + ╰╴ ━━━━━━━━━━━━━━ +warning[renaming-object]: Renaming a sequence may break existing clients. + ╭▸ +1 │ …ME TO p2; ALTER TYPE typ RENAME TO typ2; ALTER SEQUENCE seq RENAME TO seq2; ALTER SCHEMA s RENAME TO s2; ALTER DOMAIN d RENAME TO d2; … + ╰╴ ━━━━━━━━━━━━━━ +warning[renaming-object]: Renaming a schema may break existing clients. + ╭▸ +1 │ …E TO typ2; ALTER SEQUENCE seq RENAME TO seq2; ALTER SCHEMA s RENAME TO s2; ALTER DOMAIN d RENAME TO d2; ALTER INDEX i RENAME TO i2; AL… + ╰╴ ━━━━━━━━━━━━ +warning[renaming-object]: Renaming a domain may break existing clients. + ╭▸ +1 │ … RENAME TO seq2; ALTER SCHEMA s RENAME TO s2; ALTER DOMAIN d RENAME TO d2; ALTER INDEX i RENAME TO i2; ALTER TYPE typ RENAME VALUE 'a'… + ╰╴ ━━━━━━━━━━━━ +warning[renaming-object]: Renaming an index may break existing clients. + ╭▸ +1 │ …A s RENAME TO s2; ALTER DOMAIN d RENAME TO d2; ALTER INDEX i RENAME TO i2; ALTER TYPE typ RENAME VALUE 'a' TO 'b'; + ╰╴ ━━━━━━━━━━━━ +warning[renaming-object]: Renaming a type value may break existing clients. + ╭▸ +1 │ …NAME TO d2; ALTER INDEX i RENAME TO i2; ALTER TYPE typ RENAME VALUE 'a' TO 'b'; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━ From 15d732b12f1eef1b9b80c3463592274bf02b6a09 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Thu, 1 Oct 2026 14:51:54 +1000 Subject: [PATCH 03/21] docs: describe new compatibility rules --- CHANGELOG.md | 2 ++ docs/docs/ban-alter-generated-expression.md | 18 ++++++++++++++++++ docs/docs/ban-alter-identity.md | 18 ++++++++++++++++++ docs/docs/ban-disable-trigger.md | 18 ++++++++++++++++++ docs/docs/ban-drop-constraint.md | 18 ++++++++++++++++++ docs/docs/ban-drop-domain.md | 16 ++++++++++++++++ docs/docs/ban-drop-index.md | 18 ++++++++++++++++++ docs/docs/ban-drop-policy.md | 18 ++++++++++++++++++ docs/docs/ban-drop-schema.md | 16 ++++++++++++++++ docs/docs/ban-drop-sequence.md | 16 ++++++++++++++++ docs/docs/ban-replace-view-function.md | 18 ++++++++++++++++++ docs/docs/ban-replica-identity.md | 18 ++++++++++++++++++ docs/docs/ban-revoke.md | 18 ++++++++++++++++++ docs/docs/ban-set-default.md | 18 ++++++++++++++++++ docs/docs/ban-set-schema.md | 16 ++++++++++++++++ docs/docs/renaming-object.md | 16 ++++++++++++++++ docs/sidebars.js | 15 +++++++++++++++ docs/src/pages/index.js | 15 +++++++++++++++ 18 files changed, 292 insertions(+) create mode 100644 docs/docs/ban-alter-generated-expression.md create mode 100644 docs/docs/ban-alter-identity.md create mode 100644 docs/docs/ban-disable-trigger.md create mode 100644 docs/docs/ban-drop-constraint.md create mode 100644 docs/docs/ban-drop-domain.md create mode 100644 docs/docs/ban-drop-index.md create mode 100644 docs/docs/ban-drop-policy.md create mode 100644 docs/docs/ban-drop-schema.md create mode 100644 docs/docs/ban-drop-sequence.md create mode 100644 docs/docs/ban-replace-view-function.md create mode 100644 docs/docs/ban-replica-identity.md create mode 100644 docs/docs/ban-revoke.md create mode 100644 docs/docs/ban-set-default.md create mode 100644 docs/docs/ban-set-schema.md create mode 100644 docs/docs/renaming-object.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c4867b5..8fa48c5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - linter: ban-drop-view, ban-drop-function, ban-drop-type, ban-drop-default rules - linter: opt-in ban-drop-trigger rule +- linter: compatibility rules for dropped schemas, sequences, and domains, object renames, and schema moves +- linter: opt-in compatibility rules for dropped constraints and indexes, identity and generated columns, defaults, triggers, replica identity, policies, privileges, and replacements ## v2.67.0 - 2026-10-04 diff --git a/docs/docs/ban-alter-generated-expression.md b/docs/docs/ban-alter-generated-expression.md new file mode 100644 index 00000000..aebd23ac --- /dev/null +++ b/docs/docs/ban-alter-generated-expression.md @@ -0,0 +1,18 @@ +--- +id: ban-alter-generated-expression +title: ban-alter-generated-expression +--- + +## problem + +A stored generated column rejects inserts that supply its value. Dropping its expression changes the column semantics. This rule is opt-in because compatibility depends on how clients use the column. + +```sql +ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; +``` + +## solution + +Update client inserts before adding or changing a generated column. + +Enable this rule with `--include ban-alter-generated-expression` (or add `ban-alter-generated-expression` to your configured include list). diff --git a/docs/docs/ban-alter-identity.md b/docs/docs/ban-alter-identity.md new file mode 100644 index 00000000..2e885a31 --- /dev/null +++ b/docs/docs/ban-alter-identity.md @@ -0,0 +1,18 @@ +--- +id: ban-alter-identity +title: ban-alter-identity +--- + +## problem + +Changing identity can make inserts with explicit or omitted values fail. This rule is opt-in because compatibility depends on how clients insert values. + +```sql +ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY; +``` + +## solution + +Update client inserts before changing the identity setting. + +Enable this rule with `--include ban-alter-identity` (or add `ban-alter-identity` to your configured include list). diff --git a/docs/docs/ban-disable-trigger.md b/docs/docs/ban-disable-trigger.md new file mode 100644 index 00000000..04b099c5 --- /dev/null +++ b/docs/docs/ban-disable-trigger.md @@ -0,0 +1,18 @@ +--- +id: ban-disable-trigger +title: ban-disable-trigger +--- + +## problem + +Disabling a trigger or rule, or changing row level security enforcement, changes behaviour without a client error. This rule is opt-in. + +```sql +ALTER TABLE t DISABLE TRIGGER trg; +``` + +## solution + +Update clients before changing trigger, rule, or row level security enforcement. + +Enable this rule with `--include ban-disable-trigger` (or add `ban-disable-trigger` to your configured include list). diff --git a/docs/docs/ban-drop-constraint.md b/docs/docs/ban-drop-constraint.md new file mode 100644 index 00000000..299c2af9 --- /dev/null +++ b/docs/docs/ban-drop-constraint.md @@ -0,0 +1,18 @@ +--- +id: ban-drop-constraint +title: ban-drop-constraint +--- + +## problem + +Dropping a constraint removes a foreign key, check, or uniqueness guarantee that clients can depend on. This rule is opt-in because compatibility depends on whether clients rely on the constraint. + +```sql +ALTER TABLE t DROP CONSTRAINT IF EXISTS c; +``` + +## solution + +Update clients to not depend on the constraint before dropping it. + +Enable this rule with `--include ban-drop-constraint` (or add `ban-drop-constraint` to your configured include list). diff --git a/docs/docs/ban-drop-domain.md b/docs/docs/ban-drop-domain.md new file mode 100644 index 00000000..01116841 --- /dev/null +++ b/docs/docs/ban-drop-domain.md @@ -0,0 +1,16 @@ +--- +id: ban-drop-domain +title: ban-drop-domain +--- + +## problem + +Clients that use a dropped domain in casts or parameters fail. + +```sql +DROP DOMAIN IF EXISTS d CASCADE; +``` + +## solution + +Move clients to a replacement domain before dropping the old domain. diff --git a/docs/docs/ban-drop-index.md b/docs/docs/ban-drop-index.md new file mode 100644 index 00000000..ad704e9f --- /dev/null +++ b/docs/docs/ban-drop-index.md @@ -0,0 +1,18 @@ +--- +id: ban-drop-index +title: ban-drop-index +--- + +## problem + +Dropping an index can remove a unique or exclusion guarantee or change query plans. This rule is opt-in. + +```sql +DROP INDEX CONCURRENTLY IF EXISTS i; +``` + +## solution + +Check client queries and constraints before dropping the index. + +Enable this rule with `--include ban-drop-index` (or add `ban-drop-index` to your configured include list). diff --git a/docs/docs/ban-drop-policy.md b/docs/docs/ban-drop-policy.md new file mode 100644 index 00000000..92ad8283 --- /dev/null +++ b/docs/docs/ban-drop-policy.md @@ -0,0 +1,18 @@ +--- +id: ban-drop-policy +title: ban-drop-policy +--- + +## problem + +Dropping a policy or rule changes access or rewrite behaviour for clients. This rule is opt-in. + +```sql +DROP POLICY IF EXISTS p ON t; +``` + +## solution + +Update clients before dropping the policy or rule. + +Enable this rule with `--include ban-drop-policy` (or add `ban-drop-policy` to your configured include list). diff --git a/docs/docs/ban-drop-schema.md b/docs/docs/ban-drop-schema.md new file mode 100644 index 00000000..70b4bc47 --- /dev/null +++ b/docs/docs/ban-drop-schema.md @@ -0,0 +1,16 @@ +--- +id: ban-drop-schema +title: ban-drop-schema +--- + +## problem + +Queries that reference objects in a dropped schema fail. + +```sql +DROP SCHEMA IF EXISTS s CASCADE; +``` + +## solution + +Move clients to a replacement schema before dropping the old schema. diff --git a/docs/docs/ban-drop-sequence.md b/docs/docs/ban-drop-sequence.md new file mode 100644 index 00000000..6ca222c3 --- /dev/null +++ b/docs/docs/ban-drop-sequence.md @@ -0,0 +1,16 @@ +--- +id: ban-drop-sequence +title: ban-drop-sequence +--- + +## problem + +Clients that call `nextval` on a dropped sequence fail. + +```sql +DROP SEQUENCE IF EXISTS s CASCADE; +``` + +## solution + +Move clients to a new sequence before dropping the old sequence. diff --git a/docs/docs/ban-replace-view-function.md b/docs/docs/ban-replace-view-function.md new file mode 100644 index 00000000..ae056558 --- /dev/null +++ b/docs/docs/ban-replace-view-function.md @@ -0,0 +1,18 @@ +--- +id: ban-replace-view-function +title: ban-replace-view-function +--- + +## problem + +`CREATE OR REPLACE` can change returned values or side effects without changing client SQL. This rule is opt-in. + +```sql +CREATE OR REPLACE VIEW v AS SELECT 1 AS id; +``` + +## solution + +Deploy compatible clients before replacing the view, function, or procedure. + +Enable this rule with `--include ban-replace-view-function` (or add `ban-replace-view-function` to your configured include list). diff --git a/docs/docs/ban-replica-identity.md b/docs/docs/ban-replica-identity.md new file mode 100644 index 00000000..6209840b --- /dev/null +++ b/docs/docs/ban-replica-identity.md @@ -0,0 +1,18 @@ +--- +id: ban-replica-identity +title: ban-replica-identity +--- + +## problem + +Changing replica identity changes the row data available to logical replication consumers. This rule is opt-in. + +```sql +ALTER TABLE t REPLICA IDENTITY FULL; +``` + +## solution + +Update replication consumers before changing replica identity. + +Enable this rule with `--include ban-replica-identity` (or add `ban-replica-identity` to your configured include list). diff --git a/docs/docs/ban-revoke.md b/docs/docs/ban-revoke.md new file mode 100644 index 00000000..c4701bc5 --- /dev/null +++ b/docs/docs/ban-revoke.md @@ -0,0 +1,18 @@ +--- +id: ban-revoke +title: ban-revoke +--- + +## problem + +Revoking privileges can make client queries fail with a permission error. This rule is opt-in. + +```sql +REVOKE SELECT ON t FROM app; +``` + +## solution + +Move clients to a role with the required privileges before revoking them. + +Enable this rule with `--include ban-revoke` (or add `ban-revoke` to your configured include list). diff --git a/docs/docs/ban-set-default.md b/docs/docs/ban-set-default.md new file mode 100644 index 00000000..5634eea6 --- /dev/null +++ b/docs/docs/ban-set-default.md @@ -0,0 +1,18 @@ +--- +id: ban-set-default +title: ban-set-default +--- + +## problem + +Inserts that omit a column write different values after its default changes. This rule is opt-in. + +```sql +ALTER TABLE t ALTER COLUMN c SET DEFAULT 1; +``` + +## solution + +Update clients to supply explicit values before changing the default. + +Enable this rule with `--include ban-set-default` (or add `ban-set-default` to your configured include list). diff --git a/docs/docs/ban-set-schema.md b/docs/docs/ban-set-schema.md new file mode 100644 index 00000000..34a3c24d --- /dev/null +++ b/docs/docs/ban-set-schema.md @@ -0,0 +1,16 @@ +--- +id: ban-set-schema +title: ban-set-schema +--- + +## problem + +Clients that use schema-qualified names cannot find objects after `SET SCHEMA`. + +```sql +ALTER TABLE t SET SCHEMA s; +``` + +## solution + +Update clients to use the new schema before moving the object. diff --git a/docs/docs/renaming-object.md b/docs/docs/renaming-object.md new file mode 100644 index 00000000..00daca40 --- /dev/null +++ b/docs/docs/renaming-object.md @@ -0,0 +1,16 @@ +--- +id: renaming-object +title: renaming-object +--- + +## problem + +Clients that use an old object name or enum value can fail after a rename. + +```sql +ALTER VIEW v RENAME TO v2; +``` + +## solution + +Update clients to use the new name before renaming the object. diff --git a/docs/sidebars.js b/docs/sidebars.js index 679fca6b..ffe6bea1 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -48,6 +48,21 @@ module.exports = { "require-concurrent-reindex", "prefer-repack", "ban-duplicate-column-assignments", + "ban-drop-schema", + "ban-drop-sequence", + "ban-drop-domain", + "ban-drop-constraint", + "renaming-object", + "ban-set-schema", + "ban-alter-identity", + "ban-alter-generated-expression", + "ban-drop-index", + "ban-set-default", + "ban-disable-trigger", + "ban-replica-identity", + "ban-drop-policy", + "ban-revoke", + "ban-replace-view-function", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index ca30535f..f4a1e53e 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -267,6 +267,21 @@ const rules = [ tags: ["backwards compatibility"], description: "Prevent silent changes when a trigger is dropped (opt-in).", }, + { name: "ban-drop-schema", tags: ["backwards compatibility"], description: "Prevent breaking clients that use a dropped schema." }, + { name: "ban-drop-sequence", tags: ["backwards compatibility"], description: "Prevent breaking clients that use a dropped sequence." }, + { name: "ban-drop-domain", tags: ["backwards compatibility"], description: "Prevent breaking clients that use a dropped domain." }, + { name: "ban-drop-constraint", tags: ["backwards compatibility"], description: "Prevent removing a constraint guarantee (opt-in)." }, + { name: "renaming-object", tags: ["backwards compatibility"], description: "Prevent renaming objects used by clients." }, + { name: "ban-set-schema", tags: ["backwards compatibility"], description: "Prevent moving objects used by clients to another schema." }, + { name: "ban-alter-identity", tags: ["backwards compatibility"], description: "Prevent changing identity columns used by clients." }, + { name: "ban-alter-generated-expression", tags: ["backwards compatibility"], description: "Prevent breaking inserts with generated columns (opt-in)." }, + { name: "ban-drop-index", tags: ["backwards compatibility"], description: "Prevent dropping indexes used by clients (opt-in)." }, + { name: "ban-set-default", tags: ["backwards compatibility"], description: "Prevent silent changes to column defaults (opt-in)." }, + { name: "ban-disable-trigger", tags: ["backwards compatibility"], description: "Prevent changes to triggers, rules, and row level security (opt-in)." }, + { name: "ban-replica-identity", tags: ["backwards compatibility"], description: "Prevent changes to replica identity (opt-in)." }, + { name: "ban-drop-policy", tags: ["backwards compatibility"], description: "Prevent dropping policies and rules (opt-in)." }, + { name: "ban-revoke", tags: ["backwards compatibility"], description: "Prevent revoking client privileges (opt-in)." }, + { name: "ban-replace-view-function", tags: ["backwards compatibility"], description: "Prevent replacing views and routines (opt-in)." }, // xtask:new-rule:rule-doc-meta ] From 5311eab922c8d6298ceb97a7c94e00bd5c57a32c Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:32:19 +1100 Subject: [PATCH 04/21] feat(linter): enable constraint and identity checks by default and split generated drop --- crates/squawk_linter/src/lib.rs | 17 ++----- .../rules/ban_drop_generated_expression.rs | 49 +++++++++++++++++++ crates/squawk_linter/src/rules/mod.rs | 2 + docs/docs/ban-alter-identity.md | 4 +- docs/docs/ban-drop-constraint.md | 4 +- docs/docs/ban-drop-generated-expression.md | 20 ++++++++ docs/sidebars.js | 1 + docs/src/pages/index.js | 3 +- 8 files changed, 83 insertions(+), 17 deletions(-) create mode 100644 crates/squawk_linter/src/rules/ban_drop_generated_expression.rs create mode 100644 docs/docs/ban-drop-generated-expression.md diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 0b2e7df8..63f59433 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -127,6 +127,7 @@ pub enum Rule { BanDropSequence, BanDropDomain, BanDropConstraint, + BanDropGeneratedExpression, RenamingObject, BanSetSchema, BanAlterIdentity, @@ -151,8 +152,6 @@ impl Rule { Rule::RequireTableSchema | Rule::RequireTimeoutSettings | Rule::BanDropTrigger - | Rule::BanDropConstraint - | Rule::BanAlterIdentity | Rule::BanAlterGeneratedExpression | Rule::BanDropIndex | Rule::BanSetDefault @@ -232,6 +231,7 @@ impl TryFrom<&str> for Rule { "ban-drop-sequence" => Ok(Rule::BanDropSequence), "ban-drop-domain" => Ok(Rule::BanDropDomain), "ban-drop-constraint" => Ok(Rule::BanDropConstraint), + "ban-drop-generated-expression" => Ok(Rule::BanDropGeneratedExpression), "renaming-object" => Ok(Rule::RenamingObject), "ban-set-schema" => Ok(Rule::BanSetSchema), "ban-alter-identity" => Ok(Rule::BanAlterIdentity), @@ -323,6 +323,7 @@ impl fmt::Display for Rule { Rule::BanDropSequence => "ban-drop-sequence", Rule::BanDropDomain => "ban-drop-domain", Rule::BanDropConstraint => "ban-drop-constraint", + Rule::BanDropGeneratedExpression => "ban-drop-generated-expression", Rule::RenamingObject => "renaming-object", Rule::BanSetSchema => "ban-set-schema", Rule::BanAlterIdentity => "ban-alter-identity", @@ -612,6 +613,7 @@ impl Linter { (Rule::BanDropSequence, ban_drop_sequence), (Rule::BanDropDomain, ban_drop_domain), (Rule::BanDropConstraint, ban_drop_constraint), + (Rule::BanDropGeneratedExpression, ban_drop_generated_expression), (Rule::RenamingObject, renaming_object), (Rule::BanSetSchema, ban_set_schema), (Rule::BanAlterIdentity, ban_alter_identity), @@ -727,8 +729,6 @@ mod tests { assert!(!linter.rules.contains(&Rule::RequireTableSchema)); assert!(!linter.rules.contains(&Rule::BanDropTrigger)); for rule in [ - Rule::BanDropConstraint, - Rule::BanAlterIdentity, Rule::BanAlterGeneratedExpression, Rule::BanDropIndex, Rule::BanSetDefault, @@ -745,14 +745,9 @@ mod tests { #[test] fn new_opt_in_rules_only_report_when_included() { for (rule, sql) in [ - (Rule::BanDropConstraint, "ALTER TABLE t DROP CONSTRAINT c;"), - ( - Rule::BanAlterIdentity, - "ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY;", - ), ( Rule::BanAlterGeneratedExpression, - "ALTER TABLE t ALTER COLUMN c DROP EXPRESSION;", + "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1);", ), ] { let parse = SourceFile::parse(sql); @@ -777,8 +772,6 @@ mod tests { for rule in [ Rule::RequireTableSchema, Rule::BanDropTrigger, - Rule::BanDropConstraint, - Rule::BanAlterIdentity, Rule::BanAlterGeneratedExpression, Rule::BanDropIndex, Rule::BanSetDefault, diff --git a/crates/squawk_linter/src/rules/ban_drop_generated_expression.rs b/crates/squawk_linter/src/rules/ban_drop_generated_expression.rs new file mode 100644 index 00000000..5558c171 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_generated_expression.rs @@ -0,0 +1,49 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_generated_expression(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + if let ast::AlterTableAction::AlterColumn(column) = action + && let Some(ast::AlterColumnOption::DropExpression(node)) = column.option() + { + ctx.report(Violation::for_node( + Rule::BanDropGeneratedExpression, + "Dropping a generated expression changes the values returned to existing clients." + .into(), + node.syntax(), + )); + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + + #[test] + fn drop_expression() { + let errors = lint_errors( + "ALTER TABLE t ALTER COLUMN c DROP EXPRESSION;", + Rule::BanDropGeneratedExpression, + ); + assert!(errors.contains("ban-drop-generated-expression"), "{errors}"); + } + + #[test] + fn other_generated_operations() { + lint_ok( + "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (a + b);", + Rule::BanDropGeneratedExpression, + ); + } +} diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index 03381b44..8dd08b2e 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_create_domain_with_constraint; pub(crate) mod ban_disable_trigger; pub(crate) mod ban_drop_column; pub(crate) mod ban_drop_constraint; +pub(crate) mod ban_drop_generated_expression; pub(crate) mod ban_drop_database; pub(crate) mod ban_drop_default; pub(crate) mod ban_drop_domain; @@ -71,6 +72,7 @@ pub(crate) use ban_create_domain_with_constraint::ban_create_domain_with_constra pub(crate) use ban_disable_trigger::ban_disable_trigger; pub(crate) use ban_drop_column::ban_drop_column; pub(crate) use ban_drop_constraint::ban_drop_constraint; +pub(crate) use ban_drop_generated_expression::ban_drop_generated_expression; pub(crate) use ban_drop_database::ban_drop_database; pub(crate) use ban_drop_default::ban_drop_default; pub(crate) use ban_drop_domain::ban_drop_domain; diff --git a/docs/docs/ban-alter-identity.md b/docs/docs/ban-alter-identity.md index 2e885a31..7a709cc2 100644 --- a/docs/docs/ban-alter-identity.md +++ b/docs/docs/ban-alter-identity.md @@ -5,7 +5,7 @@ title: ban-alter-identity ## problem -Changing identity can make inserts with explicit or omitted values fail. This rule is opt-in because compatibility depends on how clients insert values. +Changing identity can make inserts with explicit or omitted values fail. This rule is enabled by default. Check how the old application inserts values before changing identity. ```sql ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY; @@ -15,4 +15,4 @@ ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY; Update client inserts before changing the identity setting. -Enable this rule with `--include ban-alter-identity` (or add `ban-alter-identity` to your configured include list). +Exclude this rule with `--exclude ban-alter-identity` after checking application compatibility. diff --git a/docs/docs/ban-drop-constraint.md b/docs/docs/ban-drop-constraint.md index 299c2af9..56b7632e 100644 --- a/docs/docs/ban-drop-constraint.md +++ b/docs/docs/ban-drop-constraint.md @@ -5,7 +5,7 @@ title: ban-drop-constraint ## problem -Dropping a constraint removes a foreign key, check, or uniqueness guarantee that clients can depend on. This rule is opt-in because compatibility depends on whether clients rely on the constraint. +Dropping a constraint removes a foreign key, check, or uniqueness guarantee that clients can depend on. If an old application uses `INSERT ... ON CONFLICT ON CONSTRAINT c` or infers a dropped unique constraint as its conflict arbiter, its inserts fail immediately. This rule is enabled by default. ```sql ALTER TABLE t DROP CONSTRAINT IF EXISTS c; @@ -15,4 +15,4 @@ ALTER TABLE t DROP CONSTRAINT IF EXISTS c; Update clients to not depend on the constraint before dropping it. -Enable this rule with `--include ban-drop-constraint` (or add `ban-drop-constraint` to your configured include list). +Exclude this rule with `--exclude ban-drop-constraint` after checking application compatibility. diff --git a/docs/docs/ban-drop-generated-expression.md b/docs/docs/ban-drop-generated-expression.md new file mode 100644 index 00000000..5fe733f9 --- /dev/null +++ b/docs/docs/ban-drop-generated-expression.md @@ -0,0 +1,20 @@ +--- +id: ban-drop-generated-expression +title: ban-drop-generated-expression +--- + +## What it does + +Detects `ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION` by default. + +## Why + +Dropping an expression changes a generated column into an ordinary column. The old application can read values with different semantics after rollback. Review how the old application reads and writes this column before removing the expression. + +Adding a generated column and replacing an expression have different risks. The opt-in `ban-alter-generated-expression` rule covers those operations. + +## Example + +```sql +ALTER TABLE line_items ALTER COLUMN total DROP EXPRESSION; +``` diff --git a/docs/sidebars.js b/docs/sidebars.js index ffe6bea1..8b831787 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -52,6 +52,7 @@ module.exports = { "ban-drop-sequence", "ban-drop-domain", "ban-drop-constraint", + "ban-drop-generated-expression", "renaming-object", "ban-set-schema", "ban-alter-identity", diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index f4a1e53e..e197562c 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -270,7 +270,8 @@ const rules = [ { name: "ban-drop-schema", tags: ["backwards compatibility"], description: "Prevent breaking clients that use a dropped schema." }, { name: "ban-drop-sequence", tags: ["backwards compatibility"], description: "Prevent breaking clients that use a dropped sequence." }, { name: "ban-drop-domain", tags: ["backwards compatibility"], description: "Prevent breaking clients that use a dropped domain." }, - { name: "ban-drop-constraint", tags: ["backwards compatibility"], description: "Prevent removing a constraint guarantee (opt-in)." }, + { name: "ban-drop-constraint", tags: ["backwards compatibility"], description: "Prevent removing a constraint guarantee." }, + { name: "ban-drop-generated-expression", tags: ["backwards compatibility"], description: "Prevent dropping a generated expression." }, { name: "renaming-object", tags: ["backwards compatibility"], description: "Prevent renaming objects used by clients." }, { name: "ban-set-schema", tags: ["backwards compatibility"], description: "Prevent moving objects used by clients to another schema." }, { name: "ban-alter-identity", tags: ["backwards compatibility"], description: "Prevent changing identity columns used by clients." }, From be665ca4456bf5c3ee8808fe84562590eebf646d Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:33:30 +1100 Subject: [PATCH 05/21] feat(linter): add security compatibility rules --- CHANGELOG.md | 1 + crates/squawk_linter/src/lib.rs | 54 +++- crates/squawk_linter/src/rules/mod.rs | 2 + .../src/rules/security_compatibility.rs | 296 ++++++++++++++++++ docs/docs/ban-alter-database-options.md | 13 + docs/docs/ban-alter-function-options.md | 12 + docs/docs/ban-alter-policy-condition.md | 12 + docs/docs/ban-alter-policy-roles.md | 12 + docs/docs/ban-alter-role-options.md | 13 + docs/docs/ban-alter-row-level-security.md | 12 + docs/docs/ban-alter-view-options.md | 12 + docs/docs/ban-create-policy.md | 12 + docs/docs/ban-drop-extension.md | 12 + docs/sidebars.js | 9 + docs/src/pages/index.js | 9 + 15 files changed, 480 insertions(+), 1 deletion(-) create mode 100644 crates/squawk_linter/src/rules/security_compatibility.rs create mode 100644 docs/docs/ban-alter-database-options.md create mode 100644 docs/docs/ban-alter-function-options.md create mode 100644 docs/docs/ban-alter-policy-condition.md create mode 100644 docs/docs/ban-alter-policy-roles.md create mode 100644 docs/docs/ban-alter-role-options.md create mode 100644 docs/docs/ban-alter-row-level-security.md create mode 100644 docs/docs/ban-alter-view-options.md create mode 100644 docs/docs/ban-create-policy.md create mode 100644 docs/docs/ban-drop-extension.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fa48c5b..62335d65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - linter: opt-in ban-drop-trigger rule - linter: compatibility rules for dropped schemas, sequences, and domains, object renames, and schema moves - linter: opt-in compatibility rules for dropped constraints and indexes, identity and generated columns, defaults, triggers, replica identity, policies, privileges, and replacements +- linter: default ban-drop-extension and opt-in rules for policy creation, policy conditions and roles, function and view options, role and database options, and row level security ## v2.67.0 - 2026-10-04 diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 63f59433..05c090ce 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -72,7 +72,7 @@ use rules::{ ban_alter_generated_expression, ban_alter_identity, ban_disable_trigger, ban_drop_constraint, ban_drop_domain, ban_drop_index, ban_drop_policy, ban_drop_schema, ban_drop_sequence, ban_replace_view_function, ban_replica_identity, ban_revoke, ban_set_default, ban_set_schema, - renaming_object, + renaming_object, security_compatibility, }; // xtask:new-rule:rule-import @@ -139,6 +139,15 @@ pub enum Rule { BanDropPolicy, BanRevoke, BanReplaceViewFunction, + BanDropExtension, + BanAlterPolicyCondition, + BanAlterPolicyRoles, + BanCreatePolicy, + BanAlterFunctionOptions, + BanAlterViewOptions, + BanAlterRoleOptions, + BanAlterDatabaseOptions, + BanAlterRowLevelSecurity, // xtask:new-rule:error-name } @@ -160,6 +169,14 @@ impl Rule { | Rule::BanDropPolicy | Rule::BanRevoke | Rule::BanReplaceViewFunction + | Rule::BanAlterPolicyCondition + | Rule::BanAlterPolicyRoles + | Rule::BanCreatePolicy + | Rule::BanAlterFunctionOptions + | Rule::BanAlterViewOptions + | Rule::BanAlterRoleOptions + | Rule::BanAlterDatabaseOptions + | Rule::BanAlterRowLevelSecurity ) } @@ -243,6 +260,15 @@ impl TryFrom<&str> for Rule { "ban-drop-policy" => Ok(Rule::BanDropPolicy), "ban-revoke" => Ok(Rule::BanRevoke), "ban-replace-view-function" => Ok(Rule::BanReplaceViewFunction), + "ban-drop-extension" => Ok(Rule::BanDropExtension), + "ban-alter-policy-condition" => Ok(Rule::BanAlterPolicyCondition), + "ban-alter-policy-roles" => Ok(Rule::BanAlterPolicyRoles), + "ban-create-policy" => Ok(Rule::BanCreatePolicy), + "ban-alter-function-options" => Ok(Rule::BanAlterFunctionOptions), + "ban-alter-view-options" => Ok(Rule::BanAlterViewOptions), + "ban-alter-role-options" => Ok(Rule::BanAlterRoleOptions), + "ban-alter-database-options" => Ok(Rule::BanAlterDatabaseOptions), + "ban-alter-row-level-security" => Ok(Rule::BanAlterRowLevelSecurity), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -335,6 +361,15 @@ impl fmt::Display for Rule { Rule::BanDropPolicy => "ban-drop-policy", Rule::BanRevoke => "ban-revoke", Rule::BanReplaceViewFunction => "ban-replace-view-function", + Rule::BanDropExtension => "ban-drop-extension", + Rule::BanAlterPolicyCondition => "ban-alter-policy-condition", + Rule::BanAlterPolicyRoles => "ban-alter-policy-roles", + Rule::BanCreatePolicy => "ban-create-policy", + Rule::BanAlterFunctionOptions => "ban-alter-function-options", + Rule::BanAlterViewOptions => "ban-alter-view-options", + Rule::BanAlterRoleOptions => "ban-alter-role-options", + Rule::BanAlterDatabaseOptions => "ban-alter-database-options", + Rule::BanAlterRowLevelSecurity => "ban-alter-row-level-security", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -633,6 +668,7 @@ impl Linter { check(self, file); } } + security_compatibility(self, file); // xtask:new-rule:rule-call // locate any ignores in the file @@ -737,6 +773,14 @@ mod tests { Rule::BanDropPolicy, Rule::BanRevoke, Rule::BanReplaceViewFunction, + Rule::BanAlterPolicyCondition, + Rule::BanAlterPolicyRoles, + Rule::BanCreatePolicy, + Rule::BanAlterFunctionOptions, + Rule::BanAlterViewOptions, + Rule::BanAlterRoleOptions, + Rule::BanAlterDatabaseOptions, + Rule::BanAlterRowLevelSecurity, ] { assert!(!linter.rules.contains(&rule)); } @@ -780,6 +824,14 @@ mod tests { Rule::BanDropPolicy, Rule::BanRevoke, Rule::BanReplaceViewFunction, + Rule::BanAlterPolicyCondition, + Rule::BanAlterPolicyRoles, + Rule::BanCreatePolicy, + Rule::BanAlterFunctionOptions, + Rule::BanAlterViewOptions, + Rule::BanAlterRoleOptions, + Rule::BanAlterDatabaseOptions, + Rule::BanAlterRowLevelSecurity, ] { let linter = Linter::with_rules(&[rule], &[]); assert!(linter.rules.contains(&rule)); diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index 8dd08b2e..bdc9d1c5 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -56,6 +56,7 @@ pub(crate) mod require_enum_value_ordering; pub(crate) mod require_table_schema; pub(crate) mod require_timeout_settings; pub(crate) mod transaction_nesting; +pub(crate) mod security_compatibility; // xtask:new-rule:mod-decl pub(crate) use adding_field_with_default::adding_field_with_default; @@ -116,4 +117,5 @@ pub(crate) use require_enum_value_ordering::require_enum_value_ordering; pub(crate) use require_table_schema::require_table_schema; pub(crate) use require_timeout_settings::require_timeout_settings; pub(crate) use transaction_nesting::transaction_nesting; +pub(crate) use security_compatibility::security_compatibility; // xtask:new-rule:export diff --git a/crates/squawk_linter/src/rules/security_compatibility.rs b/crates/squawk_linter/src/rules/security_compatibility.rs new file mode 100644 index 00000000..701afab3 --- /dev/null +++ b/crates/squawk_linter/src/rules/security_compatibility.rs @@ -0,0 +1,296 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn security_compatibility(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::DropExtension(node) => report( + ctx, + Rule::BanDropExtension, + "Dropping an extension removes objects used by existing clients.", + node.syntax(), + ), + ast::Stmt::CreatePolicy(node) => report( + ctx, + Rule::BanCreatePolicy, + "Creating a policy may change access for existing clients.", + node.syntax(), + ), + ast::Stmt::AlterPolicy(node) => { + if let Some(ast::AlterPolicyAction::AlterPolicyTo(action)) = node.action() { + if let Some(roles) = action.policy_roles() { + report( + ctx, + Rule::BanAlterPolicyRoles, + "Changing policy roles may change access for existing clients.", + roles.syntax(), + ); + } + if let Some(using) = action.using_expr_clause() { + report( + ctx, + Rule::BanAlterPolicyCondition, + "Changing a policy condition may change access for existing clients.", + using.syntax(), + ); + } + if let Some(check) = action.with_check_expr_clause() { + report( + ctx, + Rule::BanAlterPolicyCondition, + "Changing a policy condition may change access for existing clients.", + check.syntax(), + ); + } + } + } + ast::Stmt::AlterFunction(node) => { + if let Some(ast::AlterFunctionAction::FuncOptionList(options)) = node.action() { + report( + ctx, + Rule::BanAlterFunctionOptions, + "Changing function options may change behaviour for existing clients.", + options.syntax(), + ); + } + } + ast::Stmt::AlterView(node) => { + if let Some(action) = node.action() { + match action { + ast::AlterViewAction::SetOptions(options) => report( + ctx, + Rule::BanAlterViewOptions, + "Changing view options may change behaviour for existing clients.", + options.syntax(), + ), + ast::AlterViewAction::ResetOptions(options) => report( + ctx, + Rule::BanAlterViewOptions, + "Changing view options may change behaviour for existing clients.", + options.syntax(), + ), + _ => {} + } + } + } + ast::Stmt::AlterRole(node) => { + if let Some(action) = node.action() { + match action { + ast::AlterRoleAction::RoleOptionList(options) => report( + ctx, + Rule::BanAlterRoleOptions, + "Changing role options may change access for existing clients.", + options.syntax(), + ), + ast::AlterRoleAction::SetConfigParam(config) => report( + ctx, + Rule::BanAlterRoleOptions, + "Changing role configuration may change behaviour for existing clients.", + config.syntax(), + ), + ast::AlterRoleAction::ResetConfigParam(config) => report( + ctx, + Rule::BanAlterRoleOptions, + "Changing role configuration may change behaviour for existing clients.", + config.syntax(), + ), + _ => {} + } + } + } + ast::Stmt::AlterDatabase(node) => { + if let Some(action) = node.action() { + match action { + ast::AlterDatabaseAction::DatabaseOptionList(options) => report( + ctx, + Rule::BanAlterDatabaseOptions, + "Changing database options may change behaviour for existing clients.", + options.syntax(), + ), + ast::AlterDatabaseAction::SetConfigParam(config) => report( + ctx, + Rule::BanAlterDatabaseOptions, + "Changing database configuration may change behaviour for existing clients.", + config.syntax(), + ), + ast::AlterDatabaseAction::ResetConfigParam(config) => report( + ctx, + Rule::BanAlterDatabaseOptions, + "Changing database configuration may change behaviour for existing clients.", + config.syntax(), + ), + _ => {} + } + } + } + ast::Stmt::AlterTable(node) => { + for action in node.actions() { + match action { + ast::AlterTableAction::EnableRls(value) => report( + ctx, + Rule::BanAlterRowLevelSecurity, + "Changing row level security may change access for existing clients.", + value.syntax(), + ), + ast::AlterTableAction::DisableRls(value) => report( + ctx, + Rule::BanAlterRowLevelSecurity, + "Changing row level security may change access for existing clients.", + value.syntax(), + ), + ast::AlterTableAction::ForceRls(value) => report( + ctx, + Rule::BanAlterRowLevelSecurity, + "Changing row level security may change access for existing clients.", + value.syntax(), + ), + ast::AlterTableAction::NoForceRls(value) => report( + ctx, + Rule::BanAlterRowLevelSecurity, + "Changing row level security may change access for existing clients.", + value.syntax(), + ), + _ => {} + } + } + } + _ => {} + } + } +} + +fn report(ctx: &mut Linter, rule: Rule, message: &str, node: &squawk_syntax::SyntaxNode) { + if ctx.rules.contains(&rule) { + ctx.report(Violation::for_node(rule, message.into(), node)); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn security_rules_are_targeted_and_configurable() { + let cases = [ + ( + Rule::BanDropExtension, + "DROP EXTENSION IF EXISTS hstore, citext;", + "CREATE EXTENSION hstore;", + ), + ( + Rule::BanCreatePolicy, + "CREATE POLICY p ON t USING (true);", + "DROP POLICY p ON t;", + ), + ( + Rule::BanAlterPolicyRoles, + "ALTER POLICY p ON t TO admin;", + "ALTER POLICY p ON t USING (true);", + ), + ( + Rule::BanAlterPolicyCondition, + "ALTER POLICY p ON t USING (true) WITH CHECK (false);", + "ALTER POLICY p ON t TO admin;", + ), + ( + Rule::BanAlterFunctionOptions, + "ALTER FUNCTION f() SECURITY DEFINER;", + "ALTER FUNCTION f() RENAME TO g;", + ), + ( + Rule::BanAlterViewOptions, + "ALTER VIEW v SET (security_barrier = true);", + "ALTER VIEW v RENAME TO w;", + ), + ( + Rule::BanAlterRoleOptions, + "ALTER ROLE r NOLOGIN;", + "ALTER ROLE r RENAME TO s;", + ), + ( + Rule::BanAlterDatabaseOptions, + "ALTER DATABASE d SET search_path TO public;", + "ALTER DATABASE d RENAME TO e;", + ), + ( + Rule::BanAlterRowLevelSecurity, + "ALTER TABLE t ENABLE ROW LEVEL SECURITY;", + "ALTER TABLE t ADD COLUMN c int;", + ), + ]; + for (rule, bad, good) in cases { + assert_eq!(Rule::try_from(rule.to_string().as_str()), Ok(rule)); + let parse = SourceFile::parse(bad); + assert!(parse.errors().is_empty(), "{bad}: {:?}", parse.errors()); + assert!( + Linter::from([rule]) + .lint(&parse, bad) + .iter() + .any(|v| v.code == rule), + "{bad}" + ); + if rule != Rule::BanDropExtension { + assert!( + !Linter::with_default_rules() + .lint(&parse, bad) + .iter() + .any(|v| v.code == rule), + "{bad}" + ); + } + let parse = SourceFile::parse(good); + assert!(parse.errors().is_empty(), "{good}: {:?}", parse.errors()); + assert!( + !Linter::from([rule]) + .lint(&parse, good) + .iter() + .any(|v| v.code == rule), + "{good}" + ); + } + } + + #[test] + fn policy_and_configuration_variants() { + for (rule, sql) in [ + ( + Rule::BanAlterPolicyCondition, + "ALTER POLICY p ON t WITH CHECK (true);", + ), + ( + Rule::BanAlterViewOptions, + "ALTER VIEW v RESET (security_barrier);", + ), + ( + Rule::BanAlterRoleOptions, + "ALTER ROLE r SET search_path TO public;", + ), + (Rule::BanAlterRoleOptions, "ALTER ROLE r RESET search_path;"), + ( + Rule::BanAlterDatabaseOptions, + "ALTER DATABASE d WITH CONNECTION LIMIT 5;", + ), + ( + Rule::BanAlterDatabaseOptions, + "ALTER DATABASE d RESET search_path;", + ), + ( + Rule::BanAlterRowLevelSecurity, + "ALTER TABLE t NO FORCE ROW LEVEL SECURITY;", + ), + ] { + let parse = SourceFile::parse(sql); + assert!(parse.errors().is_empty(), "{sql}: {:?}", parse.errors()); + assert!( + Linter::from([rule]) + .lint(&parse, sql) + .iter() + .any(|v| v.code == rule), + "{sql}" + ); + } + } +} diff --git a/docs/docs/ban-alter-database-options.md b/docs/docs/ban-alter-database-options.md new file mode 100644 index 00000000..e522e000 --- /dev/null +++ b/docs/docs/ban-alter-database-options.md @@ -0,0 +1,13 @@ +--- +id: ban-alter-database-options +title: ban-alter-database-options +--- + +`ALTER DATABASE` options and configuration changes can change behaviour for all clients in the database. This rule is opt-in. It does not report database renames, owner changes, or tablespace changes. + +```sql +ALTER DATABASE app SET search_path TO public; +ALTER DATABASE app WITH CONNECTION LIMIT 20; +``` + +Review clients before changing database settings. Enable this rule with `--include ban-alter-database-options`. diff --git a/docs/docs/ban-alter-function-options.md b/docs/docs/ban-alter-function-options.md new file mode 100644 index 00000000..f5e84c4c --- /dev/null +++ b/docs/docs/ban-alter-function-options.md @@ -0,0 +1,12 @@ +--- +id: ban-alter-function-options +title: ban-alter-function-options +--- + +`ALTER FUNCTION` options can change execution behaviour or security for existing callers. This rule is opt-in. It does not report renames, ownership changes, or schema moves. + +```sql +ALTER FUNCTION f() SECURITY DEFINER; +``` + +Review callers before changing the options. Enable this rule with `--include ban-alter-function-options`. diff --git a/docs/docs/ban-alter-policy-condition.md b/docs/docs/ban-alter-policy-condition.md new file mode 100644 index 00000000..af1e2d07 --- /dev/null +++ b/docs/docs/ban-alter-policy-condition.md @@ -0,0 +1,12 @@ +--- +id: ban-alter-policy-condition +title: ban-alter-policy-condition +--- + +`ALTER POLICY` with `USING` or `WITH CHECK` changes which rows clients can read or write. This rule is opt-in. It does not report policy renames or role-only changes. + +```sql +ALTER POLICY p ON accounts USING (owner_id = current_user_id()); +``` + +Review the condition and update clients before applying it. Enable this rule with `--include ban-alter-policy-condition`. diff --git a/docs/docs/ban-alter-policy-roles.md b/docs/docs/ban-alter-policy-roles.md new file mode 100644 index 00000000..1932c738 --- /dev/null +++ b/docs/docs/ban-alter-policy-roles.md @@ -0,0 +1,12 @@ +--- +id: ban-alter-policy-roles +title: ban-alter-policy-roles +--- + +`ALTER POLICY ... TO` changes which roles the policy applies to. This can change client access. This rule is opt-in. It does not report policy renames or condition-only changes. + +```sql +ALTER POLICY p ON accounts TO app_user; +``` + +Review the affected roles before applying the change. Enable this rule with `--include ban-alter-policy-roles`. diff --git a/docs/docs/ban-alter-role-options.md b/docs/docs/ban-alter-role-options.md new file mode 100644 index 00000000..e7dc46d5 --- /dev/null +++ b/docs/docs/ban-alter-role-options.md @@ -0,0 +1,13 @@ +--- +id: ban-alter-role-options +title: ban-alter-role-options +--- + +`ALTER ROLE` options and configuration changes can change access or behaviour for clients that use the role. This rule is opt-in. It does not report role renames. + +```sql +ALTER ROLE app_user NOLOGIN; +ALTER ROLE app_user SET search_path TO public; +``` + +Review clients before changing the role. Enable this rule with `--include ban-alter-role-options`. diff --git a/docs/docs/ban-alter-row-level-security.md b/docs/docs/ban-alter-row-level-security.md new file mode 100644 index 00000000..181231f8 --- /dev/null +++ b/docs/docs/ban-alter-row-level-security.md @@ -0,0 +1,12 @@ +--- +id: ban-alter-row-level-security +title: ban-alter-row-level-security +--- + +`ALTER TABLE` with `ENABLE`, `DISABLE`, `FORCE`, or `NO FORCE ROW LEVEL SECURITY` changes access for existing clients. This rule is opt-in. + +```sql +ALTER TABLE accounts ENABLE ROW LEVEL SECURITY; +``` + +Review table policies and client access before applying the change. Enable this rule with `--include ban-alter-row-level-security`. diff --git a/docs/docs/ban-alter-view-options.md b/docs/docs/ban-alter-view-options.md new file mode 100644 index 00000000..c5647150 --- /dev/null +++ b/docs/docs/ban-alter-view-options.md @@ -0,0 +1,12 @@ +--- +id: ban-alter-view-options +title: ban-alter-view-options +--- + +`ALTER VIEW ... SET` and `ALTER VIEW ... RESET` change view options such as `security_barrier` or `security_invoker`. This rule is opt-in. It does not report view renames or column default changes. + +```sql +ALTER VIEW v SET (security_invoker = true); +``` + +Review client access before changing view options. Enable this rule with `--include ban-alter-view-options`. diff --git a/docs/docs/ban-create-policy.md b/docs/docs/ban-create-policy.md new file mode 100644 index 00000000..617bd176 --- /dev/null +++ b/docs/docs/ban-create-policy.md @@ -0,0 +1,12 @@ +--- +id: ban-create-policy +title: ban-create-policy +--- + +`CREATE POLICY` can change access for clients when row level security is enabled. Review the policy command, roles, and conditions before applying it. This rule is opt-in. + +```sql +CREATE POLICY p ON accounts TO app_user USING (owner_id = current_user_id()); +``` + +Enable this rule with `--include ban-create-policy`. diff --git a/docs/docs/ban-drop-extension.md b/docs/docs/ban-drop-extension.md new file mode 100644 index 00000000..dd24c2ab --- /dev/null +++ b/docs/docs/ban-drop-extension.md @@ -0,0 +1,12 @@ +--- +id: ban-drop-extension +title: ban-drop-extension +--- + +`DROP EXTENSION` removes the extension and its objects. Existing clients can depend on those objects. This rule is enabled by default. + +```sql +DROP EXTENSION IF EXISTS hstore; +``` + +Update clients before dropping the extension. To disable this rule, use `--exclude ban-drop-extension`. diff --git a/docs/sidebars.js b/docs/sidebars.js index 8b831787..fd4abde5 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -64,6 +64,15 @@ module.exports = { "ban-drop-policy", "ban-revoke", "ban-replace-view-function", + "ban-drop-extension", + "ban-create-policy", + "ban-alter-policy-condition", + "ban-alter-policy-roles", + "ban-alter-function-options", + "ban-alter-view-options", + "ban-alter-role-options", + "ban-alter-database-options", + "ban-alter-row-level-security", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index e197562c..ebc4d5da 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -283,6 +283,15 @@ const rules = [ { name: "ban-drop-policy", tags: ["backwards compatibility"], description: "Prevent dropping policies and rules (opt-in)." }, { name: "ban-revoke", tags: ["backwards compatibility"], description: "Prevent revoking client privileges (opt-in)." }, { name: "ban-replace-view-function", tags: ["backwards compatibility"], description: "Prevent replacing views and routines (opt-in)." }, + { name: "ban-drop-extension", tags: ["backwards compatibility"], description: "Prevent dropping extensions used by clients." }, + { name: "ban-create-policy", tags: ["backwards compatibility"], description: "Review new policy access rules (opt-in)." }, + { name: "ban-alter-policy-condition", tags: ["backwards compatibility"], description: "Review policy condition changes (opt-in)." }, + { name: "ban-alter-policy-roles", tags: ["backwards compatibility"], description: "Review policy role changes (opt-in)." }, + { name: "ban-alter-function-options", tags: ["backwards compatibility"], description: "Review function option changes (opt-in)." }, + { name: "ban-alter-view-options", tags: ["backwards compatibility"], description: "Review view option changes (opt-in)." }, + { name: "ban-alter-role-options", tags: ["backwards compatibility"], description: "Review role option and configuration changes (opt-in)." }, + { name: "ban-alter-database-options", tags: ["backwards compatibility"], description: "Review database option and configuration changes (opt-in)." }, + { name: "ban-alter-row-level-security", tags: ["backwards compatibility"], description: "Review row level security changes (opt-in)." }, // xtask:new-rule:rule-doc-meta ] From d14d4397acd5a7b7bf6ad48586bdcdf62ae59de0 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:34:31 +1100 Subject: [PATCH 06/21] fix(linter): import generated expression rule and test opt-in addition --- crates/squawk_linter/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 05c090ce..189aad91 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -70,7 +70,7 @@ use rules::require_timeout_settings; use rules::transaction_nesting; use rules::{ ban_alter_generated_expression, ban_alter_identity, ban_disable_trigger, ban_drop_constraint, - ban_drop_domain, ban_drop_index, ban_drop_policy, ban_drop_schema, ban_drop_sequence, + ban_drop_domain, ban_drop_generated_expression, ban_drop_index, ban_drop_policy, ban_drop_schema, ban_drop_sequence, ban_replace_view_function, ban_replica_identity, ban_revoke, ban_set_default, ban_set_schema, renaming_object, security_compatibility, }; @@ -791,7 +791,7 @@ mod tests { for (rule, sql) in [ ( Rule::BanAlterGeneratedExpression, - "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1);", + "ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;", ), ] { let parse = SourceFile::parse(sql); From 2724cfa80288e44c22e118f929377a8d1e3f9698 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:33:51 +1100 Subject: [PATCH 07/21] feat(linter): cover existing rule statement variants --- .../src/rules/adding_not_null_field.rs | 22 ++++ .../rules/ban_alter_generated_expression.rs | 9 +- .../src/rules/ban_disable_trigger.rs | 5 + .../src/rules/ban_drop_default.rs | 49 ++++++-- .../src/rules/ban_drop_function.rs | 5 + .../src/rules/ban_drop_not_null.rs | 36 ++++-- .../squawk_linter/src/rules/ban_drop_table.rs | 6 + .../squawk_linter/src/rules/ban_drop_type.rs | 8 +- .../src/rules/ban_replace_view_function.rs | 23 ++++ crates/squawk_linter/src/rules/ban_revoke.rs | 10 ++ .../src/rules/ban_set_default.rs | 29 ++++- .../squawk_linter/src/rules/ban_set_schema.rs | 18 +++ crates/squawk_linter/src/rules/mod.rs | 2 + .../src/rules/renaming_column.rs | 42 ++++++- .../src/rules/renaming_object.rs | 20 ++++ .../squawk_linter/src/rules/variant_tests.rs | 105 ++++++++++++++++++ docs/docs/adding-not-nullable-field.md | 2 +- docs/docs/ban-alter-generated-expression.md | 6 +- docs/docs/ban-disable-trigger.md | 2 +- docs/docs/ban-drop-default.md | 2 +- docs/docs/ban-drop-function.md | 2 +- docs/docs/ban-drop-not-null.md | 2 +- docs/docs/ban-drop-table.md | 2 +- docs/docs/ban-drop-type.md | 2 +- docs/docs/ban-replace-view-function.md | 4 +- docs/docs/ban-revoke.md | 2 +- docs/docs/ban-set-default.md | 2 +- docs/docs/ban-set-schema.md | 2 +- docs/docs/renaming-column.md | 2 +- docs/docs/renaming-object.md | 2 +- 30 files changed, 366 insertions(+), 57 deletions(-) create mode 100644 crates/squawk_linter/src/rules/variant_tests.rs diff --git a/crates/squawk_linter/src/rules/adding_not_null_field.rs b/crates/squawk_linter/src/rules/adding_not_null_field.rs index ad49120a..97aa379b 100644 --- a/crates/squawk_linter/src/rules/adding_not_null_field.rs +++ b/crates/squawk_linter/src/rules/adding_not_null_field.rs @@ -57,6 +57,28 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse) let mut tables_with_external_validated_constraints: FxHashSet = FxHashSet::default(); for stmt in file.stmts() { + if let ast::Stmt::AlterDomain(domain) = &stmt { + if let Some(ast::AlterDomainAction::SetNotNull(node)) = domain.action() { + ctx.report(Violation::for_node( + Rule::AddingNotNullableField, + "Setting a domain `NOT NULL` validates existing values.".into(), + node.syntax(), + )); + } + } + if let ast::Stmt::AlterForeignTable(table) = &stmt { + for action in table.actions() { + if let ast::AlterTableAction::AlterColumn(column) = action { + if let Some(ast::AlterColumnOption::SetNotNull(node)) = column.option() { + ctx.report(Violation::for_node( + Rule::AddingNotNullableField, + "Setting a column `NOT NULL` may block reads during validation.".into(), + node.syntax(), + )); + } + } + } + } if let ast::Stmt::AlterTable(alter_table) = stmt { let Some(table) = get_table_name(&alter_table) else { continue; diff --git a/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs index 1c937646..7a49f85b 100644 --- a/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs +++ b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs @@ -10,15 +10,14 @@ pub(crate) fn ban_alter_generated_expression(ctx: &mut Linter, parse: &Parse { - if let Some(ast::AlterColumnOption::DropExpression(node)) = column.option() - { + if let Some(ast::AlterColumnOption::SetExpression(node)) = column.option() { ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Changing a generated column may break inserts from existing clients.".into(), node.syntax())); } } ast::AlterTableAction::AddColumn(column) => { for constraint in column.constraints() { if let ast::Constraint::GeneratedConstraint(node) = constraint { - ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Changing a generated column may break inserts from existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Adding a generated column may break inserts from existing clients.".into(), node.syntax())); } } } @@ -38,13 +37,13 @@ mod test { use insta::assert_snapshot; #[test] fn err() { - let sql = "ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;"; + let sql = "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 2); ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;"; assert_snapshot!(lint_errors(sql, Rule::BanAlterGeneratedExpression)); } #[test] fn ok() { lint_ok( - "ALTER TABLE t ADD COLUMN c int;", + "ALTER TABLE t ADD COLUMN c int; ALTER TABLE t ALTER COLUMN c DROP EXPRESSION;", Rule::BanAlterGeneratedExpression, ); } diff --git a/crates/squawk_linter/src/rules/ban_disable_trigger.rs b/crates/squawk_linter/src/rules/ban_disable_trigger.rs index 55a90722..f6e98c1f 100644 --- a/crates/squawk_linter/src/rules/ban_disable_trigger.rs +++ b/crates/squawk_linter/src/rules/ban_disable_trigger.rs @@ -14,6 +14,11 @@ pub(crate) fn ban_disable_trigger(ctx: &mut Linter, parse: &Parse) { | ast::AlterTableAction::DisableRule(_) | ast::AlterTableAction::DisableRls(_) | ast::AlterTableAction::ForceRls(_) + | ast::AlterTableAction::NoForceRls(_) + | ast::AlterTableAction::EnableReplicaTrigger(_) + | ast::AlterTableAction::EnableReplicaRule(_) + | ast::AlterTableAction::EnableAlwaysTrigger(_) + | ast::AlterTableAction::EnableAlwaysRule(_) ) { ctx.report(Violation::for_node(Rule::BanDisableTrigger, "Disabling a trigger, rule, or row level security may silently change behaviour for existing clients.".into(), action.syntax())); } diff --git a/crates/squawk_linter/src/rules/ban_drop_default.rs b/crates/squawk_linter/src/rules/ban_drop_default.rs index b43165db..a4a77d21 100644 --- a/crates/squawk_linter/src/rules/ban_drop_default.rs +++ b/crates/squawk_linter/src/rules/ban_drop_default.rs @@ -7,18 +7,43 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_default(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { - if let ast::Stmt::AlterTable(alter_table) = stmt { - for action in alter_table.actions() { - if let ast::AlterTableAction::AlterColumn(alter_column) = action { - if let Some(ast::AlterColumnOption::DropDefault(drop_default)) = - alter_column.option() - { - ctx.report(Violation::for_node( - Rule::BanDropDefault, - "Dropping a column default may break existing clients.".into(), - drop_default.syntax(), - )); - } + if let ast::Stmt::AlterDomain(domain) = &stmt { + if let Some(ast::AlterDomainAction::DropDefault(node)) = domain.action() { + ctx.report(Violation::for_node( + Rule::BanDropDefault, + "Dropping a column default may break existing clients.".into(), + node.syntax(), + )); + } + } + if let ast::Stmt::AlterView(view) = &stmt { + if let Some(ast::AlterViewAction::AlterViewColumn(column)) = view.action() { + if let Some(ast::AlterViewColumnAction::DropDefault(node)) = + column.alter_view_column_action() + { + ctx.report(Violation::for_node( + Rule::BanDropDefault, + "Dropping a column default may break existing clients.".into(), + node.syntax(), + )); + } + } + } + let actions = match stmt { + ast::Stmt::AlterTable(table) => table.actions(), + ast::Stmt::AlterForeignTable(table) => table.actions(), + _ => continue, + }; + for action in actions { + if let ast::AlterTableAction::AlterColumn(alter_column) = action { + if let Some(ast::AlterColumnOption::DropDefault(drop_default)) = + alter_column.option() + { + ctx.report(Violation::for_node( + Rule::BanDropDefault, + "Dropping a column default may break existing clients.".into(), + drop_default.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/ban_drop_function.rs b/crates/squawk_linter/src/rules/ban_drop_function.rs index 2191708e..f870667a 100644 --- a/crates/squawk_linter/src/rules/ban_drop_function.rs +++ b/crates/squawk_linter/src/rules/ban_drop_function.rs @@ -18,6 +18,11 @@ pub(crate) fn ban_drop_function(ctx: &mut Linter, parse: &Parse) { "Dropping a function may break existing clients.".into(), node.syntax(), )), + ast::Stmt::DropRoutine(node) => ctx.report(Violation::for_node( + Rule::BanDropFunction, + "Dropping a routine may break existing clients.".into(), + node.syntax(), + )), _ => (), } } diff --git a/crates/squawk_linter/src/rules/ban_drop_not_null.rs b/crates/squawk_linter/src/rules/ban_drop_not_null.rs index 07274442..ea582d85 100644 --- a/crates/squawk_linter/src/rules/ban_drop_not_null.rs +++ b/crates/squawk_linter/src/rules/ban_drop_not_null.rs @@ -8,18 +8,30 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_not_null(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { - if let ast::Stmt::AlterTable(alter_table) = stmt { - for action in alter_table.actions() { - if let ast::AlterTableAction::AlterColumn(alter_column) = action { - if let Some(ast::AlterColumnOption::DropNotNull(drop_not_null)) = - alter_column.option() - { - ctx.report(Violation::for_node( - Rule::BanDropNotNull, - "Dropping a `NOT NULL` constraint may break existing clients.".into(), - drop_not_null.syntax(), - )); - } + if let ast::Stmt::AlterDomain(domain) = &stmt { + if let Some(ast::AlterDomainAction::DropNotNull(node)) = domain.action() { + ctx.report(Violation::for_node( + Rule::BanDropNotNull, + "Dropping a `NOT NULL` constraint may break existing clients.".into(), + node.syntax(), + )); + } + } + let actions = match stmt { + ast::Stmt::AlterTable(table) => table.actions(), + ast::Stmt::AlterForeignTable(table) => table.actions(), + _ => continue, + }; + for action in actions { + if let ast::AlterTableAction::AlterColumn(alter_column) = action { + if let Some(ast::AlterColumnOption::DropNotNull(drop_not_null)) = + alter_column.option() + { + ctx.report(Violation::for_node( + Rule::BanDropNotNull, + "Dropping a `NOT NULL` constraint may break existing clients.".into(), + drop_not_null.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/ban_drop_table.rs b/crates/squawk_linter/src/rules/ban_drop_table.rs index 250e3ea5..ac453ace 100644 --- a/crates/squawk_linter/src/rules/ban_drop_table.rs +++ b/crates/squawk_linter/src/rules/ban_drop_table.rs @@ -14,6 +14,12 @@ pub(crate) fn ban_drop_table(ctx: &mut Linter, parse: &Parse) { "Dropping a table may break existing clients.".into(), drop_table.syntax(), )); + } else if let ast::Stmt::DropForeignTable(drop_table) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropTable, + "Dropping a table may break existing clients.".into(), + drop_table.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/ban_drop_type.rs b/crates/squawk_linter/src/rules/ban_drop_type.rs index f5283da1..65273ef9 100644 --- a/crates/squawk_linter/src/rules/ban_drop_type.rs +++ b/crates/squawk_linter/src/rules/ban_drop_type.rs @@ -7,7 +7,13 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_type(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { - if let ast::Stmt::DropType(node) = stmt { + if let ast::Stmt::DropCast(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropType, + "Dropping a cast may break existing clients.".into(), + node.syntax(), + )); + } else if let ast::Stmt::DropType(node) = stmt { ctx.report(Violation::for_node( Rule::BanDropType, "Dropping a type may break existing clients.".into(), diff --git a/crates/squawk_linter/src/rules/ban_replace_view_function.rs b/crates/squawk_linter/src/rules/ban_replace_view_function.rs index c8301583..64a3917c 100644 --- a/crates/squawk_linter/src/rules/ban_replace_view_function.rs +++ b/crates/squawk_linter/src/rules/ban_replace_view_function.rs @@ -16,6 +16,29 @@ pub(crate) fn ban_replace_view_function(ctx: &mut Linter, parse: &Parse { ctx.report(Violation::for_node(Rule::BanReplaceViewFunction, "Replacing a view, function, or procedure may silently change behaviour for existing clients.".into(), node.syntax())); } + ast::Stmt::CreateTrigger(node) if node.or_replace().is_some() => { + ctx.report(Violation::for_node( + Rule::BanReplaceViewFunction, + "Replacing a trigger may silently change behaviour for existing clients." + .into(), + node.syntax(), + )); + } + ast::Stmt::CreateRule(node) if node.or_replace().is_some() => { + ctx.report(Violation::for_node( + Rule::BanReplaceViewFunction, + "Replacing a rule may silently change behaviour for existing clients.".into(), + node.syntax(), + )); + } + ast::Stmt::CreateAggregate(node) if node.or_replace().is_some() => { + ctx.report(Violation::for_node( + Rule::BanReplaceViewFunction, + "Replacing an aggregate may silently change behaviour for existing clients." + .into(), + node.syntax(), + )); + } _ => (), } } diff --git a/crates/squawk_linter/src/rules/ban_revoke.rs b/crates/squawk_linter/src/rules/ban_revoke.rs index bf2cec06..80981da8 100644 --- a/crates/squawk_linter/src/rules/ban_revoke.rs +++ b/crates/squawk_linter/src/rules/ban_revoke.rs @@ -14,6 +14,16 @@ pub(crate) fn ban_revoke(ctx: &mut Linter, parse: &Parse) { node.syntax(), )); } + ast::Stmt::DropOwned(node) => ctx.report(Violation::for_node( + Rule::BanRevoke, + "Dropping owned objects or privileges may break existing clients.".into(), + node.syntax(), + )), + ast::Stmt::DropRole(node) => ctx.report(Violation::for_node( + Rule::BanRevoke, + "Dropping a role may break existing clients.".into(), + node.syntax(), + )), ast::Stmt::AlterDefaultPrivileges(node) => { if matches!( node.action(), diff --git a/crates/squawk_linter/src/rules/ban_set_default.rs b/crates/squawk_linter/src/rules/ban_set_default.rs index bf85aa25..2017b357 100644 --- a/crates/squawk_linter/src/rules/ban_set_default.rs +++ b/crates/squawk_linter/src/rules/ban_set_default.rs @@ -6,12 +6,29 @@ use squawk_syntax::{ pub(crate) fn ban_set_default(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { - if let ast::Stmt::AlterTable(table) = stmt { - for action in table.actions() { - if let ast::AlterTableAction::AlterColumn(column) = action { - if let Some(ast::AlterColumnOption::SetDefault(node)) = column.option() { - ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); - } + if let ast::Stmt::AlterDomain(domain) = &stmt { + if let Some(ast::AlterDomainAction::SetDefault(node)) = domain.action() { + ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); + } + } + if let ast::Stmt::AlterView(view) = &stmt { + if let Some(ast::AlterViewAction::AlterViewColumn(column)) = view.action() { + if let Some(ast::AlterViewColumnAction::SetDefault(node)) = + column.alter_view_column_action() + { + ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); + } + } + } + let actions = match stmt { + ast::Stmt::AlterTable(table) => table.actions(), + ast::Stmt::AlterForeignTable(table) => table.actions(), + _ => continue, + }; + for action in actions { + if let ast::AlterTableAction::AlterColumn(column) = action { + if let Some(ast::AlterColumnOption::SetDefault(node)) = column.option() { + ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); } } } diff --git a/crates/squawk_linter/src/rules/ban_set_schema.rs b/crates/squawk_linter/src/rules/ban_set_schema.rs index 60bdbd58..90365a0e 100644 --- a/crates/squawk_linter/src/rules/ban_set_schema.rs +++ b/crates/squawk_linter/src/rules/ban_set_schema.rs @@ -40,6 +40,24 @@ pub(crate) fn ban_set_schema(ctx: &mut Linter, parse: &Parse) { } } } + ast::Stmt::AlterProcedure(node) => { + if let Some(ast::AlterProcedureAction::SetSchema(action)) = node.action() { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + action.syntax(), + )); + } + } + ast::Stmt::AlterRoutine(node) => { + if let Some(ast::AlterRoutineAction::SetSchema(action)) = node.action() { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + action.syntax(), + )); + } + } ast::Stmt::AlterFunction(node) => { for action in node.action().into_iter() { if let ast::AlterFunctionAction::SetSchema(node) = action { diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index bdc9d1c5..e63d9183 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -57,6 +57,8 @@ pub(crate) mod require_table_schema; pub(crate) mod require_timeout_settings; pub(crate) mod transaction_nesting; pub(crate) mod security_compatibility; +#[cfg(test)] +mod variant_tests; // xtask:new-rule:mod-decl pub(crate) use adding_field_with_default::adding_field_with_default; diff --git a/crates/squawk_linter/src/rules/renaming_column.rs b/crates/squawk_linter/src/rules/renaming_column.rs index 0445f697..6ece1b4a 100644 --- a/crates/squawk_linter/src/rules/renaming_column.rs +++ b/crates/squawk_linter/src/rules/renaming_column.rs @@ -8,16 +8,50 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn renaming_column(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { - if let ast::Stmt::AlterTable(alter_table) = stmt { - for action in alter_table.actions() { - if let ast::AlterTableAction::RenameColumn(rename_column) = action { + match stmt { + ast::Stmt::AlterTable(table) => { + for action in table.actions() { + if let ast::AlterTableAction::RenameColumn(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingColumn, + "Renaming a column may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterForeignTable(table) => { + for action in table.actions() { + if let ast::AlterTableAction::RenameColumn(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingColumn, + "Renaming a column may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterView(view) => { + if let Some(ast::AlterViewAction::RenameColumn(node)) = view.action() { ctx.report(Violation::for_node( Rule::RenamingColumn, "Renaming a column may break existing clients.".into(), - rename_column.syntax(), + node.syntax(), )); } } + ast::Stmt::AlterMaterializedView(view) => { + for action in view.action() { + if let ast::AlterMaterializedViewAction::RenameColumn(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingColumn, + "Renaming a column may break existing clients.".into(), + node.syntax(), + )); + } + } + } + _ => (), } } } diff --git a/crates/squawk_linter/src/rules/renaming_object.rs b/crates/squawk_linter/src/rules/renaming_object.rs index a3bafef4..22ff1afc 100644 --- a/crates/squawk_linter/src/rules/renaming_object.rs +++ b/crates/squawk_linter/src/rules/renaming_object.rs @@ -7,6 +7,26 @@ use squawk_syntax::{ pub(crate) fn renaming_object(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { match stmt { + ast::Stmt::AlterForeignTable(node) => { + for action in node.actions() { + if let ast::AlterTableAction::TableRenameTo(node) = action { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a foreign table may break existing clients.".into(), + node.syntax(), + )); + } + } + } + ast::Stmt::AlterRoutine(node) => { + if let Some(ast::AlterRoutineAction::RoutineRenameTo(action)) = node.action() { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a routine may break existing clients.".into(), + action.syntax(), + )); + } + } ast::Stmt::AlterView(node) => { for action in node.action().into_iter() { if let ast::AlterViewAction::ViewRenameTo(node) = action { diff --git a/crates/squawk_linter/src/rules/variant_tests.rs b/crates/squawk_linter/src/rules/variant_tests.rs new file mode 100644 index 00000000..6662e231 --- /dev/null +++ b/crates/squawk_linter/src/rules/variant_tests.rs @@ -0,0 +1,105 @@ +use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, +}; + +fn check(sql: &str, rule: Rule, count: usize) { + let errors = lint_errors(sql, rule); + assert_eq!(errors.matches("warning[").count(), count, "{errors}"); +} + +#[test] +fn renames() { + check( + "ALTER FOREIGN TABLE ft RENAME TO ft2; ALTER ROUTINE f() RENAME TO g;", + Rule::RenamingObject, + 2, + ); + check( + "ALTER FOREIGN TABLE ft RENAME COLUMN a TO b; ALTER VIEW v RENAME COLUMN a TO b; ALTER MATERIALIZED VIEW mv RENAME COLUMN a TO b;", + Rule::RenamingColumn, + 3, + ); + lint_ok("ALTER VIEW v RENAME TO v2;", Rule::RenamingColumn); +} + +#[test] +fn defaults_and_nullability() { + check( + "ALTER DOMAIN d DROP NOT NULL; ALTER FOREIGN TABLE ft ALTER COLUMN c DROP NOT NULL;", + Rule::BanDropNotNull, + 2, + ); + check( + "ALTER DOMAIN d SET NOT NULL; ALTER FOREIGN TABLE ft ALTER COLUMN c SET NOT NULL;", + Rule::AddingNotNullableField, + 2, + ); + check( + "ALTER DOMAIN d DROP DEFAULT; ALTER VIEW v ALTER COLUMN c DROP DEFAULT; ALTER FOREIGN TABLE ft ALTER COLUMN c DROP DEFAULT;", + Rule::BanDropDefault, + 3, + ); + check( + "ALTER DOMAIN d SET DEFAULT 1; ALTER VIEW v ALTER COLUMN c SET DEFAULT 1; ALTER FOREIGN TABLE ft ALTER COLUMN c SET DEFAULT 1;", + Rule::BanSetDefault, + 3, + ); + lint_ok("ALTER DOMAIN d SET DEFAULT 1;", Rule::BanDropDefault); + lint_ok("ALTER DOMAIN d DROP DEFAULT;", Rule::BanSetDefault); +} + +#[test] +fn drops_and_revokes() { + check("DROP FOREIGN TABLE IF EXISTS ft;", Rule::BanDropTable, 1); + check("DROP ROUTINE IF EXISTS f(int);", Rule::BanDropFunction, 1); + check("DROP CAST IF EXISTS (text AS int);", Rule::BanDropType, 1); + check("DROP OWNED BY app; DROP ROLE app;", Rule::BanRevoke, 2); + lint_ok("REASSIGN OWNED BY app TO admin;", Rule::BanRevoke); +} + +#[test] +fn replacements() { + check( + "CREATE OR REPLACE RULE r AS ON INSERT TO t DO INSTEAD NOTHING; CREATE OR REPLACE AGGREGATE a (int) (SFUNC = int4pl, STYPE = int); CREATE OR REPLACE TRIGGER tr BEFORE INSERT ON t FOR EACH ROW EXECUTE FUNCTION f();", + Rule::BanReplaceViewFunction, + 3, + ); + lint_ok( + "CREATE RULE r AS ON INSERT TO t DO INSTEAD NOTHING;", + Rule::BanReplaceViewFunction, + ); +} + +#[test] +fn generated_expression() { + check( + "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1);", + Rule::BanAlterGeneratedExpression, + 1, + ); + lint_ok( + "ALTER TABLE t ALTER COLUMN c DROP EXPRESSION;", + Rule::BanAlterGeneratedExpression, + ); +} + +#[test] +fn enforcement_and_policy() { + check( + "ALTER TABLE t ENABLE REPLICA TRIGGER tr; ALTER TABLE t ENABLE REPLICA RULE r; ALTER TABLE t ENABLE ALWAYS TRIGGER tr; ALTER TABLE t ENABLE ALWAYS RULE r; ALTER TABLE t NO FORCE ROW LEVEL SECURITY;", + Rule::BanDisableTrigger, + 5, + ); + lint_ok("ALTER POLICY p ON t USING (true);", Rule::BanDropPolicy); + lint_ok("ALTER TABLE t ENABLE TRIGGER tr;", Rule::BanDisableTrigger); +} + +#[test] +fn schema_moves() { + check( + "ALTER PROCEDURE p() SET SCHEMA s; ALTER ROUTINE f() SET SCHEMA s;", + Rule::BanSetSchema, + 2, + ); +} diff --git a/docs/docs/adding-not-nullable-field.md b/docs/docs/adding-not-nullable-field.md index b9cf8b4e..e7b5eaf6 100644 --- a/docs/docs/adding-not-nullable-field.md +++ b/docs/docs/adding-not-nullable-field.md @@ -10,7 +10,7 @@ Use a check constraint instead of setting a column as `NOT NULL`. Adding a column as `NOT NULL` is no longer covered by this rule. See ["adding-required-field (set a non-volatile default)"](adding-required-field.md#set-a-non-volatile-default) for more information on how to add new columns with `NOT NULL`. -Modifying a column to be `NOT NULL` may fail if the column contains records with a `NULL` value, requiring a full table scan to check before executing. Old application code may also try to write `NULL` values to the table. +Modifying a table or foreign table column, or a domain, to be `NOT NULL` may fail if existing values contain `NULL`. Validation can require a scan. Old application code may also try to write `NULL` values to the table. `ALTER TABLE` also requires an `ACCESS EXCLUSIVE` lock which will disable reads and writes while this statement is running. diff --git a/docs/docs/ban-alter-generated-expression.md b/docs/docs/ban-alter-generated-expression.md index aebd23ac..062c0174 100644 --- a/docs/docs/ban-alter-generated-expression.md +++ b/docs/docs/ban-alter-generated-expression.md @@ -5,14 +5,14 @@ title: ban-alter-generated-expression ## problem -A stored generated column rejects inserts that supply its value. Dropping its expression changes the column semantics. This rule is opt-in because compatibility depends on how clients use the column. +Adding a generated column can reject inserts that supply its value. `SET EXPRESSION` changes the values returned for existing rows when PostgreSQL rewrites the table. This rule is opt-in because compatibility depends on how clients use the column. ```sql -ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; +ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1); ``` ## solution -Update client inserts before adding or changing a generated column. +Update client inserts before adding a generated column. Check client reads before changing its expression. `DROP EXPRESSION` is not checked by this rule. Enable this rule with `--include ban-alter-generated-expression` (or add `ban-alter-generated-expression` to your configured include list). diff --git a/docs/docs/ban-disable-trigger.md b/docs/docs/ban-disable-trigger.md index 04b099c5..199b08db 100644 --- a/docs/docs/ban-disable-trigger.md +++ b/docs/docs/ban-disable-trigger.md @@ -5,7 +5,7 @@ title: ban-disable-trigger ## problem -Disabling a trigger or rule, or changing row level security enforcement, changes behaviour without a client error. This rule is opt-in. +Disabling a trigger or rule, enabling a replica or always trigger or rule, or changing row level security enforcement (including `NO FORCE ROW LEVEL SECURITY`) can change behaviour without a client error. This rule is opt-in. ```sql ALTER TABLE t DISABLE TRIGGER trg; diff --git a/docs/docs/ban-drop-default.md b/docs/docs/ban-drop-default.md index df349f6c..5f531d60 100644 --- a/docs/docs/ban-drop-default.md +++ b/docs/docs/ban-drop-default.md @@ -5,7 +5,7 @@ title: ban-drop-default ## problem -Dropping a column default may break existing clients. Inserts that omit a `NOT NULL` column fail with `23502 not_null_violation`. Inserts that omit a nullable column silently write `NULL`. +Dropping a table, foreign table, view column, or domain default may break existing clients. Inserts that omit a `NOT NULL` column can fail with `23502 not_null_violation`. Inserts that omit a nullable column can write `NULL`. ## solution diff --git a/docs/docs/ban-drop-function.md b/docs/docs/ban-drop-function.md index e104bf1e..e2aae09e 100644 --- a/docs/docs/ban-drop-function.md +++ b/docs/docs/ban-drop-function.md @@ -5,7 +5,7 @@ title: ban-drop-function ## problem -Dropping a function or procedure may break existing clients. Calls can fail with `42883 undefined_function`. +Dropping a function, procedure, or routine may break existing clients. Calls can fail with `42883 undefined_function`. ## solution diff --git a/docs/docs/ban-drop-not-null.md b/docs/docs/ban-drop-not-null.md index 8c288357..702a1205 100644 --- a/docs/docs/ban-drop-not-null.md +++ b/docs/docs/ban-drop-not-null.md @@ -5,7 +5,7 @@ title: ban-drop-not-null ## problem -Dropping a NOT NULL constraint may break existing clients. +Dropping a `NOT NULL` constraint on a table column, foreign table column, or domain may break existing clients. Application code or code written in procedural languages like PL/SQL or PL/pgSQL may not expect NULL values for the column that was previously guaranteed to be NOT NULL and therefore may fail to process them correctly. diff --git a/docs/docs/ban-drop-table.md b/docs/docs/ban-drop-table.md index 2328f0ef..9cd1e497 100644 --- a/docs/docs/ban-drop-table.md +++ b/docs/docs/ban-drop-table.md @@ -5,7 +5,7 @@ title: ban-drop-table ## problem -Dropping a table may break existing clients. +Dropping a table or foreign table may break existing clients. ## solution diff --git a/docs/docs/ban-drop-type.md b/docs/docs/ban-drop-type.md index 5ce4f673..f5b9201e 100644 --- a/docs/docs/ban-drop-type.md +++ b/docs/docs/ban-drop-type.md @@ -5,7 +5,7 @@ title: ban-drop-type ## problem -Dropping a type may break existing clients. Casts and parameters that name the type can fail with `42704 undefined_object`. `CASCADE` can also drop columns of that type. +Dropping a type or cast may break existing clients. Casts and parameters that name the type can fail with `42704 undefined_object`. `CASCADE` can also drop columns of that type. ## solution diff --git a/docs/docs/ban-replace-view-function.md b/docs/docs/ban-replace-view-function.md index ae056558..e59f5e51 100644 --- a/docs/docs/ban-replace-view-function.md +++ b/docs/docs/ban-replace-view-function.md @@ -5,7 +5,7 @@ title: ban-replace-view-function ## problem -`CREATE OR REPLACE` can change returned values or side effects without changing client SQL. This rule is opt-in. +`CREATE OR REPLACE` on a view, function, procedure, trigger, rule, or aggregate can change returned values or side effects without changing client SQL. This rule is opt-in. ```sql CREATE OR REPLACE VIEW v AS SELECT 1 AS id; @@ -13,6 +13,6 @@ CREATE OR REPLACE VIEW v AS SELECT 1 AS id; ## solution -Deploy compatible clients before replacing the view, function, or procedure. +Deploy compatible clients before replacing the view, function, procedure, trigger, rule, or aggregate. Enable this rule with `--include ban-replace-view-function` (or add `ban-replace-view-function` to your configured include list). diff --git a/docs/docs/ban-revoke.md b/docs/docs/ban-revoke.md index c4701bc5..35e88203 100644 --- a/docs/docs/ban-revoke.md +++ b/docs/docs/ban-revoke.md @@ -5,7 +5,7 @@ title: ban-revoke ## problem -Revoking privileges can make client queries fail with a permission error. This rule is opt-in. +Revoking privileges, dropping a role, or running `DROP OWNED` can make client queries fail. `DROP OWNED` can also remove objects owned by the role. This rule is opt-in. ```sql REVOKE SELECT ON t FROM app; diff --git a/docs/docs/ban-set-default.md b/docs/docs/ban-set-default.md index 5634eea6..7ff72de9 100644 --- a/docs/docs/ban-set-default.md +++ b/docs/docs/ban-set-default.md @@ -5,7 +5,7 @@ title: ban-set-default ## problem -Inserts that omit a column write different values after its default changes. This rule is opt-in. +Inserts that omit a table, foreign table, view column, or domain value can write different values after its default changes. This rule is opt-in. ```sql ALTER TABLE t ALTER COLUMN c SET DEFAULT 1; diff --git a/docs/docs/ban-set-schema.md b/docs/docs/ban-set-schema.md index 34a3c24d..537284e7 100644 --- a/docs/docs/ban-set-schema.md +++ b/docs/docs/ban-set-schema.md @@ -5,7 +5,7 @@ title: ban-set-schema ## problem -Clients that use schema-qualified names cannot find objects after `SET SCHEMA`. +Clients that use schema-qualified names cannot find objects after `SET SCHEMA`. This includes procedures and routines. ```sql ALTER TABLE t SET SCHEMA s; diff --git a/docs/docs/renaming-column.md b/docs/docs/renaming-column.md index 797eeff6..c9cc4851 100644 --- a/docs/docs/renaming-column.md +++ b/docs/docs/renaming-column.md @@ -5,7 +5,7 @@ title: renaming-column ## problem -Renaming a column may break existing clients. +Renaming a table, foreign table, view, or materialized view column may break existing clients. ## solution diff --git a/docs/docs/renaming-object.md b/docs/docs/renaming-object.md index 00daca40..50b4f344 100644 --- a/docs/docs/renaming-object.md +++ b/docs/docs/renaming-object.md @@ -5,7 +5,7 @@ title: renaming-object ## problem -Clients that use an old object name or enum value can fail after a rename. +Clients that use an old object name or enum value can fail after a rename. This includes foreign tables and routines. ```sql ALTER VIEW v RENAME TO v2; From 53cc8cc327fca21b5deb71a11ecbb19f87870c7a Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:33:43 +1100 Subject: [PATCH 08/21] feat: add opt-in compatibility rules for data changes --- CHANGELOG.md | 1 + crates/squawk_linter/src/lib.rs | 34 ++- .../src/rules/compatibility_additions.rs | 271 ++++++++++++++++++ crates/squawk_linter/src/rules/mod.rs | 2 + docs/docs/ban-add-composite-attribute.md | 18 ++ docs/docs/ban-add-enum-value.md | 18 ++ docs/docs/ban-alter-sequence-values.md | 18 ++ docs/docs/ban-detach-inheritance.md | 18 ++ docs/docs/ban-new-write-restriction.md | 18 ++ docs/sidebars.js | 5 + docs/src/pages/index.js | 5 + 11 files changed, 407 insertions(+), 1 deletion(-) create mode 100644 crates/squawk_linter/src/rules/compatibility_additions.rs create mode 100644 docs/docs/ban-add-composite-attribute.md create mode 100644 docs/docs/ban-add-enum-value.md create mode 100644 docs/docs/ban-alter-sequence-values.md create mode 100644 docs/docs/ban-detach-inheritance.md create mode 100644 docs/docs/ban-new-write-restriction.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 62335d65..9e00a167 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- linter: opt-in compatibility rules for new write restrictions, enum values, composite attributes, partition detachment, inheritance removal, and sequence changes - linter: ban-drop-view, ban-drop-function, ban-drop-type, ban-drop-default rules - linter: opt-in ban-drop-trigger rule - linter: compatibility rules for dropped schemas, sequences, and domains, object renames, and schema moves diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 189aad91..186b457a 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -72,7 +72,7 @@ use rules::{ ban_alter_generated_expression, ban_alter_identity, ban_disable_trigger, ban_drop_constraint, ban_drop_domain, ban_drop_generated_expression, ban_drop_index, ban_drop_policy, ban_drop_schema, ban_drop_sequence, ban_replace_view_function, ban_replica_identity, ban_revoke, ban_set_default, ban_set_schema, - renaming_object, security_compatibility, + compatibility_additions, renaming_object, security_compatibility, }; // xtask:new-rule:rule-import @@ -148,6 +148,11 @@ pub enum Rule { BanAlterRoleOptions, BanAlterDatabaseOptions, BanAlterRowLevelSecurity, + BanNewWriteRestriction, + BanAddEnumValue, + BanAddCompositeAttribute, + BanDetachInheritance, + BanAlterSequenceValues, // xtask:new-rule:error-name } @@ -177,6 +182,11 @@ impl Rule { | Rule::BanAlterRoleOptions | Rule::BanAlterDatabaseOptions | Rule::BanAlterRowLevelSecurity + | Rule::BanNewWriteRestriction + | Rule::BanAddEnumValue + | Rule::BanAddCompositeAttribute + | Rule::BanDetachInheritance + | Rule::BanAlterSequenceValues ) } @@ -269,6 +279,11 @@ impl TryFrom<&str> for Rule { "ban-alter-role-options" => Ok(Rule::BanAlterRoleOptions), "ban-alter-database-options" => Ok(Rule::BanAlterDatabaseOptions), "ban-alter-row-level-security" => Ok(Rule::BanAlterRowLevelSecurity), + "ban-new-write-restriction" => Ok(Rule::BanNewWriteRestriction), + "ban-add-enum-value" => Ok(Rule::BanAddEnumValue), + "ban-add-composite-attribute" => Ok(Rule::BanAddCompositeAttribute), + "ban-detach-inheritance" => Ok(Rule::BanDetachInheritance), + "ban-alter-sequence-values" => Ok(Rule::BanAlterSequenceValues), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -370,6 +385,11 @@ impl fmt::Display for Rule { Rule::BanAlterRoleOptions => "ban-alter-role-options", Rule::BanAlterDatabaseOptions => "ban-alter-database-options", Rule::BanAlterRowLevelSecurity => "ban-alter-row-level-security", + Rule::BanNewWriteRestriction => "ban-new-write-restriction", + Rule::BanAddEnumValue => "ban-add-enum-value", + Rule::BanAddCompositeAttribute => "ban-add-composite-attribute", + Rule::BanDetachInheritance => "ban-detach-inheritance", + Rule::BanAlterSequenceValues => "ban-alter-sequence-values", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -669,6 +689,18 @@ impl Linter { } } security_compatibility(self, file); + if [ + Rule::BanNewWriteRestriction, + Rule::BanAddEnumValue, + Rule::BanAddCompositeAttribute, + Rule::BanDetachInheritance, + Rule::BanAlterSequenceValues, + ] + .iter() + .any(|rule| self.rules.contains(rule)) + { + compatibility_additions(self, file); + } // xtask:new-rule:rule-call // locate any ignores in the file diff --git a/crates/squawk_linter/src/rules/compatibility_additions.rs b/crates/squawk_linter/src/rules/compatibility_additions.rs new file mode 100644 index 00000000..df34c8a4 --- /dev/null +++ b/crates/squawk_linter/src/rules/compatibility_additions.rs @@ -0,0 +1,271 @@ +use rustc_hash::FxHashSet; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +use crate::{Linter, Rule, Violation}; + +// Track only unconditional CREATE statements earlier in the file. An IF NOT EXISTS +// statement does not establish that the object was newly created. +pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse) { + let mut tables = FxHashSet::default(); + let mut types = FxHashSet::default(); + let mut sequences = FxHashSet::default(); + + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::CreateTable(table) => { + if table.if_not_exists().is_none() { + if let Some(name) = table.table_name().and_then(|n| n.path()) { + tables.insert(name.syntax().to_string()); + } + } + } + ast::Stmt::CreateType(ty) => { + if let Some(name) = ty.type_name().and_then(|n| n.path()) { + types.insert(name.syntax().to_string()); + } + } + ast::Stmt::CreateSequence(seq) => { + if seq.if_not_exists().is_none() { + if let Some(name) = seq.sequence().and_then(|n| n.path()) { + sequences.insert(name.syntax().to_string()); + } + } + } + ast::Stmt::AlterTable(table) => { + let new_table = table + .table_relation_name() + .and_then(|n| n.table_name_ref()) + .and_then(|n| n.path_ref()) + .is_some_and(|n| tables.contains(&n.syntax().to_string())); + for action in table.actions() { + if ctx.rules.contains(&Rule::BanNewWriteRestriction) && !new_table { + match &action { + ast::AlterTableAction::AddConstraint(add) => { + if let Some(constraint) = add.constraint() { + let restriction = match &constraint { + ast::Constraint::CheckConstraint(_) + | ast::Constraint::ForeignKeyConstraint(_) + | ast::Constraint::UniqueConstraint(_) + | ast::Constraint::PrimaryKeyConstraint(_) + | ast::Constraint::ExcludeConstraint(_) => true, + _ => false, + }; + if restriction { + ctx.report(Violation::for_node(Rule::BanNewWriteRestriction, + "A new constraint can reject writes from existing clients, even when it is NOT VALID.".into(), add.syntax())); + } + } + } + ast::AlterTableAction::AlterColumn(column) => { + if let Some(ast::AlterColumnOption::SetNotNull(node)) = + column.option() + { + ctx.report(Violation::for_node( + Rule::BanNewWriteRestriction, + "SET NOT NULL can reject writes from existing clients." + .into(), + node.syntax(), + )); + } + } + _ => (), + } + } + if ctx.rules.contains(&Rule::BanAlterSequenceValues) && !new_table { + if let ast::AlterTableAction::AlterColumn(column) = &action { + if let Some(option) = column.option() { + match option { + ast::AlterColumnOption::Restart(node) => ctx.report(Violation::for_node( + Rule::BanAlterSequenceValues, "Restarting an identity sequence can change values generated for existing clients.".into(), node.syntax())), + ast::AlterColumnOption::SetSequenceOption(node) => ctx.report(Violation::for_node( + Rule::BanAlterSequenceValues, "Changing an identity sequence option can change values generated for existing clients.".into(), node.syntax())), + ast::AlterColumnOption::SetGeneratedOptions(options) => { + for option in options.set_generated_options() { + if matches!(option, ast::SetGeneratedOption::Restart(_) | ast::SetGeneratedOption::SetSequenceOption(_)) { + ctx.report(Violation::for_node(Rule::BanAlterSequenceValues, + "Changing an identity sequence can change values generated for existing clients.".into(), option.syntax())); + } + } + } + _ => (), + } + } + } + } + if ctx.rules.contains(&Rule::BanDetachInheritance) && !new_table { + match action { + ast::AlterTableAction::DetachPartition(node) => ctx.report(Violation::for_node( + Rule::BanDetachInheritance, + "Detaching a partition changes which rows existing clients can read and write through the parent table.".into(), node.syntax())), + ast::AlterTableAction::NoInheritTable(node) => ctx.report(Violation::for_node( + Rule::BanDetachInheritance, + "NO INHERIT changes which rows existing clients can read and write through the parent table.".into(), node.syntax())), + _ => (), + } + } + } + } + ast::Stmt::CreateIndex(index) if ctx.rules.contains(&Rule::BanNewWriteRestriction) => { + if index.unique_token().is_some() + && index + .table_relation_name() + .and_then(|n| n.table_name_ref()) + .and_then(|n| n.path_ref()) + .is_none_or(|n| !tables.contains(&n.syntax().to_string())) + { + ctx.report(Violation::for_node(Rule::BanNewWriteRestriction, + "A unique index can reject writes from existing clients, even when created CONCURRENTLY.".into(), index.syntax())); + } + } + ast::Stmt::AlterType(ty) => { + let new_type = ty + .type_name_ref() + .and_then(|n| n.path_ref()) + .is_some_and(|n| types.contains(&n.syntax().to_string())); + if !new_type { + match ty.action() { + Some(ast::AlterTypeAction::AddValue(node)) + if ctx.rules.contains(&Rule::BanAddEnumValue) => + { + ctx.report(Violation::for_node(Rule::BanAddEnumValue, + "Adding an enum value changes the set of values existing clients can receive.".into(), node.syntax())); + } + Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) + if ctx.rules.contains(&Rule::BanAddCompositeAttribute) => + { + for action in list.actions() { + if let ast::AlterTypeAttributeAction::AddAttribute(node) = action { + ctx.report(Violation::for_node(Rule::BanAddCompositeAttribute, + "Adding a composite attribute changes the shape of values existing clients receive.".into(), node.syntax())); + } + } + } + _ => (), + } + } + } + ast::Stmt::AlterSequence(seq) if ctx.rules.contains(&Rule::BanAlterSequenceValues) => { + let new_sequence = seq + .sequence_ref() + .and_then(|n| n.path_ref()) + .is_some_and(|n| sequences.contains(&n.syntax().to_string())); + if !new_sequence { + for action in seq.actions() { + if let ast::AlterSequenceAction::SequenceOption(option) = action { + ctx.report(Violation::for_node(Rule::BanAlterSequenceValues, + "Changing a sequence option can change values generated for existing clients.".into(), option.syntax())); + } + } + } + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + + #[test] + fn write_restrictions() { + let sql = "ALTER TABLE t ADD CONSTRAINT ck CHECK (id > 0) NOT VALID; ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES p(id) NOT VALID; ALTER TABLE t ALTER COLUMN id SET NOT NULL; CREATE UNIQUE INDEX CONCURRENTLY idx ON t(id);"; + assert_eq!( + lint_errors(sql, Rule::BanNewWriteRestriction) + .matches("warning[ban-new-write-restriction]") + .count(), + 4 + ); + lint_ok( + "CREATE TABLE t (id int); ALTER TABLE t ADD CONSTRAINT ck CHECK (id > 0) NOT VALID; CREATE UNIQUE INDEX idx ON t(id);", + Rule::BanNewWriteRestriction, + ); + lint_ok( + "CREATE INDEX CONCURRENTLY idx ON t(id);", + Rule::BanNewWriteRestriction, + ); + } + + #[test] + fn enum_value() { + assert_eq!( + lint_errors( + "ALTER TYPE mood ADD VALUE IF NOT EXISTS 'happy' BEFORE 'sad';", + Rule::BanAddEnumValue + ) + .matches("warning[ban-add-enum-value]") + .count(), + 1 + ); + lint_ok( + "CREATE TYPE mood AS ENUM ('sad'); ALTER TYPE mood ADD VALUE 'happy';", + Rule::BanAddEnumValue, + ); + } + + #[test] + fn composite_attribute() { + assert_eq!( + lint_errors( + "ALTER TYPE address ADD ATTRIBUTE street text;", + Rule::BanAddCompositeAttribute + ) + .matches("warning[ban-add-composite-attribute]") + .count(), + 1 + ); + lint_ok( + "CREATE TYPE address AS (city text); ALTER TYPE address ADD ATTRIBUTE street text;", + Rule::BanAddCompositeAttribute, + ); + lint_ok( + "ALTER TYPE address DROP ATTRIBUTE street;", + Rule::BanAddCompositeAttribute, + ); + } + + #[test] + fn detach_inheritance() { + assert_eq!(lint_errors("ALTER TABLE parent DETACH PARTITION child CONCURRENTLY; ALTER TABLE child NO INHERIT parent;", Rule::BanDetachInheritance).matches("warning[ban-detach-inheritance]").count(), 2); + lint_ok( + "CREATE TABLE child (id int); ALTER TABLE child NO INHERIT parent;", + Rule::BanDetachInheritance, + ); + } + + #[test] + fn sequence_values() { + assert_eq!( + lint_errors( + "ALTER SEQUENCE ids RESTART WITH 1 INCREMENT BY 2;", + Rule::BanAlterSequenceValues + ) + .matches("warning[ban-alter-sequence-values]") + .count(), + 2 + ); + lint_ok( + "CREATE SEQUENCE ids; ALTER SEQUENCE ids RESTART WITH 1;", + Rule::BanAlterSequenceValues, + ); + lint_ok( + "ALTER SEQUENCE ids OWNER TO app;", + Rule::BanAlterSequenceValues, + ); + assert_eq!( + lint_errors( + "ALTER TABLE t ALTER COLUMN id RESTART WITH 5;", + Rule::BanAlterSequenceValues + ) + .matches("warning[ban-alter-sequence-values]") + .count(), + 1 + ); + } +} diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index e63d9183..02cc818b 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -35,6 +35,7 @@ pub(crate) mod ban_set_schema; pub(crate) mod ban_truncate_cascade; pub(crate) mod ban_uncommitted_transaction; pub(crate) mod changing_column_type; +pub(crate) mod compatibility_additions; pub(crate) mod constraint_missing_not_valid; pub(crate) mod disallow_unique_constraint; pub(crate) mod identifier_too_long; @@ -98,6 +99,7 @@ pub(crate) use ban_set_schema::ban_set_schema; 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 compatibility_additions::compatibility_additions; pub(crate) use constraint_missing_not_valid::constraint_missing_not_valid; pub(crate) use disallow_unique_constraint::disallow_unique_constraint; pub(crate) use identifier_too_long::identifier_too_long; diff --git a/docs/docs/ban-add-composite-attribute.md b/docs/docs/ban-add-composite-attribute.md new file mode 100644 index 00000000..05fa9ed2 --- /dev/null +++ b/docs/docs/ban-add-composite-attribute.md @@ -0,0 +1,18 @@ +--- +id: ban-add-composite-attribute +title: ban-add-composite-attribute +--- + +## problem + +A new composite attribute changes the structure of values that clients receive. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER TYPE address ADD ATTRIBUTE street text; +``` + +## solution + +Update clients to handle the new attribute before adding it. + +Enable this rule with `--include ban-add-composite-attribute`. diff --git a/docs/docs/ban-add-enum-value.md b/docs/docs/ban-add-enum-value.md new file mode 100644 index 00000000..74792968 --- /dev/null +++ b/docs/docs/ban-add-enum-value.md @@ -0,0 +1,18 @@ +--- +id: ban-add-enum-value +title: ban-add-enum-value +--- + +## problem + +A new enum value can reach clients that do not handle it. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER TYPE mood ADD VALUE 'happy'; +``` + +## solution + +Update clients to handle the new enum value before adding it. + +Enable this rule with `--include ban-add-enum-value`. diff --git a/docs/docs/ban-alter-sequence-values.md b/docs/docs/ban-alter-sequence-values.md new file mode 100644 index 00000000..e925bab7 --- /dev/null +++ b/docs/docs/ban-alter-sequence-values.md @@ -0,0 +1,18 @@ +--- +id: ban-alter-sequence-values +title: ban-alter-sequence-values +--- + +## problem + +Changes to standalone and identity sequence options can change generated values. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER SEQUENCE ids RESTART WITH 1; +``` + +## solution + +Coordinate sequence changes with clients that use generated values. + +Enable this rule with `--include ban-alter-sequence-values`. diff --git a/docs/docs/ban-detach-inheritance.md b/docs/docs/ban-detach-inheritance.md new file mode 100644 index 00000000..2950d5fa --- /dev/null +++ b/docs/docs/ban-detach-inheritance.md @@ -0,0 +1,18 @@ +--- +id: ban-detach-inheritance +title: ban-detach-inheritance +--- + +## problem + +DETACH PARTITION and NO INHERIT change which rows clients access through a parent table. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER TABLE parent DETACH PARTITION child CONCURRENTLY; +``` + +## solution + +Update clients that use the parent table before detaching the child. + +Enable this rule with `--include ban-detach-inheritance`. diff --git a/docs/docs/ban-new-write-restriction.md b/docs/docs/ban-new-write-restriction.md new file mode 100644 index 00000000..0169b5aa --- /dev/null +++ b/docs/docs/ban-new-write-restriction.md @@ -0,0 +1,18 @@ +--- +id: ban-new-write-restriction +title: ban-new-write-restriction +--- + +## problem + +New constraints, NOT VALID constraints, SET NOT NULL, and unique indexes (including CONCURRENTLY) can reject writes from existing clients. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0) NOT VALID; +``` + +## solution + +Update client writes before adding the restriction. + +Enable this rule with `--include ban-new-write-restriction`. diff --git a/docs/sidebars.js b/docs/sidebars.js index fd4abde5..78e497d4 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -73,6 +73,11 @@ module.exports = { "ban-alter-role-options", "ban-alter-database-options", "ban-alter-row-level-security", + "ban-new-write-restriction", + "ban-add-enum-value", + "ban-add-composite-attribute", + "ban-detach-inheritance", + "ban-alter-sequence-values", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index ebc4d5da..40f784ef 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -292,6 +292,11 @@ const rules = [ { name: "ban-alter-role-options", tags: ["backwards compatibility"], description: "Review role option and configuration changes (opt-in)." }, { name: "ban-alter-database-options", tags: ["backwards compatibility"], description: "Review database option and configuration changes (opt-in)." }, { name: "ban-alter-row-level-security", tags: ["backwards compatibility"], description: "Review row level security changes (opt-in)." }, + { name: "ban-new-write-restriction", tags: ["backwards compatibility"], description: "Detect new write restrictions on existing tables (opt-in)." }, + { name: "ban-add-enum-value", tags: ["backwards compatibility"], description: "Detect new enum values (opt-in)." }, + { name: "ban-add-composite-attribute", tags: ["backwards compatibility"], description: "Detect new composite attributes (opt-in)." }, + { name: "ban-detach-inheritance", tags: ["backwards compatibility"], description: "Detect partition detach and NO INHERIT (opt-in)." }, + { name: "ban-alter-sequence-values", tags: ["backwards compatibility"], description: "Detect changes to sequence values (opt-in)." }, // xtask:new-rule:rule-doc-meta ] From 9ac7d474a4a2971eadcb75540b1646b3cb317a7c Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:38:03 +1100 Subject: [PATCH 09/21] fix(linter): cover additional write restrictions and update snapshots --- crates/squawk_linter/src/lib.rs | 22 +++--- .../src/rules/ban_drop_default.rs | 2 +- .../src/rules/compatibility_additions.rs | 67 +++++++++++++++++++ crates/squawk_linter/src/rules/mod.rs | 8 +-- ...alter_generated_expression__test__err.snap | 11 +-- docs/docs/ban-new-write-restriction.md | 2 +- 6 files changed, 91 insertions(+), 21 deletions(-) diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 186b457a..9ff8c5d7 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -70,9 +70,10 @@ use rules::require_timeout_settings; use rules::transaction_nesting; use rules::{ ban_alter_generated_expression, ban_alter_identity, ban_disable_trigger, ban_drop_constraint, - ban_drop_domain, ban_drop_generated_expression, ban_drop_index, ban_drop_policy, ban_drop_schema, ban_drop_sequence, - ban_replace_view_function, ban_replica_identity, ban_revoke, ban_set_default, ban_set_schema, - compatibility_additions, renaming_object, security_compatibility, + ban_drop_domain, ban_drop_generated_expression, ban_drop_index, ban_drop_policy, + ban_drop_schema, ban_drop_sequence, ban_replace_view_function, ban_replica_identity, + ban_revoke, ban_set_default, ban_set_schema, compatibility_additions, renaming_object, + security_compatibility, }; // xtask:new-rule:rule-import @@ -668,7 +669,10 @@ impl Linter { (Rule::BanDropSequence, ban_drop_sequence), (Rule::BanDropDomain, ban_drop_domain), (Rule::BanDropConstraint, ban_drop_constraint), - (Rule::BanDropGeneratedExpression, ban_drop_generated_expression), + ( + Rule::BanDropGeneratedExpression, + ban_drop_generated_expression, + ), (Rule::RenamingObject, renaming_object), (Rule::BanSetSchema, ban_set_schema), (Rule::BanAlterIdentity, ban_alter_identity), @@ -820,12 +824,10 @@ mod tests { #[test] fn new_opt_in_rules_only_report_when_included() { - for (rule, sql) in [ - ( - Rule::BanAlterGeneratedExpression, - "ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;", - ), - ] { + for (rule, sql) in [( + Rule::BanAlterGeneratedExpression, + "ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;", + )] { let parse = SourceFile::parse(sql); assert!(parse.errors().is_empty()); assert!( diff --git a/crates/squawk_linter/src/rules/ban_drop_default.rs b/crates/squawk_linter/src/rules/ban_drop_default.rs index a4a77d21..2096bf54 100644 --- a/crates/squawk_linter/src/rules/ban_drop_default.rs +++ b/crates/squawk_linter/src/rules/ban_drop_default.rs @@ -72,7 +72,7 @@ ALTER TABLE tbl ALTER COLUMN c DROP DEFAULT, ALTER COLUMN d DROP DEFAULT; #[test] fn ok() { lint_ok( - "ALTER TABLE tbl ALTER COLUMN c SET DEFAULT 1; ALTER TABLE tbl ALTER COLUMN c DROP NOT NULL; DROP DOMAIN d; ALTER DOMAIN d DROP DEFAULT; DROP INDEX i;", + "ALTER TABLE tbl ALTER COLUMN c SET DEFAULT 1; ALTER TABLE tbl ALTER COLUMN c DROP NOT NULL; DROP DOMAIN d; DROP INDEX i;", Rule::BanDropDefault, ); } diff --git a/crates/squawk_linter/src/rules/compatibility_additions.rs b/crates/squawk_linter/src/rules/compatibility_additions.rs index df34c8a4..080ec4cc 100644 --- a/crates/squawk_linter/src/rules/compatibility_additions.rs +++ b/crates/squawk_linter/src/rules/compatibility_additions.rs @@ -71,6 +71,33 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse { + for constraint in column.constraints() { + if matches!( + constraint, + ast::Constraint::NotNullConstraint(_) + | ast::Constraint::ReferencesConstraint(_) + | ast::Constraint::CheckConstraint(_) + | ast::Constraint::UniqueConstraint(_) + ) { + ctx.report(Violation::for_node(Rule::BanNewWriteRestriction, + "A new column constraint can reject writes from existing clients.".into(), constraint.syntax())); + } + } + } + ast::AlterTableAction::AlterConstraint(node) => { + if node.constraint_options().any(|option| { + matches!(option, + ast::ConstraintOption::Enforced(_) + | ast::ConstraintOption::DeferrableConstraintOption(_) + | ast::ConstraintOption::NotDeferrableConstraintOption(_) + | ast::ConstraintOption::InitiallyImmediateConstraintOption(_) + | ast::ConstraintOption::InitiallyDeferredConstraintOption(_)) + }) { + ctx.report(Violation::for_node(Rule::BanNewWriteRestriction, + "Changing constraint enforcement or timing can reject existing client writes.".into(), node.syntax())); + } + } _ => (), } } @@ -108,6 +135,19 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse { + if let Some(action) = domain.action() { + match action { + ast::AlterDomainAction::SetNotNull(node) => ctx.report(Violation::for_node( + Rule::BanNewWriteRestriction, + "A domain NOT NULL requirement can reject writes from existing clients.".into(), node.syntax())), + ast::AlterDomainAction::AddConstraint(node) => ctx.report(Violation::for_node( + Rule::BanNewWriteRestriction, + "A domain constraint can reject writes from existing clients, even when it is NOT VALID.".into(), node.syntax())), + _ => (), + } + } + } ast::Stmt::CreateIndex(index) if ctx.rules.contains(&Rule::BanNewWriteRestriction) => { if index.unique_token().is_some() && index @@ -192,6 +232,33 @@ mod test { ); } + #[test] + fn more_write_restrictions() { + assert_eq!(lint_errors("ALTER DOMAIN d SET NOT NULL; ALTER DOMAIN d ADD CONSTRAINT c CHECK (VALUE > 0) NOT VALID;", Rule::BanNewWriteRestriction).matches("warning[ban-new-write-restriction]").count(), 2); + assert_eq!( + lint_errors( + "ALTER TABLE t ADD COLUMN c int NOT NULL;", + Rule::BanNewWriteRestriction + ) + .matches("warning[ban-new-write-restriction]") + .count(), + 1 + ); + assert_eq!( + lint_errors( + "ALTER TABLE t ALTER CONSTRAINT fk NOT DEFERRABLE;", + Rule::BanNewWriteRestriction + ) + .matches("warning[ban-new-write-restriction]") + .count(), + 1 + ); + lint_ok( + "ALTER TABLE t ALTER CONSTRAINT fk NOT ENFORCED;", + Rule::BanNewWriteRestriction, + ); + } + #[test] fn enum_value() { assert_eq!( diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index 02cc818b..6c6f4051 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -12,11 +12,11 @@ pub(crate) mod ban_create_domain_with_constraint; pub(crate) mod ban_disable_trigger; pub(crate) mod ban_drop_column; pub(crate) mod ban_drop_constraint; -pub(crate) mod ban_drop_generated_expression; pub(crate) mod ban_drop_database; pub(crate) mod ban_drop_default; pub(crate) mod ban_drop_domain; pub(crate) mod ban_drop_function; +pub(crate) mod ban_drop_generated_expression; pub(crate) mod ban_drop_index; pub(crate) mod ban_drop_not_null; pub(crate) mod ban_drop_policy; @@ -56,8 +56,8 @@ pub(crate) mod require_concurrent_reindex; pub(crate) mod require_enum_value_ordering; pub(crate) mod require_table_schema; pub(crate) mod require_timeout_settings; -pub(crate) mod transaction_nesting; pub(crate) mod security_compatibility; +pub(crate) mod transaction_nesting; #[cfg(test)] mod variant_tests; // xtask:new-rule:mod-decl @@ -76,11 +76,11 @@ pub(crate) use ban_create_domain_with_constraint::ban_create_domain_with_constra pub(crate) use ban_disable_trigger::ban_disable_trigger; pub(crate) use ban_drop_column::ban_drop_column; pub(crate) use ban_drop_constraint::ban_drop_constraint; -pub(crate) use ban_drop_generated_expression::ban_drop_generated_expression; pub(crate) use ban_drop_database::ban_drop_database; pub(crate) use ban_drop_default::ban_drop_default; pub(crate) use ban_drop_domain::ban_drop_domain; pub(crate) use ban_drop_function::ban_drop_function; +pub(crate) use ban_drop_generated_expression::ban_drop_generated_expression; pub(crate) use ban_drop_index::ban_drop_index; pub(crate) use ban_drop_not_null::ban_drop_not_null; pub(crate) use ban_drop_policy::ban_drop_policy; @@ -120,6 +120,6 @@ pub(crate) use require_concurrent_reindex::require_concurrent_reindex; pub(crate) use require_enum_value_ordering::require_enum_value_ordering; pub(crate) use require_table_schema::require_table_schema; pub(crate) use require_timeout_settings::require_timeout_settings; -pub(crate) use transaction_nesting::transaction_nesting; pub(crate) use security_compatibility::security_compatibility; +pub(crate) use transaction_nesting::transaction_nesting; // xtask:new-rule:export diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap index 4eec1cce..13c633cd 100644 --- a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap @@ -1,12 +1,13 @@ --- source: crates/squawk_linter/src/rules/ban_alter_generated_expression.rs +assertion_line: 41 expression: "lint_errors(sql, Rule::BanAlterGeneratedExpression)" --- warning[ban-alter-generated-expression]: Changing a generated column may break inserts from existing clients. ╭▸ -1 │ ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; - ╰╴ ━━━━━━━━━━━━━━━ -warning[ban-alter-generated-expression]: Changing a generated column may break inserts from existing clients. +1 │ ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 2); ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-alter-generated-expression]: Adding a generated column may break inserts from existing clients. ╭▸ -1 │ ALTER TABLE t ALTER COLUMN c DROP EXPRESSION; ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; - ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +1 │ ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 2); ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/docs/docs/ban-new-write-restriction.md b/docs/docs/ban-new-write-restriction.md index 0169b5aa..21fdeb6f 100644 --- a/docs/docs/ban-new-write-restriction.md +++ b/docs/docs/ban-new-write-restriction.md @@ -5,7 +5,7 @@ title: ban-new-write-restriction ## problem -New constraints, NOT VALID constraints, SET NOT NULL, and unique indexes (including CONCURRENTLY) can reject writes from existing clients. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. +New table and domain constraints, `NOT VALID` constraints, `SET NOT NULL`, constrained added columns, changed constraint timing, and unique indexes (including `CONCURRENTLY`) can reject writes from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that table. The rule does not know whether an object existed before the migration. ```sql ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0) NOT VALID; From aba4b1af034b18019c2c7b5b012f1cf3884e68b3 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:40:08 +1100 Subject: [PATCH 10/21] docs: explain application rollback checks and update rule policy --- CHANGELOG.md | 6 ++- crates/squawk_linter/src/lib.rs | 45 +++++++++++++++++++ .../application-rollback-compatibility.md | 22 +++++++++ docs/docs/ban-alter-generated-expression.md | 4 +- docs/sidebars.js | 2 +- 5 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 docs/docs/application-rollback-compatibility.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e00a167..e652ef42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - linter: ban-drop-view, ban-drop-function, ban-drop-type, ban-drop-default rules - linter: opt-in ban-drop-trigger rule - linter: compatibility rules for dropped schemas, sequences, and domains, object renames, and schema moves -- linter: opt-in compatibility rules for dropped constraints and indexes, identity and generated columns, defaults, triggers, replica identity, policies, privileges, and replacements +- linter: default checks for dropped constraints, identity changes, and dropped generated expressions; opt-in checks for dropped indexes, generated expression replacement and addition, defaults, triggers, replica identity, policies, privileges, and replacements - linter: default ban-drop-extension and opt-in rules for policy creation, policy conditions and roles, function and view options, role and database options, and row level security +### Changed + +- linter: extend existing compatibility checks to more PostgreSQL statement variants, including domain and foreign-table changes, replica-only trigger and rule firing, generated expression replacement, and routine schema moves + ## v2.67.0 - 2026-10-04 ### Added diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 9ff8c5d7..0a1c3e42 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -884,6 +884,51 @@ mod tests { assert!(!linter.rules.contains(&Rule::BanDropTable)); } + #[test] + fn compatibility_defaults_and_explicit_configuration() { + for (rule, sql) in [ + (Rule::BanDropConstraint, "ALTER TABLE t DROP CONSTRAINT c;"), + ( + Rule::BanAlterIdentity, + "ALTER TABLE t ALTER COLUMN id ADD GENERATED ALWAYS AS IDENTITY;", + ), + ( + Rule::BanDropGeneratedExpression, + "ALTER TABLE t ALTER COLUMN c DROP EXPRESSION;", + ), + (Rule::BanDropExtension, "DROP EXTENSION hstore;"), + ] { + let parse = SourceFile::parse(sql); + assert!(parse.errors().is_empty()); + assert!( + Linter::with_default_rules() + .lint(&parse, sql) + .iter() + .any(|v| v.code == rule) + ); + assert!( + !Linter::with_rules(&[], &[rule]) + .lint(&parse, sql) + .iter() + .any(|v| v.code == rule) + ); + } + let sql = "ALTER TYPE mood ADD VALUE 'new';"; + let parse = SourceFile::parse(sql); + assert!( + Linter::with_rules(&[Rule::BanAddEnumValue], &[]) + .lint(&parse, sql) + .iter() + .any(|v| v.code == Rule::BanAddEnumValue) + ); + assert!( + !Linter::with_rules(&[Rule::BanAddEnumValue], &[Rule::BanAddEnumValue]) + .lint(&parse, sql) + .iter() + .any(|v| v.code == Rule::BanAddEnumValue) + ); + } + #[test] fn require_timeout_settings_expands_to_granular_rules() { let linter = Linter::from([Rule::RequireTimeoutSettings]); diff --git a/docs/docs/application-rollback-compatibility.md b/docs/docs/application-rollback-compatibility.md new file mode 100644 index 00000000..855420cf --- /dev/null +++ b/docs/docs/application-rollback-compatibility.md @@ -0,0 +1,22 @@ +--- +id: application-rollback-compatibility +title: Application rollback compatibility +--- + +A migration can finish without blocking traffic and still break an application-only rollback. An application-only rollback keeps the migrated database and all data written by the new application. Run the old application against that database with its actual database role before declaring the migration compatible. + +## Use the compatibility rules + +Default rules detect some operations that remove objects or guarantees. Additional rules are opt-in because the effect depends on application writes, reads, permissions, and data. Enable them with `--include `. A `NOT VALID` constraint still rejects new invalid writes. A unique index built `CONCURRENTLY` still enforces uniqueness. Neither option proves rollback compatibility. + +The linter checks parsed SQL, not the live database schema. Some opt-in rules skip objects created unconditionally earlier in the same migration; this is only a statement-level approximation. Explicit inclusion, exclusion, and ignore comments still apply. The rule documentation lists each SQL form. + +## Verify an application-only rollback + +1. Apply the migration to representative data. +2. Start the new application and write representative new data. +3. Start the old application against the same database, with its actual database role. +4. Check reads, writes, result decoding, authorization, identifier generation, and database side effects. +5. Repeat for every application version that remains an allowed rollback target. + +Review data migrations (including status strings, JSON formats, destructive updates, plain `TRUNCATE`, and deletes), function and trigger bodies, role membership and privileges, and SQL inside procedural blocks or dynamic SQL separately. Lint results do not prove compatibility. Keep lock-duration and table-rewrite checks separate from application compatibility checks. diff --git a/docs/docs/ban-alter-generated-expression.md b/docs/docs/ban-alter-generated-expression.md index 062c0174..6642d3a3 100644 --- a/docs/docs/ban-alter-generated-expression.md +++ b/docs/docs/ban-alter-generated-expression.md @@ -5,7 +5,7 @@ title: ban-alter-generated-expression ## problem -Adding a generated column can reject inserts that supply its value. `SET EXPRESSION` changes the values returned for existing rows when PostgreSQL rewrites the table. This rule is opt-in because compatibility depends on how clients use the column. +Adding a generated column changes the result shape for strict `SELECT *` decoders and can affect positional inserts. Inserts with explicit column lists do not inherently fail. `SET EXPRESSION` changes values returned to existing clients. For stored generated columns, PostgreSQL rewrites stored values; assess that rewrite separately as an availability concern. This rule is opt-in because compatibility depends on how clients use the column. ```sql ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1); @@ -13,6 +13,6 @@ ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1); ## solution -Update client inserts before adding a generated column. Check client reads before changing its expression. `DROP EXPRESSION` is not checked by this rule. +Check client reads and positional inserts before adding a generated column. Check client reads before changing its expression. The default `ban-drop-generated-expression` rule checks `DROP EXPRESSION`. Enable this rule with `--include ban-alter-generated-expression` (or add `ban-alter-generated-expression` to your configured include list). diff --git a/docs/sidebars.js b/docs/sidebars.js index 78e497d4..e4546032 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -1,6 +1,6 @@ module.exports = { someSidebar: { - General: ["quick_start", "safe_migrations", "cli", "github_app", "web-frameworks", "postgres-locks", "troubleshooting"], + General: ["quick_start", "safe_migrations", "application-rollback-compatibility", "cli", "github_app", "web-frameworks", "postgres-locks", "troubleshooting"], Rules: [ "rules", "adding-field-with-default", From e83f47ef88e42f2431b4fe5ebdd9a5fd8aede2ca Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:39:38 +1100 Subject: [PATCH 11/21] feat(linter): cover remaining compatibility statement variants --- .../src/rules/ban_drop_column.rs | 29 ++++++++---- .../src/rules/ban_drop_constraint.rs | 24 ++++++++-- .../src/rules/ban_drop_function.rs | 5 ++ .../squawk_linter/src/rules/ban_drop_type.rs | 8 +++- crates/squawk_linter/src/rules/ban_revoke.rs | 13 +++++ .../squawk_linter/src/rules/ban_set_schema.rs | 11 +++++ .../src/rules/changing_column_type.rs | 31 ++++++++---- .../src/rules/renaming_column.rs | 5 ++ .../src/rules/renaming_object.rs | 47 ++++++++++++++++--- .../squawk_linter/src/rules/variant_tests.rs | 33 +++++++++++-- docs/docs/ban-drop-column.md | 2 +- docs/docs/ban-drop-constraint.md | 2 +- docs/docs/ban-drop-function.md | 2 +- docs/docs/ban-drop-type.md | 2 +- docs/docs/ban-revoke.md | 2 +- docs/docs/ban-set-schema.md | 2 +- docs/docs/changing-column-type.md | 3 +- docs/docs/renaming-column.md | 2 +- docs/docs/renaming-object.md | 2 +- 19 files changed, 184 insertions(+), 41 deletions(-) diff --git a/crates/squawk_linter/src/rules/ban_drop_column.rs b/crates/squawk_linter/src/rules/ban_drop_column.rs index 0b6bf474..5f458e33 100644 --- a/crates/squawk_linter/src/rules/ban_drop_column.rs +++ b/crates/squawk_linter/src/rules/ban_drop_column.rs @@ -8,15 +8,28 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_column(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { - if let ast::Stmt::AlterTable(alter_table) = stmt { - for action in alter_table.actions() { - if let ast::AlterTableAction::DropColumn(drop_column) = action { - ctx.report(Violation::for_node( - Rule::BanDropColumn, - "Dropping a column may break existing clients.".into(), - drop_column.syntax(), - )); + let actions: Vec<_> = match stmt { + ast::Stmt::AlterTable(table) => table.actions().collect(), + ast::Stmt::AlterForeignTable(table) => table.actions().collect(), + ast::Stmt::AlterType(ty) => { + if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() { + for action in list.actions() { + if let ast::AlterTypeAttributeAction::DropAttribute(node) = action { + ctx.report(Violation::for_node(Rule::BanDropColumn, "Dropping an attribute may break existing clients.".into(), node.syntax())); + } + } } + Vec::new() + } + _ => Vec::new(), + }; + for action in actions { + if let ast::AlterTableAction::DropColumn(drop_column) = action { + ctx.report(Violation::for_node( + Rule::BanDropColumn, + "Dropping a column may break existing clients.".into(), + drop_column.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/ban_drop_constraint.rs b/crates/squawk_linter/src/rules/ban_drop_constraint.rs index 2e058da4..85a28fd8 100644 --- a/crates/squawk_linter/src/rules/ban_drop_constraint.rs +++ b/crates/squawk_linter/src/rules/ban_drop_constraint.rs @@ -6,11 +6,27 @@ use squawk_syntax::{ pub(crate) fn ban_drop_constraint(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { - if let ast::Stmt::AlterTable(table) = stmt { - for action in table.actions() { - if let ast::AlterTableAction::DropConstraint(node) = action { - ctx.report(Violation::for_node(Rule::BanDropConstraint, "Dropping a constraint may remove a guarantee that existing clients assume.".into(), node.syntax())); + let actions: Vec<_> = match &stmt { + ast::Stmt::AlterTable(table) => table.actions().collect(), + ast::Stmt::AlterForeignTable(table) => table.actions().collect(), + _ => Vec::new(), + }; + for action in actions { + match action { + ast::AlterTableAction::DropConstraint(node) => ctx.report(Violation::for_node(Rule::BanDropConstraint, "Dropping a constraint may remove a guarantee that existing clients assume.".into(), node.syntax())), + ast::AlterTableAction::AlterConstraint(node) => { + for option in node.constraint_options() { + if let ast::ConstraintOption::NotEnforced(option) = option { + ctx.report(Violation::for_node(Rule::BanDropConstraint, "Disabling constraint enforcement may remove a guarantee that existing clients assume.".into(), option.syntax())); + } + } } + _ => (), + } + } + if let ast::Stmt::AlterDomain(domain) = stmt { + if let Some(ast::AlterDomainAction::DropConstraint(node)) = domain.action() { + ctx.report(Violation::for_node(Rule::BanDropConstraint, "Dropping a constraint may remove a guarantee that existing clients assume.".into(), node.syntax())); } } } diff --git a/crates/squawk_linter/src/rules/ban_drop_function.rs b/crates/squawk_linter/src/rules/ban_drop_function.rs index f870667a..12c5b613 100644 --- a/crates/squawk_linter/src/rules/ban_drop_function.rs +++ b/crates/squawk_linter/src/rules/ban_drop_function.rs @@ -8,6 +8,11 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_function(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { match stmt { + ast::Stmt::DropAggregate(node) => ctx.report(Violation::for_node( + Rule::BanDropFunction, + "Dropping an aggregate may break existing clients.".into(), + node.syntax(), + )), ast::Stmt::DropFunction(node) => ctx.report(Violation::for_node( Rule::BanDropFunction, "Dropping a function may break existing clients.".into(), diff --git a/crates/squawk_linter/src/rules/ban_drop_type.rs b/crates/squawk_linter/src/rules/ban_drop_type.rs index 65273ef9..6345028d 100644 --- a/crates/squawk_linter/src/rules/ban_drop_type.rs +++ b/crates/squawk_linter/src/rules/ban_drop_type.rs @@ -7,7 +7,13 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_type(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { - if let ast::Stmt::DropCast(node) = stmt { + if let ast::Stmt::DropOperator(node) = stmt { + ctx.report(Violation::for_node(Rule::BanDropType, "Dropping an operator may break existing clients.".into(), node.syntax())); + } else if let ast::Stmt::DropOperatorClass(node) = stmt { + ctx.report(Violation::for_node(Rule::BanDropType, "Dropping an operator class may break existing clients.".into(), node.syntax())); + } else if let ast::Stmt::DropOperatorFamily(node) = stmt { + ctx.report(Violation::for_node(Rule::BanDropType, "Dropping an operator family may break existing clients.".into(), node.syntax())); + } else if let ast::Stmt::DropCast(node) = stmt { ctx.report(Violation::for_node( Rule::BanDropType, "Dropping a cast may break existing clients.".into(), diff --git a/crates/squawk_linter/src/rules/ban_revoke.rs b/crates/squawk_linter/src/rules/ban_revoke.rs index 80981da8..e53a77f9 100644 --- a/crates/squawk_linter/src/rules/ban_revoke.rs +++ b/crates/squawk_linter/src/rules/ban_revoke.rs @@ -6,6 +6,9 @@ use squawk_syntax::{ pub(crate) fn ban_revoke(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { + for owner in stmt.syntax().descendants().filter_map(ast::OwnerTo::cast) { + ctx.report(Violation::for_node(Rule::BanRevoke, "Changing object ownership may break existing clients.".into(), owner.syntax())); + } match stmt { ast::Stmt::Revoke(node) => { ctx.report(Violation::for_node( @@ -19,6 +22,16 @@ pub(crate) fn ban_revoke(ctx: &mut Linter, parse: &Parse) { "Dropping owned objects or privileges may break existing clients.".into(), node.syntax(), )), + ast::Stmt::Reassign(node) => ctx.report(Violation::for_node( + Rule::BanRevoke, + "Reassigning owned objects may break existing clients.".into(), + node.syntax(), + )), + ast::Stmt::DropUser(node) => ctx.report(Violation::for_node( + Rule::BanRevoke, + "Dropping a user may break existing clients.".into(), + node.syntax(), + )), ast::Stmt::DropRole(node) => ctx.report(Violation::for_node( Rule::BanRevoke, "Dropping a role may break existing clients.".into(), diff --git a/crates/squawk_linter/src/rules/ban_set_schema.rs b/crates/squawk_linter/src/rules/ban_set_schema.rs index 90365a0e..3e954c08 100644 --- a/crates/squawk_linter/src/rules/ban_set_schema.rs +++ b/crates/squawk_linter/src/rules/ban_set_schema.rs @@ -7,6 +7,17 @@ use squawk_syntax::{ pub(crate) fn ban_set_schema(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { match stmt { + ast::Stmt::AlterForeignTable(node) => { + for action in node.actions() { + if let ast::AlterTableAction::SetSchema(node) = action { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + node.syntax(), + )); + } + } + } ast::Stmt::AlterTable(node) => { for action in node.actions() { if let ast::AlterTableAction::SetSchema(node) = action { diff --git a/crates/squawk_linter/src/rules/changing_column_type.rs b/crates/squawk_linter/src/rules/changing_column_type.rs index 6e098ef6..c731a141 100644 --- a/crates/squawk_linter/src/rules/changing_column_type.rs +++ b/crates/squawk_linter/src/rules/changing_column_type.rs @@ -8,17 +8,30 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn changing_column_type(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { - if let ast::Stmt::AlterTable(alter_table) = stmt { - for action in alter_table.actions() { - if let ast::AlterTableAction::AlterColumn(alter_column) = action { - if let Some(ast::AlterColumnOption::SetType(set_type)) = alter_column.option() { - ctx.report(Violation::for_node( - Rule::ChangingColumnType, - "Changing a column type requires an `ACCESS EXCLUSIVE` lock on the table which blocks reads and writes while the table is rewritten. Changing the type of the column may also break other clients reading from the table.".into(), - set_type.syntax(), - )); + let actions: Vec<_> = match stmt { + ast::Stmt::AlterTable(table) => table.actions().collect(), + ast::Stmt::AlterForeignTable(table) => table.actions().collect(), + ast::Stmt::AlterType(ty) => { + if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() { + for action in list.actions() { + if let ast::AlterTypeAttributeAction::AlterAttribute(node) = action { + ctx.report(Violation::for_node(Rule::ChangingColumnType, "Changing an attribute type may break existing clients.".into(), node.syntax())); + } } } + Vec::new() + } + _ => Vec::new(), + }; + for action in actions { + if let ast::AlterTableAction::AlterColumn(alter_column) = action { + if let Some(ast::AlterColumnOption::SetType(set_type)) = alter_column.option() { + ctx.report(Violation::for_node( + Rule::ChangingColumnType, + "Changing a column type requires an `ACCESS EXCLUSIVE` lock on the table which blocks reads and writes while the table is rewritten. Changing the type of the column may also break other clients reading from the table.".into(), + set_type.syntax(), + )); + } } } } diff --git a/crates/squawk_linter/src/rules/renaming_column.rs b/crates/squawk_linter/src/rules/renaming_column.rs index 6ece1b4a..2dc90dc8 100644 --- a/crates/squawk_linter/src/rules/renaming_column.rs +++ b/crates/squawk_linter/src/rules/renaming_column.rs @@ -9,6 +9,11 @@ pub(crate) fn renaming_column(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { match stmt { + ast::Stmt::AlterType(ty) => { + if let Some(ast::AlterTypeAction::RenameAttribute(node)) = ty.action() { + ctx.report(Violation::for_node(Rule::RenamingColumn, "Renaming an attribute may break existing clients.".into(), node.syntax())); + } + } ast::Stmt::AlterTable(table) => { for action in table.actions() { if let ast::AlterTableAction::RenameColumn(node) = action { diff --git a/crates/squawk_linter/src/rules/renaming_object.rs b/crates/squawk_linter/src/rules/renaming_object.rs index 22ff1afc..3b9deb2a 100644 --- a/crates/squawk_linter/src/rules/renaming_object.rs +++ b/crates/squawk_linter/src/rules/renaming_object.rs @@ -18,6 +18,43 @@ pub(crate) fn renaming_object(ctx: &mut Linter, parse: &Parse) { } } } + ast::Stmt::AlterTable(node) => { + for action in node.actions() { + if let ast::AlterTableAction::RenameConstraint(node) = action { + ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a constraint may break existing clients.".into(), node.syntax())); + } + } + } + ast::Stmt::AlterRole(node) => { + if let Some(ast::AlterRoleAction::RoleRenameTo(action)) = node.action() { + ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a role may break existing clients.".into(), action.syntax())); + } + } + ast::Stmt::AlterUser(node) => { + if let Some(ast::AlterUserAction::RoleRenameTo(action)) = node.action() { + ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a user may break existing clients.".into(), action.syntax())); + } + } + ast::Stmt::AlterGroup(node) => { + if let Some(ast::AlterGroupAction::RoleRenameTo(action)) = node.action() { + ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a group may break existing clients.".into(), action.syntax())); + } + } + ast::Stmt::AlterDatabase(node) => { + if let Some(ast::AlterDatabaseAction::DatabaseRenameTo(action)) = node.action() { + ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a database may break existing clients.".into(), action.syntax())); + } + } + ast::Stmt::AlterTrigger(node) => { + if let Some(ast::AlterTriggerAction::TriggerRenameTo(action)) = node.action() { + ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a trigger may break existing clients.".into(), action.syntax())); + } + } + ast::Stmt::AlterPolicy(node) => { + if let Some(ast::AlterPolicyAction::PolicyRenameTo(action)) = node.action() { + ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a policy may break existing clients.".into(), action.syntax())); + } + } ast::Stmt::AlterRoutine(node) => { if let Some(ast::AlterRoutineAction::RoutineRenameTo(action)) = node.action() { ctx.report(Violation::for_node( @@ -114,12 +151,10 @@ pub(crate) fn renaming_object(ctx: &mut Linter, parse: &Parse) { } ast::Stmt::AlterDomain(node) => { for action in node.action().into_iter() { - if let ast::AlterDomainAction::DomainRenameTo(node) = action { - ctx.report(Violation::for_node( - Rule::RenamingObject, - "Renaming a domain may break existing clients.".into(), - node.syntax(), - )); + match action { + ast::AlterDomainAction::DomainRenameTo(node) => ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a domain may break existing clients.".into(), node.syntax())), + ast::AlterDomainAction::RenameConstraint(node) => ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a constraint may break existing clients.".into(), node.syntax())), + _ => (), } } } diff --git a/crates/squawk_linter/src/rules/variant_tests.rs b/crates/squawk_linter/src/rules/variant_tests.rs index 6662e231..6bb11c36 100644 --- a/crates/squawk_linter/src/rules/variant_tests.rs +++ b/crates/squawk_linter/src/rules/variant_tests.rs @@ -4,6 +4,10 @@ use crate::{ }; fn check(sql: &str, rule: Rule, count: usize) { + for statement in sql.split(';').filter(|s| !s.trim().is_empty()) { + let parsed = squawk_syntax::SourceFile::parse(&format!("{statement};")); + assert!(parsed.errors().is_empty(), "{statement}: {:?}", parsed.errors()); + } let errors = lint_errors(sql, rule); assert_eq!(errors.matches("warning[").count(), count, "{errors}"); } @@ -23,6 +27,29 @@ fn renames() { lint_ok("ALTER VIEW v RENAME TO v2;", Rule::RenamingColumn); } +#[test] +fn remaining_renames() { + check("ALTER TABLE t RENAME CONSTRAINT old TO renamed; ALTER DOMAIN d RENAME CONSTRAINT old TO renamed; ALTER ROLE app RENAME TO app2; ALTER USER app RENAME TO app2; ALTER GROUP app RENAME TO app2; ALTER DATABASE db RENAME TO db2; ALTER TRIGGER tr ON t RENAME TO tr2; ALTER POLICY p ON t RENAME TO p2;", Rule::RenamingObject, 8); + check("ALTER TYPE composite RENAME ATTRIBUTE old TO renamed;", Rule::RenamingColumn, 1); + lint_ok("ALTER POLICY p ON t USING (true);", Rule::RenamingObject); +} + +#[test] +fn remaining_constraints_and_attributes() { + check("ALTER DOMAIN d DROP CONSTRAINT c; ALTER TABLE t ALTER CONSTRAINT c NOT ENFORCED; ALTER FOREIGN TABLE ft DROP CONSTRAINT c;", Rule::BanDropConstraint, 3); + check("ALTER TYPE composite DROP ATTRIBUTE a; ALTER FOREIGN TABLE ft DROP COLUMN a;", Rule::BanDropColumn, 2); + check("ALTER TYPE composite ALTER ATTRIBUTE a TYPE text; ALTER FOREIGN TABLE ft ALTER COLUMN a TYPE text;", Rule::ChangingColumnType, 2); + lint_ok("ALTER TABLE t ALTER CONSTRAINT c ENFORCED;", Rule::BanDropConstraint); +} + +#[test] +fn remaining_drops_and_ownership() { + check("DROP AGGREGATE agg(int);", Rule::BanDropFunction, 1); + check("DROP OPERATOR + (int, int); DROP OPERATOR CLASS op USING btree; DROP OPERATOR FAMILY fam USING btree;", Rule::BanDropType, 3); + check("ALTER FOREIGN TABLE ft OWNER TO app; ALTER DOMAIN d OWNER TO app;", Rule::BanRevoke, 2); + lint_ok("ALTER TABLE t SET SCHEMA s;", Rule::BanRevoke); +} + #[test] fn defaults_and_nullability() { check( @@ -55,7 +82,7 @@ fn drops_and_revokes() { check("DROP ROUTINE IF EXISTS f(int);", Rule::BanDropFunction, 1); check("DROP CAST IF EXISTS (text AS int);", Rule::BanDropType, 1); check("DROP OWNED BY app; DROP ROLE app;", Rule::BanRevoke, 2); - lint_ok("REASSIGN OWNED BY app TO admin;", Rule::BanRevoke); + check("REASSIGN OWNED BY app TO admin; ALTER TABLE t OWNER TO admin; DROP USER app;", Rule::BanRevoke, 3); } #[test] @@ -98,8 +125,8 @@ fn enforcement_and_policy() { #[test] fn schema_moves() { check( - "ALTER PROCEDURE p() SET SCHEMA s; ALTER ROUTINE f() SET SCHEMA s;", + "ALTER PROCEDURE p() SET SCHEMA s; ALTER ROUTINE f() SET SCHEMA s; ALTER FOREIGN TABLE ft SET SCHEMA s;", Rule::BanSetSchema, - 2, + 3, ); } diff --git a/docs/docs/ban-drop-column.md b/docs/docs/ban-drop-column.md index de2743b3..0dcbd89d 100644 --- a/docs/docs/ban-drop-column.md +++ b/docs/docs/ban-drop-column.md @@ -5,7 +5,7 @@ title: ban-drop-column ## problem -Dropping a column may break existing clients. +Dropping a table or foreign table column or a composite type attribute may break existing clients. ## solution diff --git a/docs/docs/ban-drop-constraint.md b/docs/docs/ban-drop-constraint.md index 56b7632e..ddba2567 100644 --- a/docs/docs/ban-drop-constraint.md +++ b/docs/docs/ban-drop-constraint.md @@ -5,7 +5,7 @@ title: ban-drop-constraint ## problem -Dropping a constraint removes a foreign key, check, or uniqueness guarantee that clients can depend on. If an old application uses `INSERT ... ON CONFLICT ON CONSTRAINT c` or infers a dropped unique constraint as its conflict arbiter, its inserts fail immediately. This rule is enabled by default. +Dropping a table, foreign table, or domain constraint, or setting a table constraint to `NOT ENFORCED`, removes a guarantee that clients can depend on. If an old application uses `INSERT ... ON CONFLICT ON CONSTRAINT c` or infers a dropped unique constraint as its conflict arbiter, its inserts fail immediately. This rule is enabled by default. ```sql ALTER TABLE t DROP CONSTRAINT IF EXISTS c; diff --git a/docs/docs/ban-drop-function.md b/docs/docs/ban-drop-function.md index e2aae09e..c3fbdcf4 100644 --- a/docs/docs/ban-drop-function.md +++ b/docs/docs/ban-drop-function.md @@ -5,7 +5,7 @@ title: ban-drop-function ## problem -Dropping a function, procedure, or routine may break existing clients. Calls can fail with `42883 undefined_function`. +Dropping a function, procedure, routine, or aggregate may break existing clients. Calls can fail with `42883 undefined_function`. ## solution diff --git a/docs/docs/ban-drop-type.md b/docs/docs/ban-drop-type.md index f5b9201e..dfff6eb5 100644 --- a/docs/docs/ban-drop-type.md +++ b/docs/docs/ban-drop-type.md @@ -5,7 +5,7 @@ title: ban-drop-type ## problem -Dropping a type or cast may break existing clients. Casts and parameters that name the type can fail with `42704 undefined_object`. `CASCADE` can also drop columns of that type. +Dropping a type, cast, operator, operator class, or operator family may break existing clients. Casts and parameters that name the type can fail with `42704 undefined_object`. `CASCADE` can also drop columns of that type. ## solution diff --git a/docs/docs/ban-revoke.md b/docs/docs/ban-revoke.md index 35e88203..b677c24f 100644 --- a/docs/docs/ban-revoke.md +++ b/docs/docs/ban-revoke.md @@ -5,7 +5,7 @@ title: ban-revoke ## problem -Revoking privileges, dropping a role, or running `DROP OWNED` can make client queries fail. `DROP OWNED` can also remove objects owned by the role. This rule is opt-in. +Revoking privileges, dropping a role or user, changing object ownership with `OWNER TO` or `REASSIGN OWNED`, or running `DROP OWNED` can make client queries fail. `DROP OWNED` can also remove objects owned by the role. This rule is opt-in. ```sql REVOKE SELECT ON t FROM app; diff --git a/docs/docs/ban-set-schema.md b/docs/docs/ban-set-schema.md index 537284e7..8276ab0e 100644 --- a/docs/docs/ban-set-schema.md +++ b/docs/docs/ban-set-schema.md @@ -5,7 +5,7 @@ title: ban-set-schema ## problem -Clients that use schema-qualified names cannot find objects after `SET SCHEMA`. This includes procedures and routines. +Clients that use schema-qualified names cannot find objects after `SET SCHEMA`. This includes foreign tables, procedures, and routines. ```sql ALTER TABLE t SET SCHEMA s; diff --git a/docs/docs/changing-column-type.md b/docs/docs/changing-column-type.md index a8515550..2a8c3adf 100644 --- a/docs/docs/changing-column-type.md +++ b/docs/docs/changing-column-type.md @@ -7,8 +7,7 @@ title: changing-column-type Changing a column type requires an `ACCESS EXCLUSIVE` lock on the table which blocks reads and writes while the table is rewritten. -Changing the type of the column may also break other clients reading from the -table. +Changing the type of a table or foreign table column or a composite type attribute may also break clients that read its values. diff --git a/docs/docs/renaming-column.md b/docs/docs/renaming-column.md index c9cc4851..bf4aea75 100644 --- a/docs/docs/renaming-column.md +++ b/docs/docs/renaming-column.md @@ -5,7 +5,7 @@ title: renaming-column ## problem -Renaming a table, foreign table, view, or materialized view column may break existing clients. +Renaming a table, foreign table, view, or materialized view column or a composite type attribute may break existing clients. ## solution diff --git a/docs/docs/renaming-object.md b/docs/docs/renaming-object.md index 50b4f344..5eb2c73d 100644 --- a/docs/docs/renaming-object.md +++ b/docs/docs/renaming-object.md @@ -5,7 +5,7 @@ title: renaming-object ## problem -Clients that use an old object name or enum value can fail after a rename. This includes foreign tables and routines. +Clients that use an old object name or enum value can fail after a rename. This includes foreign tables, routines, roles, users, groups, databases, triggers, policies, and table or domain constraints. ```sql ALTER VIEW v RENAME TO v2; From c48c8c7691eadeef97c4d6ceee76426685caccbd Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:47:19 +1100 Subject: [PATCH 12/21] fix(linter): check foreign table writes and keep cli test config local --- crates/squawk/tests/example_output.rs | 5 ++ .../squawk/tests/fixtures/empty.squawk.toml | 1 + .../src/rules/ban_drop_column.rs | 9 ++- .../src/rules/ban_drop_constraint.rs | 14 ++++- .../squawk_linter/src/rules/ban_drop_type.rs | 18 +++++- crates/squawk_linter/src/rules/ban_revoke.rs | 6 +- .../src/rules/changing_column_type.rs | 9 ++- .../src/rules/compatibility_additions.rs | 36 +++++++++++ .../src/rules/renaming_column.rs | 6 +- .../src/rules/renaming_object.rs | 58 +++++++++++++++--- .../squawk_linter/src/rules/variant_tests.rs | 59 +++++++++++++++---- docs/docs/ban-new-write-restriction.md | 2 +- 12 files changed, 192 insertions(+), 31 deletions(-) create mode 100644 crates/squawk/tests/fixtures/empty.squawk.toml diff --git a/crates/squawk/tests/example_output.rs b/crates/squawk/tests/example_output.rs index 79f3cdda..7cb44a4d 100644 --- a/crates/squawk/tests/example_output.rs +++ b/crates/squawk/tests/example_output.rs @@ -7,6 +7,11 @@ fn example_sql_svg() { Command::new(bin_path) .env("CLICOLOR_FORCE", "1") .env("SQUAWK_DISABLE_GITHUB_ANNOTATIONS", "1") + .arg("--config") + .arg(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/empty.squawk.toml" + )) .arg("../../example.sql") .assert() .code(1) // squawk returns 1 when it finds violations diff --git a/crates/squawk/tests/fixtures/empty.squawk.toml b/crates/squawk/tests/fixtures/empty.squawk.toml new file mode 100644 index 00000000..a0ce96a2 --- /dev/null +++ b/crates/squawk/tests/fixtures/empty.squawk.toml @@ -0,0 +1 @@ +# This fixture prevents the CLI integration test from reading an ancestor config. diff --git a/crates/squawk_linter/src/rules/ban_drop_column.rs b/crates/squawk_linter/src/rules/ban_drop_column.rs index 5f458e33..e44f9ec8 100644 --- a/crates/squawk_linter/src/rules/ban_drop_column.rs +++ b/crates/squawk_linter/src/rules/ban_drop_column.rs @@ -12,10 +12,15 @@ pub(crate) fn ban_drop_column(ctx: &mut Linter, parse: &Parse) { ast::Stmt::AlterTable(table) => table.actions().collect(), ast::Stmt::AlterForeignTable(table) => table.actions().collect(), ast::Stmt::AlterType(ty) => { - if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() { + if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() + { for action in list.actions() { if let ast::AlterTypeAttributeAction::DropAttribute(node) = action { - ctx.report(Violation::for_node(Rule::BanDropColumn, "Dropping an attribute may break existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::BanDropColumn, + "Dropping an attribute may break existing clients.".into(), + node.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/ban_drop_constraint.rs b/crates/squawk_linter/src/rules/ban_drop_constraint.rs index 85a28fd8..175c9ab8 100644 --- a/crates/squawk_linter/src/rules/ban_drop_constraint.rs +++ b/crates/squawk_linter/src/rules/ban_drop_constraint.rs @@ -13,7 +13,12 @@ pub(crate) fn ban_drop_constraint(ctx: &mut Linter, parse: &Parse) { }; for action in actions { match action { - ast::AlterTableAction::DropConstraint(node) => ctx.report(Violation::for_node(Rule::BanDropConstraint, "Dropping a constraint may remove a guarantee that existing clients assume.".into(), node.syntax())), + ast::AlterTableAction::DropConstraint(node) => ctx.report(Violation::for_node( + Rule::BanDropConstraint, + "Dropping a constraint may remove a guarantee that existing clients assume." + .into(), + node.syntax(), + )), ast::AlterTableAction::AlterConstraint(node) => { for option in node.constraint_options() { if let ast::ConstraintOption::NotEnforced(option) = option { @@ -26,7 +31,12 @@ pub(crate) fn ban_drop_constraint(ctx: &mut Linter, parse: &Parse) { } if let ast::Stmt::AlterDomain(domain) = stmt { if let Some(ast::AlterDomainAction::DropConstraint(node)) = domain.action() { - ctx.report(Violation::for_node(Rule::BanDropConstraint, "Dropping a constraint may remove a guarantee that existing clients assume.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::BanDropConstraint, + "Dropping a constraint may remove a guarantee that existing clients assume." + .into(), + node.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/ban_drop_type.rs b/crates/squawk_linter/src/rules/ban_drop_type.rs index 6345028d..09bdaa75 100644 --- a/crates/squawk_linter/src/rules/ban_drop_type.rs +++ b/crates/squawk_linter/src/rules/ban_drop_type.rs @@ -8,11 +8,23 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_type(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { if let ast::Stmt::DropOperator(node) = stmt { - ctx.report(Violation::for_node(Rule::BanDropType, "Dropping an operator may break existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::BanDropType, + "Dropping an operator may break existing clients.".into(), + node.syntax(), + )); } else if let ast::Stmt::DropOperatorClass(node) = stmt { - ctx.report(Violation::for_node(Rule::BanDropType, "Dropping an operator class may break existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::BanDropType, + "Dropping an operator class may break existing clients.".into(), + node.syntax(), + )); } else if let ast::Stmt::DropOperatorFamily(node) = stmt { - ctx.report(Violation::for_node(Rule::BanDropType, "Dropping an operator family may break existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::BanDropType, + "Dropping an operator family may break existing clients.".into(), + node.syntax(), + )); } else if let ast::Stmt::DropCast(node) = stmt { ctx.report(Violation::for_node( Rule::BanDropType, diff --git a/crates/squawk_linter/src/rules/ban_revoke.rs b/crates/squawk_linter/src/rules/ban_revoke.rs index e53a77f9..0480ded8 100644 --- a/crates/squawk_linter/src/rules/ban_revoke.rs +++ b/crates/squawk_linter/src/rules/ban_revoke.rs @@ -7,7 +7,11 @@ use squawk_syntax::{ pub(crate) fn ban_revoke(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { for owner in stmt.syntax().descendants().filter_map(ast::OwnerTo::cast) { - ctx.report(Violation::for_node(Rule::BanRevoke, "Changing object ownership may break existing clients.".into(), owner.syntax())); + ctx.report(Violation::for_node( + Rule::BanRevoke, + "Changing object ownership may break existing clients.".into(), + owner.syntax(), + )); } match stmt { ast::Stmt::Revoke(node) => { diff --git a/crates/squawk_linter/src/rules/changing_column_type.rs b/crates/squawk_linter/src/rules/changing_column_type.rs index c731a141..84b38948 100644 --- a/crates/squawk_linter/src/rules/changing_column_type.rs +++ b/crates/squawk_linter/src/rules/changing_column_type.rs @@ -12,10 +12,15 @@ pub(crate) fn changing_column_type(ctx: &mut Linter, parse: &Parse) ast::Stmt::AlterTable(table) => table.actions().collect(), ast::Stmt::AlterForeignTable(table) => table.actions().collect(), ast::Stmt::AlterType(ty) => { - if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() { + if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() + { for action in list.actions() { if let ast::AlterTypeAttributeAction::AlterAttribute(node) = action { - ctx.report(Violation::for_node(Rule::ChangingColumnType, "Changing an attribute type may break existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::ChangingColumnType, + "Changing an attribute type may break existing clients.".into(), + node.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/compatibility_additions.rs b/crates/squawk_linter/src/rules/compatibility_additions.rs index 080ec4cc..54f55052 100644 --- a/crates/squawk_linter/src/rules/compatibility_additions.rs +++ b/crates/squawk_linter/src/rules/compatibility_additions.rs @@ -135,6 +135,33 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse + { + for action in table.actions() { + match action { + ast::AlterTableAction::AddConstraint(node) => { + ctx.report(Violation::for_node( + Rule::BanNewWriteRestriction, + "A foreign table constraint can reject existing client writes." + .into(), + node.syntax(), + )) + } + ast::AlterTableAction::AlterColumn(column) => { + if let Some(ast::AlterColumnOption::SetNotNull(node)) = column.option() + { + ctx.report(Violation::for_node( + Rule::BanNewWriteRestriction, + "SET NOT NULL can reject existing client writes.".into(), + node.syntax(), + )); + } + } + _ => (), + } + } + } ast::Stmt::AlterDomain(domain) if ctx.rules.contains(&Rule::BanNewWriteRestriction) => { if let Some(action) = domain.action() { match action { @@ -257,6 +284,15 @@ mod test { "ALTER TABLE t ALTER CONSTRAINT fk NOT ENFORCED;", Rule::BanNewWriteRestriction, ); + assert_eq!( + lint_errors( + "ALTER FOREIGN TABLE ft ALTER COLUMN c SET NOT NULL;", + Rule::BanNewWriteRestriction + ) + .matches("warning[ban-new-write-restriction]") + .count(), + 1 + ); } #[test] diff --git a/crates/squawk_linter/src/rules/renaming_column.rs b/crates/squawk_linter/src/rules/renaming_column.rs index 2dc90dc8..242daf5e 100644 --- a/crates/squawk_linter/src/rules/renaming_column.rs +++ b/crates/squawk_linter/src/rules/renaming_column.rs @@ -11,7 +11,11 @@ pub(crate) fn renaming_column(ctx: &mut Linter, parse: &Parse) { match stmt { ast::Stmt::AlterType(ty) => { if let Some(ast::AlterTypeAction::RenameAttribute(node)) = ty.action() { - ctx.report(Violation::for_node(Rule::RenamingColumn, "Renaming an attribute may break existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingColumn, + "Renaming an attribute may break existing clients.".into(), + node.syntax(), + )); } } ast::Stmt::AlterTable(table) => { diff --git a/crates/squawk_linter/src/rules/renaming_object.rs b/crates/squawk_linter/src/rules/renaming_object.rs index 3b9deb2a..bbed162f 100644 --- a/crates/squawk_linter/src/rules/renaming_object.rs +++ b/crates/squawk_linter/src/rules/renaming_object.rs @@ -21,38 +21,66 @@ pub(crate) fn renaming_object(ctx: &mut Linter, parse: &Parse) { ast::Stmt::AlterTable(node) => { for action in node.actions() { if let ast::AlterTableAction::RenameConstraint(node) = action { - ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a constraint may break existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a constraint may break existing clients.".into(), + node.syntax(), + )); } } } ast::Stmt::AlterRole(node) => { if let Some(ast::AlterRoleAction::RoleRenameTo(action)) = node.action() { - ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a role may break existing clients.".into(), action.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a role may break existing clients.".into(), + action.syntax(), + )); } } ast::Stmt::AlterUser(node) => { if let Some(ast::AlterUserAction::RoleRenameTo(action)) = node.action() { - ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a user may break existing clients.".into(), action.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a user may break existing clients.".into(), + action.syntax(), + )); } } ast::Stmt::AlterGroup(node) => { if let Some(ast::AlterGroupAction::RoleRenameTo(action)) = node.action() { - ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a group may break existing clients.".into(), action.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a group may break existing clients.".into(), + action.syntax(), + )); } } ast::Stmt::AlterDatabase(node) => { if let Some(ast::AlterDatabaseAction::DatabaseRenameTo(action)) = node.action() { - ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a database may break existing clients.".into(), action.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a database may break existing clients.".into(), + action.syntax(), + )); } } ast::Stmt::AlterTrigger(node) => { if let Some(ast::AlterTriggerAction::TriggerRenameTo(action)) = node.action() { - ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a trigger may break existing clients.".into(), action.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a trigger may break existing clients.".into(), + action.syntax(), + )); } } ast::Stmt::AlterPolicy(node) => { if let Some(ast::AlterPolicyAction::PolicyRenameTo(action)) = node.action() { - ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a policy may break existing clients.".into(), action.syntax())); + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a policy may break existing clients.".into(), + action.syntax(), + )); } } ast::Stmt::AlterRoutine(node) => { @@ -152,8 +180,20 @@ pub(crate) fn renaming_object(ctx: &mut Linter, parse: &Parse) { ast::Stmt::AlterDomain(node) => { for action in node.action().into_iter() { match action { - ast::AlterDomainAction::DomainRenameTo(node) => ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a domain may break existing clients.".into(), node.syntax())), - ast::AlterDomainAction::RenameConstraint(node) => ctx.report(Violation::for_node(Rule::RenamingObject, "Renaming a constraint may break existing clients.".into(), node.syntax())), + ast::AlterDomainAction::DomainRenameTo(node) => { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a domain may break existing clients.".into(), + node.syntax(), + )) + } + ast::AlterDomainAction::RenameConstraint(node) => { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming a constraint may break existing clients.".into(), + node.syntax(), + )) + } _ => (), } } diff --git a/crates/squawk_linter/src/rules/variant_tests.rs b/crates/squawk_linter/src/rules/variant_tests.rs index 6bb11c36..0962db0a 100644 --- a/crates/squawk_linter/src/rules/variant_tests.rs +++ b/crates/squawk_linter/src/rules/variant_tests.rs @@ -6,7 +6,11 @@ use crate::{ fn check(sql: &str, rule: Rule, count: usize) { for statement in sql.split(';').filter(|s| !s.trim().is_empty()) { let parsed = squawk_syntax::SourceFile::parse(&format!("{statement};")); - assert!(parsed.errors().is_empty(), "{statement}: {:?}", parsed.errors()); + assert!( + parsed.errors().is_empty(), + "{statement}: {:?}", + parsed.errors() + ); } let errors = lint_errors(sql, rule); assert_eq!(errors.matches("warning[").count(), count, "{errors}"); @@ -29,24 +33,55 @@ fn renames() { #[test] fn remaining_renames() { - check("ALTER TABLE t RENAME CONSTRAINT old TO renamed; ALTER DOMAIN d RENAME CONSTRAINT old TO renamed; ALTER ROLE app RENAME TO app2; ALTER USER app RENAME TO app2; ALTER GROUP app RENAME TO app2; ALTER DATABASE db RENAME TO db2; ALTER TRIGGER tr ON t RENAME TO tr2; ALTER POLICY p ON t RENAME TO p2;", Rule::RenamingObject, 8); - check("ALTER TYPE composite RENAME ATTRIBUTE old TO renamed;", Rule::RenamingColumn, 1); + check( + "ALTER TABLE t RENAME CONSTRAINT old TO renamed; ALTER DOMAIN d RENAME CONSTRAINT old TO renamed; ALTER ROLE app RENAME TO app2; ALTER USER app RENAME TO app2; ALTER GROUP app RENAME TO app2; ALTER DATABASE db RENAME TO db2; ALTER TRIGGER tr ON t RENAME TO tr2; ALTER POLICY p ON t RENAME TO p2;", + Rule::RenamingObject, + 8, + ); + check( + "ALTER TYPE composite RENAME ATTRIBUTE old TO renamed;", + Rule::RenamingColumn, + 1, + ); lint_ok("ALTER POLICY p ON t USING (true);", Rule::RenamingObject); } #[test] fn remaining_constraints_and_attributes() { - check("ALTER DOMAIN d DROP CONSTRAINT c; ALTER TABLE t ALTER CONSTRAINT c NOT ENFORCED; ALTER FOREIGN TABLE ft DROP CONSTRAINT c;", Rule::BanDropConstraint, 3); - check("ALTER TYPE composite DROP ATTRIBUTE a; ALTER FOREIGN TABLE ft DROP COLUMN a;", Rule::BanDropColumn, 2); - check("ALTER TYPE composite ALTER ATTRIBUTE a TYPE text; ALTER FOREIGN TABLE ft ALTER COLUMN a TYPE text;", Rule::ChangingColumnType, 2); - lint_ok("ALTER TABLE t ALTER CONSTRAINT c ENFORCED;", Rule::BanDropConstraint); + check( + "ALTER DOMAIN d DROP CONSTRAINT c; ALTER TABLE t ALTER CONSTRAINT c NOT ENFORCED; ALTER FOREIGN TABLE ft DROP CONSTRAINT c;", + Rule::BanDropConstraint, + 3, + ); + check( + "ALTER TYPE composite DROP ATTRIBUTE a; ALTER FOREIGN TABLE ft DROP COLUMN a;", + Rule::BanDropColumn, + 2, + ); + check( + "ALTER TYPE composite ALTER ATTRIBUTE a TYPE text; ALTER FOREIGN TABLE ft ALTER COLUMN a TYPE text;", + Rule::ChangingColumnType, + 2, + ); + lint_ok( + "ALTER TABLE t ALTER CONSTRAINT c ENFORCED;", + Rule::BanDropConstraint, + ); } #[test] fn remaining_drops_and_ownership() { check("DROP AGGREGATE agg(int);", Rule::BanDropFunction, 1); - check("DROP OPERATOR + (int, int); DROP OPERATOR CLASS op USING btree; DROP OPERATOR FAMILY fam USING btree;", Rule::BanDropType, 3); - check("ALTER FOREIGN TABLE ft OWNER TO app; ALTER DOMAIN d OWNER TO app;", Rule::BanRevoke, 2); + check( + "DROP OPERATOR + (int, int); DROP OPERATOR CLASS op USING btree; DROP OPERATOR FAMILY fam USING btree;", + Rule::BanDropType, + 3, + ); + check( + "ALTER FOREIGN TABLE ft OWNER TO app; ALTER DOMAIN d OWNER TO app;", + Rule::BanRevoke, + 2, + ); lint_ok("ALTER TABLE t SET SCHEMA s;", Rule::BanRevoke); } @@ -82,7 +117,11 @@ fn drops_and_revokes() { check("DROP ROUTINE IF EXISTS f(int);", Rule::BanDropFunction, 1); check("DROP CAST IF EXISTS (text AS int);", Rule::BanDropType, 1); check("DROP OWNED BY app; DROP ROLE app;", Rule::BanRevoke, 2); - check("REASSIGN OWNED BY app TO admin; ALTER TABLE t OWNER TO admin; DROP USER app;", Rule::BanRevoke, 3); + check( + "REASSIGN OWNED BY app TO admin; ALTER TABLE t OWNER TO admin; DROP USER app;", + Rule::BanRevoke, + 3, + ); } #[test] diff --git a/docs/docs/ban-new-write-restriction.md b/docs/docs/ban-new-write-restriction.md index 21fdeb6f..9fde724a 100644 --- a/docs/docs/ban-new-write-restriction.md +++ b/docs/docs/ban-new-write-restriction.md @@ -5,7 +5,7 @@ title: ban-new-write-restriction ## problem -New table and domain constraints, `NOT VALID` constraints, `SET NOT NULL`, constrained added columns, changed constraint timing, and unique indexes (including `CONCURRENTLY`) can reject writes from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that table. The rule does not know whether an object existed before the migration. +New table, foreign-table, and domain constraints, `NOT VALID` constraints, `SET NOT NULL`, constrained added columns, changed constraint timing, and unique indexes (including `CONCURRENTLY`) can reject writes from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that table. The rule does not know whether an object existed before the migration. ```sql ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0) NOT VALID; From bf715a48ca5723f989a6137953fb4fd5cc8e1684 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:49:01 +1100 Subject: [PATCH 13/21] fix(linter): clarify replica-only firing and enforcement diagnostics --- crates/squawk_linter/src/lib.rs | 13 +++++++ .../src/rules/ban_disable_trigger.rs | 35 ++++++++++++------- ...rules__ban_disable_trigger__test__err.snap | 10 +++--- docs/docs/ban-disable-trigger.md | 2 +- 4 files changed, 41 insertions(+), 19 deletions(-) diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 0a1c3e42..9fb84a57 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -929,6 +929,19 @@ mod tests { ); } + #[test] + fn compatibility_rule_respects_ignore_comment() { + let sql = "-- squawk-ignore ban-add-enum-value\nALTER TYPE mood ADD VALUE 'new';"; + let parse = SourceFile::parse(sql); + assert!(parse.errors().is_empty()); + assert!( + !Linter::with_rules(&[Rule::BanAddEnumValue], &[]) + .lint(&parse, sql) + .iter() + .any(|v| v.code == Rule::BanAddEnumValue) + ); + } + #[test] fn require_timeout_settings_expands_to_granular_rules() { let linter = Linter::from([Rule::RequireTimeoutSettings]); diff --git a/crates/squawk_linter/src/rules/ban_disable_trigger.rs b/crates/squawk_linter/src/rules/ban_disable_trigger.rs index f6e98c1f..fd0a4531 100644 --- a/crates/squawk_linter/src/rules/ban_disable_trigger.rs +++ b/crates/squawk_linter/src/rules/ban_disable_trigger.rs @@ -8,20 +8,29 @@ pub(crate) fn ban_disable_trigger(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { if let ast::Stmt::AlterTable(table) = stmt { for action in table.actions() { - if matches!( - action, + let message = match action { + ast::AlterTableAction::EnableReplicaTrigger(_) + | ast::AlterTableAction::EnableReplicaRule(_) => { + "Replica-only triggers and rules do not fire for normal application writes." + } ast::AlterTableAction::DisableTrigger(_) - | ast::AlterTableAction::DisableRule(_) - | ast::AlterTableAction::DisableRls(_) - | ast::AlterTableAction::ForceRls(_) - | ast::AlterTableAction::NoForceRls(_) - | ast::AlterTableAction::EnableReplicaTrigger(_) - | ast::AlterTableAction::EnableReplicaRule(_) - | ast::AlterTableAction::EnableAlwaysTrigger(_) - | ast::AlterTableAction::EnableAlwaysRule(_) - ) { - ctx.report(Violation::for_node(Rule::BanDisableTrigger, "Disabling a trigger, rule, or row level security may silently change behaviour for existing clients.".into(), action.syntax())); - } + | ast::AlterTableAction::DisableRule(_) + | ast::AlterTableAction::EnableAlwaysTrigger(_) + | ast::AlterTableAction::EnableAlwaysRule(_) => { + "Changing trigger or rule firing may change database side effects for existing clients." + } + ast::AlterTableAction::DisableRls(_) + | ast::AlterTableAction::ForceRls(_) + | ast::AlterTableAction::NoForceRls(_) => { + "Changing row level security can change visible rows or reject access for existing clients." + } + _ => continue, + }; + ctx.report(Violation::for_node( + Rule::BanDisableTrigger, + message.into(), + action.syntax(), + )); } } } diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap index 742e6ba5..b65f377a 100644 --- a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap @@ -1,20 +1,20 @@ --- source: crates/squawk_linter/src/rules/ban_disable_trigger.rs -expression: "lint_errors(sql, Rule::BanDisableTrigger)" +expression: errors --- -warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. +warning[ban-disable-trigger]: Changing trigger or rule firing may change database side effects for existing clients. ╭▸ 1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… ╰╴ ━━━━━━━━━━━━━━━━━━━ -warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. +warning[ban-disable-trigger]: Changing trigger or rule firing may change database side effects for existing clients. ╭▸ 1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… ╰╴ ━━━━━━━━━━━━━━ -warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. +warning[ban-disable-trigger]: Changing row level security can change visible rows or reject access for existing clients. ╭▸ 1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━ -warning[ban-disable-trigger]: Disabling a trigger, rule, or row level security may silently change behaviour for existing clients. +warning[ban-disable-trigger]: Changing row level security can change visible rows or reject access for existing clients. ╭▸ 1 │ …LTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVEL SECURITY; ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/docs/docs/ban-disable-trigger.md b/docs/docs/ban-disable-trigger.md index 199b08db..26164402 100644 --- a/docs/docs/ban-disable-trigger.md +++ b/docs/docs/ban-disable-trigger.md @@ -5,7 +5,7 @@ title: ban-disable-trigger ## problem -Disabling a trigger or rule, enabling a replica or always trigger or rule, or changing row level security enforcement (including `NO FORCE ROW LEVEL SECURITY`) can change behaviour without a client error. This rule is opt-in. +Disabling a trigger or rule, enabling replica-only or always firing, or changing row level security enforcement (including `NO FORCE ROW LEVEL SECURITY`) can change data or reject access. `ENABLE REPLICA TRIGGER` and `ENABLE REPLICA RULE` stop firing in normal application sessions. Some enforcement changes cause explicit errors. This rule is opt-in. ```sql ALTER TABLE t DISABLE TRIGGER trg; From 75f92049bae75c32aa2b0cbe3cf003aa834b70d9 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 13:55:21 +1100 Subject: [PATCH 14/21] fix(linter): describe generated expression value changes --- .../squawk_linter/src/rules/ban_alter_generated_expression.rs | 2 +- ...nter__rules__ban_alter_generated_expression__test__err.snap | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs index 7a49f85b..b43e0959 100644 --- a/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs +++ b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs @@ -11,7 +11,7 @@ pub(crate) fn ban_alter_generated_expression(ctx: &mut Linter, parse: &Parse { if let Some(ast::AlterColumnOption::SetExpression(node)) = column.option() { - ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Changing a generated column may break inserts from existing clients.".into(), node.syntax())); + ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Changing a generated column expression may change values read by existing clients.".into(), node.syntax())); } } ast::AlterTableAction::AddColumn(column) => { diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap index 13c633cd..71d3a89e 100644 --- a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap @@ -1,9 +1,8 @@ --- source: crates/squawk_linter/src/rules/ban_alter_generated_expression.rs -assertion_line: 41 expression: "lint_errors(sql, Rule::BanAlterGeneratedExpression)" --- -warning[ban-alter-generated-expression]: Changing a generated column may break inserts from existing clients. +warning[ban-alter-generated-expression]: Changing a generated column expression may change values read by existing clients. ╭▸ 1 │ ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 2); ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━ From 113084fbd44ebc981af0d04ab4c599d2872323b1 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 14:02:08 +1100 Subject: [PATCH 15/21] fix(linter): use matches macro for new constraints --- crates/squawk_linter/src/lib.rs | 14 ++++++++++---- .../src/rules/compatibility_additions.rs | 14 +++++++------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 9fb84a57..08c2d2a3 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -824,10 +824,16 @@ mod tests { #[test] fn new_opt_in_rules_only_report_when_included() { - for (rule, sql) in [( - Rule::BanAlterGeneratedExpression, - "ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;", - )] { + for (rule, sql) in [ + ( + Rule::BanAlterGeneratedExpression, + "ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;", + ), + ( + Rule::BanAlterGeneratedExpression, + "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1);", + ), + ] { let parse = SourceFile::parse(sql); assert!(parse.errors().is_empty()); assert!( diff --git a/crates/squawk_linter/src/rules/compatibility_additions.rs b/crates/squawk_linter/src/rules/compatibility_additions.rs index 54f55052..0dd1a658 100644 --- a/crates/squawk_linter/src/rules/compatibility_additions.rs +++ b/crates/squawk_linter/src/rules/compatibility_additions.rs @@ -45,14 +45,14 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse { if let Some(constraint) = add.constraint() { - let restriction = match &constraint { + let restriction = matches!( + constraint, ast::Constraint::CheckConstraint(_) - | ast::Constraint::ForeignKeyConstraint(_) - | ast::Constraint::UniqueConstraint(_) - | ast::Constraint::PrimaryKeyConstraint(_) - | ast::Constraint::ExcludeConstraint(_) => true, - _ => false, - }; + | ast::Constraint::ForeignKeyConstraint(_) + | ast::Constraint::UniqueConstraint(_) + | ast::Constraint::PrimaryKeyConstraint(_) + | ast::Constraint::ExcludeConstraint(_) + ); if restriction { ctx.report(Violation::for_node(Rule::BanNewWriteRestriction, "A new constraint can reject writes from existing clients, even when it is NOT VALID.".into(), add.syntax())); From ee799340bfe9e04975aa447aa241699f78292661 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 14:33:58 +1100 Subject: [PATCH 16/21] fix(linter): cover remaining compatibility statement variants --- crates/squawk_linter/src/rules/ban_revoke.rs | 9 +++ .../src/rules/compatibility_additions.rs | 45 ++++++++++---- .../src/rules/security_compatibility.rs | 60 +++++++++++++++++++ .../squawk_linter/src/rules/variant_tests.rs | 49 +++++++++++++++ docs/docs/ban-alter-function-options.md | 4 +- docs/docs/ban-alter-role-options.md | 3 +- docs/docs/ban-new-write-restriction.md | 5 +- docs/docs/ban-revoke.md | 3 +- 8 files changed, 162 insertions(+), 16 deletions(-) diff --git a/crates/squawk_linter/src/rules/ban_revoke.rs b/crates/squawk_linter/src/rules/ban_revoke.rs index 0480ded8..88868d72 100644 --- a/crates/squawk_linter/src/rules/ban_revoke.rs +++ b/crates/squawk_linter/src/rules/ban_revoke.rs @@ -41,6 +41,15 @@ pub(crate) fn ban_revoke(ctx: &mut Linter, parse: &Parse) { "Dropping a role may break existing clients.".into(), node.syntax(), )), + ast::Stmt::AlterGroup(node) => { + if let Some(ast::AlterGroupAction::DropUsers(users)) = node.action() { + ctx.report(Violation::for_node( + Rule::BanRevoke, + "Removing role membership may break existing clients.".into(), + users.syntax(), + )); + } + } ast::Stmt::AlterDefaultPrivileges(node) => { if matches!( node.action(), diff --git a/crates/squawk_linter/src/rules/compatibility_additions.rs b/crates/squawk_linter/src/rules/compatibility_additions.rs index 0dd1a658..9ed9007e 100644 --- a/crates/squawk_linter/src/rules/compatibility_additions.rs +++ b/crates/squawk_linter/src/rules/compatibility_additions.rs @@ -52,6 +52,7 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse { - for constraint in column.constraints() { - if matches!( - constraint, - ast::Constraint::NotNullConstraint(_) - | ast::Constraint::ReferencesConstraint(_) - | ast::Constraint::CheckConstraint(_) - | ast::Constraint::UniqueConstraint(_) - ) { - ctx.report(Violation::for_node(Rule::BanNewWriteRestriction, - "A new column constraint can reject writes from existing clients.".into(), constraint.syntax())); - } - } + check_column_constraints(ctx, column); } ast::AlterTableAction::AlterConstraint(node) => { if node.constraint_options().any(|option| { @@ -140,6 +130,9 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse { + check_column_constraints(ctx, &column); + } ast::AlterTableAction::AddConstraint(node) => { ctx.report(Violation::for_node( Rule::BanNewWriteRestriction, @@ -233,6 +226,25 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse ); } } + ast::Stmt::AlterProcedure(node) => { + if let Some(ast::AlterProcedureAction::FuncOptionList(options)) = node.action() { + report( + ctx, + Rule::BanAlterFunctionOptions, + "Changing procedure options may change behaviour for existing clients.", + options.syntax(), + ); + } + } + ast::Stmt::AlterRoutine(node) => { + if let Some(ast::AlterRoutineAction::FuncOptionList(options)) = node.action() { + report( + ctx, + Rule::BanAlterFunctionOptions, + "Changing routine options may change behaviour for existing clients.", + options.syntax(), + ); + } + } ast::Stmt::AlterView(node) => { if let Some(action) = node.action() { match action { @@ -101,6 +121,31 @@ pub(crate) fn security_compatibility(ctx: &mut Linter, parse: &Parse } } } + ast::Stmt::AlterUser(node) => { + if let Some(action) = node.action() { + match action { + ast::AlterUserAction::RoleOptionList(options) => report( + ctx, + Rule::BanAlterRoleOptions, + "Changing role options may change access for existing clients.", + options.syntax(), + ), + ast::AlterUserAction::SetConfigParam(config) => report( + ctx, + Rule::BanAlterRoleOptions, + "Changing role configuration may change behaviour for existing clients.", + config.syntax(), + ), + ast::AlterUserAction::ResetConfigParam(config) => report( + ctx, + Rule::BanAlterRoleOptions, + "Changing role configuration may change behaviour for existing clients.", + config.syntax(), + ), + _ => {} + } + } + } ast::Stmt::AlterDatabase(node) => { if let Some(action) = node.action() { match action { @@ -200,6 +245,21 @@ mod tests { "ALTER FUNCTION f() SECURITY DEFINER;", "ALTER FUNCTION f() RENAME TO g;", ), + ( + Rule::BanAlterFunctionOptions, + "ALTER PROCEDURE p() SECURITY DEFINER;", + "ALTER PROCEDURE p() RENAME TO q;", + ), + ( + Rule::BanAlterFunctionOptions, + "ALTER ROUTINE f() SET search_path TO private;", + "ALTER ROUTINE f() SET SCHEMA private;", + ), + ( + Rule::BanAlterRoleOptions, + "ALTER USER app NOLOGIN;", + "ALTER USER app RENAME TO app2;", + ), ( Rule::BanAlterViewOptions, "ALTER VIEW v SET (security_barrier = true);", diff --git a/crates/squawk_linter/src/rules/variant_tests.rs b/crates/squawk_linter/src/rules/variant_tests.rs index 0962db0a..ab4aa23e 100644 --- a/crates/squawk_linter/src/rules/variant_tests.rs +++ b/crates/squawk_linter/src/rules/variant_tests.rs @@ -137,6 +137,55 @@ fn replacements() { ); } +#[test] +fn routine_options() { + check( + "ALTER PROCEDURE p() SECURITY DEFINER; ALTER PROCEDURE p() SET search_path TO private; ALTER PROCEDURE p() RESET ALL; ALTER ROUTINE f() SECURITY INVOKER; ALTER ROUTINE f() SET search_path TO private; ALTER ROUTINE f() RESET ALL;", + Rule::BanAlterFunctionOptions, + 6, + ); + lint_ok( + "ALTER PROCEDURE p() RENAME TO q; ALTER ROUTINE f() SET SCHEMA private; ALTER PROCEDURE p() OWNER TO app;", + Rule::BanAlterFunctionOptions, + ); +} + +#[test] +fn user_options() { + check( + "ALTER USER app NOLOGIN; ALTER USER app NOBYPASSRLS; ALTER USER app IN DATABASE db SET search_path TO private; ALTER USER app RESET ALL;", + Rule::BanAlterRoleOptions, + 4, + ); + lint_ok("ALTER USER app RENAME TO app2;", Rule::BanAlterRoleOptions); +} + +#[test] +fn group_membership_removal() { + check( + "ALTER GROUP writers DROP USER app, worker;", + Rule::BanRevoke, + 1, + ); + lint_ok( + "ALTER GROUP writers ADD USER app; ALTER GROUP writers RENAME TO editors;", + Rule::BanRevoke, + ); +} + +#[test] +fn additional_write_constraint_forms() { + check( + "ALTER TABLE t ADD CONSTRAINT id_required NOT NULL id; ALTER TABLE t ADD COLUMN c bigint PRIMARY KEY; ALTER FOREIGN TABLE ft ADD COLUMN c int NOT NULL;", + Rule::BanNewWriteRestriction, + 3, + ); + lint_ok( + "CREATE TABLE t (id bigint); ALTER TABLE t ADD CONSTRAINT id_required NOT NULL id; ALTER TABLE t ADD COLUMN c bigint PRIMARY KEY; ALTER FOREIGN TABLE ft ADD COLUMN c int;", + Rule::BanNewWriteRestriction, + ); +} + #[test] fn generated_expression() { check( diff --git a/docs/docs/ban-alter-function-options.md b/docs/docs/ban-alter-function-options.md index f5e84c4c..2fa7d3c2 100644 --- a/docs/docs/ban-alter-function-options.md +++ b/docs/docs/ban-alter-function-options.md @@ -3,10 +3,12 @@ id: ban-alter-function-options title: ban-alter-function-options --- -`ALTER FUNCTION` options can change execution behaviour or security for existing callers. This rule is opt-in. It does not report renames, ownership changes, or schema moves. +`ALTER FUNCTION`, `ALTER PROCEDURE`, and `ALTER ROUTINE` options can change execution behaviour or security for existing callers. This includes security modes and `SET` or `RESET` configuration options, such as `search_path`. This rule is opt-in. It does not report renames, ownership changes, or schema moves. ```sql ALTER FUNCTION f() SECURITY DEFINER; +ALTER PROCEDURE p() SET search_path TO private; +ALTER ROUTINE f() RESET ALL; ``` Review callers before changing the options. Enable this rule with `--include ban-alter-function-options`. diff --git a/docs/docs/ban-alter-role-options.md b/docs/docs/ban-alter-role-options.md index e7dc46d5..122517e0 100644 --- a/docs/docs/ban-alter-role-options.md +++ b/docs/docs/ban-alter-role-options.md @@ -3,11 +3,12 @@ id: ban-alter-role-options title: ban-alter-role-options --- -`ALTER ROLE` options and configuration changes can change access or behaviour for clients that use the role. This rule is opt-in. It does not report role renames. +`ALTER ROLE` and its alias `ALTER USER` can change access or behaviour for clients that use the role. This rule checks role options and `SET` or `RESET` configuration options, including changes for a specific database. This rule is opt-in. It does not report role renames. ```sql ALTER ROLE app_user NOLOGIN; ALTER ROLE app_user SET search_path TO public; +ALTER USER app_user IN DATABASE app_db RESET search_path; ``` Review clients before changing the role. Enable this rule with `--include ban-alter-role-options`. diff --git a/docs/docs/ban-new-write-restriction.md b/docs/docs/ban-new-write-restriction.md index 9fde724a..92dc993d 100644 --- a/docs/docs/ban-new-write-restriction.md +++ b/docs/docs/ban-new-write-restriction.md @@ -5,10 +5,13 @@ title: ban-new-write-restriction ## problem -New table, foreign-table, and domain constraints, `NOT VALID` constraints, `SET NOT NULL`, constrained added columns, changed constraint timing, and unique indexes (including `CONCURRENTLY`) can reject writes from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that table. The rule does not know whether an object existed before the migration. +New table, foreign-table, and domain constraints, `NOT VALID` constraints, `SET NOT NULL`, named `NOT NULL` constraints, constrained added columns (including inline primary keys), changed constraint timing, and unique indexes (including `CONCURRENTLY`) can reject writes from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that table. The rule does not know whether an object existed before the migration. ```sql ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0) NOT VALID; +ALTER TABLE t ADD CONSTRAINT id_required NOT NULL id; +ALTER TABLE t ADD COLUMN external_id bigint PRIMARY KEY; +ALTER FOREIGN TABLE ft ADD COLUMN external_id bigint NOT NULL; ``` ## solution diff --git a/docs/docs/ban-revoke.md b/docs/docs/ban-revoke.md index b677c24f..af180b9d 100644 --- a/docs/docs/ban-revoke.md +++ b/docs/docs/ban-revoke.md @@ -5,10 +5,11 @@ title: ban-revoke ## problem -Revoking privileges, dropping a role or user, changing object ownership with `OWNER TO` or `REASSIGN OWNED`, or running `DROP OWNED` can make client queries fail. `DROP OWNED` can also remove objects owned by the role. This rule is opt-in. +Revoking privileges, removing role membership with `ALTER GROUP ... DROP USER`, dropping a role or user, changing object ownership with `OWNER TO` or `REASSIGN OWNED`, or running `DROP OWNED` can make client queries fail. `DROP OWNED` can also remove objects owned by the role. This rule is opt-in. ```sql REVOKE SELECT ON t FROM app; +ALTER GROUP writers DROP USER app; ``` ## solution From f62f049f304223a0198e9fc520debeecde5a362a Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 14:34:07 +1100 Subject: [PATCH 17/21] feat(linter): check aggregate changes and server and extension settings --- CHANGELOG.md | 3 +- crates/squawk_linter/src/lib.rs | 12 ++++ .../squawk_linter/src/rules/ban_set_schema.rs | 29 ++++++++++ .../src/rules/renaming_object.rs | 25 ++++++++ .../src/rules/security_compatibility.rs | 57 +++++++++++++++++++ docs/docs/ban-alter-extension.md | 13 +++++ docs/docs/ban-alter-system-options.md | 13 +++++ docs/sidebars.js | 2 + docs/src/pages/index.js | 2 + 9 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 docs/docs/ban-alter-extension.md create mode 100644 docs/docs/ban-alter-system-options.md diff --git a/CHANGELOG.md b/CHANGELOG.md index e652ef42..90ff4bd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,10 +15,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - linter: compatibility rules for dropped schemas, sequences, and domains, object renames, and schema moves - linter: default checks for dropped constraints, identity changes, and dropped generated expressions; opt-in checks for dropped indexes, generated expression replacement and addition, defaults, triggers, replica identity, policies, privileges, and replacements - linter: default ban-drop-extension and opt-in rules for policy creation, policy conditions and roles, function and view options, role and database options, and row level security +- linter: opt-in ban-alter-system-options and ban-alter-extension rules ### Changed -- linter: extend existing compatibility checks to more PostgreSQL statement variants, including domain and foreign-table changes, replica-only trigger and rule firing, generated expression replacement, and routine schema moves +- linter: extend existing compatibility checks to more PostgreSQL statement variants, including domain and foreign-table changes, replica-only trigger and rule firing, generated expression replacement, routine schema moves and options, aggregate renames and schema moves, extension schema moves, `ALTER USER` options, group membership removal, named `NOT NULL` constraints, and inline primary keys ## v2.67.0 - 2026-10-04 diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 08c2d2a3..ab29b3c2 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -154,6 +154,8 @@ pub enum Rule { BanAddCompositeAttribute, BanDetachInheritance, BanAlterSequenceValues, + BanAlterSystemOptions, + BanAlterExtension, // xtask:new-rule:error-name } @@ -188,6 +190,8 @@ impl Rule { | Rule::BanAddCompositeAttribute | Rule::BanDetachInheritance | Rule::BanAlterSequenceValues + | Rule::BanAlterSystemOptions + | Rule::BanAlterExtension ) } @@ -285,6 +289,8 @@ impl TryFrom<&str> for Rule { "ban-add-composite-attribute" => Ok(Rule::BanAddCompositeAttribute), "ban-detach-inheritance" => Ok(Rule::BanDetachInheritance), "ban-alter-sequence-values" => Ok(Rule::BanAlterSequenceValues), + "ban-alter-system-options" => Ok(Rule::BanAlterSystemOptions), + "ban-alter-extension" => Ok(Rule::BanAlterExtension), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -391,6 +397,8 @@ impl fmt::Display for Rule { Rule::BanAddCompositeAttribute => "ban-add-composite-attribute", Rule::BanDetachInheritance => "ban-detach-inheritance", Rule::BanAlterSequenceValues => "ban-alter-sequence-values", + Rule::BanAlterSystemOptions => "ban-alter-system-options", + Rule::BanAlterExtension => "ban-alter-extension", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -817,6 +825,8 @@ mod tests { Rule::BanAlterRoleOptions, Rule::BanAlterDatabaseOptions, Rule::BanAlterRowLevelSecurity, + Rule::BanAlterSystemOptions, + Rule::BanAlterExtension, ] { assert!(!linter.rules.contains(&rule)); } @@ -872,6 +882,8 @@ mod tests { Rule::BanAlterRoleOptions, Rule::BanAlterDatabaseOptions, Rule::BanAlterRowLevelSecurity, + Rule::BanAlterSystemOptions, + Rule::BanAlterExtension, ] { let linter = Linter::with_rules(&[rule], &[]); assert!(linter.rules.contains(&rule)); diff --git a/crates/squawk_linter/src/rules/ban_set_schema.rs b/crates/squawk_linter/src/rules/ban_set_schema.rs index 3e954c08..ec50434c 100644 --- a/crates/squawk_linter/src/rules/ban_set_schema.rs +++ b/crates/squawk_linter/src/rules/ban_set_schema.rs @@ -51,6 +51,24 @@ pub(crate) fn ban_set_schema(ctx: &mut Linter, parse: &Parse) { } } } + ast::Stmt::AlterAggregate(node) => { + if let Some(ast::AlterAggregateAction::SetSchema(action)) = node.action() { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + action.syntax(), + )); + } + } + ast::Stmt::AlterExtension(node) => { + if let Some(ast::AlterExtensionAction::SetSchema(action)) = node.action() { + ctx.report(Violation::for_node( + Rule::BanSetSchema, + "Moving an object to another schema may break existing clients.".into(), + action.syntax(), + )); + } + } ast::Stmt::AlterProcedure(node) => { if let Some(ast::AlterProcedureAction::SetSchema(action)) = node.action() { ctx.report(Violation::for_node( @@ -133,7 +151,18 @@ mod test { assert_snapshot!(errors); } #[test] + fn aggregate_and_extension() { + let sql = "ALTER AGGREGATE agg(int) SET SCHEMA s; ALTER EXTENSION hstore SET SCHEMA s;"; + assert_eq!( + lint_errors(sql, Rule::BanSetSchema) + .matches("warning[ban-set-schema]") + .count(), + 2 + ); + } + #[test] fn ok() { lint_ok("ALTER TABLE t OWNER TO app;", Rule::BanSetSchema); + lint_ok("ALTER AGGREGATE agg(int) OWNER TO app;", Rule::BanSetSchema); } } diff --git a/crates/squawk_linter/src/rules/renaming_object.rs b/crates/squawk_linter/src/rules/renaming_object.rs index bbed162f..6c7399ef 100644 --- a/crates/squawk_linter/src/rules/renaming_object.rs +++ b/crates/squawk_linter/src/rules/renaming_object.rs @@ -125,6 +125,15 @@ pub(crate) fn renaming_object(ctx: &mut Linter, parse: &Parse) { } } } + ast::Stmt::AlterAggregate(node) => { + if let Some(ast::AlterAggregateAction::AggregateRenameTo(action)) = node.action() { + ctx.report(Violation::for_node( + Rule::RenamingObject, + "Renaming an aggregate may break existing clients.".into(), + action.syntax(), + )); + } + } ast::Stmt::AlterProcedure(node) => { for action in node.action().into_iter() { if let ast::AlterProcedureAction::ProcedureRenameTo(node) = action { @@ -229,10 +238,26 @@ mod test { assert_snapshot!(errors); } #[test] + fn aggregate() { + assert_eq!( + lint_errors( + "ALTER AGGREGATE agg(int) RENAME TO agg2;", + Rule::RenamingObject + ) + .matches("warning[renaming-object]") + .count(), + 1 + ); + } + #[test] fn ok() { lint_ok( "ALTER TABLE t RENAME TO t2; ALTER TABLE t RENAME COLUMN c TO d;", Rule::RenamingObject, ); + lint_ok( + "ALTER AGGREGATE agg(int) OWNER TO app;", + Rule::RenamingObject, + ); } } diff --git a/crates/squawk_linter/src/rules/security_compatibility.rs b/crates/squawk_linter/src/rules/security_compatibility.rs index 7dc09e97..288c62b1 100644 --- a/crates/squawk_linter/src/rules/security_compatibility.rs +++ b/crates/squawk_linter/src/rules/security_compatibility.rs @@ -171,6 +171,43 @@ pub(crate) fn security_compatibility(ctx: &mut Linter, parse: &Parse } } } + ast::Stmt::AlterSystem(node) => { + if let Some(action) = node.action() { + match action { + ast::AlterSystemAction::SetConfigParam(config) => report( + ctx, + Rule::BanAlterSystemOptions, + "Changing server configuration may change behaviour for existing clients.", + config.syntax(), + ), + ast::AlterSystemAction::ResetConfigParam(config) => report( + ctx, + Rule::BanAlterSystemOptions, + "Changing server configuration may change behaviour for existing clients.", + config.syntax(), + ), + } + } + } + ast::Stmt::AlterExtension(node) => { + if let Some(action) = node.action() { + match action { + ast::AlterExtensionAction::AlterExtensionUpdate(update) => report( + ctx, + Rule::BanAlterExtension, + "Updating an extension can change or remove objects used by existing clients.", + update.syntax(), + ), + ast::AlterExtensionAction::AlterExtensionDrop(drop) => report( + ctx, + Rule::BanAlterExtension, + "Removing an object from an extension changes how the object is managed for existing clients.", + drop.syntax(), + ), + _ => {} + } + } + } ast::Stmt::AlterTable(node) => { for action in node.actions() { match action { @@ -280,6 +317,26 @@ mod tests { "ALTER TABLE t ENABLE ROW LEVEL SECURITY;", "ALTER TABLE t ADD COLUMN c int;", ), + ( + Rule::BanAlterSystemOptions, + "ALTER SYSTEM SET timezone = 'UTC';", + "ALTER DATABASE d SET timezone = 'UTC';", + ), + ( + Rule::BanAlterSystemOptions, + "ALTER SYSTEM RESET ALL;", + "ALTER ROLE r RESET ALL;", + ), + ( + Rule::BanAlterExtension, + "ALTER EXTENSION postgis UPDATE TO '3.4.0';", + "ALTER EXTENSION postgis ADD FUNCTION f();", + ), + ( + Rule::BanAlterExtension, + "ALTER EXTENSION postgis DROP FUNCTION f();", + "ALTER EXTENSION postgis SET SCHEMA gis;", + ), ]; for (rule, bad, good) in cases { assert_eq!(Rule::try_from(rule.to_string().as_str()), Ok(rule)); diff --git a/docs/docs/ban-alter-extension.md b/docs/docs/ban-alter-extension.md new file mode 100644 index 00000000..02326846 --- /dev/null +++ b/docs/docs/ban-alter-extension.md @@ -0,0 +1,13 @@ +--- +id: ban-alter-extension +title: ban-alter-extension +--- + +`ALTER EXTENSION ... UPDATE` runs the extension's upgrade scripts. These scripts can remove functions, change function signatures, or change results for existing clients. `ALTER EXTENSION ... DROP` removes an object from the extension so that later extension changes no longer manage it. This rule is opt-in. It does not report `ALTER EXTENSION ... ADD`. `ALTER EXTENSION ... SET SCHEMA` is reported by `ban-set-schema`. + +```sql +ALTER EXTENSION postgis UPDATE TO '3.4.0'; +ALTER EXTENSION postgis DROP FUNCTION legacy_fn(); +``` + +Read the extension's upgrade notes and update clients before updating the extension. Enable this rule with `--include ban-alter-extension`. diff --git a/docs/docs/ban-alter-system-options.md b/docs/docs/ban-alter-system-options.md new file mode 100644 index 00000000..63b7f9ba --- /dev/null +++ b/docs/docs/ban-alter-system-options.md @@ -0,0 +1,13 @@ +--- +id: ban-alter-system-options +title: ban-alter-system-options +--- + +`ALTER SYSTEM SET` and `ALTER SYSTEM RESET` change server configuration for every database and every client. Parameters such as `search_path`, `timezone`, `DateStyle`, `IntervalStyle`, `bytea_output`, and `standard_conforming_strings` change name resolution or result formats for existing clients. This rule is opt-in. + +```sql +ALTER SYSTEM SET timezone = 'UTC'; +ALTER SYSTEM RESET search_path; +``` + +Review clients before changing server settings. Enable this rule with `--include ban-alter-system-options`. diff --git a/docs/sidebars.js b/docs/sidebars.js index e4546032..08e0fa81 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -78,6 +78,8 @@ module.exports = { "ban-add-composite-attribute", "ban-detach-inheritance", "ban-alter-sequence-values", + "ban-alter-system-options", + "ban-alter-extension", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index 40f784ef..17a66cd8 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -297,6 +297,8 @@ const rules = [ { name: "ban-add-composite-attribute", tags: ["backwards compatibility"], description: "Detect new composite attributes (opt-in)." }, { name: "ban-detach-inheritance", tags: ["backwards compatibility"], description: "Detect partition detach and NO INHERIT (opt-in)." }, { name: "ban-alter-sequence-values", tags: ["backwards compatibility"], description: "Detect changes to sequence values (opt-in)." }, + { name: "ban-alter-system-options", tags: ["backwards compatibility"], description: "Review server configuration changes (opt-in)." }, + { name: "ban-alter-extension", tags: ["backwards compatibility"], description: "Review extension updates and member removals (opt-in)." }, // xtask:new-rule:rule-doc-meta ] From d904f9a421c995ccddbf48ed1de42b9cd2b5b659 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 14:34:11 +1100 Subject: [PATCH 18/21] test(cli): use default configuration for example output --- crates/squawk/tests/example_output.rs | 5 ----- crates/squawk/tests/fixtures/empty.squawk.toml | 1 - 2 files changed, 6 deletions(-) delete mode 100644 crates/squawk/tests/fixtures/empty.squawk.toml diff --git a/crates/squawk/tests/example_output.rs b/crates/squawk/tests/example_output.rs index 7cb44a4d..79f3cdda 100644 --- a/crates/squawk/tests/example_output.rs +++ b/crates/squawk/tests/example_output.rs @@ -7,11 +7,6 @@ fn example_sql_svg() { Command::new(bin_path) .env("CLICOLOR_FORCE", "1") .env("SQUAWK_DISABLE_GITHUB_ANNOTATIONS", "1") - .arg("--config") - .arg(concat!( - env!("CARGO_MANIFEST_DIR"), - "/tests/fixtures/empty.squawk.toml" - )) .arg("../../example.sql") .assert() .code(1) // squawk returns 1 when it finds violations diff --git a/crates/squawk/tests/fixtures/empty.squawk.toml b/crates/squawk/tests/fixtures/empty.squawk.toml deleted file mode 100644 index a0ce96a2..00000000 --- a/crates/squawk/tests/fixtures/empty.squawk.toml +++ /dev/null @@ -1 +0,0 @@ -# This fixture prevents the CLI integration test from reading an ancestor config. From 0d30628978ee9683f17475c13ff822b5ec2b46c4 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 14:34:11 +1100 Subject: [PATCH 19/21] docs: remove application rollback compatibility guide --- .../application-rollback-compatibility.md | 22 ------------------- docs/sidebars.js | 2 +- 2 files changed, 1 insertion(+), 23 deletions(-) delete mode 100644 docs/docs/application-rollback-compatibility.md diff --git a/docs/docs/application-rollback-compatibility.md b/docs/docs/application-rollback-compatibility.md deleted file mode 100644 index 855420cf..00000000 --- a/docs/docs/application-rollback-compatibility.md +++ /dev/null @@ -1,22 +0,0 @@ ---- -id: application-rollback-compatibility -title: Application rollback compatibility ---- - -A migration can finish without blocking traffic and still break an application-only rollback. An application-only rollback keeps the migrated database and all data written by the new application. Run the old application against that database with its actual database role before declaring the migration compatible. - -## Use the compatibility rules - -Default rules detect some operations that remove objects or guarantees. Additional rules are opt-in because the effect depends on application writes, reads, permissions, and data. Enable them with `--include `. A `NOT VALID` constraint still rejects new invalid writes. A unique index built `CONCURRENTLY` still enforces uniqueness. Neither option proves rollback compatibility. - -The linter checks parsed SQL, not the live database schema. Some opt-in rules skip objects created unconditionally earlier in the same migration; this is only a statement-level approximation. Explicit inclusion, exclusion, and ignore comments still apply. The rule documentation lists each SQL form. - -## Verify an application-only rollback - -1. Apply the migration to representative data. -2. Start the new application and write representative new data. -3. Start the old application against the same database, with its actual database role. -4. Check reads, writes, result decoding, authorization, identifier generation, and database side effects. -5. Repeat for every application version that remains an allowed rollback target. - -Review data migrations (including status strings, JSON formats, destructive updates, plain `TRUNCATE`, and deletes), function and trigger bodies, role membership and privileges, and SQL inside procedural blocks or dynamic SQL separately. Lint results do not prove compatibility. Keep lock-duration and table-rewrite checks separate from application compatibility checks. diff --git a/docs/sidebars.js b/docs/sidebars.js index 08e0fa81..91b0b3b7 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -1,6 +1,6 @@ module.exports = { someSidebar: { - General: ["quick_start", "safe_migrations", "application-rollback-compatibility", "cli", "github_app", "web-frameworks", "postgres-locks", "troubleshooting"], + General: ["quick_start", "safe_migrations", "cli", "github_app", "web-frameworks", "postgres-locks", "troubleshooting"], Rules: [ "rules", "adding-field-with-default", From 99a1f1569ce96ce45565982dafdb3e5ae52c720c Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 15:07:56 +1100 Subject: [PATCH 20/21] feat(linter): detect added columns and publication changes --- CHANGELOG.md | 2 + crates/squawk_linter/src/lib.rs | 9 +++ .../src/rules/ban_disable_trigger.rs | 18 ++++- .../src/rules/ban_replica_identity.rs | 46 +++++++++++-- .../src/rules/compatibility_additions.rs | 68 +++++++++++++++++-- .../squawk_linter/src/rules/variant_tests.rs | 6 +- docs/docs/ban-add-column.md | 18 +++++ docs/docs/ban-disable-trigger.md | 2 +- docs/docs/ban-replica-identity.md | 5 +- docs/sidebars.js | 1 + docs/src/pages/index.js | 3 +- 11 files changed, 163 insertions(+), 15 deletions(-) create mode 100644 docs/docs/ban-add-column.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 90ff4bd4..bab3f842 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- linter: opt-in ban-add-column check for existing tables and foreign tables; detect publication changes with ban-replica-identity - linter: opt-in compatibility rules for new write restrictions, enum values, composite attributes, partition detachment, inheritance removal, and sequence changes - linter: ban-drop-view, ban-drop-function, ban-drop-type, ban-drop-default rules - linter: opt-in ban-drop-trigger rule @@ -19,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- linter: detect plain trigger and rule enabling; exclude non-value sequence options from ban-alter-sequence-values - linter: extend existing compatibility checks to more PostgreSQL statement variants, including domain and foreign-table changes, replica-only trigger and rule firing, generated expression replacement, routine schema moves and options, aggregate renames and schema moves, extension schema moves, `ALTER USER` options, group membership removal, named `NOT NULL` constraints, and inline primary keys ## v2.67.0 - 2026-10-04 diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index ab29b3c2..bdc3e86b 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -152,6 +152,7 @@ pub enum Rule { BanNewWriteRestriction, BanAddEnumValue, BanAddCompositeAttribute, + BanAddColumn, BanDetachInheritance, BanAlterSequenceValues, BanAlterSystemOptions, @@ -188,6 +189,7 @@ impl Rule { | Rule::BanNewWriteRestriction | Rule::BanAddEnumValue | Rule::BanAddCompositeAttribute + | Rule::BanAddColumn | Rule::BanDetachInheritance | Rule::BanAlterSequenceValues | Rule::BanAlterSystemOptions @@ -287,6 +289,7 @@ impl TryFrom<&str> for Rule { "ban-new-write-restriction" => Ok(Rule::BanNewWriteRestriction), "ban-add-enum-value" => Ok(Rule::BanAddEnumValue), "ban-add-composite-attribute" => Ok(Rule::BanAddCompositeAttribute), + "ban-add-column" => Ok(Rule::BanAddColumn), "ban-detach-inheritance" => Ok(Rule::BanDetachInheritance), "ban-alter-sequence-values" => Ok(Rule::BanAlterSequenceValues), "ban-alter-system-options" => Ok(Rule::BanAlterSystemOptions), @@ -395,6 +398,7 @@ impl fmt::Display for Rule { Rule::BanNewWriteRestriction => "ban-new-write-restriction", Rule::BanAddEnumValue => "ban-add-enum-value", Rule::BanAddCompositeAttribute => "ban-add-composite-attribute", + Rule::BanAddColumn => "ban-add-column", Rule::BanDetachInheritance => "ban-detach-inheritance", Rule::BanAlterSequenceValues => "ban-alter-sequence-values", Rule::BanAlterSystemOptions => "ban-alter-system-options", @@ -705,6 +709,7 @@ impl Linter { Rule::BanNewWriteRestriction, Rule::BanAddEnumValue, Rule::BanAddCompositeAttribute, + Rule::BanAddColumn, Rule::BanDetachInheritance, Rule::BanAlterSequenceValues, ] @@ -827,6 +832,7 @@ mod tests { Rule::BanAlterRowLevelSecurity, Rule::BanAlterSystemOptions, Rule::BanAlterExtension, + Rule::BanAddColumn, ] { assert!(!linter.rules.contains(&rule)); } @@ -843,6 +849,8 @@ mod tests { Rule::BanAlterGeneratedExpression, "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1);", ), + (Rule::BanAddColumn, "ALTER TABLE t ADD COLUMN c int;"), + (Rule::BanReplicaIdentity, "DROP PUBLICATION p;"), ] { let parse = SourceFile::parse(sql); assert!(parse.errors().is_empty()); @@ -884,6 +892,7 @@ mod tests { Rule::BanAlterRowLevelSecurity, Rule::BanAlterSystemOptions, Rule::BanAlterExtension, + Rule::BanAddColumn, ] { let linter = Linter::with_rules(&[rule], &[]); assert!(linter.rules.contains(&rule)); diff --git a/crates/squawk_linter/src/rules/ban_disable_trigger.rs b/crates/squawk_linter/src/rules/ban_disable_trigger.rs index fd0a4531..79147d7d 100644 --- a/crates/squawk_linter/src/rules/ban_disable_trigger.rs +++ b/crates/squawk_linter/src/rules/ban_disable_trigger.rs @@ -14,6 +14,8 @@ pub(crate) fn ban_disable_trigger(ctx: &mut Linter, parse: &Parse) { "Replica-only triggers and rules do not fire for normal application writes." } ast::AlterTableAction::DisableTrigger(_) + | ast::AlterTableAction::EnableTrigger(_) + | ast::AlterTableAction::EnableRule(_) | ast::AlterTableAction::DisableRule(_) | ast::AlterTableAction::EnableAlwaysTrigger(_) | ast::AlterTableAction::EnableAlwaysRule(_) => { @@ -50,8 +52,22 @@ mod test { assert_eq!(errors.matches("warning[ban-disable-trigger]").count(), 4); assert_snapshot!(errors); } + #[test] + fn enable() { + let sql = "ALTER TABLE t ENABLE TRIGGER trg; ALTER TABLE t ENABLE RULE r;"; + assert_eq!( + lint_errors(sql, Rule::BanDisableTrigger) + .matches("warning[ban-disable-trigger]") + .count(), + 2 + ); + } + #[test] fn ok() { - lint_ok("ALTER TABLE t ENABLE TRIGGER trg;", Rule::BanDisableTrigger); + lint_ok( + "ALTER TABLE t ENABLE ROW LEVEL SECURITY;", + Rule::BanDisableTrigger, + ); } } diff --git a/crates/squawk_linter/src/rules/ban_replica_identity.rs b/crates/squawk_linter/src/rules/ban_replica_identity.rs index c81f3f3a..b4e72a74 100644 --- a/crates/squawk_linter/src/rules/ban_replica_identity.rs +++ b/crates/squawk_linter/src/rules/ban_replica_identity.rs @@ -6,12 +6,37 @@ use squawk_syntax::{ pub(crate) fn ban_replica_identity(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { - if let ast::Stmt::AlterTable(table) = stmt { - for action in table.actions() { - if let ast::AlterTableAction::ReplicaIdentity(node) = action { - ctx.report(Violation::for_node(Rule::BanReplicaIdentity, "Changing replica identity may silently change replication for existing clients.".into(), node.syntax())); + match stmt { + ast::Stmt::AlterTable(table) => { + for action in table.actions() { + if let ast::AlterTableAction::ReplicaIdentity(node) = action { + ctx.report(Violation::for_node(Rule::BanReplicaIdentity, "Changing replica identity may silently change replication for existing clients.".into(), node.syntax())); + } } } + ast::Stmt::DropPublication(node) => ctx.report(Violation::for_node( + Rule::BanReplicaIdentity, + "Dropping a publication stops replication for existing consumers.".into(), + node.syntax(), + )), + ast::Stmt::AlterPublication(publication) => { + if let Some(action) = publication.action() { + if matches!( + action, + ast::AlterPublicationAction::DropPublicationObjects(_) + | ast::AlterPublicationAction::SetPublicationObjects(_) + | ast::AlterPublicationAction::SetAllPublicationObjectList(_) + | ast::AlterPublicationAction::SetOptions(_) + ) { + ctx.report(Violation::for_node( + Rule::BanReplicaIdentity, + "Changing publication tables or options can stop or change replication for existing consumers.".into(), + action.syntax(), + )); + } + } + } + _ => (), } } } @@ -31,8 +56,19 @@ mod test { #[test] fn ok() { lint_ok( - "ALTER TABLE t ENABLE ROW LEVEL SECURITY;", + "ALTER TABLE t ENABLE ROW LEVEL SECURITY; ALTER PUBLICATION p ADD TABLE t; CREATE PUBLICATION p FOR TABLE t;", Rule::BanReplicaIdentity, ); } + + #[test] + fn publication_changes() { + let sql = "DROP PUBLICATION p; ALTER PUBLICATION p DROP TABLE t; ALTER PUBLICATION p SET TABLE t; ALTER PUBLICATION p SET (publish = 'insert'); ALTER PUBLICATION p SET TABLES IN SCHEMA public;"; + assert_eq!( + lint_errors(sql, Rule::BanReplicaIdentity) + .matches("warning[ban-replica-identity]") + .count(), + 5 + ); + } } diff --git a/crates/squawk_linter/src/rules/compatibility_additions.rs b/crates/squawk_linter/src/rules/compatibility_additions.rs index 9ed9007e..08ddb020 100644 --- a/crates/squawk_linter/src/rules/compatibility_additions.rs +++ b/crates/squawk_linter/src/rules/compatibility_additions.rs @@ -41,6 +41,15 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse { @@ -125,10 +134,20 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse - { + ast::Stmt::AlterForeignTable(table) => { for action in table.actions() { + if ctx.rules.contains(&Rule::BanAddColumn) { + if let ast::AlterTableAction::AddColumn(column) = &action { + ctx.report(Violation::for_node( + Rule::BanAddColumn, + "Adding a column changes the shape of rows existing clients receive and can break positional inserts.".into(), + column.syntax(), + )); + } + } + if !ctx.rules.contains(&Rule::BanNewWriteRestriction) { + continue; + } match action { ast::AlterTableAction::AddColumn(column) => { check_column_constraints(ctx, &column); @@ -215,6 +234,15 @@ pub(crate) fn compatibility_additions(ctx: &mut Linter, parse: &Parse Date: Mon, 5 Oct 2026 15:23:20 +1100 Subject: [PATCH 21/21] docs: correct rollback rule changelog --- CHANGELOG.md | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bab3f842..ac956496 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,19 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- linter: opt-in ban-add-column check for existing tables and foreign tables; detect publication changes with ban-replica-identity -- linter: opt-in compatibility rules for new write restrictions, enum values, composite attributes, partition detachment, inheritance removal, and sequence changes -- linter: ban-drop-view, ban-drop-function, ban-drop-type, ban-drop-default rules -- linter: opt-in ban-drop-trigger rule -- linter: compatibility rules for dropped schemas, sequences, and domains, object renames, and schema moves -- linter: default checks for dropped constraints, identity changes, and dropped generated expressions; opt-in checks for dropped indexes, generated expression replacement and addition, defaults, triggers, replica identity, policies, privileges, and replacements -- linter: default ban-drop-extension and opt-in rules for policy creation, policy conditions and roles, function and view options, role and database options, and row level security -- linter: opt-in ban-alter-system-options and ban-alter-extension rules +- linter: default rollback compatibility rules: ban-drop-schema, ban-drop-sequence, ban-drop-domain, ban-drop-constraint, ban-drop-generated-expression, ban-drop-extension, ban-alter-identity, renaming-object, ban-set-schema +- linter: opt-in rollback compatibility rules: ban-alter-generated-expression, ban-drop-index, ban-set-default, ban-disable-trigger, ban-replica-identity, ban-drop-policy, ban-revoke, ban-replace-view-function, ban-create-policy, ban-alter-policy-condition, ban-alter-policy-roles, ban-alter-row-level-security, ban-alter-function-options, ban-alter-view-options, ban-alter-role-options, ban-alter-database-options, ban-alter-system-options, ban-alter-extension, ban-new-write-restriction, ban-add-column, ban-add-enum-value, ban-add-composite-attribute, ban-detach-inheritance, ban-alter-sequence-values ### Changed -- linter: detect plain trigger and rule enabling; exclude non-value sequence options from ban-alter-sequence-values -- linter: extend existing compatibility checks to more PostgreSQL statement variants, including domain and foreign-table changes, replica-only trigger and rule firing, generated expression replacement, routine schema moves and options, aggregate renames and schema moves, extension schema moves, `ALTER USER` options, group membership removal, named `NOT NULL` constraints, and inline primary keys +- linter: extend adding-not-nullable-field and ban-drop-not-null to domains and foreign tables; ban-drop-default to domains, foreign tables, and views; ban-drop-column and changing-column-type to foreign tables and composite attributes; renaming-column to foreign tables, composite attributes, views, and materialized views +- linter: extend ban-drop-function to aggregates and routines, ban-drop-table to foreign tables, and ban-drop-type to operators, operator classes, operator families, and casts ## v2.67.0 - 2026-10-04