From 286b350ad7da06d8dc83287397fdd05e84be94ba Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 4 Oct 2026 10:01:06 +0100 Subject: [PATCH 1/3] 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. --- .../src/sql_lint/_helpers/engine.rs | 4 +- .../src/sql_quality/_helpers/json_parses.rs | 201 +++++++++++++ .../src/sql_quality/constants.rs | 5 + .../src/sql_quality/main/diagnostics.rs | 8 +- .../src/sql_quality/mod.rs | 2 + .../src/sql_quality/models.rs | 3 +- .../src/sql_quality/tests.rs | 2 + .../src/sql_quality/tests/helpers.rs | 24 ++ .../src/sql_quality/tests/test_json_parses.rs | 282 ++++++++++++++++++ .../src/sql_quality/tests/test_types.rs | 15 + .../rules/configuration-and-selection.md | 56 +++- .../sqlbuild/cli/commands/main/_test_types.py | 13 + .../src/sqlbuild/cli/commands/main/helpers.py | 22 ++ .../commands/main/test_repeated_json_parse.py | 163 ++++++++++ .../_helpers/engine/test_catalogue.py | 2 +- .../rules/configuration-and-selection.mdx | 55 +++- 16 files changed, 846 insertions(+), 11 deletions(-) create mode 100644 crates/sqlbuild-rules-native/src/sql_quality/_helpers/json_parses.rs create mode 100644 crates/sqlbuild-rules-native/src/sql_quality/tests/test_json_parses.rs create mode 100644 tests/integration/src/sqlbuild/cli/commands/main/test_repeated_json_parse.py diff --git a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/engine.rs b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/engine.rs index 7669306fe9..8e851b00bf 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/engine.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/engine.rs @@ -257,7 +257,7 @@ const DEFAULT_RULES: [&str; 13] = [ INLINE_QUERY_RELATION.code, ]; -const ALL_RULE_METADATA: [&LintRuleMetadata; 44] = [ +const ALL_RULE_METADATA: [&LintRuleMetadata; 45] = [ &NULL_COMPARISON, &IMPLICIT_CARTESIAN_JOIN, &JOIN_WITHOUT_CONDITION, @@ -302,6 +302,7 @@ const ALL_RULE_METADATA: [&LintRuleMetadata; 44] = [ &crate::sql_quality::constants::UNUSED_CTE_OUTPUT, &crate::sql_quality::constants::RANKING_SORT_CAP, &crate::sql_quality::constants::LONG_LITERAL, + &crate::sql_quality::constants::REPEATED_JSON_PARSE, ]; fn is_ceremonial_cte_name(name: &str) -> bool { @@ -442,6 +443,7 @@ pub(crate) fn lint(request: LintRequest) -> Result { &crate::sql_quality::models::QualityRequest { statements: &statements, tokens: &tokens, + dialect: dialect_type, enabled: &enabled, max_literal_length: request .max_literal_length diff --git a/crates/sqlbuild-rules-native/src/sql_quality/_helpers/json_parses.rs b/crates/sqlbuild-rules-native/src/sql_quality/_helpers/json_parses.rs new file mode 100644 index 0000000000..38228dad45 --- /dev/null +++ b/crates/sqlbuild-rules-native/src/sql_quality/_helpers/json_parses.rs @@ -0,0 +1,201 @@ +//! SQBRSQL045: the same JSON payload column parsed more than once in one SELECT scope. + +use std::borrow::Cow; +use std::collections::HashMap; + +use polyglot_sql::expressions::{Column, Identifier}; +use polyglot_sql::scope::walk_in_scope; +use polyglot_sql::tokens::Token; +use polyglot_sql::{DialectType, Expression, ExpressionWalk}; + +use crate::sql_lint::models::LintDiagnostic; +use crate::sql_quality::constants::REPEATED_JSON_PARSE; +use crate::sql_quality::models::QualityRequest; +use crate::sql_quality::syntax::{is_trivia, lower}; + +/// Significant tokens searched backwards from the column for the function name. +const MAX_FUNCTION_NAME_DISTANCE: usize = 32; +const PARSE_ONCE_FIX_REFUSAL: &str = + "Parsing the payload once requires naming a column in the CTE that reads it"; + +/// An identifier normalised the way the dialect resolves it. +type IdentifierKey = String; + +/// One JSON parse call whose argument resolves to a plain column. +struct ParseCall<'a> { + function: &'static str, + column: &'a Column, + column_start: usize, +} + +pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Vec { + let functions: &[&'static str] = parse_functions(request.dialect); + if functions.is_empty() { + return Vec::new(); + } + let mut found: Vec = Vec::new(); + for statement in request.statements { + for node in statement.dfs() { + if matches!(node, Expression::Select(_)) { + found.extend(scope_diagnostics( + request.tokens, + node, + functions, + request.dialect, + )); + } + } + } + found +} + +/// JSON parse functions that take one string argument, by dialect, in upper case. +fn parse_functions(dialect: DialectType) -> &'static [&'static str] { + match dialect { + DialectType::Snowflake | DialectType::Databricks => &["PARSE_JSON", "TRY_PARSE_JSON"], + DialectType::BigQuery => &["PARSE_JSON", "SAFE.PARSE_JSON"], + DialectType::DuckDB => &["JSON"], + _ => &[], + } +} + +fn scope_diagnostics( + tokens: &[Token], + select: &Expression, + functions: &[&'static str], + dialect: DialectType, +) -> Vec { + let quote: char = identifier_quote(dialect); + let mut calls: Vec> = walk_in_scope(select, false) + .filter_map(|node| parse_call(node, functions)) + .collect(); + calls.sort_by_key(|call| call.column_start); + let mut groups: HashMap< + (&'static str, Option, IdentifierKey), + Vec<&ParseCall<'_>>, + > = HashMap::new(); + for call in &calls { + let qualifier: Option = call + .column + .table + .as_ref() + .map(|table| identifier_key(table, dialect)); + groups + .entry(( + call.function, + qualifier, + identifier_key(&call.column.name, dialect), + )) + .or_default() + .push(call); + } + let mut found: Vec = Vec::new(); + for group in groups.values().filter(|group| group.len() > 1) { + let second: &ParseCall<'_> = group[1]; + let (start, end) = function_span(tokens, second); + found.push(LintDiagnostic { + code: REPEATED_JSON_PARSE.code, + message: REPEATED_JSON_PARSE.message, + remediation: Cow::Owned(remediation(second, group.len(), quote)), + start, + end, + fix: None, + fix_unavailable_reason: Some(PARSE_ONCE_FIX_REFUSAL), + }); + } + found +} + +/// Normalises a name the way the dialect resolves it (Snowflake folds unquoted names upward). +fn identifier_key(identifier: &Identifier, dialect: DialectType) -> IdentifierKey { + match dialect { + DialectType::Snowflake if identifier.quoted => identifier.name.clone(), + DialectType::Snowflake => identifier.name.to_uppercase(), + _ => lower(&identifier.name), + } +} + +fn parse_call<'a>(node: &'a Expression, functions: &[&'static str]) -> Option> { + let (name, argument): (&str, &Expression) = match node { + Expression::ParseJson(function) => ("PARSE_JSON", &function.this), + Expression::Function(function) if function.args.len() == 1 => { + (function.name.as_str(), &function.args[0]) + } + _ => return None, + }; + let function: &'static str = functions + .iter() + .copied() + .find(|candidate| candidate.eq_ignore_ascii_case(name))?; + let column: &Column = unwrap_column(argument)?; + let column_start: usize = column.span.or(column.name.span)?.start; + Some(ParseCall { + function, + column, + column_start, + }) +} + +/// The column under wrappers that keep the parsed value tied to that column. +fn unwrap_column(expression: &Expression) -> Option<&Column> { + let mut current: &Expression = expression; + loop { + current = match current { + Expression::Column(column) => return Some(column), + Expression::Paren(paren) => &paren.this, + Expression::Lower(function) | Expression::Upper(function) => &function.this, + Expression::Trim(trim) if trim.characters.is_none() => &trim.this, + Expression::Cast(cast) | Expression::TryCast(cast) | Expression::SafeCast(cast) => { + &cast.this + } + Expression::Coalesce(coalesce) if coalesce.expressions.len() == 1 => { + &coalesce.expressions[0] + } + _ => return None, + }; + } +} + +/// Span of the function-name token that opens `call`, or of its column when not found. +fn function_span(tokens: &[Token], call: &ParseCall<'_>) -> (usize, usize) { + let bare: &str = call.function.rsplit('.').next().unwrap_or(call.function); + let before: usize = tokens.partition_point(|token| token.span.start < call.column_start); + tokens[..before] + .iter() + .rev() + .filter(|token| !is_trivia(token)) + .take(MAX_FUNCTION_NAME_DISTANCE) + .find(|token| token.text.eq_ignore_ascii_case(bare)) + .map_or((call.column_start, call.column_start), |token| { + (token.span.start, token.span.end) + }) +} + +fn remediation(call: &ParseCall<'_>, count: usize, quote: char) -> String { + let name: String = written(&call.column.name, quote); + let column: String = call.column.table.as_ref().map_or_else( + || name.clone(), + |table| format!("{}.{name}", written(table, quote)), + ); + let function: &str = call.function; + format!( + "{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." + ) +} + +/// The identifier as authored, re-quoted with the dialect's identifier quote when it was quoted. +fn written(identifier: &Identifier, quote: char) -> String { + if identifier.quoted { + let escaped: String = identifier.name.replace(quote, &format!("{quote}{quote}")); + format!("{quote}{escaped}{quote}") + } else { + identifier.name.clone() + } +} + +fn identifier_quote(dialect: DialectType) -> char { + match dialect { + DialectType::BigQuery | DialectType::Databricks => '`', + _ => '"', + } +} diff --git a/crates/sqlbuild-rules-native/src/sql_quality/constants.rs b/crates/sqlbuild-rules-native/src/sql_quality/constants.rs index 618d9afbe7..cb570ec0f7 100644 --- a/crates/sqlbuild-rules-native/src/sql_quality/constants.rs +++ b/crates/sqlbuild-rules-native/src/sql_quality/constants.rs @@ -15,6 +15,11 @@ pub(crate) const LONG_LITERAL: LintRuleMetadata = LintRuleMetadata { message: "String literal is longer than max_literal_length", remediation: "Define the value once as a constant and reference it with `@const`, or shorten it.", }; +pub(crate) const REPEATED_JSON_PARSE: LintRuleMetadata = LintRuleMetadata { + code: "SQBRSQL045", + message: "JSON payload is parsed more than once in one SELECT", + 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.", +}; pub(crate) const UNSTABLE_ROW_NUMBER_CODE: &str = "SQBRSQL018"; pub(crate) const UNPROVEN_RANKING_MESSAGE: &str = "Ranking window order is not proven unique within its partition"; diff --git a/crates/sqlbuild-rules-native/src/sql_quality/main/diagnostics.rs b/crates/sqlbuild-rules-native/src/sql_quality/main/diagnostics.rs index 510d285a08..76848ccae4 100644 --- a/crates/sqlbuild-rules-native/src/sql_quality/main/diagnostics.rs +++ b/crates/sqlbuild-rules-native/src/sql_quality/main/diagnostics.rs @@ -1,10 +1,11 @@ use crate::sql_lint::models::LintDiagnostic; use crate::sql_quality::constants::{ - LONG_LITERAL, RANKING_SORT_CAP, UNSTABLE_ROW_NUMBER_CODE, UNUSED_CTE_OUTPUT, + LONG_LITERAL, RANKING_SORT_CAP, REPEATED_JSON_PARSE, UNSTABLE_ROW_NUMBER_CODE, + UNUSED_CTE_OUTPUT, }; use crate::sql_quality::keys::{KeyEnvironment, key_environment}; use crate::sql_quality::models::QualityRequest; -use crate::sql_quality::{cte_columns, literals, ranking}; +use crate::sql_quality::{cte_columns, json_parses, literals, ranking}; /// Run every enabled SQL quality Rule over one parsed body. pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Result, String> { @@ -15,6 +16,9 @@ pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Result { pub(crate) statements: &'a [Expression], pub(crate) tokens: &'a [Token], + pub(crate) dialect: DialectType, pub(crate) enabled: &'a HashSet, pub(crate) max_literal_length: usize, pub(crate) max_ranking_order_by: usize, diff --git a/crates/sqlbuild-rules-native/src/sql_quality/tests.rs b/crates/sqlbuild-rules-native/src/sql_quality/tests.rs index 9eac086363..2643da6c21 100644 --- a/crates/sqlbuild-rules-native/src/sql_quality/tests.rs +++ b/crates/sqlbuild-rules-native/src/sql_quality/tests.rs @@ -1,5 +1,7 @@ #[path = "tests/helpers.rs"] mod helpers; +#[path = "tests/test_json_parses.rs"] +mod json_parses; #[path = "tests/test_quality_rules.rs"] mod quality_rules; #[path = "tests/test_quality_scale.rs"] diff --git a/crates/sqlbuild-rules-native/src/sql_quality/tests/helpers.rs b/crates/sqlbuild-rules-native/src/sql_quality/tests/helpers.rs index 68769c0765..3b8a43de34 100644 --- a/crates/sqlbuild-rules-native/src/sql_quality/tests/helpers.rs +++ b/crates/sqlbuild-rules-native/src/sql_quality/tests/helpers.rs @@ -23,6 +23,30 @@ pub(crate) fn lint(test_case: &QualityRuleTestCase) -> Result, String .unwrap_or_default()) } +/// Lint `sql` in `dialect` with only `rule` enabled. +pub(crate) fn lint_dialect(sql: &str, dialect: &str, rule: &str) -> Result, String> { + let request: Value = json!({ + "version": 1, + "sql": sql, + "dialect": dialect, + "enabled_rules": [rule], + }); + let response: Value = serde_json::from_str(&lint_json(&request.to_string())?) + .map_err(|error| error.to_string())?; + Ok(response["diagnostics"] + .as_array() + .cloned() + .unwrap_or_default()) +} + +/// One DuckDB SELECT that parses each of `column_count` payload columns twice. +pub(crate) fn wide_json_select(column_count: usize) -> String { + let projections: Vec = (0..column_count * 2) + .map(|index| format!("json(e.c{})->>'f{index}' AS f{index}", index % column_count)) + .collect(); + format!("SELECT {} FROM events AS e", projections.join(", ")) +} + pub(crate) fn anchors(sql: &str, diagnostics: &[Value]) -> Vec { diagnostics .iter() diff --git a/crates/sqlbuild-rules-native/src/sql_quality/tests/test_json_parses.rs b/crates/sqlbuild-rules-native/src/sql_quality/tests/test_json_parses.rs new file mode 100644 index 0000000000..29a89e6b6c --- /dev/null +++ b/crates/sqlbuild-rules-native/src/sql_quality/tests/test_json_parses.rs @@ -0,0 +1,282 @@ +use std::time::Instant; + +use serde_json::Value; + +use crate::sql_quality::tests::helpers::{anchors, lint_dialect, wide_json_select}; +use crate::sql_quality::tests::test_types::{JsonParseScaleTestCase, JsonParseTestCase}; + +const RULE: &str = "SQBRSQL045"; + +#[test] +fn given_json_parse_calls_when_linting_then_repeats_in_one_select_are_reported() +-> Result<(), String> { + let test_cases = [ + JsonParseTestCase { + description: "same column parsed twice reports the second call", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(e.value):order_id AS order_id, TRY_PARSE_JSON(e.value):status AS status FROM events AS e", + expected_anchors: &["TRY_PARSE_JSON"], + expected_details: &["TRY_PARSE_JSON(e.value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "the finding is located at the second call", + dialect: "snowflake", + sql: "SELECT try_parse_json(value):a AS a, TRY_PARSE_JSON(value):b AS b FROM events", + expected_anchors: &["TRY_PARSE_JSON"], + expected_details: &["TRY_PARSE_JSON(value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "three parses give one finding with the count", + dialect: "snowflake", + sql: "SELECT PARSE_JSON(value):a AS a, PARSE_JSON(value):b AS b FROM events WHERE PARSE_JSON(value):c IS NOT NULL", + expected_anchors: &["PARSE_JSON"], + expected_details: &["PARSE_JSON(value) is parsed 3 times in one SELECT."], + }, + JsonParseTestCase { + description: "column casing and wrappers do not hide a repeat", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(LOWER(E.Value)):a AS a, TRY_PARSE_JSON((CAST(TRIM(e.value) AS VARCHAR))):b AS b, TRY_PARSE_JSON(COALESCE(UPPER(e.VALUE))):c AS c FROM events AS e", + expected_anchors: &["TRY_PARSE_JSON"], + expected_details: &["TRY_PARSE_JSON(e.value) is parsed 3 times in one SELECT."], + }, + JsonParseTestCase { + description: "multi-argument COALESCE is not a plain column", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(COALESCE(e.value, e.backup)):a AS a, TRY_PARSE_JSON(COALESCE(e.value, e.fallback)):b AS b, TRY_PARSE_JSON(e.value):c AS c FROM events AS e", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "TRIM with characters is not a plain column", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(TRIM(e.value, ' ')):a AS a, TRY_PARSE_JSON(e.value):b AS b FROM events AS e", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "quoted names differing in case are different columns", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(e.\"Value\"):a AS a, TRY_PARSE_JSON(e.\"value\"):b AS b FROM events AS e", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "quoted and unquoted names are not merged", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(e.\"value\"):a AS a, TRY_PARSE_JSON(e.value):b AS b FROM events AS e", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "quoted qualifiers differing in case are different relations", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(\"E\".value):a AS a, TRY_PARSE_JSON(\"e\".value):b AS b FROM events AS \"E\" INNER JOIN events AS \"e\" ON \"E\".id = \"e\".id", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "identical quoted names are reported as written", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(\"E\".\"Value\"):a AS a, TRY_PARSE_JSON(\"E\".\"Value\"):b AS b FROM events AS \"E\"", + expected_anchors: &["TRY_PARSE_JSON"], + expected_details: &["TRY_PARSE_JSON(\"E\".\"Value\") is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "bigquery quoted names keep backticks", + dialect: "bigquery", + sql: "SELECT JSON_VALUE(PARSE_JSON(e.`Value`), '$.a') AS a, JSON_VALUE(PARSE_JSON(e.`Value`), '$.b') AS b FROM events AS e", + expected_anchors: &["PARSE_JSON"], + expected_details: &["PARSE_JSON(e.`Value`) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "snowflake quoted upper case matches the folded unquoted name", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(e.\"VALUE\"):a AS a, TRY_PARSE_JSON(e.value):b AS b FROM events AS e", + expected_anchors: &["TRY_PARSE_JSON"], + expected_details: &["TRY_PARSE_JSON(e.value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "duckdb quoted names are case-insensitive", + dialect: "duckdb", + sql: "SELECT json(e.\"Value\") -> 'a' AS a, json(e.value) -> 'b' AS b FROM events AS e", + expected_anchors: &["json"], + expected_details: &["JSON(e.value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "bigquery backticked names are case-insensitive", + dialect: "bigquery", + sql: "SELECT JSON_VALUE(PARSE_JSON(e.`Value`), '$.a') AS a, JSON_VALUE(PARSE_JSON(e.value), '$.b') AS b FROM events AS e", + expected_anchors: &["PARSE_JSON"], + expected_details: &["PARSE_JSON(e.value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "databricks backticked names are case-insensitive", + dialect: "databricks", + sql: "SELECT try_parse_json(e.`v`) AS a, try_parse_json(e.V) AS b FROM events AS e", + expected_anchors: &["try_parse_json"], + expected_details: &["TRY_PARSE_JSON(e.V) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "references are grouped as written", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(value):a AS a, TRY_PARSE_JSON(e.value):b AS b, TRY_PARSE_JSON(events.value):c AS c FROM events AS e", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "different columns pass", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(e.payload):a AS a, TRY_PARSE_JSON(e.metadata):b AS b FROM events AS e", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "same column name on different tables passes", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(o.payload):a AS a, TRY_PARSE_JSON(c.payload):b AS b FROM orders AS o INNER JOIN customers AS c ON o.customer_id = c.customer_id", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "different parse functions are not merged", + dialect: "snowflake", + sql: "SELECT PARSE_JSON(value):a AS a, TRY_PARSE_JSON(value):b AS b FROM events", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "one parse per CTE scope passes", + dialect: "snowflake", + sql: "WITH parsed AS (SELECT TRY_PARSE_JSON(value) AS payload FROM events), final AS (SELECT TRY_PARSE_JSON(value) AS payload FROM events) SELECT payload FROM final", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "nested SELECT is its own scope", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(e.value):a AS a, (SELECT MAX(TRY_PARSE_JSON(x.value):b) FROM events AS x) AS b FROM events AS e", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "repeat inside a subquery is reported in that scope", + dialect: "snowflake", + sql: "SELECT e.order_id FROM events AS e WHERE e.order_id IN (SELECT TRY_PARSE_JSON(value):id FROM events WHERE TRY_PARSE_JSON(value):ok)", + expected_anchors: &["TRY_PARSE_JSON"], + expected_details: &["TRY_PARSE_JSON(value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "union branches are separate scopes", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(value) AS payload FROM events UNION ALL SELECT TRY_PARSE_JSON(value) AS payload FROM events", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "a repeat inside one CTE body is reported", + dialect: "snowflake", + sql: "WITH parsed AS (SELECT TRY_PARSE_JSON(value):a AS a, TRY_PARSE_JSON(value):b AS b FROM events) SELECT a, b FROM parsed", + expected_anchors: &["TRY_PARSE_JSON"], + expected_details: &["TRY_PARSE_JSON(value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "non-column arguments pass", + dialect: "snowflake", + sql: "SELECT TRY_PARSE_JSON(value || '') AS a, TRY_PARSE_JSON(value || '') AS b FROM events", + expected_anchors: &[], + expected_details: &[], + }, + JsonParseTestCase { + description: "bigquery safe parse is matched", + dialect: "bigquery", + sql: "SELECT JSON_VALUE(SAFE.PARSE_JSON(e.value), '$.a') AS a, JSON_VALUE(SAFE.PARSE_JSON(e.value), '$.b') AS b FROM events AS e", + expected_anchors: &["PARSE_JSON"], + expected_details: &["SAFE.PARSE_JSON(e.value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "databricks try parse is matched", + dialect: "databricks", + sql: "SELECT try_parse_json(value) AS a, try_parse_json(value) AS b FROM events", + expected_anchors: &["try_parse_json"], + expected_details: &["TRY_PARSE_JSON(value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "duckdb json is matched", + dialect: "duckdb", + sql: "SELECT json(e.value)->>'a' AS a, json(e.value)->>'b' AS b FROM events AS e", + expected_anchors: &["json"], + expected_details: &["JSON(e.value) is parsed 2 times in one SELECT."], + }, + JsonParseTestCase { + description: "dialects without an equivalent are not checked", + dialect: "postgres", + sql: "SELECT CAST(value AS JSONB) AS a, CAST(value AS JSONB) AS b FROM events", + expected_anchors: &[], + expected_details: &[], + }, + ]; + for test_case in &test_cases { + let diagnostics: Vec = lint_dialect(test_case.sql, test_case.dialect, RULE)?; + assert_eq!( + anchors(test_case.sql, &diagnostics), + test_case.expected_anchors, + "{}", + test_case.description + ); + let details: Vec = diagnostics + .iter() + .map(|diagnostic| { + diagnostic["remediation"] + .as_str() + .unwrap_or_default() + .split_inclusive(". ") + .next() + .unwrap_or_default() + .trim_end() + .to_owned() + }) + .collect(); + assert_eq!( + details, test_case.expected_details, + "{}", + test_case.description + ); + assert!( + diagnostics + .iter() + .all(|diagnostic| diagnostic["fix"].is_null()), + "{}", + test_case.description + ); + } + Ok(()) +} + +#[test] +fn given_wide_select_with_many_repeated_parses_when_linting_then_it_stays_linear() +-> Result<(), String> { + let test_cases = [JsonParseScaleTestCase { + description: "3,000 projections parsing 1,500 payload columns twice each", + column_count: 1_500, + expected_findings: 1_500, + expected_max_seconds: 10.0, + }]; + for test_case in &test_cases { + let started: Instant = Instant::now(); + let diagnostics: Vec = + lint_dialect(&wide_json_select(test_case.column_count), "duckdb", RULE)?; + let elapsed: f64 = started.elapsed().as_secs_f64(); + assert_eq!( + diagnostics.len(), + test_case.expected_findings, + "{}", + test_case.description + ); + assert!( + elapsed <= test_case.expected_max_seconds, + "{}: {elapsed:.2}s", + test_case.description + ); + } + Ok(()) +} diff --git a/crates/sqlbuild-rules-native/src/sql_quality/tests/test_types.rs b/crates/sqlbuild-rules-native/src/sql_quality/tests/test_types.rs index d37c791c18..bc80b426ff 100644 --- a/crates/sqlbuild-rules-native/src/sql_quality/tests/test_types.rs +++ b/crates/sqlbuild-rules-native/src/sql_quality/tests/test_types.rs @@ -21,3 +21,18 @@ pub(crate) struct QualityRemediationTestCase { pub(crate) rule: &'static str, pub(crate) expected_fragments: &'static [&'static str], } + +pub(crate) struct JsonParseTestCase { + pub(crate) description: &'static str, + pub(crate) dialect: &'static str, + pub(crate) sql: &'static str, + pub(crate) expected_anchors: &'static [&'static str], + pub(crate) expected_details: &'static [&'static str], +} + +pub(crate) struct JsonParseScaleTestCase { + pub(crate) description: &'static str, + pub(crate) column_count: usize, + pub(crate) expected_findings: usize, + pub(crate) expected_max_seconds: f64, +} diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/configuration-and-selection.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/configuration-and-selection.md index 03964b35c9..5f3cc3c8ea 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/configuration-and-selection.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/configuration-and-selection.md @@ -12,6 +12,7 @@ Online: https://sqlbuild.com/docs/concepts/rules/configuration-and-selection/ - Rule options - Focused selection - Join keys and final CTE names +- Repeated JSON parsing - SQL quality limits and ranking determinism Configure built-in and custom Rules together: @@ -115,6 +116,55 @@ owns the terminal `SELECT`'s plain projection from that last CTE, so select both the `SQBRSQL` family, for the complete convention. Parentheses around the entire query do not create a nested CTE scope. +## Repeated JSON parsing + +`SQBRSQL045` reports a JSON parse function applied to the same column more than once in one +`SELECT`. Parse the payload once in the CTE that reads it, then read every field from that column: + +```sql +WITH parsed AS ( + SELECT e.event_id, TRY_PARSE_JSON(e.value) AS payload + FROM events AS e +), +final AS ( + SELECT p.event_id, p.payload:order_id AS order_id, p.payload:status AS status + FROM parsed AS p +) +SELECT event_id, order_id, status FROM final +``` + +Each `SELECT` is checked on its own: its select list, `FROM`, `JOIN ... ON`, `WHERE`, `GROUP BY`, +`HAVING`, `QUALIFY` and `ORDER BY`. A nested `SELECT`, subquery, CTE body or set-operation branch is +a separate scope. Calls are grouped by function and by the column reference as written, with names +compared the way the warehouse resolves them: on Snowflake unquoted names fold to upper case and +quoted names compare exactly, so `"VALUE"` matches `value` but `"Value"` does not; on DuckDB, +BigQuery and Databricks names compare case-insensitively whether or not they are quoted. References +are not resolved, so `value` and `e.value`, or `events.value` and +`e.value`, are different groups. The argument may be wrapped in parentheses, `LOWER`, `UPPER`, +`TRIM` without trim characters, `CAST`, `TRY_CAST` or a single-argument `COALESCE`; a wrapper with +more arguments, such as `COALESCE(e.value, e.backup)`, is not a plain column and is not counted. One +finding is reported at the second call and names the count with the column as written, for example +`TRY_PARSE_JSON(e.value) is parsed 3 times in one SELECT`. Different parse functions, such as +`PARSE_JSON` and `TRY_PARSE_JSON`, are counted separately. There is no automatic fix. + +Known limitations: + +- Calls inside a named `WINDOW` clause, a window frame, or a Databricks `LATERAL VIEW` are not + checked. +- A parse repeated in `GROUP BY` or `ORDER BY` to match the select list is still reported; group or + order by the parsed column from an earlier CTE instead. +- A parse call generated inside a macro is reported at the macro call site. + +| Adapter | Parse functions | +| --- | --- | +| Snowflake | `PARSE_JSON`, `TRY_PARSE_JSON` | +| Databricks | `PARSE_JSON`, `TRY_PARSE_JSON` | +| BigQuery | `PARSE_JSON`, `SAFE.PARSE_JSON` | +| DuckDB and MotherDuck | `JSON` | + +PostgreSQL and SQL Server have no single-argument JSON parse function (they use casts such as +`::jsonb` or path functions), so the Rule reports nothing on those adapters. + ## SQL quality limits and ranking determinism These Rules are errors with project-wide limits. No model can opt out of a limit locally. @@ -180,6 +230,6 @@ A finding that `sqb format --fix` can fix says so in text output and carries `"f `sqb rules --json run` output; `sqb compile` adds the same note to the diagnostic. Family selectors such as `select = ["SQBRSQL", "SQBRMODEL", "SQBRGRAPH"]` pick up new built-in Rules -on upgrade, so a release that adds `SQBRSQL040` to `SQBRSQL044` can expose computed join keys, -differently named final CTEs, unused CTE columns, long literals or ranking sorts that were accepted -before. +on upgrade, so a release that adds `SQBRSQL040` to `SQBRSQL045` can expose computed join keys, +differently named final CTEs, unused CTE columns, long literals, ranking sorts or repeated JSON +parses that were accepted before. diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 4bf59b42a7..6ad2171cea 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -929,3 +929,16 @@ class WarmTargetEditTestCase: edited_contents: str expected_changed: tuple[str, ...] expected_exit_code: int = 0 + + +@dataclass(frozen=True) +class RepeatedJsonParseTestCase: + """One model body checked by SQBRSQL045 through the CLI.""" + + description: str + adapter: str + header: str + sql: str + expected_locations: tuple[tuple[int, int], ...] + expected_detail: str = "" + extra_files: tuple[tuple[str, str], ...] = () diff --git a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py index 9e2971265c..8e6533315b 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py @@ -31,6 +31,9 @@ from sqlbuild.compiler.planner.exceptions import NativeSqlTestPlanningError from sqlbuild.compiler.planner.models import NativeSqlTestArtifact from sqlbuild.spec.contracts.models import SourceLocation +from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + RepeatedJsonParseTestCase, +) def staging_directories(target_dir: Path) -> list[str]: @@ -661,3 +664,22 @@ def recording(**kwargs: Any) -> dict[str, SourceLocation]: discovery_model_files_module, "matched_model_output_column_locations", recording ) return scanned + + +def write_repeated_json_parse_project( + *, tmp_path: Path, test_case: RepeatedJsonParseTestCase +) -> None: + """Write a one-model project that selects only SQBRSQL045.""" + + (tmp_path / "sqlbuild_project.toml").write_text( + f'name = "orders"\nadapter = "{test_case.adapter}"\n\n[rules]\nselect = ["SQBRSQL045"]\n', + encoding="utf-8", + ) + files: tuple[tuple[str, str], ...] = ( + ("models/order_events.sql", f"{test_case.header}{test_case.sql}"), + *test_case.extra_files, + ) + for relative_path, contents in files: + path: Path = tmp_path / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(contents, encoding="utf-8") diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_repeated_json_parse.py b/tests/integration/src/sqlbuild/cli/commands/main/test_repeated_json_parse.py new file mode 100644 index 0000000000..a6e0dc22f6 --- /dev/null +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_repeated_json_parse.py @@ -0,0 +1,163 @@ +"""Real-CLI coverage for SQBRSQL045, a JSON payload parsed more than once in one SELECT.""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from sqlbuild.cli.commands.main.entrypoint.entry import main +from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + RepeatedJsonParseTestCase, +) +from tests.integration.src.sqlbuild.cli.commands.main.helpers import ( + write_repeated_json_parse_project, +) + +_DUCKDB_HEADER: str = 'MODEL (description "Order events");\n' +_SNOWFLAKE_HEADER: str = ( + 'MODEL (description "Order events", database warehouse, schema analytics);\n' +) +_DUCKDB_EVENTS: str = "WITH events AS (SELECT 1 AS event_id, '{\"order_id\": 1}' AS payload),\n" + + +@pytest.mark.parametrize( + "test_case", + [ + RepeatedJsonParseTestCase( + description="duckdb payload parsed per field", + adapter="duckdb", + header=_DUCKDB_HEADER, + sql=( + f"{_DUCKDB_EVENTS}" + "final AS (\n" + " SELECT\n" + " e.event_id,\n" + " json(e.payload)->>'order_id' AS order_id,\n" + " json(e.payload)->>'status' AS status\n" + " FROM events AS e\n" + ")\n" + "SELECT event_id, order_id, status FROM final\n" + ), + expected_locations=((7, 5),), + expected_detail="JSON(e.payload) is parsed 2 times in one SELECT.", + ), + RepeatedJsonParseTestCase( + description="repeat generated by a macro is located at the macro call", + adapter="duckdb", + header=_DUCKDB_HEADER, + sql=( + f"{_DUCKDB_EVENTS}" + "final AS (\n" + " SELECT\n" + " e.event_id,\n" + " json(e.payload)->>'status' AS status,\n" + " @order_id_field() AS order_id\n" + " FROM events AS e\n" + ")\n" + "SELECT event_id, order_id, status FROM final\n" + ), + expected_locations=((7, 5),), + expected_detail="JSON(e.payload) is parsed 2 times in one SELECT.", + extra_files=( + ( + "macros/fields.py", + "def order_id_field(ctx):\n return \"json(e.payload)->>'order_id'\"\n", + ), + ), + ), + RepeatedJsonParseTestCase( + description="duckdb payload parsed once in its own CTE", + adapter="duckdb", + header=_DUCKDB_HEADER, + sql=( + f"{_DUCKDB_EVENTS}" + "parsed AS (SELECT e.event_id, json(e.payload) AS payload FROM events AS e),\n" + "final AS (\n" + " SELECT\n" + " p.event_id,\n" + " p.payload->>'order_id' AS order_id,\n" + " p.payload->>'status' AS status\n" + " FROM parsed AS p\n" + ")\n" + "SELECT event_id, order_id, status FROM final\n" + ), + expected_locations=(), + ), + ], + ids=lambda case: case.description, +) +def test_given_json_parses_when_compiling_and_running_rules_then_repeats_fail_at_second_parse( + test_case: RepeatedJsonParseTestCase, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + write_repeated_json_parse_project(tmp_path=tmp_path, test_case=test_case) + expected_exit_code: int = int(bool(test_case.expected_locations)) + + assert main(["--project-dir", str(tmp_path), "compile", "--no-cache"]) == expected_exit_code + capsys.readouterr() + result: int = main(["--project-dir", str(tmp_path), "rules", "--json", "run", "SQBRSQL045"]) + findings: list[dict[str, object]] = json.loads(capsys.readouterr().out)["findings"] + + assert result == expected_exit_code + assert tuple((finding["line"], finding["column"]) for finding in findings) == ( + test_case.expected_locations + ) + assert all( + str(finding["remediation"]).startswith(test_case.expected_detail) for finding in findings + ) + assert all( + "parse it once" in str(finding["remediation"]).lower() + and "AS payload" in str(finding["remediation"]) + for finding in findings + ) + + +@pytest.mark.parametrize( + "test_case", + [ + RepeatedJsonParseTestCase( + description="snowflake try parse repeated in a filter", + adapter="snowflake", + header=_SNOWFLAKE_HEADER, + sql=( + "SELECT TRY_PARSE_JSON(e.value):order_id AS order_id\n" + "FROM raw_events AS e\n" + "WHERE TRY_PARSE_JSON(e.value):status = 'shipped'\n" + ), + expected_locations=((4, 7),), + expected_detail="TRY_PARSE_JSON(e.value) is parsed 2 times in one SELECT.", + ), + RepeatedJsonParseTestCase( + description="snowflake parse and try parse are different calls", + adapter="snowflake", + header=_SNOWFLAKE_HEADER, + sql=( + "SELECT TRY_PARSE_JSON(e.value):order_id AS order_id\n" + "FROM raw_events AS e\n" + "WHERE PARSE_JSON(e.value):status = 'shipped'\n" + ), + expected_locations=(), + ), + ], + ids=lambda case: case.description, +) +def test_given_snowflake_json_parses_when_running_rules_then_repeats_are_reported_offline( + test_case: RepeatedJsonParseTestCase, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + write_repeated_json_parse_project(tmp_path=tmp_path, test_case=test_case) + + result: int = main(["--project-dir", str(tmp_path), "rules", "--json", "run", "SQBRSQL045"]) + findings: list[dict[str, object]] = json.loads(capsys.readouterr().out)["findings"] + + assert result == int(bool(test_case.expected_locations)) + assert tuple((finding["line"], finding["column"]) for finding in findings) == ( + test_case.expected_locations + ) + assert all( + str(finding["remediation"]).startswith(test_case.expected_detail) for finding in findings + ) + + +if __name__ == "__main__": + pytest.main([__file__, "-vv"]) diff --git a/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_catalogue.py b/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_catalogue.py index c31b0956bb..5593ed2d7e 100644 --- a/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_catalogue.py +++ b/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_catalogue.py @@ -65,7 +65,7 @@ "SQBRTEST203", "SQBRTEST301", ) -SQL_CODES: tuple[str, ...] = tuple(f"SQBRSQL{number:03d}" for number in range(1, 45)) +SQL_CODES: tuple[str, ...] = tuple(f"SQBRSQL{number:03d}" for number in range(1, 46)) BUILT_IN_CODES: tuple[str, ...] = tuple(sorted((*RULES_BUILT_IN_CODES, *SQL_CODES))) STRUCTURE_CODES: tuple[str, ...] = ( "SQBRMODEL101", diff --git a/website/src/content/docs/docs/concepts/rules/configuration-and-selection.mdx b/website/src/content/docs/docs/concepts/rules/configuration-and-selection.mdx index c38af35958..3ef7a3e73e 100644 --- a/website/src/content/docs/docs/concepts/rules/configuration-and-selection.mdx +++ b/website/src/content/docs/docs/concepts/rules/configuration-and-selection.mdx @@ -108,6 +108,55 @@ owns the terminal `SELECT`'s plain projection from that last CTE, so select both the `SQBRSQL` family, for the complete convention. Parentheses around the entire query do not create a nested CTE scope. +## Repeated JSON parsing + +`SQBRSQL045` reports a JSON parse function applied to the same column more than once in one +`SELECT`. Parse the payload once in the CTE that reads it, then read every field from that column: + +```sql +WITH parsed AS ( + SELECT e.event_id, TRY_PARSE_JSON(e.value) AS payload + FROM events AS e +), +final AS ( + SELECT p.event_id, p.payload:order_id AS order_id, p.payload:status AS status + FROM parsed AS p +) +SELECT event_id, order_id, status FROM final +``` + +Each `SELECT` is checked on its own: its select list, `FROM`, `JOIN ... ON`, `WHERE`, `GROUP BY`, +`HAVING`, `QUALIFY` and `ORDER BY`. A nested `SELECT`, subquery, CTE body or set-operation branch is +a separate scope. Calls are grouped by function and by the column reference as written, with names +compared the way the warehouse resolves them: on Snowflake unquoted names fold to upper case and +quoted names compare exactly, so `"VALUE"` matches `value` but `"Value"` does not; on DuckDB, +BigQuery and Databricks names compare case-insensitively whether or not they are quoted. References +are not resolved, so `value` and `e.value`, or `events.value` and +`e.value`, are different groups. The argument may be wrapped in parentheses, `LOWER`, `UPPER`, +`TRIM` without trim characters, `CAST`, `TRY_CAST` or a single-argument `COALESCE`; a wrapper with +more arguments, such as `COALESCE(e.value, e.backup)`, is not a plain column and is not counted. One +finding is reported at the second call and names the count with the column as written, for example +`TRY_PARSE_JSON(e.value) is parsed 3 times in one SELECT`. Different parse functions, such as +`PARSE_JSON` and `TRY_PARSE_JSON`, are counted separately. There is no automatic fix. + +Known limitations: + +- Calls inside a named `WINDOW` clause, a window frame, or a Databricks `LATERAL VIEW` are not + checked. +- A parse repeated in `GROUP BY` or `ORDER BY` to match the select list is still reported; group or + order by the parsed column from an earlier CTE instead. +- A parse call generated inside a macro is reported at the macro call site. + +| Adapter | Parse functions | +| --- | --- | +| Snowflake | `PARSE_JSON`, `TRY_PARSE_JSON` | +| Databricks | `PARSE_JSON`, `TRY_PARSE_JSON` | +| BigQuery | `PARSE_JSON`, `SAFE.PARSE_JSON` | +| DuckDB and MotherDuck | `JSON` | + +PostgreSQL and SQL Server have no single-argument JSON parse function (they use casts such as +`::jsonb` or path functions), so the Rule reports nothing on those adapters. + ## SQL quality limits and ranking determinism These Rules are errors with project-wide limits. No model can opt out of a limit locally. @@ -173,6 +222,6 @@ A finding that `sqb format --fix` can fix says so in text output and carries `"f `sqb rules --json run` output; `sqb compile` adds the same note to the diagnostic. Family selectors such as `select = ["SQBRSQL", "SQBRMODEL", "SQBRGRAPH"]` pick up new built-in Rules -on upgrade, so a release that adds `SQBRSQL040` to `SQBRSQL044` can expose computed join keys, -differently named final CTEs, unused CTE columns, long literals or ranking sorts that were accepted -before. +on upgrade, so a release that adds `SQBRSQL040` to `SQBRSQL045` can expose computed join keys, +differently named final CTEs, unused CTE columns, long literals, ranking sorts or repeated JSON +parses that were accepted before. From 7ac3bd350755b7663fbf9d21c3afcef08b87bf70 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 4 Oct 2026 10:01:12 +0100 Subject: [PATCH 2/3] 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. --- .../src/configuration/_helpers/loading.rs | 1 + crates/sqlbuild-rules-native/src/models.rs | 2 + .../docs/concepts/project-configuration.md | 4 + .../references/docs/concepts/rules.md | 3 +- .../concepts/rules/findings-and-exceptions.md | 31 ++++ .../cli/commands/_helpers/lint/runs.py | 4 + src/sqlbuild/lint/_helpers/suppressions.py | 60 +++++- src/sqlbuild/lint/constants.py | 2 + src/sqlbuild/lint/main/run_format.py | 1 + src/sqlbuild/lint/main/run_lint.py | 6 +- src/sqlbuild/lint/models.py | 1 + .../rule_engine/_helpers/engine/config.py | 61 +++++- .../rule_engine/_helpers/run/rules.py | 5 + src/sqlbuild/rule_engine/constants.py | 4 + src/sqlbuild/rule_engine/exceptions.py | 9 + src/sqlbuild/rule_engine/models.py | 1 + .../sqlbuild/cli/commands/main/_test_types.py | 22 +++ .../main/test_rule_exception_policy.py | 173 ++++++++++++++++++ tests/unit/src/sqlbuild/lint/test_run_lint.py | 40 +++- .../_helpers/engine/_test_types.py | 9 + .../_helpers/engine/test_config.py | 92 +++++++++- .../docs/concepts/project-configuration.mdx | 4 + .../src/content/docs/docs/concepts/rules.mdx | 3 +- .../rules/findings-and-exceptions.mdx | 31 ++++ 24 files changed, 558 insertions(+), 11 deletions(-) create mode 100644 tests/integration/src/sqlbuild/cli/commands/main/test_rule_exception_policy.py diff --git a/crates/sqlbuild-rules-native/src/configuration/_helpers/loading.rs b/crates/sqlbuild-rules-native/src/configuration/_helpers/loading.rs index 76a90afcda..f36c31c00f 100644 --- a/crates/sqlbuild-rules-native/src/configuration/_helpers/loading.rs +++ b/crates/sqlbuild-rules-native/src/configuration/_helpers/loading.rs @@ -51,6 +51,7 @@ fn validate_raw(value: &toml::Value) -> Result<(), String> { "rule_exceptions", "graph_edge_exceptions", "rule_ignores", + "allow_exceptions", "select_star_allow", "domains", "approved_source_tokens", diff --git a/crates/sqlbuild-rules-native/src/models.rs b/crates/sqlbuild-rules-native/src/models.rs index e20ba27d73..e45f0528f6 100644 --- a/crates/sqlbuild-rules-native/src/models.rs +++ b/crates/sqlbuild-rules-native/src/models.rs @@ -145,6 +145,7 @@ pub(crate) struct RulesConfig { pub rule_exceptions: Vec, pub graph_edge_exceptions: Vec, pub rule_ignores: Vec, + pub allow_exceptions: bool, pub select_star_allow: Vec, pub domains: Vec, pub approved_source_tokens: Vec, @@ -165,6 +166,7 @@ impl Default for RulesConfig { rule_exceptions: vec![], graph_edge_exceptions: vec![], rule_ignores: vec![], + allow_exceptions: true, select_star_allow: vec![], domains: vec![], approved_source_tokens: vec![], diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/project-configuration.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/project-configuration.md index e5cfd5e06e..044ebcf639 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/project-configuration.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/project-configuration.md @@ -494,6 +494,10 @@ Rules are opt-in. Exact codes activate individual checks and prefixes activate a codes begin with `SQBR`; repository-defined codes begin with `XSQBR`. See [Compiler-integrated Rules](rules.md) for configuration, authoring, and suppressions. +| Key | Default | Description | +| --- | --- | --- | +| `allow_exceptions` | `true` | When `false`, `[[rules.rule_exceptions]]` and `[[rules.rule_ignores]]` entries are configuration errors and inline `-- sqb: ignore` directives are `SQBRSQL000` errors. The project-wide `ignore` list stays allowed. See [Forbidding exceptions](rules/findings-and-exceptions.md#forbidding-exceptions). | + ## Scopes Declaration placement findings are errors by default. Set `enforce_placement = false` to report diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md index 993ccef02d..a512a2610d 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md @@ -211,7 +211,8 @@ This makes the same requirement usable in a terminal, JSON output, CI annotation Intentional departures remain explicit. Exact exceptions and path- or resource-scoped ignores require a reason; exact exceptions are stale-checked so obsolete suppressions do not silently -accumulate. Mandatory compiler correctness cannot be suppressed. +accumulate. Mandatory compiler correctness cannot be suppressed. A project can forbid exceptions, +scoped ignores and inline suppressions entirely with `[rules] allow_exceptions = false`. See [Findings and exceptions](rules/findings-and-exceptions.md) for configuration examples. diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/findings-and-exceptions.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/findings-and-exceptions.md index 887b4b818e..b833e7b2a6 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/findings-and-exceptions.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/findings-and-exceptions.md @@ -59,6 +59,37 @@ reason = "These intermediate SQL files intentionally preserve upstream columns." `paths` and `selectors` may be combined in one scoped ignore. Both forms require a reason. +## Inline suppressions + +Suppress one SQL Rule finding on the next SQL line with a reasoned comment: + +```sql +-- sqb: ignore SQBRSQL004 because the sample intentionally keeps one row +SELECT customer_id FROM customers LIMIT 1 +``` + +A directive without a reason, or one that no longer matches a finding, is reported as +`SQBRSQL000`. + +## Forbidding exceptions + +Set `allow_exceptions = false` to require that every selected Rule passes everywhere: + +```toml +[rules] +select = ["SQBRSQL", "SQBRMODEL"] +ignore = ["SQBRSQL004"] +allow_exceptions = false # default true +``` + +With exceptions forbidden, any `[[rules.rule_exceptions]]` or `[[rules.rule_ignores]]` entry fails +configuration loading with an error that counts the entries and shows the setting to change, so +`sqb compile` and `sqb rules run` stop before evaluating Rules. Each inline `-- sqb: ignore` +directive becomes a `SQBRSQL000` error at the directive and suppresses nothing, so the finding it +was hiding is reported too. The project-wide `ignore` list stays allowed: it is the adoption switch +for a Rule across the whole project, not a scoped escape hatch. `[[rules.graph_edge_exceptions]]`, +`[[rules.select_star_allow]]` and `[[rules.threshold_overrides]]` are not affected. + ## Mandatory correctness Mandatory compiler correctness is not configurable and cannot be suppressed. A project must first diff --git a/src/sqlbuild/cli/commands/_helpers/lint/runs.py b/src/sqlbuild/cli/commands/_helpers/lint/runs.py index 3a234a60b0..cd1383c81c 100644 --- a/src/sqlbuild/cli/commands/_helpers/lint/runs.py +++ b/src/sqlbuild/cli/commands/_helpers/lint/runs.py @@ -28,6 +28,7 @@ LintRunResult, ) from sqlbuild.presentation.classes.cli_style import CliStyle +from sqlbuild.rule_engine.main.load_config import load_rules_config def resolve_lint_config(*, project_dir: Path) -> LintConfig: @@ -37,11 +38,13 @@ def resolve_lint_config(*, project_dir: Path) -> LintConfig: line_width: int = DEFAULT_LINE_WIDTH dialect: str = "generic" thresholds: dict[str, object] = {} + allow_suppressions: bool = True config_file: Path = project_dir / PROJECT_CONFIG_FILENAME_KEY if config_file.is_file(): with config_file.open("rb") as handle: payload: dict[str, object] = tomllib.load(handle) thresholds = _rule_thresholds(payload) + allow_suppressions = load_rules_config(project_dir=project_dir).allow_exceptions format_section: object = payload.get(FORMAT_SECTION_KEY) raw_adapter: object = payload.get(ADAPTER_CONFIG_KEY) if isinstance(raw_adapter, str): @@ -78,6 +81,7 @@ def resolve_lint_config(*, project_dir: Path) -> LintConfig: key=MAX_RANKING_ORDER_BY_THRESHOLD, default=DEFAULT_MAX_RANKING_ORDER_BY, ), + allow_suppressions=allow_suppressions, ) diff --git a/src/sqlbuild/lint/_helpers/suppressions.py b/src/sqlbuild/lint/_helpers/suppressions.py index 7db6cc0f10..9b62f460e1 100644 --- a/src/sqlbuild/lint/_helpers/suppressions.py +++ b/src/sqlbuild/lint/_helpers/suppressions.py @@ -3,10 +3,19 @@ from __future__ import annotations import re -from dataclasses import dataclass +from dataclasses import dataclass, replace from pathlib import Path -from sqlbuild.lint.constants import LINT_ENGINE_NATIVE, VIOLATION_SEVERITY_WARNING +from sqlbuild.errors.setting_help.main.setting_help import setting_help +from sqlbuild.errors.setting_help.main.setting_note import setting_note +from sqlbuild.lint.constants import ( + ALLOW_EXCEPTIONS_KEY, + LINT_ENGINE_NATIVE, + PROJECT_CONFIG_FILENAME_KEY, + RULES_SECTION_KEY, + VIOLATION_SEVERITY_FAULT, + VIOLATION_SEVERITY_WARNING, +) from sqlbuild.lint.models import LintEdit, LintViolation _SUPPRESSION_PREFIX: str = "sqb: ignore" @@ -24,13 +33,24 @@ class _Suppression: def apply_suppressions( - *, violations: list[LintViolation], contents_by_path: dict[Path, str] + *, + violations: list[LintViolation], + contents_by_path: dict[Path, str], + allow_suppressions: bool, ) -> list[LintViolation]: - """Remove matching next-line diagnostics and report invalid or unused directives.""" + """Remove matching next-line diagnostics, or reject every directive when forbidden.""" retained: list[LintViolation] = list(violations) for file_path, contents in contents_by_path.items(): suppressions, invalid = _parse_suppressions(file_path=file_path, contents=contents) + if not allow_suppressions: + directive_lines: set[int] = {violation.line for violation in invalid} + directive_lines.update(suppression.directive_line for suppression in suppressions) + retained.extend( + _forbidden_violation(file_path=file_path, line=line, contents=contents) + for line in sorted(directive_lines) + ) + continue retained.extend(invalid) for suppression in suppressions: match_index: int | None = next( @@ -98,6 +118,38 @@ def _parse_suppressions( return tuple(suppressions), tuple(invalid) +def _forbidden_violation(*, file_path: Path, line: int, contents: str) -> LintViolation: + violation: LintViolation = _suppression_violation( + file_path=file_path, + line=line, + message="Inline Rule suppressions are not allowed in this project", + remediation=( + "Remove the directive and fix the finding it suppresses; " + + setting_note( + file_name=PROJECT_CONFIG_FILENAME_KEY, + section=RULES_SECTION_KEY, + key=ALLOW_EXCEPTIONS_KEY, + value=False, + ) + + ". " + + setting_help( + purpose="To allow inline suppressions", + file_name=PROJECT_CONFIG_FILENAME_KEY, + section=RULES_SECTION_KEY, + key=ALLOW_EXCEPTIONS_KEY, + value=True, + ) + ), + contents=contents, + fixable=False, + ) + return replace( + violation, + severity=VIOLATION_SEVERITY_FAULT, + fix_unavailable_reason="removing the directive exposes the finding it suppresses", + ) + + def _suppression_violation( *, file_path: Path, diff --git a/src/sqlbuild/lint/constants.py b/src/sqlbuild/lint/constants.py index ebf419e223..7ee0d132fe 100644 --- a/src/sqlbuild/lint/constants.py +++ b/src/sqlbuild/lint/constants.py @@ -82,6 +82,8 @@ PROJECT_CONFIG_FILENAME_KEY: str = "sqlbuild_project.toml" FORMAT_SECTION_KEY: str = "format" +RULES_SECTION_KEY: str = "rules" +ALLOW_EXCEPTIONS_KEY: str = "allow_exceptions" MAX_DESCRIPTION_LINES_KEY: str = "max_description_lines" LINE_WIDTH_KEY: str = "line_width" diff --git a/src/sqlbuild/lint/main/run_format.py b/src/sqlbuild/lint/main/run_format.py index 5bccae6650..4d86565a2a 100644 --- a/src/sqlbuild/lint/main/run_format.py +++ b/src/sqlbuild/lint/main/run_format.py @@ -100,6 +100,7 @@ def run_format( apply_suppressions( violations=violations, contents_by_path=final_contents, + allow_suppressions=config.allow_suppressions, ) ), formatted_files=tuple(change.file_path for change in changes), diff --git a/src/sqlbuild/lint/main/run_lint.py b/src/sqlbuild/lint/main/run_lint.py index 9756337ee1..5c7f0dd96e 100644 --- a/src/sqlbuild/lint/main/run_lint.py +++ b/src/sqlbuild/lint/main/run_lint.py @@ -106,7 +106,11 @@ def run_lint( return LintRunResult( files_checked=len(files), violations=sort_violations( - apply_suppressions(violations=violations, contents_by_path=files) + apply_suppressions( + violations=violations, + contents_by_path=files, + allow_suppressions=config.allow_suppressions, + ) ), formatted_files=(), source_texts=files, diff --git a/src/sqlbuild/lint/models.py b/src/sqlbuild/lint/models.py index eb88a403d0..30539080cd 100644 --- a/src/sqlbuild/lint/models.py +++ b/src/sqlbuild/lint/models.py @@ -177,6 +177,7 @@ class LintConfig: header_rules_enabled: bool = True max_literal_length: int = DEFAULT_MAX_LITERAL_LENGTH max_ranking_order_by: int = DEFAULT_MAX_RANKING_ORDER_BY + allow_suppressions: bool = True relation_keys: Mapping[str, tuple[tuple[str, ...], ...]] = field( default_factory=dict, compare=False ) diff --git a/src/sqlbuild/rule_engine/_helpers/engine/config.py b/src/sqlbuild/rule_engine/_helpers/engine/config.py index 3cb0774f4c..d8cbe73b35 100644 --- a/src/sqlbuild/rule_engine/_helpers/engine/config.py +++ b/src/sqlbuild/rule_engine/_helpers/engine/config.py @@ -7,10 +7,21 @@ from typing import cast from sqlbuild.compiler.compile.models import CompiledObjectKey, CompiledProject +from sqlbuild.compiler.discovery.constants import PROJECT_CONFIG_FILENAME from sqlbuild.compiler.planner.main.selection.scope import build_planner_scope from sqlbuild.compiler.planner.models import PlannerScope +from sqlbuild.errors.setting_help.main.join_helps import join_helps +from sqlbuild.errors.setting_help.main.setting_help import setting_help +from sqlbuild.errors.setting_help.main.setting_note import setting_note +from sqlbuild.presentation.main.count_noun import format_count_noun from sqlbuild.rule_engine._helpers.engine.native import load_native_config -from sqlbuild.rule_engine.exceptions import RulesError +from sqlbuild.rule_engine.constants import ( + ALLOW_EXCEPTIONS_KEY, + RULE_EXCEPTIONS_KEY, + RULE_IGNORES_KEY, + RULES_CONFIG_SECTION, +) +from sqlbuild.rule_engine.exceptions import RulesConfigError, RulesError from sqlbuild.rule_engine.models import ( GraphEdgeExclusion, LayoutConfig, @@ -29,7 +40,7 @@ def load_rules_config(project_dir: Path) -> RulesConfig: """Load configuration validated and normalized by the native engine.""" payload: dict[str, object] = load_native_config(project_dir) - return RulesConfig( + config: RulesConfig = RulesConfig( select=_strings(payload.get("select")), ignore=_strings(payload.get("ignore")), thresholds=_integers(payload.get("thresholds")), @@ -65,6 +76,7 @@ def load_rules_config(project_dir: Path) -> RulesConfig: ) for item in _tables(payload.get("rule_ignores")) ), + allow_exceptions=payload.get("allow_exceptions") is not False, select_star_allow=tuple( SelectStarAllow(paths=_strings(item.get("paths")), reason=str(item["reason"])) for item in _tables(payload.get("select_star_allow")) @@ -76,6 +88,51 @@ def load_rules_config(project_dir: Path) -> RulesConfig: layout=_layout(payload.get("layout")), cache=_cache(payload.get("cache")), ) + _reject_forbidden_exceptions(config) + return config + + +def _reject_forbidden_exceptions(config: RulesConfig) -> None: + """Fail when `[rules] allow_exceptions = false` and exceptions or scoped ignores exist.""" + + if config.allow_exceptions: + return + entries: list[str] = [] + tables: list[str] = [] + for key, count in ( + (RULE_EXCEPTIONS_KEY, len(config.rule_exceptions)), + (RULE_IGNORES_KEY, len(config.rule_ignores)), + ): + if count: + table: str = f"[[{RULES_CONFIG_SECTION}.{key}]]" + tables.append(table) + entries.append( + format_count_noun(count=count, singular=f"{table} entry", plural=f"{table} entries") + ) + if not entries: + return + raise RulesConfigError( + f"{PROJECT_CONFIG_FILENAME} has {' and '.join(entries)}, which this project does not " + "allow; " + + setting_note( + file_name=PROJECT_CONFIG_FILENAME, + section=RULES_CONFIG_SECTION, + key=ALLOW_EXCEPTIONS_KEY, + value=False, + ), + help=join_helps( + f"remove every {' and '.join(tables)} entry and fix the findings it hides; " + f"to turn a Rule off for the whole project, add its code to " + f"[{RULES_CONFIG_SECTION}] ignore", + setting_help( + purpose="to allow Rule exceptions, scoped ignores and inline suppressions", + file_name=PROJECT_CONFIG_FILENAME, + section=RULES_CONFIG_SECTION, + key=ALLOW_EXCEPTIONS_KEY, + value=True, + ), + ), + ) def resolve_rule_ignore_selectors(*, config: RulesConfig, project: CompiledProject) -> RulesConfig: diff --git a/src/sqlbuild/rule_engine/_helpers/run/rules.py b/src/sqlbuild/rule_engine/_helpers/run/rules.py index a4053439f7..e6b0fed4cd 100644 --- a/src/sqlbuild/rule_engine/_helpers/run/rules.py +++ b/src/sqlbuild/rule_engine/_helpers/run/rules.py @@ -178,6 +178,7 @@ def evaluate_rules( codes=_sql_rule_codes(native_rules), thresholds=effective_config.thresholds, relation_keys=compiled_relation_keys(graph.project), + allow_suppressions=effective_config.allow_exceptions, ) sql_rule_inputs: _SqlRuleInputs = _prepare_sql_rule_inputs( rules=native_rules, @@ -294,6 +295,7 @@ def prepare_sql_rules( dialect=dialect, codes=codes, thresholds=config.thresholds, + allow_suppressions=config.allow_exceptions, relation_keys=declared_relation_keys( relations=( *( @@ -671,6 +673,7 @@ def sql_lint_config( codes: tuple[str, ...], thresholds: dict[str, int], relation_keys: RelationKeys, + allow_suppressions: bool, ) -> LintConfig: """Build the native SQL lint configuration that SQL Rules run with.""" @@ -683,6 +686,7 @@ def sql_lint_config( MAX_RANKING_ORDER_BY_THRESHOLD, DEFAULT_MAX_RANKING_ORDER_BY ), relation_keys=relation_keys, + allow_suppressions=allow_suppressions, ) @@ -695,6 +699,7 @@ def _lint_identity(config: LintConfig) -> str: config.max_literal_length, config.max_ranking_order_by, sorted(config.relation_keys.items()), + config.allow_suppressions, ], separators=(",", ":"), ) diff --git a/src/sqlbuild/rule_engine/constants.py b/src/sqlbuild/rule_engine/constants.py index 98078ffac1..bc80715cb5 100644 --- a/src/sqlbuild/rule_engine/constants.py +++ b/src/sqlbuild/rule_engine/constants.py @@ -156,3 +156,7 @@ FACT_DECLARATIONS_PUBLIC_CONSTANTS: str = "declarations.public_constants" FACT_DECLARATIONS_ENUMS: str = "declarations.enums" FACT_DECLARATIONS_CONSTANTS: str = "declarations.constants" +RULES_CONFIG_SECTION: str = "rules" +ALLOW_EXCEPTIONS_KEY: str = "allow_exceptions" +RULE_EXCEPTIONS_KEY: str = "rule_exceptions" +RULE_IGNORES_KEY: str = "rule_ignores" diff --git a/src/sqlbuild/rule_engine/exceptions.py b/src/sqlbuild/rule_engine/exceptions.py index 9786c0c1f5..5e32b7cb94 100644 --- a/src/sqlbuild/rule_engine/exceptions.py +++ b/src/sqlbuild/rule_engine/exceptions.py @@ -5,6 +5,15 @@ class RulesError(RuntimeError): """Raised for invalid compiler rules or execution.""" +class RulesConfigError(RulesError): + """Raised when rules configuration breaks a project policy; help shows the setting to change.""" + + def __init__(self, message: str, *, help: str) -> None: + super().__init__(message) + self.message = message + self.help = help + + class RuleUsageError(RulesError): """Raised when a rule or rule test uses the public API incorrectly.""" diff --git a/src/sqlbuild/rule_engine/models.py b/src/sqlbuild/rule_engine/models.py index 99055f59b4..040acdc066 100644 --- a/src/sqlbuild/rule_engine/models.py +++ b/src/sqlbuild/rule_engine/models.py @@ -292,6 +292,7 @@ class RulesConfig: rule_exceptions: tuple[RuleExemption, ...] = () graph_edge_exceptions: tuple[GraphEdgeExclusion, ...] = () rule_ignores: tuple[RuleIgnore, ...] = () + allow_exceptions: bool = True select_star_allow: tuple[SelectStarAllow, ...] = () domains: tuple[str, ...] = () approved_source_tokens: tuple[str, ...] = () diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 6ad2171cea..a1cf537f81 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -931,6 +931,28 @@ class WarmTargetEditTestCase: expected_exit_code: int = 0 +@dataclass(frozen=True) +class RuleExceptionPolicyTestCase: + """One `[rules] allow_exceptions` value with one escape hatch, compiled through the CLI.""" + + description: str + rules_toml: str + directive: str + expected_exit_code: int + expected_fragments: tuple[str, ...] = () + + +@dataclass(frozen=True) +class FormatExceptionPolicyTestCase: + """One `[rules] allow_exceptions` value read by `sqb format`.""" + + description: str + setting: str + expected_exit_code: int + expected_fragment: str + expected_compile_fragment: str + + @dataclass(frozen=True) class RepeatedJsonParseTestCase: """One model body checked by SQBRSQL045 through the CLI.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rule_exception_policy.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rule_exception_policy.py new file mode 100644 index 0000000000..32c72fd46c --- /dev/null +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rule_exception_policy.py @@ -0,0 +1,173 @@ +"""Real-CLI coverage for forbidding Rule exceptions, scoped ignores and inline suppressions.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from _pytest.capture import CaptureResult + +from sqlbuild.cli.commands.main.entrypoint.entry import main +from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + FormatExceptionPolicyTestCase, + RuleExceptionPolicyTestCase, +) + +_EXCEPTION: str = ( + '[[rules.rule_exceptions]]\nrule = "SQBRSQL004"\npath = "models/customers.sql"\n' + 'reason = "The sample intentionally keeps one row."\n' +) +_IGNORE: str = ( + '[[rules.rule_ignores]]\nrules = ["SQBRSQL004"]\npaths = ["models/**"]\n' + 'reason = "Samples intentionally keep one row."\n' +) +_DIRECTIVE: str = "-- sqb: ignore SQBRSQL004 because the sample intentionally keeps one row\n" +_SETTING_HELP: str = "[rules]\n allow_exceptions = true" + + +@pytest.mark.parametrize( + "test_case", + [ + RuleExceptionPolicyTestCase( + description="exception forbidden", + rules_toml=f"allow_exceptions = false\n\n{_EXCEPTION}", + directive="", + expected_exit_code=1, + expected_fragments=( + "sqlbuild_project.toml has 1 [[rules.rule_exceptions]] entry", + "sets [rules] allow_exceptions = false", + _SETTING_HELP, + ), + ), + RuleExceptionPolicyTestCase( + description="scoped ignore forbidden", + rules_toml=f"allow_exceptions = false\n\n{_IGNORE}", + directive="", + expected_exit_code=1, + expected_fragments=("1 [[rules.rule_ignores]] entry", _SETTING_HELP), + ), + RuleExceptionPolicyTestCase( + description="inline suppression forbidden", + rules_toml="allow_exceptions = false\n", + directive=_DIRECTIVE, + expected_exit_code=1, + expected_fragments=( + "SQBRSQL000", + "Inline Rule suppressions are not allowed in this project", + "SQBRSQL004", + "allow_exceptions = true", + ), + ), + RuleExceptionPolicyTestCase( + description="project-wide ignore stays allowed", + rules_toml='allow_exceptions = false\nignore = ["SQBRSQL004"]\n', + directive="", + expected_exit_code=0, + ), + RuleExceptionPolicyTestCase( + description="exception allowed explicitly", + rules_toml=f"allow_exceptions = true\n\n{_EXCEPTION}", + directive="", + expected_exit_code=0, + ), + RuleExceptionPolicyTestCase( + description="exception allowed by default", + rules_toml=_EXCEPTION, + directive="", + expected_exit_code=0, + ), + RuleExceptionPolicyTestCase( + description="inline suppression allowed by default", + rules_toml="", + directive=_DIRECTIVE, + expected_exit_code=0, + ), + ], + ids=lambda case: case.description, +) +def test_given_allow_exceptions_setting_when_compiling_then_escape_hatches_follow_it( + test_case: RuleExceptionPolicyTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n\n[rules]\nselect = ["SQBRSQL004"]\n' + f"{test_case.rules_toml}", + encoding="utf-8", + ) + models: Path = tmp_path / "models" + models.mkdir() + (models / "customers.sql").write_text( + "MODEL (description 'Customer sample.');\n" + "WITH customers AS (SELECT 1 AS customer_id)\n" + f"{test_case.directive}SELECT customer_id FROM customers LIMIT 1\n", + encoding="utf-8", + ) + + exit_code: int = main(["--project-dir", str(tmp_path), "compile", "--no-cache"]) + output: CaptureResult[str] = capsys.readouterr() + + assert exit_code == test_case.expected_exit_code, output.out + output.err + for fragment in test_case.expected_fragments: + assert fragment in output.out + output.err + + +@pytest.mark.parametrize( + "test_case", + [ + FormatExceptionPolicyTestCase( + description="string false is rejected like compile", + setting='allow_exceptions = "false"', + expected_exit_code=1, + expected_fragment="invalid rules config", + expected_compile_fragment="invalid rules config", + ), + FormatExceptionPolicyTestCase( + description="boolean false rejects the inline suppression", + setting="allow_exceptions = false", + expected_exit_code=1, + expected_fragment="Inline Rule suppressions are not allowed in this project", + expected_compile_fragment="Inline Rule suppressions are not allowed in this project", + ), + FormatExceptionPolicyTestCase( + description="boolean true keeps the inline suppression", + setting="allow_exceptions = true", + expected_exit_code=0, + expected_fragment="FAULT=0", + expected_compile_fragment="", + ), + ], + ids=lambda case: case.description, +) +def test_given_allow_exceptions_setting_when_formatting_then_it_is_validated_like_compile( + test_case: FormatExceptionPolicyTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n\n[rules]\nselect = ["SQBRSQL004"]\n' + f"{test_case.setting}\n", + encoding="utf-8", + ) + models: Path = tmp_path / "models" + models.mkdir() + (models / "customers.sql").write_text( + "MODEL (description 'Customer sample.');\n" + "WITH customers AS (\n SELECT 1 AS customer_id\n)\n\n" + f"SELECT customer_id\nFROM customers\n{_DIRECTIVE}LIMIT 1\n", + encoding="utf-8", + ) + + format_exit_code: int = main(["--project-dir", str(tmp_path), "format", "--check"]) + format_output: CaptureResult[str] = capsys.readouterr() + compile_exit_code: int = main(["--project-dir", str(tmp_path), "compile", "--no-cache"]) + compile_output: CaptureResult[str] = capsys.readouterr() + + assert format_exit_code == test_case.expected_exit_code, format_output.out + format_output.err + assert compile_exit_code == test_case.expected_exit_code + assert test_case.expected_fragment in format_output.out + format_output.err + assert test_case.expected_compile_fragment in compile_output.out + compile_output.err + + +if __name__ == "__main__": + pytest.main([__file__, "-vv"]) diff --git a/tests/unit/src/sqlbuild/lint/test_run_lint.py b/tests/unit/src/sqlbuild/lint/test_run_lint.py index cf24ff4d79..f85f003ec9 100644 --- a/tests/unit/src/sqlbuild/lint/test_run_lint.py +++ b/tests/unit/src/sqlbuild/lint/test_run_lint.py @@ -361,6 +361,44 @@ def test_given_reasoned_local_suppression_when_linting_then_matching_warning_is_ assert result.violations == test_case.expected_value +@pytest.mark.parametrize( + "test_case", + [ + LintBehaviorTestCase( + description="forbidden suppression keeps the finding and fails", + expected_value=(("SQBRSQL000", 2, "fault"), ("SQBRSQL004", 3, "warning")), + ) + ], + ids=lambda case: case.description, +) +def test_given_suppressions_forbidden_when_linting_then_directive_is_a_fault_and_suppresses_nothing( + test_case: LintBehaviorTestCase, + tmp_path: Path, +) -> None: + _ = (tmp_path / "sqlbuild_project.toml").write_text(PROJECT_TOML, encoding="utf-8") + target: Path = tmp_path / "models" / "sample.sql" + target.parent.mkdir() + _ = target.write_text( + 'MODEL (description "ok");\n' + "-- sqb: ignore SQBRSQL004 because this fixture intentionally samples one row\n" + "SELECT value FROM items LIMIT 1\n", + encoding="utf-8", + ) + + result: LintRunResult = run_lint( + project_dir=tmp_path, + config=LintConfig(dialect="duckdb", allow_suppressions=False), + ) + + assert ( + tuple((item.code, item.line, str(item.severity)) for item in result.violations) + == test_case.expected_value + ) + remediation: str = result.violations[0].remediation or "" + assert "sqlbuild_project.toml sets [rules] allow_exceptions = false" in remediation + assert "[rules]\n allow_exceptions = true" in remediation + + @pytest.mark.parametrize( "test_case", [ @@ -426,7 +464,7 @@ def test_given_header_fault_suppression_when_linting_then_mandatory_fault_remain ) result: list[LintViolation] = apply_suppressions( - violations=[mandatory], contents_by_path={target: contents} + violations=[mandatory], contents_by_path={target: contents}, allow_suppressions=True ) assert {item.code for item in result} == test_case.expected_value diff --git a/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/_test_types.py b/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/_test_types.py index 79617b04b7..d68bf921ab 100644 --- a/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/_test_types.py +++ b/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/_test_types.py @@ -12,6 +12,15 @@ class RulesConfigErrorTestCase: expected_error_pattern: str +@dataclass(frozen=True) +class AllowExceptionsConfigTestCase: + """One `[rules] allow_exceptions` configuration that loads.""" + + description: str + source: str + expected_allow_exceptions: bool + + @dataclass(frozen=True) class RuleIgnoreConfigTestCase: """One scoped-ignore configuration expectation.""" diff --git a/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_config.py b/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_config.py index a9e23caea0..095e19d3d9 100644 --- a/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_config.py +++ b/tests/unit/src/sqlbuild/rule_engine/_helpers/engine/test_config.py @@ -4,16 +4,26 @@ import pytest -from sqlbuild.rule_engine.exceptions import RulesError +from sqlbuild.rule_engine.exceptions import RulesConfigError, RulesError from sqlbuild.rule_engine.main.load_config import load_rules_config from sqlbuild.rule_engine.models import LayoutConfig, RulesConfig, SqlTestRulesConfig from tests.unit.src.sqlbuild.rule_engine._helpers.engine._test_types import ( + AllowExceptionsConfigTestCase, PolicyLayoutConfigTestCase, RuleIgnoreConfigTestCase, RulesConfigErrorTestCase, SqlTestRulesConfigTestCase, ) +_EXCEPTION_ENTRY: str = ( + '[[rules.rule_exceptions]]\nrule = "SQBRSQL004"\npath = "models/orders.sql"\n' + 'reason = "A reviewed sample"\n' +) +_IGNORE_ENTRY: str = ( + '[[rules.rule_ignores]]\nrules = ["SQBRSQL"]\npaths = ["models/examples/**"]\n' + 'reason = "Examples keep minimal SQL"\n' +) + @pytest.mark.parametrize( "test_case", @@ -150,6 +160,11 @@ source='[rules.sql_tests]\npipeline_directory = "chains//commerce"\n', expected_error_pattern="must be a normalized path relative to tests/unit", ), + RulesConfigErrorTestCase( + description="non-boolean allow_exceptions", + source='[rules]\nallow_exceptions = "no"\n', + expected_error_pattern="invalid rules config", + ), RulesConfigErrorTestCase( description="unknown SQL test rules key", source='[rules.sql_tests]\npipeline_directory = "pipelines"\nroot = "tests"\n', @@ -168,6 +183,81 @@ def test_given_invalid_rules_config_when_loading_then_raises_clear_error( load_rules_config(project_dir=tmp_path) +@pytest.mark.parametrize( + "test_case", + ( + RulesConfigErrorTestCase( + description="exception entry", + source=f"[rules]\nallow_exceptions = false\n{_EXCEPTION_ENTRY}", + expected_error_pattern=( + r"has 1 \[\[rules\.rule_exceptions\]\] entry, which this project does not " + r"allow; sqlbuild_project\.toml sets \[rules\] allow_exceptions = false" + ), + ), + RulesConfigErrorTestCase( + description="scoped ignore entry", + source=f"[rules]\nallow_exceptions = false\n{_IGNORE_ENTRY}", + expected_error_pattern=r"has 1 \[\[rules\.rule_ignores\]\] entry,", + ), + RulesConfigErrorTestCase( + description="exceptions and scoped ignores", + source=f"[rules]\nallow_exceptions = false\n{_EXCEPTION_ENTRY}{_EXCEPTION_ENTRY}" + f"{_IGNORE_ENTRY}", + expected_error_pattern=( + r"has 2 \[\[rules\.rule_exceptions\]\] entries and " + r"1 \[\[rules\.rule_ignores\]\] entry," + ), + ), + ), + ids=lambda case: case.description, +) +def test_given_exceptions_forbidden_when_loading_then_raises_error_with_exact_setting_help( + tmp_path: Path, + test_case: RulesConfigErrorTestCase, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text(test_case.source, encoding="utf-8") + + with pytest.raises(RulesConfigError, match=test_case.expected_error_pattern) as raised: + load_rules_config(project_dir=tmp_path) + + assert raised.value.help.endswith( + "set this in sqlbuild_project.toml:\n [rules]\n" + " allow_exceptions = true" + ) + + +@pytest.mark.parametrize( + "test_case", + ( + AllowExceptionsConfigTestCase( + description="absent setting allows exceptions", + source=_EXCEPTION_ENTRY + _IGNORE_ENTRY, + expected_allow_exceptions=True, + ), + AllowExceptionsConfigTestCase( + description="true allows exceptions", + source=f"[rules]\nallow_exceptions = true\n{_EXCEPTION_ENTRY}{_IGNORE_ENTRY}", + expected_allow_exceptions=True, + ), + AllowExceptionsConfigTestCase( + description="false with only the project-wide ignore list", + source='[rules]\nallow_exceptions = false\nignore = ["SQBRSQL004"]\n', + expected_allow_exceptions=False, + ), + ), + ids=lambda case: case.description, +) +def test_given_allow_exceptions_setting_when_loading_then_config_records_it( + tmp_path: Path, + test_case: AllowExceptionsConfigTestCase, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text(test_case.source, encoding="utf-8") + + config: RulesConfig = load_rules_config(project_dir=tmp_path) + + assert config.allow_exceptions is test_case.expected_allow_exceptions + + @pytest.mark.parametrize( "test_case", ( diff --git a/website/src/content/docs/docs/concepts/project-configuration.mdx b/website/src/content/docs/docs/concepts/project-configuration.mdx index 8cffb29913..59942120a3 100644 --- a/website/src/content/docs/docs/concepts/project-configuration.mdx +++ b/website/src/content/docs/docs/concepts/project-configuration.mdx @@ -471,6 +471,10 @@ Rules are opt-in. Exact codes activate individual checks and prefixes activate a codes begin with `SQBR`; repository-defined codes begin with `XSQBR`. See [Compiler-integrated Rules](/docs/concepts/rules) for configuration, authoring, and suppressions. +| Key | Default | Description | +| --- | --- | --- | +| `allow_exceptions` | `true` | When `false`, `[[rules.rule_exceptions]]` and `[[rules.rule_ignores]]` entries are configuration errors and inline `-- sqb: ignore` directives are `SQBRSQL000` errors. The project-wide `ignore` list stays allowed. See [Forbidding exceptions](/docs/concepts/rules/findings-and-exceptions#forbidding-exceptions). | + ## Scopes Declaration placement findings are errors by default. Set `enforce_placement = false` to report diff --git a/website/src/content/docs/docs/concepts/rules.mdx b/website/src/content/docs/docs/concepts/rules.mdx index 3e45134ac5..ddaf3b5a6d 100644 --- a/website/src/content/docs/docs/concepts/rules.mdx +++ b/website/src/content/docs/docs/concepts/rules.mdx @@ -201,7 +201,8 @@ This makes the same requirement usable in a terminal, JSON output, CI annotation Intentional departures remain explicit. Exact exceptions and path- or resource-scoped ignores require a reason; exact exceptions are stale-checked so obsolete suppressions do not silently -accumulate. Mandatory compiler correctness cannot be suppressed. +accumulate. Mandatory compiler correctness cannot be suppressed. A project can forbid exceptions, +scoped ignores and inline suppressions entirely with `[rules] allow_exceptions = false`. See [Findings and exceptions](/docs/concepts/rules/findings-and-exceptions) for configuration examples. diff --git a/website/src/content/docs/docs/concepts/rules/findings-and-exceptions.mdx b/website/src/content/docs/docs/concepts/rules/findings-and-exceptions.mdx index 30b43386bf..5928733c12 100644 --- a/website/src/content/docs/docs/concepts/rules/findings-and-exceptions.mdx +++ b/website/src/content/docs/docs/concepts/rules/findings-and-exceptions.mdx @@ -56,6 +56,37 @@ reason = "These intermediate SQL files intentionally preserve upstream columns." `paths` and `selectors` may be combined in one scoped ignore. Both forms require a reason. +## Inline suppressions + +Suppress one SQL Rule finding on the next SQL line with a reasoned comment: + +```sql +-- sqb: ignore SQBRSQL004 because the sample intentionally keeps one row +SELECT customer_id FROM customers LIMIT 1 +``` + +A directive without a reason, or one that no longer matches a finding, is reported as +`SQBRSQL000`. + +## Forbidding exceptions + +Set `allow_exceptions = false` to require that every selected Rule passes everywhere: + +```toml +[rules] +select = ["SQBRSQL", "SQBRMODEL"] +ignore = ["SQBRSQL004"] +allow_exceptions = false # default true +``` + +With exceptions forbidden, any `[[rules.rule_exceptions]]` or `[[rules.rule_ignores]]` entry fails +configuration loading with an error that counts the entries and shows the setting to change, so +`sqb compile` and `sqb rules run` stop before evaluating Rules. Each inline `-- sqb: ignore` +directive becomes a `SQBRSQL000` error at the directive and suppresses nothing, so the finding it +was hiding is reported too. The project-wide `ignore` list stays allowed: it is the adoption switch +for a Rule across the whole project, not a scoped escape hatch. `[[rules.graph_edge_exceptions]]`, +`[[rules.select_star_allow]]` and `[[rules.threshold_overrides]]` are not affected. + ## Mandatory correctness Mandatory compiler correctness is not configurable and cannot be suppressed. A project must first From 818737b78886ef203375227dd0fc5e31d7ca9d7b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 4 Oct 2026 11:29:04 +0100 Subject: [PATCH 3/3] test(rules): include SQBRSQL045 in the unevaluated-rules E2E code list --- .../src/sqlbuild/cli/commands/main/rules/test_unevaluated.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_unevaluated.py b/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_unevaluated.py index 41516c64ab..f00ba2ddad 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_unevaluated.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_unevaluated.py @@ -207,7 +207,7 @@ def test_given_explicit_escape_hatch_when_running_rules_then_honors_policy( "Rules CLI groups selected family", ("rules", "--json", "run", "SQBRSQL"), "findings", - tuple(f"SQBRSQL{number:03}" for number in range(1, 45)), + tuple(f"SQBRSQL{number:03}" for number in range(1, 46)), ), GroupedRulesCase( "suppression precedes grouping",