feat(linter): add opt-in rollback compatibility rules

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
 

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;
 

crates/squawk_linter/src/lib.rs

@@ -72,6 +72,9 @@ use rules::{
     ban_alter_identity, ban_drop_constraint, ban_drop_domain, ban_drop_extension,
     ban_drop_generated_expression, ban_drop_schema, ban_drop_sequence, ban_set_schema,
     renaming_object,
+    ban_alter_generated_expression, ban_disable_trigger, ban_drop_index, ban_drop_policy,
+    ban_replace_view_function, ban_replica_identity, ban_revoke, ban_set_default,
+    compatibility_additions, security_compatibility,
 };
 // xtask:new-rule:rule-import
 
@@ -131,6 +134,30 @@ pub enum Rule {
     BanSetSchema,
     BanAlterIdentity,
     BanDropExtension,
+    BanAlterGeneratedExpression,
+    BanDropIndex,
+    BanSetDefault,
+    BanDisableTrigger,
+    BanReplicaIdentity,
+    BanDropPolicy,
+    BanRevoke,
+    BanReplaceViewFunction,
+    BanAlterPolicyCondition,
+    BanAlterPolicyRoles,
+    BanCreatePolicy,
+    BanAlterFunctionOptions,
+    BanAlterViewOptions,
+    BanAlterRoleOptions,
+    BanAlterDatabaseOptions,
+    BanAlterRowLevelSecurity,
+    BanNewWriteRestriction,
+    BanAddEnumValue,
+    BanAddCompositeAttribute,
+    BanAddColumn,
+    BanDetachInheritance,
+    BanAlterSequenceValues,
+    BanAlterSystemOptions,
+    BanAlterExtension,
     // xtask:new-rule:error-name
 }
 
@@ -141,7 +168,33 @@ impl Rule {
         // require-timeout-settings is an alias, see `Rule::expands_to`
         matches!(
             self,
-            Rule::RequireTableSchema | Rule::RequireTimeoutSettings | Rule::BanDropTrigger
+            Rule::RequireTableSchema
+                | Rule::RequireTimeoutSettings
+                | Rule::BanDropTrigger
+                | Rule::BanAlterGeneratedExpression
+                | Rule::BanDropIndex
+                | Rule::BanSetDefault
+                | Rule::BanDisableTrigger
+                | Rule::BanReplicaIdentity
+                | Rule::BanDropPolicy
+                | Rule::BanRevoke
+                | Rule::BanReplaceViewFunction
+                | Rule::BanAlterPolicyCondition
+                | Rule::BanAlterPolicyRoles
+                | Rule::BanCreatePolicy
+                | Rule::BanAlterFunctionOptions
+                | Rule::BanAlterViewOptions
+                | Rule::BanAlterRoleOptions
+                | Rule::BanAlterDatabaseOptions
+                | Rule::BanAlterRowLevelSecurity
+                | Rule::BanNewWriteRestriction
+                | Rule::BanAddEnumValue
+                | Rule::BanAddCompositeAttribute
+                | Rule::BanAddColumn
+                | Rule::BanDetachInheritance
+                | Rule::BanAlterSequenceValues
+                | Rule::BanAlterSystemOptions
+                | Rule::BanAlterExtension
         )
     }
 
@@ -218,6 +271,30 @@ impl TryFrom<&str> for Rule {
             "ban-set-schema" => Ok(Rule::BanSetSchema),
             "ban-alter-identity" => Ok(Rule::BanAlterIdentity),
             "ban-drop-extension" => Ok(Rule::BanDropExtension),
+            "ban-alter-generated-expression" => Ok(Rule::BanAlterGeneratedExpression),
+            "ban-drop-index" => Ok(Rule::BanDropIndex),
+            "ban-set-default" => Ok(Rule::BanSetDefault),
+            "ban-disable-trigger" => Ok(Rule::BanDisableTrigger),
+            "ban-replica-identity" => Ok(Rule::BanReplicaIdentity),
+            "ban-drop-policy" => Ok(Rule::BanDropPolicy),
+            "ban-revoke" => Ok(Rule::BanRevoke),
+            "ban-replace-view-function" => Ok(Rule::BanReplaceViewFunction),
+            "ban-alter-policy-condition" => Ok(Rule::BanAlterPolicyCondition),
+            "ban-alter-policy-roles" => Ok(Rule::BanAlterPolicyRoles),
+            "ban-create-policy" => Ok(Rule::BanCreatePolicy),
+            "ban-alter-function-options" => Ok(Rule::BanAlterFunctionOptions),
+            "ban-alter-view-options" => Ok(Rule::BanAlterViewOptions),
+            "ban-alter-role-options" => Ok(Rule::BanAlterRoleOptions),
+            "ban-alter-database-options" => Ok(Rule::BanAlterDatabaseOptions),
+            "ban-alter-row-level-security" => Ok(Rule::BanAlterRowLevelSecurity),
+            "ban-new-write-restriction" => Ok(Rule::BanNewWriteRestriction),
+            "ban-add-enum-value" => Ok(Rule::BanAddEnumValue),
+            "ban-add-composite-attribute" => Ok(Rule::BanAddCompositeAttribute),
+            "ban-add-column" => Ok(Rule::BanAddColumn),
+            "ban-detach-inheritance" => Ok(Rule::BanDetachInheritance),
+            "ban-alter-sequence-values" => Ok(Rule::BanAlterSequenceValues),
+            "ban-alter-system-options" => Ok(Rule::BanAlterSystemOptions),
+            "ban-alter-extension" => Ok(Rule::BanAlterExtension),
             // xtask:new-rule:str-name
             _ => Err(format!("Unknown violation name: {s}")),
         }
@@ -303,6 +380,30 @@ impl fmt::Display for Rule {
             Rule::BanSetSchema => "ban-set-schema",
             Rule::BanAlterIdentity => "ban-alter-identity",
             Rule::BanDropExtension => "ban-drop-extension",
+            Rule::BanAlterGeneratedExpression => "ban-alter-generated-expression",
+            Rule::BanDropIndex => "ban-drop-index",
+            Rule::BanSetDefault => "ban-set-default",
+            Rule::BanDisableTrigger => "ban-disable-trigger",
+            Rule::BanReplicaIdentity => "ban-replica-identity",
+            Rule::BanDropPolicy => "ban-drop-policy",
+            Rule::BanRevoke => "ban-revoke",
+            Rule::BanReplaceViewFunction => "ban-replace-view-function",
+            Rule::BanAlterPolicyCondition => "ban-alter-policy-condition",
+            Rule::BanAlterPolicyRoles => "ban-alter-policy-roles",
+            Rule::BanCreatePolicy => "ban-create-policy",
+            Rule::BanAlterFunctionOptions => "ban-alter-function-options",
+            Rule::BanAlterViewOptions => "ban-alter-view-options",
+            Rule::BanAlterRoleOptions => "ban-alter-role-options",
+            Rule::BanAlterDatabaseOptions => "ban-alter-database-options",
+            Rule::BanAlterRowLevelSecurity => "ban-alter-row-level-security",
+            Rule::BanNewWriteRestriction => "ban-new-write-restriction",
+            Rule::BanAddEnumValue => "ban-add-enum-value",
+            Rule::BanAddCompositeAttribute => "ban-add-composite-attribute",
+            Rule::BanAddColumn => "ban-add-column",
+            Rule::BanDetachInheritance => "ban-detach-inheritance",
+            Rule::BanAlterSequenceValues => "ban-alter-sequence-values",
+            Rule::BanAlterSystemOptions => "ban-alter-system-options",
+            Rule::BanAlterExtension => "ban-alter-extension",
             // xtask:new-rule:variant-to-name
         };
         write!(f, "{val}")
@@ -599,6 +700,44 @@ impl Linter {
         if self.rules.contains(&Rule::BanDropExtension) {
             ban_drop_extension(self, file);
         }
+        if self.rules.contains(&Rule::BanAlterGeneratedExpression) {
+            ban_alter_generated_expression(self, file);
+        }
+        if self.rules.contains(&Rule::BanDropIndex) {
+            ban_drop_index(self, file);
+        }
+        if self.rules.contains(&Rule::BanSetDefault) {
+            ban_set_default(self, file);
+        }
+        if self.rules.contains(&Rule::BanDisableTrigger) {
+            ban_disable_trigger(self, file);
+        }
+        if self.rules.contains(&Rule::BanReplicaIdentity) {
+            ban_replica_identity(self, file);
+        }
+        if self.rules.contains(&Rule::BanDropPolicy) {
+            ban_drop_policy(self, file);
+        }
+        if self.rules.contains(&Rule::BanRevoke) {
+            ban_revoke(self, file);
+        }
+        if self.rules.contains(&Rule::BanReplaceViewFunction) {
+            ban_replace_view_function(self, file);
+        }
+        security_compatibility(self, file);
+        if [
+            Rule::BanNewWriteRestriction,
+            Rule::BanAddEnumValue,
+            Rule::BanAddCompositeAttribute,
+            Rule::BanAddColumn,
+            Rule::BanDetachInheritance,
+            Rule::BanAlterSequenceValues,
+        ]
+        .iter()
+        .any(|rule| self.rules.contains(rule))
+        {
+            compatibility_additions(self, file);
+        }
         // xtask:new-rule:rule-call
 
         // locate any ignores in the file
@@ -777,4 +916,38 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn compatibility_rules_require_explicit_configuration() {
+        for (rule, sql) in [
+            (
+                Rule::BanAlterGeneratedExpression,
+                "ALTER TABLE t ALTER COLUMN c SET EXPRESSION AS (id + 1);",
+            ),
+            (Rule::BanAddColumn, "ALTER TABLE t ADD COLUMN c int;"),
+            (Rule::BanReplicaIdentity, "DROP PUBLICATION p;"),
+            (Rule::BanAddEnumValue, "ALTER TYPE mood ADD VALUE 'new';"),
+        ] {
+            let parse = SourceFile::parse(sql);
+            assert!(parse.errors().is_empty());
+            assert!(
+                !Linter::with_default_rules()
+                    .lint(&parse, sql)
+                    .iter()
+                    .any(|v| v.code == rule)
+            );
+            assert!(
+                Linter::with_rules(&[rule], &[])
+                    .lint(&parse, sql)
+                    .iter()
+                    .any(|v| v.code == rule)
+            );
+            assert!(
+                !Linter::with_rules(&[rule], &[rule])
+                    .lint(&parse, sql)
+                    .iter()
+                    .any(|v| v.code == rule)
+            );
+        }
+    }
 }

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<SourceFile>) {
+    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,
+        );
+    }
+}

crates/squawk_linter/src/rules/ban_disable_trigger.rs

@@ -0,0 +1,73 @@
+use crate::{Linter, Rule, Violation};
+use squawk_syntax::{
+    Parse, SourceFile,
+    ast::{self, AstNode},
+};
+
+pub(crate) fn ban_disable_trigger(ctx: &mut Linter, parse: &Parse<SourceFile>) {
+    for stmt in parse.tree().stmts() {
+        if let ast::Stmt::AlterTable(table) = stmt {
+            for action in table.actions() {
+                let message = match action {
+                    ast::AlterTableAction::EnableReplicaTrigger(_)
+                    | ast::AlterTableAction::EnableReplicaRule(_) => {
+                        "Replica-only triggers and rules do not fire for normal application writes."
+                    }
+                    ast::AlterTableAction::DisableTrigger(_)
+                    | ast::AlterTableAction::EnableTrigger(_)
+                    | ast::AlterTableAction::EnableRule(_)
+                    | ast::AlterTableAction::DisableRule(_)
+                    | ast::AlterTableAction::EnableAlwaysTrigger(_)
+                    | ast::AlterTableAction::EnableAlwaysRule(_) => {
+                        "Changing trigger or rule firing may change database side effects for existing clients."
+                    }
+                    ast::AlterTableAction::DisableRls(_)
+                    | ast::AlterTableAction::ForceRls(_)
+                    | ast::AlterTableAction::NoForceRls(_) => {
+                        "Changing row level security can change visible rows or reject access for existing clients."
+                    }
+                    _ => continue,
+                };
+                ctx.report(Violation::for_node(
+                    Rule::BanDisableTrigger,
+                    message.into(),
+                    action.syntax(),
+                ));
+            }
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use crate::{
+        Rule,
+        test_utils::{lint_errors, lint_ok},
+    };
+    use insta::assert_snapshot;
+    #[test]
+    fn err() {
+        let sql = "ALTER TABLE t DISABLE TRIGGER trg; ALTER TABLE t DISABLE RULE r; ALTER TABLE t DISABLE ROW LEVEL SECURITY; ALTER TABLE t FORCE ROW LEVEL SECURITY;";
+        let errors = lint_errors(sql, Rule::BanDisableTrigger);
+        assert_eq!(errors.matches("warning[ban-disable-trigger]").count(), 4);
+        assert_snapshot!(errors);
+    }
+    #[test]
+    fn enable() {
+        let sql = "ALTER TABLE t ENABLE TRIGGER trg; ALTER TABLE t ENABLE RULE r;";
+        assert_eq!(
+            lint_errors(sql, Rule::BanDisableTrigger)
+                .matches("warning[ban-disable-trigger]")
+                .count(),
+            2
+        );
+    }
+
+    #[test]
+    fn ok() {
+        lint_ok(
+            "ALTER TABLE t ENABLE ROW LEVEL SECURITY;",
+            Rule::BanDisableTrigger,
+        );
+    }
+}