diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 9da89fbe..e72654b6 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -53,6 +53,7 @@ use rules::renaming_column; use rules::renaming_table; use rules::require_concurrent_index_creation; use rules::require_concurrent_index_deletion; +use rules::require_enum_value_ordering; use rules::require_timeout_settings; use rules::transaction_nesting; // xtask:new-rule:rule-import @@ -90,6 +91,7 @@ pub enum Rule { BanTruncateCascade, RequireTimeoutSettings, BanUncommittedTransaction, + RequireEnumValueOrdering, // xtask:new-rule:error-name } @@ -132,6 +134,7 @@ impl TryFrom<&str> for Rule { "ban-truncate-cascade" => Ok(Rule::BanTruncateCascade), "require-timeout-settings" => Ok(Rule::RequireTimeoutSettings), "ban-uncommitted-transaction" => Ok(Rule::BanUncommittedTransaction), + "require-enum-value-ordering" => Ok(Rule::RequireEnumValueOrdering), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -194,6 +197,7 @@ impl fmt::Display for Rule { Rule::BanTruncateCascade => "ban-truncate-cascade", Rule::RequireTimeoutSettings => "require-timeout-settings", Rule::BanUncommittedTransaction => "ban-uncommitted-transaction", + Rule::RequireEnumValueOrdering => "require-enum-value-ordering", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -421,6 +425,9 @@ impl Linter { if self.rules.contains(&Rule::BanUncommittedTransaction) { ban_uncommitted_transaction(self, file); } + if self.rules.contains(&Rule::RequireEnumValueOrdering) { + require_enum_value_ordering(self, file); + } // xtask:new-rule:rule-call // locate any ignores in the file diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index 1bb4e019..1cc5fe98 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -26,6 +26,7 @@ pub(crate) mod renaming_column; pub(crate) mod renaming_table; pub(crate) mod require_concurrent_index_creation; pub(crate) mod require_concurrent_index_deletion; +pub(crate) mod require_enum_value_ordering; pub(crate) mod require_timeout_settings; pub(crate) mod transaction_nesting; // xtask:new-rule:mod-decl @@ -58,6 +59,7 @@ pub(crate) use renaming_column::renaming_column; pub(crate) use renaming_table::renaming_table; pub(crate) use require_concurrent_index_creation::require_concurrent_index_creation; pub(crate) use require_concurrent_index_deletion::require_concurrent_index_deletion; +pub(crate) use require_enum_value_ordering::require_enum_value_ordering; pub(crate) use require_timeout_settings::require_timeout_settings; pub(crate) use transaction_nesting::transaction_nesting; // xtask:new-rule:export diff --git a/crates/squawk_linter/src/rules/require_enum_value_ordering.rs b/crates/squawk_linter/src/rules/require_enum_value_ordering.rs new file mode 100644 index 00000000..3d5bdba2 --- /dev/null +++ b/crates/squawk_linter/src/rules/require_enum_value_ordering.rs @@ -0,0 +1,82 @@ +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +use crate::{Edit, Fix, Linter, Rule, Violation}; + +fn create_fix(add_value: &ast::AddValue) -> Option { + let literal = add_value.literal()?; + let insert_at = literal.syntax().text_range().end(); + let edit = Edit::insert(" BEFORE 'existing_value'", insert_at); + Some(Fix::new("Insert `BEFORE` clause", vec![edit])) +} + +pub(crate) fn require_enum_value_ordering(ctx: &mut Linter, parse: &Parse) { + let file = parse.tree(); + for stmt in file.stmts() { + if let ast::Stmt::AlterType(alter_type) = stmt + && let Some(add_value) = alter_type.add_value() + && add_value.value_position().is_none() + { + let fix = create_fix(&add_value); + ctx.report( + Violation::for_node( + Rule::RequireEnumValueOrdering, + "ADD VALUE without BEFORE or AFTER appends the value to the end of the enum, which may result in unexpected ordering.".into(), + add_value.syntax(), + ) + .help("Add `BEFORE` or `AFTER` to specify the position of the new enum value.") + .fix(fix), + ); + } + } +} + +#[cfg(test)] +mod test { + use insta::assert_snapshot; + + use crate::Rule; + use crate::test_utils::{fix_sql, lint_errors, lint_ok}; + + #[test] + fn err_add_value_without_ordering() { + let sql = r#" +ALTER TYPE my_enum ADD VALUE 'new_value'; +"#; + assert_snapshot!(lint_errors(sql, Rule::RequireEnumValueOrdering)); + } + + #[test] + fn err_add_value_if_not_exists_without_ordering() { + let sql = r#" +ALTER TYPE my_enum ADD VALUE IF NOT EXISTS 'new_value'; +"#; + assert_snapshot!(lint_errors(sql, Rule::RequireEnumValueOrdering)); + } + + #[test] + fn fix_add_value_without_ordering() { + let sql = r#" +ALTER TYPE my_enum ADD VALUE 'new_value'; +"#; + assert_snapshot!(fix_sql(sql, Rule::RequireEnumValueOrdering), @"ALTER TYPE my_enum ADD VALUE 'new_value' BEFORE 'existing_value';"); + } + + #[test] + fn ok_add_value_before() { + let sql = r#" +ALTER TYPE my_enum ADD VALUE 'new_value' BEFORE 'existing_value'; +"#; + lint_ok(sql, Rule::RequireEnumValueOrdering); + } + + #[test] + fn ok_add_value_after() { + let sql = r#" +ALTER TYPE my_enum ADD VALUE 'new_value' AFTER 'existing_value'; +"#; + lint_ok(sql, Rule::RequireEnumValueOrdering); + } +} diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__require_enum_value_ordering__test__err_add_value_if_not_exists_without_ordering.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__require_enum_value_ordering__test__err_add_value_if_not_exists_without_ordering.snap new file mode 100644 index 00000000..6fd0e4d1 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__require_enum_value_ordering__test__err_add_value_if_not_exists_without_ordering.snap @@ -0,0 +1,13 @@ +--- +source: crates/squawk_linter/src/rules/require_enum_value_ordering.rs +expression: "lint_errors(sql, Rule::RequireEnumValueOrdering)" +--- +warning[require-enum-value-ordering]: ADD VALUE without BEFORE or AFTER appends the value to the end of the enum, which may result in unexpected ordering. + ╭▸ +2 │ ALTER TYPE my_enum ADD VALUE IF NOT EXISTS 'new_value'; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Add `BEFORE` or `AFTER` to specify the position of the new enum value. + ╭╴ +2 │ ALTER TYPE my_enum ADD VALUE IF NOT EXISTS 'new_value' BEFORE 'existing_value'; + ╰╴ +++++++++++++++++++++++ diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__require_enum_value_ordering__test__err_add_value_without_ordering.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__require_enum_value_ordering__test__err_add_value_without_ordering.snap new file mode 100644 index 00000000..0522e3b3 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__require_enum_value_ordering__test__err_add_value_without_ordering.snap @@ -0,0 +1,13 @@ +--- +source: crates/squawk_linter/src/rules/require_enum_value_ordering.rs +expression: "lint_errors(sql, Rule::RequireEnumValueOrdering)" +--- +warning[require-enum-value-ordering]: ADD VALUE without BEFORE or AFTER appends the value to the end of the enum, which may result in unexpected ordering. + ╭▸ +2 │ ALTER TYPE my_enum ADD VALUE 'new_value'; + │ ━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Add `BEFORE` or `AFTER` to specify the position of the new enum value. + ╭╴ +2 │ ALTER TYPE my_enum ADD VALUE 'new_value' BEFORE 'existing_value'; + ╰╴ +++++++++++++++++++++++ diff --git a/crates/squawk_parser/src/generated/syntax_kind.rs b/crates/squawk_parser/src/generated/syntax_kind.rs index 82bca450..0f9e1296 100644 --- a/crates/squawk_parser/src/generated/syntax_kind.rs +++ b/crates/squawk_parser/src/generated/syntax_kind.rs @@ -557,6 +557,7 @@ pub enum SyntaxKind { ADD_GENERATED, ADD_OP_CLASS_OPTIONS, ADD_VALUE, + AFTER_VALUE, AGGREGATE, ALIAS, ALL_FN, @@ -622,6 +623,7 @@ pub enum SyntaxKind { ATTRIBUTE_OPTION, ATTRIBUTE_VALUE, AT_TIME_ZONE, + BEFORE_VALUE, BEGIN, BEGIN_FUNC_OPTION, BEGIN_FUNC_OPTION_LIST, diff --git a/crates/squawk_parser/src/grammar.rs b/crates/squawk_parser/src/grammar.rs index ad3727d5..2b67f55d 100644 --- a/crates/squawk_parser/src/grammar.rs +++ b/crates/squawk_parser/src/grammar.rs @@ -8003,9 +8003,7 @@ fn alter_type(p: &mut Parser<'_>) -> CompletedMarker { p.expect(VALUE_KW); opt_if_not_exists(p); string_literal(p); - if p.eat(BEFORE_KW) || p.eat(AFTER_KW) { - string_literal(p); - } + opt_value_position(p); m.complete(p, ADD_VALUE); } _ => p.error("expected ALTER TYPE option"), @@ -8013,6 +8011,20 @@ fn alter_type(p: &mut Parser<'_>) -> CompletedMarker { m.complete(p, ALTER_TYPE) } +fn opt_value_position(p: &mut Parser<'_>) { + let m = p.start(); + let kind = if p.eat(BEFORE_KW) { + BEFORE_VALUE + } else if p.eat(AFTER_KW) { + AFTER_VALUE + } else { + m.abandon(p); + return; + }; + string_literal(p); + m.complete(p, kind); +} + // ALTER USER role_specification [ WITH ] option [ ... ] // where option can be: // SUPERUSER | NOSUPERUSER diff --git a/crates/squawk_parser/tests/snapshots/tests__alter_type_ok.snap b/crates/squawk_parser/tests/snapshots/tests__alter_type_ok.snap index 684929e2..ac02acce 100644 --- a/crates/squawk_parser/tests/snapshots/tests__alter_type_ok.snap +++ b/crates/squawk_parser/tests/snapshots/tests__alter_type_ok.snap @@ -229,10 +229,11 @@ SOURCE_FILE LITERAL STRING "'v'" WHITESPACE " " - BEFORE_KW "before" - WHITESPACE " " - LITERAL - STRING "'w'" + BEFORE_VALUE + BEFORE_KW "before" + WHITESPACE " " + LITERAL + STRING "'w'" SEMICOLON ";" WHITESPACE "\n" ALTER_TYPE @@ -265,10 +266,11 @@ SOURCE_FILE LITERAL STRING "'v'" WHITESPACE " " - AFTER_KW "after" - WHITESPACE " " - LITERAL - STRING "'w'" + AFTER_VALUE + AFTER_KW "after" + WHITESPACE " " + LITERAL + STRING "'w'" SEMICOLON ";" WHITESPACE "\n\n" COMMENT "-- rename_value" diff --git a/crates/squawk_syntax/src/ast/generated/nodes.rs b/crates/squawk_syntax/src/ast/generated/nodes.rs index ae0322c0..5314d316 100644 --- a/crates/squawk_syntax/src/ast/generated/nodes.rs +++ b/crates/squawk_syntax/src/ast/generated/nodes.rs @@ -168,20 +168,31 @@ impl AddValue { support::child(&self.syntax) } #[inline] + pub fn value_position(&self) -> Option { + support::child(&self.syntax) + } + #[inline] pub fn add_token(&self) -> Option { support::token(&self.syntax, SyntaxKind::ADD_KW) } #[inline] - pub fn after_token(&self) -> Option { - support::token(&self.syntax, SyntaxKind::AFTER_KW) + pub fn value_token(&self) -> Option { + support::token(&self.syntax, SyntaxKind::VALUE_KW) } +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct AfterValue { + pub(crate) syntax: SyntaxNode, +} +impl AfterValue { #[inline] - pub fn before_token(&self) -> Option { - support::token(&self.syntax, SyntaxKind::BEFORE_KW) + pub fn literal(&self) -> Option { + support::child(&self.syntax) } #[inline] - pub fn value_token(&self) -> Option { - support::token(&self.syntax, SyntaxKind::VALUE_KW) + pub fn after_token(&self) -> Option { + support::token(&self.syntax, SyntaxKind::AFTER_KW) } } @@ -2160,6 +2171,21 @@ impl AttributeValue { } } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct BeforeValue { + pub(crate) syntax: SyntaxNode, +} +impl BeforeValue { + #[inline] + pub fn literal(&self) -> Option { + support::child(&self.syntax) + } + #[inline] + pub fn before_token(&self) -> Option { + support::token(&self.syntax, SyntaxKind::BEFORE_KW) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Begin { pub(crate) syntax: SyntaxNode, @@ -17763,6 +17789,12 @@ pub enum Type { TimeType(TimeType), } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum ValuePosition { + AfterValue(AfterValue), + BeforeValue(BeforeValue), +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum WithQuery { CompoundSelect(CompoundSelect), @@ -17883,6 +17915,24 @@ impl AstNode for AddValue { &self.syntax } } +impl AstNode for AfterValue { + #[inline] + fn can_cast(kind: SyntaxKind) -> bool { + kind == SyntaxKind::AFTER_VALUE + } + #[inline] + fn cast(syntax: SyntaxNode) -> Option { + if Self::can_cast(syntax.kind()) { + Some(Self { syntax }) + } else { + None + } + } + #[inline] + fn syntax(&self) -> &SyntaxNode { + &self.syntax + } +} impl AstNode for Aggregate { #[inline] fn can_cast(kind: SyntaxKind) -> bool { @@ -19053,6 +19103,24 @@ impl AstNode for AttributeValue { &self.syntax } } +impl AstNode for BeforeValue { + #[inline] + fn can_cast(kind: SyntaxKind) -> bool { + kind == SyntaxKind::BEFORE_VALUE + } + #[inline] + fn cast(syntax: SyntaxNode) -> Option { + if Self::can_cast(syntax.kind()) { + Some(Self { syntax }) + } else { + None + } + } + #[inline] + fn syntax(&self) -> &SyntaxNode { + &self.syntax + } +} impl AstNode for Begin { #[inline] fn can_cast(kind: SyntaxKind) -> bool { @@ -34297,6 +34365,42 @@ impl From for Type { Type::TimeType(node) } } +impl AstNode for ValuePosition { + #[inline] + fn can_cast(kind: SyntaxKind) -> bool { + matches!(kind, SyntaxKind::AFTER_VALUE | SyntaxKind::BEFORE_VALUE) + } + #[inline] + fn cast(syntax: SyntaxNode) -> Option { + let res = match syntax.kind() { + SyntaxKind::AFTER_VALUE => ValuePosition::AfterValue(AfterValue { syntax }), + SyntaxKind::BEFORE_VALUE => ValuePosition::BeforeValue(BeforeValue { syntax }), + _ => { + return None; + } + }; + Some(res) + } + #[inline] + fn syntax(&self) -> &SyntaxNode { + match self { + ValuePosition::AfterValue(it) => &it.syntax, + ValuePosition::BeforeValue(it) => &it.syntax, + } + } +} +impl From for ValuePosition { + #[inline] + fn from(node: AfterValue) -> ValuePosition { + ValuePosition::AfterValue(node) + } +} +impl From for ValuePosition { + #[inline] + fn from(node: BeforeValue) -> ValuePosition { + ValuePosition::BeforeValue(node) + } +} impl AstNode for WithQuery { #[inline] fn can_cast(kind: SyntaxKind) -> bool { diff --git a/crates/squawk_syntax/src/postgresql.ungram b/crates/squawk_syntax/src/postgresql.ungram index 59f1157a..fae16493 100644 --- a/crates/squawk_syntax/src/postgresql.ungram +++ b/crates/squawk_syntax/src/postgresql.ungram @@ -2148,7 +2148,17 @@ AlterAttribute = 'alter' 'attribute' (Cascade | Restrict)? AddValue = - 'add' 'value' IfNotExists? Literal (('before' | 'after') Literal)? + 'add' 'value' IfNotExists? Literal ValuePosition? + +ValuePosition = + BeforeValue +| AfterValue + +BeforeValue = + 'before' Literal + +AfterValue = + 'after' Literal AlterUser = 'alter' 'user' RoleRef diff --git a/docs/docs/require-enum-value-ordering.md b/docs/docs/require-enum-value-ordering.md new file mode 100644 index 00000000..ce17b17e --- /dev/null +++ b/docs/docs/require-enum-value-ordering.md @@ -0,0 +1,53 @@ +--- +id: require-enum-value-ordering +title: require-enum-value-ordering +--- + +## problem + +Using `ALTER TYPE ... ADD VALUE` without specifying `BEFORE` or `AFTER` appends the new value to the end of the enum type. + +This can lead to unexpected ordering, especially when enum values have a meaningful order (e.g., workflow status, severity, priority). + +```sql +-- existing type +CREATE TYPE status AS ENUM ( + 'draft', + 'submitted', + 'approved', + 'rejected' +); +-- existing query to find all not yet approved +select * from papers where status < 'approved'; + +-- bad +ALTER TYPE status ADD VALUE 'pending_review'; + +-- now our query isn't including any of the 'pending_review' items! +``` + +## solution + +Explicitly specify where the new value should be inserted using `BEFORE` or `AFTER`. + +```sql +-- existing type +CREATE TYPE status AS ENUM ( + 'draft', + 'submitted', + 'approved', + 'rejected' +); + +-- existing query to find all not yet approved +select * from papers where status < 'approved'; + +-- good +ALTER TYPE status ADD VALUE 'pending_review' BEFORE 'approved'; +-- or +ALTER TYPE status ADD VALUE 'pending_review' AFTER 'submitted'; +``` + +## links + +- https://www.postgresql.org/docs/current/sql-altertype.html diff --git a/docs/sidebars.js b/docs/sidebars.js index 873997b8..c4cd1592 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -34,6 +34,7 @@ module.exports = { "syntax-error", "require-timeout-settings", "ban-uncommitted-transaction", + "require-enum-value-ordering", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index 8e36419d..b9b9b5b7 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -202,6 +202,11 @@ const rules = [ tags: ["schema"], description: "Ensure all transactions are committed", }, + { + name: "require-enum-value-ordering", + tags: ["schema"], + description: "Require BEFORE or AFTER when adding enum values", + }, // xtask:new-rule:rule-doc-meta ]