Skip to content

Commit f16a20a

Browse files
authored
feat(rules): repeated JSON parse rule and allow_exceptions setting (#936)
* feat(rules): report a JSON payload parsed more than once in one SELECT SQBRSQL045 reports the same JSON parse function applied to the same column more than once in one SELECT scope, at the second call, and names the count, for example `TRY_PARSE_JSON(e.value) is parsed 3 times in one SELECT`. Parse the payload once in the CTE that reads it and read every field from that column. Each SELECT is its own scope; nested SELECTs, subqueries, CTE bodies and set-operation branches are checked separately. Calls are keyed by function, table qualifier and column (case-insensitive) through parentheses, LOWER, UPPER, TRIM, CAST, TRY_CAST and the first COALESCE argument. Parse functions: Snowflake and Databricks PARSE_JSON and TRY_PARSE_JSON, BigQuery PARSE_JSON and SAFE.PARSE_JSON, DuckDB and MotherDuck JSON. PostgreSQL and SQL Server have no single-argument equivalent. The check is native and walks each body once. Refs CHI-488. * feat(rules): add [rules] allow_exceptions to forbid Rule escape hatches With `[rules] allow_exceptions = false` (default true), any [[rules.rule_exceptions]] or [[rules.rule_ignores]] entry fails rules configuration loading, so compile and rules run stop with an error that counts the entries, states the current value and shows the exact TOML to allow them again. Each inline `-- sqb: ignore` directive becomes a SQBRSQL000 error at the directive and suppresses nothing, so the hidden finding is reported too. The project-wide `ignore` list stays allowed. Document inline suppressions and the new setting in the Rules and project configuration references. Refs CHI-489. * test(rules): include SQBRSQL045 in the unevaluated-rules E2E code list
1 parent 6d35706 commit f16a20a

40 files changed

Lines changed: 1405 additions & 23 deletions

File tree

‎crates/sqlbuild-rules-native/src/configuration/_helpers/loading.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ fn validate_raw(value: &toml::Value) -> Result<(), String> {
5151
"rule_exceptions",
5252
"graph_edge_exceptions",
5353
"rule_ignores",
54+
"allow_exceptions",
5455
"select_star_allow",
5556
"domains",
5657
"approved_source_tokens",

‎crates/sqlbuild-rules-native/src/models.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ pub(crate) struct RulesConfig {
145145
pub rule_exceptions: Vec<RuleException>,
146146
pub graph_edge_exceptions: Vec<GraphEdgeException>,
147147
pub rule_ignores: Vec<RuleIgnore>,
148+
pub allow_exceptions: bool,
148149
pub select_star_allow: Vec<SelectStarAllow>,
149150
pub domains: Vec<String>,
150151
pub approved_source_tokens: Vec<String>,
@@ -165,6 +166,7 @@ impl Default for RulesConfig {
165166
rule_exceptions: vec![],
166167
graph_edge_exceptions: vec![],
167168
rule_ignores: vec![],
169+
allow_exceptions: true,
168170
select_star_allow: vec![],
169171
domains: vec![],
170172
approved_source_tokens: vec![],

‎crates/sqlbuild-rules-native/src/sql_lint/_helpers/engine.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ const DEFAULT_RULES: [&str; 13] = [
257257
INLINE_QUERY_RELATION.code,
258258
];
259259

260-
const ALL_RULE_METADATA: [&LintRuleMetadata; 44] = [
260+
const ALL_RULE_METADATA: [&LintRuleMetadata; 45] = [
261261
&NULL_COMPARISON,
262262
&IMPLICIT_CARTESIAN_JOIN,
263263
&JOIN_WITHOUT_CONDITION,
@@ -302,6 +302,7 @@ const ALL_RULE_METADATA: [&LintRuleMetadata; 44] = [
302302
&crate::sql_quality::constants::UNUSED_CTE_OUTPUT,
303303
&crate::sql_quality::constants::RANKING_SORT_CAP,
304304
&crate::sql_quality::constants::LONG_LITERAL,
305+
&crate::sql_quality::constants::REPEATED_JSON_PARSE,
305306
];
306307

307308
fn is_ceremonial_cte_name(name: &str) -> bool {
@@ -442,6 +443,7 @@ pub(crate) fn lint(request: LintRequest) -> Result<LintResponse, String> {
442443
&crate::sql_quality::models::QualityRequest {
443444
statements: &statements,
444445
tokens: &tokens,
446+
dialect: dialect_type,
445447
enabled: &enabled,
446448
max_literal_length: request
447449
.max_literal_length
Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
//! SQBRSQL045: the same JSON payload column parsed more than once in one SELECT scope.
2+
3+
use std::borrow::Cow;
4+
use std::collections::HashMap;
5+
6+
use polyglot_sql::expressions::{Column, Identifier};
7+
use polyglot_sql::scope::walk_in_scope;
8+
use polyglot_sql::tokens::Token;
9+
use polyglot_sql::{DialectType, Expression, ExpressionWalk};
10+
11+
use crate::sql_lint::models::LintDiagnostic;
12+
use crate::sql_quality::constants::REPEATED_JSON_PARSE;
13+
use crate::sql_quality::models::QualityRequest;
14+
use crate::sql_quality::syntax::{is_trivia, lower};
15+
16+
/// Significant tokens searched backwards from the column for the function name.
17+
const MAX_FUNCTION_NAME_DISTANCE: usize = 32;
18+
const PARSE_ONCE_FIX_REFUSAL: &str =
19+
"Parsing the payload once requires naming a column in the CTE that reads it";
20+
21+
/// An identifier normalised the way the dialect resolves it.
22+
type IdentifierKey = String;
23+
24+
/// One JSON parse call whose argument resolves to a plain column.
25+
struct ParseCall<'a> {
26+
function: &'static str,
27+
column: &'a Column,
28+
column_start: usize,
29+
}
30+
31+
pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Vec<LintDiagnostic> {
32+
let functions: &[&'static str] = parse_functions(request.dialect);
33+
if functions.is_empty() {
34+
return Vec::new();
35+
}
36+
let mut found: Vec<LintDiagnostic> = Vec::new();
37+
for statement in request.statements {
38+
for node in statement.dfs() {
39+
if matches!(node, Expression::Select(_)) {
40+
found.extend(scope_diagnostics(
41+
request.tokens,
42+
node,
43+
functions,
44+
request.dialect,
45+
));
46+
}
47+
}
48+
}
49+
found
50+
}
51+
52+
/// JSON parse functions that take one string argument, by dialect, in upper case.
53+
fn parse_functions(dialect: DialectType) -> &'static [&'static str] {
54+
match dialect {
55+
DialectType::Snowflake | DialectType::Databricks => &["PARSE_JSON", "TRY_PARSE_JSON"],
56+
DialectType::BigQuery => &["PARSE_JSON", "SAFE.PARSE_JSON"],
57+
DialectType::DuckDB => &["JSON"],
58+
_ => &[],
59+
}
60+
}
61+
62+
fn scope_diagnostics(
63+
tokens: &[Token],
64+
select: &Expression,
65+
functions: &[&'static str],
66+
dialect: DialectType,
67+
) -> Vec<LintDiagnostic> {
68+
let quote: char = identifier_quote(dialect);
69+
let mut calls: Vec<ParseCall<'_>> = walk_in_scope(select, false)
70+
.filter_map(|node| parse_call(node, functions))
71+
.collect();
72+
calls.sort_by_key(|call| call.column_start);
73+
let mut groups: HashMap<
74+
(&'static str, Option<IdentifierKey>, IdentifierKey),
75+
Vec<&ParseCall<'_>>,
76+
> = HashMap::new();
77+
for call in &calls {
78+
let qualifier: Option<IdentifierKey> = call
79+
.column
80+
.table
81+
.as_ref()
82+
.map(|table| identifier_key(table, dialect));
83+
groups
84+
.entry((
85+
call.function,
86+
qualifier,
87+
identifier_key(&call.column.name, dialect),
88+
))
89+
.or_default()
90+
.push(call);
91+
}
92+
let mut found: Vec<LintDiagnostic> = Vec::new();
93+
for group in groups.values().filter(|group| group.len() > 1) {
94+
let second: &ParseCall<'_> = group[1];
95+
let (start, end) = function_span(tokens, second);
96+
found.push(LintDiagnostic {
97+
code: REPEATED_JSON_PARSE.code,
98+
message: REPEATED_JSON_PARSE.message,
99+
remediation: Cow::Owned(remediation(second, group.len(), quote)),
100+
start,
101+
end,
102+
fix: None,
103+
fix_unavailable_reason: Some(PARSE_ONCE_FIX_REFUSAL),
104+
});
105+
}
106+
found
107+
}
108+
109+
/// Normalises a name the way the dialect resolves it (Snowflake folds unquoted names upward).
110+
fn identifier_key(identifier: &Identifier, dialect: DialectType) -> IdentifierKey {
111+
match dialect {
112+
DialectType::Snowflake if identifier.quoted => identifier.name.clone(),
113+
DialectType::Snowflake => identifier.name.to_uppercase(),
114+
_ => lower(&identifier.name),
115+
}
116+
}
117+
118+
fn parse_call<'a>(node: &'a Expression, functions: &[&'static str]) -> Option<ParseCall<'a>> {
119+
let (name, argument): (&str, &Expression) = match node {
120+
Expression::ParseJson(function) => ("PARSE_JSON", &function.this),
121+
Expression::Function(function) if function.args.len() == 1 => {
122+
(function.name.as_str(), &function.args[0])
123+
}
124+
_ => return None,
125+
};
126+
let function: &'static str = functions
127+
.iter()
128+
.copied()
129+
.find(|candidate| candidate.eq_ignore_ascii_case(name))?;
130+
let column: &Column = unwrap_column(argument)?;
131+
let column_start: usize = column.span.or(column.name.span)?.start;
132+
Some(ParseCall {
133+
function,
134+
column,
135+
column_start,
136+
})
137+
}
138+
139+
/// The column under wrappers that keep the parsed value tied to that column.
140+
fn unwrap_column(expression: &Expression) -> Option<&Column> {
141+
let mut current: &Expression = expression;
142+
loop {
143+
current = match current {
144+
Expression::Column(column) => return Some(column),
145+
Expression::Paren(paren) => &paren.this,
146+
Expression::Lower(function) | Expression::Upper(function) => &function.this,
147+
Expression::Trim(trim) if trim.characters.is_none() => &trim.this,
148+
Expression::Cast(cast) | Expression::TryCast(cast) | Expression::SafeCast(cast) => {
149+
&cast.this
150+
}
151+
Expression::Coalesce(coalesce) if coalesce.expressions.len() == 1 => {
152+
&coalesce.expressions[0]
153+
}
154+
_ => return None,
155+
};
156+
}
157+
}
158+
159+
/// Span of the function-name token that opens `call`, or of its column when not found.
160+
fn function_span(tokens: &[Token], call: &ParseCall<'_>) -> (usize, usize) {
161+
let bare: &str = call.function.rsplit('.').next().unwrap_or(call.function);
162+
let before: usize = tokens.partition_point(|token| token.span.start < call.column_start);
163+
tokens[..before]
164+
.iter()
165+
.rev()
166+
.filter(|token| !is_trivia(token))
167+
.take(MAX_FUNCTION_NAME_DISTANCE)
168+
.find(|token| token.text.eq_ignore_ascii_case(bare))
169+
.map_or((call.column_start, call.column_start), |token| {
170+
(token.span.start, token.span.end)
171+
})
172+
}
173+
174+
fn remediation(call: &ParseCall<'_>, count: usize, quote: char) -> String {
175+
let name: String = written(&call.column.name, quote);
176+
let column: String = call.column.table.as_ref().map_or_else(
177+
|| name.clone(),
178+
|table| format!("{}.{name}", written(table, quote)),
179+
);
180+
let function: &str = call.function;
181+
format!(
182+
"{function}({column}) is parsed {count} times in one SELECT. Parse it once in the CTE that reads it, for example `{function}({name}) AS payload`, and read every field from that column in a later step."
183+
)
184+
}
185+
186+
/// The identifier as authored, re-quoted with the dialect's identifier quote when it was quoted.
187+
fn written(identifier: &Identifier, quote: char) -> String {
188+
if identifier.quoted {
189+
let escaped: String = identifier.name.replace(quote, &format!("{quote}{quote}"));
190+
format!("{quote}{escaped}{quote}")
191+
} else {
192+
identifier.name.clone()
193+
}
194+
}
195+
196+
fn identifier_quote(dialect: DialectType) -> char {
197+
match dialect {
198+
DialectType::BigQuery | DialectType::Databricks => '`',
199+
_ => '"',
200+
}
201+
}

‎crates/sqlbuild-rules-native/src/sql_quality/constants.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ pub(crate) const LONG_LITERAL: LintRuleMetadata = LintRuleMetadata {
1515
message: "String literal is longer than max_literal_length",
1616
remediation: "Define the value once as a constant and reference it with `@const`, or shorten it.",
1717
};
18+
pub(crate) const REPEATED_JSON_PARSE: LintRuleMetadata = LintRuleMetadata {
19+
code: "SQBRSQL045",
20+
message: "JSON payload is parsed more than once in one SELECT",
21+
remediation: "Parse the payload once in the CTE that reads it, for example `TRY_PARSE_JSON(value) AS payload`, and read every field from that column in a later step.",
22+
};
1823
pub(crate) const UNSTABLE_ROW_NUMBER_CODE: &str = "SQBRSQL018";
1924
pub(crate) const UNPROVEN_RANKING_MESSAGE: &str =
2025
"Ranking window order is not proven unique within its partition";

‎crates/sqlbuild-rules-native/src/sql_quality/main/diagnostics.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
use crate::sql_lint::models::LintDiagnostic;
22
use crate::sql_quality::constants::{
3-
LONG_LITERAL, RANKING_SORT_CAP, UNSTABLE_ROW_NUMBER_CODE, UNUSED_CTE_OUTPUT,
3+
LONG_LITERAL, RANKING_SORT_CAP, REPEATED_JSON_PARSE, UNSTABLE_ROW_NUMBER_CODE,
4+
UNUSED_CTE_OUTPUT,
45
};
56
use crate::sql_quality::keys::{KeyEnvironment, key_environment};
67
use crate::sql_quality::models::QualityRequest;
7-
use crate::sql_quality::{cte_columns, literals, ranking};
8+
use crate::sql_quality::{cte_columns, json_parses, literals, ranking};
89

910
/// Run every enabled SQL quality Rule over one parsed body.
1011
pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Result<Vec<LintDiagnostic>, String> {
@@ -15,6 +16,9 @@ pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Result<Vec<LintDiagno
1516
if request.enabled.contains(LONG_LITERAL.code) {
1617
found.extend(literals::diagnostics(request));
1718
}
19+
if request.enabled.contains(REPEATED_JSON_PARSE.code) {
20+
found.extend(json_parses::diagnostics(request));
21+
}
1822
if request.enabled.contains(UNUSED_CTE_OUTPUT.code) {
1923
found.extend(cte_columns::diagnostics(request));
2024
}

‎crates/sqlbuild-rules-native/src/sql_quality/mod.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
pub(crate) mod constants;
44
#[path = "_helpers/cte_columns.rs"]
55
mod cte_columns;
6+
#[path = "_helpers/json_parses.rs"]
7+
mod json_parses;
68
#[path = "_helpers/keys.rs"]
79
mod keys;
810
#[path = "_helpers/literals.rs"]

‎crates/sqlbuild-rules-native/src/sql_quality/models.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
use std::collections::{HashMap, HashSet};
22

3-
use polyglot_sql::Expression;
43
use polyglot_sql::tokens::Token;
4+
use polyglot_sql::{DialectType, Expression};
55

66
/// Inputs for one body's quality Rules, borrowed from the native lint request.
77
#[derive(Debug)]
88
pub(crate) struct QualityRequest<'a> {
99
pub(crate) statements: &'a [Expression],
1010
pub(crate) tokens: &'a [Token],
11+
pub(crate) dialect: DialectType,
1112
pub(crate) enabled: &'a HashSet<String>,
1213
pub(crate) max_literal_length: usize,
1314
pub(crate) max_ranking_order_by: usize,

‎crates/sqlbuild-rules-native/src/sql_quality/tests.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#[path = "tests/helpers.rs"]
22
mod helpers;
3+
#[path = "tests/test_json_parses.rs"]
4+
mod json_parses;
35
#[path = "tests/test_quality_rules.rs"]
46
mod quality_rules;
57
#[path = "tests/test_quality_scale.rs"]

‎crates/sqlbuild-rules-native/src/sql_quality/tests/helpers.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,30 @@ pub(crate) fn lint(test_case: &QualityRuleTestCase) -> Result<Vec<Value>, String
2323
.unwrap_or_default())
2424
}
2525

26+
/// Lint `sql` in `dialect` with only `rule` enabled.
27+
pub(crate) fn lint_dialect(sql: &str, dialect: &str, rule: &str) -> Result<Vec<Value>, String> {
28+
let request: Value = json!({
29+
"version": 1,
30+
"sql": sql,
31+
"dialect": dialect,
32+
"enabled_rules": [rule],
33+
});
34+
let response: Value = serde_json::from_str(&lint_json(&request.to_string())?)
35+
.map_err(|error| error.to_string())?;
36+
Ok(response["diagnostics"]
37+
.as_array()
38+
.cloned()
39+
.unwrap_or_default())
40+
}
41+
42+
/// One DuckDB SELECT that parses each of `column_count` payload columns twice.
43+
pub(crate) fn wide_json_select(column_count: usize) -> String {
44+
let projections: Vec<String> = (0..column_count * 2)
45+
.map(|index| format!("json(e.c{})->>'f{index}' AS f{index}", index % column_count))
46+
.collect();
47+
format!("SELECT {} FROM events AS e", projections.join(", "))
48+
}
49+
2650
pub(crate) fn anchors(sql: &str, diagnostics: &[Value]) -> Vec<String> {
2751
diagnostics
2852
.iter()

0 commit comments

Comments
 (0)