diff --git a/CHANGELOG.md b/CHANGELOG.md index a4bf0872..ac956496 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 b1d80c1b..1346b09e 100644 --- a/crates/squawk_linter/src/ignore.rs +++ b/crates/squawk_linter/src/ignore.rs @@ -278,7 +278,7 @@ alter table t drop column c cascade; alter table t add column c char; ALTER TABLE foo --- squawk-ignore adding-field-with-default,prefer-robust-stmts +-- squawk-ignore adding-field-with-default,prefer-robust-stmts,ban-alter-generated-expression ADD COLUMN bar numeric GENERATED ALWAYS AS (bar + baz) STORED; diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 4dfcaf58..e5fee87c 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -69,9 +69,11 @@ use rules::require_table_schema; use rules::require_timeout_settings; use rules::transaction_nesting; use rules::{ - ban_alter_identity, ban_drop_constraint, ban_drop_domain, ban_drop_extension, - ban_drop_generated_expression, ban_drop_schema, ban_drop_sequence, ban_set_schema, - renaming_object, + ban_alter_generated_expression, ban_alter_identity, ban_disable_trigger, ban_drop_constraint, + ban_drop_domain, ban_drop_extension, ban_drop_generated_expression, ban_drop_index, + ban_drop_policy, ban_drop_schema, ban_drop_sequence, ban_replace_view_function, + ban_replica_identity, ban_revoke, ban_set_default, ban_set_schema, + existing_object_compatibility, renaming_object, security_compatibility, }; // xtask:new-rule:rule-import @@ -131,6 +133,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 +167,33 @@ impl Rule { // require-timeout-settings is an alias, see `Rule::expands_to` matches!( self, - Rule::RequireTableSchema | Rule::RequireTimeoutSettings | Rule::BanDropTrigger + Rule::RequireTableSchema + | Rule::RequireTimeoutSettings + | Rule::BanDropTrigger + | Rule::BanAlterGeneratedExpression + | Rule::BanDropIndex + | Rule::BanSetDefault + | Rule::BanDisableTrigger + | Rule::BanReplicaIdentity + | Rule::BanDropPolicy + | Rule::BanRevoke + | Rule::BanReplaceViewFunction + | Rule::BanAlterPolicyCondition + | Rule::BanAlterPolicyRoles + | Rule::BanCreatePolicy + | Rule::BanAlterFunctionOptions + | Rule::BanAlterViewOptions + | Rule::BanAlterRoleOptions + | Rule::BanAlterDatabaseOptions + | Rule::BanAlterRowLevelSecurity + | Rule::BanNewWriteRestriction + | Rule::BanAddEnumValue + | Rule::BanAddCompositeAttribute + | Rule::BanAddColumn + | Rule::BanDetachInheritance + | Rule::BanAlterSequenceValues + | Rule::BanAlterSystemOptions + | Rule::BanAlterExtension ) } @@ -218,6 +270,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 +379,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 +699,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)) + { + existing_object_compatibility(self, file); + } // xtask:new-rule:rule-call // locate any ignores in the file @@ -777,4 +915,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 00000000..b43e0959 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_alter_generated_expression.rs @@ -0,0 +1,50 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_alter_generated_expression(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(table) = stmt { + for action in table.actions() { + match action { + ast::AlterTableAction::AlterColumn(column) => { + if let Some(ast::AlterColumnOption::SetExpression(node)) = column.option() { + ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Changing a generated column expression may change values read by existing clients.".into(), node.syntax())); + } + } + ast::AlterTableAction::AddColumn(column) => { + for constraint in column.constraints() { + if let ast::Constraint::GeneratedConstraint(node) = constraint { + ctx.report(Violation::for_node(Rule::BanAlterGeneratedExpression, "Adding a generated column may break inserts from existing clients.".into(), node.syntax())); + } + } + } + _ => (), + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 2); ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED;"; + assert_snapshot!(lint_errors(sql, Rule::BanAlterGeneratedExpression)); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ADD COLUMN c int; ALTER TABLE t ALTER COLUMN c DROP EXPRESSION;", + Rule::BanAlterGeneratedExpression, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_disable_trigger.rs b/crates/squawk_linter/src/rules/ban_disable_trigger.rs new file mode 100644 index 00000000..be719185 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_disable_trigger.rs @@ -0,0 +1,86 @@ +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(_) => { + "Making a trigger or rule replica-only stops it firing for normal application writes. Earlier application revisions may depend on its side effects." + } + 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, + ); + } + + #[test] + fn enforcement_variants() { + let sql = "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;"; + let errors = lint_errors(sql, Rule::BanDisableTrigger); + assert_eq!(errors.matches("warning[ban-disable-trigger]").count(), 5); + assert_eq!( + errors + .matches("Earlier application revisions may depend on its side effects.") + .count(), + 2 + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_index.rs b/crates/squawk_linter/src/rules/ban_drop_index.rs new file mode 100644 index 00000000..cb80db81 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_index.rs @@ -0,0 +1,37 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_drop_index(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::DropIndex(node) = stmt { + ctx.report(Violation::for_node( + Rule::BanDropIndex, + "Dropping an index may remove a guarantee or change query plans for existing clients.".into(), + node.syntax(), + )); + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "DROP INDEX CONCURRENTLY IF EXISTS i; DROP INDEX i;"; + let errors = lint_errors(sql, Rule::BanDropIndex); + assert_eq!(errors.matches("warning[ban-drop-index]").count(), 2); + assert_snapshot!(errors); + } + #[test] + fn ok() { + lint_ok("CREATE INDEX i ON t (id);", Rule::BanDropIndex); + } +} diff --git a/crates/squawk_linter/src/rules/ban_drop_policy.rs b/crates/squawk_linter/src/rules/ban_drop_policy.rs new file mode 100644 index 00000000..b0a62d3a --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_drop_policy.rs @@ -0,0 +1,48 @@ +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); + lint_ok("ALTER 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 00000000..733d0a02 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_replace_view_function.rs @@ -0,0 +1,78 @@ +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); + } + + #[test] + fn replacement_variants() { + let sql = "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();"; + assert_eq!( + lint_errors(sql, Rule::BanReplaceViewFunction) + .matches("warning[ban-replace-view-function]") + .count(), + 3 + ); + lint_ok( + "CREATE RULE r AS ON INSERT TO t DO INSTEAD NOTHING;", + Rule::BanReplaceViewFunction, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_replica_identity.rs b/crates/squawk_linter/src/rules/ban_replica_identity.rs new file mode 100644 index 00000000..b4e72a74 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_replica_identity.rs @@ -0,0 +1,74 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_replica_identity(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::AlterTable(table) => { + for action in table.actions() { + if let ast::AlterTableAction::ReplicaIdentity(node) = action { + ctx.report(Violation::for_node(Rule::BanReplicaIdentity, "Changing replica identity may silently change replication for existing clients.".into(), node.syntax())); + } + } + } + ast::Stmt::DropPublication(node) => ctx.report(Violation::for_node( + Rule::BanReplicaIdentity, + "Dropping a publication stops replication for existing consumers.".into(), + node.syntax(), + )), + ast::Stmt::AlterPublication(publication) => { + if let Some(action) = publication.action() { + if matches!( + action, + ast::AlterPublicationAction::DropPublicationObjects(_) + | ast::AlterPublicationAction::SetPublicationObjects(_) + | ast::AlterPublicationAction::SetAllPublicationObjectList(_) + | ast::AlterPublicationAction::SetOptions(_) + ) { + ctx.report(Violation::for_node( + Rule::BanReplicaIdentity, + "Changing publication tables or options can stop or change replication for existing consumers.".into(), + action.syntax(), + )); + } + } + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t REPLICA IDENTITY FULL;"; + assert_snapshot!(lint_errors(sql, Rule::BanReplicaIdentity)); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ENABLE ROW LEVEL SECURITY; ALTER PUBLICATION p ADD TABLE t; CREATE PUBLICATION p FOR TABLE t;", + Rule::BanReplicaIdentity, + ); + } + + #[test] + fn publication_changes() { + let sql = "DROP PUBLICATION p; ALTER PUBLICATION p DROP TABLE t; ALTER PUBLICATION p SET TABLE t; ALTER PUBLICATION p SET (publish = 'insert'); ALTER PUBLICATION p SET TABLES IN SCHEMA public;"; + assert_eq!( + lint_errors(sql, Rule::BanReplicaIdentity) + .matches("warning[ban-replica-identity]") + .count(), + 5 + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_revoke.rs b/crates/squawk_linter/src/rules/ban_revoke.rs new file mode 100644 index 00000000..85cbaf98 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_revoke.rs @@ -0,0 +1,103 @@ +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); + } + + #[test] + fn group_membership_removal() { + let sql = "ALTER GROUP writers DROP USER app, worker;"; + assert_eq!( + lint_errors(sql, Rule::BanRevoke) + .matches("warning[ban-revoke]") + .count(), + 1 + ); + lint_ok( + "ALTER GROUP writers ADD USER app; ALTER GROUP writers RENAME TO editors;", + Rule::BanRevoke, + ); + } +} diff --git a/crates/squawk_linter/src/rules/ban_set_default.rs b/crates/squawk_linter/src/rules/ban_set_default.rs new file mode 100644 index 00000000..2017b357 --- /dev/null +++ b/crates/squawk_linter/src/rules/ban_set_default.rs @@ -0,0 +1,57 @@ +use crate::{Linter, Rule, Violation}; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +pub(crate) fn ban_set_default(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterDomain(domain) = &stmt { + if let Some(ast::AlterDomainAction::SetDefault(node)) = domain.action() { + ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); + } + } + if let ast::Stmt::AlterView(view) = &stmt { + if let Some(ast::AlterViewAction::AlterViewColumn(column)) = view.action() { + if let Some(ast::AlterViewColumnAction::SetDefault(node)) = + column.alter_view_column_action() + { + ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); + } + } + } + let actions = match stmt { + ast::Stmt::AlterTable(table) => table.actions(), + ast::Stmt::AlterForeignTable(table) => table.actions(), + _ => continue, + }; + for action in actions { + if let ast::AlterTableAction::AlterColumn(column) = action { + if let Some(ast::AlterColumnOption::SetDefault(node)) = column.option() { + ctx.report(Violation::for_node(Rule::BanSetDefault, "Setting a column default may silently change values written by existing clients.".into(), node.syntax())); + } + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + use insta::assert_snapshot; + #[test] + fn err() { + let sql = "ALTER TABLE t ALTER COLUMN c SET DEFAULT 1;"; + assert_snapshot!(lint_errors(sql, Rule::BanSetDefault)); + } + #[test] + fn ok() { + lint_ok( + "ALTER TABLE t ALTER COLUMN c DROP DEFAULT;", + Rule::BanSetDefault, + ); + } +} diff --git a/crates/squawk_linter/src/rules/existing_object_compatibility.rs b/crates/squawk_linter/src/rules/existing_object_compatibility.rs new file mode 100644 index 00000000..7264910e --- /dev/null +++ b/crates/squawk_linter/src/rules/existing_object_compatibility.rs @@ -0,0 +1,115 @@ +mod ban_add_column; +mod ban_add_composite_attribute; +mod ban_add_enum_value; +mod ban_alter_sequence_values; +mod ban_detach_inheritance; +mod ban_new_write_restriction; + +use rustc_hash::FxHashSet; +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +use crate::{Linter, Rule}; + +// Only unconditional CREATE statements establish that an object is new to this file. +pub(crate) fn existing_object_compatibility(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 !new_table { + if ctx.rules.contains(&Rule::BanAddColumn) { + ban_add_column::check(ctx, &action); + } + if ctx.rules.contains(&Rule::BanNewWriteRestriction) { + ban_new_write_restriction::check_table_action(ctx, &action); + } + if ctx.rules.contains(&Rule::BanAlterSequenceValues) { + ban_alter_sequence_values::check_table_action(ctx, &action); + } + if ctx.rules.contains(&Rule::BanDetachInheritance) { + ban_detach_inheritance::check(ctx, &action); + } + } + } + } + ast::Stmt::AlterForeignTable(table) => { + for action in table.actions() { + if ctx.rules.contains(&Rule::BanAddColumn) { + ban_add_column::check(ctx, &action); + } + if ctx.rules.contains(&Rule::BanNewWriteRestriction) { + ban_new_write_restriction::check_foreign_table_action(ctx, &action); + } + } + } + ast::Stmt::AlterDomain(domain) if ctx.rules.contains(&Rule::BanNewWriteRestriction) => { + ban_new_write_restriction::check_domain(ctx, &domain); + } + ast::Stmt::CreateIndex(index) if ctx.rules.contains(&Rule::BanNewWriteRestriction) => { + if 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())) + { + ban_new_write_restriction::check_index(ctx, &index); + } + } + 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 { + if ctx.rules.contains(&Rule::BanAddEnumValue) { + ban_add_enum_value::check(ctx, &ty); + } + if ctx.rules.contains(&Rule::BanAddCompositeAttribute) { + ban_add_composite_attribute::check(ctx, &ty); + } + } + } + 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 { + ban_alter_sequence_values::check_sequence(ctx, &seq); + } + } + _ => (), + } + } +} diff --git a/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_column.rs b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_column.rs new file mode 100644 index 00000000..5c857f03 --- /dev/null +++ b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_column.rs @@ -0,0 +1,53 @@ +use squawk_syntax::ast::{self, AstNode}; + +use crate::{Linter, Rule, Violation}; + +pub(super) fn check(ctx: &mut Linter, action: &ast::AlterTableAction) { + 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(), + )); + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + + #[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 + ); + } +} diff --git a/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_composite_attribute.rs b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_composite_attribute.rs new file mode 100644 index 00000000..2cf5349c --- /dev/null +++ b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_composite_attribute.rs @@ -0,0 +1,46 @@ +use squawk_syntax::ast::{self, AstNode}; + +use crate::{Linter, Rule, Violation}; + +pub(super) fn check(ctx: &mut Linter, ty: &ast::AlterType) { + if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() { + 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(), + )); + } + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + + #[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, + ); + } +} diff --git a/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_enum_value.rs b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_enum_value.rs new file mode 100644 index 00000000..627f21e4 --- /dev/null +++ b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_add_enum_value.rs @@ -0,0 +1,38 @@ +use squawk_syntax::ast::{self, AstNode}; + +use crate::{Linter, Rule, Violation}; + +pub(super) fn check(ctx: &mut Linter, ty: &ast::AlterType) { + if let Some(ast::AlterTypeAction::AddValue(node)) = ty.action() { + ctx.report(Violation::for_node( + Rule::BanAddEnumValue, + "Adding an enum value changes the set of values existing clients can receive.".into(), + node.syntax(), + )); + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + + #[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, + ); + } +} diff --git a/crates/squawk_linter/src/rules/existing_object_compatibility/ban_alter_sequence_values.rs b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_alter_sequence_values.rs new file mode 100644 index 00000000..c48983d0 --- /dev/null +++ b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_alter_sequence_values.rs @@ -0,0 +1,85 @@ +use squawk_syntax::ast::{self, AstNode}; + +use crate::{Linter, Rule, Violation}; + +pub(super) fn check_table_action(ctx: &mut Linter, action: &ast::AlterTableAction) { + 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())); + } + } + } + _ => (), + } + } + } +} + +pub(super) fn check_sequence(ctx: &mut Linter, seq: &ast::AlterSequence) { + 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(), + )); + } + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + + #[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/existing_object_compatibility/ban_detach_inheritance.rs b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_detach_inheritance.rs new file mode 100644 index 00000000..cccd63cd --- /dev/null +++ b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_detach_inheritance.rs @@ -0,0 +1,32 @@ +use squawk_syntax::ast::{self, AstNode}; + +use crate::{Linter, Rule, Violation}; + +pub(super) fn check(ctx: &mut Linter, action: &ast::AlterTableAction) { + 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())), + _ => (), + } +} + +#[cfg(test)] +mod test { + use crate::{ + Rule, + test_utils::{lint_errors, lint_ok}, + }; + + #[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, + ); + } +} diff --git a/crates/squawk_linter/src/rules/existing_object_compatibility/ban_new_write_restriction.rs b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_new_write_restriction.rs new file mode 100644 index 00000000..a258fb8a --- /dev/null +++ b/crates/squawk_linter/src/rules/existing_object_compatibility/ban_new_write_restriction.rs @@ -0,0 +1,205 @@ +use squawk_syntax::ast::{self, AstNode}; + +use crate::{Linter, Rule, Violation}; + +pub(super) fn check_table_action(ctx: &mut Linter, action: &ast::AlterTableAction) { + 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(), + )); + } + } + _ => (), + } +} + +pub(super) fn check_foreign_table_action(ctx: &mut Linter, action: &ast::AlterTableAction) { + 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(), + )); + } + } + _ => (), + } +} + +pub(super) fn check_domain(ctx: &mut Linter, domain: &ast::AlterDomain) { + 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())), + _ => (), + } + } +} + +pub(super) fn check_index(ctx: &mut Linter, index: &ast::CreateIndex) { + if index.unique_token().is_some() { + ctx.report(Violation::for_node(Rule::BanNewWriteRestriction, + "A unique index can reject writes from existing clients, even when created CONCURRENTLY.".into(), index.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 additional_write_constraint_forms() { + let sql = "ALTER TABLE t ADD COLUMN c bigint PRIMARY KEY; ALTER FOREIGN TABLE ft ADD COLUMN c int NOT NULL;"; + assert_eq!( + lint_errors(sql, Rule::BanNewWriteRestriction) + .matches("warning[ban-new-write-restriction]") + .count(), + 2 + ); + lint_ok( + "CREATE TABLE t (id bigint); ALTER TABLE t ADD COLUMN c bigint PRIMARY KEY; ALTER FOREIGN TABLE ft ADD COLUMN c int;", + Rule::BanNewWriteRestriction, + ); + } +} diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index f21b1cbb..40cfc5c8 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -4,10 +4,12 @@ pub(crate) mod adding_not_null_field; pub(crate) mod adding_primary_key_constraint; pub(crate) mod adding_required_field; pub(crate) mod ban_alter_domain_with_add_constraint; +pub(crate) mod ban_alter_generated_expression; pub(crate) mod ban_alter_identity; pub(crate) mod ban_char_field; pub(crate) mod ban_concurrent_index_creation_in_transaction; pub(crate) mod ban_create_domain_with_constraint; +pub(crate) mod ban_disable_trigger; pub(crate) mod ban_drop_column; pub(crate) mod ban_drop_constraint; pub(crate) mod ban_drop_database; @@ -16,7 +18,9 @@ pub(crate) mod ban_drop_domain; pub(crate) mod ban_drop_extension; pub(crate) mod ban_drop_function; pub(crate) mod ban_drop_generated_expression; +pub(crate) mod ban_drop_index; pub(crate) mod ban_drop_not_null; +pub(crate) mod ban_drop_policy; pub(crate) mod ban_drop_schema; pub(crate) mod ban_drop_sequence; pub(crate) mod ban_drop_table; @@ -24,12 +28,17 @@ pub(crate) mod ban_drop_trigger; pub(crate) mod ban_drop_type; pub(crate) mod ban_drop_view; pub(crate) mod ban_duplicate_column_assignments; +pub(crate) mod ban_replace_view_function; +pub(crate) mod ban_replica_identity; +pub(crate) mod ban_revoke; +pub(crate) mod ban_set_default; pub(crate) mod ban_set_schema; pub(crate) mod ban_truncate_cascade; pub(crate) mod ban_uncommitted_transaction; pub(crate) mod changing_column_type; pub(crate) mod constraint_missing_not_valid; pub(crate) mod disallow_unique_constraint; +pub(crate) mod existing_object_compatibility; pub(crate) mod identifier_too_long; pub(crate) mod prefer_bigint_over_int; pub(crate) mod prefer_bigint_over_smallint; @@ -48,6 +57,7 @@ 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; // xtask:new-rule:mod-decl @@ -57,10 +67,12 @@ pub(crate) use adding_not_null_field::adding_not_null_field; pub(crate) use adding_primary_key_constraint::adding_primary_key_constraint; pub(crate) use adding_required_field::adding_required_field; pub(crate) use ban_alter_domain_with_add_constraint::ban_alter_domain_with_add_constraint; +pub(crate) use ban_alter_generated_expression::ban_alter_generated_expression; pub(crate) use ban_alter_identity::ban_alter_identity; pub(crate) use ban_char_field::ban_char_field; pub(crate) use ban_concurrent_index_creation_in_transaction::ban_concurrent_index_creation_in_transaction; pub(crate) use ban_create_domain_with_constraint::ban_create_domain_with_constraint; +pub(crate) use ban_disable_trigger::ban_disable_trigger; pub(crate) use ban_drop_column::ban_drop_column; pub(crate) use ban_drop_constraint::ban_drop_constraint; pub(crate) use ban_drop_database::ban_drop_database; @@ -69,7 +81,9 @@ pub(crate) use ban_drop_domain::ban_drop_domain; pub(crate) use ban_drop_extension::ban_drop_extension; pub(crate) use ban_drop_function::ban_drop_function; pub(crate) use ban_drop_generated_expression::ban_drop_generated_expression; +pub(crate) use ban_drop_index::ban_drop_index; pub(crate) use ban_drop_not_null::ban_drop_not_null; +pub(crate) use ban_drop_policy::ban_drop_policy; pub(crate) use ban_drop_schema::ban_drop_schema; pub(crate) use ban_drop_sequence::ban_drop_sequence; pub(crate) use ban_drop_table::ban_drop_table; @@ -77,12 +91,17 @@ pub(crate) use ban_drop_trigger::ban_drop_trigger; pub(crate) use ban_drop_type::ban_drop_type; pub(crate) use ban_drop_view::ban_drop_view; pub(crate) use ban_duplicate_column_assignments::ban_duplicate_column_assignments; +pub(crate) use ban_replace_view_function::ban_replace_view_function; +pub(crate) use ban_replica_identity::ban_replica_identity; +pub(crate) use ban_revoke::ban_revoke; +pub(crate) use ban_set_default::ban_set_default; pub(crate) use ban_set_schema::ban_set_schema; pub(crate) use ban_truncate_cascade::ban_truncate_cascade; pub(crate) use ban_uncommitted_transaction::ban_uncommitted_transaction; pub(crate) use changing_column_type::changing_column_type; pub(crate) use constraint_missing_not_valid::constraint_missing_not_valid; pub(crate) use disallow_unique_constraint::disallow_unique_constraint; +pub(crate) use existing_object_compatibility::existing_object_compatibility; pub(crate) use identifier_too_long::identifier_too_long; pub(crate) use prefer_bigint_over_int::prefer_bigint_over_int; pub(crate) use prefer_bigint_over_smallint::prefer_bigint_over_smallint; @@ -101,5 +120,6 @@ pub(crate) use require_concurrent_reindex::require_concurrent_reindex; pub(crate) use require_enum_value_ordering::require_enum_value_ordering; pub(crate) use require_table_schema::require_table_schema; pub(crate) use require_timeout_settings::require_timeout_settings; +pub(crate) use security_compatibility::security_compatibility; pub(crate) use transaction_nesting::transaction_nesting; // xtask:new-rule:export diff --git a/crates/squawk_linter/src/rules/security_compatibility.rs b/crates/squawk_linter/src/rules/security_compatibility.rs new file mode 100644 index 00000000..8a58d573 --- /dev/null +++ b/crates/squawk_linter/src/rules/security_compatibility.rs @@ -0,0 +1,427 @@ +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::*; + use crate::test_utils::{lint_errors, lint_ok}; + + #[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 routine_options() { + let sql = "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;"; + assert_eq!( + lint_errors(sql, Rule::BanAlterFunctionOptions) + .matches("warning[ban-alter-function-options]") + .count(), + 5 + ); + lint_ok( + "ALTER PROCEDURE p() OWNER TO app;", + Rule::BanAlterFunctionOptions, + ); + } + + #[test] + fn user_options() { + let sql = "ALTER USER app NOBYPASSRLS; ALTER USER app IN DATABASE db SET search_path TO private; ALTER USER app RESET ALL;"; + assert_eq!( + lint_errors(sql, Rule::BanAlterRoleOptions) + .matches("warning[ban-alter-role-options]") + .count(), + 3 + ); + } + + #[test] + fn policy_and_configuration_variants() { + for (rule, sql) in [ + ( + Rule::BanAlterPolicyCondition, + "ALTER POLICY p ON t WITH CHECK (true);", + ), + ( + Rule::BanAlterViewOptions, + "ALTER VIEW v RESET (security_barrier);", + ), + ( + Rule::BanAlterRoleOptions, + "ALTER ROLE r SET search_path TO public;", + ), + (Rule::BanAlterRoleOptions, "ALTER ROLE r RESET search_path;"), + ( + Rule::BanAlterDatabaseOptions, + "ALTER DATABASE d WITH CONNECTION LIMIT 5;", + ), + ( + Rule::BanAlterDatabaseOptions, + "ALTER DATABASE d RESET search_path;", + ), + ( + Rule::BanAlterRowLevelSecurity, + "ALTER TABLE t NO FORCE ROW LEVEL SECURITY;", + ), + ] { + let parse = SourceFile::parse(sql); + assert!(parse.errors().is_empty(), "{sql}: {:?}", parse.errors()); + assert!( + Linter::from([rule]) + .lint(&parse, sql) + .iter() + .any(|v| v.code == rule), + "{sql}" + ); + } + } +} diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap new file mode 100644 index 00000000..71d3a89e --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_alter_generated_expression__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_alter_generated_expression.rs +expression: "lint_errors(sql, Rule::BanAlterGeneratedExpression)" +--- +warning[ban-alter-generated-expression]: Changing a generated column expression may change values read by existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 2); ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-alter-generated-expression]: Adding a generated column may break inserts from existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 2); ALTER TABLE t ADD COLUMN c int GENERATED ALWAYS AS (id + 1) STORED; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap new file mode 100644 index 00000000..b65f377a --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_disable_trigger__test__err.snap @@ -0,0 +1,20 @@ +--- +source: crates/squawk_linter/src/rules/ban_disable_trigger.rs +expression: errors +--- +warning[ban-disable-trigger]: Changing trigger or rule firing may change database side effects for existing clients. + ╭▸ +1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… + ╰╴ ━━━━━━━━━━━━━━━━━━━ +warning[ban-disable-trigger]: Changing trigger or rule firing may change database side effects for existing clients. + ╭▸ +1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… + ╰╴ ━━━━━━━━━━━━━━ +warning[ban-disable-trigger]: Changing row level security can change visible rows or reject access for existing clients. + ╭▸ +1 │ ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVE… + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-disable-trigger]: Changing row level security can change visible rows or reject access for existing clients. + ╭▸ +1 │ …LTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVEL SECURITY; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap new file mode 100644 index 00000000..49fbcc83 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_index__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_index.rs +expression: errors +--- +warning[ban-drop-index]: Dropping an index may remove a guarantee or change query plans for existing clients. + ╭▸ +1 │ DROP INDEX CONCURRENTLY IF EXISTS i; DROP INDEX i; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-drop-index]: Dropping an index may remove a guarantee or change query plans for existing clients. + ╭▸ +1 │ DROP INDEX CONCURRENTLY IF EXISTS i; DROP INDEX i; + ╰╴ ━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap new file mode 100644 index 00000000..05775b45 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_drop_policy__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_drop_policy.rs +expression: "lint_errors(sql, Rule::BanDropPolicy)" +--- +warning[ban-drop-policy]: Dropping a policy or rule may silently change behaviour for existing clients. + ╭▸ +1 │ DROP POLICY IF EXISTS p ON t; DROP RULE IF EXISTS r ON t; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-drop-policy]: Dropping a policy or rule may silently change behaviour for existing clients. + ╭▸ +1 │ DROP POLICY IF EXISTS p ON t; DROP RULE IF EXISTS r ON t; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap new file mode 100644 index 00000000..fbf6ae32 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replace_view_function__test__err.snap @@ -0,0 +1,16 @@ +--- +source: crates/squawk_linter/src/rules/ban_replace_view_function.rs +expression: "lint_errors(sql, Rule::BanReplaceViewFunction)" +--- +warning[ban-replace-view-function]: Replacing a view, function, or procedure may silently change behaviour for existing clients. + ╭▸ +1 │ CREATE OR REPLACE VIEW v AS SELECT 1 AS id; CREATE OR REPLACE FUNCTION f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$; CREATE OR REPLAC… + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-replace-view-function]: Replacing a view, function, or procedure may silently change behaviour for existing clients. + ╭▸ +1 │ CREATE OR REPLACE VIEW v AS SELECT 1 AS id; CREATE OR REPLACE FUNCTION f() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$; CREATE OR REPLAC… + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-replace-view-function]: Replacing a view, function, or procedure may silently change behaviour for existing clients. + ╭▸ +1 │ …nt LANGUAGE sql AS $$ SELECT 1 $$; CREATE OR REPLACE PROCEDURE p() LANGUAGE sql AS $$ SELECT 1 $$; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap new file mode 100644 index 00000000..a098d6a7 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_replica_identity__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_replica_identity.rs +expression: "lint_errors(sql, Rule::BanReplicaIdentity)" +--- +warning[ban-replica-identity]: Changing replica identity may silently change replication for existing clients. + ╭▸ +1 │ ALTER TABLE t REPLICA IDENTITY FULL; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap new file mode 100644 index 00000000..211d5195 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_revoke__test__err.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/ban_revoke.rs +expression: "lint_errors(sql, Rule::BanRevoke)" +--- +warning[ban-revoke]: Revoking privileges may break existing clients. + ╭▸ +1 │ REVOKE SELECT ON t FROM app; ALTER DEFAULT PRIVILEGES REVOKE SELECT ON TABLES FROM app; + ╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +warning[ban-revoke]: Revoking privileges may break existing clients. + ╭▸ +1 │ REVOKE SELECT ON t FROM app; ALTER DEFAULT PRIVILEGES REVOKE SELECT ON TABLES FROM app; + ╰╴ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap new file mode 100644 index 00000000..2ee52a65 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__ban_set_default__test__err.snap @@ -0,0 +1,8 @@ +--- +source: crates/squawk_linter/src/rules/ban_set_default.rs +expression: "lint_errors(sql, Rule::BanSetDefault)" +--- +warning[ban-set-default]: Setting a column default may silently change values written by existing clients. + ╭▸ +1 │ ALTER TABLE t ALTER COLUMN c SET DEFAULT 1; + ╰╴ ━━━━━━━━━━━━━ diff --git a/docs/docs/ban-add-column.md b/docs/docs/ban-add-column.md new file mode 100644 index 00000000..2355ed96 --- /dev/null +++ b/docs/docs/ban-add-column.md @@ -0,0 +1,18 @@ +--- +id: ban-add-column +title: ban-add-column +--- + +## problem + +Adding a column to an existing table or foreign table changes the shape of `SELECT *` results and can break positional inserts from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that new table. + +```sql +ALTER TABLE accounts ADD COLUMN status text; +``` + +## solution + +Check clients that use `SELECT *` or positional inserts before adding the column. + +Enable this rule with `--include ban-add-column`. diff --git a/docs/docs/ban-add-composite-attribute.md b/docs/docs/ban-add-composite-attribute.md new file mode 100644 index 00000000..2517a4ef --- /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 existing clients receive and can break positional composite input. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER TYPE address ADD ATTRIBUTE street text; +``` + +## solution + +Update clients to handle the new attribute before adding it. + +Enable this rule with `--include ban-add-composite-attribute`. diff --git a/docs/docs/ban-add-enum-value.md b/docs/docs/ban-add-enum-value.md new file mode 100644 index 00000000..8e822e40 --- /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 existing clients that do not handle it. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER TYPE mood ADD VALUE 'happy'; +``` + +## solution + +Update clients to handle the new enum value before adding it. + +Enable this rule with `--include ban-add-enum-value`. diff --git a/docs/docs/ban-alter-database-options.md b/docs/docs/ban-alter-database-options.md new file mode 100644 index 00000000..1630b4f6 --- /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 connection behaviour, name resolution, or results for existing clients in the database. This rule is opt-in. It does not report database renames, owner changes, or tablespace changes. + +```sql +ALTER DATABASE app SET search_path TO public; +ALTER DATABASE app WITH CONNECTION LIMIT 20; +``` + +Review clients before changing database settings. Enable this rule with `--include ban-alter-database-options`. diff --git a/docs/docs/ban-alter-extension.md b/docs/docs/ban-alter-extension.md new file mode 100644 index 00000000..02326846 --- /dev/null +++ b/docs/docs/ban-alter-extension.md @@ -0,0 +1,13 @@ +--- +id: ban-alter-extension +title: ban-alter-extension +--- + +`ALTER EXTENSION ... UPDATE` runs the extension's upgrade scripts. These scripts can remove functions, change function signatures, or change results for existing clients. `ALTER EXTENSION ... DROP` removes an object from the extension so that later extension changes no longer manage it. This rule is opt-in. It does not report `ALTER EXTENSION ... ADD`. `ALTER EXTENSION ... SET SCHEMA` is reported by `ban-set-schema`. + +```sql +ALTER EXTENSION postgis UPDATE TO '3.4.0'; +ALTER EXTENSION postgis DROP FUNCTION legacy_fn(); +``` + +Read the extension's upgrade notes and update clients before updating the extension. Enable this rule with `--include ban-alter-extension`. diff --git a/docs/docs/ban-alter-function-options.md b/docs/docs/ban-alter-function-options.md new file mode 100644 index 00000000..ec28e140 --- /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 clients that call them. This includes security modes and `SET` or `RESET` configuration options, such as `search_path`. This rule is opt-in. It does not report renames, ownership changes, or schema moves. + +```sql +ALTER FUNCTION f() SECURITY DEFINER; +ALTER PROCEDURE p() SET search_path TO private; +ALTER ROUTINE f() RESET ALL; +``` + +Review callers before changing the options. Enable this rule with `--include ban-alter-function-options`. diff --git a/docs/docs/ban-alter-generated-expression.md b/docs/docs/ban-alter-generated-expression.md new file mode 100644 index 00000000..6642d3a3 --- /dev/null +++ b/docs/docs/ban-alter-generated-expression.md @@ -0,0 +1,18 @@ +--- +id: ban-alter-generated-expression +title: ban-alter-generated-expression +--- + +## problem + +Adding a generated column changes the result shape for strict `SELECT *` decoders and can affect positional inserts. Inserts with explicit column lists do not inherently fail. `SET EXPRESSION` changes values returned to existing clients. For stored generated columns, PostgreSQL rewrites stored values; assess that rewrite separately as an availability concern. This rule is opt-in because compatibility depends on how clients use the column. + +```sql +ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1); +``` + +## solution + +Check client reads and positional inserts before adding a generated column. Check client reads before changing its expression. The default `ban-drop-generated-expression` rule checks `DROP EXPRESSION`. + +Enable this rule with `--include ban-alter-generated-expression` (or add `ban-alter-generated-expression` to your configured include list). diff --git a/docs/docs/ban-alter-policy-condition.md b/docs/docs/ban-alter-policy-condition.md new file mode 100644 index 00000000..f700f9b8 --- /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 existing clients can read or write. This rule is opt-in. It does not report policy renames or role-only changes. + +```sql +ALTER POLICY p ON accounts USING (owner_id = current_user_id()); +``` + +Review the condition and update clients before applying it. Enable this rule with `--include ban-alter-policy-condition`. diff --git a/docs/docs/ban-alter-policy-roles.md b/docs/docs/ban-alter-policy-roles.md new file mode 100644 index 00000000..448ee2c8 --- /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 access for existing clients that use those roles. This rule is opt-in. It does not report policy renames or condition-only changes. + +```sql +ALTER POLICY p ON accounts TO app_user; +``` + +Review the affected roles before applying the change. Enable this rule with `--include ban-alter-policy-roles`. diff --git a/docs/docs/ban-alter-role-options.md b/docs/docs/ban-alter-role-options.md new file mode 100644 index 00000000..b231aeac --- /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 existing clients that use the role. This rule checks role options and `SET` or `RESET` configuration options, including changes for a specific database. This rule is opt-in. It does not report role renames. + +```sql +ALTER ROLE app_user NOLOGIN; +ALTER ROLE app_user SET search_path TO public; +ALTER USER app_user IN DATABASE app_db RESET search_path; +``` + +Review clients before changing the role. Enable this rule with `--include ban-alter-role-options`. diff --git a/docs/docs/ban-alter-row-level-security.md b/docs/docs/ban-alter-row-level-security.md new file mode 100644 index 00000000..181231f8 --- /dev/null +++ b/docs/docs/ban-alter-row-level-security.md @@ -0,0 +1,12 @@ +--- +id: ban-alter-row-level-security +title: ban-alter-row-level-security +--- + +`ALTER TABLE` with `ENABLE`, `DISABLE`, `FORCE`, or `NO FORCE ROW LEVEL SECURITY` changes access for existing clients. This rule is opt-in. + +```sql +ALTER TABLE accounts ENABLE ROW LEVEL SECURITY; +``` + +Review table policies and client access before applying the change. Enable this rule with `--include ban-alter-row-level-security`. diff --git a/docs/docs/ban-alter-sequence-values.md b/docs/docs/ban-alter-sequence-values.md new file mode 100644 index 00000000..7355aa42 --- /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 reissue identifiers or change the range of values that existing clients receive. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER SEQUENCE ids RESTART WITH 1; +``` + +## solution + +Coordinate sequence changes with clients that use generated values. + +Enable this rule with `--include ban-alter-sequence-values`. diff --git a/docs/docs/ban-alter-system-options.md b/docs/docs/ban-alter-system-options.md new file mode 100644 index 00000000..63b7f9ba --- /dev/null +++ b/docs/docs/ban-alter-system-options.md @@ -0,0 +1,13 @@ +--- +id: ban-alter-system-options +title: ban-alter-system-options +--- + +`ALTER SYSTEM SET` and `ALTER SYSTEM RESET` change server configuration for every database and every client. Parameters such as `search_path`, `timezone`, `DateStyle`, `IntervalStyle`, `bytea_output`, and `standard_conforming_strings` change name resolution or result formats for existing clients. This rule is opt-in. + +```sql +ALTER SYSTEM SET timezone = 'UTC'; +ALTER SYSTEM RESET search_path; +``` + +Review clients before changing server settings. Enable this rule with `--include ban-alter-system-options`. diff --git a/docs/docs/ban-alter-view-options.md b/docs/docs/ban-alter-view-options.md new file mode 100644 index 00000000..1d8b6473 --- /dev/null +++ b/docs/docs/ban-alter-view-options.md @@ -0,0 +1,15 @@ +--- +id: ban-alter-view-options +title: ban-alter-view-options +--- + +`ALTER VIEW ... SET` and `ALTER VIEW ... RESET` change options on an existing view. Existing clients, including an earlier application revision after a rollback, may still use that view after a migration. Rolling back the application does not restore the previous view options. + +For example, setting `security_invoker = true` makes PostgreSQL check access to the underlying tables and apply row-level security as the caller rather than the view owner. An earlier application revision that could query the view before the migration may then get a permission error or see different rows. Changing `security_barrier` can change when view conditions are evaluated relative to caller-supplied functions, which can affect information exposure and query performance. Changing `check_option` can reject writes through the view that previously succeeded. + +```sql +ALTER VIEW v SET (security_invoker = true); +ALTER VIEW v RESET (security_barrier); +``` + +This rule is opt-in. It reports view option changes, not view renames or column default changes. Review the privileges, row-level security policies, and read and write queries of application revisions that may use the migrated database. Enable this rule with `--include ban-alter-view-options`. diff --git a/docs/docs/ban-create-policy.md b/docs/docs/ban-create-policy.md new file mode 100644 index 00000000..6ae19e1e --- /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 existing clients when row level security is enabled. Review the policy command, roles, and conditions before applying it. This rule is opt-in. + +```sql +CREATE POLICY p ON accounts TO app_user USING (owner_id = current_user_id()); +``` + +Enable this rule with `--include ban-create-policy`. diff --git a/docs/docs/ban-detach-inheritance.md b/docs/docs/ban-detach-inheritance.md new file mode 100644 index 00000000..46a2e768 --- /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 existing clients access through a parent table. This rule is opt-in. An unconditional CREATE earlier in the file suppresses a warning for that new object. + +```sql +ALTER TABLE parent DETACH PARTITION child CONCURRENTLY; +``` + +## solution + +Update clients that use the parent table before detaching the child. + +Enable this rule with `--include ban-detach-inheritance`. diff --git a/docs/docs/ban-disable-trigger.md b/docs/docs/ban-disable-trigger.md new file mode 100644 index 00000000..a5464ad8 --- /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 for existing clients. `ENABLE REPLICA TRIGGER` and `ENABLE REPLICA RULE` stop the trigger or rule from firing for normal application writes. An earlier application revision may depend on the side effects that those writes previously produced. Some enforcement changes cause explicit errors. This rule is opt-in. + +```sql +ALTER TABLE t DISABLE TRIGGER trg; +``` + +## solution + +Test the earlier application revision against the migrated database before changing trigger, rule, or row level security enforcement. Keep any trigger or rule that its normal writes require active for normal application sessions. + +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 00000000..b3403cb1 --- /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 for existing clients. This rule is opt-in. + +```sql +DROP INDEX CONCURRENTLY IF EXISTS i; +``` + +## solution + +Check client queries and constraints before dropping the index. + +Enable this rule with `--include ban-drop-index` (or add `ban-drop-index` to your configured include list). diff --git a/docs/docs/ban-drop-policy.md b/docs/docs/ban-drop-policy.md new file mode 100644 index 00000000..8f4bdbb5 --- /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 existing 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 00000000..92dc993d --- /dev/null +++ b/docs/docs/ban-new-write-restriction.md @@ -0,0 +1,21 @@ +--- +id: ban-new-write-restriction +title: ban-new-write-restriction +--- + +## problem + +New table, foreign-table, and domain constraints, `NOT VALID` constraints, `SET NOT NULL`, named `NOT NULL` constraints, constrained added columns (including inline primary keys), changed constraint timing, and unique indexes (including `CONCURRENTLY`) can reject writes from existing clients. This rule is opt-in. An unconditional `CREATE TABLE` earlier in the file suppresses a warning for that table. The rule does not know whether an object existed before the migration. + +```sql +ALTER TABLE t ADD CONSTRAINT c CHECK (id > 0) NOT VALID; +ALTER TABLE t ADD CONSTRAINT id_required NOT NULL id; +ALTER TABLE t ADD COLUMN external_id bigint PRIMARY KEY; +ALTER FOREIGN TABLE ft ADD COLUMN external_id bigint NOT NULL; +``` + +## solution + +Update client writes before adding the restriction. + +Enable this rule with `--include ban-new-write-restriction`. diff --git a/docs/docs/ban-replace-view-function.md b/docs/docs/ban-replace-view-function.md new file mode 100644 index 00000000..39d64e3e --- /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 for existing clients without changing their SQL. This rule is opt-in. + +```sql +CREATE OR REPLACE VIEW v AS SELECT 1 AS id; +``` + +## solution + +Deploy compatible clients before replacing the view, function, procedure, trigger, rule, or aggregate. + +Enable this rule with `--include ban-replace-view-function` (or add `ban-replace-view-function` to your configured include list). diff --git a/docs/docs/ban-replica-identity.md b/docs/docs/ban-replica-identity.md new file mode 100644 index 00000000..0e5c2991 --- /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. Existing clients that depend on the replicated data may then see stale, missing, or changed data. This rule is opt-in. + +```sql +ALTER TABLE t REPLICA IDENTITY FULL; +ALTER PUBLICATION p DROP TABLE t; +``` + +## solution + +Update replication consumers before changing replica identity or publication membership and options. + +Enable this rule with `--include ban-replica-identity` (or add `ban-replica-identity` to your configured include list). diff --git a/docs/docs/ban-revoke.md b/docs/docs/ban-revoke.md new file mode 100644 index 00000000..70a839d2 --- /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 queries from existing clients fail. `DROP OWNED` can also remove objects owned by the role. This rule is opt-in. + +```sql +REVOKE SELECT ON t FROM app; +ALTER GROUP writers DROP USER app; +``` + +## solution + +Move clients to a role with the required privileges before revoking them. + +Enable this rule with `--include ban-revoke` (or add `ban-revoke` to your configured include list). diff --git a/docs/docs/ban-set-default.md b/docs/docs/ban-set-default.md new file mode 100644 index 00000000..2b55ba05 --- /dev/null +++ b/docs/docs/ban-set-default.md @@ -0,0 +1,18 @@ +--- +id: ban-set-default +title: ban-set-default +--- + +## problem + +A changed default can make existing clients write different values when they omit a column of a table, foreign table, or view, or a domain value. 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 4e9aa28c..9d831b03 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 bd232ea2..0b20b97b 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 ]