Skip to content

Commit cbc49c8

Browse files
juleswritescodejuleswritescode
committed
feat(rules): adding-required-field
1 parent 4504d68 commit cbc49c8

File tree

13 files changed

+167
-10
lines changed

13 files changed

+167
-10
lines changed

‎crates/pglt_analyser/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,8 @@ mod tests {
9292
String::from_utf8(buffer).unwrap()
9393
}
9494

95-
const SQL: &str = r#"alter table test drop column id;"#;
96-
let rule_filter = RuleFilter::Rule("safety", "banDropColumn");
95+
const SQL: &str = r#"alter table test add column count int not null;"#;
96+
let rule_filter = RuleFilter::Rule("safety", "addingRequiredField");
9797

9898
let filter = AnalysisFilter {
9999
enabled_rules: Some(slice::from_ref(&rule_filter)),

‎crates/pglt_analyser/src/lint.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
//! Generated file, do not edit by hand, see `xtask/codegen`
22
33
pub mod safety;
4-
::pglt_analyse::declare_category! { pub Lint { kind : Lint , groups : [self :: safety :: Safety , ] } }
4+
::pglt_analyse::declare_category! { pub Lint { kind : Lint , groups : [self :: safety :: Safety ,] } }

‎crates/pglt_analyser/src/lint/safety.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
//! Generated file, do not edit by hand, see `xtask/codegen`
22
33
use pglt_analyse::declare_lint_group;
4+
pub mod adding_required_field;
45
pub mod ban_drop_column;
56
pub mod ban_drop_not_null;
67
pub mod ban_drop_table;
7-
declare_lint_group! { pub Safety { name : "safety" , rules : [self :: ban_drop_column :: BanDropColumn , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable ,] } }
8+
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 ,] } }

‎crates/pglt_analyser/src/lint/safety/adding_required_field.rs

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
use pglt_analyse::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource};
2+
use pglt_console::markup;
3+
4+
declare_lint_rule! {
5+
/// Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required.
6+
///
7+
/// 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.
8+
///
9+
/// 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.
10+
/// 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.
11+
///
12+
/// ## Invalid
13+
/// alter table test add column count int not null;
14+
///
15+
/// ## Valid in Postgres >= 11
16+
/// alter table test add column count int not null default 0;
17+
pub AddingRequiredField {
18+
version: "next",
19+
name: "addingRequiredField",
20+
recommended: false,
21+
sources: &[RuleSource::Squawk("adding-required-field")],
22+
}
23+
}
24+
25+
impl Rule for AddingRequiredField {
26+
type Options = ();
27+
28+
fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
29+
let mut diagnostics = vec![];
30+
31+
if let pglt_query_ext::NodeEnum::AlterTableStmt(stmt) = ctx.stmt() {
32+
// We are currently lacking a way to check if a `AtAddColumn` subtype sets a
33+
// not null constraint – so we'll need to check the plain SQL.
34+
let plain_sql = ctx.stmt().to_ref().deparse().unwrap().to_ascii_lowercase();
35+
let is_nullable = !plain_sql.contains("not null");
36+
let has_set_default = plain_sql.contains("default");
37+
if is_nullable || has_set_default {
38+
return diagnostics;
39+
}
40+
41+
for cmd in &stmt.cmds {
42+
if let Some(pglt_query_ext::NodeEnum::AlterTableCmd(alter_table_cmd)) = &cmd.node {
43+
if alter_table_cmd.subtype()
44+
== pglt_query_ext::protobuf::AlterTableType::AtAddColumn
45+
{
46+
diagnostics.push(
47+
RuleDiagnostic::new(
48+
rule_category!(),
49+
None,
50+
markup! {
51+
"Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required."
52+
},
53+
)
54+
.detail(
55+
None,
56+
"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.
57+
",
58+
),
59+
);
60+
}
61+
}
62+
}
63+
}
64+
65+
diagnostics
66+
}
67+
}

‎crates/pglt_analyser/src/options.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
//! Generated file, do not edit by hand, see `xtask/codegen`
22
33
use crate::lint;
4+
pub type AddingRequiredField =
5+
<lint::safety::adding_required_field::AddingRequiredField as pglt_analyse::Rule>::Options;
46
pub type BanDropColumn =
57
<lint::safety::ban_drop_column::BanDropColumn as pglt_analyse::Rule>::Options;
68
pub type BanDropNotNull =

‎crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
-- expect_only_lint/safety/addingRequiredField
2+
alter table test add column c int not null;

‎crates/pglt_analyser/tests/specs/safety/addingRequiredField/basic.sql.snap

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
source: crates/pglt_analyser/tests/rules_tests.rs
3+
expression: snapshot
4+
---
5+
# Input
6+
```
7+
-- expect_only_lint/safety/addingRequiredField
8+
alter table test add column c int not null;
9+
```
10+
11+
# Diagnostics
12+
lint/safety/addingRequiredField ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
13+
14+
× Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required.
15+
16+
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.

‎crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- expect_no_diagnostics
2+
alter table test
3+
add column c int not null default 0;

‎crates/pglt_analyser/tests/specs/safety/addingRequiredField/with_default.sql.snap

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
source: crates/pglt_analyser/tests/rules_tests.rs
3+
expression: snapshot
4+
---
5+
# Input
6+
```
7+
-- expect_no_diagnostics
8+
alter table test
9+
add column c int not null default 0;
10+
```

‎crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- expect_no_diagnostics
2+
alter table test
3+
add column c int;

‎crates/pglt_analyser/tests/specs/safety/addingRequiredField/without_required.sql.snap

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
source: crates/pglt_analyser/tests/rules_tests.rs
3+
expression: snapshot
4+
---
5+
# Input
6+
```
7+
-- expect_no_diagnostics
8+
alter table test
9+
add column c int;
10+
```

‎crates/pglt_configuration/src/analyser/linter/rules.rs

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -143,24 +143,38 @@ pub struct Safety {
143143
#[doc = r" It enables ALL rules for this group."]
144144
#[serde(skip_serializing_if = "Option::is_none")]
145145
pub all: Option<bool>,
146+
#[doc = "Succinct description of the rule."]
147+
#[serde(skip_serializing_if = "Option::is_none")]
148+
pub adding_required_field:
149+
Option<RuleConfiguration<pglt_analyser::options::AddingRequiredField>>,
146150
#[doc = "Dropping a column may break existing clients."]
147151
#[serde(skip_serializing_if = "Option::is_none")]
148152
pub ban_drop_column: Option<RuleConfiguration<pglt_analyser::options::BanDropColumn>>,
149153
#[doc = "Dropping a NOT NULL constraint may break existing clients."]
150154
#[serde(skip_serializing_if = "Option::is_none")]
151155
pub ban_drop_not_null: Option<RuleConfiguration<pglt_analyser::options::BanDropNotNull>>,
156+
#[doc = "Dropping a table may break existing clients."]
157+
#[serde(skip_serializing_if = "Option::is_none")]
158+
pub ban_drop_table: Option<RuleConfiguration<pglt_analyser::options::BanDropTable>>,
152159
}
153160
impl Safety {
154161
const GROUP_NAME: &'static str = "safety";
155-
pub(crate) const GROUP_RULES: &'static [&'static str] = &["banDropColumn", "banDropNotNull"];
162+
pub(crate) const GROUP_RULES: &'static [&'static str] = &[
163+
"addingRequiredField",
164+
"banDropColumn",
165+
"banDropNotNull",
166+
"banDropTable",
167+
];
156168
const RECOMMENDED_RULES: &'static [&'static str] = &["banDropColumn", "banDropNotNull"];
157169
const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
158-
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
159170
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
171+
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]),
160172
];
161173
const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
162174
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
163175
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
176+
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]),
177+
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]),
164178
];
165179
#[doc = r" Retrieves the recommended rules"]
166180
pub(crate) fn is_recommended_true(&self) -> bool {
@@ -177,30 +191,50 @@ impl Safety {
177191
}
178192
pub(crate) fn get_enabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
179193
let mut index_set = FxHashSet::default();
180-
if let Some(rule) = self.ban_drop_column.as_ref() {
194+
if let Some(rule) = self.adding_required_field.as_ref() {
181195
if rule.is_enabled() {
182196
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
183197
}
184198
}
185-
if let Some(rule) = self.ban_drop_not_null.as_ref() {
199+
if let Some(rule) = self.ban_drop_column.as_ref() {
186200
if rule.is_enabled() {
187201
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
188202
}
189203
}
204+
if let Some(rule) = self.ban_drop_not_null.as_ref() {
205+
if rule.is_enabled() {
206+
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]));
207+
}
208+
}
209+
if let Some(rule) = self.ban_drop_table.as_ref() {
210+
if rule.is_enabled() {
211+
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]));
212+
}
213+
}
190214
index_set
191215
}
192216
pub(crate) fn get_disabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
193217
let mut index_set = FxHashSet::default();
194-
if let Some(rule) = self.ban_drop_column.as_ref() {
218+
if let Some(rule) = self.adding_required_field.as_ref() {
195219
if rule.is_disabled() {
196220
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
197221
}
198222
}
199-
if let Some(rule) = self.ban_drop_not_null.as_ref() {
223+
if let Some(rule) = self.ban_drop_column.as_ref() {
200224
if rule.is_disabled() {
201225
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
202226
}
203227
}
228+
if let Some(rule) = self.ban_drop_not_null.as_ref() {
229+
if rule.is_disabled() {
230+
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]));
231+
}
232+
}
233+
if let Some(rule) = self.ban_drop_table.as_ref() {
234+
if rule.is_disabled() {
235+
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]));
236+
}
237+
}
204238
index_set
205239
}
206240
#[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"]
@@ -237,6 +271,10 @@ impl Safety {
237271
rule_name: &str,
238272
) -> Option<(RulePlainConfiguration, Option<RuleOptions>)> {
239273
match rule_name {
274+
"addingRequiredField" => self
275+
.adding_required_field
276+
.as_ref()
277+
.map(|conf| (conf.level(), conf.get_options())),
240278
"banDropColumn" => self
241279
.ban_drop_column
242280
.as_ref()
@@ -245,6 +283,10 @@ impl Safety {
245283
.ban_drop_not_null
246284
.as_ref()
247285
.map(|conf| (conf.level(), conf.get_options())),
286+
"banDropTable" => self
287+
.ban_drop_table
288+
.as_ref()
289+
.map(|conf| (conf.level(), conf.get_options())),
248290
_ => None,
249291
}
250292
}

‎crates/pglt_diagnostics_categories/src/categories.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// must be between `define_categories! {\n` and `\n ;\n`.
1414

1515
define_categories! {
16+
"lint/safety/addingRequiredField": "https://pglt.dev/linter/rules/adding-required-field",
1617
"lint/safety/banDropColumn": "https://pglt.dev/linter/rules/ban-drop-column",
1718
"lint/safety/banDropNotNull": "https://pglt.dev/linter/rules/ban-drop-not-null",
1819
"lint/safety/banDropTable": "https://pglt.dev/linter/rules/ban-drop-table",

0 commit comments

Comments
 (0)