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..836c295c --- /dev/null +++ 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; \ 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; +``` 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", 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": [