Skip to content

Commit f744369

Browse files
sj-robertrobert-sjoblom
sj-robert
authored and
robert-sjoblom
committed
refactor(rules): add Finding constructor and LintContext helpers
1 parent 3e2b937 commit f744369

15 files changed

Lines changed: 175 additions & 187 deletions

‎src/rules/column_type_check.rs‎

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,13 @@ pub fn check_column_types(
2525
IrNode::CreateTable(ct) => {
2626
for col in &ct.columns {
2727
if predicate(&col.type_name) {
28-
findings.push(Finding {
29-
rule_id: rule_id.to_string(),
28+
findings.push(Finding::new(
29+
rule_id,
3030
severity,
31-
message: message_fn(&col.name, &ct.name, &col.type_name),
32-
file: ctx.file.clone(),
33-
start_line: stmt.span.start_line,
34-
end_line: stmt.span.end_line,
35-
});
31+
message_fn(&col.name, &ct.name, &col.type_name),
32+
ctx.file,
33+
&stmt.span,
34+
));
3635
}
3736
}
3837
}
@@ -41,14 +40,13 @@ pub fn check_column_types(
4140
match action {
4241
AlterTableAction::AddColumn(col) => {
4342
if predicate(&col.type_name) {
44-
findings.push(Finding {
45-
rule_id: rule_id.to_string(),
43+
findings.push(Finding::new(
44+
rule_id,
4645
severity,
47-
message: message_fn(&col.name, &at.name, &col.type_name),
48-
file: ctx.file.clone(),
49-
start_line: stmt.span.start_line,
50-
end_line: stmt.span.end_line,
51-
});
46+
message_fn(&col.name, &at.name, &col.type_name),
47+
ctx.file,
48+
&stmt.span,
49+
));
5250
}
5351
}
5452
AlterTableAction::AlterColumnType {
@@ -57,14 +55,13 @@ pub fn check_column_types(
5755
..
5856
} => {
5957
if predicate(new_type) {
60-
findings.push(Finding {
61-
rule_id: rule_id.to_string(),
58+
findings.push(Finding::new(
59+
rule_id,
6260
severity,
63-
message: message_fn(column_name, &at.name, new_type),
64-
file: ctx.file.clone(),
65-
start_line: stmt.span.start_line,
66-
end_line: stmt.span.end_line,
67-
});
61+
message_fn(column_name, &at.name, new_type),
62+
ctx.file,
63+
&stmt.span,
64+
));
6865
}
6966
}
7067
_ => {}

‎src/rules/mod.rs‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ pub mod pgm104;
2525
pub mod pgm105;
2626

2727
use crate::catalog::Catalog;
28-
use crate::parser::ir::{IrNode, Located};
28+
use crate::parser::ir::{IrNode, Located, SourceSpan};
2929
use std::collections::HashSet;
3030
use std::fmt;
31-
use std::path::PathBuf;
31+
use std::path::{Path, PathBuf};
3232

3333
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
3434
pub enum Severity {
@@ -86,6 +86,26 @@ pub struct Finding {
8686
pub end_line: usize,
8787
}
8888

89+
impl Finding {
90+
/// Create a finding from a rule, lint context, source span, and message.
91+
pub fn new(
92+
rule_id: &str,
93+
severity: Severity,
94+
message: String,
95+
file: &Path,
96+
span: &SourceSpan,
97+
) -> Self {
98+
Self {
99+
rule_id: rule_id.to_string(),
100+
severity,
101+
message,
102+
file: file.to_path_buf(),
103+
start_line: span.start_line,
104+
end_line: span.end_line,
105+
}
106+
}
107+
}
108+
89109
/// Context available to rules during linting.
90110
pub struct LintContext<'a> {
91111
/// The catalog state BEFORE the current unit was applied.
@@ -110,7 +130,16 @@ pub struct LintContext<'a> {
110130
pub is_down: bool,
111131

112132
/// The source file being linted.
113-
pub file: &'a PathBuf,
133+
pub file: &'a Path,
134+
}
135+
136+
impl<'a> LintContext<'a> {
137+
/// Check if a table existed before this change and was not created in the
138+
/// current set of changed files.
139+
pub fn is_existing_table(&self, table_key: &str) -> bool {
140+
self.catalog_before.has_table(table_key)
141+
&& !self.tables_created_in_change.contains(table_key)
142+
}
114143
}
115144

116145
/// Trait that every rule implements.

‎src/rules/pgm001.rs‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -65,21 +65,18 @@ impl Rule for Pgm001 {
6565

6666
// Only flag if table exists in catalog_before (pre-existing)
6767
// AND was not created in the current set of changed files.
68-
if ctx.catalog_before.has_table(table_key)
69-
&& !ctx.tables_created_in_change.contains(table_key)
70-
{
71-
findings.push(Finding {
72-
rule_id: self.id().to_string(),
73-
severity: self.default_severity(),
74-
message: format!(
68+
if ctx.is_existing_table(table_key) {
69+
findings.push(Finding::new(
70+
self.id(),
71+
self.default_severity(),
72+
format!(
7573
"CREATE INDEX on existing table '{}' should use CONCURRENTLY \
7674
to avoid holding an exclusive lock.",
7775
ci.table_name
7876
),
79-
file: ctx.file.clone(),
80-
start_line: stmt.span.start_line,
81-
end_line: stmt.span.end_line,
82-
});
77+
ctx.file,
78+
&stmt.span,
79+
));
8380
}
8481
}
8582
}

‎src/rules/pgm002.rs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -67,16 +67,15 @@ impl Rule for Pgm002 {
6767
});
6868

6969
if belongs_to_existing_table {
70-
findings.push(Finding {
71-
rule_id: self.id().to_string(),
72-
severity: self.default_severity(),
73-
message: "DROP INDEX on existing table should use CONCURRENTLY \
70+
findings.push(Finding::new(
71+
self.id(),
72+
self.default_severity(),
73+
"DROP INDEX on existing table should use CONCURRENTLY \
7474
to avoid holding an exclusive lock."
7575
.to_string(),
76-
file: ctx.file.clone(),
77-
start_line: stmt.span.start_line,
78-
end_line: stmt.span.end_line,
79-
});
76+
ctx.file,
77+
&stmt.span,
78+
));
8079
}
8180
}
8281
}

‎src/rules/pgm003.rs‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
//! deletes and updates on the referenced table cause sequential scans on
66
//! the referencing table, leading to severe performance degradation.
77
8-
use crate::parser::ir::{IrNode, Located, TableConstraint};
8+
use crate::parser::ir::{IrNode, Located, SourceSpan, TableConstraint};
99
use crate::rules::{Finding, LintContext, Rule, Severity};
1010

1111
/// Rule that flags foreign keys without a covering index on the referencing table.
@@ -16,8 +16,7 @@ pub struct Pgm003;
1616
struct FkInfo {
1717
table_name: String,
1818
columns: Vec<String>,
19-
start_line: usize,
20-
end_line: usize,
19+
span: SourceSpan,
2120
}
2221

2322
impl Rule for Pgm003 {
@@ -80,8 +79,7 @@ impl Rule for Pgm003 {
8079
fks.push(FkInfo {
8180
table_name: ct.name.catalog_key().to_string(),
8281
columns: columns.clone(),
83-
start_line: stmt.span.start_line,
84-
end_line: stmt.span.end_line,
82+
span: stmt.span.clone(),
8583
});
8684
}
8785
}
@@ -98,8 +96,7 @@ impl Rule for Pgm003 {
9896
fks.push(FkInfo {
9997
table_name: at.name.catalog_key().to_string(),
10098
columns: columns.clone(),
101-
start_line: stmt.span.start_line,
102-
end_line: stmt.span.end_line,
99+
span: stmt.span.clone(),
103100
});
104101
}
105102
}
@@ -119,20 +116,19 @@ impl Rule for Pgm003 {
119116

120117
if !has_index {
121118
let cols_display = fk.columns.join(", ");
122-
findings.push(Finding {
123-
rule_id: self.id().to_string(),
124-
severity: self.default_severity(),
125-
message: format!(
119+
findings.push(Finding::new(
120+
self.id(),
121+
self.default_severity(),
122+
format!(
126123
"Foreign key on '{table}({cols})' has no covering index. \
127124
Sequential scans on the referencing table during deletes/updates \
128125
on the referenced table will cause performance issues.",
129126
table = fk.table_name,
130127
cols = cols_display,
131128
),
132-
file: ctx.file.clone(),
133-
start_line: fk.start_line,
134-
end_line: fk.end_line,
135-
});
129+
ctx.file,
130+
&fk.span,
131+
));
136132
}
137133
}
138134

‎src/rules/pgm004.rs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,13 @@ impl Rule for Pgm004 {
7777
.unwrap_or(false);
7878

7979
if !has_unique_not_null {
80-
findings.push(Finding {
81-
rule_id: self.id().to_string(),
82-
severity: self.default_severity(),
83-
message: format!("Table '{}' has no primary key.", ct.name),
84-
file: ctx.file.clone(),
85-
start_line: stmt.span.start_line,
86-
end_line: stmt.span.end_line,
87-
});
80+
findings.push(Finding::new(
81+
self.id(),
82+
self.default_severity(),
83+
format!("Table '{}' has no primary key.", ct.name),
84+
ctx.file,
85+
&stmt.span,
86+
));
8887
}
8988
}
9089
}

‎src/rules/pgm005.rs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,19 +75,18 @@ impl Rule for Pgm005 {
7575
.unwrap_or(false);
7676

7777
if has_unique_not_null {
78-
findings.push(Finding {
79-
rule_id: self.id().to_string(),
80-
severity: self.default_severity(),
81-
message: format!(
78+
findings.push(Finding::new(
79+
self.id(),
80+
self.default_severity(),
81+
format!(
8282
"Table '{}' uses UNIQUE NOT NULL instead of PRIMARY KEY. \
8383
Functionally equivalent but PRIMARY KEY is conventional \
8484
and more explicit.",
8585
ct.name
8686
),
87-
file: ctx.file.clone(),
88-
start_line: stmt.span.start_line,
89-
end_line: stmt.span.end_line,
90-
});
87+
ctx.file,
88+
&stmt.span,
89+
));
9190
}
9291
}
9392
}

‎src/rules/pgm006.rs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -66,17 +66,16 @@ impl Rule for Pgm006 {
6666
};
6767

6868
if is_concurrent {
69-
findings.push(Finding {
70-
rule_id: self.id().to_string(),
71-
severity: self.default_severity(),
72-
message: "CONCURRENTLY cannot run inside a transaction. \
69+
findings.push(Finding::new(
70+
self.id(),
71+
self.default_severity(),
72+
"CONCURRENTLY cannot run inside a transaction. \
7373
Set runInTransaction=\"false\" (Liquibase) or disable \
7474
transactions for this migration."
7575
.to_string(),
76-
file: ctx.file.clone(),
77-
start_line: stmt.span.start_line,
78-
end_line: stmt.span.end_line,
79-
});
76+
ctx.file,
77+
&stmt.span,
78+
));
8079
}
8180
}
8281

0 commit comments

Comments
 (0)