diff --git a/crates/pg_analyser/CONTRIBUTING.md b/crates/pg_analyser/CONTRIBUTING.md index 200be87c..05975adf 100644 --- a/crates/pg_analyser/CONTRIBUTING.md +++ b/crates/pg_analyser/CONTRIBUTING.md @@ -17,12 +17,14 @@ We follow a naming convention according to what the rule does: 1. Forbid a concept ```block - no + ban ``` - When a rule's sole intention is to **forbid a single concept** the rule should be named using the `no` prefix. + When a rule's sole intention is to **forbid a single concept** the rule should be named using the `ban` prefix. -1. Mandate a concept + Example: "banDropColumn" + +2. Mandate a concept ```block use @@ -35,9 +37,10 @@ We follow a naming convention according to what the rule does: A rule should be informative to the user, and give as much explanation as possible. When writing a rule, you must adhere to the following **pillars**: + 1. Explain to the user the error. Generally, this is the message of the diagnostic. -1. Explain to the user **why** the error is triggered. Generally, this is implemented with an additional node. -1. Tell the user what they should do. Generally, this is implemented using a code action. If a code action is not applicable a note should tell the user what they should do to fix the error. +2. Explain to the user **why** the error is triggered. Generally, this is implemented with an additional node. +3. Tell the user what they should do. Generally, this is implemented using a code action. If a code action is not applicable a note should tell the user what they should do to fix the error. ### Create and implement the rule @@ -53,10 +56,11 @@ Let's say we want to create a new **lint** rule called `useMyRuleName`, follow t ```shell just new-lintrule safety useMyRuleName ``` + The script will generate a bunch of files inside the `pg_analyser` crate. Among the other files, you'll find a file called `use_my_new_rule_name.rs` inside the `pg_analyser/lib/src/lint/safety` folder. You'll implement your rule in this file. -1. The `Option` type doesn't have to be used, so it can be considered optional. However, it has to be defined as `type Option = ()`. +1. The `Options` type doesn't have to be used, so it can be considered optional. However, it has to be defined as `type Options = ()`. 1. Implement the `run` function: The function is called for every statement, and should return zero or more diagnostics. Follow the [pillars](#explain-a-rule-to-the-user) when writing the message of a diagnostic Don't forget to format your code with `just f` and lint with `just l`. @@ -107,7 +111,7 @@ pub enum Behavior { ``` Note that we use a boxed slice `Box<[Box]>` instead of `Vec`. -This allows saving memory: [boxed slices and boxed str use 2 words instead of three words](https://nnethercote.github.io/perf-book/type-sizes.html#boxed-slices). +This allows saving memory: [boxed slices and boxed str use two instead of three words](https://nnethercote.github.io/perf-book/type-sizes.html#boxed-slices). With these types in place, you can set the associated type `Options` of the rule: @@ -127,6 +131,7 @@ The compiler should warn you that `MyRuleOptions` does not implement some requir We currently require implementing _serde_'s traits `Deserialize`/`Serialize`. Also, we use other `serde` macros to adjust the JSON configuration: + - `rename_all = "snake_case"`: it renames all fields in camel-case, so they are in line with the naming style of the `pglsp.toml`. - `deny_unknown_fields`: it raises an error if the configuration contains extraneous fields. - `default`: it uses the `Default` value when the field is missing from `pglsp.toml`. This macro makes the field optional. @@ -159,7 +164,6 @@ pub enum Behavior { Below, there are many tips and guidelines on how to create a lint rule using our infrastructure. - #### `declare_lint_rule` This macro is used to declare an analyzer rule type, and implement the [RuleMeta] trait for it. @@ -235,6 +239,7 @@ impl Rule for BanDropColumn { ### Document the rule The documentation needs to adhere to the following rules: + - The **first** paragraph of the documentation is used as brief description of the rule, and it **must** be written in one single line. Breaking the paragraph in multiple lines will break the table content of the rules page. - The next paragraphs can be used to further document the rule with as many details as you see fit. - The documentation must have a `## Examples` header, followed by two headers: `### Invalid` and `### Valid`. `### Invalid` must go first because we need to show when the rule is triggered. @@ -246,7 +251,7 @@ The documentation needs to adhere to the following rules: Here's an example of how the documentation could look like: -```rust +````rust declare_lint_rule! { /// Dropping a column may break existing clients. /// @@ -269,7 +274,7 @@ declare_lint_rule! { sources: &[RuleSource::Squawk("ban-drop-column")], } } -``` +```` This will cause the documentation generator to ensure the rule does emit exactly one diagnostic for this code, and to include a snapshot for the @@ -294,7 +299,6 @@ Stage and commit your changes: > git commit -m 'feat(pg_analyser): myRuleName' ``` - ### Deprecate a rule There are occasions when a rule must be deprecated, to avoid breaking changes. The reason @@ -302,7 +306,7 @@ of deprecation can be multiple. In order to do, the macro allows adding additional field to add the reason for deprecation -```rust +````rust use pg_analyse::declare_lint_rule; declare_lint_rule! { @@ -328,5 +332,4 @@ declare_lint_rule! { sources: &[RuleSource::Squawk("ban-drop-column")], } } -``` - +```` diff --git a/crates/pg_analyser/src/lint/safety.rs b/crates/pg_analyser/src/lint/safety.rs index 4d78797b..cb767ad2 100644 --- a/crates/pg_analyser/src/lint/safety.rs +++ b/crates/pg_analyser/src/lint/safety.rs @@ -2,4 +2,5 @@ use pg_analyse::declare_lint_group; pub mod ban_drop_column; -declare_lint_group! { pub Safety { name : "safety" , rules : [self :: ban_drop_column :: BanDropColumn ,] } } +pub mod ban_drop_not_null; +declare_lint_group! { pub Safety { name : "safety" , rules : [self :: ban_drop_column :: BanDropColumn , self :: ban_drop_not_null :: BanDropNotNull ,] } } diff --git a/crates/pg_analyser/src/lint/safety/ban_drop_column.rs b/crates/pg_analyser/src/lint/safety/ban_drop_column.rs index 9f20227d..d18ac19f 100644 --- a/crates/pg_analyser/src/lint/safety/ban_drop_column.rs +++ b/crates/pg_analyser/src/lint/safety/ban_drop_column.rs @@ -1,13 +1,6 @@ use pg_analyse::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource}; use pg_console::markup; -#[derive(Clone, Debug, Default, Eq, PartialEq, serde::Deserialize, serde::Serialize)] -// #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] -#[serde(rename_all = "camelCase", deny_unknown_fields, default)] -pub struct Options { - test: String, -} - declare_lint_rule! { /// Dropping a column may break existing clients. /// @@ -19,7 +12,7 @@ declare_lint_rule! { /// /// ### Invalid /// - /// ```sql,expect_diagnostic + /// ```sql,ignore /// alter table test drop column id; /// ``` /// @@ -32,7 +25,7 @@ declare_lint_rule! { } impl Rule for BanDropColumn { - type Options = Options; + type Options = (); fn run(ctx: &RuleContext) -> Vec { let mut diagnostics = Vec::new(); @@ -47,7 +40,7 @@ impl Rule for BanDropColumn { markup! { "Dropping a column may break existing clients." }, - ).detail(None, format!("[{}] You can leave the column as nullable or delete the column once queries no longer select or modify the column.", ctx.options().test))); + ).detail(None, "You can leave the column as nullable or delete the column once queries no longer select or modify the column.")); } } } diff --git a/crates/pg_analyser/src/lint/safety/ban_drop_not_null.rs b/crates/pg_analyser/src/lint/safety/ban_drop_not_null.rs new file mode 100644 index 00000000..92c1a372 --- /dev/null +++ b/crates/pg_analyser/src/lint/safety/ban_drop_not_null.rs @@ -0,0 +1,51 @@ +use pg_analyse::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource}; +use pg_console::markup; + +declare_lint_rule! { + /// Dropping a NOT NULL constraint may break existing clients. + /// + /// Application code or code written in procedural languages like PL/SQL or PL/pgSQL may not expect NULL values for the column that was previously guaranteed to be NOT NULL and therefore may fail to process them correctly. + /// + /// You can consider using a marker value that represents NULL. Alternatively, create a new table allowing NULL values, copy the data from the old table, and create a view that filters NULL values. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```sql,ignore + /// alter table users alter column email drop not null; + /// ``` + pub BanDropNotNull { + version: "next", + name: "banDropNotNull", + recommended: true, + sources: &[RuleSource::Squawk("ban-drop-not-null")], + + } +} + +impl Rule for BanDropNotNull { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + if let pg_query_ext::NodeEnum::AlterTableStmt(stmt) = &ctx.stmt() { + for cmd in &stmt.cmds { + if let Some(pg_query_ext::NodeEnum::AlterTableCmd(cmd)) = &cmd.node { + if cmd.subtype() == pg_query_ext::protobuf::AlterTableType::AtDropNotNull { + diagnostics.push(RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Dropping a NOT NULL constraint may break existing clients." + }, + ).detail(None, "Consider using a marker value that represents NULL. Alternatively, create a new table allowing NULL values, copy the data from the old table, and create a view that filters NULL values.")); + } + } + } + } + + diagnostics + } +} diff --git a/crates/pg_analyser/src/options.rs b/crates/pg_analyser/src/options.rs index 13e54068..3bce4494 100644 --- a/crates/pg_analyser/src/options.rs +++ b/crates/pg_analyser/src/options.rs @@ -3,3 +3,5 @@ use crate::lint; pub type BanDropColumn = ::Options; +pub type BanDropNotNull = + ::Options; diff --git a/crates/pg_configuration/src/analyser/linter/rules.rs b/crates/pg_configuration/src/analyser/linter/rules.rs index 5b5deb06..a74843af 100644 --- a/crates/pg_configuration/src/analyser/linter/rules.rs +++ b/crates/pg_configuration/src/analyser/linter/rules.rs @@ -146,15 +146,22 @@ pub struct Safety { #[doc = "Dropping a column may break existing clients."] #[serde(skip_serializing_if = "Option::is_none")] pub ban_drop_column: Option>, + #[doc = "Dropping a NOT NULL constraint may break existing clients."] + #[serde(skip_serializing_if = "Option::is_none")] + pub ban_drop_not_null: Option>, } impl Safety { const GROUP_NAME: &'static str = "safety"; - pub(crate) const GROUP_RULES: &'static [&'static str] = &["banDropColumn"]; - const RECOMMENDED_RULES: &'static [&'static str] = &["banDropColumn"]; - const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = - &[RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])]; - const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = - &[RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])]; + pub(crate) const GROUP_RULES: &'static [&'static str] = &["banDropColumn", "banDropNotNull"]; + const RECOMMENDED_RULES: &'static [&'static str] = &["banDropColumn", "banDropNotNull"]; + 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]), + ]; + 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]), + ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { matches!(self.recommended, Some(true)) @@ -175,6 +182,11 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])); } } + 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[1])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> FxHashSet> { @@ -184,6 +196,11 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])); } } + 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[1])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -224,6 +241,10 @@ impl Safety { .ban_drop_column .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "banDropNotNull" => self + .ban_drop_not_null + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), _ => None, } } diff --git a/crates/pg_diagnostics_categories/src/categories.rs b/crates/pg_diagnostics_categories/src/categories.rs index 40795789..bc791272 100644 --- a/crates/pg_diagnostics_categories/src/categories.rs +++ b/crates/pg_diagnostics_categories/src/categories.rs @@ -14,6 +14,7 @@ define_categories! { "lint/safety/banDropColumn": "https://pglsp.dev/linter/rules/ban-drop-column", + "lint/safety/banDropNotNull": "https://pglsp.dev/linter/rules/ban-drop-not-null", // end lint rules ; // General categories diff --git a/pglsp.toml b/pglsp.toml index ea394361..eab1d853 100644 --- a/pglsp.toml +++ b/pglsp.toml @@ -25,6 +25,6 @@ recommended = true [linter.rules.safety.banDropColumn] level = "warn" -options = { test = "HELLO" } - +[linter.rules.safety.banDropNotNull] +level = "warn" \ No newline at end of file