diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c3e2520..ac956496 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ 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 +- 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: 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/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..bdc3e86b 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -68,6 +68,13 @@ 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_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 #[derive(Debug, PartialEq, Clone, Copy, Hash, Eq, Sequence)] @@ -117,6 +124,39 @@ pub enum Rule { BanDropType, BanDropDefault, BanDropTrigger, + BanDropSchema, + BanDropSequence, + BanDropDomain, + BanDropConstraint, + BanDropGeneratedExpression, + RenamingObject, + BanSetSchema, + BanAlterIdentity, + BanAlterGeneratedExpression, + BanDropIndex, + BanSetDefault, + BanDisableTrigger, + BanReplicaIdentity, + BanDropPolicy, + BanRevoke, + BanReplaceViewFunction, + BanDropExtension, + BanAlterPolicyCondition, + BanAlterPolicyRoles, + BanCreatePolicy, + BanAlterFunctionOptions, + BanAlterViewOptions, + BanAlterRoleOptions, + BanAlterDatabaseOptions, + BanAlterRowLevelSecurity, + BanNewWriteRestriction, + BanAddEnumValue, + BanAddCompositeAttribute, + BanAddColumn, + BanDetachInheritance, + BanAlterSequenceValues, + BanAlterSystemOptions, + BanAlterExtension, // xtask:new-rule:error-name } @@ -127,7 +167,33 @@ 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::BanAlterGeneratedExpression + | Rule::BanDropIndex + | Rule::BanSetDefault + | Rule::BanDisableTrigger + | Rule::BanReplicaIdentity + | Rule::BanDropPolicy + | Rule::BanRevoke + | Rule::BanReplaceViewFunction + | Rule::BanAlterPolicyCondition + | Rule::BanAlterPolicyRoles + | Rule::BanCreatePolicy + | Rule::BanAlterFunctionOptions + | Rule::BanAlterViewOptions + | Rule::BanAlterRoleOptions + | Rule::BanAlterDatabaseOptions + | Rule::BanAlterRowLevelSecurity + | Rule::BanNewWriteRestriction + | Rule::BanAddEnumValue + | Rule::BanAddCompositeAttribute + | Rule::BanAddColumn + | Rule::BanDetachInheritance + | Rule::BanAlterSequenceValues + | Rule::BanAlterSystemOptions + | Rule::BanAlterExtension ) } @@ -195,6 +261,39 @@ 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-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), + "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), + "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), + "ban-alter-extension" => Ok(Rule::BanAlterExtension), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -271,6 +370,39 @@ 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::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", + 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", + 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", + Rule::BanAlterExtension => "ban-alter-extension", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -540,6 +672,52 @@ 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::BanDropGeneratedExpression, + ban_drop_generated_expression, + ), + (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); + } + } + security_compatibility(self, file); + if [ + Rule::BanNewWriteRestriction, + Rule::BanAddEnumValue, + Rule::BanAddCompositeAttribute, + Rule::BanAddColumn, + 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 @@ -635,11 +813,87 @@ mod tests { let linter = Linter::with_rules(&[], &[]); assert!(!linter.rules.contains(&Rule::RequireTableSchema)); assert!(!linter.rules.contains(&Rule::BanDropTrigger)); + for rule in [ + Rule::BanAlterGeneratedExpression, + Rule::BanDropIndex, + Rule::BanSetDefault, + Rule::BanDisableTrigger, + Rule::BanReplicaIdentity, + Rule::BanDropPolicy, + Rule::BanRevoke, + Rule::BanReplaceViewFunction, + Rule::BanAlterPolicyCondition, + Rule::BanAlterPolicyRoles, + Rule::BanCreatePolicy, + Rule::BanAlterFunctionOptions, + Rule::BanAlterViewOptions, + Rule::BanAlterRoleOptions, + Rule::BanAlterDatabaseOptions, + Rule::BanAlterRowLevelSecurity, + Rule::BanAlterSystemOptions, + Rule::BanAlterExtension, + Rule::BanAddColumn, + ] { + assert!(!linter.rules.contains(&rule)); + } + } + + #[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;", + ), + ( + 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()); + 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::BanAlterGeneratedExpression, + Rule::BanDropIndex, + Rule::BanSetDefault, + Rule::BanDisableTrigger, + Rule::BanReplicaIdentity, + Rule::BanDropPolicy, + Rule::BanRevoke, + Rule::BanReplaceViewFunction, + Rule::BanAlterPolicyCondition, + Rule::BanAlterPolicyRoles, + Rule::BanCreatePolicy, + Rule::BanAlterFunctionOptions, + Rule::BanAlterViewOptions, + Rule::BanAlterRoleOptions, + Rule::BanAlterDatabaseOptions, + Rule::BanAlterRowLevelSecurity, + Rule::BanAlterSystemOptions, + Rule::BanAlterExtension, + Rule::BanAddColumn, + ] { let linter = Linter::with_rules(&[rule], &[]); assert!(linter.rules.contains(&rule)); } @@ -657,6 +911,64 @@ 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 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/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 new file mode 100644 index 00000000..b43e0959 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs @@ -0,0 +1,50 @@ +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::SetExpression(node)) = column.option() { + 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) => { + for constraint in column.constraints() { + if let ast::Constraint::GeneratedConstraint(node) = constraint { + ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Adding 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 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 ALTER COLUMN c DROP EXPRESSION;", + 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..79147d7d --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_disable_trigger.rs @@ -0,0 +1,73 @@ +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() { + 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::EnableTrigger(_) + | ast::AlterTableAction::EnableRule(_) + | 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(), + )); + } + } + } +} + +#[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 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 ROW LEVEL SECURITY;", + Rule::BanDisableTrigger, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_column.rs b/crates/squawk_linter/src/rules/ban_drop_column.rs index 0b6bf474..e44f9ec8 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(), + )); } } } 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..175c9ab8 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_constraint.rs @@ -0,0 +1,64 @@ +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 ok() { + lint_ok( + "ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0);", + 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 b43165db..2096bf54 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 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_function.rs b/crates/squawk_linter/src/rules/ban_drop_function.rs index 2191708e..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(), @@ -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 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/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_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_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_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..09bdaa75 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_replace_view_function.rs b/crates/squawk_linter/src/rules/ban_replace_view_function.rs new file mode 100644 index 00000000..64a3917c --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_replace_view_function.rs @@ -0,0 +1,63 @@ +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())); + } + 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(), + )); + } + _ => (), + } + } +} + +#[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..b4e72a74 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_replica_identity.rs @@ -0,0 +1,74 @@ +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() { + 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(), + )); + } + } + } + _ => (), + } + } +} + +#[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; 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/ban_revoke.rs b/crates/squawk_linter/src/rules/ban_revoke.rs new file mode 100644 index 00000000..88868d72 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_revoke.rs @@ -0,0 +1,88 @@ +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() { + 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( + Rule::BanRevoke, + "Revoking privileges may break existing clients.".into(), + 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::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(), + 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(), + 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..2017b357 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_set_default.rs @@ -0,0 +1,57 @@ +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::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())); + } + } + } + } +} + +#[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..ec50434c --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_set_schema.rs @@ -0,0 +1,168 @@ +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 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 6e098ef6..84b38948 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(), + )); + } } } } 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..08ddb020 --- /dev/null +++ b/crates/squawk_linter/src/rules/compatibility_additions.rs @@ -0,0 +1,455 @@ +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::BanAddColumn) && !new_table { + 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) && !new_table { + match &action { + ast::AlterTableAction::AddConstraint(add) => { + if let Some(constraint) = add.constraint() { + let restriction = matches!( + constraint, + ast::Constraint::CheckConstraint(_) + | ast::Constraint::ForeignKeyConstraint(_) + | ast::Constraint::UniqueConstraint(_) + | ast::Constraint::PrimaryKeyConstraint(_) + | ast::Constraint::ExcludeConstraint(_) + | ast::Constraint::NotNullConstraint(_) + ); + 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(), + )); + } + } + ast::AlterTableAction::AddColumn(column) => { + check_column_constraints(ctx, column); + } + 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())); + } + } + _ => (), + } + } + 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::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); + } + 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 { + 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 + .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 { + if matches!( + option, + ast::SequenceOption::OptionOwnedBy(_) + | ast::SequenceOption::OptionLogged(_) + | ast::SequenceOption::OptionUnlogged(_) + | ast::SequenceOption::OptionSequenceName(_) + ) { + continue; + } + ctx.report(Violation::for_node(Rule::BanAlterSequenceValues, + "Changing a sequence option can change values generated for existing clients.".into(), option.syntax())); + } + } + } + } + _ => (), + } + } +} + +fn check_column_constraints(ctx: &mut Linter, column: &ast::AddColumn) { + for constraint in column.constraints() { + if matches!( + constraint, + ast::Constraint::NotNullConstraint(_) + | ast::Constraint::ReferencesConstraint(_) + | ast::Constraint::CheckConstraint(_) + | ast::Constraint::UniqueConstraint(_) + | ast::Constraint::PrimaryKeyConstraint(_) + ) { + ctx.report(Violation::for_node( + Rule::BanNewWriteRestriction, + "A new column constraint can reject writes from existing clients.".into(), + constraint.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 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 ADD CONSTRAINT nn NOT NULL c; ALTER TABLE t ADD NOT NULL c NOT VALID;", + Rule::BanNewWriteRestriction + ) + .matches("warning[ban-new-write-restriction]") + .count(), + 2 + ); + 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, + ); + 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] + 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 add_column() { + assert_eq!( + lint_errors("ALTER TABLE t ADD COLUMN c int;", Rule::BanAddColumn) + .matches("warning[ban-add-column]") + .count(), + 1 + ); + assert_eq!( + lint_errors( + "ALTER FOREIGN TABLE ft ADD COLUMN c int;", + Rule::BanAddColumn + ) + .matches("warning[ban-add-column]") + .count(), + 1 + ); + lint_ok( + "CREATE TABLE t (id int); ALTER TABLE t ADD COLUMN c int;", + Rule::BanAddColumn, + ); + assert_eq!( + lint_errors( + "CREATE TABLE IF NOT EXISTS t (id int); ALTER TABLE t ADD COLUMN c int;", + Rule::BanAddColumn + ) + .matches("warning[ban-add-column]") + .count(), + 1 + ); + } + + #[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; ALTER SEQUENCE ids OWNED BY t.id; ALTER SEQUENCE ids SET LOGGED; ALTER SEQUENCE ids SET UNLOGGED;", + 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 e62d5272..6c6f4051 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -4,22 +4,38 @@ 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_generated_expression; +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; +pub(crate) mod compatibility_additions; pub(crate) mod constraint_missing_not_valid; pub(crate) mod disallow_unique_constraint; pub(crate) mod identifier_too_long; @@ -31,6 +47,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; @@ -39,7 +56,10 @@ 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 security_compatibility; pub(crate) mod transaction_nesting; +#[cfg(test)] +mod variant_tests; // xtask:new-rule:mod-decl pub(crate) use adding_field_with_default::adding_field_with_default; @@ -48,22 +68,38 @@ 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_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; +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; +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; @@ -75,6 +111,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; @@ -83,5 +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 security_compatibility::security_compatibility; pub(crate) use transaction_nesting::transaction_nesting; // xtask:new-rule:export diff --git a/crates/squawk_linter/src/rules/renaming_column.rs b/crates/squawk_linter/src/rules/renaming_column.rs index 0445f697..242daf5e 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(), + )); + } + } + } + _ => (), } } } 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..6c7399ef --- /dev/null +++ b/crates/squawk_linter/src/rules/renaming_object.rs @@ -0,0 +1,263 @@ +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 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 new file mode 100644 index 00000000..288c62b1 --- /dev/null +++ b/crates/squawk_linter/src/rules/security_compatibility.rs @@ -0,0 +1,413 @@ +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::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 { + 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::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 { + 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::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 { + 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::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);", + "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;", + ), + ( + 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)); + 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/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..71d3a89e --- /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 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; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-alter-generated-expression]: Adding 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; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 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..b65f377a --- /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: errors +--- +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]: 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]: 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]: 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/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'; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━ 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..d0f61fb6 --- /dev/null +++ b/crates/squawk_linter/src/rules/variant_tests.rs @@ -0,0 +1,224 @@ +use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, +}; + +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}"); +} + +#[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 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( + "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); + check( + "REASSIGN OWNED BY app TO admin; ALTER TABLE t OWNER TO admin; DROP USER app;", + Rule::BanRevoke, + 3, + ); +} + +#[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 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( + "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); + check( + "ALTER TABLE t ENABLE TRIGGER tr;", + Rule::BanDisableTrigger, + 1, + ); +} + +#[test] +fn schema_moves() { + check( + "ALTER PROCEDURE p() SET SCHEMA s; ALTER ROUTINE f() SET SCHEMA s; ALTER FOREIGN TABLE ft SET SCHEMA s;", + Rule::BanSetSchema, + 3, + ); +} 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-add-column.md b/docs/docs/ban-add-column.md new file mode 100644 index 00000000..2355ed96 --- /dev/null +++ b/docs/docs/ban-add-column.md @@ -0,0 +1,18 @@ +--- +id: ban-add-column +title: ban-add-column +--- + +## problem + +Adding a column to an existing table or foreign table changes the shape of `SELECT *` results and can break positional inserts from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that new table. + +```sql +ALTER TABLE accounts ADD COLUMN status text; +``` + +## solution + +Check clients that use `SELECT *` or positional inserts before adding the column. + +Enable this rule with `--include ban-add-column`. 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-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-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-function-options.md b/docs/docs/ban-alter-function-options.md new file mode 100644 index 00000000..2fa7d3c2 --- /dev/null +++ b/docs/docs/ban-alter-function-options.md @@ -0,0 +1,14 @@ +--- +id: ban-alter-function-options +title: ban-alter-function-options +--- + +`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-generated-expression.md b/docs/docs/ban-alter-generated-expression.md new file mode 100644 index 00000000..6642d3a3 --- /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 + +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); +``` + +## solution + +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/docs/ban-alter-identity.md b/docs/docs/ban-alter-identity.md new file mode 100644 index 00000000..7a709cc2 --- /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-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..122517e0 --- /dev/null +++ b/docs/docs/ban-alter-role-options.md @@ -0,0 +1,14 @@ +--- +id: ban-alter-role-options +title: ban-alter-role-options +--- + +`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-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-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-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/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-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-disable-trigger.md b/docs/docs/ban-disable-trigger.md new file mode 100644 index 00000000..77873045 --- /dev/null +++ b/docs/docs/ban-disable-trigger.md @@ -0,0 +1,18 @@ +--- +id: ban-disable-trigger +title: ban-disable-trigger +--- + +## problem + +Disabling or enabling 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; +``` + +## 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-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 new file mode 100644 index 00000000..ddba2567 --- /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 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-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-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/docs/ban-drop-function.md b/docs/docs/ban-drop-function.md index e104bf1e..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 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 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/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-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-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-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..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 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-new-write-restriction.md b/docs/docs/ban-new-write-restriction.md new file mode 100644 index 00000000..92dc993d --- /dev/null +++ b/docs/docs/ban-new-write-restriction.md @@ -0,0 +1,21 @@ +--- +id: ban-new-write-restriction +title: ban-new-write-restriction +--- + +## problem + +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 + +Update client writes before adding the restriction. + +Enable this rule with `--include ban-new-write-restriction`. diff --git a/docs/docs/ban-replace-view-function.md b/docs/docs/ban-replace-view-function.md new file mode 100644 index 00000000..e59f5e51 --- /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` 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; +``` + +## solution + +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-replica-identity.md b/docs/docs/ban-replica-identity.md new file mode 100644 index 00000000..1b2cf88b --- /dev/null +++ b/docs/docs/ban-replica-identity.md @@ -0,0 +1,19 @@ +--- +id: ban-replica-identity +title: ban-replica-identity +--- + +## problem + +Changing replica identity changes the row data available to logical replication consumers. Dropping a publication or changing its published tables or options can stop or change replication for those consumers. This rule is opt-in. + +```sql +ALTER TABLE t REPLICA IDENTITY FULL; +ALTER PUBLICATION p DROP TABLE t; +``` + +## solution + +Update replication consumers before changing replica identity or publication membership and options. + +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..af180b9d --- /dev/null +++ b/docs/docs/ban-revoke.md @@ -0,0 +1,19 @@ +--- +id: ban-revoke +title: ban-revoke +--- + +## problem + +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 + +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..7ff72de9 --- /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 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; +``` + +## 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..8276ab0e --- /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`. This includes foreign tables, procedures, and routines. + +```sql +ALTER TABLE t SET SCHEMA s; +``` + +## solution + +Update clients to use the new schema before moving the object. 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 797eeff6..bf4aea75 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 00000000..5eb2c73d --- /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 679fca6b..c1fbba82 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -48,6 +48,39 @@ 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-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-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", + "ban-new-write-restriction", + "ban-add-enum-value", + "ban-add-composite-attribute", + "ban-add-column", + "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 ca30535f..62e62f67 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -267,6 +267,39 @@ 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-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: "Detect changes to replica identity and publications (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)." }, + { 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)." }, + { 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-add-column", tags: ["backwards compatibility"], description: "Detect columns added to existing tables (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 ]