From 729368d962c0ddc605e576b74e1ff919a6de8b51 Mon Sep 17 00:00:00 2001 From: John Mastro Date: Wed, 26 Mar 2025 16:20:21 -0400 Subject: [PATCH] ban creating domains with constraints --- .../docs/ban-create-domain-with-constraint.md | 16 +++++ docs/sidebars.js | 1 + docs/src/pages/index.js | 5 ++ linter/src/lib.rs | 10 +++ .../ban_create_domain_with_constraint.rs | 66 +++++++++++++++++++ linter/src/rules/mod.rs | 2 + ...n_create_domain_with_constraint_works.snap | 21 ++++++ ...er__test_rules__rule_names_debug_snap.snap | 2 + ...__test_rules__rule_names_display_snap.snap | 2 + linter/src/violations.rs | 2 + parser/src/ast.rs | 12 +++- ...ser__parse__tests__create_domain_stmt.snap | 59 ++++++++--------- 12 files changed, 167 insertions(+), 31 deletions(-) create mode 100644 docs/docs/ban-create-domain-with-constraint.md create mode 100644 linter/src/rules/ban_create_domain_with_constraint.rs create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap diff --git a/docs/docs/ban-create-domain-with-constraint.md b/docs/docs/ban-create-domain-with-constraint.md new file mode 100644 index 00000000..41f293cf --- /dev/null +++ b/docs/docs/ban-create-domain-with-constraint.md @@ -0,0 +1,16 @@ +--- +id: ban-create-domain-with-constraint +title: ban-create-domain-with-constraint +--- + +## problem + + + +## solution + + + +## links + + \ No newline at end of file diff --git a/docs/sidebars.js b/docs/sidebars.js index 1f5e6a0a..81b1db1f 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -29,6 +29,7 @@ module.exports = { "require-concurrent-index-creation", "require-concurrent-index-deletion", "transaction-nesting", + "ban-create-domain-with-constraint", // generator::new-rule-above ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index 81ee46ec..1bada8ca 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -183,6 +183,11 @@ const rules = [ tags: ["schema"], description: "Prevent forbidden use of transactions during concurrent index creation.", }, + { + name: "ban-create-domain-with-constraint", + tags: ["TODO"], + description: "TODO", + }, // generator::new-rule-above ] diff --git a/linter/src/lib.rs b/linter/src/lib.rs index 174edeb2..60b20062 100644 --- a/linter/src/lib.rs +++ b/linter/src/lib.rs @@ -10,6 +10,7 @@ extern crate lazy_static; use crate::errors::CheckSqlError; use crate::rules::adding_required_field; use crate::rules::ban_concurrent_index_creation_in_transaction; +use crate::rules::ban_create_domain_with_constraint; use crate::rules::ban_drop_not_null; use crate::rules::prefer_big_int; use crate::rules::prefer_identity; @@ -119,6 +120,15 @@ lazy_static! { ), ], }, + SquawkRule { + name: RuleViolationKind::BanCreateDomainWithConstraint, + func: ban_create_domain_with_constraint, + messages: vec![ + ViolationMessage::Note( + "Domains with constraints have poor support for online migrations".into() + ), + ], + }, SquawkRule { name: RuleViolationKind::BanDropColumn, func: ban_drop_column, diff --git a/linter/src/rules/ban_create_domain_with_constraint.rs b/linter/src/rules/ban_create_domain_with_constraint.rs new file mode 100644 index 00000000..0daf98f1 --- /dev/null +++ b/linter/src/rules/ban_create_domain_with_constraint.rs @@ -0,0 +1,66 @@ +use crate::{ + versions::Version, + violations::{RuleViolation, RuleViolationKind}, +}; + +use squawk_parser::ast::{RawStmt, Stmt}; + +#[must_use] +pub fn ban_create_domain_with_constraint( + tree: &[RawStmt], + _pg_version: Option, + _assume_in_transaction: bool, +) -> Vec { + let mut errs = vec![]; + for raw_stmt in tree { + match &raw_stmt.stmt { + Stmt::CreateDomainStmt(stmt) if stmt.constraints.len() > 0 => { + println!("X: {:?}", &raw_stmt.stmt); + println!("{}", serde_json::to_string_pretty(&stmt).unwrap()); + errs.push(RuleViolation::new( + RuleViolationKind::BanCreateDomainWithConstraint, + raw_stmt.into(), + None, + )); + } + _ => continue, + } + } + errs +} + +#[cfg(test)] +mod test_rules { + use crate::{ + check_sql_with_rule, + violations::{RuleViolation, RuleViolationKind}, + }; + use insta::assert_debug_snapshot; + + fn lint_sql(sql: &str) -> Vec { + check_sql_with_rule( + sql, + &RuleViolationKind::BanCreateDomainWithConstraint, + None, + false, + ) + .unwrap() + } + + #[test] + fn ban_create_domain_without_constraint_is_ok() { + let sql = r#" + CREATE DOMAIN domain_name_1 AS TEXT; + CREATE DOMAIN domain_name_2 AS CHARACTER VARYING; + "#; + assert_eq!(lint_sql(sql), vec![]); + } + + #[test] + fn ban_create_domain_with_constraint_works() { + let sql = r#" + CREATE DOMAIN domain_name_3 AS NUMERIC(15,5) CHECK (value > 0); + "#; + assert_debug_snapshot!(lint_sql(sql)); + } +} diff --git a/linter/src/rules/mod.rs b/linter/src/rules/mod.rs index e4d11ce7..d1a756ab 100644 --- a/linter/src/rules/mod.rs +++ b/linter/src/rules/mod.rs @@ -52,3 +52,5 @@ pub mod adding_required_field; pub use adding_required_field::*; pub mod ban_concurrent_index_creation_in_transaction; pub use ban_concurrent_index_creation_in_transaction::*; +pub mod ban_create_domain_with_constraint; +pub use ban_create_domain_with_constraint::*; \ No newline at end of file diff --git a/linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap b/linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap new file mode 100644 index 00000000..bb1fd486 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap @@ -0,0 +1,21 @@ +--- +source: linter/src/rules/ban_create_domain_with_constraint.rs +expression: lint_sql(sql) + +--- +[ + RuleViolation { + kind: BanCreateDomainWithConstraint, + span: Span { + start: 0, + len: Some( + 67, + ), + }, + messages: [ + Note( + "Domains with constraints have poor support for online migrations", + ), + ], + }, +] diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap index 83322f6d..ba050f46 100644 --- a/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap +++ b/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap @@ -1,6 +1,7 @@ --- source: linter/src/lib.rs expression: rule_names + --- [ "adding-field-with-default", @@ -10,6 +11,7 @@ expression: rule_names "adding-serial-primary-key-field", "ban-char-field", "ban-concurrent-index-creation-in-transaction", + "ban-create-domain-with-constraint", "ban-drop-column", "ban-drop-database", "ban-drop-not-null", diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap index 12a30380..8b74ab82 100644 --- a/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap +++ b/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap @@ -1,6 +1,7 @@ --- source: linter/src/lib.rs expression: "rule_names.join(\"\\n\")" + --- adding-field-with-default adding-foreign-key-constraint @@ -9,6 +10,7 @@ adding-required-field adding-serial-primary-key-field ban-char-field ban-concurrent-index-creation-in-transaction +ban-create-domain-with-constraint ban-drop-column ban-drop-database ban-drop-not-null diff --git a/linter/src/violations.rs b/linter/src/violations.rs index 0a753ebd..4661442c 100644 --- a/linter/src/violations.rs +++ b/linter/src/violations.rs @@ -61,6 +61,8 @@ pub enum RuleViolationKind { AddingRequiredField, #[serde(rename = "ban-concurrent-index-creation-in-transaction")] BanConcurrentIndexCreationInTransaction, + #[serde(rename = "ban-create-domain-with-constraint")] + BanCreateDomainWithConstraint, // generator::new-rule-above } diff --git a/parser/src/ast.rs b/parser/src/ast.rs index 66805572..ebb5337d 100644 --- a/parser/src/ast.rs +++ b/parser/src/ast.rs @@ -429,6 +429,16 @@ pub struct AlterTableStmt { pub missing_ok: bool, } +#[derive(Debug, Deserialize, Serialize)] +pub struct CreateDomainStmt { + #[serde(rename = "domainname")] + pub domain_name: Vec, + #[serde(rename = "typeName")] + pub typename: Value, + #[serde(default)] + pub constraints: Vec, +} + /// Source: #[derive(Serialize, Deserialize, PartialEq, Debug)] pub enum DropBehavior { @@ -928,7 +938,7 @@ pub enum Stmt { CreateSeqStmt(Value), AlterSeqStmt(Value), DefineStmt(Value), - CreateDomainStmt(Value), + CreateDomainStmt(CreateDomainStmt), CreateOpClassStmt(Value), CreateOpFamilyStmt(Value), AlterOpFamilyStmt(Value), diff --git a/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap b/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap index ff0eba6f..bd69cad7 100644 --- a/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap +++ b/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap @@ -1,13 +1,38 @@ --- source: parser/src/parse.rs expression: res + --- Ok( [ RawStmt { stmt: CreateDomainStmt( - Object({ - "constraints": Array([ + CreateDomainStmt { + domain_name: [ + QualifiedName { + string: PGString { + sval: "us_postal_code", + }, + }, + ], + typename: Object({ + "location": Number( + 33, + ), + "names": Array([ + Object({ + "String": Object({ + "sval": String( + "text", + ), + }), + }), + ]), + "typemod": Number( + -1, + ), + }), + constraints: [ Object({ "Constraint": Object({ "contype": String( @@ -127,34 +152,8 @@ Ok( }), }), }), - ]), - "domainname": Array([ - Object({ - "String": Object({ - "sval": String( - "us_postal_code", - ), - }), - }), - ]), - "typeName": Object({ - "location": Number( - 33, - ), - "names": Array([ - Object({ - "String": Object({ - "sval": String( - "text", - ), - }), - }), - ]), - "typemod": Number( - -1, - ), - }), - }), + ], + }, ), stmt_location: 0, stmt_len: Some(