diff --git a/Cargo.lock b/Cargo.lock index 875d4d9f..41f807d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5128,6 +5128,7 @@ dependencies = [ "bpaf", "pgt_analyse", "pgt_analyser", + "pgt_diagnostics", "pgt_workspace", "proc-macro2", "pulldown-cmark", diff --git a/crates/pgt_analyse/src/macros.rs b/crates/pgt_analyse/src/macros.rs index d9f70ed3..aa7b25c5 100644 --- a/crates/pgt_analyse/src/macros.rs +++ b/crates/pgt_analyse/src/macros.rs @@ -24,6 +24,7 @@ macro_rules! declare_lint_rule { ( $( #[doc = $doc:literal] )+ $vis:vis $id:ident { version: $version:literal, name: $name:tt, + severity: $severity:expr_2021, $( $key:ident: $value:expr_2021, )* } ) => { @@ -32,6 +33,7 @@ macro_rules! declare_lint_rule { $vis $id { version: $version, name: $name, + severity: $severity, $( $key: $value, )* } ); @@ -53,6 +55,7 @@ macro_rules! declare_rule { ( $( #[doc = $doc:literal] )+ $vis:vis $id:ident { version: $version:literal, name: $name:tt, + severity: $severity:expr_2021, $( $key:ident: $value:expr_2021, )* } ) => { $( #[doc = $doc] )* @@ -61,7 +64,7 @@ macro_rules! declare_rule { impl $crate::RuleMeta for $id { type Group = super::Group; const METADATA: $crate::RuleMetadata = - $crate::RuleMetadata::new($version, $name, concat!( $( $doc, "\n", )* )) $( .$key($value) )*; + $crate::RuleMetadata::new($version, $name, concat!( $( $doc, "\n", )* ), $severity) $( .$key($value) )*; } } } diff --git a/crates/pgt_analyse/src/rule.rs b/crates/pgt_analyse/src/rule.rs index f135705e..fae3cda3 100644 --- a/crates/pgt_analyse/src/rule.rs +++ b/crates/pgt_analyse/src/rule.rs @@ -3,7 +3,7 @@ use pgt_console::{MarkupBuf, markup}; use pgt_diagnostics::advice::CodeSuggestionAdvice; use pgt_diagnostics::{ Advices, Category, Diagnostic, DiagnosticTags, Location, LogCategory, MessageAndDescription, - Visit, + Severity, Visit, }; use pgt_text_size::TextRange; use std::cmp::Ordering; @@ -31,10 +31,17 @@ pub struct RuleMetadata { pub recommended: bool, /// The source URL of the rule pub sources: &'static [RuleSource], + /// The default severity of the rule + pub severity: Severity, } impl RuleMetadata { - pub const fn new(version: &'static str, name: &'static str, docs: &'static str) -> Self { + pub const fn new( + version: &'static str, + name: &'static str, + docs: &'static str, + severity: Severity, + ) -> Self { Self { deprecated: None, version, @@ -42,6 +49,7 @@ impl RuleMetadata { docs, sources: &[], recommended: false, + severity, } } diff --git a/crates/pgt_analyser/CONTRIBUTING.md b/crates/pgt_analyser/CONTRIBUTING.md index 50327d5e..b0929eda 100644 --- a/crates/pgt_analyser/CONTRIBUTING.md +++ b/crates/pgt_analyser/CONTRIBUTING.md @@ -54,9 +54,11 @@ Let's say we want to create a new **lint** rule called `useMyRuleName`, follow t 1. Run the command ```shell - just new-lintrule safety useMyRuleName + just new-lintrule safety useMyRuleName () ``` + Where severity is optional but can be "info", "warn", or "error" (default). + The script will generate a bunch of files inside the `pgt_analyser` crate. Among the other files, you'll find a file called `use_my_new_rule_name.rs` inside the `pgt_analyser/lib/src/lint/safety` folder. You'll implement your rule in this file. @@ -187,6 +189,7 @@ declare_lint_rule! { pub(crate) ExampleRule { version: "next", name: "myRuleName", + severity: Severity::Error, recommended: false, } } @@ -206,6 +209,7 @@ declare_lint_rule! { pub(crate) ExampleRule { version: "next", name: "myRuleName", + severity: Severity::Error, recommended: false, sources: &[RuleSource::Squawk("ban-drop-column")], } @@ -228,6 +232,7 @@ declare_lint_rule! { pub(crate) ExampleRule { version: "next", name: "myRuleName", + severity: Severity::Error, recommended: false, } } @@ -280,6 +285,7 @@ declare_lint_rule! { version: "next", name: "banDropColumn", recommended: true, + severity: Severity::Error, sources: &[RuleSource::Squawk("ban-drop-column")], } } @@ -351,6 +357,7 @@ declare_lint_rule! { version: "next", name: "banDropColumn", recommended: true, + severity: Severity::Error, deprecated: true, sources: &[RuleSource::Squawk("ban-drop-column")], } diff --git a/crates/pgt_analyser/Cargo.toml b/crates/pgt_analyser/Cargo.toml index bd51c36a..e77aaa4f 100644 --- a/crates/pgt_analyser/Cargo.toml +++ b/crates/pgt_analyser/Cargo.toml @@ -12,10 +12,11 @@ repository.workspace = true version = "0.0.0" [dependencies] -pgt_analyse = { workspace = true } -pgt_console = { workspace = true } -pgt_query_ext = { workspace = true } -serde = { workspace = true } +pgt_analyse = { workspace = true } +pgt_console = { workspace = true } +pgt_diagnostics = { workspace = true } +pgt_query_ext = { workspace = true } +serde = { workspace = true } [dev-dependencies] insta = { version = "1.42.1" } diff --git a/crates/pgt_analyser/src/lint/safety.rs b/crates/pgt_analyser/src/lint/safety.rs index 920326c2..a2b72fce 100644 --- a/crates/pgt_analyser/src/lint/safety.rs +++ b/crates/pgt_analyser/src/lint/safety.rs @@ -3,6 +3,8 @@ use pgt_analyse::declare_lint_group; pub mod adding_required_field; pub mod ban_drop_column; +pub mod ban_drop_database; pub mod ban_drop_not_null; pub mod ban_drop_table; -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 ,] } } +pub mod ban_truncate_cascade; +declare_lint_group! { pub Safety { name : "safety" , rules : [self :: adding_required_field :: AddingRequiredField , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade ,] } } diff --git a/crates/pgt_analyser/src/lint/safety/adding_required_field.rs b/crates/pgt_analyser/src/lint/safety/adding_required_field.rs index d4f72a7f..06901952 100644 --- a/crates/pgt_analyser/src/lint/safety/adding_required_field.rs +++ b/crates/pgt_analyser/src/lint/safety/adding_required_field.rs @@ -1,5 +1,6 @@ use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; use pgt_console::markup; +use pgt_diagnostics::Severity; declare_lint_rule! { /// Adding a new column that is NOT NULL and has no default value to an existing table effectively makes it required. @@ -17,6 +18,7 @@ declare_lint_rule! { pub AddingRequiredField { version: "next", name: "addingRequiredField", + severity: Severity::Error, recommended: false, sources: &[RuleSource::Squawk("adding-required-field")], } diff --git a/crates/pgt_analyser/src/lint/safety/ban_drop_column.rs b/crates/pgt_analyser/src/lint/safety/ban_drop_column.rs index aab5d515..165d4230 100644 --- a/crates/pgt_analyser/src/lint/safety/ban_drop_column.rs +++ b/crates/pgt_analyser/src/lint/safety/ban_drop_column.rs @@ -1,5 +1,6 @@ use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; use pgt_console::markup; +use pgt_diagnostics::Severity; declare_lint_rule! { /// Dropping a column may break existing clients. @@ -19,6 +20,7 @@ declare_lint_rule! { pub BanDropColumn { version: "next", name: "banDropColumn", + severity: Severity::Warning, recommended: true, sources: &[RuleSource::Squawk("ban-drop-column")], } diff --git a/crates/pgt_analyser/src/lint/safety/ban_drop_database.rs b/crates/pgt_analyser/src/lint/safety/ban_drop_database.rs new file mode 100644 index 00000000..11d07da9 --- /dev/null +++ b/crates/pgt_analyser/src/lint/safety/ban_drop_database.rs @@ -0,0 +1,39 @@ +use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; +use pgt_console::markup; +use pgt_diagnostics::Severity; + +declare_lint_rule! { + /// Dropping a database may break existing clients (and everything else, really). + /// + /// Make sure that you really want to drop it. + pub BanDropDatabase { + version: "next", + name: "banDropDatabase", + severity: Severity::Warning, + recommended: false, + sources: &[RuleSource::Squawk("ban-drop-database")], + } +} + +impl Rule for BanDropDatabase { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = vec![]; + + if let pgt_query_ext::NodeEnum::DropdbStmt(_) = &ctx.stmt() { + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Dropping a database may break existing clients." + }, + ) + .detail(None, "You probably don't want to drop your database."), + ); + } + + diagnostics + } +} diff --git a/crates/pgt_analyser/src/lint/safety/ban_drop_not_null.rs b/crates/pgt_analyser/src/lint/safety/ban_drop_not_null.rs index eb17f694..fa4c9011 100644 --- a/crates/pgt_analyser/src/lint/safety/ban_drop_not_null.rs +++ b/crates/pgt_analyser/src/lint/safety/ban_drop_not_null.rs @@ -1,5 +1,6 @@ use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; use pgt_console::markup; +use pgt_diagnostics::Severity; declare_lint_rule! { /// Dropping a NOT NULL constraint may break existing clients. @@ -18,6 +19,7 @@ declare_lint_rule! { pub BanDropNotNull { version: "next", name: "banDropNotNull", + severity: Severity::Warning, recommended: true, sources: &[RuleSource::Squawk("ban-drop-not-null")], diff --git a/crates/pgt_analyser/src/lint/safety/ban_drop_table.rs b/crates/pgt_analyser/src/lint/safety/ban_drop_table.rs index 4ce00a60..90c08514 100644 --- a/crates/pgt_analyser/src/lint/safety/ban_drop_table.rs +++ b/crates/pgt_analyser/src/lint/safety/ban_drop_table.rs @@ -1,5 +1,6 @@ use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; use pgt_console::markup; +use pgt_diagnostics::Severity; declare_lint_rule! { /// Dropping a table may break existing clients. @@ -18,6 +19,7 @@ declare_lint_rule! { pub BanDropTable { version: "next", name: "banDropTable", + severity: Severity::Warning, recommended: true, sources: &[RuleSource::Squawk("ban-drop-table")], } diff --git a/crates/pgt_analyser/src/lint/safety/ban_truncate_cascade.rs b/crates/pgt_analyser/src/lint/safety/ban_truncate_cascade.rs new file mode 100644 index 00000000..cef5cd47 --- /dev/null +++ b/crates/pgt_analyser/src/lint/safety/ban_truncate_cascade.rs @@ -0,0 +1,51 @@ +use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; +use pgt_console::markup; +use pgt_diagnostics::Severity; +use pgt_query_ext::protobuf::DropBehavior; + +declare_lint_rule! { + /// Using `TRUNCATE`'s `CASCADE` option will truncate any tables that are also foreign-keyed to the specified tables. + /// + /// So if you had tables with foreign-keys like: + /// + /// `a <- b <- c` + /// + /// and ran: + /// + /// `truncate a cascade;` + /// + /// You'd end up with a, b, & c all being truncated! + /// + /// Instead, you can manually specify the tables you want. + /// + /// `truncate a, b;` + pub BanTruncateCascade { + version: "next", + name: "banTruncateCascade", + severity: Severity::Error, + recommended: false, + sources: &[RuleSource::Squawk("ban-truncate-cascade")], + } +} + +impl Rule for BanTruncateCascade { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + if let pgt_query_ext::NodeEnum::TruncateStmt(stmt) = &ctx.stmt() { + if stmt.behavior() == DropBehavior::DropCascade { + diagnostics.push(RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "The `CASCADE` option will also truncate any tables that are foreign-keyed to the specified tables." + }, + ).detail(None, "Do not use the `CASCADE` option. Instead, specify manually what you want: `TRUNCATE a, b;`.")); + } + } + + diagnostics + } +} diff --git a/crates/pgt_analyser/src/options.rs b/crates/pgt_analyser/src/options.rs index d78020f8..d893b84f 100644 --- a/crates/pgt_analyser/src/options.rs +++ b/crates/pgt_analyser/src/options.rs @@ -5,6 +5,10 @@ pub type AddingRequiredField = ::Options; pub type BanDropColumn = ::Options; +pub type BanDropDatabase = + ::Options; pub type BanDropNotNull = ::Options; pub type BanDropTable = ::Options; +pub type BanTruncateCascade = + ::Options; diff --git a/crates/pgt_analyser/tests/specs/safety/banDropDatabase/basic.sql b/crates/pgt_analyser/tests/specs/safety/banDropDatabase/basic.sql new file mode 100644 index 00000000..0dc01652 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/banDropDatabase/basic.sql @@ -0,0 +1,2 @@ +-- expect_only_lint/safety/banDropDatabase +drop database all_users; \ No newline at end of file diff --git a/crates/pgt_analyser/tests/specs/safety/banDropDatabase/basic.sql.snap b/crates/pgt_analyser/tests/specs/safety/banDropDatabase/basic.sql.snap new file mode 100644 index 00000000..90e35820 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/banDropDatabase/basic.sql.snap @@ -0,0 +1,16 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +--- +# Input +``` +-- expect_only_lint/safety/banDropDatabase +drop database all_users; +``` + +# Diagnostics +lint/safety/banDropDatabase ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Dropping a database may break existing clients. + + i You probably don't want to drop your database. diff --git a/crates/pgt_analyser/tests/specs/safety/banTruncateCascade/basic.sql b/crates/pgt_analyser/tests/specs/safety/banTruncateCascade/basic.sql new file mode 100644 index 00000000..d17fed13 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/banTruncateCascade/basic.sql @@ -0,0 +1,2 @@ +-- expect_only_lint/safety/banTruncateCascade +truncate a cascade; \ No newline at end of file diff --git a/crates/pgt_analyser/tests/specs/safety/banTruncateCascade/basic.sql.snap b/crates/pgt_analyser/tests/specs/safety/banTruncateCascade/basic.sql.snap new file mode 100644 index 00000000..d214b978 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/banTruncateCascade/basic.sql.snap @@ -0,0 +1,16 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +--- +# Input +``` +-- expect_only_lint/safety/banTruncateCascade +truncate a cascade; +``` + +# Diagnostics +lint/safety/banTruncateCascade ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × The `CASCADE` option will also truncate any tables that are foreign-keyed to the specified tables. + + i Do not use the `CASCADE` option. Instead, specify manually what you want: `TRUNCATE a, b;`. diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index 14d796bf..8db5a0ab 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -65,10 +65,9 @@ impl Rules { } #[doc = r" Given a category coming from [Diagnostic](pgt_diagnostics::Diagnostic), this function returns"] #[doc = r" the [Severity](pgt_diagnostics::Severity) associated to the rule, if the configuration changed it."] - #[doc = r" If the severity is off or not set, then the function returns the default severity of the rule:"] - #[doc = r" [Severity::Error] for recommended rules and [Severity::Warning] for other rules."] - #[doc = r""] - #[doc = r" If not, the function returns [None]."] + #[doc = r" If the severity is off or not set, then the function returns the default severity of the rule,"] + #[doc = r" which is configured at the rule definition."] + #[doc = r" The function can return `None` if the rule is not properly configured."] pub fn get_severity_from_code(&self, category: &Category) -> Option { let mut split_code = category.name().split('/'); let _lint = split_code.next(); @@ -82,16 +81,7 @@ impl Rules { .as_ref() .and_then(|group| group.get_rule_configuration(rule_name)) .filter(|(level, _)| !matches!(level, RulePlainConfiguration::Off)) - .map_or_else( - || { - if Safety::is_recommended_rule(rule_name) { - Severity::Error - } else { - Severity::Warning - } - }, - |(level, _)| level.into(), - ), + .map_or_else(|| Safety::severity(rule_name), |(level, _)| level.into()), }; Some(severity) } @@ -150,33 +140,41 @@ 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 database may break existing clients (and everything else, really)."] + #[serde(skip_serializing_if = "Option::is_none")] + pub ban_drop_database: Option>, #[doc = "Dropping a NOT NULL constraint may break existing clients."] #[serde(skip_serializing_if = "Option::is_none")] pub ban_drop_not_null: Option>, #[doc = "Dropping a table may break existing clients."] #[serde(skip_serializing_if = "Option::is_none")] pub ban_drop_table: Option>, + #[doc = "Using TRUNCATE's CASCADE option will truncate any tables that are also foreign-keyed to the specified tables."] + #[serde(skip_serializing_if = "Option::is_none")] + pub ban_truncate_cascade: Option>, } impl Safety { const GROUP_NAME: &'static str = "safety"; pub(crate) const GROUP_RULES: &'static [&'static str] = &[ "addingRequiredField", "banDropColumn", + "banDropDatabase", "banDropNotNull", "banDropTable", + "banTruncateCascade", ]; - 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[1]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[4]), ]; 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]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[4]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[5]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { @@ -203,16 +201,26 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1])); } } - if let Some(rule) = self.ban_drop_not_null.as_ref() { + if let Some(rule) = self.ban_drop_database.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 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[3])); } } + 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[4])); + } + } + if let Some(rule) = self.ban_truncate_cascade.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[5])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> FxHashSet> { @@ -227,26 +235,32 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1])); } } - if let Some(rule) = self.ban_drop_not_null.as_ref() { + if let Some(rule) = self.ban_drop_database.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 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[3])); } } + 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[4])); + } + } + if let Some(rule) = self.ban_truncate_cascade.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[5])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] pub(crate) fn has_rule(rule_name: &str) -> Option<&'static str> { Some(Self::GROUP_RULES[Self::GROUP_RULES.binary_search(&rule_name).ok()?]) } - #[doc = r" Checks if, given a rule name, it is marked as recommended"] - pub(crate) fn is_recommended_rule(rule_name: &str) -> bool { - Self::RECOMMENDED_RULES.contains(&rule_name) - } pub(crate) fn recommended_rules_as_filters() -> &'static [RuleFilter<'static>] { Self::RECOMMENDED_RULES_AS_FILTERS } @@ -268,6 +282,17 @@ impl Safety { enabled_rules.extend(Self::recommended_rules_as_filters()); } } + pub(crate) fn severity(rule_name: &str) -> Severity { + match rule_name { + "addingRequiredField" => Severity::Error, + "banDropColumn" => Severity::Warning, + "banDropDatabase" => Severity::Warning, + "banDropNotNull" => Severity::Warning, + "banDropTable" => Severity::Warning, + "banTruncateCascade" => Severity::Error, + _ => unreachable!(), + } + } pub(crate) fn get_rule_configuration( &self, rule_name: &str, @@ -281,6 +306,10 @@ impl Safety { .ban_drop_column .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "banDropDatabase" => self + .ban_drop_database + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), "banDropNotNull" => self .ban_drop_not_null .as_ref() @@ -289,6 +318,10 @@ impl Safety { .ban_drop_table .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "banTruncateCascade" => self + .ban_truncate_cascade + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), _ => None, } } diff --git a/crates/pgt_diagnostics/src/diagnostic.rs b/crates/pgt_diagnostics/src/diagnostic.rs index 9a62ede1..3f365aed 100644 --- a/crates/pgt_diagnostics/src/diagnostic.rs +++ b/crates/pgt_diagnostics/src/diagnostic.rs @@ -6,6 +6,7 @@ use std::{ str::FromStr, }; +use bpaf::Bpaf; use enumflags2::{BitFlags, bitflags, make_bitflags}; use serde::{Deserialize, Serialize}; @@ -115,7 +116,7 @@ pub trait Diagnostic: Debug { } #[derive( - Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize, Default, + Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize, Default, Bpaf, )] #[serde(rename_all = "camelCase")] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] diff --git a/crates/pgt_diagnostics_categories/src/categories.rs b/crates/pgt_diagnostics_categories/src/categories.rs index 8a91cfb5..d5ce48d7 100644 --- a/crates/pgt_diagnostics_categories/src/categories.rs +++ b/crates/pgt_diagnostics_categories/src/categories.rs @@ -15,8 +15,10 @@ 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/banDropDatabase": "https://pgtools.dev/linter/rules/ban-drop-database", "lint/safety/banDropNotNull": "https://pglt.dev/linter/rules/ban-drop-not-null", "lint/safety/banDropTable": "https://pglt.dev/linter/rules/ban-drop-table", + "lint/safety/banTruncateCascade": "https://pgtools.dev/linter/rules/ban-truncate-cascade", // end lint rules ; // General categories diff --git a/crates/pgt_workspace/src/settings.rs b/crates/pgt_workspace/src/settings.rs index ac55d8a1..40db2c1e 100644 --- a/crates/pgt_workspace/src/settings.rs +++ b/crates/pgt_workspace/src/settings.rs @@ -275,12 +275,10 @@ impl Settings { &self, code: &Category, ) -> Option { - let rules = self.linter.rules.as_ref(); - if let Some(rules) = rules { - rules.get_severity_from_code(code) - } else { - None - } + self.linter + .rules + .as_ref() + .and_then(|r| r.get_severity_from_code(code)) } } diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index 67a3713c..d0c8d13a 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -427,34 +427,20 @@ impl Workspace for WorkspaceServer { } }; - // create analyser for this run - // first, collect enabled and disabled rules from the workspace settings - let (enabled_rules, disabled_rules) = AnalyserVisitorBuilder::new(settings) - .with_linter_rules(¶ms.only, ¶ms.skip) - .finish(); - // then, build a map that contains all options - let options = AnalyserOptions { - rules: to_analyser_rules(settings), - }; - // next, build the analysis filter which will be used to match rules - let filter = AnalysisFilter { - categories: params.categories, - enabled_rules: Some(enabled_rules.as_slice()), - disabled_rules: &disabled_rules, - }; - // finally, create the analyser that will be used during this run - let analyser = Analyser::new(AnalyserConfig { - options: &options, - filter, - }); - let parser = self .parsed_documents .get(¶ms.path) .ok_or(WorkspaceError::not_found())?; + /* + * The statements in the document might already have associated diagnostics, + * e.g. if they contain syntax errors that surfaced while parsing/splitting the statements + */ let mut diagnostics: Vec = parser.document_diagnostics().to_vec(); + /* + * Type-checking against database connection + */ if let Some(pool) = self.get_current_connection() { let path_clone = params.path.clone(); let schema_cache = self.schema_cache.load(pool.clone())?; @@ -518,6 +504,29 @@ impl Workspace for WorkspaceServer { } } + /* + * Below, we'll apply our static linting rules against the statements, + * considering the user's settings + */ + let (enabled_rules, disabled_rules) = AnalyserVisitorBuilder::new(settings) + .with_linter_rules(¶ms.only, ¶ms.skip) + .finish(); + + let options = AnalyserOptions { + rules: to_analyser_rules(settings), + }; + + let filter = AnalysisFilter { + categories: params.categories, + enabled_rules: Some(enabled_rules.as_slice()), + disabled_rules: &disabled_rules, + }; + + let analyser = Analyser::new(AnalyserConfig { + options: &options, + filter, + }); + diagnostics.extend(parser.iter(SyncDiagnosticsMapper).flat_map( |(_id, range, ast, diag)| { let mut errors: Vec = vec![]; diff --git a/crates/pgt_workspace/src/workspace/server/annotation.rs b/crates/pgt_workspace/src/workspace/server/annotation.rs index 321dd3ac..2fdf32eb 100644 --- a/crates/pgt_workspace/src/workspace/server/annotation.rs +++ b/crates/pgt_workspace/src/workspace/server/annotation.rs @@ -22,10 +22,10 @@ impl AnnotationStore { #[allow(unused)] pub fn get_annotations( &self, - statement: &StatementId, + statement_id: &StatementId, content: &str, ) -> Option> { - if let Some(existing) = self.db.get(statement).map(|x| x.clone()) { + if let Some(existing) = self.db.get(statement_id).map(|x| x.clone()) { return existing; } @@ -43,7 +43,7 @@ impl AnnotationStore { }) }); - self.db.insert(statement.clone(), None); + self.db.insert(statement_id.clone(), None); annotations } diff --git a/docs/rule_sources.md b/docs/rule_sources.md index b5c1f49f..8c0f085d 100644 --- a/docs/rule_sources.md +++ b/docs/rule_sources.md @@ -5,5 +5,7 @@ | ---- | ---- | | [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-database](https://squawkhq.com/docs/ban-drop-database) |[banDropDatabase](./rules/ban-drop-database) | | [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) | +| [ban-truncate-cascade](https://squawkhq.com/docs/ban-truncate-cascade) |[banTruncateCascade](./rules/ban-truncate-cascade) | diff --git a/docs/rules.md b/docs/rules.md index 1f674af6..19e110c6 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -14,8 +14,10 @@ Rules that detect potential safety issues in your code. | --- | --- | --- | | [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. | ✅ | +| [banDropDatabase](/rules/ban-drop-database) | Dropping a database may break existing clients (and everything else, really). | | | [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. | ✅ | +| [banTruncateCascade](/rules/ban-truncate-cascade) | Using `TRUNCATE`'s `CASCADE` option will truncate any tables that are also foreign-keyed to the specified tables. | | [//]: # (END RULES_INDEX) diff --git a/docs/rules/ban-drop-column.md b/docs/rules/ban-drop-column.md index 49a0d054..0c46d40a 100644 --- a/docs/rules/ban-drop-column.md +++ b/docs/rules/ban-drop-column.md @@ -27,7 +27,7 @@ alter table test drop column id; ```sh code-block.sql lint/safety/banDropColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Dropping a column may break existing clients. + ! Dropping a column may break existing clients. i You can leave the column as nullable or delete the column once queries no longer select or modify the column. diff --git a/docs/rules/ban-drop-database.md b/docs/rules/ban-drop-database.md new file mode 100644 index 00000000..8bbcd396 --- /dev/null +++ b/docs/rules/ban-drop-database.md @@ -0,0 +1,28 @@ +# banDropDatabase +**Diagnostic Category: `lint/safety/banDropDatabase`** + +**Since**: `vnext` + + +**Sources**: +- Inspired from: squawk/ban-drop-database + +## Description +Dropping a database may break existing clients (and everything else, really). + +Make sure that you really want to drop it. + +## How to configure +```json + +{ + "linter": { + "rules": { + "safety": { + "banDropDatabase": "error" + } + } + } +} + +``` diff --git a/docs/rules/ban-drop-not-null.md b/docs/rules/ban-drop-not-null.md index ccf49f95..b860c45c 100644 --- a/docs/rules/ban-drop-not-null.md +++ b/docs/rules/ban-drop-not-null.md @@ -27,7 +27,7 @@ alter table users alter column email drop not null; ```sh code-block.sql lint/safety/banDropNotNull ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Dropping a NOT NULL constraint may break existing clients. + ! Dropping a NOT NULL constraint may break existing clients. i 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. diff --git a/docs/rules/ban-drop-table.md b/docs/rules/ban-drop-table.md index f2f34156..4b81755f 100644 --- a/docs/rules/ban-drop-table.md +++ b/docs/rules/ban-drop-table.md @@ -28,7 +28,7 @@ drop table some_table; ```sh code-block.sql lint/safety/banDropTable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Dropping a table may break existing clients. + ! Dropping a table may break existing clients. i Update your application code to no longer read or write the table, and only then delete the table. Be sure to create a backup. diff --git a/docs/rules/ban-truncate-cascade.md b/docs/rules/ban-truncate-cascade.md new file mode 100644 index 00000000..1a4502d2 --- /dev/null +++ b/docs/rules/ban-truncate-cascade.md @@ -0,0 +1,40 @@ +# banTruncateCascade +**Diagnostic Category: `lint/safety/banTruncateCascade`** + +**Since**: `vnext` + + +**Sources**: +- Inspired from: squawk/ban-truncate-cascade + +## Description +Using `TRUNCATE`'s `CASCADE` option will truncate any tables that are also foreign-keyed to the specified tables. + +So if you had tables with foreign-keys like: + +`a <- b <- c` + +and ran: + +`truncate a cascade;` + +You'd end up with a, b, & c all being truncated! + +Instead, you can manually specify the tables you want. + +`truncate a, b;` + +## How to configure +```json + +{ + "linter": { + "rules": { + "safety": { + "banTruncateCascade": "error" + } + } + } +} + +``` diff --git a/docs/schema.json b/docs/schema.json index 8c478d0a..1c56618e 100644 --- a/docs/schema.json +++ b/docs/schema.json @@ -349,6 +349,17 @@ } ] }, + "banDropDatabase": { + "description": "Dropping a database may break existing clients (and everything else, really).", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "banDropNotNull": { "description": "Dropping a NOT NULL constraint may break existing clients.", "anyOf": [ @@ -371,6 +382,17 @@ } ] }, + "banTruncateCascade": { + "description": "Using TRUNCATE's CASCADE option will truncate any tables that are also foreign-keyed to the specified tables.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "recommended": { "description": "It enables the recommended rules for this group", "type": [ diff --git a/justfile b/justfile index c868a122..7e53a8a6 100644 --- a/justfile +++ b/justfile @@ -31,8 +31,8 @@ gen-lint: just format # Creates a new lint rule in the given path, with the given name. Name has to be camel case. Group should be lowercase. -new-lintrule group rulename: - cargo run -p xtask_codegen -- new-lintrule --category=lint --name={{rulename}} --group={{group}} +new-lintrule group rulename severity="error": + cargo run -p xtask_codegen -- new-lintrule --category=lint --name={{rulename}} --group={{group}} --severity={{severity}} just gen-lint # Format Rust, JS and TOML files diff --git a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts index a81a1ca9..72b77bc1 100644 --- a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts @@ -65,8 +65,10 @@ export interface Advices { export type Category = | "lint/safety/addingRequiredField" | "lint/safety/banDropColumn" + | "lint/safety/banDropDatabase" | "lint/safety/banDropNotNull" | "lint/safety/banDropTable" + | "lint/safety/banTruncateCascade" | "stdin" | "check" | "configuration" @@ -217,7 +219,8 @@ export type CompletionItemKind = | "function" | "column" | "schema" - | "policy"; + | "policy" + | "role"; export interface UpdateSettingsParams { configuration: PartialConfiguration; gitignore_matches: string[]; @@ -391,6 +394,10 @@ export interface Safety { * Dropping a column may break existing clients. */ banDropColumn?: RuleConfiguration_for_Null; + /** + * Dropping a database may break existing clients (and everything else, really). + */ + banDropDatabase?: RuleConfiguration_for_Null; /** * Dropping a NOT NULL constraint may break existing clients. */ @@ -399,6 +406,10 @@ export interface Safety { * Dropping a table may break existing clients. */ banDropTable?: RuleConfiguration_for_Null; + /** + * Using TRUNCATE's CASCADE option will truncate any tables that are also foreign-keyed to the specified tables. + */ + banTruncateCascade?: RuleConfiguration_for_Null; /** * It enables the recommended rules for this group */ diff --git a/xtask/codegen/Cargo.toml b/xtask/codegen/Cargo.toml index b5497b2c..758a3212 100644 --- a/xtask/codegen/Cargo.toml +++ b/xtask/codegen/Cargo.toml @@ -14,6 +14,7 @@ biome_string_case = { workspace = true } bpaf = { workspace = true, features = ["derive"] } pgt_analyse = { workspace = true } pgt_analyser = { workspace = true } +pgt_diagnostics = { workspace = true } pgt_workspace = { workspace = true, features = ["schema"] } proc-macro2 = { workspace = true, features = ["span-locations"] } pulldown-cmark = { version = "0.12.2" } diff --git a/xtask/codegen/src/generate_configuration.rs b/xtask/codegen/src/generate_configuration.rs index 91ae304c..3799f19b 100644 --- a/xtask/codegen/src/generate_configuration.rs +++ b/xtask/codegen/src/generate_configuration.rs @@ -1,6 +1,7 @@ use crate::{to_capitalized, update}; use biome_string_case::Case; use pgt_analyse::{GroupCategory, RegistryVisitor, Rule, RuleCategory, RuleGroup, RuleMetadata}; +use pgt_diagnostics::Severity; use proc_macro2::{Ident, Literal, Span, TokenStream}; use pulldown_cmark::{Event, Parser, Tag, TagEnd}; use quote::quote; @@ -135,10 +136,9 @@ fn generate_for_groups( /// Given a category coming from [Diagnostic](pgt_diagnostics::Diagnostic), this function returns /// the [Severity](pgt_diagnostics::Severity) associated to the rule, if the configuration changed it. - /// If the severity is off or not set, then the function returns the default severity of the rule: - /// [Severity::Error] for recommended rules and [Severity::Warning] for other rules. - /// - /// If not, the function returns [None]. + /// If the severity is off or not set, then the function returns the default severity of the rule, + /// which is configured at the rule definition. + /// The function can return `None` if the rule is not properly configured. pub fn get_severity_from_code(&self, category: &Category) -> Option { let mut split_code = category.name().split('/'); @@ -155,13 +155,10 @@ fn generate_for_groups( .as_ref() .and_then(|group| group.get_rule_configuration(rule_name)) .filter(|(level, _)| !matches!(level, RulePlainConfiguration::Off)) - .map_or_else(|| { - if #group_pascal_idents::is_recommended_rule(rule_name) { - Severity::Error - } else { - Severity::Warning - } - }, |(level, _)| level.into()), + .map_or_else( + || #group_pascal_idents::severity(rule_name), + |(level, _)| level.into() + ), )* }; Some(severity) @@ -453,7 +450,6 @@ fn generate_group_struct( rules: &BTreeMap<&'static str, RuleMetadata>, kind: RuleCategory, ) -> TokenStream { - let mut lines_recommended_rule = Vec::new(); let mut lines_recommended_rule_as_filter = Vec::new(); let mut lines_all_rule_as_filter = Vec::new(); let mut lines_rule = Vec::new(); @@ -461,6 +457,7 @@ fn generate_group_struct( let mut rule_enabled_check_line = Vec::new(); let mut rule_disabled_check_line = Vec::new(); let mut get_rule_configuration_line = Vec::new(); + let mut get_severity_lines = Vec::new(); for (index, (rule, metadata)) in rules.iter().enumerate() { let summary = { @@ -522,10 +519,6 @@ fn generate_group_struct( lines_recommended_rule_as_filter.push(quote! { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[#rule_position]) }); - - lines_recommended_rule.push(quote! { - #rule - }); } lines_all_rule_as_filter.push(quote! { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[#rule_position]) @@ -567,6 +560,18 @@ fn generate_group_struct( get_rule_configuration_line.push(quote! { #rule => self.#rule_identifier.as_ref().map(|conf| (conf.level(), conf.get_options())) }); + + let severity = match metadata.severity { + Severity::Hint => quote! { Severity::Hint }, + Severity::Information => quote! { Severity::Information }, + Severity::Warning => quote! { Severity::Warning }, + Severity::Error => quote! { Severity::Error }, + Severity::Fatal => quote! { Severity::Fatal }, + }; + + get_severity_lines.push(quote! { + #rule => #severity + }) } let group_pascal_ident = Ident::new(&to_capitalized(group), Span::call_site()); @@ -648,10 +653,6 @@ fn generate_group_struct( #( #lines_rule ),* ]; - const RECOMMENDED_RULES: &'static [&'static str] = &[ - #( #lines_recommended_rule ),* - ]; - const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ #( #lines_recommended_rule_as_filter ),* ]; @@ -695,11 +696,6 @@ fn generate_group_struct( Some(Self::GROUP_RULES[Self::GROUP_RULES.binary_search(&rule_name).ok()?]) } - /// Checks if, given a rule name, it is marked as recommended - pub(crate) fn is_recommended_rule(rule_name: &str) -> bool { - Self::RECOMMENDED_RULES.contains(&rule_name) - } - pub(crate) fn recommended_rules_as_filters() -> &'static [RuleFilter<'static>] { Self::RECOMMENDED_RULES_AS_FILTERS } @@ -725,6 +721,13 @@ fn generate_group_struct( } } + pub(crate) fn severity(rule_name: &str) -> Severity { + match rule_name { + #( #get_severity_lines ),*, + _ => unreachable!() + } + } + #get_configuration_function } } diff --git a/xtask/codegen/src/generate_new_analyser_rule.rs b/xtask/codegen/src/generate_new_analyser_rule.rs index 6fecdff7..fc225712 100644 --- a/xtask/codegen/src/generate_new_analyser_rule.rs +++ b/xtask/codegen/src/generate_new_analyser_rule.rs @@ -1,5 +1,6 @@ use biome_string_case::Case; use bpaf::Bpaf; +use pgt_diagnostics::Severity; use std::str::FromStr; use xtask::project_root; @@ -24,15 +25,26 @@ fn generate_rule_template( category: &Category, rule_name_upper_camel: &str, rule_name_lower_camel: &str, + severity: Severity, ) -> String { let macro_name = match category { Category::Lint => "declare_lint_rule", }; + + let severity_code = match severity { + Severity::Hint => "Severity::Hint", + Severity::Information => "Severity::Information", + Severity::Warning => "Severity::Warning", + Severity::Error => "Severity::Error", + Severity::Fatal => "Severity::Fatal", + }; + format!( r#"use pgt_analyse::{{ context::RuleContext, {macro_name}, Rule, RuleDiagnostic }}; use pgt_console::markup; +use pgt_diagnostics::Severity; {macro_name}! {{ /// Succinct description of the rule. @@ -58,6 +70,7 @@ use pgt_console::markup; pub {rule_name_upper_camel} {{ version: "next", name: "{rule_name_lower_camel}", + severity: {severity_code}, recommended: false, }} }} @@ -77,7 +90,12 @@ fn gen_sql(category_name: &str) -> String { format!("-- expect_only_{category_name}\n-- select 1;") } -pub fn generate_new_analyser_rule(category: Category, rule_name: &str, group: &str) { +pub fn generate_new_analyser_rule( + category: Category, + rule_name: &str, + group: &str, + severity: Severity, +) { let rule_name_camel = Case::Camel.convert(rule_name); let crate_folder = project_root().join("crates/pgt_analyser"); let rule_folder = match &category { @@ -92,6 +110,7 @@ pub fn generate_new_analyser_rule(category: Category, rule_name: &str, group: &s &category, Case::Pascal.convert(rule_name).as_str(), rule_name_camel.as_str(), + severity, ); let file_name = format!( "{}/{}.rs", diff --git a/xtask/codegen/src/lib.rs b/xtask/codegen/src/lib.rs index 61ae5e4f..dc6f81a0 100644 --- a/xtask/codegen/src/lib.rs +++ b/xtask/codegen/src/lib.rs @@ -13,6 +13,7 @@ pub use self::generate_crate::generate_crate; pub use self::generate_new_analyser_rule::generate_new_analyser_rule; use bpaf::Bpaf; use generate_new_analyser_rule::Category; +use pgt_diagnostics::Severity; use std::path::Path; use xtask::{glue::fs2, Mode, Result}; @@ -84,5 +85,9 @@ pub enum TaskCommand { /// Group of the rule #[bpaf(long("group"))] group: String, + + /// Severity of the rule + #[bpaf(long("severity"), fallback(Severity::Error))] + severity: Severity, }, } diff --git a/xtask/codegen/src/main.rs b/xtask/codegen/src/main.rs index 8e0e6cd8..4ff33c21 100644 --- a/xtask/codegen/src/main.rs +++ b/xtask/codegen/src/main.rs @@ -21,8 +21,9 @@ fn main() -> Result<()> { name, category, group, + severity, } => { - generate_new_analyser_rule(category, &name, &group); + generate_new_analyser_rule(category, &name, &group, severity); } TaskCommand::Configuration => { generate_rules_configuration(Overwrite)?;