Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions crates/squawk_linter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ 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_table_schema;
use rules::require_timeout_settings;
use rules::transaction_nesting;
// xtask:new-rule:rule-import
Expand Down Expand Up @@ -92,9 +93,18 @@ pub enum Rule {
RequireTimeoutSettings,
BanUncommittedTransaction,
RequireEnumValueOrdering,
RequireTableSchema,
// xtask:new-rule:error-name
}

impl Rule {
/// Rules that are opt-in are not enabled by default.
/// They must be explicitly included via configuration.
pub fn is_opt_in(&self) -> bool {
matches!(self, Rule::RequireTableSchema)
}
}

impl TryFrom<&str> for Rule {
type Error = String;

Expand Down Expand Up @@ -135,6 +145,7 @@ impl TryFrom<&str> for Rule {
"require-timeout-settings" => Ok(Rule::RequireTimeoutSettings),
"ban-uncommitted-transaction" => Ok(Rule::BanUncommittedTransaction),
"require-enum-value-ordering" => Ok(Rule::RequireEnumValueOrdering),
"require-table-schema" => Ok(Rule::RequireTableSchema),
// xtask:new-rule:str-name
_ => Err(format!("Unknown violation name: {s}")),
}
Expand Down Expand Up @@ -198,6 +209,7 @@ impl fmt::Display for Rule {
Rule::RequireTimeoutSettings => "require-timeout-settings",
Rule::BanUncommittedTransaction => "ban-uncommitted-transaction",
Rule::RequireEnumValueOrdering => "require-enum-value-ordering",
Rule::RequireTableSchema => "require-table-schema",
// xtask:new-rule:variant-to-name
};
write!(f, "{val}")
Expand Down Expand Up @@ -428,6 +440,9 @@ impl Linter {
if self.rules.contains(&Rule::RequireEnumValueOrdering) {
require_enum_value_ordering(self, file);
}
if self.rules.contains(&Rule::RequireTableSchema) {
require_table_schema(self, file);
}
// xtask:new-rule:rule-call

// locate any ignores in the file
Expand All @@ -452,12 +467,16 @@ impl Linter {
}

pub fn with_all_rules() -> Self {
let rules = all::<Rule>().collect::<FxHashSet<_>>();
let rules = all::<Rule>()
.filter(|r| !r.is_opt_in())
.collect::<FxHashSet<_>>();
Linter::from(rules)
}
Comment on lines 469 to 474

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How bad is this? I'm afraid that adding require-table-schema will affect many current squawk users (the linter will fail after the update).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we'll want to have it disabled by default, I'm not 100% sure of people's usage patterns

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at where we call with_all_rules, it's only used in the LSP Server & Playground (via Wasm)

So I think we could skip this is_opt_in method

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about without_rules? Just always insert RequireTableSchema into exclude_set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think so, without_rules is disabling the rules that are explicitly passed in


pub fn without_rules(exclude: &[Rule]) -> Self {
let all_rules = all::<Rule>().collect::<FxHashSet<_>>();
let all_rules = all::<Rule>()
.filter(|r| !r.is_opt_in())
.collect::<FxHashSet<_>>();
let mut exclude_set = FxHashSet::with_capacity_and_hasher(exclude.len(), FxBuildHasher);
for e in exclude {
exclude_set.insert(e);
Expand Down
2 changes: 2 additions & 0 deletions crates/squawk_linter/src/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ 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_table_schema;
pub(crate) mod require_timeout_settings;
pub(crate) mod transaction_nesting;
// xtask:new-rule:mod-decl
Expand Down Expand Up @@ -60,6 +61,7 @@ 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_table_schema::require_table_schema;
pub(crate) use require_timeout_settings::require_timeout_settings;
pub(crate) use transaction_nesting::transaction_nesting;
// xtask:new-rule:export
113 changes: 113 additions & 0 deletions crates/squawk_linter/src/rules/require_table_schema.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
use squawk_syntax::{
Parse, SourceFile,
ast::{self, AstNode},
};

use crate::{Linter, Rule, Violation};

pub(crate) fn require_table_schema(ctx: &mut Linter, parse: &Parse<SourceFile>) {
let file = parse.tree();
for stmt in file.stmts() {
match stmt {
ast::Stmt::CreateTable(create_table) => {
check_path(ctx, create_table.path(), create_table.syntax());
}
ast::Stmt::CreateTableAs(create_table_as) => {
check_path(ctx, create_table_as.path(), create_table_as.syntax());
}
ast::Stmt::AlterTable(alter_table) => {
let path = alter_table.relation_name().and_then(|r| r.path());
check_path(ctx, path, alter_table.syntax());
}
ast::Stmt::DropTable(drop_table) => {
check_path(ctx, drop_table.path(), drop_table.syntax());
}
_ => {}
}
}
}

fn check_path(ctx: &mut Linter, path: Option<ast::Path>, syntax: &squawk_syntax::SyntaxNode) {
if let Some(path) = path {
if path.qualifier().is_none() {
ctx.report(Violation::for_node(
Rule::RequireTableSchema,
"Table name is not schema-qualified. Use schema.table (e.g., public.my_table)."
.into(),
syntax,
));
}
}
}

#[cfg(test)]
mod test {
use insta::assert_snapshot;

use crate::Rule;
use crate::test_utils::{lint_errors, lint_ok};

#[test]
fn create_table_err() {
let sql = r#"
CREATE TABLE my_table (id int);
"#;
assert_snapshot!(lint_errors(sql, Rule::RequireTableSchema));
}

#[test]
fn create_table_ok() {
let sql = r#"
CREATE TABLE public.my_table (id int);
"#;
lint_ok(sql, Rule::RequireTableSchema);
}

#[test]
fn alter_table_err() {
let sql = r#"
ALTER TABLE my_table ADD COLUMN name text;
"#;
assert_snapshot!(lint_errors(sql, Rule::RequireTableSchema));
}

#[test]
fn alter_table_ok() {
let sql = r#"
ALTER TABLE public.my_table ADD COLUMN name text;
"#;
lint_ok(sql, Rule::RequireTableSchema);
}

#[test]
fn drop_table_err() {
let sql = r#"
DROP TABLE my_table;
"#;
assert_snapshot!(lint_errors(sql, Rule::RequireTableSchema));
}

#[test]
fn drop_table_ok() {
let sql = r#"
DROP TABLE public.my_table;
"#;
lint_ok(sql, Rule::RequireTableSchema);
}

#[test]
fn create_table_as_err() {
let sql = r#"
CREATE TABLE my_table AS SELECT 1;
"#;
assert_snapshot!(lint_errors(sql, Rule::RequireTableSchema));
}

#[test]
fn create_table_as_ok() {
let sql = r#"
CREATE TABLE public.my_table AS SELECT 1;
"#;
lint_ok(sql, Rule::RequireTableSchema);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
source: crates/squawk_linter/src/rules/require_table_schema.rs
expression: "lint_errors(sql, Rule::RequireTableSchema)"
---
warning[require-table-schema]: Table name is not schema-qualified. Use schema.table (e.g., public.my_table).
╭▸
2 │ ALTER TABLE my_table ADD COLUMN name text;
╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
source: crates/squawk_linter/src/rules/require_table_schema.rs
expression: "lint_errors(sql, Rule::RequireTableSchema)"
---
warning[require-table-schema]: Table name is not schema-qualified. Use schema.table (e.g., public.my_table).
╭▸
2 │ CREATE TABLE my_table AS SELECT 1;
╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
source: crates/squawk_linter/src/rules/require_table_schema.rs
expression: "lint_errors(sql, Rule::RequireTableSchema)"
---
warning[require-table-schema]: Table name is not schema-qualified. Use schema.table (e.g., public.my_table).
╭▸
2 │ CREATE TABLE my_table (id int);
╰╴━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
source: crates/squawk_linter/src/rules/require_table_schema.rs
expression: "lint_errors(sql, Rule::RequireTableSchema)"
---
warning[require-table-schema]: Table name is not schema-qualified. Use schema.table (e.g., public.my_table).
╭▸
2 │ DROP TABLE my_table;
╰╴━━━━━━━━━━━━━━━━━━━
Loading