diff --git a/Cargo.lock b/Cargo.lock index 2b6a5aceb6..6d7ab5141e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4431,7 +4431,6 @@ dependencies = [ "blake3", "fensu-policy", "globset", - "polyglot-sql", "rayon", "regex", "rustc-hash", @@ -4445,6 +4444,7 @@ dependencies = [ "sqlbuild-scopes", "sqlbuild-sqltext", "sqlclean", + "sqlquack-syntax", "stacker", "tempfile", "toml", diff --git a/crates/sqlbuild-analysis/src/sql_tree/_helpers/rules_tree_parser.rs b/crates/sqlbuild-analysis/src/sql_tree/_helpers/rules_tree_parser.rs index b330c0a39c..db8eb603d9 100644 --- a/crates/sqlbuild-analysis/src/sql_tree/_helpers/rules_tree_parser.rs +++ b/crates/sqlbuild-analysis/src/sql_tree/_helpers/rules_tree_parser.rs @@ -1,10 +1,7 @@ //! `rules_tree_parser`: the parser that reads a dialect's SQL for rules. -use sqlquack_syntax::Dialect; - -use crate::sql_tree::constants::{ - DUCKDB_DIALECTS, POSTGRES_DIALECTS, SNOWFLAKE_DIALECT, SQLITE_DIALECT, -}; +use crate::sql_tree::constants::POSTGRES_DIALECTS; +use crate::sql_tree::main::shared_dialect::shared_dialect; use crate::sql_tree::models::TreeParser; /// The parser for `dialect`, or `None`; add a dialect (one line) only once its parser passes the depth probe with an empty allowlist. @@ -12,12 +9,5 @@ pub(crate) fn rules_tree_parser(dialect: &str) -> Option { if POSTGRES_DIALECTS.contains(&dialect) { return Some(TreeParser::Postgres); } - if DUCKDB_DIALECTS.contains(&dialect) { - return Some(TreeParser::Shared(Dialect::DuckDB)); - } - match dialect { - SNOWFLAKE_DIALECT => Some(TreeParser::Shared(Dialect::Snowflake)), - SQLITE_DIALECT => Some(TreeParser::Shared(Dialect::SQLite)), - _ => None, - } + shared_dialect(dialect).map(TreeParser::Shared) } diff --git a/crates/sqlbuild-analysis/src/sql_tree/main/mod.rs b/crates/sqlbuild-analysis/src/sql_tree/main/mod.rs index 0c71a2f7d5..924d435a79 100644 --- a/crates/sqlbuild-analysis/src/sql_tree/main/mod.rs +++ b/crates/sqlbuild-analysis/src/sql_tree/main/mod.rs @@ -1,3 +1,4 @@ //! Entry points. +pub mod shared_dialect; pub mod sql_tree_nodes; diff --git a/crates/sqlbuild-analysis/src/sql_tree/main/shared_dialect.rs b/crates/sqlbuild-analysis/src/sql_tree/main/shared_dialect.rs new file mode 100644 index 0000000000..5a7e95518e --- /dev/null +++ b/crates/sqlbuild-analysis/src/sql_tree/main/shared_dialect.rs @@ -0,0 +1,22 @@ +//! `shared_dialect`: the grammar of SQLQuack's shared parser that reads a dialect's SQL. + +use sqlquack_syntax::Dialect; + +use crate::sql_tree::constants::{ + DUCKDB_DIALECTS, POSTGRES_DIALECTS, SNOWFLAKE_DIALECT, SQLITE_DIALECT, +}; + +/// The shared-parser grammar for `dialect`, or `None` when the shared parser reads none of it. +pub fn shared_dialect(dialect: &str) -> Option { + if DUCKDB_DIALECTS.contains(&dialect) { + return Some(Dialect::DuckDB); + } + if POSTGRES_DIALECTS.contains(&dialect) { + return Some(Dialect::PostgreSQL); + } + match dialect { + SNOWFLAKE_DIALECT => Some(Dialect::Snowflake), + SQLITE_DIALECT => Some(Dialect::SQLite), + _ => None, + } +} diff --git a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/analysis.rs b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/analysis.rs index eaabced465..e7fd6c7603 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/analysis.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/analysis.rs @@ -6,6 +6,7 @@ use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyModule, PyModuleMethods, PyRes use pyo3::types::{PyBool, PyDict, PyDictMethods, PyString, PyTuple, PyTupleMethods}; use pyo3::{IntoPyObject, pyfunction, wrap_pyfunction}; use sqlbuild_host::entries::models::ExecutorHost; +use sqlbuild_rules::lint_text::main::counts_toward_exit::counts_toward_exit; use crate::bindings::_helpers::boundary::python_calls::{ Status, call_with, imported, keywords, within, @@ -546,7 +547,7 @@ impl<'py> Driver<'_, 'py> { } } -/// The core diagnostics followed by one rule-phase error per rule finding. +/// The core diagnostics followed by one rule-phase error per rule finding; lint-unavailable ones only warn. fn with_rule_diagnostics<'py>( py: Python<'py>, core: &Bound<'py, PyTuple>, @@ -554,6 +555,7 @@ fn with_rule_diagnostics<'py>( ) -> PyResult> { let phase: Bound<'py, PyAny> = enum_member(py, RecordId::DiagnosticPhase, "RULE")?; let severity: Bound<'py, PyAny> = enum_member(py, RecordId::DiagnosticSeverity, "ERROR")?; + let unavailable: Bound<'py, PyAny> = enum_member(py, RecordId::DiagnosticSeverity, "WARNING")?; let fix_note: Bound<'py, PyAny> = PyString::new(py, RULE_FIX_AVAILABLE_NOTE).into_any(); let mut diagnostics: Vec> = core.iter().collect(); for fault in native_field(rules_result, "findings")?.try_iter()? { @@ -568,7 +570,13 @@ fn with_rule_diagnostics<'py>( RecordId::CompilerDiagnostic, &[ ("phase", phase.clone()), - ("severity", severity.clone()), + ( + "severity", + match counts_toward_exit(&native_field(&fault, "code")?.extract::()?) { + true => severity.clone(), + false => unavailable.clone(), + }, + ), ("code", native_field(&fault, "code")?), ("message", native_field(&fault, "message")?), ("path", native_field(&fault, "path")?), diff --git a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_pipeline.rs b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_pipeline.rs index 59da81f65f..f8340fe4d9 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_pipeline.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_pipeline.rs @@ -1,9 +1,10 @@ //! The compile-and-plan pipeline of execution commands: compile, rules gate, then planning. use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyModule, PyModuleMethods, PyResult, Python}; -use pyo3::types::PyDict; +use pyo3::types::{PyDict, PyTuple}; use pyo3::{IntoPyObject, PyErr, pyfunction, wrap_pyfunction}; use sqlbuild_host::entries::models::ExecutorHost; +use sqlbuild_rules::lint_text::main::counts_toward_exit::counts_toward_exit; use sqlbuild_rules::rule_results::main::default_rules_notice::default_rules_notice; use crate::bindings::_helpers::boundary::python_calls::{KeywordArguments, keywords}; @@ -135,10 +136,17 @@ fn run_rules_gate<'py>( let message: String = rules_progress(elapsed, &result, true)?; on_progress.call1((message,))?; } - let findings: Bound<'py, PyAny> = native_field(&result, "findings")?; - if !findings.is_truthy()? { + let mut failing: Vec> = Vec::new(); + for finding in native_field(&result, "findings")?.try_iter()? { + let finding: Bound<'py, PyAny> = finding?; + if counts_toward_exit(&native_field(&finding, "code")?.extract::()?) { + failing.push(finding); + } + } + if failing.is_empty() { return Ok(()); } + let findings: Bound<'py, PyAny> = PyTuple::new(py, failing)?.into_any(); let summary: Bound<'py, PyAny> = new_record_with( py, RecordId::RulesResult, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/plan_command/rules.rs b/crates/sqlbuild-python/src/bindings/_helpers/plan_command/rules.rs index 19fc6051ec..36081f97c2 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/plan_command/rules.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/plan_command/rules.rs @@ -5,6 +5,7 @@ use pyo3::prelude::{ }; use pyo3::types::{PyDict, PyDictMethods, PyTuple}; use pyo3::wrap_pyfunction; +use sqlbuild_rules::lint_text::main::counts_toward_exit::counts_toward_exit; use sqlbuild_rules::rule_catalogue::main::render_rule_list::render_rule_list; use sqlbuild_rules::rule_catalogue::models::RuleListing; @@ -246,7 +247,12 @@ fn run_rule_selection( native_field(request, "json_output")?.is_truthy()?, )?; print_line(py, &text, false)?; - let findings: usize = native_field(&result, "findings")?.len()?; + let mut findings: usize = 0; + for finding in native_field(&result, "findings")?.try_iter()? { + if counts_toward_exit(&native_field(&finding?, "code")?.extract::()?) { + findings += 1; + } + } if findings > 0 { print_line( py, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/rules/sql_rules.rs b/crates/sqlbuild-python/src/bindings/_helpers/rules/sql_rules.rs index e211ebcefa..a404881bf2 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/rules/sql_rules.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/rules/sql_rules.rs @@ -13,6 +13,7 @@ use pyo3::prelude::{ use pyo3::types::{PyBytes, PyDict, PyDictMethods, PyString, PyTuple}; use pyo3::{IntoPyObjectExt, pyfunction, wrap_pyfunction}; use sqlbuild_core::text::main::python_splitlines::python_splitlines; +use sqlbuild_rules::lint_text::main::counts_toward_exit::counts_toward_exit; use sqlbuild_rules::rule_cache::main::file_rule_key::file_rule_key; use sqlbuild_rules::rule_results::main::duplicate_literal_hints::duplicate_literal_hints; use sqlbuild_rules::rule_results::models::RuleFinding; @@ -670,7 +671,7 @@ fn lint_expansion_context( }) } -/// The findings of the selected SQL rules' violations; an engine failure leaves each unevaluated. +/// The selected SQL rules' findings and lint-unavailable warnings; an engine failure leaves each rule unevaluated. fn violation_findings( run: &SqlRun<'_, '_>, violations: &[(PathBuf, LintViolationRow)], @@ -686,6 +687,8 @@ fn violation_findings( let code: &str = &violation.code; if code == SQL_RULE_SUPPRESSION_CODE || run.codes.iter().any(|known| known == code) { evaluated.push(lint_finding((path, violation), &resolved_dir, model_paths)); + } else if !counts_toward_exit(code) { + evaluated.push(lint_finding((path, violation), &resolved_dir, &no_paths)); } else if code == engine_error { let finding: RuleFinding = lint_finding((path, violation), &resolved_dir, &no_paths); for rule_code in run.codes { diff --git a/crates/sqlbuild-rules/Cargo.toml b/crates/sqlbuild-rules/Cargo.toml index 535ee3687a..1070397264 100644 --- a/crates/sqlbuild-rules/Cargo.toml +++ b/crates/sqlbuild-rules/Cargo.toml @@ -11,7 +11,7 @@ rust-version.workspace = true blake3.workspace = true fensu-policy.workspace = true globset.workspace = true -polyglot-sql.workspace = true +sqlquack-syntax.workspace = true rayon.workspace = true regex.workspace = true rustc-hash.workspace = true diff --git a/crates/sqlbuild-rules/src/constants.rs b/crates/sqlbuild-rules/src/constants.rs index 37b4e3f953..e3f4b5ad79 100644 --- a/crates/sqlbuild-rules/src/constants.rs +++ b/crates/sqlbuild-rules/src/constants.rs @@ -55,3 +55,6 @@ pub(crate) const ALLOWED_QUALIFIERS_OPTION: &str = "allowed_qualifiers"; pub(crate) const ALLOWED_PATHS_KEY: &str = "paths"; /// The published layers when a project does not configure them. pub(crate) const DEFAULT_PUBLISHED_LAYERS: [&str; 2] = ["mart", "mart_v"]; +/// What to do when SQLBuild does not parse a project's dialect for built-in rules. +pub(crate) const RULES_UNAVAILABLE_REMEDIATION: &str = + "These rules did not run. Review this file by hand until built-in rules support this dialect."; diff --git a/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs b/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs index 64d11d4ead..d76c61a0cb 100644 --- a/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs @@ -1,6 +1,7 @@ use crate::configuration::main::validate as config; use crate::constants::{API_VERSION, NATIVE_BUILD_IDENTITY, RELATION_EXCEPTION_RULES}; use crate::engine::_helpers::cache::{Cache, PendingBucketWrite, RuleCacheEntry}; +use crate::lint_text::constants::LINT_UNAVAILABLE_CODE; use crate::models::{ EvaluateRequest, EvaluateResponse, Fault, Model, RuleException, RuleMetadata, RuleScope, RulesCodeGrammar, RulesConfig, @@ -580,11 +581,15 @@ pub(crate) fn finalize_findings( apply_fault_policy(&request, evaluated_codes, findings) } +/// Apply rule exceptions to `faults`; lint-unavailable warnings report our gap, not a rule, so they pass through unsuppressed. fn apply_fault_policy( request: &EvaluateRequest, evaluated_codes: &[String], faults: Vec, ) -> Result, String> { + let (unavailable, faults): (Vec, Vec) = faults + .into_iter() + .partition(|fault| fault.code == LINT_UNAVAILABLE_CODE); let unevaluated: BTreeSet<_> = faults .iter() .filter(|fault| fault.unevaluated) @@ -641,6 +646,7 @@ fn apply_fault_policy( )); fault }) + .chain(unavailable) .collect()) } diff --git a/crates/sqlbuild-rules/src/engine/tests/test_empty_input_tests.rs b/crates/sqlbuild-rules/src/engine/tests/test_empty_input_tests.rs index 3c9f34d22b..92657d96c2 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_empty_input_tests.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_empty_input_tests.rs @@ -14,6 +14,7 @@ const NO_ROWS_ASSERTION: (&str, &str) = ( "__assert__empty_inputs_produce_no_rows", "SELECT 1 AS unexpected_row FROM __ref(\"orders\")", ); +const UNAVAILABLE_MESSAGE: &str = "L007 tests/unit/test_orders__empty_inputs_produce_no_rows.sql: SQLBuild cannot parse mssql SQL for built-in rules yet, so these rules were not evaluated: SQBRTEST203"; const FLAGGED_MESSAGE: &str = "SQBRTEST203 tests/unit/test_orders__empty_inputs_produce_no_rows.sql: unit test block 1 (\"orders__empty_inputs_produce_no_rows\") mocks only empty inputs and asserts only that no rows are produced"; #[test] @@ -419,7 +420,7 @@ fn given_cached_minimum_tests_when_filler_becomes_real_test_then_model_cache_is_ } #[test] -fn given_sql_server_conversions_when_evaluating_empty_input_rule_then_type_arguments_are_not_calls() +fn given_sql_server_conversions_when_evaluating_empty_input_rule_then_reports_the_dialect_unavailable() -> Result<(), String> { let project_dir = TempDir::new().map_err(|error| error.to_string())?; let empty_orders = ( @@ -428,7 +429,7 @@ fn given_sql_server_conversions_when_evaluating_empty_input_rule_then_type_argum ); let test_cases = [ test_types::EmptyInputTestRuleTestCase { - description: "a CONVERT to a parameterized type under a false predicate is an empty mock", + description: "a CONVERT to a parameterized type under a false predicate is an empty mock, reported unavailable until SQLBuild parses the dialect", test: helpers::empty_input_test_fact( "orders__empty_inputs_produce_no_rows", &[( @@ -439,10 +440,10 @@ fn given_sql_server_conversions_when_evaluating_empty_input_rule_then_type_argum &[NO_ROWS_ASSERTION], ), allowed_tests: json!([]), - expected_messages: &[FLAGGED_MESSAGE], + expected_messages: &[UNAVAILABLE_MESSAGE], }, test_types::EmptyInputTestRuleTestCase { - description: "a CONVERT with a style argument is an empty mock", + description: "a CONVERT with a style argument is an empty mock, reported unavailable until SQLBuild parses the dialect", test: helpers::empty_input_test_fact( "orders__empty_inputs_produce_no_rows", &[( @@ -453,10 +454,10 @@ fn given_sql_server_conversions_when_evaluating_empty_input_rule_then_type_argum &[NO_ROWS_ASSERTION], ), allowed_tests: json!([]), - expected_messages: &[FLAGGED_MESSAGE], + expected_messages: &[UNAVAILABLE_MESSAGE], }, test_types::EmptyInputTestRuleTestCase { - description: "a TRY_CONVERT to a parameterized type keeps a bare existence check", + description: "a TRY_CONVERT to a parameterized type keeps a bare existence check, reported unavailable until SQLBuild parses the dialect", test: helpers::empty_input_test_fact( "orders__empty_inputs_produce_no_rows", &[empty_orders], @@ -467,10 +468,10 @@ fn given_sql_server_conversions_when_evaluating_empty_input_rule_then_type_argum )], ), allowed_tests: json!([]), - expected_messages: &[FLAGGED_MESSAGE], + expected_messages: &[UNAVAILABLE_MESSAGE], }, test_types::EmptyInputTestRuleTestCase { - description: "a function call in a CONVERT value argument still counts", + description: "a function call in a CONVERT value argument still counts, reported unavailable until SQLBuild parses the dialect", test: helpers::empty_input_test_fact( "orders__empty_inputs_produce_no_rows", &[( @@ -481,7 +482,7 @@ fn given_sql_server_conversions_when_evaluating_empty_input_rule_then_type_argum &[NO_ROWS_ASSERTION], ), allowed_tests: json!([]), - expected_messages: &[], + expected_messages: &[UNAVAILABLE_MESSAGE], }, ]; diff --git a/crates/sqlbuild-rules/src/engine/tests/test_evaluation.rs b/crates/sqlbuild-rules/src/engine/tests/test_evaluation.rs index c46bd6e88e..80c3f872c3 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_evaluation.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_evaluation.rs @@ -29,11 +29,13 @@ fn given_dependency_tokens_when_classifying_imports_then_preserves_dialect_and_o expected_messages: &[], }, test_types::DependencyTokenTestCase { - description: "backtick quoting and comment-separated source arguments", + description: "backtick quoting and comment-separated source arguments, reported unavailable until SQLBuild parses the dialect", expected_columns: &[], dialect: "bigquery", query_sql: "WITH imported AS (SELECT * FROM __source /* ignored */ (`Orders`)) SELECT order_id FROM imported", - expected_messages: &[], + expected_messages: &[ + "SQLBuild cannot parse bigquery SQL for built-in rules yet, so these rules were not evaluated: SQBRMODEL101", + ], }, test_types::DependencyTokenTestCase { description: "dollar strings do not contribute spurious calls", @@ -518,7 +520,8 @@ fn given_snowflake_expression_when_evaluating_rules_then_uses_project_dialect() "CAST(amounts AS ARRAY(NUMBER(38, 10))) AS amounts, ", "OBJECT_CONSTRUCT_KEEP_NULL('group_id', group_id) AS details, ", "FROM __table_fn(\"inventory_items\")(42)" - ), + ) + .to_owned(), expected_code: "SQBRCONTRACT101", }]; @@ -550,26 +553,29 @@ fn given_dialect_quoting_around_table_functions_when_evaluating_rules_then_model query_sql: concat!( "SELECT 'it\\'s (' AS label, item_id ", "FROM __table_fn(\"inventory_items\")(42)" - ), + ) + .to_owned(), expected_code: "SQBRCONTRACT101", }, test_types::DialectEvaluationTestCase { - description: "BigQuery backslash-escaped quote inside table function arguments", + description: "BigQuery backslash-escaped quote inside table function arguments, reported unavailable until SQLBuild parses the dialect", dialect: "bigquery", query_sql: concat!( "SELECT item_id ", "FROM __table_fn(\"inventory_items\", 'a\\')')(42)" - ), - expected_code: "SQBRCONTRACT101", + ) + .to_owned(), + expected_code: "L007", }, test_types::DialectEvaluationTestCase { - description: "BigQuery backtick identifier containing a parenthesis in arguments", + description: "BigQuery backtick identifier containing a parenthesis in arguments, reported unavailable until SQLBuild parses the dialect", dialect: "bigquery", query_sql: concat!( "SELECT item_id ", "FROM __table_fn(\"inventory_items\", `batch)size`)(42)" - ), - expected_code: "SQBRCONTRACT101", + ) + .to_owned(), + expected_code: "L007", }, ]; @@ -593,12 +599,27 @@ fn given_dialect_quoting_around_table_functions_when_evaluating_rules_then_model #[test] fn given_unparseable_model_when_evaluating_then_retains_named_rule_fault() -> Result<(), String> { let project_dir = TempDir::new().map_err(|error| error.to_string())?; - let test_cases = [test_types::DialectEvaluationTestCase { - description: "invalid query is missing evidence rather than a passing contract", - dialect: "duckdb", - query_sql: "SELECT (", - expected_code: "SQBRCONTRACT101", - }]; + let deep_sql = format!("SELECT {}1 AS order_id FROM orders", "1 + ".repeat(50_000)); + let test_cases = [ + test_types::DialectEvaluationTestCase { + description: "invalid query is missing evidence rather than a passing contract", + dialect: "duckdb", + query_sql: "SELECT (".to_owned(), + expected_code: "SQBRCONTRACT101", + }, + test_types::DialectEvaluationTestCase { + description: "a DuckDB expression deeper than the AST limit is refused, not a crash", + dialect: "duckdb", + query_sql: deep_sql.clone(), + expected_code: "SQBRCONTRACT101", + }, + test_types::DialectEvaluationTestCase { + description: "a Snowflake expression deeper than the AST limit is refused, not a crash", + dialect: "snowflake", + query_sql: deep_sql, + expected_code: "SQBRCONTRACT101", + }, + ]; for test_case in test_cases { let config = json!({"select": ["SQBRCONTRACT101"], "cache": {"enabled": false}}); let mut request: Value = serde_json::from_str(&helpers::request(&project_dir, &config)) @@ -641,19 +662,19 @@ fn given_comment_apostrophes_when_generic_fallback_normalizes_rules_sql_then_mod test_types::DialectEvaluationTestCase { description: "line comment apostrophe before a colon path", dialect: "postgres", - query_sql: "-- don't drop the customer path\nSELECT payload:customer_id AS customer_id FROM orders", + query_sql: "-- don't drop the customer path\nSELECT payload:customer_id AS customer_id FROM orders".to_owned(), expected_code: "SQBRCONTRACT101", }, test_types::DialectEvaluationTestCase { description: "block comment apostrophe before a colon path", dialect: "postgres", - query_sql: "/* customer's path */ SELECT payload:customer_id AS customer_id FROM orders", + query_sql: "/* customer's path */ SELECT payload:customer_id AS customer_id FROM orders".to_owned(), expected_code: "SQBRCONTRACT101", }, test_types::DialectEvaluationTestCase { description: "colon inside a line comment stays commented", dialect: "postgres", - query_sql: "SELECT payload:customer_id AS customer_id -- note: it's nested\nFROM orders", + query_sql: "SELECT payload:customer_id AS customer_id -- note: it's nested\nFROM orders".to_owned(), expected_code: "SQBRCONTRACT101", }, ]; diff --git a/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs b/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs index b03abed424..fca1058f78 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs @@ -32,13 +32,13 @@ fn given_dialect_sql_when_reading_query_facts_then_findings_match_the_authored_s expected_faults: &[], }, QueryFactsTestCase { - description: "an apostrophe in a BigQuery # comment opens no string", + description: "an apostrophe in a BigQuery # comment opens no string, reported unavailable until SQLBuild parses the dialect", dialect: "bigquery", query_sql: "WITH imported AS (\n # don't read the archive\n SELECT * FROM __ref(\"orders\")\n)\nSELECT order_id FROM imported", select: IMPORT_RULES, enum_columns: &[], columns: json!([]), - expected_faults: &[], + expected_faults: &[("L007", 1, 1, "cannot parse bigquery SQL for built-in rules")], }, QueryFactsTestCase { description: "a DuckDB FROM-first import CTE star is exempt without a SELECT keyword", @@ -115,6 +115,48 @@ fn given_dialect_sql_when_reading_query_facts_then_findings_match_the_authored_s columns: json!([]), expected_faults: &[("SQBRDECLARATION102", 1, 35, "3 < 7")], }, + QueryFactsTestCase { + description: "an anchorless comparison nested in another keeps its own position", + dialect: "snowflake", + query_sql: "SELECT order_id FROM orders\nWHERE CASE WHEN 3 = 2 THEN 10 ELSE 20 END = 1", + select: NUMERIC_RULE, + enum_columns: &[], + columns: json!([]), + expected_faults: &[("SQBRDECLARATION102", 2, 17, "3 = 2")], + }, + QueryFactsTestCase { + description: "a Snowflake LAST_DAY date part compared with an enum column is no enum literal", + dialect: "snowflake", + query_sql: "SELECT order_id FROM orders WHERE LAST_DAY(placed_at, 'month') = status", + select: ENUM_RULE, + enum_columns: STATUS, + columns: json!([]), + expected_faults: &[], + }, + QueryFactsTestCase { + description: "DuckDB literal comparisons on separate lines each keep their own position", + dialect: "duckdb", + query_sql: "SELECT order_id FROM orders\nWHERE 3 < 7\n AND 5 > 2", + select: NUMERIC_RULE, + enum_columns: &[], + columns: json!([]), + expected_faults: &[ + ("SQBRDECLARATION102", 2, 7, "3 < 7"), + ("SQBRDECLARATION102", 3, 7, "5 > 2"), + ], + }, + QueryFactsTestCase { + description: "Snowflake literal comparisons on separate lines each keep their own position", + dialect: "snowflake", + query_sql: "SELECT order_id FROM orders\nWHERE 3 < 7\n AND 5 > 2", + select: NUMERIC_RULE, + enum_columns: &[], + columns: json!([]), + expected_faults: &[ + ("SQBRDECLARATION102", 2, 7, "3 < 7"), + ("SQBRDECLARATION102", 3, 7, "5 > 2"), + ], + }, QueryFactsTestCase { description: "unaliased later union branches keep their cast types", dialect: "duckdb", @@ -181,22 +223,22 @@ fn given_dialect_sql_when_reading_query_facts_then_findings_match_the_authored_s )], }, QueryFactsTestCase { - description: "a BigQuery double-quoted string compared with an enum column", + description: "a BigQuery double-quoted string compared with an enum column, reported unavailable until SQLBuild parses the dialect", dialect: "bigquery", query_sql: "SELECT order_id FROM orders WHERE status = \"active\"", select: ENUM_RULE, enum_columns: STATUS, columns: json!([]), - expected_faults: &[("SQBRDECLARATION101", 1, 35, "")], + expected_faults: &[("L007", 1, 1, "cannot parse bigquery SQL for built-in rules")], }, QueryFactsTestCase { - description: "a BigQuery triple-quoted string compared with an enum column", + description: "a BigQuery triple-quoted string compared with an enum column, reported unavailable until SQLBuild parses the dialect", dialect: "bigquery", query_sql: "SELECT order_id FROM orders WHERE status = '''active'''", select: ENUM_RULE, enum_columns: STATUS, columns: json!([]), - expected_faults: &[("SQBRDECLARATION101", 1, 35, "")], + expected_faults: &[("L007", 1, 1, "cannot parse bigquery SQL for built-in rules")], }, QueryFactsTestCase { description: "a Postgres escape string compared with an enum column", @@ -226,32 +268,22 @@ fn given_dialect_sql_when_reading_query_facts_then_findings_match_the_authored_s expected_faults: &[("SQBRDECLARATION102", 1, 35, "amount == 42")], }, QueryFactsTestCase { - description: "a parenthesized T-SQL TOP argument is not projection", + description: "a parenthesized T-SQL TOP argument is not projection, reported unavailable until SQLBuild parses the dialect", dialect: "mssql", query_sql: "SELECT TOP (5) amount + 1 AS amount FROM orders", select: OUTPUT_TYPE_RULE, enum_columns: &[], columns: json!([{"name": "amount", "type": "INTEGER", "type_proven": true}]), - expected_faults: &[( - "SQBRCONTRACT105", - 1, - 16, - "calculated without an outer explicit cast", - )], + expected_faults: &[("L007", 1, 1, "cannot parse mssql SQL for built-in rules")], }, QueryFactsTestCase { - description: "a T-SQL TOP PERCENT clause is not projection", + description: "a T-SQL TOP PERCENT clause is not projection, reported unavailable until SQLBuild parses the dialect", dialect: "mssql", query_sql: "SELECT TOP 5 PERCENT amount + 1 AS amount FROM orders", select: OUTPUT_TYPE_RULE, enum_columns: &[], columns: json!([{"name": "amount", "type": "INTEGER", "type_proven": true}]), - expected_faults: &[( - "SQBRCONTRACT105", - 1, - 22, - "calculated without an outer explicit cast", - )], + expected_faults: &[("L007", 1, 1, "cannot parse mssql SQL for built-in rules")], }, QueryFactsTestCase { description: "a placeholder in relation position still parses", diff --git a/crates/sqlbuild-rules/src/engine/tests/test_types.rs b/crates/sqlbuild-rules/src/engine/tests/test_types.rs index 24d53f6a12..7bd4f88f21 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_types.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_types.rs @@ -35,7 +35,7 @@ pub(crate) struct DeclarationScopeTestCase { pub(crate) struct DialectEvaluationTestCase { pub(crate) description: &'static str, pub(crate) dialect: &'static str, - pub(crate) query_sql: &'static str, + pub(crate) query_sql: String, pub(crate) expected_code: &'static str, } diff --git a/crates/sqlbuild-rules/src/lint_text/_helpers/sql_diagnostics.rs b/crates/sqlbuild-rules/src/lint_text/_helpers/sql_diagnostics.rs index d7ddf43c3c..a2d51c33d3 100644 --- a/crates/sqlbuild-rules/src/lint_text/_helpers/sql_diagnostics.rs +++ b/crates/sqlbuild-rules/src/lint_text/_helpers/sql_diagnostics.rs @@ -1,6 +1,7 @@ //! Native SQL lint responses mapped back onto authored source through expansion passes. use std::borrow::Cow; +use std::collections::HashSet; use std::sync::LazyLock; use regex::Regex; @@ -10,7 +11,8 @@ use crate::lint_text::_helpers::sites::backtick_identifiers; use crate::lint_text::_helpers::sites::{byte_offset, compiled, lint_policy, site_end}; use crate::lint_text::constants::{ GENERATED_FIX, GENERATED_SQL_SUFFIX, HARNESS_CTE_PREFIXES, INVALID_DIAGNOSTIC, - INVALID_FIX_EDIT, INVALID_FIX_OBJECT, INVALID_FIX_REASON, LINT_API_VERSION, LONG_LITERAL_CODE, + INVALID_FIX_EDIT, INVALID_FIX_OBJECT, INVALID_FIX_REASON, INVALID_UNAVAILABLE, + LINT_API_VERSION, LINT_UNAVAILABLE_CODE, LINT_UNAVAILABLE_REMEDIATION, LONG_LITERAL_CODE, MACRO_MIXED_QUALIFICATION_REMEDIATION, MISSING_DIAGNOSTICS, MIXED_QUALIFICATION_CODE, NATIVE_LINT_ERROR_CODE, NON_CONTIGUOUS_FIX, NON_OBJECT_RESPONSE, NOT_AN_OBJECT, PARSE_ERROR_POSITION_PATTERN, PARSE_FAILURE_PREFIX, PARSE_FAILURE_REMEDIATION, @@ -109,8 +111,15 @@ pub(crate) fn map_offset(offset: i64, passes: &[Vec]) -> (i64, bo (current, generated) } -/// The diagnostics list of one engine response, or the error it reports. -pub(crate) fn response_diagnostics(response: &Value) -> Result, String> { +/// The diagnostics list of one engine response, or why its dialect could not be linted. +#[derive(Clone, Debug, PartialEq)] +pub(crate) enum LintResult { + Diagnostics(Vec), + Unavailable(String), +} + +/// The result of one engine response, or the error it reports. +pub(crate) fn response_diagnostics(response: &Value) -> Result { let Some(payload) = response.as_object() else { return Err(NON_OBJECT_RESPONSE.to_string()); }; @@ -123,9 +132,67 @@ pub(crate) fn response_diagnostics(response: &Value) -> Result, Strin if integer(result.get("version")) != Some(LINT_API_VERSION) { return Err(UNSUPPORTED_VERSION.to_string()); } - match result.get("diagnostics") { - Some(Value::Array(diagnostics)) => Ok(diagnostics.clone()), - _ => Err(MISSING_DIAGNOSTICS.to_string()), + match (result.get("unavailable"), result.get("diagnostics")) { + (Some(Value::String(reason)), _) => Ok(LintResult::Unavailable(reason.clone())), + (Some(_), _) => Err(INVALID_UNAVAILABLE.to_string()), + (None, Some(Value::Array(diagnostics))) => Ok(LintResult::Diagnostics(diagnostics.clone())), + (None, _) => Err(MISSING_DIAGNOSTICS.to_string()), + } +} + +/// One body's authored diagnostics from its engine response, or the warning that it was not linted. +pub(crate) fn body_diagnostics( + response: &Result, + (body, omitted): (&LintSqlBody, Option), + file: &AuthoredFile<'_>, +) -> Vec { + let raw: Vec = match response { + Ok(LintResult::Diagnostics(raw)) => raw.clone(), + Ok(LintResult::Unavailable(reason)) => { + return vec![unavailable_warning(reason, body, file)]; + } + Err(message) => return vec![parse_failure(message, body, file)], + }; + without_generated_frame(raw, body, omitted) + .iter() + .filter_map(|item| match authored_diagnostic(item, body, file) { + Ok(diagnostic) => diagnostic, + Err(message) => Some(parse_failure(&message, body, file)), + }) + .collect() +} + +/// The diagnostics with only each file's first lint-unavailable warning. +pub(crate) fn unavailable_once_per_file( + diagnostics: Vec, +) -> Vec { + let mut warned: HashSet = HashSet::new(); + diagnostics + .into_iter() + .filter(|diagnostic| { + diagnostic.code != LINT_UNAVAILABLE_CODE || warned.insert(diagnostic.file) + }) + .collect() +} + +/// The warning that a body's SQL was not linted because the engine cannot parse its dialect. +fn unavailable_warning( + reason: &str, + body: &LintSqlBody, + file: &AuthoredFile<'_>, +) -> AuthoredDiagnostic { + let (line, column): (i64, i64) = file.position(body.body_start); + AuthoredDiagnostic { + file: body.file, + line, + column, + code: LINT_UNAVAILABLE_CODE.to_string(), + message: reason.to_string(), + fault: false, + end: None, + remediation: LINT_UNAVAILABLE_REMEDIATION.to_string(), + fix: None, + fix_unavailable_reason: None, } } diff --git a/crates/sqlbuild-rules/src/lint_text/constants.rs b/crates/sqlbuild-rules/src/lint_text/constants.rs index ea4e21a0bf..adc92a97cb 100644 --- a/crates/sqlbuild-rules/src/lint_text/constants.rs +++ b/crates/sqlbuild-rules/src/lint_text/constants.rs @@ -106,6 +106,10 @@ pub(crate) const CTE_ARGUMENT: &str = "__cte"; /// The help for mixed qualification in a SQL macro body, where slot reads are qualified through an alias. pub(crate) const MACRO_MIXED_QUALIFICATION_REMEDIATION: &str = "Use one consistent qualification style in this query scope. A plain slot read such as `@rel.column` counts as unqualified; to qualify it, read through an alias: `FROM @rel AS s`, then `s.@rel.column`."; pub(crate) const NATIVE_LINT_ERROR_CODE: &str = "L003"; +/// The warning code for SQL the native lint engine cannot parse in the project's dialect. +pub(crate) const LINT_UNAVAILABLE_CODE: &str = "L007"; +pub(crate) const LINT_UNAVAILABLE_REMEDIATION: &str = + "This SQL was not linted. Review it by hand until SQL lint supports this dialect."; pub(crate) const PARSE_ERROR_POSITION_PATTERN: &str = r"^Parse error at line (?P\d+), column (?P\d+):"; pub(crate) const HARNESS_CTE_PREFIXES: [&str; 13] = [ @@ -152,6 +156,8 @@ pub(crate) const NOT_AN_OBJECT: &str = "native lint diagnostic must be an object pub(crate) const NON_OBJECT_RESPONSE: &str = "native lint engine returned a non-object response"; pub(crate) const UNSUPPORTED_VERSION: &str = "native lint engine returned an unsupported response version"; +pub(crate) const INVALID_UNAVAILABLE: &str = + "native lint response has a non-string unavailable reason"; pub(crate) const MISSING_DIAGNOSTICS: &str = "native lint response is missing a diagnostics list"; pub(crate) const INVALID_BATCH: &str = "native lint engine returned an invalid batch response"; pub(crate) const NON_CONTIGUOUS_FIX: &str = diff --git a/crates/sqlbuild-rules/src/lint_text/main/counts_toward_exit.rs b/crates/sqlbuild-rules/src/lint_text/main/counts_toward_exit.rs new file mode 100644 index 0000000000..6fcb69d909 --- /dev/null +++ b/crates/sqlbuild-rules/src/lint_text/main/counts_toward_exit.rs @@ -0,0 +1,8 @@ +//! `counts_toward_exit`: whether a lint finding can make a command fail. + +use crate::lint_text::constants::LINT_UNAVAILABLE_CODE; + +/// Whether a finding with `code` counts toward a command's exit status; SQL lint being unavailable for a dialect never does. +pub fn counts_toward_exit(code: &str) -> bool { + code != LINT_UNAVAILABLE_CODE +} diff --git a/crates/sqlbuild-rules/src/lint_text/main/lint_sql_bodies.rs b/crates/sqlbuild-rules/src/lint_text/main/lint_sql_bodies.rs index be43f45778..5eefd1e8f0 100644 --- a/crates/sqlbuild-rules/src/lint_text/main/lint_sql_bodies.rs +++ b/crates/sqlbuild-rules/src/lint_text/main/lint_sql_bodies.rs @@ -5,12 +5,12 @@ use std::collections::HashMap; use serde_json::Value; use crate::lint_text::_helpers::sql_diagnostics::{ - AuthoredFile, authored_diagnostic, parse_failure, response_diagnostics, without_generated_frame, + AuthoredFile, LintResult, body_diagnostics, response_diagnostics, unavailable_once_per_file, }; use crate::lint_text::_helpers::sql_requests::{lint_payload, omitted_select, run_batch}; use crate::lint_text::models::{AuthoredDiagnostic, LintSqlBody, LintSqlOptions}; -/// Lint every body once per distinct request and map each diagnostic onto its authored file. +/// Lint each distinct request once and map its diagnostics, or one unavailable warning per file, onto source. pub fn lint_sql_bodies( bodies: &[LintSqlBody], files: &[&str], @@ -30,31 +30,20 @@ pub fn lint_sql_bodies( } request_of_body.push(index); } - let responses: Vec, String>> = run_batch(&payloads) + let responses: Vec> = run_batch(&payloads) .map(|responses| responses.iter().map(response_diagnostics).collect()) .unwrap_or_else(|error| vec![Err(error); payloads.len()]); let authored: Vec> = files .iter() .map(|text| AuthoredFile::new(text, &options.dialect)) .collect(); - let mut diagnostics: Vec = Vec::new(); - for ((body, offset), request) in bodies.iter().zip(omitted).zip(request_of_body) { - let file: &AuthoredFile<'_> = &authored[body.file]; - let raw: Vec = match &responses[request] { - Ok(raw) => raw.clone(), - Err(message) => { - diagnostics.push(parse_failure(message, body, file)); - continue; - } - }; - let raw: Vec = without_generated_frame(raw, body, offset); - for item in &raw { - match authored_diagnostic(item, body, file) { - Ok(Some(diagnostic)) => diagnostics.push(diagnostic), - Ok(None) => {} - Err(message) => diagnostics.push(parse_failure(&message, body, file)), - } - } - } - diagnostics + let diagnostics: Vec = bodies + .iter() + .zip(omitted) + .zip(request_of_body) + .flat_map(|((body, offset), request)| { + body_diagnostics(&responses[request], (body, offset), &authored[body.file]) + }) + .collect(); + unavailable_once_per_file(diagnostics) } diff --git a/crates/sqlbuild-rules/src/lint_text/main/mod.rs b/crates/sqlbuild-rules/src/lint_text/main/mod.rs index 3343ef6e90..b3640027a2 100644 --- a/crates/sqlbuild-rules/src/lint_text/main/mod.rs +++ b/crates/sqlbuild-rules/src/lint_text/main/mod.rs @@ -1,3 +1,4 @@ +pub mod counts_toward_exit; pub mod defined_cte_names; pub mod dependency_sites; pub mod first_header_only; diff --git a/crates/sqlbuild-rules/src/lint_text/tests/test_lint_text.rs b/crates/sqlbuild-rules/src/lint_text/tests/test_lint_text.rs index 454ce07874..0142637b30 100644 --- a/crates/sqlbuild-rules/src/lint_text/tests/test_lint_text.rs +++ b/crates/sqlbuild-rules/src/lint_text/tests/test_lint_text.rs @@ -1,8 +1,12 @@ use crate::lint_text::_helpers::format_bodies::unwrapped_expression; use crate::lint_text::_helpers::sites::site_pattern; -use crate::lint_text::_helpers::sql_diagnostics::interpolation_text_at; +use crate::lint_text::_helpers::sql_diagnostics::{ + AuthoredFile, body_diagnostics, interpolation_text_at, response_diagnostics, + unavailable_once_per_file, +}; use crate::lint_text::constants::SITE_START_BYTES; use crate::lint_text::errors::LintTextError; +use crate::lint_text::main::counts_toward_exit::counts_toward_exit; use crate::lint_text::main::format_sql_bodies::format_sql_bodies; use crate::lint_text::main::frame_sql_macro_expression::frame_sql_macro_expression; use crate::lint_text::main::lint_sql_bodies::lint_sql_bodies; @@ -16,9 +20,9 @@ use crate::lint_text::main::restore_interpolation::restore_interpolation; use crate::lint_text::models::{ExpansionSpan, FormatFile, LintSqlBody, LintSqlOptions}; use crate::lint_text::tests::helpers::directive_finding; use crate::lint_text::tests::test_types::{ - Finding, FormatBodiesTestCase, LintBodiesTestCase, NeutralizeTestCase, ParameterTestCase, - RestoreTestCase, SitePatternTestCase, SlotReadTestCase, SuppressionTestCase, TokenAtTestCase, - UnwrappedExpressionTestCase, + ExitCountTestCase, Finding, FormatBodiesTestCase, LintBodiesTestCase, NeutralizeTestCase, + ParameterTestCase, RestoreTestCase, SitePatternTestCase, SlotReadTestCase, SuppressionTestCase, + TokenAtTestCase, UnavailableFinding, UnavailableLintTestCase, UnwrappedExpressionTestCase, }; use sqlbuild_analysis::sql_macros::main::parse_sql_macro::parse_sql_macro; @@ -666,3 +670,97 @@ fn given_sql_macro_bodies_when_linting_then_frames_and_generated_comments_are_no ); } } + +#[test] +fn given_engine_response_when_dialect_is_unavailable_then_each_file_gets_one_warning() { + const REASON: &str = + "SQL lint is unavailable for the tsql dialect: SQLQuack does not parse it yet"; + let test_cases = [ + UnavailableLintTestCase { + description: "an unavailable reason warns once at the file's first body", + response: serde_json::json!({"Ok": {"version": 1, "diagnostics": [], "unavailable": REASON}}), + bodies_in_file: 2, + expected_findings: vec![("L007", 2, 1, false, REASON)], + }, + UnavailableLintTestCase { + description: "a linted response without the key keeps its empty diagnostics", + response: serde_json::json!({"Ok": {"version": 1, "diagnostics": []}}), + bodies_in_file: 2, + expected_findings: Vec::new(), + }, + UnavailableLintTestCase { + description: "a non-string reason is an invalid response, not a clean one", + response: serde_json::json!({"Ok": {"version": 1, "diagnostics": [], "unavailable": null}}), + bodies_in_file: 1, + expected_findings: vec![( + "L003", + 2, + 1, + true, + "Native SQL parser could not analyze this body: native lint response has a non-string unavailable reason", + )], + }, + ]; + let file = AuthoredFile::new("MODEL ();\nselect a from t\n", "tsql"); + let body = LintSqlBody { + body_start: 10, + lint_text: "select a from t".to_string(), + ..LintSqlBody::default() + }; + + for test_case in test_cases { + let response = response_diagnostics(&test_case.response); + let diagnostics = unavailable_once_per_file( + (0..test_case.bodies_in_file) + .flat_map(|_| body_diagnostics(&response, (&body, None), &file)) + .collect(), + ); + let findings: Vec> = diagnostics + .iter() + .map(|finding| { + ( + finding.code.as_str(), + finding.line, + finding.column, + finding.fault, + finding.message.as_str(), + ) + }) + .collect(); + + assert_eq!( + findings, test_case.expected_findings, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_findings_when_deciding_exit_then_only_lint_unavailable_never_fails_a_command() { + let test_cases = [ + ExitCountTestCase { + description: "an unavailable-dialect warning reports our gap, not the user's SQL", + code: "L007", + expected_counts: false, + }, + ExitCountTestCase { + description: "a SQL lint rule finding still fails", + code: "SQLCLEAN005", + expected_counts: true, + }, + ExitCountTestCase { + description: "an engine fault still fails", + code: "L003", + expected_counts: true, + }, + ]; + for test_case in test_cases { + assert_eq!( + counts_toward_exit(test_case.code), + test_case.expected_counts, + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-rules/src/lint_text/tests/test_types.rs b/crates/sqlbuild-rules/src/lint_text/tests/test_types.rs index 60c28b778c..94f883b554 100644 --- a/crates/sqlbuild-rules/src/lint_text/tests/test_types.rs +++ b/crates/sqlbuild-rules/src/lint_text/tests/test_types.rs @@ -56,6 +56,22 @@ pub(crate) struct LintBodiesTestCase { pub(crate) expected_findings: Vec>, } +pub(crate) struct ExitCountTestCase { + pub(crate) description: &'static str, + pub(crate) code: &'static str, + pub(crate) expected_counts: bool, +} + +pub(crate) struct UnavailableLintTestCase { + pub(crate) description: &'static str, + pub(crate) response: serde_json::Value, + pub(crate) bodies_in_file: usize, + pub(crate) expected_findings: Vec>, +} + +/// One authored finding: code, line, column, whether it is a fault, and its message. +pub(crate) type UnavailableFinding<'a> = (&'a str, i64, i64, bool, &'a str); + /// One mapped finding: code, line, column and optional end position. pub(crate) type Finding<'a> = (&'a str, i64, i64, Option<(i64, i64)>); diff --git a/crates/sqlbuild-rules/src/query_facts/_helpers/facts.rs b/crates/sqlbuild-rules/src/query_facts/_helpers/facts.rs index 5af9afca09..3b468b18cb 100644 --- a/crates/sqlbuild-rules/src/query_facts/_helpers/facts.rs +++ b/crates/sqlbuild-rules/src/query_facts/_helpers/facts.rs @@ -1,53 +1,75 @@ use crate::query_facts::_helpers::shape::stub_call; use crate::query_facts::_helpers::tokens::{ComparisonOperator, QueryTokens, SelectTokens}; use crate::query_facts::constants::{ - MAX_FUNCTION_CALL_DEPTH, TABLE_FUNCTION_STUB, TRIPLE_QUOTE_LENGTH, + MAX_AST_DEPTH, MAX_FUNCTION_CALL_DEPTH, SNOWFLAKE_DATE_PART_ARGUMENTS, TABLE_FUNCTION_STUB, + TRIPLE_QUOTE_LENGTH, }; use crate::query_facts::models::{ CallKind, ColumnFact, ComparisonFacts, ComparisonSpan, CteFacts, ExpressionItem, ItemFacts, ModelShape, ParsedFixture, QueryFacts, RelationFacts, SelectFacts, SetFacts, SourceText, }; -use polyglot_sql::dialects::Dialect; -use polyglot_sql::expressions::{Identifier, Literal, Select, TableRef, With}; -use polyglot_sql::{ComplexityGuardOptions, Expression, ExpressionWalk, ParseOptions}; -use sqlbuild_analysis::lineage::main::parser_dialect::parser_dialect; +use sqlbuild_analysis::sql_tree::main::shared_dialect::shared_dialect; +use sqlquack_syntax::expressions::names::models::{Identifier, Literal, TableRef}; +use sqlquack_syntax::expressions::queries::models::{Select, With}; +use sqlquack_syntax::{ + CallMode, ComplexityGuardOptions, Dialect, Expression, ExpressionWalk, ParseOptions, +}; use std::cell::{Cell, RefCell}; use std::collections::{HashMap, HashSet}; use std::ops::Range; fn parse_query(sql: &str, dialect_name: &str) -> Result<(Expression, Dialect), String> { - let dialect_type = parser_dialect(Some(dialect_name)) - .map_err(|unknown| format!("unknown dialect {unknown}"))?; - let dialect = Dialect::get(dialect_type); + let dialect = shared_dialect(dialect_name) + .ok_or_else(|| format!("no SQL parser for dialect {dialect_name}"))?; let options = ParseOptions { complexity_guard: Some(ComplexityGuardOptions { max_function_call_depth: Some(MAX_FUNCTION_CALL_DEPTH), ..Default::default() }), }; - let mut statements = dialect - .parse_with_options(sql, &options) - .map_err(|error| error.to_string())?; + let mut statements = + sqlquack_syntax::parse_statements_with(sql, dialect, &options, CallMode::FaithfulLocated) + .map_err(|error| error.to_string())?; if statements.len() != 1 { return Err("expected one statement".to_owned()); } let statement = statements.remove(0); + if deeper_than(&statement, MAX_AST_DEPTH) { + return Err(format!( + "E_GUARD_AST_DEPTH_EXCEEDED: value {} exceeds configured limit {MAX_AST_DEPTH}", + MAX_AST_DEPTH + 1 + )); + } if !is_query(&statement) { return Err("expected a query".to_owned()); } Ok((statement, dialect)) } +/// Whether `statement` nests deeper than `limit` levels, measured without recursion. +fn deeper_than(statement: &Expression, limit: usize) -> bool { + let mut stack: Vec<(&Expression, usize)> = vec![(statement, 1)]; + while let Some((node, depth)) = stack.pop() { + if depth > limit { + return true; + } + stack.extend(node.children().into_iter().map(|child| (child, depth + 1))); + } + false +} + +fn code_tokens(sql: &str, dialect: Dialect) -> Result, String> { + let tokens = sqlquack_syntax::tokenize(sql, dialect).map_err(|error| error.to_string())?; + Ok(QueryTokens::new(tokens, sql)) +} + /// Parse a stubbed SQL-test fixture as one query. pub(crate) fn parse_fixture( shape: ModelShape, dialect_name: &str, ) -> Result { let (statement, dialect) = parse_query(&shape.sql, dialect_name)?; - let tokens = dialect - .tokenize(&shape.sql) - .map_err(|error| error.to_string())?; - let tokens = QueryTokens::new(tokens, &shape.sql); + let tokens = code_tokens(&shape.sql, dialect)?; let selects = tokens.selects(); let order = tokens.top_level_word("ORDER").unwrap_or(tokens.len()); let order_end = ["LIMIT", "OFFSET", "FETCH"] @@ -72,11 +94,10 @@ pub(crate) fn parse_fixture( }) } -/// Parse normalized model SQL with Polyglot and read the facts built-in rules evaluate. +/// Parse normalized model SQL with SQLQuack and read the facts built-in rules evaluate. pub(crate) fn query_facts(sql: &str, dialect_name: &str) -> Result { let (statement, dialect) = parse_query(sql, dialect_name)?; - let tokens = dialect.tokenize(sql).map_err(|error| error.to_string())?; - let tokens = QueryTokens::new(tokens, sql); + let tokens = code_tokens(sql, dialect)?; let builder = Builder { selects: tokens.selects(), tokens, @@ -304,16 +325,14 @@ impl Builder<'_> { let unwrapped = unwrap_paren(value); let cast_type = match unwrapped { Expression::Cast(cast) | Expression::TryCast(cast) | Expression::SafeCast(cast) => { - Some( - range - .as_ref() - .and_then(|range| self.tokens.cast_type(range)) - .unwrap_or_else(|| { - self.dialect - .generate(&Expression::DataType(cast.to.clone())) - .unwrap_or_default() - }), - ) + range + .as_ref() + .and_then(|range| self.tokens.cast_type(range)) + .or_else(|| { + let span = cast.span.0?; + self.tokens + .cast_type(&self.tokens.covering(span.start, span.end)) + }) } _ => None, }; @@ -361,15 +380,19 @@ impl Builder<'_> { if let Some(span) = self.spans.borrow().get(&key) { return *span; } - let anchors = anchors(parts); - let generated = if anchors == (None, None) { - self.dialect.generate(node).unwrap_or_default() - } else { - String::new() + let anchors = match anchors(parts) { + (None, None) => ( + parts.operands[0] + .source_span() + .map(|span| span.end) + .or_else(|| node.source_span().map(|span| span.start)), + None, + ), + anchors => anchors, }; let span = self .tokens - .comparison_range(parts.operator, anchors, parts.negated, &generated); + .comparison_range(parts.operator, anchors, parts.negated); self.spans.borrow_mut().insert(key, span); span } @@ -404,7 +427,10 @@ impl Builder<'_> { let mut fact = ComparisonFacts { span, generated_sql: if span.is_none() { - self.dialect.generate(node).unwrap_or_default() + node.source_span() + .and_then(|span| self.tokens.slice(span.start, span.end)) + .unwrap_or_default() + .to_owned() } else { String::new() }, @@ -420,6 +446,7 @@ impl Builder<'_> { let mut string_tokens: Vec<(usize, usize, &str)> = span.map_or_else(Vec::new, |span| { self.tokens.string_tokens(span.start, span.end) }); + let units = date_part_arguments(node, self.dialect); for value in node.dfs() { match value { Expression::Column(column) => { @@ -428,11 +455,20 @@ impl Builder<'_> { Expression::Identifier(identifier) => { fact.columns.insert(identifier.name.clone()); } + Expression::Literal(literal) if units.contains(&std::ptr::from_ref(value)) => { + if let Some(unit) = string_literal(literal) + && let Some(index) = + string_tokens.iter().position(|(_, _, text)| *text == unit) + { + string_tokens.remove(index); + } + } Expression::Literal(literal) => { if let Some(generated) = string_literal(literal) { let position = string_tokens .iter() - .position(|(_, _, text)| *text == literal.value_str()); + .position(|(_, _, text)| *text == generated) + .or_else(|| (!string_tokens.is_empty()).then_some(0)); let range = position.map(|index| { let (start, end, _) = string_tokens.remove(index); (start, end) @@ -452,9 +488,10 @@ impl Builder<'_> { .extend(operand.dfs().filter_map(direct_column)); } if direct_string_literal(operand).is_none() - && operand - .dfs() - .any(|value| direct_string_literal(value).is_some()) + && operand.dfs().any(|value| { + !units.contains(&std::ptr::from_ref(value)) + && direct_string_literal(value).is_some() + }) { fact.modified_string_literal = true; } @@ -463,6 +500,31 @@ impl Builder<'_> { } } +/// The quoted date-part arguments of Snowflake calls such as `DATEDIFF('day', a, b)`. +fn date_part_arguments(root: &Expression, dialect: Dialect) -> HashSet<*const Expression> { + if dialect != Dialect::Snowflake { + return HashSet::new(); + } + root.dfs().filter_map(date_part_argument).collect() +} + +/// The quoted date-part argument of `node` when it is a Snowflake date-part call. +fn date_part_argument(node: &Expression) -> Option<*const Expression> { + let Expression::Function(function) = node else { + return None; + }; + let name = function.name.to_ascii_uppercase(); + for (call, count, index) in SNOWFLAKE_DATE_PART_ARGUMENTS { + if call == name && count == function.args.len() { + let argument = function.args.get(index)?; + return direct_string_literal(argument) + .is_some() + .then(|| std::ptr::from_ref(argument)); + } + } + None +} + fn comparison_parts(node: &Expression) -> Option> { let (operator, operands, negated) = match node { Expression::Eq(op) => (ComparisonOperator::Eq, vec![&op.left, &op.right], false), @@ -668,7 +730,7 @@ fn own_span(expression: &Expression) -> Option { Expression::Column(column) => column.span.or(column.name.span), Expression::Identifier(identifier) => identifier.span, Expression::Star(star) => star.table.as_ref().and_then(|table| table.span), - Expression::Function(function) => function.span, + Expression::Function(function) => function.span.0, Expression::Table(table) => table .catalog .as_ref() @@ -692,7 +754,7 @@ fn span_bounds(expression: &Expression) -> Option<(usize, usize)> { Expression::Column(column) => column.span.or(column.name.span), Expression::Identifier(identifier) => identifier.span, Expression::Star(star) => star.table.as_ref().and_then(|table| table.span), - Expression::Function(function) => function.span, + Expression::Function(function) => function.span.0, Expression::Count(count) => count.span, Expression::Table(table) => table.name.span, _ => None, diff --git a/crates/sqlbuild-rules/src/query_facts/_helpers/tokens.rs b/crates/sqlbuild-rules/src/query_facts/_helpers/tokens.rs index 9e6863c110..b847237997 100644 --- a/crates/sqlbuild-rules/src/query_facts/_helpers/tokens.rs +++ b/crates/sqlbuild-rules/src/query_facts/_helpers/tokens.rs @@ -1,6 +1,8 @@ use crate::query_facts::constants::MIN_CAST_TOKENS; -use crate::query_facts::models::ComparisonSpan; -use polyglot_sql::tokens::{Token, TokenType}; +use crate::query_facts::models::{CodeToken, ComparisonSpan}; +use sqlquack_syntax::LosslessToken; +use sqlquack_syntax::TokenKind; +use sqlquack_syntax::tokens::models::TokenType; use std::cell::RefCell; use std::collections::HashSet; use std::ops::Range; @@ -150,32 +152,35 @@ pub(crate) struct SelectTokens { /// The code tokens of a normalized query, without comments, over its text. pub(crate) struct QueryTokens<'a> { - tokens: Vec, + tokens: Vec>, sql: &'a str, bytes: Vec, used: RefCell>, } impl<'a> QueryTokens<'a> { - pub(crate) fn new(tokens: Vec, sql: &'a str) -> Self { + /// The code tokens of `tokens`, a lossless tokenization of `sql`, at character offsets. + pub(crate) fn new(tokens: Vec, sql: &'a str) -> Self { let mut bytes: Vec = sql.char_indices().map(|(byte, _)| byte).collect(); bytes.push(sql.len()); + let character = |byte: usize| bytes.partition_point(|offset| *offset < byte); + let tokens: Vec> = tokens + .into_iter() + .filter_map(|token| match token.kind { + TokenKind::Significant(token_type) => Some(CodeToken { + token_type, + text: &sql[token.start..token.end], + start: character(token.start), + end: character(token.end), + }), + TokenKind::Whitespace | TokenKind::LineComment | TokenKind::BlockComment => None, + }) + .collect(); Self { sql, bytes, used: RefCell::default(), - tokens: tokens - .into_iter() - .filter(|token| { - !matches!( - token.token_type, - TokenType::BlockComment - | TokenType::LineComment - | TokenType::Space - | TokenType::Break - ) - }) - .collect(), + tokens, } } @@ -185,11 +190,11 @@ impl<'a> QueryTokens<'a> { } pub(crate) fn start(&self, index: usize) -> usize { - self.tokens[index].span.start + self.tokens[index].start } pub(crate) fn end(&self, index: usize) -> usize { - self.tokens[index].span.end + self.tokens[index].end } fn kind(&self, index: usize) -> Option { @@ -369,14 +374,21 @@ impl<'a> QueryTokens<'a> { parameterized && (cast || conversion) } - /// The string-literal tokens within a normalized range, as character range and parsed text. + /// The tokens within characters `start` to `end`, as a token range. + pub(crate) fn covering(&self, start: usize, end: usize) -> Range { + let first = self.tokens.partition_point(|token| token.start < start); + let last = self.tokens.partition_point(|token| token.end <= end); + first..last.max(first) + } + + /// The string-literal tokens within a normalized range, as character range and source text. pub(crate) fn string_tokens(&self, start: usize, end: usize) -> Vec<(usize, usize, &str)> { self.tokens .iter() .filter(|token| { - token.span.start >= start && token.span.end <= end && string_token(token.token_type) + token.start >= start && token.end <= end && string_token(token.token_type) }) - .map(|token| (token.span.start, token.span.end, token.text.as_str())) + .map(|token| (token.start, token.end, token.text)) .collect() } @@ -595,7 +607,6 @@ impl<'a> QueryTokens<'a> { operator: ComparisonOperator, (left_end, right_start): (Option, Option), negated: bool, - generated: &str, ) -> Option { let anchor = match (left_end, right_start) { (Some(end), _) => (0..self.tokens.len()).find(|index| { @@ -608,16 +619,7 @@ impl<'a> QueryTokens<'a> { && self.operator(*index, operator) && !self.used.borrow().contains(index) }), - (None, None) => { - let wanted = compact(generated); - (0..self.tokens.len()).find(|index| { - self.operator(*index, operator) && !self.used.borrow().contains(index) && { - let (first, last) = self.extent(*index, operator, negated); - self.slice(self.start(first), self.end(last)) - .is_some_and(|text| compact(text) == wanted) - } - }) - } + (None, None) => None, }?; self.used.borrow_mut().insert(anchor); let (first, last) = self.extent(anchor, operator, negated); @@ -685,13 +687,6 @@ impl<'a> QueryTokens<'a> { } } -fn compact(text: &str) -> String { - text.chars() - .filter(|character| !character.is_whitespace()) - .flat_map(char::to_lowercase) - .collect() -} - fn string_token(token_type: TokenType) -> bool { matches!( token_type, diff --git a/crates/sqlbuild-rules/src/query_facts/constants.rs b/crates/sqlbuild-rules/src/query_facts/constants.rs index 564143160d..2ff3bfcad4 100644 --- a/crates/sqlbuild-rules/src/query_facts/constants.rs +++ b/crates/sqlbuild-rules/src/query_facts/constants.rs @@ -6,7 +6,17 @@ pub(crate) const UDF_STUB: &str = "__sqlbuild_udf_"; pub(crate) const PLACEHOLDER_PREFIX: &[u8] = b"@@@"; pub(crate) const PLACEHOLDER_VALUE: &str = "NULL"; pub(crate) const MAX_FUNCTION_CALL_DEPTH: usize = 128; +/// The deepest statement built-in rules read, Polyglot's former AST-depth limit. +pub(crate) const MAX_AST_DEPTH: usize = 512; pub(crate) const UDF_CALL: &str = "__udf"; pub(crate) const TABLE_FUNCTION_CALL: &str = "__table_fn"; pub(crate) const MIN_CAST_TOKENS: usize = 2; pub(crate) const TRIPLE_QUOTE_LENGTH: usize = 3; +/// Snowflake calls with a date-part argument, as (name, argument count, date-part index). +pub(crate) const SNOWFLAKE_DATE_PART_ARGUMENTS: [(&str, usize, usize); 5] = [ + ("DATEDIFF", 3, 0), + ("DATE_DIFF", 3, 0), + ("TIMEDIFF", 3, 0), + ("TIMESTAMPDIFF", 3, 0), + ("LAST_DAY", 2, 1), +]; diff --git a/crates/sqlbuild-rules/src/query_facts/main/facts_available.rs b/crates/sqlbuild-rules/src/query_facts/main/facts_available.rs new file mode 100644 index 0000000000..d4e29aac88 --- /dev/null +++ b/crates/sqlbuild-rules/src/query_facts/main/facts_available.rs @@ -0,0 +1,8 @@ +//! `facts_available`: whether SQLBuild parses a dialect's SQL for built-in rules. + +use sqlbuild_analysis::sql_tree::main::shared_dialect::shared_dialect; + +/// Whether SQLBuild parses `dialect`'s SQL for the built-in rules that read query facts. +pub(crate) fn facts_available(dialect: &str) -> bool { + shared_dialect(dialect).is_some() +} diff --git a/crates/sqlbuild-rules/src/query_facts/main/mod.rs b/crates/sqlbuild-rules/src/query_facts/main/mod.rs index 12999bd6c5..a4b8b0f0c7 100644 --- a/crates/sqlbuild-rules/src/query_facts/main/mod.rs +++ b/crates/sqlbuild-rules/src/query_facts/main/mod.rs @@ -1,4 +1,5 @@ pub mod dependency_texts; +pub mod facts_available; pub mod model_shape; pub mod parse_fixture; pub mod query_facts; diff --git a/crates/sqlbuild-rules/src/query_facts/main/query_facts.rs b/crates/sqlbuild-rules/src/query_facts/main/query_facts.rs index dba714d2c9..e246caab16 100644 --- a/crates/sqlbuild-rules/src/query_facts/main/query_facts.rs +++ b/crates/sqlbuild-rules/src/query_facts/main/query_facts.rs @@ -1,6 +1,6 @@ use crate::query_facts::models::QueryFacts; -/// Parse normalized model SQL with Polyglot and read the facts built-in rules evaluate. +/// Parse normalized model SQL with SQLQuack and read the facts built-in rules evaluate. pub(crate) fn query_facts(sql: &str, dialect: &str) -> Result { crate::query_facts::_helpers::facts::query_facts(sql, dialect) } diff --git a/crates/sqlbuild-rules/src/query_facts/models.rs b/crates/sqlbuild-rules/src/query_facts/models.rs index 35dd961830..a5688f1e06 100644 --- a/crates/sqlbuild-rules/src/query_facts/models.rs +++ b/crates/sqlbuild-rules/src/query_facts/models.rs @@ -1,4 +1,5 @@ -use polyglot_sql::Expression; +use sqlquack_syntax::Expression; +use sqlquack_syntax::tokens::models::TokenType; use std::collections::BTreeSet; use crate::lint_text::main::map_expanded_offset::map_expanded_offset; @@ -192,7 +193,7 @@ impl QueryFacts { } } -/// An SQL-test fixture query parsed with Polyglot, with the function-call facts its rules read. +/// An SQL-test fixture query parsed with SQLQuack, with the function-call facts its rules read. #[derive(Debug)] pub(crate) struct ParsedFixture { pub(crate) statement: Expression, @@ -366,3 +367,12 @@ impl<'a> ModelSource<'a> { self.sql.get(from..to.max(from)).unwrap_or_default() } } + +/// One code token of a normalized query: its type, source text and character range. +#[derive(Debug)] +pub(crate) struct CodeToken<'a> { + pub(crate) token_type: TokenType, + pub(crate) text: &'a str, + pub(crate) start: usize, + pub(crate) end: usize, +} diff --git a/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs b/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs index 733cabe15d..4e3256ff5e 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs @@ -6,9 +6,12 @@ use crate::constants::{ }; use crate::constants::{ MIN_CUSTOM_RULE_TEST_CASES_KEY, MODEL_NAME_LAYERS, PUBLISHED_READ_CODE, PUBLISHED_SHAPE_CODE, + RULES_UNAVAILABLE_REMEDIATION, }; +use crate::lint_text::constants::LINT_UNAVAILABLE_CODE; use crate::models::{Declaration, EvaluateRequest, Fault, Model, RuleMetadata, RulesConfig}; use crate::query_facts::main::dependency_texts::dependency_texts; +use crate::query_facts::main::facts_available::facts_available; use crate::query_facts::main::model_shape::model_shape; use crate::query_facts::main::query_facts::query_facts; use crate::query_facts::models::{ @@ -134,6 +137,17 @@ fn evaluate_model_inner(request: ModelEvaluationRequest<'_>) -> Result) -> faults.into_inner() } +/// One non-failing warning that `dialect` SQL is not parsed, so the built-in rules `codes` did not run on `path`. +pub(crate) fn rules_unavailable<'a>( + path: &str, + codes: impl Iterator, + dialect: &str, +) -> Fault { + Fault { + unevaluated: false, + code: LINT_UNAVAILABLE_CODE.to_owned(), + path: path.to_owned(), + line: 1, + column: 1, + message: format!( + "SQLBuild cannot parse {dialect} SQL for built-in rules yet, so these rules were not evaluated: {}", + codes.collect::>().join(", ") + ), + remediation: RULES_UNAVAILABLE_REMEDIATION.to_owned(), + } +} + fn requires_parsed_model(selected: &BTreeMap) -> bool { selected.values().any(|rule| requires_model_query(rule)) } diff --git a/crates/sqlbuild-rules/src/rules/_helpers/sql_test_rules.rs b/crates/sqlbuild-rules/src/rules/_helpers/sql_test_rules.rs index ae4c1d8d29..f53e86786c 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/sql_test_rules.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/sql_test_rules.rs @@ -2,18 +2,20 @@ use crate::models::{ DeclarationKind, EvaluateRequest, Fault, Model, ResourceKind, RuleMetadata, RulesConfig, ScopeResource, SqlScenarioFact, SqlTestCteFact, SqlTestFact, SqlTestMode, }; +use crate::query_facts::main::facts_available::facts_available; use crate::query_facts::main::model_shape::model_shape; use crate::query_facts::main::parse_fixture::parse_fixture; use crate::query_facts::main::stub_call::stub_call; use crate::query_facts::models::{CallKind, ParsedFixture}; -use crate::rules::_helpers::evaluation::path_fault; +use crate::rules::_helpers::evaluation::{path_fault, rules_unavailable}; use crate::rules::models::ProjectEvaluationRequest; -use polyglot_sql::Expression; -use polyglot_sql::expressions::{Literal, Select}; use serde::{Deserialize, Serialize}; use sqlbuild_analysis::compiler::main::sql_test_fixture_facts::fixture_facts; +use sqlquack_syntax::Expression; +use sqlquack_syntax::expressions::names::models::Literal; +use sqlquack_syntax::expressions::queries::models::Select; use std::cell::RefCell; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::path::Path; use std::rc::Rc; @@ -42,17 +44,35 @@ fn evaluated_project(evaluation: ProjectEvaluationRequest<'_>) -> Result> = HashMap::new(); - for rule in evaluation.selected.values().filter(|rule| { - matches!( - rule.code.as_str(), - "SQBRTEST201" | "SQBRTEST202" | EMPTY_INPUT_RULE_CODE - ) - }) { + let fixture_rules: Vec<&RuleMetadata> = evaluation + .selected + .values() + .copied() + .filter(|rule| { + matches!( + rule.code.as_str(), + "SQBRTEST201" | "SQBRTEST202" | EMPTY_INPUT_RULE_CODE + ) + }) + .collect(); + let dialect: &str = &evaluation.request.dialect; + if !fixture_rules.is_empty() && !facts_available(dialect) { + let paths: BTreeSet<&str> = tests + .iter() + .filter(|test| reads_fixtures(test)) + .map(|test| test.source_path.as_str()) + .collect(); + let codes: Vec<&str> = fixture_rules + .iter() + .map(|rule| rule.code.as_str()) + .collect(); + for path in paths { + faults.push(rules_unavailable(path, codes.iter().copied(), dialect)); + } + } + for rule in fixture_rules.iter().filter(|_| facts_available(dialect)) { for test in &tests { - if !matches!(test.mode, SqlTestMode::Model) - || test.has_macro_mocks - || test.has_model_query_overrides - { + if !reads_fixtures(test) { continue; } for cte in test @@ -114,6 +134,13 @@ fn evaluated_project(evaluation: ProjectEvaluationRequest<'_>) -> Result bool { + matches!(test.mode, SqlTestMode::Model) + && !test.has_macro_mocks + && !test.has_model_query_overrides +} + fn canonical_roots( rule: &RuleMetadata, tests: &[&SqlTestFact], 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 09d9a86da7..2f7792a7a3 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md @@ -12,6 +12,7 @@ Online: https://sqlbuild.com/docs/concepts/rules/ - How Rules work - SQL Rules - Built-in and custom Rules +- Dialect support - One definition of project validity - Rules compared with tests, audits, and contracts - Findings and intentional exceptions @@ -178,6 +179,26 @@ select = [ Define typed model or project checks over compiler-owned facts with `@rule`, `RuleContext`, and `Finding`. +## Dialect support + +SQL Rules, and the built-in Rules that read SQL, such as `SQBRMODEL102` and the SQL-test Rules +`SQBRTEST201` to `SQBRTEST203`, run only where SQLBuild parses the project's dialect. Built-in Rules +that read SQL support these adapters: + +| Adapter | Built-in Rules that read SQL | +|---|---| +| DuckDB, MotherDuck | Supported | +| PostgreSQL | Supported | +| Snowflake | Supported | +| SQLite | Supported | +| BigQuery, ClickHouse, Databricks, SQL Server | Not yet | + +Where SQLBuild does not parse the dialect yet, `sqb compile` and `sqb rules run` show `L007` +warnings that name it: one per file for SQL Rules, and one per model or SQL test listing the +built-in Rules that did not run. `L007` never changes an exit code, including for `sqb plan` and +`sqb build`: it reports a gap in SQLBuild's dialect support, not a problem in your SQL. A syntax +error in a supported dialect still fails the Rules that read it. + ## One definition of project validity `sqb compile` is authoritative. SQLBuild evaluates a project in this order: diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/rules/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/rules/_test_types.py index f4859f7693..1c7a62f52e 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/rules/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/rules/_test_types.py @@ -179,6 +179,22 @@ class RankingSortDefaultCase: expected_codes: tuple[str, ...] +@dataclass(frozen=True) +class UnparsedDialectRulesCase: + description: str + adapter_toml: str + expected_codes: tuple[str, ...] + expected_message: str + expected_exit: int + + +@dataclass(frozen=True) +class ReplayedFindingExitCase: + description: str + code: str + expected_exit: int + + @dataclass(frozen=True) class FixableReportingCase: description: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_clickhouse_comments.py b/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_clickhouse_comments.py index 0cc24a9f8b..ba1ee05735 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_clickhouse_comments.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_clickhouse_comments.py @@ -23,14 +23,14 @@ "test_case", [ ClickHouseCommentE2ETestCase( - description="a hash comment with an apostrophe is skipped", + description="a hash comment with an apostrophe is skipped, reported unavailable until SQLBuild parses the dialect", comment="# don't keep the star", - expected_location="models/orders.sql:4:8 [SQBRMODEL102]", + expected_location="models/orders.sql:1:1 [L007]", ), ClickHouseCommentE2ETestCase( - description="a double-slash comment with an apostrophe is skipped", + description="a double-slash comment with an apostrophe is skipped, reported unavailable until SQLBuild parses the dialect", comment="// don't keep the star", - expected_location="models/orders.sql:4:8 [SQBRMODEL102]", + expected_location="models/orders.sql:1:1 [L007]", ), ], ids=lambda case: case.description, diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_sql_quality_rules.py b/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_sql_quality_rules.py index 09961d554d..9199ee3e4b 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_sql_quality_rules.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_sql_quality_rules.py @@ -9,13 +9,16 @@ import pytest +from sqlbuild._native import rules_bulk_cache_path from tests.e2e.src.sqlbuild.cli.commands.main.rules._test_types import ( ConstantLiteralCase, DuplicateLiteralCase, FixableReportingCase, RankingKeyProofCase, RankingSortDefaultCase, + ReplayedFindingExitCase, SqlQualityRuleCase, + UnparsedDialectRulesCase, ) from tests.e2e.src.sqlbuild.cli.commands.main.rules.helpers import ( diagnostic_codes, @@ -557,5 +560,125 @@ def test_given_repeated_long_literal_when_running_rules_then_help_names_the_shar ), result.stdout +@pytest.mark.parametrize( + "test_case", + ( + ReplayedFindingExitCase( + description="a SQL rule finding fails every command", + code="SQLCLEAN043", + expected_exit=1, + ), + ReplayedFindingExitCase( + description="a lint-unavailable warning fails no command", + code="L007", + expected_exit=0, + ), + ), + ids=lambda case: case.description, +) +def test_given_cached_sql_finding_when_running_commands_then_only_rule_findings_fail( + test_case: ReplayedFindingExitCase, tmp_path: Path +) -> None: + write_quality_project( + project_dir=tmp_path, + toml=( + 'name = "orders"\nadapter = "duckdb"\ndefault_target = "dev"\n\n' + '[connection]\ndatabase = "plan.duckdb"\n\n[targets.dev]\nschema = "analytics"\n\n' + '[rules]\nselect = ["SQLCLEAN043"]\n' + ), + upstream=_RAW_HEADER + _UPSTREAM_SQL, + summary=( + _SUMMARY_HEADER + "SELECT o.order_id, ROW_NUMBER() OVER (ORDER BY o.amount," + " o.status, o.note, o.customer_id, o.amount + 1, o.order_id) AS rn\n" + 'FROM __ref("raw_orders") AS o\n' + ), + ) + seeded: subprocess.CompletedProcess[str] = run_unevaluated_rules_cli( + tmp_path, "compile", "--json" + ) + assert diagnostic_codes(seeded) == ("SQLCLEAN043",), seeded.stdout + seeded.stderr + # Replay the cached finding under the case's code: the engine cannot emit L007 yet. + cache: Path = rules_bulk_cache_path(project_dir=tmp_path, file_name="sql.json") + stored: dict[str, Any] = json.loads(cache.read_text(encoding="utf-8")) + for entry in stored["entries"].values(): + for finding in entry["findings"]: + finding["code"] = test_case.code + _ = cache.write_text(json.dumps(stored), encoding="utf-8") + + compiled: subprocess.CompletedProcess[str] = run_unevaluated_rules_cli( + tmp_path, "compile", "--json" + ) + exits: tuple[int, ...] = ( + compiled.returncode, + run_unevaluated_rules_cli(tmp_path, "rules", "run", "SQLCLEAN043").returncode, + run_unevaluated_rules_cli(tmp_path, "plan").returncode, + run_unevaluated_rules_cli(tmp_path, "build").returncode, + ) + + assert (diagnostic_codes(compiled), exits) == ( + (test_case.code,), + (test_case.expected_exit,) * len(exits), + ), compiled.stdout + compiled.stderr + + if __name__ == "__main__": pytest.main([__file__, "-vv"]) + + +@pytest.mark.parametrize( + "test_case", + ( + UnparsedDialectRulesCase( + description="a parsed dialect reports the built-in rule finding", + adapter_toml='adapter = "duckdb"\n', + expected_codes=("SQBRMODEL102",), + expected_message="SELECT * is allowed only", + expected_exit=1, + ), + UnparsedDialectRulesCase( + description="an unparsed dialect warns once per model and fails nothing", + adapter_toml=( + 'adapter = "bigquery"\ndefault_target = "dev"\n\n' + '[targets.dev]\ndatabase = "warehouse"\nschema = "analytics"\n' + ), + expected_codes=("L007", "L007"), + expected_message=( + "SQLBuild cannot parse bigquery SQL for built-in rules yet," + " so these rules were not evaluated: SQBRMODEL102" + ), + expected_exit=0, + ), + ), + ids=lambda case: case.description, +) +def test_given_project_dialect_when_running_built_in_rules_then_unparsed_dialects_only_warn( + test_case: UnparsedDialectRulesCase, tmp_path: Path +) -> None: + write_quality_project( + project_dir=tmp_path, + toml=( + 'name = "orders"\n' + test_case.adapter_toml + '\n[rules]\nselect = ["SQBRMODEL102"]\n' + ), + upstream=_RAW_HEADER + _UPSTREAM_SQL, + summary=_SUMMARY_HEADER + 'SELECT * FROM __ref("raw_orders")\n', + ) + + compiled: subprocess.CompletedProcess[str] = run_unevaluated_rules_cli( + tmp_path, "compile", "--json" + ) + ran: subprocess.CompletedProcess[str] = run_unevaluated_rules_cli( + tmp_path, "rules", "run", "SQBRMODEL102" + ) + + messages: list[str] = [ + str(item["message"]) for item in json.loads(compiled.stdout)["diagnostics"] + ] + assert ( + diagnostic_codes(compiled), + all(test_case.expected_message in message for message in messages), + (compiled.returncode, ran.returncode), + ) == ( + test_case.expected_codes, + True, + (test_case.expected_exit, test_case.expected_exit), + ), compiled.stdout + compiled.stderr + ran.stdout + ran.stderr diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py index 66c39436dc..1ad17c825b 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -2142,10 +2142,16 @@ def test_given_commented_dependency_call_when_running_import_rule_then_comment_i @pytest.mark.parametrize( "test_case", - [RulesIntegrationTestCase("quoted comment marker preserves real call", 1, "SQBRMODEL101")], + [ + RulesIntegrationTestCase( + "quoted comment marker" + ", reported unavailable until SQLBuild parses the dialect", + 0, + "SQBRMODEL101", + ) + ], ids=lambda case: case.description, ) -def test_given_bigquery_quoted_comment_marker_when_running_import_rule_then_real_call_is_detected( +def test_given_bigquery_quoted_comment_marker_when_running_import_rule_then_reports_dialect_unavailable( test_case: RulesIntegrationTestCase, tmp_path: Path, capsys: pytest.CaptureFixture[str], @@ -2194,8 +2200,9 @@ def test_given_bigquery_quoted_comment_marker_when_running_import_rule_then_real payload: dict[str, object] = json.loads(capsys.readouterr().out) findings: list[dict[str, object]] = cast(list[dict[str, object]], payload["findings"]) assert [finding["message"] for finding in findings] == [ - 'import CTE "imported_orders" contains transformation logic' - ] + "SQLBuild cannot parse bigquery SQL for built-in rules yet," + " so these rules were not evaluated: SQBRMODEL101" + ] * 2 @pytest.mark.parametrize( diff --git a/website/src/content/docs/docs/concepts/rules.mdx b/website/src/content/docs/docs/concepts/rules.mdx index 8f761a9ee0..a1e0eb1845 100644 --- a/website/src/content/docs/docs/concepts/rules.mdx +++ b/website/src/content/docs/docs/concepts/rules.mdx @@ -168,6 +168,26 @@ select = [ `Finding`. +## Dialect support + +SQL Rules, and the built-in Rules that read SQL, such as `SQBRMODEL102` and the SQL-test Rules +`SQBRTEST201` to `SQBRTEST203`, run only where SQLBuild parses the project's dialect. Built-in Rules +that read SQL support these adapters: + +| Adapter | Built-in Rules that read SQL | +|---|---| +| DuckDB, MotherDuck | Supported | +| PostgreSQL | Supported | +| Snowflake | Supported | +| SQLite | Supported | +| BigQuery, ClickHouse, Databricks, SQL Server | Not yet | + +Where SQLBuild does not parse the dialect yet, `sqb compile` and `sqb rules run` show `L007` +warnings that name it: one per file for SQL Rules, and one per model or SQL test listing the +built-in Rules that did not run. `L007` never changes an exit code, including for `sqb plan` and +`sqb build`: it reports a gap in SQLBuild's dialect support, not a problem in your SQL. A syntax +error in a supported dialect still fails the Rules that read it. + ## One definition of project validity `sqb compile` is authoritative. SQLBuild evaluates a project in this order: