From 5a9eb47449fbaf00f661c6d877d50a69df1aed9d Mon Sep 17 00:00:00 2001 From: t-monaghan Date: Mon, 5 Oct 2026 15:59:45 +1100 Subject: [PATCH] feat(linter): add opt-in rollback compatibility rules --- CHANGELOG.md | 1 + crates/squawk_linter/src/ignore.rs | 2 +- crates/squawk_linter/src/lib.rs | 175 ++++++- .../rules/ban_alter_generated_expression.rs | 50 ++ .../src/rules/ban_disable_trigger.rs | 73 +++ .../squawk_linter/src/rules/ban_drop_index.rs | 37 ++ .../src/rules/ban_drop_policy.rs | 47 ++ .../src/rules/ban_replace_view_function.rs | 63 +++ .../src/rules/ban_replica_identity.rs | 74 +++ crates/squawk_linter/src/rules/ban_revoke.rs | 88 ++++ .../src/rules/ban_set_default.rs | 57 +++ .../src/rules/compatibility_additions.rs | 455 ++++++++++++++++++ crates/squawk_linter/src/rules/mod.rs | 22 + .../src/rules/security_compatibility.rs | 400 +++++++++++++++ ...alter_generated_expression__test__err.snap | 12 + ...rules__ban_disable_trigger__test__err.snap | 20 + ...ter__rules__ban_drop_index__test__err.snap | 12 + ...er__rules__ban_drop_policy__test__err.snap | 12 + ..._ban_replace_view_function__test__err.snap | 16 + ...ules__ban_replica_identity__test__err.snap | 8 + ..._linter__rules__ban_revoke__test__err.snap | 12 + ...er__rules__ban_set_default__test__err.snap | 8 + .../squawk_linter/src/rules/variant_tests.rs | 107 ++++ docs/docs/ban-add-column.md | 18 + docs/docs/ban-add-composite-attribute.md | 18 + docs/docs/ban-add-enum-value.md | 18 + docs/docs/ban-alter-database-options.md | 13 + docs/docs/ban-alter-extension.md | 13 + docs/docs/ban-alter-function-options.md | 14 + docs/docs/ban-alter-generated-expression.md | 18 + docs/docs/ban-alter-policy-condition.md | 12 + docs/docs/ban-alter-policy-roles.md | 12 + docs/docs/ban-alter-role-options.md | 14 + docs/docs/ban-alter-row-level-security.md | 12 + docs/docs/ban-alter-sequence-values.md | 18 + docs/docs/ban-alter-system-options.md | 13 + docs/docs/ban-alter-view-options.md | 12 + docs/docs/ban-create-policy.md | 12 + docs/docs/ban-detach-inheritance.md | 18 + docs/docs/ban-disable-trigger.md | 18 + docs/docs/ban-drop-index.md | 18 + docs/docs/ban-drop-policy.md | 18 + docs/docs/ban-new-write-restriction.md | 21 + docs/docs/ban-replace-view-function.md | 18 + docs/docs/ban-replica-identity.md | 19 + docs/docs/ban-revoke.md | 19 + docs/docs/ban-set-default.md | 18 + docs/sidebars.js | 24 + docs/src/pages/index.js | 24 + 49 files changed, 2181 insertions(+), 2 deletions(-) create mode 100644 crates/squawk_linter/src/rules/ban_alter_generated_expression.rs create mode 100644 crates/squawk_linter/src/rules/ban_disable_trigger.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_index.rs create mode 100644 crates/squawk_linter/src/rules/ban_drop_policy.rs create mode 100644 crates/squawk_linter/src/rules/ban_replace_view_function.rs create mode 100644 crates/squawk_linter/src/rules/ban_replica_identity.rs create mode 100644 crates/squawk_linter/src/rules/ban_revoke.rs create mode 100644 crates/squawk_linter/src/rules/ban_set_default.rs create mode 100644 crates/squawk_linter/src/rules/compatibility_additions.rs create mode 100644 crates/squawk_linter/src/rules/security_compatibility.rs create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap create mode 100644 crates/squawk_linter/src/rules/variant_tests.rs create mode 100644 docs/docs/ban-add-column.md create mode 100644 docs/docs/ban-add-composite-attribute.md create mode 100644 docs/docs/ban-add-enum-value.md create mode 100644 docs/docs/ban-alter-database-options.md create mode 100644 docs/docs/ban-alter-extension.md create mode 100644 docs/docs/ban-alter-function-options.md create mode 100644 docs/docs/ban-alter-generated-expression.md create mode 100644 docs/docs/ban-alter-policy-condition.md create mode 100644 docs/docs/ban-alter-policy-roles.md create mode 100644 docs/docs/ban-alter-role-options.md create mode 100644 docs/docs/ban-alter-row-level-security.md create mode 100644 docs/docs/ban-alter-sequence-values.md create mode 100644 docs/docs/ban-alter-system-options.md create mode 100644 docs/docs/ban-alter-view-options.md create mode 100644 docs/docs/ban-create-policy.md create mode 100644 docs/docs/ban-detach-inheritance.md create mode 100644 docs/docs/ban-disable-trigger.md create mode 100644 docs/docs/ban-drop-index.md create mode 100644 docs/docs/ban-drop-policy.md create mode 100644 docs/docs/ban-new-write-restriction.md create mode 100644 docs/docs/ban-replace-view-function.md create mode 100644 docs/docs/ban-replica-identity.md create mode 100644 docs/docs/ban-revoke.md create mode 100644 docs/docs/ban-set-default.md diff --git a/CHANGELOG.md b/CHANGELOG.md index a4bf0872b..ac956496c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 diff --git a/crates/squawk_linter/src/ignore.rs b/crates/squawk_linter/src/ignore.rs index b1d80c1b1..1346b09e7 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 4dfcaf581..17371725c 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -72,6 +72,9 @@ use rules::{ ban_alter_identity, ban_drop_constraint, ban_drop_domain, ban_drop_extension, ban_drop_generated_expression, ban_drop_schema, ban_drop_sequence, ban_set_schema, renaming_object, + ban_alter_generated_expression, ban_disable_trigger, ban_drop_index, ban_drop_policy, + ban_replace_view_function, ban_replica_identity, ban_revoke, ban_set_default, + compatibility_additions, security_compatibility, }; // xtask:new-rule:rule-import @@ -131,6 +134,30 @@ pub enum Rule { BanSetSchema, BanAlterIdentity, BanDropExtension, + BanAlterGeneratedExpression, + BanDropIndex, + BanSetDefault, + BanDisableTrigger, + BanReplicaIdentity, + BanDropPolicy, + BanRevoke, + BanReplaceViewFunction, + BanAlterPolicyCondition, + BanAlterPolicyRoles, + BanCreatePolicy, + BanAlterFunctionOptions, + BanAlterViewOptions, + BanAlterRoleOptions, + BanAlterDatabaseOptions, + BanAlterRowLevelSecurity, + BanNewWriteRestriction, + BanAddEnumValue, + BanAddCompositeAttribute, + BanAddColumn, + BanDetachInheritance, + BanAlterSequenceValues, + BanAlterSystemOptions, + BanAlterExtension, // xtask:new-rule:error-name } @@ -141,7 +168,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 ) } @@ -218,6 +271,30 @@ impl TryFrom<&str> for Rule { "ban-set-schema" => Ok(Rule::BanSetSchema), "ban-alter-identity" => Ok(Rule::BanAlterIdentity), "ban-drop-extension" => Ok(Rule::BanDropExtension), + "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-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}")), } @@ -303,6 +380,30 @@ impl fmt::Display for Rule { Rule::BanSetSchema => "ban-set-schema", Rule::BanAlterIdentity => "ban-alter-identity", Rule::BanDropExtension => "ban-drop-extension", + 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::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}") @@ -599,6 +700,44 @@ impl Linter { if self.rules.contains(&Rule::BanDropExtension) { ban_drop_extension(self, file); } + if self.rules.contains(&Rule::BanAlterGeneratedExpression) { + ban_alter_generated_expression(self, file); + } + if self.rules.contains(&Rule::BanDropIndex) { + ban_drop_index(self, file); + } + if self.rules.contains(&Rule::BanSetDefault) { + ban_set_default(self, file); + } + if self.rules.contains(&Rule::BanDisableTrigger) { + ban_disable_trigger(self, file); + } + if self.rules.contains(&Rule::BanReplicaIdentity) { + ban_replica_identity(self, file); + } + if self.rules.contains(&Rule::BanDropPolicy) { + ban_drop_policy(self, file); + } + if self.rules.contains(&Rule::BanRevoke) { + ban_revoke(self, file); + } + if self.rules.contains(&Rule::BanReplaceViewFunction) { + ban_replace_view_function(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 @@ -777,4 +916,38 @@ mod tests { ); } } + + #[test] + fn compatibility_rules_require_explicit_configuration() { + for (rule, sql) in [ + ( + 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;"), + (Rule::BanAddEnumValue, "ALTER TYPE mood ADD VALUE 'new';"), + ] { + 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) + ); + assert!( + !Linter::with_rules(&[rule], &[rule]) + .lint(&parse, sql) + .iter() + .any(|v| v.code == rule) + ); + } + } } diff --git a/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs new file mode 100644 index 000000000..b43e0959d --- /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_disable_trigger.rs b/crates/squawk_linter/src/rules/ban_disable_trigger.rs new file mode 100644 index 000000000..79147d7de --- /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_index.rs b/crates/squawk_linter/src/rules/ban_drop_index.rs new file mode 100644 index 000000000..cb80db818 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_index.rs @@ -0,0 +1,37 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_index(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::DropIndex(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropIndex, + "Dropping an index may remove a guarantee or change query plans for existing clients.".into(), + node.syntax(), + )); + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "DROP INDEX CONCURRENTLY IF EXISTS i; DROP INDEX i;"; + let errors = lint_errors(sql, Rule::BanDropIndex); + assert_eq!(errors.matches("warning[ban-drop-index]").count(), 2); + assert_snapshot!(errors); + } + #[test] + fn ok() { + lint_ok("CREATE INDEX i ON t (id);", Rule::BanDropIndex); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_policy.rs b/crates/squawk_linter/src/rules/ban_drop_policy.rs new file mode 100644 index 000000000..91da93656 --- /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_replace_view_function.rs b/crates/squawk_linter/src/rules/ban_replace_view_function.rs new file mode 100644 index 000000000..64a3917c4 --- /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 000000000..b4e72a740 --- /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 000000000..88868d725 --- /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 000000000..2017b3572 --- /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/compatibility_additions.rs b/crates/squawk_linter/src/rules/compatibility_additions.rs new file mode 100644 index 000000000..08ddb0205 --- /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 f21b1cbb4..9c8107e53 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -5,9 +5,11 @@ pub(crate) mod adding_primary_key_constraint; pub(crate) mod adding_required_field; pub(crate) mod ban_alter_domain_with_add_constraint; pub(crate) mod ban_alter_identity; +pub(crate) mod ban_alter_generated_expression; 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; @@ -19,15 +21,22 @@ pub(crate) mod ban_drop_generated_expression; pub(crate) mod ban_drop_not_null; pub(crate) mod ban_drop_schema; pub(crate) mod ban_drop_sequence; +pub(crate) mod ban_drop_index; +pub(crate) mod ban_drop_policy; pub(crate) mod ban_drop_table; pub(crate) mod ban_drop_trigger; pub(crate) mod ban_drop_type; pub(crate) mod ban_drop_view; pub(crate) mod ban_duplicate_column_assignments; pub(crate) mod ban_set_schema; +pub(crate) mod ban_replace_view_function; +pub(crate) mod ban_replica_identity; +pub(crate) mod ban_revoke; +pub(crate) mod ban_set_default; 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; @@ -48,7 +57,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; @@ -58,9 +70,11 @@ pub(crate) use adding_primary_key_constraint::adding_primary_key_constraint; pub(crate) use adding_required_field::adding_required_field; pub(crate) use ban_alter_domain_with_add_constraint::ban_alter_domain_with_add_constraint; pub(crate) use ban_alter_identity::ban_alter_identity; +pub(crate) use ban_alter_generated_expression::ban_alter_generated_expression; 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; @@ -72,15 +86,22 @@ pub(crate) use ban_drop_generated_expression::ban_drop_generated_expression; pub(crate) use ban_drop_not_null::ban_drop_not_null; pub(crate) use ban_drop_schema::ban_drop_schema; pub(crate) use ban_drop_sequence::ban_drop_sequence; +pub(crate) use ban_drop_index::ban_drop_index; +pub(crate) use ban_drop_policy::ban_drop_policy; pub(crate) use ban_drop_table::ban_drop_table; pub(crate) use ban_drop_trigger::ban_drop_trigger; pub(crate) use ban_drop_type::ban_drop_type; pub(crate) use ban_drop_view::ban_drop_view; pub(crate) use ban_duplicate_column_assignments::ban_duplicate_column_assignments; pub(crate) use ban_set_schema::ban_set_schema; +pub(crate) use ban_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_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; @@ -101,5 +122,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/security_compatibility.rs b/crates/squawk_linter/src/rules/security_compatibility.rs new file mode 100644 index 000000000..254973128 --- /dev/null +++ b/crates/squawk_linter/src/rules/security_compatibility.rs @@ -0,0 +1,400 @@ +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::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::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}" + ); + 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 000000000..71d3a89ec --- /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_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 000000000..b65f377aa --- /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_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 000000000..49fbcc83e --- /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 000000000..05775b45a --- /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_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 000000000..fbf6ae32f --- /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 000000000..a098d6a7e --- /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 000000000..211d51955 --- /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 000000000..2ee52a655 --- /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/variant_tests.rs b/crates/squawk_linter/src/rules/variant_tests.rs new file mode 100644 index 000000000..d057c91cc --- /dev/null +++ b/crates/squawk_linter/src/rules/variant_tests.rs @@ -0,0 +1,107 @@ +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 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, + ); +} diff --git a/docs/docs/ban-add-column.md b/docs/docs/ban-add-column.md new file mode 100644 index 000000000..2355ed962 --- /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 000000000..05fa9ed2d --- /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 000000000..74792968d --- /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 000000000..e522e000e --- /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 000000000..023268466 --- /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 000000000..2fa7d3c2c --- /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 000000000..6642d3a30 --- /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-policy-condition.md b/docs/docs/ban-alter-policy-condition.md new file mode 100644 index 000000000..af1e2d073 --- /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 000000000..1932c7381 --- /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 000000000..122517e07 --- /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 000000000..181231f87 --- /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 000000000..e925bab74 --- /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 000000000..63b7f9ba4 --- /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 000000000..c5647150c --- /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 000000000..617bd1763 --- /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 000000000..2950d5fae --- /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 000000000..778730457 --- /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-index.md b/docs/docs/ban-drop-index.md new file mode 100644 index 000000000..ad704e9f7 --- /dev/null +++ b/docs/docs/ban-drop-index.md @@ -0,0 +1,18 @@ +--- +id: ban-drop-index +title: ban-drop-index +--- + +## problem + +Dropping an index can remove a unique or exclusion guarantee or change query plans. This rule is opt-in. + +```sql +DROP INDEX CONCURRENTLY IF EXISTS i; +``` + +## solution + +Check client queries and constraints before dropping the index. + +Enable this rule with `--include ban-drop-index` (or add `ban-drop-index` to your configured include list). diff --git a/docs/docs/ban-drop-policy.md b/docs/docs/ban-drop-policy.md new file mode 100644 index 000000000..92ad82833 --- /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-new-write-restriction.md b/docs/docs/ban-new-write-restriction.md new file mode 100644 index 000000000..92dc993d6 --- /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 000000000..e59f5e51e --- /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 000000000..1b2cf88b4 --- /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 000000000..af180b9da --- /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 000000000..7ff72de9c --- /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/sidebars.js b/docs/sidebars.js index 4e9aa28cc..9d831b032 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -57,6 +57,30 @@ module.exports = { "ban-set-schema", "ban-alter-identity", "ban-drop-extension", + "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-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 bd232ea26..0b20b97b4 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -276,6 +276,30 @@ const rules = [ { name: "ban-set-schema", tags: ["backwards compatibility"], description: "Prevent moving objects used by clients to another schema." }, { name: "ban-alter-identity", tags: ["backwards compatibility"], description: "Prevent changing identity columns used by clients." }, { name: "ban-drop-extension", tags: ["backwards compatibility"], description: "Prevent dropping extensions used by clients." }, + { 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-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 ]