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/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/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/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/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/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", 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..a1cf537f81 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,38 @@ class WarmTargetEditTestCase: edited_contents: str expected_changed: tuple[str, ...] 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.""" + + 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/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_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/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/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. 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