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/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/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/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md index 09d9a86da7..0b2aac11f9 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules.md @@ -144,6 +144,11 @@ These checks run through ordinary compilation. There is intentionally no separat lifecycle. If an SQL requirement affects whether the project is acceptable, the compiler enforces it with the rest of the configured Rules. +If SQL Rules cannot run because SQLBuild does not parse the project's dialect yet, `sqb compile` +and `sqb rules run` show one `L007` warning per file that names the dialect. `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, so the file's SQL Rules simply did not run. + Not every traditional lint concern should become a diagnostic. SQLBuild separates three kinds of SQL responsibility: 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 6e710ee77e..977b4079b0 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,13 @@ class RankingSortDefaultCase: expected_codes: tuple[str, ...] +@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_sql_quality_rules.py b/tests/e2e/src/sqlbuild/cli/commands/main/rules/test_sql_quality_rules.py index 09961d554d..a22ba9759c 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,12 +9,14 @@ 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, ) from tests.e2e.src.sqlbuild.cli.commands.main.rules.helpers import ( @@ -557,5 +559,66 @@ 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"]) diff --git a/website/src/content/docs/docs/concepts/rules.mdx b/website/src/content/docs/docs/concepts/rules.mdx index 8f761a9ee0..884993b4a5 100644 --- a/website/src/content/docs/docs/concepts/rules.mdx +++ b/website/src/content/docs/docs/concepts/rules.mdx @@ -132,6 +132,11 @@ These checks run through ordinary compilation. There is intentionally no separat lifecycle. If an SQL requirement affects whether the project is acceptable, the compiler enforces it with the rest of the configured Rules. +If SQL Rules cannot run because SQLBuild does not parse the project's dialect yet, `sqb compile` +and `sqb rules run` show one `L007` warning per file that names the dialect. `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, so the file's SQL Rules simply did not run. + Not every traditional lint concern should become a diagnostic. SQLBuild separates three kinds of SQL responsibility: