Skip to content

Commit b2fd75a

Browse files
juleswritescodejuleswritescode
committed
rebased
1 parent 1a11556 commit b2fd75a

File tree

12 files changed

+172
-8
lines changed

12 files changed

+172
-8
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/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: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -143,29 +143,50 @@ 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+
<<<<<<< HEAD
152157
#[doc = "Succinct description of the rule."]
158+
=======
159+
#[doc = "Dropping a table may break existing clients."]
160+
>>>>>>> cbc49c8 (feat(rules): adding-required-field)
153161
#[serde(skip_serializing_if = "Option::is_none")]
154162
pub ban_drop_table: Option<RuleConfiguration<pglt_analyser::options::BanDropTable>>,
155163
}
156164
impl Safety {
157165
const GROUP_NAME: &'static str = "safety";
166+
<<<<<<< HEAD
158167
pub(crate) const GROUP_RULES: &'static [&'static str] =
159168
&["banDropColumn", "banDropNotNull", "banDropTable"];
169+
=======
170+
pub(crate) const GROUP_RULES: &'static [&'static str] = &[
171+
"addingRequiredField",
172+
"banDropColumn",
173+
"banDropNotNull",
174+
"banDropTable",
175+
];
176+
>>>>>>> cbc49c8 (feat(rules): adding-required-field)
160177
const RECOMMENDED_RULES: &'static [&'static str] = &["banDropColumn", "banDropNotNull"];
161178
const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
162-
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
163179
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
180+
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]),
164181
];
165182
const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
166183
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
167184
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
168185
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]),
186+
<<<<<<< HEAD
187+
=======
188+
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]),
189+
>>>>>>> cbc49c8 (feat(rules): adding-required-field)
169190
];
170191
#[doc = r" Retrieves the recommended rules"]
171192
pub(crate) fn is_recommended_true(&self) -> bool {
@@ -182,40 +203,64 @@ impl Safety {
182203
}
183204
pub(crate) fn get_enabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
184205
let mut index_set = FxHashSet::default();
185-
if let Some(rule) = self.ban_drop_column.as_ref() {
206+
if let Some(rule) = self.adding_required_field.as_ref() {
186207
if rule.is_enabled() {
187208
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
188209
}
189210
}
190-
if let Some(rule) = self.ban_drop_not_null.as_ref() {
211+
if let Some(rule) = self.ban_drop_column.as_ref() {
191212
if rule.is_enabled() {
192213
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
193214
}
194215
}
216+
<<<<<<< HEAD
195217
if let Some(rule) = self.ban_drop_table.as_ref() {
218+
=======
219+
if let Some(rule) = self.ban_drop_not_null.as_ref() {
220+
>>>>>>> cbc49c8 (feat(rules): adding-required-field)
196221
if rule.is_enabled() {
197222
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]));
198223
}
199224
}
225+
<<<<<<< HEAD
226+
=======
227+
if let Some(rule) = self.ban_drop_table.as_ref() {
228+
if rule.is_enabled() {
229+
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]));
230+
}
231+
}
232+
>>>>>>> cbc49c8 (feat(rules): adding-required-field)
200233
index_set
201234
}
202235
pub(crate) fn get_disabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
203236
let mut index_set = FxHashSet::default();
204-
if let Some(rule) = self.ban_drop_column.as_ref() {
237+
if let Some(rule) = self.adding_required_field.as_ref() {
205238
if rule.is_disabled() {
206239
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
207240
}
208241
}
209-
if let Some(rule) = self.ban_drop_not_null.as_ref() {
242+
if let Some(rule) = self.ban_drop_column.as_ref() {
210243
if rule.is_disabled() {
211244
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
212245
}
213246
}
247+
<<<<<<< HEAD
214248
if let Some(rule) = self.ban_drop_table.as_ref() {
249+
=======
250+
if let Some(rule) = self.ban_drop_not_null.as_ref() {
251+
>>>>>>> cbc49c8 (feat(rules): adding-required-field)
215252
if rule.is_disabled() {
216253
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]));
217254
}
218255
}
256+
<<<<<<< HEAD
257+
=======
258+
if let Some(rule) = self.ban_drop_table.as_ref() {
259+
if rule.is_disabled() {
260+
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]));
261+
}
262+
}
263+
>>>>>>> cbc49c8 (feat(rules): adding-required-field)
219264
index_set
220265
}
221266
#[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"]
@@ -252,6 +297,10 @@ impl Safety {
252297
rule_name: &str,
253298
) -> Option<(RulePlainConfiguration, Option<RuleOptions>)> {
254299
match rule_name {
300+
"addingRequiredField" => self
301+
.adding_required_field
302+
.as_ref()
303+
.map(|conf| (conf.level(), conf.get_options())),
255304
"banDropColumn" => self
256305
.ban_drop_column
257306
.as_ref()

‎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)