feat(rules): adding-required-field (#208)

juleswritescode · web-flow · commit 6d904d894c7a · 2025-02-20T10:36:19.000+01:00
diff --git a/crates/pglt_analyser/src/lint/safety.rs b/crates/pglt_analyser/src/lint/safety.rs
@@ -1,7 +1,8 @@
 //! Generated file, do not edit by hand, see `xtask/codegen`
 
 use pglt_analyse::declare_lint_group;
+pub mod adding_required_field;
 pub mod ban_drop_column;
 pub mod ban_drop_not_null;
 pub mod ban_drop_table;
-declare_lint_group! { pub Safety { name : "safety" , rules : [self :: ban_drop_column :: BanDropColumn , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable ,] } }
+declare_lint_group! { pub Safety { name : "safety" , rules : [self :: adding_required_field :: AddingRequiredField , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable ,] } }
diff --git a/crates/pglt_analyser/src/lint/safety/adding_required_field.rs b/crates/pglt_analyser/src/lint/safety/adding_required_field.rs
@@ -0,0 +1,67 @@
+use pglt_analyse::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource};
+use pglt_console::markup;
+
+declare_lint_rule! {
+    /// Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required.
+    ///
+    /// This will fail immediately upon running for any populated table. Furthermore, old application code that is unaware of this column will fail to INSERT to this table.
+    ///
+    /// Make new columns optional initially by omitting the NOT NULL constraint until all existing data and application code has been updated. Once no NULL values are written to or persisted in the database, set it to NOT NULL.
+    /// Alternatively, if using Postgres version 11 or later, add a DEFAULT value that is not volatile. This allows the column to keep its NOT NULL constraint.
+    ///
+    /// ## Invalid
+    /// alter table test add column count int not null;
+    ///
+    /// ## Valid in Postgres >= 11
+    /// alter table test add column count int not null default 0;
+    pub AddingRequiredField {
+        version: "next",
+        name: "addingRequiredField",
+        recommended: false,
+        sources: &[RuleSource::Squawk("adding-required-field")],
+    }
+}
+
+impl Rule for AddingRequiredField {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = vec![];
+
+        if let pglt_query_ext::NodeEnum::AlterTableStmt(stmt) = ctx.stmt() {
+            // We are currently lacking a way to check if a `AtAddColumn` subtype sets a
+            // not null constraint – so we'll need to check the plain SQL.
+            let plain_sql = ctx.stmt().to_ref().deparse().unwrap().to_ascii_lowercase();
+            let is_nullable = !plain_sql.contains("not null");
+            let has_set_default = plain_sql.contains("default");
+            if is_nullable || has_set_default {
+                return diagnostics;
+            }
+
+            for cmd in &stmt.cmds {
+                if let Some(pglt_query_ext::NodeEnum::AlterTableCmd(alter_table_cmd)) = &cmd.node {
+                    if alter_table_cmd.subtype()
+                        == pglt_query_ext::protobuf::AlterTableType::AtAddColumn
+                    {
+                        diagnostics.push(
+                            RuleDiagnostic::new(
+                                rule_category!(),
+                                None,
+                                markup! {
+                                    "Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required."
+                                },
+                            )
+                            .detail(
+                                None,
+                                "Make new columns optional initially by omitting the NOT NULL constraint until all existing data and application code has been updated. Once no NULL values are written to or persisted in the database, set it to NOT NULL. Alternatively, if using Postgres version 11 or later, add a DEFAULT value that is not volatile. This allows the column to keep its NOT NULL constraint.
+                                ",
+                        ),
+                    );
+                    }
+                }
+            }
+        }
+
+        diagnostics
+    }
+}
diff --git a/crates/pglt_analyser/src/options.rs b/crates/pglt_analyser/src/options.rs
@@ -1,6 +1,8 @@
 //! Generated file, do not edit by hand, see `xtask/codegen`
 
 use crate::lint;
+pub type AddingRequiredField =
+    <lint::safety::adding_required_field::AddingRequiredField as pglt_analyse::Rule>::Options;
 pub type BanDropColumn =
     <lint::safety::ban_drop_column::BanDropColumn as pglt_analyse::Rule>::Options;
 pub type BanDropNotNull =
diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql
@@ -0,0 +1,3 @@
+-- expect_only_lint/safety/addingRequiredField
+alter table test
+add column c int not null;
diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql.snap b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql.snap
@@ -0,0 +1,17 @@
+---
+source: crates/pglt_analyser/tests/rules_tests.rs
+expression: snapshot
+---
+# Input
+```
+-- expect_only_lint/safety/addingRequiredField
+alter table test
+add column c int not null;
+```
+
+# Diagnostics
+lint/safety/addingRequiredField ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required.
+  
+  i Make new columns optional initially by omitting the NOT NULL constraint until all existing data and application code has been updated. Once no NULL values are written to or persisted in the database, set it to NOT NULL. Alternatively, if using Postgres version 11 or later, add a DEFAULT value that is not volatile. This allows the column to keep its NOT NULL constraint.
diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql
@@ -0,0 +1,3 @@
+-- expect_no_diagnostics
+alter table test
+add column c int not null default 0;
diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql.snap b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql.snap
@@ -0,0 +1,10 @@
+---
+source: crates/pglt_analyser/tests/rules_tests.rs
+expression: snapshot
+---
+# Input
+```
+-- expect_no_diagnostics
+alter table test
+add column c int not null default 0;
+```
diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql
@@ -0,0 +1,3 @@
+-- expect_no_diagnostics
+alter table test
+add column c int;
diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql.snap b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql.snap
@@ -0,0 +1,10 @@
+---
+source: crates/pglt_analyser/tests/rules_tests.rs
+expression: snapshot
+---
+# Input
+```
+-- expect_no_diagnostics
+alter table test
+add column c int;
+```
diff --git a/crates/pglt_configuration/src/analyser/linter/rules.rs b/crates/pglt_configuration/src/analyser/linter/rules.rs
@@ -143,6 +143,10 @@ pub struct Safety {
     #[doc = r" It enables ALL rules for this group."]
     #[serde(skip_serializing_if = "Option::is_none")]
     pub all: Option<bool>,
+    #[doc = "Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required."]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub adding_required_field:
+        Option<RuleConfiguration<pglt_analyser::options::AddingRequiredField>>,
     #[doc = "Dropping a column may break existing clients."]
     #[serde(skip_serializing_if = "Option::is_none")]
     pub ban_drop_column: Option<RuleConfiguration<pglt_analyser::options::BanDropColumn>>,
@@ -155,19 +159,24 @@ pub struct Safety {
 }
 impl Safety {
     const GROUP_NAME: &'static str = "safety";
-    pub(crate) const GROUP_RULES: &'static [&'static str] =
-        &["banDropColumn", "banDropNotNull", "banDropTable"];
+    pub(crate) const GROUP_RULES: &'static [&'static str] = &[
+        "addingRequiredField",
+        "banDropColumn",
+        "banDropNotNull",
+        "banDropTable",
+    ];
     const RECOMMENDED_RULES: &'static [&'static str] =
         &["banDropColumn", "banDropNotNull", "banDropTable"];
     const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
-        RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
         RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
         RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]),
+        RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]),
     ];
     const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
         RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
         RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
         RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]),
+        RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]),
     ];
     #[doc = r" Retrieves the recommended rules"]
     pub(crate) fn is_recommended_true(&self) -> bool {
@@ -184,40 +193,50 @@ impl Safety {
     }
     pub(crate) fn get_enabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
         let mut index_set = FxHashSet::default();
-        if let Some(rule) = self.ban_drop_column.as_ref() {
+        if let Some(rule) = self.adding_required_field.as_ref() {
             if rule.is_enabled() {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
             }
         }
-        if let Some(rule) = self.ban_drop_not_null.as_ref() {
+        if let Some(rule) = self.ban_drop_column.as_ref() {
             if rule.is_enabled() {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
             }
         }
-        if let Some(rule) = self.ban_drop_table.as_ref() {
+        if let Some(rule) = self.ban_drop_not_null.as_ref() {
             if rule.is_enabled() {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]));
             }
         }
+        if let Some(rule) = self.ban_drop_table.as_ref() {
+            if rule.is_enabled() {
+                index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]));
+            }
+        }
         index_set
     }
     pub(crate) fn get_disabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
         let mut index_set = FxHashSet::default();
-        if let Some(rule) = self.ban_drop_column.as_ref() {
+        if let Some(rule) = self.adding_required_field.as_ref() {
             if rule.is_disabled() {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
             }
         }
-        if let Some(rule) = self.ban_drop_not_null.as_ref() {
+        if let Some(rule) = self.ban_drop_column.as_ref() {
             if rule.is_disabled() {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
             }
         }
-        if let Some(rule) = self.ban_drop_table.as_ref() {
+        if let Some(rule) = self.ban_drop_not_null.as_ref() {
             if rule.is_disabled() {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]));
             }
         }
+        if let Some(rule) = self.ban_drop_table.as_ref() {
+            if rule.is_disabled() {
+                index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]));
+            }
+        }
         index_set
     }
     #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"]
@@ -254,6 +273,10 @@ impl Safety {
         rule_name: &str,
     ) -> Option<(RulePlainConfiguration, Option<RuleOptions>)> {
         match rule_name {
+            "addingRequiredField" => self
+                .adding_required_field
+                .as_ref()
+                .map(|conf| (conf.level(), conf.get_options())),
             "banDropColumn" => self
                 .ban_drop_column
                 .as_ref()
diff --git a/crates/pglt_diagnostics_categories/src/categories.rs b/crates/pglt_diagnostics_categories/src/categories.rs
@@ -13,6 +13,7 @@
 // must be between `define_categories! {\n` and `\n    ;\n`.
 
 define_categories! {
+    "lint/safety/addingRequiredField": "https://pglt.dev/linter/rules/adding-required-field",
     "lint/safety/banDropColumn": "https://pglt.dev/linter/rules/ban-drop-column",
     "lint/safety/banDropNotNull": "https://pglt.dev/linter/rules/ban-drop-not-null",
     "lint/safety/banDropTable": "https://pglt.dev/linter/rules/ban-drop-table",
diff --git a/docs/rule_sources.md b/docs/rule_sources.md
@@ -3,6 +3,7 @@
 ### Squawk
 | Squawk Rule Name | Rule Name |
 | ---- | ---- |
+| [adding-required-field](https://squawkhq.com/docs/adding-required-field) |[addingRequiredField](./rules/adding-required-field) |
 | [ban-drop-column](https://squawkhq.com/docs/ban-drop-column) |[banDropColumn](./rules/ban-drop-column) |
 | [ban-drop-not-null](https://squawkhq.com/docs/ban-drop-not-null) |[banDropNotNull](./rules/ban-drop-not-null) |
 | [ban-drop-table](https://squawkhq.com/docs/ban-drop-table) |[banDropTable](./rules/ban-drop-table) |
diff --git a/docs/rules.md b/docs/rules.md
@@ -11,6 +11,7 @@ Below the list of rules supported by Postgres Language Tools, divided by group.
 Rules that detect potential safety issues in your code.
 | Rule name | Description | Properties |
 | --- | --- | --- |
+| [addingRequiredField](./rules/adding-required-field) | Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required. |  |
 | [banDropColumn](./rules/ban-drop-column) | Dropping a column may break existing clients. | <span class='inline-icon' title="This rule is recommended" ><Icon name="approve-check-circle" size="1.2rem" label="This rule is recommended" /></span> |
 | [banDropNotNull](./rules/ban-drop-not-null) | Dropping a NOT NULL constraint may break existing clients. | <span class='inline-icon' title="This rule is recommended" ><Icon name="approve-check-circle" size="1.2rem" label="This rule is recommended" /></span> |
 | [banDropTable](./rules/ban-drop-table) | Dropping a table may break existing clients. | <span class='inline-icon' title="This rule is recommended" ><Icon name="approve-check-circle" size="1.2rem" label="This rule is recommended" /></span> |
diff --git a/docs/rules/adding-required-field.md b/docs/rules/adding-required-field.md
@@ -0,0 +1,31 @@
+# addingRequiredField
+**Diagnostic Category: `lint/safety/addingRequiredField`**
+
+**Since**: `vnext`
+
+
+**Sources**: 
+- Inspired from: <a href="https://squawkhq.com/docs/adding-required-field" target="_blank"><code>squawk/adding-required-field</code></a>
+
+## Description
+Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required.
+
+This will fail immediately upon running for any populated table. Furthermore, old application code that is unaware of this column will fail to INSERT to this table.
+
+Make new columns optional initially by omitting the NOT NULL constraint until all existing data and application code has been updated. Once no NULL values are written to or persisted in the database, set it to NOT NULL.
+Alternatively, if using Postgres version 11 or later, add a DEFAULT value that is not volatile. This allows the column to keep its NOT NULL constraint.
+
+## Invalid
+
+alter table test add column count int not null;
+
+## Valid in Postgres >= 11
+
+alter table test add column count int not null default 0;
+
+## How to configure
+```toml title="pglt.toml"
+[linter.rules.safety]
+addingRequiredField = "error"
+
+```
diff --git a/docs/schemas/0.0.0/schema.json b/docs/schemas/0.0.0/schema.json
@@ -292,6 +292,17 @@
       "description": "A list of rules that belong to this group",
       "type": "object",
       "properties": {
+        "addingRequiredField": {
+          "description": "Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required.",
+          "anyOf": [
+            {
+              "$ref": "#/definitions/RuleConfiguration"
+            },
+            {
+              "type": "null"
+            }
+          ]
+        },
         "all": {
           "description": "It enables ALL rules for this group.",
           "type": [
diff --git a/docs/schemas/latest/schema.json b/docs/schemas/latest/schema.json
@@ -292,6 +292,17 @@
       "description": "A list of rules that belong to this group",
       "type": "object",
       "properties": {
+        "addingRequiredField": {
+          "description": "Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required.",
+          "anyOf": [
+            {
+              "$ref": "#/definitions/RuleConfiguration"
+            },
+            {
+              "type": "null"
+            }
+          ]
+        },
         "all": {
           "description": "It enables ALL rules for this group.",
           "type": [

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+-- expect_only_lint/safety/addingRequiredField`
	`2`	`+alter table test`
	`3`	`+add column c int not null;`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+-- expect_no_diagnostics`
	`2`	`+alter table test`
	`3`	`+add column c int not null default 0;`