From c42d3cb34c3781e7ef396c13774e482b212def0a Mon Sep 17 00:00:00 2001 From: Julian Date: Wed, 19 Feb 2025 08:47:32 +0100 Subject: [PATCH 1/3] so far --- crates/pglt_analyser/src/lint/safety.rs | 3 +- .../src/lint/safety/adding_required_field.rs | 67 +++++++++++++++++++ crates/pglt_analyser/src/options.rs | 2 + .../safety/addingRequiredField/basic.sql | 2 + .../src/analyser/linter/rules.rs | 41 +++++++++--- .../src/categories.rs | 1 + 6 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 crates/pglt_analyser/src/lint/safety/adding_required_field.rs create mode 100644 crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql diff --git a/crates/pglt_analyser/src/lint/safety.rs b/crates/pglt_analyser/src/lint/safety.rs index b92fc752..2bf348be 100644 --- 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 new file mode 100644 index 00000000..780639a1 --- /dev/null +++ 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) -> Vec { + 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 index 3ff79743..6e9cae68 100644 --- 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 = + ::Options; pub type BanDropColumn = ::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 new file mode 100644 index 00000000..82afd1d8 --- /dev/null +++ b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql @@ -0,0 +1,2 @@ +-- expect_only_lint/safety/addingRequiredField +-- select 1; \ No newline at end of file diff --git a/crates/pglt_configuration/src/analyser/linter/rules.rs b/crates/pglt_configuration/src/analyser/linter/rules.rs index 47165c7d..8f8dcfae 100644 --- 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, + #[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>, #[doc = "Dropping a column may break existing clients."] #[serde(skip_serializing_if = "Option::is_none")] pub ban_drop_column: Option>, @@ -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> { 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> { 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)> { 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 index d20a0148..8a91cfb5 100644 --- 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", From e77d6b9671c860734faa26bfa84b90efa23e8e9f Mon Sep 17 00:00:00 2001 From: Julian Date: Wed, 19 Feb 2025 08:49:29 +0100 Subject: [PATCH 2/3] straightened --- .../specs/safety/addingRequiredField/basic.sql | 3 ++- .../safety/addingRequiredField/basic.sql.snap | 17 +++++++++++++++++ .../safety/addingRequiredField/with_default.sql | 3 +++ .../addingRequiredField/with_default.sql.snap | 10 ++++++++++ .../addingRequiredField/without_required.sql | 3 +++ .../without_required.sql.snap | 10 ++++++++++ 6 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql.snap create mode 100644 crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql create mode 100644 crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql.snap create mode 100644 crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql create mode 100644 crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql.snap diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql index 82afd1d8..836c295c 100644 --- a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql +++ b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql @@ -1,2 +1,3 @@ -- expect_only_lint/safety/addingRequiredField --- select 1; \ No newline at end of file +alter table test +add column c int not null; \ No newline at end of file diff --git a/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql.snap b/crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql.snap new file mode 100644 index 00000000..39d6cc8f --- /dev/null +++ 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 new file mode 100644 index 00000000..988f0a71 --- /dev/null +++ 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; \ No newline at end of file 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 new file mode 100644 index 00000000..505d87ec --- /dev/null +++ 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 new file mode 100644 index 00000000..1990edc1 --- /dev/null +++ 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; \ No newline at end of file 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 new file mode 100644 index 00000000..669dbbdd --- /dev/null +++ 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; +``` From 36a273024ca8f4b71e21273b69667d3f7dfcbef3 Mon Sep 17 00:00:00 2001 From: Julian Date: Wed, 19 Feb 2025 09:31:35 +0100 Subject: [PATCH 3/3] generate manually --- docs/rule_sources.md | 1 + docs/rules.md | 1 + docs/rules/adding-required-field.md | 31 +++++++++++++++++++++++++++++ docs/schemas/0.0.0/schema.json | 11 ++++++++++ docs/schemas/latest/schema.json | 11 ++++++++++ 5 files changed, 55 insertions(+) create mode 100644 docs/rules/adding-required-field.md diff --git a/docs/rule_sources.md b/docs/rule_sources.md index 73c71551..b5c1f49f 100644 --- 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 index e17cc51c..949443fe 100644 --- 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. | | | [banDropNotNull](./rules/ban-drop-not-null) | Dropping a NOT NULL constraint may break existing clients. | | | [banDropTable](./rules/ban-drop-table) | Dropping a table may break existing clients. | | diff --git a/docs/rules/adding-required-field.md b/docs/rules/adding-required-field.md new file mode 100644 index 00000000..8d9dff6d --- /dev/null +++ b/docs/rules/adding-required-field.md @@ -0,0 +1,31 @@ +# addingRequiredField +**Diagnostic Category: `lint/safety/addingRequiredField`** + +**Since**: `vnext` + + +**Sources**: +- Inspired from: squawk/adding-required-field + +## 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 index 9c6a4598..05981eb0 100644 --- 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 index 9c6a4598..05981eb0 100644 --- 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": [