From 5cb82568baf56bd0ef08f705f8d380bbf946c951 Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 15:59:45 +1100 Subject: [PATCH] feat(linter): add default rollback compatibility rules --- CHANGELOG.md | 9 + crates/squawk_linter/src/lib.rs | 90 ++++++ .../src/rules/adding_not_null_field.rs | 8 + .../src/rules/ban_alter_identity.rs | 70 +++++ .../src/rules/ban_drop_column.rs | 41 ++- .../src/rules/ban_drop_constraint.rs | 74 +++++ .../src/rules/ban_drop_default.rs | 51 +++- .../src/rules/ban_drop_domain.rs | 35 +++ .../src/rules/ban_drop_extension.rs | 32 +++ .../src/rules/ban_drop_function.rs | 10 + .../rules/ban_drop_generated_expression.rs | 52 ++++ .../src/rules/ban_drop_not_null.rs | 36 ++- .../src/rules/ban_drop_schema.rs | 35 +++ .../src/rules/ban_drop_sequence.rs | 35 +++ .../squawk_linter/src/rules/ban_drop_table.rs | 6 + .../squawk_linter/src/rules/ban_drop_type.rs | 26 +- .../squawk_linter/src/rules/ban_set_schema.rs | 174 +++++++++++ .../src/rules/changing_column_type.rs | 43 ++- crates/squawk_linter/src/rules/mod.rs | 18 ++ .../src/rules/renaming_column.rs | 65 ++++- .../src/rules/renaming_object.rs | 270 ++++++++++++++++++ ..._rules__ban_alter_identity__test__err.snap | 16 ++ ...rules__ban_drop_constraint__test__err.snap | 8 + ...er__rules__ban_drop_domain__test__err.snap | 8 + ...er__rules__ban_drop_schema__test__err.snap | 8 + ...__rules__ban_drop_sequence__test__err.snap | 8 + ...ter__rules__ban_set_schema__test__err.snap | 32 +++ ...er__rules__renaming_object__test__err.snap | 44 +++ docs/docs/ban-alter-identity.md | 18 ++ docs/docs/ban-drop-column.md | 2 +- docs/docs/ban-drop-constraint.md | 18 ++ docs/docs/ban-drop-default.md | 2 +- docs/docs/ban-drop-domain.md | 16 ++ docs/docs/ban-drop-extension.md | 12 + docs/docs/ban-drop-function.md | 2 +- docs/docs/ban-drop-generated-expression.md | 20 ++ docs/docs/ban-drop-not-null.md | 2 +- docs/docs/ban-drop-schema.md | 16 ++ docs/docs/ban-drop-sequence.md | 16 ++ docs/docs/ban-drop-table.md | 2 +- docs/docs/ban-drop-type.md | 2 +- docs/docs/ban-set-schema.md | 16 ++ docs/docs/changing-column-type.md | 3 +- docs/docs/renaming-column.md | 2 +- docs/docs/renaming-object.md | 16 ++ docs/sidebars.js | 9 + docs/src/pages/index.js | 9 + 47 files changed, 1430 insertions(+), 57 deletions(-) create mode 100644 crates/squawk_linter/src/rules/ban_alter_identity.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_extension.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_generated_expression.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_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_identity__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_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_set_schema__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__renaming_object__test__err.snap create mode 100644 docs/docs/ban-alter-identity.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-extension.md create mode 100644 docs/docs/ban-drop-generated-expression.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-set-schema.md create mode 100644 docs/docs/renaming-object.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c3e25205..a4bf0872b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- 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 + +### Changed + +- 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 ### Added diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 16c004c14..4dfcaf581 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -68,6 +68,11 @@ use rules::require_enum_value_ordering; use rules::require_table_schema; use rules::require_timeout_settings; use rules::transaction_nesting; +use rules::{ + ban_alter_identity, ban_drop_constraint, ban_drop_domain, ban_drop_extension, + ban_drop_generated_expression, ban_drop_schema, ban_drop_sequence, ban_set_schema, + renaming_object, +}; // xtask:new-rule:rule-import #[derive(Debug, PartialEq, Clone, Copy, Hash, Eq, Sequence)] @@ -117,6 +122,15 @@ pub enum Rule { BanDropType, BanDropDefault, BanDropTrigger, + BanDropSchema, + BanDropSequence, + BanDropDomain, + BanDropConstraint, + BanDropGeneratedExpression, + RenamingObject, + BanSetSchema, + BanAlterIdentity, + BanDropExtension, // xtask:new-rule:error-name } @@ -195,6 +209,15 @@ 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), + "ban-drop-generated-expression" => Ok(Rule::BanDropGeneratedExpression), + "renaming-object" => Ok(Rule::RenamingObject), + "ban-set-schema" => Ok(Rule::BanSetSchema), + "ban-alter-identity" => Ok(Rule::BanAlterIdentity), + "ban-drop-extension" => Ok(Rule::BanDropExtension), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -271,6 +294,15 @@ 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::BanDropGeneratedExpression => "ban-drop-generated-expression", + Rule::RenamingObject => "renaming-object", + Rule::BanSetSchema => "ban-set-schema", + Rule::BanAlterIdentity => "ban-alter-identity", + Rule::BanDropExtension => "ban-drop-extension", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -540,6 +572,33 @@ impl Linter { if self.rules.contains(&Rule::BanDropTrigger) { ban_drop_trigger(self, file); } + if self.rules.contains(&Rule::BanDropSchema) { + ban_drop_schema(self, file); + } + if self.rules.contains(&Rule::BanDropSequence) { + ban_drop_sequence(self, file); + } + if self.rules.contains(&Rule::BanDropDomain) { + ban_drop_domain(self, file); + } + if self.rules.contains(&Rule::BanDropConstraint) { + ban_drop_constraint(self, file); + } + if self.rules.contains(&Rule::BanDropGeneratedExpression) { + ban_drop_generated_expression(self, file); + } + if self.rules.contains(&Rule::RenamingObject) { + renaming_object(self, file); + } + if self.rules.contains(&Rule::BanSetSchema) { + ban_set_schema(self, file); + } + if self.rules.contains(&Rule::BanAlterIdentity) { + ban_alter_identity(self, file); + } + if self.rules.contains(&Rule::BanDropExtension) { + ban_drop_extension(self, file); + } // xtask:new-rule:rule-call // locate any ignores in the file @@ -687,4 +746,35 @@ mod tests { assert!(linter.rules.contains(&Rule::RequireLockTimeout)); assert!(!linter.rules.contains(&Rule::RequireStatementTimeout)); } + + #[test] + fn compatibility_rules_are_enabled_by_default_and_can_be_excluded() { + 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) + ); + } + } } 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 ad49120a3..e92a01a16 100644 --- a/crates/squawk_linter/src/rules/adding_not_null_field.rs +++ b/crates/squawk_linter/src/rules/adding_not_null_field.rs @@ -168,6 +168,14 @@ ALTER TABLE "core_recipe" ALTER COLUMN "foo" SET NOT NULL; assert_snapshot!(lint_errors(sql, Rule::AddingNotNullableField)); } + #[test] + fn domain_and_foreign_table_set_not_null_are_not_locking_warnings() { + lint_ok( + "ALTER DOMAIN d SET NOT NULL; ALTER FOREIGN TABLE ft ALTER COLUMN c SET NOT NULL;", + Rule::AddingNotNullableField, + ); + } + #[test] fn adding_field_that_is_not_nullable() { let sql = r#" 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 000000000..84dc73ed2 --- /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_drop_column.rs b/crates/squawk_linter/src/rules/ban_drop_column.rs index 0b6bf4743..1c23a97db 100644 --- a/crates/squawk_linter/src/rules/ban_drop_column.rs +++ b/crates/squawk_linter/src/rules/ban_drop_column.rs @@ -8,15 +8,33 @@ 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(), + )); } } } @@ -36,4 +54,11 @@ ALTER TABLE "bar_tbl" DROP COLUMN "foo_col" CASCADE; "#; assert_snapshot!(lint_errors(sql, Rule::BanDropColumn)); } + + #[test] + fn other_columns() { + let sql = "ALTER TYPE composite DROP ATTRIBUTE a; ALTER FOREIGN TABLE ft DROP COLUMN a;"; + let errors = lint_errors(sql, Rule::BanDropColumn); + assert_eq!(errors.matches("warning[ban-drop-column]").count(), 2); + } } 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 000000000..42fb19cc8 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_constraint.rs @@ -0,0 +1,74 @@ +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() { + 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(), + )); + } + } + } +} + +#[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 other_constraints() { + let sql = "ALTER DOMAIN d DROP CONSTRAINT c; ALTER TABLE t ALTER CONSTRAINT c NOT ENFORCED; ALTER FOREIGN TABLE ft DROP CONSTRAINT c;"; + let errors = lint_errors(sql, Rule::BanDropConstraint); + assert_eq!(errors.matches("warning[ban-drop-constraint]").count(), 3); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0);", + Rule::BanDropConstraint, + ); + lint_ok( + "ALTER TABLE t ALTER CONSTRAINT c ENFORCED;", + Rule::BanDropConstraint, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_default.rs b/crates/squawk_linter/src/rules/ban_drop_default.rs index b43165db6..2096bf547 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(), + )); } } } @@ -47,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/ban_drop_domain.rs b/crates/squawk_linter/src/rules/ban_drop_domain.rs new file mode 100644 index 000000000..ac3da2429 --- /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_extension.rs b/crates/squawk_linter/src/rules/ban_drop_extension.rs new file mode 100644 index 000000000..f07fce818 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_extension.rs @@ -0,0 +1,32 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_extension(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::DropExtension(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropExtension, + "Dropping an extension also removes its functions, types, and other objects, which may break existing clients.".into(), + node.syntax(), + )); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_utils::{lint_errors, lint_ok}; + + #[test] + fn drop_extension() { + let errors = lint_errors("DROP EXTENSION IF EXISTS hstore;", Rule::BanDropExtension); + assert!(errors.contains( + "warning[ban-drop-extension]: Dropping an extension also removes its functions, types, and other objects, which may break existing clients." + )); + lint_ok("CREATE EXTENSION hstore;", Rule::BanDropExtension); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_function.rs b/crates/squawk_linter/src/rules/ban_drop_function.rs index 2191708e1..12c5b6137 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(), @@ -18,6 +23,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_generated_expression.rs b/crates/squawk_linter/src/rules/ban_drop_generated_expression.rs new file mode 100644 index 000000000..cb4b2630a --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_generated_expression.rs @@ -0,0 +1,52 @@ +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 may break 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}"); + assert!( + errors.contains("Dropping a generated expression may break existing clients"), + "{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/ban_drop_not_null.rs b/crates/squawk_linter/src/rules/ban_drop_not_null.rs index 072744423..ea582d85a 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_schema.rs b/crates/squawk_linter/src/rules/ban_drop_schema.rs new file mode 100644 index 000000000..1db2fd084 --- /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 000000000..f282d0541 --- /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_drop_table.rs b/crates/squawk_linter/src/rules/ban_drop_table.rs index 250e3ea56..ac453acea 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 f5283da1e..09bdaa751 100644 --- a/crates/squawk_linter/src/rules/ban_drop_type.rs +++ b/crates/squawk_linter/src/rules/ban_drop_type.rs @@ -7,7 +7,31 @@ 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::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(), + 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_set_schema.rs b/crates/squawk_linter/src/rules/ban_set_schema.rs new file mode 100644 index 000000000..ca0fedfb7 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_set_schema.rs @@ -0,0 +1,174 @@ +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::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 { + 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::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( + 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 { + 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 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 other_schema_moves() { + let sql = "ALTER PROCEDURE p() SET SCHEMA s; ALTER ROUTINE f() SET SCHEMA s; ALTER FOREIGN TABLE ft SET SCHEMA s;"; + let errors = lint_errors(sql, Rule::BanSetSchema); + assert_eq!(errors.matches("warning[ban-set-schema]").count(), 3); + } + #[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/changing_column_type.rs b/crates/squawk_linter/src/rules/changing_column_type.rs index 6e098ef6c..ab8827f00 100644 --- a/crates/squawk_linter/src/rules/changing_column_type.rs +++ b/crates/squawk_linter/src/rules/changing_column_type.rs @@ -8,17 +8,35 @@ 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(), + )); + } } } } @@ -57,4 +75,11 @@ COMMIT; "#; assert_snapshot!(lint_errors(sql, Rule::ChangingColumnType)); } + + #[test] + fn other_column_types() { + let sql = "ALTER TYPE composite ALTER ATTRIBUTE a TYPE text; ALTER FOREIGN TABLE ft ALTER COLUMN a TYPE text;"; + let errors = lint_errors(sql, Rule::ChangingColumnType); + assert_eq!(errors.matches("warning[changing-column-type]").count(), 2); + } } diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index e62d5272e..f21b1cbb4 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -4,19 +4,27 @@ 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_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_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_extension; pub(crate) mod ban_drop_function; +pub(crate) mod ban_drop_generated_expression; pub(crate) mod ban_drop_not_null; +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_set_schema; pub(crate) mod ban_truncate_cascade; pub(crate) mod ban_uncommitted_transaction; pub(crate) mod changing_column_type; @@ -31,6 +39,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 +57,27 @@ 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_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_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_extension::ban_drop_extension; 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_not_null::ban_drop_not_null; +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_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 +92,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_column.rs b/crates/squawk_linter/src/rules/renaming_column.rs index 0445f6979..dde82efb3 100644 --- a/crates/squawk_linter/src/rules/renaming_column.rs +++ b/crates/squawk_linter/src/rules/renaming_column.rs @@ -8,16 +8,59 @@ 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::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 { + 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(), + )); + } + } + } + _ => (), } } } @@ -27,7 +70,7 @@ mod test { use insta::assert_snapshot; use crate::Rule; - use crate::test_utils::lint_errors; + use crate::test_utils::{lint_errors, lint_ok}; #[test] fn err() { @@ -36,4 +79,16 @@ ALTER TABLE "table_name" RENAME COLUMN "column_name" TO "new_column_name"; "#; assert_snapshot!(lint_errors(sql, Rule::RenamingColumn)); } + + #[test] + fn other_renames() { + let sql = "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; ALTER TYPE composite RENAME ATTRIBUTE old TO renamed;"; + let errors = lint_errors(sql, Rule::RenamingColumn); + assert_eq!(errors.matches("warning[renaming-column]").count(), 4); + } + + #[test] + fn ok() { + lint_ok("ALTER VIEW v RENAME TO v2;", Rule::RenamingColumn); + } } 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 000000000..795de6237 --- /dev/null +++ b/crates/squawk_linter/src/rules/renaming_object.rs @@ -0,0 +1,270 @@ +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::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::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( + 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 { + 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::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 { + 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() { + 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::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 aggregate() { + assert_eq!( + lint_errors( + "ALTER AGGREGATE agg(int) RENAME TO agg2;", + Rule::RenamingObject + ) + .matches("warning[renaming-object]") + .count(), + 1 + ); + } + #[test] + fn other_renames() { + let sql = "ALTER FOREIGN TABLE ft RENAME TO ft2; ALTER ROUTINE f() RENAME TO g; 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;"; + let errors = lint_errors(sql, Rule::RenamingObject); + assert_eq!(errors.matches("warning[renaming-object]").count(), 10); + } + #[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, + ); + lint_ok("ALTER POLICY p ON t USING (true);", Rule::RenamingObject); + } +} 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 000000000..fb162586a --- /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_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 000000000..7adc6f82e --- /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 000000000..71c78c62f --- /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_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 000000000..a185c1acb --- /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 000000000..510c377c8 --- /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_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 000000000..bd430de05 --- /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 000000000..349aa1c3b --- /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'; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/docs/docs/ban-alter-identity.md b/docs/docs/ban-alter-identity.md new file mode 100644 index 000000000..7a709cc2f --- /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 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; +``` + +## solution + +Update client inserts before changing the identity setting. + +Exclude this rule with `--exclude ban-alter-identity` after checking application compatibility. diff --git a/docs/docs/ban-drop-column.md b/docs/docs/ban-drop-column.md index de2743b36..0dcbd89d7 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 new file mode 100644 index 000000000..ddba25671 --- /dev/null +++ b/docs/docs/ban-drop-constraint.md @@ -0,0 +1,18 @@ +--- +id: ban-drop-constraint +title: ban-drop-constraint +--- + +## problem + +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; +``` + +## solution + +Update clients to not depend on the constraint before dropping it. + +Exclude this rule with `--exclude ban-drop-constraint` after checking application compatibility. diff --git a/docs/docs/ban-drop-default.md b/docs/docs/ban-drop-default.md index df349f6c4..5f531d603 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-domain.md b/docs/docs/ban-drop-domain.md new file mode 100644 index 000000000..011168410 --- /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-extension.md b/docs/docs/ban-drop-extension.md new file mode 100644 index 000000000..dd24c2ab5 --- /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/docs/ban-drop-function.md b/docs/docs/ban-drop-function.md index e104bf1e5..c3fbdcf41 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, routine, or aggregate may break existing clients. Calls can fail with `42883 undefined_function`. ## solution diff --git a/docs/docs/ban-drop-generated-expression.md b/docs/docs/ban-drop-generated-expression.md new file mode 100644 index 000000000..838c53fba --- /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 stored generated column into an ordinary column. Existing stored values remain, but PostgreSQL no longer computes the value for future inserts or updates. An insert that omits the column can return `NULL` instead of a computed value. 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/docs/ban-drop-not-null.md b/docs/docs/ban-drop-not-null.md index 8c2883573..702a1205b 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-schema.md b/docs/docs/ban-drop-schema.md new file mode 100644 index 000000000..5333b5ac2 --- /dev/null +++ b/docs/docs/ban-drop-schema.md @@ -0,0 +1,16 @@ +--- +id: ban-drop-schema +title: ban-drop-schema +--- + +## problem + +Dropping a schema may break existing clients. For example, an earlier application revision may still query `s.orders` after a database migration drops `s`. Rolling back the application does not restore the schema, so that query fails. `CASCADE` also drops objects in the schema. + +```sql +DROP SCHEMA IF EXISTS s CASCADE; +``` + +## solution + +Update clients to stop using the schema and its objects. Keep the schema until all running application revisions, including any revision used for rollback, no longer depend on it. Drop the schema in a later migration. diff --git a/docs/docs/ban-drop-sequence.md b/docs/docs/ban-drop-sequence.md new file mode 100644 index 000000000..bcc633b07 --- /dev/null +++ b/docs/docs/ban-drop-sequence.md @@ -0,0 +1,16 @@ +--- +id: ban-drop-sequence +title: ban-drop-sequence +--- + +## problem + +Dropping a sequence may break existing clients. For example, an earlier application revision may still call `nextval('s')` after a database migration drops `s`. Rolling back the application does not restore the sequence, so that call fails. `CASCADE` can also remove defaults that depend on the sequence. + +```sql +DROP SEQUENCE IF EXISTS s CASCADE; +``` + +## solution + +Update clients to stop using the sequence. Keep it until all running application revisions, including any revision used for rollback, no longer depend on it. Drop the sequence in a later migration. diff --git a/docs/docs/ban-drop-table.md b/docs/docs/ban-drop-table.md index 2328f0ef5..9cd1e4978 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 5ce4f6733..dfff6eb5a 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, 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-set-schema.md b/docs/docs/ban-set-schema.md new file mode 100644 index 000000000..49630cb06 --- /dev/null +++ b/docs/docs/ban-set-schema.md @@ -0,0 +1,16 @@ +--- +id: ban-set-schema +title: ban-set-schema +--- + +## problem + +Moving an object to another schema may break existing clients that use its old schema-qualified name. For example, an earlier application revision may still query `public.t` after a migration moves the table to `s`. Rolling back the application does not move the table back, so that query fails. This also applies to foreign tables, procedures, and routines. + +```sql +ALTER TABLE t SET SCHEMA s; +``` + +## solution + +Update clients to use the new schema-qualified name. Keep the object at its old name until all running application revisions, including any revision used for rollback, no longer depend on it. Move the object in a later migration. diff --git a/docs/docs/changing-column-type.md b/docs/docs/changing-column-type.md index a85155506..2a8c3adfe 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 797eeff68..bf4aea758 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 or a composite type attribute may break existing clients. ## solution diff --git a/docs/docs/renaming-object.md b/docs/docs/renaming-object.md new file mode 100644 index 000000000..5eb2c73df --- /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. This includes foreign tables, routines, roles, users, groups, databases, triggers, policies, and table or domain constraints. + +```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 679fca6bd..4e9aa28cc 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -48,6 +48,15 @@ module.exports = { "require-concurrent-reindex", "prefer-repack", "ban-duplicate-column-assignments", + "ban-drop-schema", + "ban-drop-sequence", + "ban-drop-domain", + "ban-drop-constraint", + "ban-drop-generated-expression", + "renaming-object", + "ban-set-schema", + "ban-alter-identity", + "ban-drop-extension", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index ca30535f1..bd232ea26 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -267,6 +267,15 @@ 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." }, + { 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." }, + { name: "ban-drop-extension", tags: ["backwards compatibility"], description: "Prevent dropping extensions used by clients." }, // xtask:new-rule:rule-doc-meta ]