From 61ddaeb6b2086162af7c54763b006af799604a77 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 07:57:04 +0100 Subject: [PATCH 01/15] fix: move and rename a model's unit tests with sqb rename and sqb mv A model's own unit test files, whose every TEST block targets only that model, now follow it: test_[__].sql files take the new name, TEST (name "__") headers take the new subject, and files in the folder mirroring the model move to the folder mirroring the destination. The edits are verified in the scratch compile like every other edit, reported in --dry-run, and written only when verification passes. --- .../_helpers/refactor/refactor_facts.rs | 20 ++- .../_helpers/edits/header_edits.rs | 81 +++++++++-- .../src/refactoring/_helpers/planning/mod.rs | 1 + .../_helpers/planning/model_planning.rs | 17 ++- .../_helpers/planning/unit_test_moves.rs | 101 ++++++++++++++ .../src/refactoring/constants.rs | 9 ++ .../src/refactoring/models.rs | 13 ++ .../src/refactoring/tests/helpers.rs | 66 ++++++++- .../src/refactoring/tests/test_planning.rs | 65 ++++++++- .../src/refactoring/tests/test_types.rs | 7 + .../sqlbuild/references/docs/cli/rename.md | 8 ++ .../cli/commands/main/refactor/_test_types.py | 16 +++ .../main/refactor/test_rename_models.py | 2 +- .../refactor/test_unit_tests_follow_models.py | 132 ++++++++++++++++++ website/src/content/docs/docs/cli/rename.mdx | 8 ++ 15 files changed, 529 insertions(+), 17 deletions(-) create mode 100644 crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py diff --git a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs index 666f5985c9..31d1ee4be7 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs @@ -8,7 +8,7 @@ use sqlbuild_refactor::refactoring::models::{ AuthoredFile, DeclarationPlacement, DependencyFacts, DestinationFacts, DiscoveredFile, ExpansionFacts, ExpansionSpan, LocationFacts, ManualLocation, ModelFacts, PlacedDeclaration, RefactorFacts, RefactorOperation, RefactorRequest, RelocatedDeclaration, ResourceColumnsFacts, - SchemaColumnFacts, SqlFileRole, + SchemaColumnFacts, SqlFileRole, UnitTestFacts, }; use crate::bindings::_helpers::model_config::config_values::config_str; @@ -331,10 +331,28 @@ pub(crate) fn refactor_facts( model_files: discovered_files(&native_field(&discovered, "model_files")?)?, authored_files: authored_files(&discovered)?, yaml_files, + unit_tests: unit_tests(&compiled)?, python_locations: python_locations(project, request)?, }) } +/// Every compiled SQL unit test with its file, header name and target models. +fn unit_tests(compiled: &Bound<'_, PyAny>) -> PyResult> { + let mut tests: Vec = Vec::new(); + for test in native_field(compiled, "sql_tests")?.try_iter()? { + let test: Bound<'_, PyAny> = test?; + tests.push(UnitTestFacts { + path: posix(&native_field( + &native_field(&test, "test_file")?, + "relative_path", + )?)?, + name: native_field(&native_field(&test, "test_block")?, "name")?.extract()?, + target_model_names: native_field(&test, "target_model_names")?.extract()?, + }); + } + Ok(tests) +} + fn identity_key(identity: &Bound<'_, PyAny>) -> PyResult { identity.repr()?.extract() } diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs index ef6ec15a1f..ddae3f23cf 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs @@ -14,7 +14,7 @@ use crate::refactoring::constants::{ HEADER_DESCRIPTION_KEY, HEADER_INDENT, HEADER_KEY_AND_VALUE_TOKENS, HEADER_OPEN_PAREN, HEADER_OPENERS, HEADER_SEPARATOR, LIST_OPENER, MIGRATE_FROM_KEY, PARENTHESIZED_EMPTY_TOKENS, REF_FUNCTION, RELATIONSHIPS_AUDIT, RELATIONSHIPS_FIELD_KEY, RELATIONSHIPS_TO_KEY, - SCHEMA_KEYWORD, + SCHEMA_KEYWORD, TEST_KEYWORD, TEST_NAME_KEY, TEST_NAME_SEPARATOR, }; use crate::refactoring::errors::RefactorError; use crate::refactoring::models::{EditKind, TextEdit}; @@ -652,21 +652,39 @@ fn line_indent(text: &[char], offset: usize) -> String { /// Code-point spans of every `SCHEMA (...)` header body, as `SCHEMA_STATEMENT_PATTERN.finditer`. fn schema_header_spans(contents: &str, text: &[char]) -> Vec<(usize, usize)> { + statement_header_spans( + contents, + text, + SCHEMA_KEYWORD, + |index| { + (index == 0 || !is_word_char(text[index - 1])) + && starts_with(text, index, SCHEMA_KEYWORD) + }, + |end| statement_end(text, end), + ) +} + +/// Code-point spans of `keyword (...)` header bodies starting where `opens` allows, scanning on +/// from `resume` of each body's end. +fn statement_header_spans( + contents: &str, + text: &[char], + keyword: &str, + opens: impl Fn(usize) -> bool, + resume: impl Fn(usize) -> usize, +) -> Vec<(usize, usize)> { let mut byte_offsets: Vec = contents.char_indices().map(|(offset, _)| offset).collect(); byte_offsets.push(contents.len()); let mut spans: Vec<(usize, usize)> = Vec::new(); let mut index = 0; while index < text.len() { - let bounded = index == 0 || !is_word_char(text[index - 1]); - if bounded - && starts_with(text, index, SCHEMA_KEYWORD) + if opens(index) && let Some((start, end, _)) = - match_statement_header(contents, byte_offsets[index], SCHEMA_KEYWORD) + match_statement_header(contents, byte_offsets[index], keyword) { - let start = byte_to_char(&byte_offsets, start); let end = byte_to_char(&byte_offsets, end); - spans.push((start, end)); - index = statement_end(text, end); + spans.push((byte_to_char(&byte_offsets, start), end)); + index = resume(end); continue; } index += 1; @@ -705,6 +723,53 @@ pub(crate) fn schema_model_name_edits( Ok(edits) } +/// Code-point spans of every `TEST (...)` header body that starts a line, as discovery splits blocks. +fn test_header_spans(contents: &str, text: &[char]) -> Vec<(usize, usize)> { + statement_header_spans( + contents, + text, + TEST_KEYWORD, + |index| index == 0 || text[index - 1] == '\n', + |end| end, + ) +} + +/// Rename the subject of `TEST (name "__")` headers whose name is in `owned`. +pub(crate) fn test_name_edits( + contents: &str, + text: &[char], + names: (&str, &str), + owned: &[&str], +) -> Result, RefactorError> { + let (old, new) = names; + let prefix = format!("{old}{TEST_NAME_SEPARATOR}"); + let mut edits: Vec = Vec::new(); + for span in test_header_spans(contents, text) { + let tokens = span_tokens(text, span)?; + for (key, entry) in header_entries(&tokens) { + if !key.eq_ignore_ascii_case(TEST_NAME_KEY) { + continue; + } + let Some(value) = entry.get(1).filter(|value| is_value_kind(value.kind)) else { + continue; + }; + let Some(behavior) = value.value.strip_prefix(&prefix) else { + continue; + }; + if behavior.is_empty() || !owned.contains(&value.value.as_str()) { + continue; + } + edits.push(rename_value_token( + text, + value, + &format!("{new}{TEST_NAME_SEPARATOR}{behavior}"), + EditKind::Header, + )); + } + } + Ok(edits) +} + /// Rename relationships `field` values pointing at a renamed column of a model. pub(crate) fn schema_column_edits( contents: &str, diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs index 3817d5ef15..2cde89557e 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs @@ -4,3 +4,4 @@ pub(crate) mod column_references; pub(crate) mod declaration_moves; pub(crate) mod model_planning; pub(crate) mod model_references; +pub(crate) mod unit_test_moves; diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs index 2a1bbc82e3..e277dc8676 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs @@ -15,6 +15,7 @@ use crate::refactoring::_helpers::planning::declaration_moves::declaration_moves use crate::refactoring::_helpers::planning::model_references::{ macro_reference_locations, model_reference_edits, }; +use crate::refactoring::_helpers::planning::unit_test_moves::{FollowedModel, unit_test_parts}; use crate::refactoring::_helpers::scanning::scan_context::ScanContext; use crate::refactoring::constants::{ CURRENT_DIRECTORY, HISTORY_MATERIALIZATIONS, MIGRATABLE_MATERIALIZATIONS, MIGRATE_FROM_KEY, @@ -183,16 +184,30 @@ pub(crate) fn model_parts( edits.extend(model_reference_edits(&files, (old, new), context)?); edits.extend(yaml_model_edits(&yaml_files(facts), old, new)?); } + let destination = target.destination(); + let tests = unit_test_parts( + Path::new(&facts.project_dir), + &facts.unit_tests, + &files, + &FollowedModel { + names: (old, new), + paths: (&target.source_path, &destination), + }, + )?; + edits.extend(tests.edits); let mut manual = macro_reference_locations(&facts.models, &files, old, context); manual.extend(facts.python_locations.iter().cloned()); let mut blocking = collisions(facts, target); blocking.extend(declaration.blocking); + blocking.extend(tests.blocking); blocking.extend(pending_migration(&files, target)?); + let mut moves = declaration.moves; + moves.extend(tests.moves); Ok(RefactorParts { edits, manual, blocking, - moves: declaration.moves, + moves, ..RefactorParts::default() }) } diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs new file mode 100644 index 0000000000..e9b0715319 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs @@ -0,0 +1,101 @@ +//! Unit test files and `TEST` names that follow a renamed or moved model. + +use std::collections::BTreeMap; +use std::path::Path; + +use crate::refactoring::_helpers::edits::header_edits::test_name_edits; +use crate::refactoring::_helpers::edits::text_edits::{RefactorParts, path_edits}; +use crate::refactoring::_helpers::files::paths::{join, name, parent, stem, suffix}; +use crate::refactoring::_helpers::files::project_files::ProjectSqlFile; +use crate::refactoring::constants::{ + CURRENT_DIRECTORY, MODELS_ROOT, TEST_FILE_PREFIX, TEST_NAME_SEPARATOR, UNIT_TEST_ROOT, +}; +use crate::refactoring::errors::RefactorError; +use crate::refactoring::models::{ManualLocation, UnitTestFacts}; + +/// The model's names and project-relative paths before and after the refactoring. +pub(crate) struct FollowedModel<'a> { + pub(crate) names: (&'a str, &'a str), + pub(crate) paths: (&'a str, &'a str), +} + +/// Rename `TEST` names of tests that target only the model, and move files holding only them. +pub(crate) fn unit_test_parts( + project_dir: &Path, + tests: &[UnitTestFacts], + files: &[ProjectSqlFile], + model: &FollowedModel<'_>, +) -> Result { + let (old, _) = model.names; + let mut by_path: BTreeMap<&str, Vec<&UnitTestFacts>> = BTreeMap::new(); + for test in tests { + by_path.entry(test.path.as_str()).or_default().push(test); + } + let mut parts = RefactorParts::default(); + for (path, blocks) in by_path { + let targets_only_model = |test: &UnitTestFacts| test.target_model_names == [old]; + let owned: Vec<&str> = blocks + .iter() + .filter(|test| targets_only_model(test)) + .filter_map(|test| test.name.as_deref()) + .collect(); + if model.names.0 != model.names.1 + && !owned.is_empty() + && let Some(file) = files.iter().find(|file| file.path == path) + { + parts.edits.extend(path_edits( + path, + test_name_edits(&file.contents, &file.text, model.names, &owned)?, + )); + } + if !blocks.iter().all(|test| targets_only_model(test)) { + continue; + } + let destination = test_destination(path, model); + if destination == path { + continue; + } + if join(project_dir, &destination).exists() + || parts + .moves + .iter() + .any(|(_, planned)| *planned == destination) + { + parts.blocking.push(ManualLocation { + path: destination.clone(), + line: None, + column: None, + reason: format!("the unit test {path} must move here, but the file already exists"), + }); + continue; + } + parts.moves.push((path.to_owned(), destination)); + } + Ok(parts) +} + +/// Where a test file of the model belongs after the refactoring. +fn test_destination(path: &str, model: &FollowedModel<'_>) -> String { + let (old, new) = model.names; + let file_stem = stem(path); + let old_stem = format!("{TEST_FILE_PREFIX}{old}"); + let new_stem = format!("{TEST_FILE_PREFIX}{new}"); + let renamed_stem = match file_stem.strip_prefix(&old_stem) { + Some("") => new_stem, + Some(rest) if rest.starts_with(TEST_NAME_SEPARATOR) => format!("{new_stem}{rest}"), + _ => file_stem.to_owned(), + }; + let directory = parent(path); + let mirrored = mirrored_test_directory(model.paths.0) + .zip(mirrored_test_directory(model.paths.1)) + .filter(|(before, _)| *before == directory) + .map(|(_, after)| after); + let directory = mirrored.unwrap_or(directory); + format!("{directory}/{renamed_stem}{}", suffix(name(path))) +} + +/// `tests/unit/` mirroring a model below `models//`, when it has folders. +fn mirrored_test_directory(model_path: &str) -> Option { + let folders = parent(model_path.strip_prefix(&format!("{MODELS_ROOT}/"))?); + (folders != CURRENT_DIRECTORY).then(|| format!("{UNIT_TEST_ROOT}/{folders}")) +} diff --git a/crates/sqlbuild-refactor/src/refactoring/constants.rs b/crates/sqlbuild-refactor/src/refactoring/constants.rs index 84645fd5f6..f540140c7c 100644 --- a/crates/sqlbuild-refactor/src/refactoring/constants.rs +++ b/crates/sqlbuild-refactor/src/refactoring/constants.rs @@ -62,6 +62,15 @@ pub const MODEL_RESOURCE_TYPE: &str = "model"; pub const UNKNOWN_COLUMN_TYPE: &str = "UNKNOWN"; pub const GENERIC_DIALECT: &str = "generic"; pub const SCHEMA_KEYWORD: &str = "SCHEMA"; +pub const TEST_KEYWORD: &str = "TEST"; +pub const TEST_NAME_KEY: &str = "name"; +/// Separates a unit test's subject from its behavior, in `TEST` names and file stems. +pub const TEST_NAME_SEPARATOR: &str = "__"; +/// The prefix of unit test file stems, as in `test___.sql`. +pub const TEST_FILE_PREFIX: &str = "test_"; +/// The root that holds unit tests, mirroring model folders below `models/`. +pub const UNIT_TEST_ROOT: &str = "tests/unit"; +pub const MODELS_ROOT: &str = "models"; pub const MODEL_MANUAL_HELP: &str = "Pass the model into the macro as an argument, or edit the listed locations, then run the command again."; pub const COLUMN_MANUAL_HELP: &str = "Edit the listed locations, then run the command again."; /// `RefactorInputError`'s default code. diff --git a/crates/sqlbuild-refactor/src/refactoring/models.rs b/crates/sqlbuild-refactor/src/refactoring/models.rs index 950a2ad7f7..f16c24efd1 100644 --- a/crates/sqlbuild-refactor/src/refactoring/models.rs +++ b/crates/sqlbuild-refactor/src/refactoring/models.rs @@ -234,6 +234,16 @@ pub struct AuthoredFile { pub texts: Vec, } +/// One compiled unit test, the file that declares it and the models it targets. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +pub struct UnitTestFacts { + /// The project-relative POSIX path of the test file. + pub path: String, + /// The `name` its `TEST` header declares, if any. + pub name: Option, + pub target_model_names: Vec, +} + /// Declaration files a model move takes along, and what blocks the move. #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct DeclarationMoves { @@ -286,6 +296,9 @@ pub struct RefactorFacts { pub authored_files: Vec, /// Discovered source and seed YAML declaration files, in discovery order. pub yaml_files: Vec, + /// Compiled unit tests, which follow a renamed or moved model they target alone. + #[serde(default)] + pub unit_tests: Vec, /// Python strings naming the renamed model or column, found by the Python host. pub python_locations: Vec, } diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index 7f01e62d17..1c182230b8 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -27,7 +27,7 @@ use crate::refactoring::errors::{RefactorError, RefactorErrorKind}; use crate::refactoring::models::{ AuthoredFile, DeclarationMoves, DeclarationPlacement, DestinationFacts, DiscoveredFile, EditKind, ExpansionSpan, FileChange, ModelFacts, PlacedDeclaration, RefactorFacts, - RefactorOperation, RefactorRequest, RelocatedDeclaration, SqlFileRole, TextEdit, + RefactorOperation, RefactorRequest, RelocatedDeclaration, SqlFileRole, TextEdit, UnitTestFacts, }; use crate::refactoring::tests::test_types::{ExpectedSpans, PlannedMoves, Spans}; @@ -292,6 +292,7 @@ pub(super) fn facts(project_dir: &str, models: Vec) -> RefactorFacts model_files: Vec::new(), authored_files: Vec::new(), yaml_files: Vec::new(), + unit_tests: Vec::new(), python_locations: Vec::new(), } } @@ -646,3 +647,66 @@ pub(super) fn owner_header_parts( .collect(); Ok((described(&edits), manual)) } + +/// Each planned file as `original -> path`, with the replacements made in it. +pub(super) fn followed_unit_tests( + request: &RefactorRequest, + tests: &[(&str, &str, &[&str])], +) -> Result)>, RefactorError> { + let mut orders: ModelFacts = model("orders", Some("view"), "analytics"); + orders.path = "models/marts/orders.sql".to_owned(); + let mut project_facts = facts("/nonexistent/project", vec![orders]); + for (path, name, targets) in tests { + project_facts.authored_files.push(AuthoredFile { + role: SqlFileRole::Test, + path: (*path).to_owned(), + contents: format!("TEST (name \"{name}\");\n\nSELECT 1\n"), + texts: vec!["SELECT 1".to_owned()], + }); + project_facts.unit_tests.push(UnitTestFacts { + path: (*path).to_owned(), + name: Some((*name).to_owned()), + target_model_names: targets.iter().map(|target| (*target).to_owned()).collect(), + }); + } + let host = |_: &str, _: &str| { + Ok(DeclarationPlacement { + relocated: Some(Vec::new()), + ..DeclarationPlacement::default() + }) + }; + let plan = + crate::refactoring::main::plan_refactor::plan_refactor(&project_facts, request, &host)?; + let mut changes: Vec<(String, Vec)> = plan + .changes + .into_iter() + .filter(|change| change.original_path.starts_with("tests/")) + .map(|change| { + ( + format!("{} -> {}", change.original_path, change.path), + change + .edits + .into_iter() + .map(|edit| edit.replacement) + .collect(), + ) + }) + .collect(); + changes.sort(); + Ok(changes) +} + +/// Expected `original -> path` test file changes with their replacements, sorted by path. +pub(super) fn followed_test_changes(changes: &[(&str, &[&str])]) -> Vec<(String, Vec)> { + let mut expected = changes + .iter() + .map(|(path, edits)| { + ( + (*path).to_owned(), + edits.iter().map(|edit| (*edit).to_owned()).collect(), + ) + }) + .collect::>(); + expected.sort(); + expected +} diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs index c396b8eb57..09858464d9 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs @@ -2,13 +2,13 @@ use crate::refactoring::_helpers::planning::model_planning::decide_model_migrati use crate::refactoring::errors::{RefactorError, RefactorErrorKind}; use crate::refactoring::models::{DeclarationPlacement, RefactorOperation}; use crate::refactoring::tests::helpers::{ - blocking_reasons, change, commit_failure, facts, files_on_disk, model, model_in_database, - owned_pairs, placed, planned_declaration_moves, refused_target, relocated, replace_all, - request, write_files, + blocking_reasons, change, commit_failure, facts, files_on_disk, followed_test_changes, + followed_unit_tests, model, model_in_database, owned_pairs, placed, planned_declaration_moves, + refused_target, relocated, replace_all, request, write_files, }; use crate::refactoring::tests::test_types::{ - CollisionTestCase, CommitTestCase, DeclarationMovesTestCase, MigrationDecisionTestCase, - RefusedTargetTestCase, + CollisionTestCase, CommitTestCase, DeclarationMovesTestCase, FollowedUnitTestTestCase, + MigrationDecisionTestCase, RefusedTargetTestCase, }; #[test] @@ -324,3 +324,58 @@ fn given_scope_placement_when_moving_across_folders_then_declaration_files_follo } Ok(()) } + +#[test] +fn given_unit_tests_of_model_when_renaming_or_moving_then_tests_follow() -> Result<(), RefactorError> +{ + let tests: &[(&str, &str, &[&str])] = &[ + ( + "tests/unit/marts/test_orders__keeps_rows.sql", + "orders__keeps_rows", + &["orders"], + ), + ( + "tests/unit/test_orders.sql", + "orders__flat_layout", + &["orders"], + ), + ( + "tests/unit/marts/test_marts__totals.sql", + "marts__totals", + &["orders", "customers"], + ), + ]; + let test_cases = [ + FollowedUnitTestTestCase { + description: "a rename renames owned files and TEST names in place", + request: request(RefactorOperation::RenameModel, "order_lines", None), + expected_changes: followed_test_changes(&[ + ( + "tests/unit/marts/test_orders__keeps_rows.sql -> tests/unit/marts/test_order_lines__keeps_rows.sql", + &["\"order_lines__keeps_rows\""] as &[&str], + ), + ( + "tests/unit/test_orders.sql -> tests/unit/test_order_lines.sql", + &["\"order_lines__flat_layout\""], + ), + ]), + }, + FollowedUnitTestTestCase { + description: "a move takes mirrored tests to the mirrored folder and leaves others", + request: request(RefactorOperation::MoveModel, "", Some("models/finance/")), + expected_changes: followed_test_changes(&[( + "tests/unit/marts/test_orders__keeps_rows.sql -> tests/unit/finance/test_orders__keeps_rows.sql", + &[] as &[&str], + )]), + }, + ]; + for test_case in test_cases { + assert_eq!( + followed_unit_tests(&test_case.request, tests)?, + test_case.expected_changes, + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs index 869adc4ac1..1b31da41f5 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs @@ -217,3 +217,10 @@ pub(super) struct HeaderPartsTestCase { pub(super) expected_edits: &'static [&'static str], pub(super) expected_manual: &'static [&'static str], } + +pub(super) struct FollowedUnitTestTestCase { + pub(super) description: &'static str, + pub(super) request: RefactorRequest, + /// Each `original -> path` test file change with its replacements, sorted by path. + pub(super) expected_changes: Vec<(String, Vec)>, +} diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md index 042f572a9e..97ab0cf67d 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md @@ -64,6 +64,14 @@ Renaming or moving a model rewrites: - `cursor_inputs` keys, `relationships (to ...)` audits, and `__ref("")` inside quoted SQL in `MODEL` headers and reusable `SCHEMA` declarations; - `relationships` audits (`to:` and `field:`) and `__ref("")` in source and seed YAML. +The model's own unit tests, the test files whose every `TEST` block targets only that model, follow it: + +- a file named `test_.sql` or `test___.sql` is renamed to the new name; +- `TEST (name "__")` takes the new name as its subject; +- a test file in the folder that mirrors the model, `tests/unit//` for `models//`, moves to the folder that mirrors the destination. Test files kept elsewhere stay where they are. + +Tests that also target other models keep their file and name; only their fixtures and references change. + Moving a model to another folder also moves the macros, enums, constants, reusable schemas, generic audits, and named hooks it uses when their [placement](../concepts/declaration-scopes/placement.md) must change, following the same rules `sqb compile` enforces: - a declaration only the moved model uses moves with it, into the destination folder's matching role; diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py index a5116cd00c..5497a0df56 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py @@ -132,3 +132,19 @@ class OtherRenameRefusalE2ETestCase: old_name: str new_name: str expected_codes: tuple[str, ...] + + +@dataclass(frozen=True) +class FollowedUnitTestE2ETestCase: + description: str + command: tuple[str, ...] + expected_fragments: dict[str, tuple[str, ...]] + extra_files: dict[str, str] = field(default_factory=dict) + + +@dataclass(frozen=True) +class FollowedUnitTestDryRunE2ETestCase: + description: str + command: tuple[str, ...] + expected_status: str + expected_path: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_models.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_models.py index 9f6d5ef417..ee5db6bb35 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_models.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_models.py @@ -109,7 +109,7 @@ expected_file="models/reporting/order_facts.sql", expected_fragments={ "models/marts/customer_totals.sql": ('__ref("order_facts")',), - "tests/unit/test_fact_orders.sql": ("__expected__order_facts AS (",), + "tests/unit/test_order_facts.sql": ("__expected__order_facts AS (",), }, expected_relation="order_facts", ), diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py new file mode 100644 index 0000000000..60bf0b9572 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py @@ -0,0 +1,132 @@ +"""CLI e2e coverage: a model's unit tests follow it through `sqb rename` and `sqb mv`.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.refactor._test_types import ( + FollowedUnitTestDryRunE2ETestCase, + FollowedUnitTestE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( + FACT_ORDERS_TEST, + fragments_present, + project_text, + project_toml, + remove_files, + sqb, + sqb_json, + write_orders_project, +) + +_MIRRORED_TEST: str = "tests/unit/marts/test_fact_orders__keeps_orders.sql" +_SHARED_TEST: str = "tests/unit/marts/test_marts__totals_follow_orders.sql" +_TEST_RULES: str = '\n[rules]\nselect = ["SQBRTEST102", "SQBRTEST103", "SQBRTEST104"]\n' +_CUSTOMER_TOTALS_FIXTURE: str = ( + "__expected__customer_totals AS (\n SELECT 10 AS customer_id, 25 AS total_amount\n)\n" +) +_FILES: dict[str, str] = { + "sqlbuild_project.toml": project_toml() + _TEST_RULES, + _MIRRORED_TEST: FACT_ORDERS_TEST.replace("TEST();", 'TEST (name "fact_orders__keeps_orders");'), +} +_SHARED_FILES: dict[str, str] = { + _SHARED_TEST: FACT_ORDERS_TEST.replace( + "TEST();", 'TEST (name "marts__totals_follow_orders");' + ).replace(")\nSELECT 1\n", f"),\n{_CUSTOMER_TOTALS_FIXTURE}SELECT 1\n"), +} + + +@pytest.mark.parametrize( + "test_case", + [ + FollowedUnitTestE2ETestCase( + description="rename renames the test file and its TEST name", + command=("rename", "model:fact_orders", "order_facts"), + expected_fragments={ + "tests/unit/marts/test_order_facts__keeps_orders.sql": ( + 'TEST (name "order_facts__keeps_orders");', + "__expected__order_facts", + ), + _SHARED_TEST: ( + 'TEST (name "marts__totals_follow_orders");', + "__expected__order_facts AS (", + ), + }, + extra_files=_SHARED_FILES, + ), + FollowedUnitTestE2ETestCase( + description="move to another folder moves the test to the mirrored folder", + command=("mv", "model:fact_orders", "models/reporting/"), + expected_fragments={ + "tests/unit/reporting/test_fact_orders__keeps_orders.sql": ( + 'TEST (name "fact_orders__keeps_orders");', + "__expected__fact_orders", + ), + }, + ), + FollowedUnitTestE2ETestCase( + description="move with a new name moves and renames the test", + command=("mv", "model:fact_orders", "models/reporting/order_facts.sql"), + expected_fragments={ + "tests/unit/reporting/test_order_facts__keeps_orders.sql": ( + 'TEST (name "order_facts__keeps_orders");', + "__expected__order_facts", + ), + }, + ), + ], + ids=lambda case: case.description, +) +def test_given_model_with_unit_test_when_refactored_then_test_follows_and_passes( + tmp_path: Path, test_case: FollowedUnitTestE2ETestCase +) -> None: + """The model's own test is renamed and moved with it; a shared test only has fixtures edited.""" + + project_dir: Path = write_orders_project( + tmp_path=tmp_path, files={**_FILES, **test_case.extra_files} + ) + remove_files(project_dir=project_dir, paths=("tests/unit/test_fact_orders.sql",)) + + result: subprocess.CompletedProcess[str] = sqb(project_dir, *test_case.command) + tested: subprocess.CompletedProcess[str] = sqb(project_dir, "test") + + assert result.returncode == 0, result.stdout + result.stderr + assert not (project_dir / _MIRRORED_TEST).exists() + files: dict[str, str] = project_text(project_dir) + assert fragments_present(files=files, expected=test_case.expected_fragments), files + assert tested.returncode == 0, tested.stdout + tested.stderr + + +@pytest.mark.parametrize( + "test_case", + [ + FollowedUnitTestDryRunE2ETestCase( + description="dry-run rename lists the renamed test file", + command=("rename", "model:fact_orders", "order_facts", "--dry-run"), + expected_status="dry_run", + expected_path="tests/unit/marts/test_order_facts__keeps_orders.sql", + ), + ], + ids=lambda case: case.description, +) +def test_given_dry_run_when_renaming_model_with_unit_test_then_reports_move_and_writes_nothing( + tmp_path: Path, test_case: FollowedUnitTestDryRunE2ETestCase +) -> None: + """A dry run lists the test file at its new path and leaves every file untouched.""" + + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=_FILES) + remove_files(project_dir=project_dir, paths=("tests/unit/test_fact_orders.sql",)) + before: dict[str, str] = project_text(project_dir) + + code: int + payload: dict[str, Any] + code, payload = sqb_json(project_dir, *test_case.command) + + assert code == 0, payload + assert payload["status"] == test_case.expected_status + assert test_case.expected_path in [str(item["path"]) for item in payload["files"]] + assert project_text(project_dir) == before diff --git a/website/src/content/docs/docs/cli/rename.mdx b/website/src/content/docs/docs/cli/rename.mdx index 4946eb90d2..bc6de89b0a 100644 --- a/website/src/content/docs/docs/cli/rename.mdx +++ b/website/src/content/docs/docs/cli/rename.mdx @@ -49,6 +49,14 @@ Renaming or moving a model rewrites: - `cursor_inputs` keys, `relationships (to ...)` audits, and `__ref("")` inside quoted SQL in `MODEL` headers and reusable `SCHEMA` declarations; - `relationships` audits (`to:` and `field:`) and `__ref("")` in source and seed YAML. +The model's own unit tests, the test files whose every `TEST` block targets only that model, follow it: + +- a file named `test_.sql` or `test___.sql` is renamed to the new name; +- `TEST (name "__")` takes the new name as its subject; +- a test file in the folder that mirrors the model, `tests/unit//` for `models//`, moves to the folder that mirrors the destination. Test files kept elsewhere stay where they are. + +Tests that also target other models keep their file and name; only their fixtures and references change. + Moving a model to another folder also moves the macros, enums, constants, reusable schemas, generic audits, and named hooks it uses when their [placement](/docs/concepts/declaration-scopes/placement) must change, following the same rules `sqb compile` enforces: - a declaration only the moved model uses moves with it, into the destination folder's matching role; From 39ed73034c1f152f2a58ec52039107861554b50d Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 07:57:12 +0100 Subject: [PATCH 02/15] fix: list USING joins and keep lambda parameters in audit-argument column renames An audit argument that joins on the renamed column with USING or NATURAL is now listed for a manual edit instead of failing the verification compile. The column analyser treats a lambda parameter named like the column as its own scope, so references in the lambda body are no longer renamed to the outer column. --- .../column_references/_helpers/references.rs | 35 +++++++++++++++++++ .../tests/test_column_references.rs | 15 ++++++++ .../_helpers/planning/column_references.rs | 10 ++++++ .../sqlbuild/references/docs/cli/rename.md | 2 +- .../cli/commands/main/refactor/helpers.py | 21 +++++++++++ .../main/refactor/test_rename_columns.py | 21 +++++++++++ website/src/content/docs/docs/cli/rename.mdx | 2 +- 7 files changed, 104 insertions(+), 2 deletions(-) diff --git a/crates/sqlbuild-analysis/src/column_references/_helpers/references.rs b/crates/sqlbuild-analysis/src/column_references/_helpers/references.rs index 392ffeb864..2282e89ede 100644 --- a/crates/sqlbuild-analysis/src/column_references/_helpers/references.rs +++ b/crates/sqlbuild-analysis/src/column_references/_helpers/references.rs @@ -346,6 +346,7 @@ impl Analysis<'_> { let Some(body) = select_body(expression) else { return; }; + let shadowed: HashSet<(usize, usize)> = self.lambda_parameter_uses(body); for node in walk_in_scope(body, false) { let Expression::Column(column) = node else { continue; @@ -353,10 +354,44 @@ impl Analysis<'_> { if column.name.name == STAR || column.name.name.to_lowercase() != self.column { continue; } + if column.table.is_none() + && column + .span + .is_some_and(|span| shadowed.contains(&(span.start, span.end))) + { + continue; + } self.record_column(column, select, chain, role); } } + /// Spans of unqualified uses of a lambda parameter named like the column, which read the parameter. + fn lambda_parameter_uses(&self, body: &Expression) -> HashSet<(usize, usize)> { + let mut shadowed: HashSet<(usize, usize)> = HashSet::new(); + for node in walk_in_scope(body, false) { + let Expression::Lambda(lambda) = node else { + continue; + }; + if !lambda + .parameters + .iter() + .any(|parameter| parameter.name.to_lowercase() == self.column) + { + continue; + } + for inner in walk_in_scope(&lambda.body, false) { + if let Expression::Column(column) = inner + && column.table.is_none() + && column.name.name.to_lowercase() == self.column + && let Some(span) = column.span + { + shadowed.insert((span.start, span.end)); + } + } + } + shadowed + } + fn record_column( &mut self, column: &polyglot_sql::expressions::Column, diff --git a/crates/sqlbuild-analysis/src/column_references/tests/test_column_references.rs b/crates/sqlbuild-analysis/src/column_references/tests/test_column_references.rs index ec7ab77d68..58a89993ad 100644 --- a/crates/sqlbuild-analysis/src/column_references/tests/test_column_references.rs +++ b/crates/sqlbuild-analysis/src/column_references/tests/test_column_references.rs @@ -76,6 +76,21 @@ fn given_column_uses_when_analyzing_then_reports_rename_sites() -> Result<(), St expected_output_aliases: &[], expected_outputs: 0, }, + ColumnReferenceTestCase { + description: "a lambda parameter named like the column shadows it in the body", + sql: "SELECT list_filter([amount], amount -> amount > 0) AS kept, \ + list_filter([1], lambda amount: amount > 0) AS other FROM orders_fact", + output_ctes: &[], + expected_parsed: true, + expected_names: &["amount"], + expected_scopes: &["root"], + expected_bare_projections: 0, + expected_aliases: &[], + expected_star_scopes: &[], + expected_joins: 0, + expected_output_aliases: &[], + expected_outputs: 0, + }, ColumnReferenceTestCase { description: "output scopes report aliased and bare projections of every branch", sql: "WITH fixture AS (SELECT 1 AS amount UNION ALL SELECT 2 AS amount) \ diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_references.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_references.rs index af00f40706..25c52b79a5 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_references.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_references.rs @@ -472,6 +472,16 @@ pub(crate) fn expression_edits( EditKind::Header, )); } + for join in &facts.joins { + result.manual.push(manual_at( + site.path, + site.contents, + join.start.and_then(locate).or(Some(site.offset)), + format!( + "USING or NATURAL join on {old} in this audit argument; rewrite it as an ON condition" + ), + )); + } for unresolved in &facts.unresolved { result.manual.push(manual_at( site.path, diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md index 97ab0cf67d..38bf749fcc 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md @@ -85,7 +85,7 @@ Renaming a column rewrites the model that produces it and every query that reads - the producing projection, as `amount AS revenue`, or the existing alias; - every downstream reference, including filters, joins, grouping, and ordering; - column declarations, including the model's own `MODEL (columns (...))` entries with or without commas between them, and column-valued config such as `unique_key`, `cursor`, and `partition_by`, and `cursor_inputs` and `relationships (field ...)` entries that point at the column; -- string arguments of audits attached in `MODEL` headers, such as `where_condition "is_flagged = TRUE"` or `expression "amount > 0"`, when the generic audit's SQL definition splices the argument in as SQL with `@where_condition`. These arguments are parsed as SQL expressions, so only references to the audited model's column change: string literals stay as written, and a subquery reading another model, such as `customer_id IN (SELECT customer_id FROM __ref("vip_customers"))`, keeps that model's column. You are asked to edit an argument by hand when it cannot be parsed, when it reads a relation whose columns SQLBuild does not know, when the audit quotes it as a string with `@'prefix'`, or when no SQL definition of the audit is found; +- string arguments of audits attached in `MODEL` headers, such as `where_condition "is_flagged = TRUE"` or `expression "amount > 0"`, when the generic audit's SQL definition splices the argument in as SQL with `@where_condition`. These arguments are parsed as SQL expressions, so only references to the audited model's column change: string literals stay as written, and a subquery reading another model, such as `customer_id IN (SELECT customer_id FROM __ref("vip_customers"))`, keeps that model's column. You are asked to edit an argument by hand when it cannot be parsed, when it reads a relation whose columns SQLBuild does not know, when the audit quotes it as a string with `@'prefix'`, when it joins on the column with `USING` or `NATURAL`, or when no SQL definition of the audit is found. A lambda parameter named like the column, as in `list_filter(xs, customer_id -> customer_id > 0)`, keeps its name; - fixture columns of `__ref__` and `__expected__` CTEs. By default a column rename is one step: downstream models keep their output names, so `SELECT o.amount` becomes `SELECT o.revenue AS amount`. With `--cascade`, a downstream model that passes the column through unchanged, as a bare column, a same-named alias, or `SELECT *`, renames its output too, and the rename continues through its own consumers. diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py index a1b8b383a4..ca0ac74105 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py @@ -251,6 +251,27 @@ def order_amount_union(*, first: str, second: str) -> dict[str, str]: " ],\n);", ), } +FLAGGED_USING_AUDIT: dict[str, str] = { + **FLAGGED_SUBQUERY_AUDITS, + "models/marts/flagged_orders.sql": FLAGGED_ORDERS.replace( + " ),\n);", + " ),\n audits [\n" + ' flag_check (name "joined_flag", where_condition ' + '\'order_id IN (SELECT o.order_id FROM __ref("flagged_orders") AS o ' + 'JOIN __ref("vip_flags") AS v USING (is_flagged))\'),\n' + " ],\n);", + ), +} +FLAGGED_LAMBDA_AUDIT: dict[str, str] = { + "models/marts/_sqlbuild/_audits/generic/flag_check.sql": _FLAG_CHECK, + "models/marts/flagged_orders.sql": FLAGGED_ORDERS.replace( + " ),\n);", + " ),\n audits [\n" + ' flag_check (name "lambda_flag", where_condition ' + "'len(list_filter([is_flagged], lambda is_flagged: is_flagged)) > 0'),\n" + " ],\n);", + ), +} FLAGGED_LITERAL_AUDIT: dict[str, str] = { "models/marts/_sqlbuild/_audits/generic/prefix_check.sql": ( "AUDIT ();\n\nSELECT *\nFROM @relation\nWHERE @'prefix' = ''\n" diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_columns.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_columns.py index f96ce95eec..9dc7396f87 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_columns.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_rename_columns.py @@ -20,8 +20,10 @@ CUSTOMER_DECLARATIONS, FLAGGED_AUDITS, FLAGGED_CONTRACT, + FLAGGED_LAMBDA_AUDIT, FLAGGED_LITERAL_AUDIT, FLAGGED_SUBQUERY_AUDITS, + FLAGGED_USING_AUDIT, ORDER_AMOUNT_CHECK, ORDER_EXPORT, ORDER_SHAPE, @@ -178,6 +180,17 @@ "vip_flags": ("is_flagged",), }, ), + ColumnRenameE2ETestCase( + description="audit lambda parameters named like the column keep their name", + command=("rename", "column:flagged_orders.is_flagged", "is_marked"), + extra_files=FLAGGED_LAMBDA_AUDIT, + expected_fragments={ + "models/marts/flagged_orders.sql": ( + "'len(list_filter([is_marked], lambda is_flagged: is_flagged)) > 0'", + ), + }, + expected_columns={"flagged_orders": ("order_id", "is_marked")}, + ), ], ids=lambda case: case.description, ) @@ -266,6 +279,14 @@ def test_given_column_rename_when_applied_then_project_tests_and_builds( expected_reason="audit prefix_check quotes argument prefix as a string", expected_paths=("models/marts/flagged_orders.sql",), ), + RefusedRefactorE2ETestCase( + description="audit argument joining on the column with USING needs a manual edit", + command=("rename", "column:flagged_orders.is_flagged", "is_marked"), + extra_files=FLAGGED_USING_AUDIT, + expected_status="refused", + expected_reason="USING or NATURAL join on is_flagged in this audit argument", + expected_paths=("models/marts/flagged_orders.sql",), + ), ], ids=lambda case: case.description, ) diff --git a/website/src/content/docs/docs/cli/rename.mdx b/website/src/content/docs/docs/cli/rename.mdx index bc6de89b0a..01d053e8e4 100644 --- a/website/src/content/docs/docs/cli/rename.mdx +++ b/website/src/content/docs/docs/cli/rename.mdx @@ -70,7 +70,7 @@ Renaming a column rewrites the model that produces it and every query that reads - the producing projection, as `amount AS revenue`, or the existing alias; - every downstream reference, including filters, joins, grouping, and ordering; - column declarations, including the model's own `MODEL (columns (...))` entries with or without commas between them, and column-valued config such as `unique_key`, `cursor`, and `partition_by`, and `cursor_inputs` and `relationships (field ...)` entries that point at the column; -- string arguments of audits attached in `MODEL` headers, such as `where_condition "is_flagged = TRUE"` or `expression "amount > 0"`, when the generic audit's SQL definition splices the argument in as SQL with `@where_condition`. These arguments are parsed as SQL expressions, so only references to the audited model's column change: string literals stay as written, and a subquery reading another model, such as `customer_id IN (SELECT customer_id FROM __ref("vip_customers"))`, keeps that model's column. You are asked to edit an argument by hand when it cannot be parsed, when it reads a relation whose columns SQLBuild does not know, when the audit quotes it as a string with `@'prefix'`, or when no SQL definition of the audit is found; +- string arguments of audits attached in `MODEL` headers, such as `where_condition "is_flagged = TRUE"` or `expression "amount > 0"`, when the generic audit's SQL definition splices the argument in as SQL with `@where_condition`. These arguments are parsed as SQL expressions, so only references to the audited model's column change: string literals stay as written, and a subquery reading another model, such as `customer_id IN (SELECT customer_id FROM __ref("vip_customers"))`, keeps that model's column. You are asked to edit an argument by hand when it cannot be parsed, when it reads a relation whose columns SQLBuild does not know, when the audit quotes it as a string with `@'prefix'`, when it joins on the column with `USING` or `NATURAL`, or when no SQL definition of the audit is found. A lambda parameter named like the column, as in `list_filter(xs, customer_id -> customer_id > 0)`, keeps its name; - fixture columns of `__ref__` and `__expected__` CTEs. By default a column rename is one step: downstream models keep their output names, so `SELECT o.amount` becomes `SELECT o.revenue AS amount`. With `--cascade`, a downstream model that passes the column through unchanged, as a bare column, a same-named alias, or `SELECT *`, renames its output too, and the rename continues through its own consumers. From 7520c48356fb9c9232a7f151c2957f865f09ea1d Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 08:11:14 +0100 Subject: [PATCH 03/15] fix: resume lifecycle sink delivery after one slow call A sink call over the one-second timeout used to block that sink for the rest of the command, silently dropping every later event including run and invocation outcomes. Delivery now resumes on a fresh worker once the slow call returns; events dropped meanwhile are still counted, outcome events are held and delivered instead of dropped, and the command warns when the stall begins. The timeout clock only charges calls the delivery thread has started and caps each dispatcher round's charge, so time spent waiting for the GIL is not blamed on the sink. The exit warning now points at sink health and the shutdown deadline instead of only the shutdown timeout. --- .../src/dispatch/main/event_export_warning.rs | 6 +- .../tests/test_event_export_warning.rs | 12 +- .../_helpers/event_exporting/constants.rs | 2 + .../_helpers/event_exporting/dispatcher.rs | 12 ++ .../_helpers/event_exporting/workers.rs | 177 +++++++++++++----- .../docs/concepts/observability/sinks.md | 7 +- .../cli/commands/main/audit/_test_types.py | 9 + .../cli/commands/main/audit/helpers.py | 26 +++ .../main/audit/test_audit_event_delivery.py | 52 ++++- .../main/providers/test_event_exporters.py | 5 +- .../classes/test_dispatcher.py | 50 +++++ .../docs/concepts/observability/sinks.mdx | 7 +- 12 files changed, 314 insertions(+), 51 deletions(-) diff --git a/crates/sqlbuild-cli/src/dispatch/main/event_export_warning.rs b/crates/sqlbuild-cli/src/dispatch/main/event_export_warning.rs index 365528aa8f..b8c3c861e6 100644 --- a/crates/sqlbuild-cli/src/dispatch/main/event_export_warning.rs +++ b/crates/sqlbuild-cli/src/dispatch/main/event_export_warning.rs @@ -1,5 +1,7 @@ //! The stderr warning for lifecycle events dropped or failed at command exit. +use sqlbuild_core::event_export::constants::DEFAULT_EVENT_EXPORT_INVOCATION_TIMEOUT_SECONDS; + use crate::dispatch::constants::SHUTDOWN_TIMEOUT_SETTING; use crate::dispatch::models::{ExportSummary, ExporterLoss}; @@ -32,7 +34,9 @@ pub fn event_export_warning(summary: &ExportSummary, command_succeeded: bool) -> }; Some(format!( "{prefix}: {} of {} {unit} dropped, {} failed ({sink_label} {names}). \ - Increase {SHUTDOWN_TIMEOUT_SETTING} or check sink health.", + Check sink health: a sink call slower than {DEFAULT_EVENT_EXPORT_INVOCATION_TIMEOUT_SECONDS}s \ + drops that sink's events until it returns; events still queued at exit need a longer \ + {SHUTDOWN_TIMEOUT_SETTING}.", summary.dropped, summary.accepted, summary.failed )) } diff --git a/crates/sqlbuild-cli/src/dispatch/tests/test_event_export_warning.rs b/crates/sqlbuild-cli/src/dispatch/tests/test_event_export_warning.rs index c799b4b25a..ae9ff3eb88 100644 --- a/crates/sqlbuild-cli/src/dispatch/tests/test_event_export_warning.rs +++ b/crates/sqlbuild-cli/src/dispatch/tests/test_event_export_warning.rs @@ -18,7 +18,9 @@ fn given_final_export_summary_when_formatting_warning_then_reports_loss_only() { expected_warning: Some( "Warning: command completed successfully, but lifecycle event export was \ incomplete: 140 of 248 events dropped, 0 failed (sink 'orders_events'). \ - Increase sinks.lifecycle.shutdown_timeout or check sink health.", + Check sink health: a sink call slower than 1s drops that sink's events until it \ + returns; events still queued at exit need a longer \ + sinks.lifecycle.shutdown_timeout.", ), }, EventExportWarningTestCase { @@ -28,7 +30,9 @@ fn given_final_export_summary_when_formatting_warning_then_reports_loss_only() { expected_warning: Some( "Warning: lifecycle event export was incomplete: 0 of 10 events dropped, \ 1 failed (sink 'orders_events'). \ - Increase sinks.lifecycle.shutdown_timeout or check sink health.", + Check sink health: a sink call slower than 1s drops that sink's events until it \ + returns; events still queued at exit need a longer \ + sinks.lifecycle.shutdown_timeout.", ), }, EventExportWarningTestCase { @@ -41,7 +45,9 @@ fn given_final_export_summary_when_formatting_warning_then_reports_loss_only() { expected_warning: Some( "Warning: command completed successfully, but lifecycle event export was \ incomplete: 4 of 40 event deliveries dropped, 1 failed (sink 'orders_events'). \ - Increase sinks.lifecycle.shutdown_timeout or check sink health.", + Check sink health: a sink call slower than 1s drops that sink's events until it \ + returns; events still queued at exit need a longer \ + sinks.lifecycle.shutdown_timeout.", ), }, ]; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/constants.rs b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/constants.rs index 6ceb9fe54e..40a01f27cc 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/constants.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/constants.rs @@ -11,6 +11,8 @@ pub(crate) const INVOCATION_THREAD_PREFIX: &str = "sqlbuild-event-exporter-"; pub(crate) const WORKER_ATTRIBUTE: &str = "_sqlbuild_event_exporter_worker"; /// The longest single wait of a polling delivery loop, as the Python dispatcher polled. pub(crate) const POLL_SECONDS: f64 = 0.01; +/// The most one dispatch round charges a sink call, so the dispatcher's GIL waits are not counted. +pub(crate) const CHARGE_LIMIT_SECONDS: f64 = 2.0 * POLL_SECONDS; /// Extra time `shutdown` waits for the dispatcher thread beyond the drain deadline. pub(crate) const JOIN_GRACE_SECONDS: f64 = 0.05; /// How long shutdown and interpreter exit wait for each stopped dispatcher thread. diff --git a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/dispatcher.rs b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/dispatcher.rs index 673e708842..7d746e5fdf 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/dispatcher.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/dispatcher.rs @@ -95,6 +95,14 @@ pub(crate) struct State { pub(crate) stopping: bool, pub(crate) force_stop: bool, pub(crate) blocked: HashSet, + /// The exporter of each abandoned sink call that may still be running, by attempt token. + pub(crate) stalled: HashMap, + /// Outcome events owed to blocked exporters, delivered once their stalled call returns. + pub(crate) held: Vec<(usize, Queued)>, + /// Exporters whose sink call timed out since the last dispatch round, to warn about. + pub(crate) newly_blocked: Vec, + /// When the dispatcher last charged running sink calls toward their timeout. + pub(crate) last_charged: Option, pub(crate) live_invocations: HashSet, pub(crate) next_invocation: u64, dispatcher_running: bool, @@ -699,6 +707,10 @@ impl EventExporterDispatcher { stopping: false, force_stop: false, blocked: HashSet::new(), + stalled: HashMap::new(), + held: Vec::new(), + newly_blocked: Vec::new(), + last_charged: None, live_invocations: HashSet::new(), next_invocation: 0, dispatcher_running: true, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs index 8ed535cbd5..533e25d678 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs @@ -10,14 +10,12 @@ use sqlbuild_core::event_export::constants::TIMEOUT_ERROR_TYPE; use sqlbuild_core::event_export::main::drop_lifecycle_attempts::drop_lifecycle_attempts; use sqlbuild_core::event_export::main::retain_outcome_events::retain_outcome_events; use sqlbuild_core::event_export::main::take_lifecycle_event::take_lifecycle_event; -use sqlbuild_core::event_export::models::{ - EventQueue, ExportSeverity, ExportSnapshot, QueuedEvent, -}; +use sqlbuild_core::event_export::models::{EventQueue, ExportSnapshot, QueuedEvent}; use sqlbuild_host::entries::models::UserCodeHost; use crate::bindings::_helpers::boundary::python_calls::{call_with, imported}; use crate::bindings::_helpers::event_exporting::constants::{ - INVOCATION_THREAD_PREFIX, LANE_WINDOW, NO_SECONDS, POLL_SECONDS, + CHARGE_LIMIT_SECONDS, INVOCATION_THREAD_PREFIX, LANE_WINDOW, NO_SECONDS, POLL_SECONDS, }; use crate::bindings::_helpers::event_exporting::dispatcher::{Exporter, Queued, Shared, State}; use crate::bindings::_helpers::event_exporting::models::{ @@ -25,6 +23,7 @@ use crate::bindings::_helpers::event_exporting::models::{ }; use crate::bindings::_helpers::event_exporting::values::error_type_of; use crate::bindings::_helpers::native_host::host; +use crate::bindings::_helpers::recall::console::write_stderr; /// Which long-lived dispatcher thread a worker runs. #[derive(Clone, Copy)] @@ -41,11 +40,13 @@ pub(crate) struct DispatcherWorker { role: WorkerRole, } +/// Outcome events held for blocked exporters, each with its exporter index. +type HeldOutcomes = Vec<(usize, Queued)>; + /// One attempt handed to a lane and not yet reported. pub(crate) struct Attempt { token: u64, - kind: String, - severity: ExportSeverity, + queued: Queued, outcome: bool, } @@ -54,8 +55,8 @@ pub(crate) struct Lane { calls: Py, thread: Py, pending: VecDeque, - /// When the attempt at the front of `pending` began, approximately. - head_started: Instant, + /// The running attempt at the front of `pending` and the time charged to it so far. + timed: Option<(u64, Duration)>, } /// What one dispatch round reads from the dispatcher besides its state. @@ -102,7 +103,7 @@ impl Lane { calls: calls.unbind(), thread: thread.unbind(), pending: VecDeque::new(), - head_started: Instant::now(), + timed: None, }) } @@ -133,6 +134,10 @@ impl State { claims.del_item(token)?; } let Some(index) = self.tokens.remove(&token) else { + if let Some(index) = self.stalled.remove(&token) { + self.abandoned_calls = !self.stalled.is_empty(); + notify |= self.resume(py, index, round)?; + } continue; }; let Some(lane) = self.lanes.get_mut(index).and_then(Option::as_mut) else { @@ -148,12 +153,11 @@ impl State { let Some(attempt) = lane.pending.remove(position) else { continue; }; - lane.head_started = Instant::now(); match error_type { Some(error_type) => { notify |= self.note_failure( round.diagnostics, - (index, &attempt.kind, attempt.severity), + (index, &attempt.queued.kind, attempt.queued.severity), error_type, ); } @@ -178,39 +182,72 @@ impl State { let claims: &Bound<'_, PyDict> = claims.bind(py); let mut claimed: Vec = Vec::new(); for attempt in attempts { - let already_claimed: bool = match claims.get_item(attempt.token)? { - Some(claim) => claim.is_truthy()?, - None => false, - }; - if already_claimed { + if claim(claims, attempt.token)? == Some(true) { claimed.push(attempt); continue; } - claims.set_item(attempt.token, false)?; + cancel(claims, attempt.token)?; self.tokens.remove(&attempt.token); self.counts = drop_lifecycle_attempts(std::mem::take(&mut self.counts), &[index]); } Ok(claimed) } - /// Retire `index`'s lane: calls it already claimed fail as timed out and the rest are dropped. - fn abandon_lane(&mut self, py: Python<'_>, index: usize, round: Round<'_>) -> PyResult { + /// Unblock `index` once its stalled call returned, delivering its held outcomes on a fresh lane. + fn resume(&mut self, py: Python<'_>, index: usize, round: Round<'_>) -> PyResult { + if self.stalled.values().any(|stalled| *stalled == index) { + return Ok(false); + } + self.blocked.remove(&index); + let (owed, kept): (HeldOutcomes, HeldOutcomes) = std::mem::take(&mut self.held) + .into_iter() + .partition(|(exporter, _)| *exporter == index); + self.held = kept; + let mut notify: bool = false; + for (_, queued) in owed { + notify |= self.offer(py, index, (&queued, true), round)?; + } + Ok(notify) + } + + /// Retire `index`'s lane: claimed calls fail as timed out and block it until they return. + fn abandon_lane( + &mut self, + py: Python<'_>, + (index, timed_out): (usize, bool), + round: Round<'_>, + ) -> PyResult { let Some(mut lane) = self.lanes.get_mut(index).and_then(Option::take) else { return Ok(false); }; - let attempts: VecDeque = std::mem::take(&mut lane.pending); + let claims: &Bound<'_, PyDict> = round.shared.claims.bind(py); + let mut attempts: Vec = Vec::new(); + for attempt in std::mem::take(&mut lane.pending) { + if timed_out && attempt.outcome && claim(claims, attempt.token)?.is_none() { + cancel(claims, attempt.token)?; + self.tokens.remove(&attempt.token); + self.held.push((index, attempt.queued)); + } else { + attempts.push(attempt); + } + } let claimed: Vec = self.cancel_unclaimed(py, index, attempts, &round.shared.claims)?; - let claims: &Bound<'_, PyDict> = round.shared.claims.bind(py); let mut notify: bool = false; for running in claimed { claims.del_item(running.token)?; self.tokens.remove(&running.token); - self.blocked.insert(index); + self.stalled.insert(running.token, index); + if self.blocked.insert(index) + && timed_out + && let Some(exporter) = self.exporters.get(index) + { + self.newly_blocked.push(exporter.name.clone()); + } self.abandoned_calls = true; notify |= self.note_failure( round.diagnostics, - (index, &running.kind, running.severity), + (index, &running.queued.kind, running.queued.severity), TIMEOUT_ERROR_TYPE.to_owned(), ); } @@ -219,20 +256,35 @@ impl State { Ok(notify) } - /// Abandon every lane whose current sink call outlived `timeout`. + /// Abandon lanes whose started call outlived `timeout`, charging each round at most `CHARGE_LIMIT_SECONDS`. fn expire_lanes( &mut self, py: Python<'_>, timeout: Duration, round: Round<'_>, ) -> PyResult { + let now: Instant = Instant::now(); + let charge: Duration = self.last_charged.map_or(Duration::ZERO, |last| { + now.saturating_duration_since(last) + .min(Duration::from_secs_f64(CHARGE_LIMIT_SECONDS)) + }); + self.last_charged = Some(now); + let claims: &Bound<'_, PyDict> = round.shared.claims.bind(py); let mut notify: bool = false; for index in 0..self.lanes.len() { - let expired: bool = self.lanes[index].as_ref().is_some_and(|lane| { - !lane.pending.is_empty() && lane.head_started.elapsed() >= timeout + let Some(lane) = self.lanes[index].as_mut() else { + continue; + }; + let running: Option = match lane.pending.front() { + Some(head) if claim(claims, head.token)? == Some(true) => Some(head.token), + _ => None, + }; + lane.timed = running.map(|token| match lane.timed { + Some((timed, charged)) if timed == token => (token, charged + charge), + _ => (token, Duration::ZERO), }); - if expired { - notify |= self.abandon_lane(py, index, round)?; + if lane.timed.is_some_and(|(_, charged)| charged >= timeout) { + notify |= self.abandon_lane(py, (index, true), round)?; } } Ok(notify) @@ -295,11 +347,20 @@ impl State { } for index in &item.eligible_exporters { if self.blocked.contains(index) { - self.counts = - drop_lifecycle_attempts(std::mem::take(&mut self.counts), &[*index]); + if item.outcome { + let queued: Queued = Queued { + event: item.item.event.clone_ref(py), + kind: item.item.kind.clone(), + severity: item.item.severity, + }; + self.held.push((*index, queued)); + } else { + self.counts = + drop_lifecycle_attempts(std::mem::take(&mut self.counts), &[*index]); + } continue; } - notify |= self.offer(py, *index, &item, round)?; + notify |= self.offer(py, *index, (&item.item, item.outcome), round)?; } } Ok(notify) @@ -310,7 +371,7 @@ impl State { &mut self, py: Python<'_>, index: usize, - item: &QueuedEvent, + (item, outcome): (&Queued, bool), round: Round<'_>, ) -> PyResult { let Some(exporter) = self.exporters.get(index).map(Arc::clone) else { @@ -325,7 +386,7 @@ impl State { Err(error) => { return Ok(self.note_failure( round.diagnostics, - (index, &item.item.kind, item.item.severity), + (index, &item.kind, item.severity), error_type_of(py, &error), )); } @@ -341,18 +402,18 @@ impl State { (( exporter.function.bind(py), exporter.provider_arguments.bind(py), - item.item.event.bind(py), + item.event.bind(py), token, ),), )?; - if lane.pending.is_empty() { - lane.head_started = Instant::now(); - } lane.pending.push_back(Attempt { token, - kind: item.item.kind.clone(), - severity: item.item.severity, - outcome: item.outcome, + queued: Queued { + event: item.event.clone_ref(py), + kind: item.kind.clone(), + severity: item.severity, + }, + outcome, }); self.tokens.insert(token, index); Ok(false) @@ -377,6 +438,31 @@ impl State { } } +/// Whether a delivery thread claimed `token` (`Some(true)`), it was cancelled, or neither (`None`). +fn claim(claims: &Bound<'_, PyDict>, token: u64) -> PyResult> { + claims + .get_item(token)? + .map(|claim| claim.is_truthy()) + .transpose() +} + +/// Mark `token` cancelled, so its delivery thread skips the call. +fn cancel(claims: &Bound<'_, PyDict>, token: u64) -> PyResult<()> { + claims.set_item(token, false) +} + +/// Tell the user, once per stall, that one sink call outlived its timeout and what that costs. +fn warn_blocked(py: Python<'_>, name: &str, timeout: Duration) -> PyResult<()> { + write_stderr( + py, + &format!( + "Warning: lifecycle sink '{name}' did not return from one call within {}s; \ + its events are dropped until that call returns.\n", + timeout.as_secs_f64() + ), + ) +} + impl DispatcherWorker { pub(crate) fn new(shared: Arc, role: WorkerRole) -> Self { Self { shared, role } @@ -441,7 +527,10 @@ impl DispatcherWorker { ); } for index in 0..state.lanes.len() { - notify |= state.abandon_lane(py, index, round)?; + notify |= state.abandon_lane(py, (index, false), round)?; + } + for (index, _) in std::mem::take(&mut state.held) { + state.counts = drop_lifecycle_attempts(std::mem::take(&mut state.counts), &[index]); } drop(drained); true @@ -452,9 +541,13 @@ impl DispatcherWorker { state.reserve_for_outcomes(py, &shared.claims)?; } notify |= state.feed(py, round)?; - state.stopping && !state.busy() && state.queue.items.is_empty() + state.stopping && !state.busy() && state.queue.items.is_empty() && state.held.is_empty() }; + let blocked: Vec = std::mem::take(&mut state.newly_blocked); drop(state); + for name in blocked { + warn_blocked(py, &name, shared.invocation_timeout)?; + } if notify { shared.notify_all(py)?; } diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/sinks.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/sinks.md index c66d46e273..589612e3b7 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/sinks.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/sinks.md @@ -110,11 +110,16 @@ durability. SQLBuild core does not provide a Kafka or ClickHouse implementation. Run and invocation outcomes (`run_started`, `run_completed`, `run_failed`, `invocation_started`, `invocation_completed`, `invocation_failed`) are always admitted and never displaced. - A failing or timed-out sink is isolated from command correctness. +- Each sink call has one second to return. When a call takes longer, the command prints + `Warning: lifecycle sink '' did not return from one call within 1s; its events are dropped until that call returns.` + That call counts as failed and the sink's events are dropped and counted while it runs. Run and + invocation outcomes are kept instead and delivered, with every later event, once the call returns. + Time SQLBuild itself spends waiting for Python's GIL does not count against the call. - `audit_completed` facts are published as each audit finishes (model audits in `sqb build` when their model finishes), not in one burst at the end of the run. - When lifecycle events are dropped or fail, the command prints one warning to stderr with the counts and affected sinks, for example - `Warning: command completed successfully, but lifecycle event export was incomplete: 140 of 248 events dropped, 0 failed (sink 'orders_events'). Increase sinks.lifecycle.shutdown_timeout or check sink health.` + `Warning: command completed successfully, but lifecycle event export was incomplete: 140 of 248 events dropped, 0 failed (sink 'orders_events'). Check sink health: a sink call slower than 1s drops that sink's events until it returns; events still queued at exit need a longer sinks.lifecycle.shutdown_timeout.` A successful command then ends with `sqb audit completed successfully; only its lifecycle event export is affected.` The exit code and stdout, including `--json` output, are unchanged. diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py index f1f3f57f1b..93eb9a0cac 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py @@ -47,6 +47,15 @@ class SlowSinkFullDeliveryE2ETestCase: expected_delivered: int +@dataclass(frozen=True) +class SlowOnceSinkE2ETestCase: + """A sink with one call slower than the call timeout and the warning it must print.""" + + description: str + slow_call: int + expected_warning: str + + @dataclass(frozen=True) class InvalidDrainTimeoutE2ETestCase: """One invalid lifecycle drain timeout value.""" diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py index f610101374..b01d6ea289 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py @@ -93,6 +93,32 @@ def record_event(event: LifecycleEvent) -> None: ).lstrip() +def slow_once_sink_source(*, slow_call: int) -> str: + """Return a lifecycle sink whose call number ``slow_call`` sleeps past the 1s call timeout.""" + + return dedent( + f""" + import os + import time + from pathlib import Path + + from sqlbuild.sinks import LifecycleEvent, lifecycle_event_sink, lifecycle_event_to_json + + CALLS: list[int] = [] + + + @lifecycle_event_sink(name="slow_once_events") + def record_event(event: LifecycleEvent) -> None: + CALLS.append(1) + if len(CALLS) == {slow_call}: + time.sleep(1.5) + path = Path(os.environ["ORDERS_EVENT_PATH"]) + with path.open("a", encoding="utf-8") as stream: + stream.write(lifecycle_event_to_json(event) + "\\n") + """ + ).lstrip() + + def read_sequenced_events(path: Path) -> list[dict[str, object]]: """Return recorded events ordered by canonical invocation sequence.""" diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_audit_event_delivery.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_audit_event_delivery.py index 1e7c4fc1c6..58189b2797 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_audit_event_delivery.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_audit_event_delivery.py @@ -12,6 +12,7 @@ from tests.e2e.src.sqlbuild.cli.commands.main.audit._test_types import ( IncrementalAuditEventE2ETestCase, InvalidDrainTimeoutE2ETestCase, + SlowOnceSinkE2ETestCase, SlowSinkDropE2ETestCase, SlowSinkFullDeliveryE2ETestCase, ) @@ -21,6 +22,7 @@ events_of_type, read_sequenced_events, recording_sink_source, + slow_once_sink_source, ) from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import ( prepare_inline_project, @@ -32,7 +34,8 @@ r"Warning: command completed successfully, but lifecycle event export was incomplete: " r"(?P\d+) of (?P\d+) events dropped, (?P\d+) failed " r"\(sink 'slow_orders_events'\)\. " - r"Increase sinks\.lifecycle\.shutdown_timeout or check sink health\." + r"Check sink health: a sink call slower than 1s drops that sink's events until it returns; " + r"events still queued at exit need a longer sinks\.lifecycle\.shutdown_timeout\." ) @@ -267,5 +270,52 @@ def test_given_invalid_drain_timeout_when_auditing_then_fails_with_config_error( assert test_case.expected_error in result.stdout + result.stderr +@pytest.mark.parametrize( + "test_case", + ( + SlowOnceSinkE2ETestCase( + description="tenth call sleeps past the timeout", + slow_call=10, + expected_warning=( + "Warning: lifecycle sink 'slow_once_events' did not return from one call within 1s" + ), + ), + ), + ids=lambda case: case.description, +) +def test_given_sink_slow_once_when_building_then_later_and_outcome_events_still_arrive( + test_case: SlowOnceSinkE2ETestCase, + tmp_path: Path, +) -> None: + """One call over the timeout pauses the sink only until that call returns.""" + + event_path: Path = tmp_path / "orders-events.jsonl" + event_path.touch() + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="audit_event_delivery", + repo_files={ + "sqlbuild_project.toml": delivery_project_toml(lifecycle_config=""), + **delivery_model_files(), + "sinks/orders_events.py": slow_once_sink_source(slow_call=test_case.slow_call), + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "build"), + project_dir=project_dir, + env={"ORDERS_EVENT_PATH": str(event_path)}, + ) + + assert result.returncode == 0, result.stdout + result.stderr + events: list[dict[str, object]] = read_sequenced_events(event_path) + event_types: list[object] = [event["event_type"] for event in events] + assert "run_completed" in event_types + assert event_types[-1] == "invocation_completed" + assert len(event_types) > test_case.slow_call + assert test_case.expected_warning in result.stderr + assert "Check sink health" in result.stderr + + if __name__ == "__main__": pytest.main([__file__, "-vv"]) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/providers/test_event_exporters.py b/tests/e2e/src/sqlbuild/cli/commands/main/providers/test_event_exporters.py index 79d1d9a0ea..2da770b29a 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/providers/test_event_exporters.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/providers/test_event_exporters.py @@ -332,8 +332,9 @@ def test_given_no_exporters_and_exploding_provider_when_non_provider_command_run expected_errors_only=("invocation_failed",), expected_warning=( "Warning: lifecycle event export was incomplete: 0 of {accepted} event " - "deliveries dropped, 1 failed (sink 'failing_on_errors'). Increase " - "sinks.lifecycle.shutdown_timeout or check sink health." + "deliveries dropped, 1 failed (sink 'failing_on_errors'). Check sink health: a " + "sink call slower than 1s drops that sink's events until it returns; events " + "still queued at exit need a longer sinks.lifecycle.shutdown_timeout." ), ), ), diff --git a/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py b/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py index 0eb28211fb..6ca4da21b6 100644 --- a/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py +++ b/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py @@ -890,3 +890,53 @@ def record(*, event: LifecycleEvent) -> None: assert results.qsize() == test_case.expected_delivered assert results.get_nowait() == (1, None) assert claims == {1: True} + + +@pytest.mark.parametrize( + "test_case", + (EventExporterDispatcherTestCase("delivery resumes after the slow call returns", 5),), + ids=lambda case: case.description, +) +def test_given_sink_slow_once_when_call_returns_then_later_and_outcome_events_arrive( + test_case: EventExporterDispatcherTestCase, +) -> None: + event_ids: tuple[str, ...] = tuple(f"event-{index}" for index in range(1, 8)) + started: dict[str, threading.Event] = {event_id: threading.Event() for event_id in event_ids} + gates: dict[str, threading.Event] = {event_id: threading.Event() for event_id in event_ids} + for event_id in event_ids[:1] + event_ids[2:]: + gates[event_id].set() + first_delivered: threading.Event = started["event-1"] + slow_started: threading.Event = started["event-2"] + release: threading.Event = gates["event-2"] + delivered: list[str] = [] + + def record(*, event: LifecycleEvent) -> None: + delivered.append(event.event_id) + started[event.event_id].set() + _ = gates[event.event_id].wait() + + dispatcher: EventExporterDispatcher = EventExporterDispatcher( + exporters=(BoundEventExporter("recorder", record, {}),), + shutdown_timeout_seconds=5.0, + invocation_timeout_seconds=0.1, + ) + dispatcher.enqueue(correlated_event(1, "run_started")) + assert first_delivered.wait(timeout=1.0) + dispatcher.enqueue(correlated_event(2, "statement_completed")) + assert slow_started.wait(timeout=1.0) + time.sleep(0.3) + dispatcher.enqueue(correlated_event(3, "statement_completed")) + dispatcher.enqueue(correlated_event(4, "run_completed")) + time.sleep(0.05) + release.set() + time.sleep(0.1) + dispatcher.enqueue(correlated_event(5, "statement_completed")) + dispatcher.enqueue(correlated_event(6, "statement_completed")) + dispatcher.enqueue(lifecycle_event(7, "invocation_completed")) + + summary: EventExportSummary = dispatcher.shutdown() + + assert delivered == ["event-1", "event-2", "event-4", "event-5", "event-6", "event-7"] + assert summary.delivered == test_case.expected_delivered + assert (summary.failed, summary.dropped) == (1, 1) + assert summary.delivered + summary.failed + summary.dropped == summary.accepted diff --git a/website/src/content/docs/docs/concepts/observability/sinks.mdx b/website/src/content/docs/docs/concepts/observability/sinks.mdx index f58c00658b..198b135abe 100644 --- a/website/src/content/docs/docs/concepts/observability/sinks.mdx +++ b/website/src/content/docs/docs/concepts/observability/sinks.mdx @@ -102,11 +102,16 @@ durability. SQLBuild core does not provide a Kafka or ClickHouse implementation. Run and invocation outcomes (`run_started`, `run_completed`, `run_failed`, `invocation_started`, `invocation_completed`, `invocation_failed`) are always admitted and never displaced. - A failing or timed-out sink is isolated from command correctness. +- Each sink call has one second to return. When a call takes longer, the command prints + `Warning: lifecycle sink '' did not return from one call within 1s; its events are dropped until that call returns.` + That call counts as failed and the sink's events are dropped and counted while it runs. Run and + invocation outcomes are kept instead and delivered, with every later event, once the call returns. + Time SQLBuild itself spends waiting for Python's GIL does not count against the call. - `audit_completed` facts are published as each audit finishes (model audits in `sqb build` when their model finishes), not in one burst at the end of the run. - When lifecycle events are dropped or fail, the command prints one warning to stderr with the counts and affected sinks, for example - `Warning: command completed successfully, but lifecycle event export was incomplete: 140 of 248 events dropped, 0 failed (sink 'orders_events'). Increase sinks.lifecycle.shutdown_timeout or check sink health.` + `Warning: command completed successfully, but lifecycle event export was incomplete: 140 of 248 events dropped, 0 failed (sink 'orders_events'). Check sink health: a sink call slower than 1s drops that sink's events until it returns; events still queued at exit need a longer sinks.lifecycle.shutdown_timeout.` A successful command then ends with `sqb audit completed successfully; only its lifecycle event export is affected.` The exit code and stdout, including `--json` output, are unchanged. From a530f8a91f37a23218e7921aea8f353104a0436d Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 08:48:08 +0100 Subject: [PATCH 04/15] fix!: reject generic audit arguments the audit's SQL never uses An attached generic audit that passes an argument neither its SQL nor its evidence SQL reads, including a misspelt one, now fails the compile with an error naming the audit, the argument to remove and the definition file. Such arguments used to be ignored, so the audit silently checked something other than what the attachment declared. BREAKING CHANGE: projects whose audit attachments pass arguments the audit never reads must remove those arguments. --- .../src/audits/_helpers/rendering.rs | 37 ++++++++- .../src/audits/main/render_attached_audit.rs | 5 +- .../sqlbuild-attachments/src/audits/models.rs | 2 + .../src/audits/tests/helpers.rs | 1 + .../tests/test_render_attached_audit.rs | 22 +++++ .../_helpers/input_loops/audit_loop.rs | 12 ++- .../_helpers/corpus/render_failure_cases.py | 11 +++ .../references/docs/concepts/audits.md | 10 +++ .../cli/commands/main/audit/_test_types.py | 10 +++ .../audit/test_generic_audit_arguments.py | 82 +++++++++++++++++++ .../generic-audit-unused-argument.json | 18 ++++ .../sqlbuild/compiler/model_config/helpers.py | 1 + .../src/content/docs/docs/concepts/audits.mdx | 10 +++ 13 files changed, 218 insertions(+), 3 deletions(-) create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py create mode 100644 tests/goldens/compiler/failure/generic-audit-unused-argument.json diff --git a/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs b/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs index 84c1e1fe2e..c60ebc63a2 100644 --- a/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs +++ b/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs @@ -1,6 +1,8 @@ //! Python's argument merge, SQL rendering and policy resolution for one attachment. -use crate::audits::_helpers::parameters::{RenderStop, render_parameterized_sql}; +use crate::audits::_helpers::parameters::{ + RenderStop, parameter_matches, render_parameterized_sql, +}; use crate::audits::_helpers::policies::{resolve_run_scope, resolve_severity}; use crate::audits::constants::{MODEL_RESOURCE, SEED_RESOURCE, SOURCE_RESOURCE}; use crate::audits::models::{ArgumentValue, AuditAttachment, PolicySource}; @@ -94,3 +96,36 @@ pub(crate) fn merged_arguments( } Ok(merged) } + +/// The error for the first explicit argument neither the audit SQL nor its evidence SQL reads. +pub(crate) fn unused_argument(attachment: &AuditAttachment) -> Option { + let mut used: Vec<&str> = parameter_matches(&attachment.sql_body) + .into_iter() + .map(|item| item.name) + .collect(); + if let Some(evidence) = &attachment.evidence_sql { + used.extend( + parameter_matches(evidence) + .into_iter() + .map(|item| item.name), + ); + } + let implicit: Vec<&str> = attachment + .implicit_arguments + .iter() + .map(|(name, _)| name.as_str()) + .collect(); + let (name, _) = attachment + .explicit_arguments + .iter() + .find(|(name, _)| !used.contains(&name.as_str()) && !implicit.contains(&name.as_str()))?; + let definition: String = attachment + .definition_path + .as_deref() + .map_or_else(|| "its built-in definition".to_owned(), str::to_owned); + Some(format!( + "{} audit '{}' passes argument '{name}', which the audit's SQL in {definition} never \ + uses; remove '{name}' from the attachment", + attachment.owner_label, attachment.definition_name + )) +} diff --git a/crates/sqlbuild-attachments/src/audits/main/render_attached_audit.rs b/crates/sqlbuild-attachments/src/audits/main/render_attached_audit.rs index e26aaed35c..99894aabc3 100644 --- a/crates/sqlbuild-attachments/src/audits/main/render_attached_audit.rs +++ b/crates/sqlbuild-attachments/src/audits/main/render_attached_audit.rs @@ -1,6 +1,6 @@ //! Attached generic audit rendering: argument merge, SQL, severity and run scope. -use crate::audits::_helpers::rendering::{merged_arguments, policies, rendered}; +use crate::audits::_helpers::rendering::{merged_arguments, policies, rendered, unused_argument}; use crate::audits::models::{ArgumentValue, AuditAttachment, AuditRendering, RenderedAudit}; /// Render one attachment, or the error rendering raises. @@ -14,6 +14,9 @@ pub fn render_attached_audit(attachment: &AuditAttachment) -> AuditRendering { .as_deref() .map(|evidence| rendered(attachment, evidence, &arguments)) .transpose()?; + if let Some(message) = unused_argument(attachment) { + return Err(message); + } Ok(RenderedAudit { sql_body, evidence_sql, diff --git a/crates/sqlbuild-attachments/src/audits/models.rs b/crates/sqlbuild-attachments/src/audits/models.rs index 362a576a8a..d9b20242e6 100644 --- a/crates/sqlbuild-attachments/src/audits/models.rs +++ b/crates/sqlbuild-attachments/src/audits/models.rs @@ -28,6 +28,8 @@ pub struct AuditAttachment { /// Python's `str(owner_file)` and the generic audit name, which its error messages quote. pub owner_label: String, pub definition_name: String, + /// The project file defining the audit, or `None` for a built-in audit. + pub definition_path: Option, pub sql_body: String, pub evidence_sql: Option, /// Arguments from the attached resource, such as `model` and `column`; always text. diff --git a/crates/sqlbuild-attachments/src/audits/tests/helpers.rs b/crates/sqlbuild-attachments/src/audits/tests/helpers.rs index 392b4fd6d6..245ce1628d 100644 --- a/crates/sqlbuild-attachments/src/audits/tests/helpers.rs +++ b/crates/sqlbuild-attachments/src/audits/tests/helpers.rs @@ -40,6 +40,7 @@ pub(super) fn attachment( AuditAttachment { owner_label: "models/schema.yml".to_owned(), definition_name: "accepted_values".to_owned(), + definition_path: None, sql_body: "SELECT * FROM t WHERE @column NOT IN (@'values')".to_owned(), evidence_sql: Some("SELECT @column FROM t".to_owned()), implicit_arguments: vec![("column".to_owned(), "status".to_owned())], diff --git a/crates/sqlbuild-attachments/src/audits/tests/test_render_attached_audit.rs b/crates/sqlbuild-attachments/src/audits/tests/test_render_attached_audit.rs index 9d9981a31b..0a5ec0c20f 100644 --- a/crates/sqlbuild-attachments/src/audits/tests/test_render_attached_audit.rs +++ b/crates/sqlbuild-attachments/src/audits/tests/test_render_attached_audit.rs @@ -80,6 +80,28 @@ fn given_attached_audits_when_rendering_then_python_rendering_is_returned() { implicit column from attached context", ), }, + AttachedAuditTestCase { + description: "an argument the audit SQL never reads names itself for removal", + attachment: AuditAttachment { + definition_path: Some("audits/generic/accepted_values.sql".to_owned()), + ..attachment( + vec![ + values.clone(), + ( + "where_condition".to_owned(), + ArgumentValue::Text("status <> 'void'".to_owned()), + ), + ], + None, + None, + ) + }, + expected_rendering: failed( + "models/schema.yml audit 'accepted_values' passes argument 'where_condition', \ + which the audit's SQL in audits/generic/accepted_values.sql never uses; remove \ + 'where_condition' from the attachment", + ), + }, AttachedAuditTestCase { description: "a missing argument is Python's error before the policies", attachment: attachment(Vec::new(), Some("fatal"), None), diff --git a/crates/sqlbuild-python/src/bindings/_helpers/input_loops/audit_loop.rs b/crates/sqlbuild-python/src/bindings/_helpers/input_loops/audit_loop.rs index 29e2899e36..c4e9323075 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/input_loops/audit_loop.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/input_loops/audit_loop.rs @@ -71,6 +71,7 @@ type Definitions<'a, 'py> = (HashMap>, Vec { file: &'a AuditFileRow, block: &'a AuditBlockRow, + builtin: bool, } /// One resource's attached audits: its target, consumer path, and owner files and instances. @@ -151,7 +152,14 @@ fn generic_definitions<'a, 'py>( ); return Err(compile_error(context.classes, &message, None)); } - project.insert(file.stem.clone(), Definition { file, block }); + project.insert( + file.stem.clone(), + Definition { + file, + block, + builtin: false, + }, + ); } let mut shadows: Vec> = Vec::new(); let mut definitions: HashMap> = HashMap::new(); @@ -162,6 +170,7 @@ fn generic_definitions<'a, 'py>( Definition { file: builtin, block, + builtin: true, }, ); } @@ -409,6 +418,7 @@ impl<'py> AuditScope<'_, 'py> { let rendering: AuditRendering = render_attached_audit(&AuditAttachment { owner_label: owner_file.to_owned(), definition_name: definition_name.clone(), + definition_path: (!definition.builtin).then(|| definition.file.relative_path.clone()), sql_body: definition.block.sql_body.clone(), evidence_sql: definition.block.evidence_sql.clone(), implicit_arguments: implicit, diff --git a/scripts/compiler_differential/_helpers/corpus/render_failure_cases.py b/scripts/compiler_differential/_helpers/corpus/render_failure_cases.py index ea8e88f1f6..bef83b3851 100644 --- a/scripts/compiler_differential/_helpers/corpus/render_failure_cases.py +++ b/scripts/compiler_differential/_helpers/corpus/render_failure_cases.py @@ -275,6 +275,17 @@ def _interpolation_cases() -> tuple[FailureCase, ...]: **_staging_header(" audits [floor],"), }, ), + failure_case( + name="generic-audit-unused-argument", + expected_code="P001", + expected_message="passes argument 'where_condition', which the audit's SQL in", + files={ + "models/staging/_sqlbuild/_audits/generic/floor.sql": _FLOOR_AUDIT, + **_staging_header( + " audits [floor (minimum 0, where_condition \"status <> 'void'\")]," + ), + }, + ), ) diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/audits.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/audits.md index be6e5ae7f3..6f64c8f1b7 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/audits.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/audits.md @@ -257,6 +257,16 @@ MODEL ( ); ``` +Every argument an attachment passes must be read by the audit's SQL or evidence SQL as `@name` or +`@'name'`. An argument the audit never reads, including a misspelt one, is a compile error that names +the argument to remove, so a check is never silently broader or narrower than its attachment says: + +```text +models/marts/fct_orders.sql audit 'expression_is_true' passes argument 'where_condition', which the audit's SQL in audits/generic/expression_is_true.sql never uses; remove 'where_condition' from the attachment +``` + +An argument that repeats one the attached resource supplies, such as `column`, is not checked. + ### Audits that read other resources An attached audit can read other models, sources, seeds, and table functions besides its target, diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py index 93eb9a0cac..26471d8025 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py @@ -94,3 +94,13 @@ class InvalidAuditMetaE2ETestCase: relative_path: str contents: str expected_fragments: tuple[str, ...] + + +@dataclass(frozen=True) +class GenericAuditArgumentE2ETestCase: + """One generic audit attachment and how its compile ends.""" + + description: str + arguments: str + expected_exit_code: int + expected_message: str | None diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py new file mode 100644 index 0000000000..d319351b59 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py @@ -0,0 +1,82 @@ +"""CLI e2e coverage: generic audit arguments the audit's SQL never reads fail the compile.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.audit._test_types import ( + GenericAuditArgumentE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb + +_AUDIT_PATH: str = "audits/generic/unique_within_group.sql" +_UNIQUE_WITHIN_GROUP: str = ( + "AUDIT ();\n\n" + "SELECT @group_columns, COUNT(DISTINCT @column) AS distinct_count\n" + "FROM @relation\nWHERE @column IS NOT NULL\nGROUP BY @group_columns\n" + "HAVING COUNT(DISTINCT @column) > 1\n" +) + + +def _orders_model(arguments: str) -> str: + return ( + "MODEL (description 'Test model orders.',\n materialized table,\n columns (\n" + f" status (audits [unique_within_group ({arguments})]),\n ),\n);\n\n" + "SELECT 1 AS order_id, 'open' AS status, 'web' AS channel\n" + ) + + +@pytest.mark.parametrize( + "test_case", + ( + GenericAuditArgumentE2ETestCase( + description="every argument read by the audit compiles", + arguments='group_columns "channel"', + expected_exit_code=0, + expected_message=None, + ), + GenericAuditArgumentE2ETestCase( + description="an argument the audit never reads names itself for removal", + arguments='group_columns "channel", where_condition "channel = \'web\'"', + expected_exit_code=1, + expected_message=( + "models/orders.sql audit 'unique_within_group' passes argument " + "'where_condition', which the audit's SQL in " + f"{_AUDIT_PATH} never uses; remove 'where_condition' from the attachment" + ), + ), + GenericAuditArgumentE2ETestCase( + description="a misspelt argument is reported as unused", + arguments='group_columns "channel", group_column "channel"', + expected_exit_code=1, + expected_message="passes argument 'group_column', which the audit's SQL in", + ), + ), + ids=lambda case: case.description, +) +def test_given_generic_audit_arguments_when_compiling_then_unused_arguments_fail( + test_case: GenericAuditArgumentE2ETestCase, tmp_path: Path +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="audit_arguments", + repo_files={ + "sqlbuild_project.toml": ( + 'name = "audit_arguments"\nadapter = "duckdb"\n\n' + '[connection]\ndatabase = "audit_arguments.duckdb"\n' + ), + _AUDIT_PATH: _UNIQUE_WITHIN_GROUP, + "models/orders.sql": _orders_model(test_case.arguments), + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "compile"), project_dir=project_dir + ) + + assert result.returncode == test_case.expected_exit_code, result.stdout + result.stderr + if test_case.expected_message is not None: + assert test_case.expected_message in result.stdout + result.stderr diff --git a/tests/goldens/compiler/failure/generic-audit-unused-argument.json b/tests/goldens/compiler/failure/generic-audit-unused-argument.json new file mode 100644 index 0000000000..187cc93f97 --- /dev/null +++ b/tests/goldens/compiler/failure/generic-audit-unused-argument.json @@ -0,0 +1,18 @@ +{ + "commands": [ + { + "label": "compile", + "exit_code": 1, + "diagnostics": [ + { + "phase": "compile", + "severity": "error", + "code": "P001", + "message": "models/staging/stg_orders.sql audit 'floor' passes argument 'where_condition', which the audit's SQL in models/staging/_sqlbuild/_audits/generic/floor.sql never uses; remove 'where_condition' from the attachment" + } + ] + } + ], + "compiled": {}, + "manifest": null +} diff --git a/tests/integration/src/sqlbuild/compiler/model_config/helpers.py b/tests/integration/src/sqlbuild/compiler/model_config/helpers.py index 697c4439f6..053c8d57c3 100644 --- a/tests/integration/src/sqlbuild/compiler/model_config/helpers.py +++ b/tests/integration/src/sqlbuild/compiler/model_config/helpers.py @@ -145,6 +145,7 @@ LATER_VALIDATION_PATTERNS: tuple[re.Pattern[str], ...] = ( re.compile(r"is missing argument '[^']+' for generic audit '[^']+'$"), + re.compile(r"passes argument '[^']+', which the audit's SQL in .+ never uses; remove .+$"), re.compile(r"column '[^']+' migrate_from must name one column of the model's existing table$"), re.compile( r"audit '[^']+': thresholds and minimum_samples are only valid for measurement audits$" diff --git a/website/src/content/docs/docs/concepts/audits.mdx b/website/src/content/docs/docs/concepts/audits.mdx index aed697b813..3e0f006fc6 100644 --- a/website/src/content/docs/docs/concepts/audits.mdx +++ b/website/src/content/docs/docs/concepts/audits.mdx @@ -238,6 +238,16 @@ MODEL ( ); ``` +Every argument an attachment passes must be read by the audit's SQL or evidence SQL as `@name` or +`@'name'`. An argument the audit never reads, including a misspelt one, is a compile error that names +the argument to remove, so a check is never silently broader or narrower than its attachment says: + +```text +models/marts/fct_orders.sql audit 'expression_is_true' passes argument 'where_condition', which the audit's SQL in audits/generic/expression_is_true.sql never uses; remove 'where_condition' from the attachment +``` + +An argument that repeats one the attached resource supplies, such as `column`, is not checked. + ### Audits that read other resources An attached audit can read other models, sources, seeds, and table functions besides its target, From 6e4d68e3a05b330eec47a0c88ba6dd1e1342f981 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 08:51:06 +0100 Subject: [PATCH 05/15] docs: document the rendered audit SQL compile writes under target/compiled/audits sqb compile already writes each attached generic audit's rendered SQL to target/compiled/audits/generic// and removes it with the attachment, but nothing said so. Document the compiled artifact layout and cover the audit files, and their cleanup, with an end-to-end test. --- .../sqlbuild/references/docs/cli/compile.md | 19 ++++ .../cli/commands/main/audit/_test_types.py | 16 ++- .../cli/commands/main/audit/helpers.py | 25 +++++ .../audit/test_generic_audit_arguments.py | 104 +++++++++++------- website/src/content/docs/docs/cli/compile.mdx | 18 +++ 5 files changed, 141 insertions(+), 41 deletions(-) diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md index bfffb7f32d..abf369a31b 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md @@ -11,6 +11,7 @@ Online: https://sqlbuild.com/docs/cli/compile/ - Usage - Flags - What compile does +- Compiled artifacts - Focused compilation - Reusing an unchanged compile - Static analysis @@ -55,6 +56,24 @@ Positional `SELECTOR` arguments work like `--select`, and both forms can be comb and compile prints one note naming them 7. **Artifact write** - writes compiled SQL to `target/compiled/` when Rules pass +## Compiled artifacts + +`target/compiled/` mirrors what each resource will run, with every macro, variable and audit +argument already rendered: + +| Path | Contents | +|------|----------| +| `models/.sql` | Each model's query | +| `functions//.sql` | Each function's `CREATE FUNCTION` statements | +| `audits/generic//__.sql` | Each attached generic audit, under the model, source or seed it checks; column audits end in `__`, and an audit's `name` replaces the audit name | +| `audits/singular/.sql` | Each singular audit | +| `tests/...` | Each SQL unit test's comparison query | + +Read an audit's file to see how its arguments were spliced into the SQL without running it or +looking up the warehouse's query history. Files are rewritten on every compile, and files for +resources that no longer exist are removed. References such as `__ref("orders")` stay as written, +as in compiled models. + ## Focused compilation Use normal selectors to limit expensive analysis and reporting while retaining complete project diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py index 26471d8025..a8f887adbe 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/_test_types.py @@ -98,9 +98,19 @@ class InvalidAuditMetaE2ETestCase: @dataclass(frozen=True) class GenericAuditArgumentE2ETestCase: - """One generic audit attachment and how its compile ends.""" + """One generic audit attachment with an unused argument and the error it must report.""" description: str arguments: str - expected_exit_code: int - expected_message: str | None + expected_message: str + + +@dataclass(frozen=True) +class RenderedGenericAuditE2ETestCase: + """One generic audit attachment, its compiled SQL file and the SQL it must render to.""" + + description: str + arguments: str + detached_column: str + expected_relative_path: str + expected_sql: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py index b01d6ea289..fd8e1e11d5 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/helpers.py @@ -311,3 +311,28 @@ def plan_reasons(project_dir: Path) -> dict[str, str]: return { str(model["name"]): str(model["reason"]) for model in json.loads(result.stdout)["models"] } + + +GENERIC_AUDIT_PATH: str = "audits/generic/unique_within_group.sql" + + +def generic_audit_project_files(*, arguments: str) -> dict[str, str]: + """Return a project whose orders model attaches unique_within_group with ``arguments``.""" + + return { + "sqlbuild_project.toml": ( + 'name = "audit_arguments"\nadapter = "duckdb"\n\n' + '[connection]\ndatabase = "audit_arguments.duckdb"\n' + ), + GENERIC_AUDIT_PATH: ( + "AUDIT ();\n\n" + "SELECT @group_columns, COUNT(DISTINCT @column) AS distinct_count\n" + "FROM @relation\nWHERE @column IS NOT NULL\nGROUP BY @group_columns\n" + "HAVING COUNT(DISTINCT @column) > 1\n" + ), + "models/orders.sql": ( + "MODEL (description 'Test model orders.',\n materialized table,\n columns (\n" + f" status (audits [unique_within_group ({arguments})]),\n ),\n);\n\n" + "SELECT 1 AS order_id, 'open' AS status, 'web' AS channel\n" + ), + } diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py b/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py index d319351b59..91a4bb02e1 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/audit/test_generic_audit_arguments.py @@ -1,4 +1,4 @@ -"""CLI e2e coverage: generic audit arguments the audit's SQL never reads fail the compile.""" +"""CLI e2e coverage: generic audit arguments are checked and their rendered SQL is compiled.""" from __future__ import annotations @@ -9,74 +9,102 @@ from tests.e2e.src.sqlbuild.cli.commands.main.audit._test_types import ( GenericAuditArgumentE2ETestCase, + RenderedGenericAuditE2ETestCase, ) -from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb - -_AUDIT_PATH: str = "audits/generic/unique_within_group.sql" -_UNIQUE_WITHIN_GROUP: str = ( - "AUDIT ();\n\n" - "SELECT @group_columns, COUNT(DISTINCT @column) AS distinct_count\n" - "FROM @relation\nWHERE @column IS NOT NULL\nGROUP BY @group_columns\n" - "HAVING COUNT(DISTINCT @column) > 1\n" +from tests.e2e.src.sqlbuild.cli.commands.main.audit.helpers import ( + GENERIC_AUDIT_PATH, + generic_audit_project_files, ) - - -def _orders_model(arguments: str) -> str: - return ( - "MODEL (description 'Test model orders.',\n materialized table,\n columns (\n" - f" status (audits [unique_within_group ({arguments})]),\n ),\n);\n\n" - "SELECT 1 AS order_id, 'open' AS status, 'web' AS channel\n" - ) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb @pytest.mark.parametrize( "test_case", ( - GenericAuditArgumentE2ETestCase( - description="every argument read by the audit compiles", - arguments='group_columns "channel"', - expected_exit_code=0, - expected_message=None, - ), GenericAuditArgumentE2ETestCase( description="an argument the audit never reads names itself for removal", arguments='group_columns "channel", where_condition "channel = \'web\'"', - expected_exit_code=1, expected_message=( "models/orders.sql audit 'unique_within_group' passes argument " "'where_condition', which the audit's SQL in " - f"{_AUDIT_PATH} never uses; remove 'where_condition' from the attachment" + f"{GENERIC_AUDIT_PATH} never uses; remove 'where_condition' from the attachment" ), ), GenericAuditArgumentE2ETestCase( description="a misspelt argument is reported as unused", arguments='group_columns "channel", group_column "channel"', - expected_exit_code=1, expected_message="passes argument 'group_column', which the audit's SQL in", ), ), ids=lambda case: case.description, ) -def test_given_generic_audit_arguments_when_compiling_then_unused_arguments_fail( +def test_given_unused_generic_audit_argument_when_compiling_then_compile_fails( test_case: GenericAuditArgumentE2ETestCase, tmp_path: Path ) -> None: project_dir: Path = prepare_inline_project( tmp_path=tmp_path, project_name="audit_arguments", - repo_files={ - "sqlbuild_project.toml": ( - 'name = "audit_arguments"\nadapter = "duckdb"\n\n' - '[connection]\ndatabase = "audit_arguments.duckdb"\n' - ), - _AUDIT_PATH: _UNIQUE_WITHIN_GROUP, - "models/orders.sql": _orders_model(test_case.arguments), - }, + repo_files=generic_audit_project_files(arguments=test_case.arguments), ) result: subprocess.CompletedProcess[str] = run_sqb( command=("--no-color", "compile"), project_dir=project_dir ) - assert result.returncode == test_case.expected_exit_code, result.stdout + result.stderr - if test_case.expected_message is not None: - assert test_case.expected_message in result.stdout + result.stderr + assert result.returncode == 1, result.stdout + result.stderr + assert test_case.expected_message in result.stdout + result.stderr + + +@pytest.mark.parametrize( + "test_case", + ( + RenderedGenericAuditE2ETestCase( + description="every argument read by the audit compiles and renders", + arguments='group_columns "channel"', + detached_column=" (type VARCHAR)", + expected_relative_path=( + "target/compiled/audits/generic/orders/unique_within_group__status.sql" + ), + expected_sql=( + "SELECT channel, COUNT(DISTINCT status) AS distinct_count\n" + 'FROM __ref("orders")\nWHERE status IS NOT NULL\nGROUP BY channel\n' + "HAVING COUNT(DISTINCT status) > 1\n" + ), + ), + ), + ids=lambda case: case.description, +) +def test_given_attached_generic_audit_when_compiling_then_rendered_sql_is_written_and_cleaned( + test_case: RenderedGenericAuditE2ETestCase, tmp_path: Path +) -> None: + """Compile writes each audit's rendered SQL beside the models and removes it once detached.""" + + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="audit_arguments", + repo_files=generic_audit_project_files(arguments=test_case.arguments), + ) + audit_file: Path = project_dir / test_case.expected_relative_path + + first: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "compile"), project_dir=project_dir + ) + rendered: str = audit_file.read_text(encoding="utf-8") + model_path: Path = project_dir / "models/orders.sql" + _ = model_path.write_text( + model_path.read_text(encoding="utf-8").replace( + f" (audits [unique_within_group ({test_case.arguments})])", + test_case.detached_column, + ), + encoding="utf-8", + ) + (project_dir / GENERIC_AUDIT_PATH).unlink() + second: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "compile"), project_dir=project_dir + ) + + assert first.returncode == 0, first.stdout + first.stderr + assert rendered == test_case.expected_sql + assert second.returncode == 0, second.stdout + second.stderr + assert not audit_file.exists() + assert (project_dir / "target/compiled/models/orders.sql").is_file() diff --git a/website/src/content/docs/docs/cli/compile.mdx b/website/src/content/docs/docs/cli/compile.mdx index 0ded0472b2..bff480f577 100644 --- a/website/src/content/docs/docs/cli/compile.mdx +++ b/website/src/content/docs/docs/cli/compile.mdx @@ -44,6 +44,24 @@ Positional `SELECTOR` arguments work like `--select`, and both forms can be comb and compile prints one note naming them 7. **Artifact write** - writes compiled SQL to `target/compiled/` when Rules pass +## Compiled artifacts + +`target/compiled/` mirrors what each resource will run, with every macro, variable and audit +argument already rendered: + +| Path | Contents | +|------|----------| +| `models/.sql` | Each model's query | +| `functions//.sql` | Each function's `CREATE FUNCTION` statements | +| `audits/generic//__.sql` | Each attached generic audit, under the model, source or seed it checks; column audits end in `__`, and an audit's `name` replaces the audit name | +| `audits/singular/.sql` | Each singular audit | +| `tests/...` | Each SQL unit test's comparison query | + +Read an audit's file to see how its arguments were spliced into the SQL without running it or +looking up the warehouse's query history. Files are rewritten on every compile, and files for +resources that no longer exist are removed. References such as `__ref("orders")` stay as written, +as in compiled models. + ## Focused compilation Use normal selectors to limit expensive analysis and reporting while retaining complete project From fc38785faadfc058fafb6ce3e44c0419a8529205 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:19:43 +0100 Subject: [PATCH 06/15] fix: print expected CLI errors whose text holds undecodable bytes A selector with undecodable bytes made sqb compile crash with a Unicode error instead of reporting S007, in text mode and with --json. Expected error text now keeps lone surrogates as \xNN or \uXXXX escapes before it is printed or put in the JSON report. --- .../src/bindings/_helpers/cli/entry.rs | 21 +++--- .../cli/commands/main/compile/_test_types.py | 20 ++++++ .../main/compile/test_undecodable_selector.py | 66 +++++++++++++++++++ 3 files changed, 99 insertions(+), 8 deletions(-) create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/compile/test_undecodable_selector.py diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs index 61fb26ea65..f8cf896796 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs @@ -28,6 +28,7 @@ use crate::bindings::_helpers::diff_report::output::render_diff_error_json; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::output_capture::records::invocation_context; use crate::bindings::_helpers::presentation_text::terminal::stdout_supports_color; +use crate::bindings::_helpers::recall::events::escaped_text; use crate::bindings::_helpers::recall::session::{ InvocationRecall, finish_invocation_recall, start_invocation_recall, }; @@ -113,23 +114,27 @@ fn print_object(value: &Bound<'_, PyAny>, stream: Option<&str>, end: Option<&str Ok(()) } -/// `(code, message, help)` of an expected error as Python objects, as the CLI renders it. +/// `(code, message, help)` of an expected error as printable strings, lone surrogates escaped. fn error_parts<'py>( error: &Bound<'py, PyAny>, fallback_code: &str, ) -> PyResult<(Bound<'py, PyAny>, Bound<'py, PyAny>, Bound<'py, PyAny>)> { let py: Python<'py> = error.py(); + let printable = |text: Bound<'py, PyString>| -> PyResult> { + Ok(PyString::new(py, &escaped_text(&text)?).into_any()) + }; let code: Bound<'py, PyAny> = match error.getattr_opt("code")? { - Some(code) => code.str()?.into_any(), + Some(code) => printable(code.str()?)?, None => PyString::new(py, fallback_code).into_any(), }; let message: Bound<'py, PyAny> = match error.getattr_opt("message")? { - Some(message) => message.str()?.into_any(), - None => error.str()?.into_any(), + Some(message) => printable(message.str()?)?, + None => printable(error.str()?)?, + }; + let help: Bound<'py, PyAny> = match error.getattr_opt("help")? { + Some(help) if !help.is_none() => printable(help.str()?)?, + _ => py.None().into_bound(py), }; - let help: Bound<'py, PyAny> = error - .getattr_opt("help")? - .unwrap_or_else(|| py.None().into_bound(py)); Ok((code, message, help)) } @@ -144,7 +149,7 @@ fn print_expected_error( let help: Option = if help.is_none() { None } else { - Some(help.str()?.extract()?) + Some(help.extract()?) }; let text: String = format_coded_error( ( diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py index ebd260fa5e..3d196f0420 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py @@ -1199,3 +1199,23 @@ class DialectLineCommentTestCase: project_toml: str query_sql: str expected_column_count: int + + +@dataclass(frozen=True) +class MissingTargetDatabaseTestCase: + """A Snowflake project without a target database and the S101 error it must print.""" + + description: str + project_toml: str + expected_error: str + expected_example_count: int + + +@dataclass(frozen=True) +class UndecodableSelectorTestCase: + """An output mode for a selector holding undecodable bytes and the error it must print.""" + + description: str + output_flags: tuple[str, ...] + expected_code: str + expected_escaped_selector: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_undecodable_selector.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_undecodable_selector.py new file mode 100644 index 0000000000..b874832853 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_undecodable_selector.py @@ -0,0 +1,66 @@ +"""CLI e2e coverage: a selector with undecodable bytes reports S007 in text and JSON mode.""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.compile._test_types import ( + UndecodableSelectorTestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb + +_UNDECODABLE_SELECTOR: str = os.fsdecode(b"orders\xff") + + +@pytest.mark.parametrize( + "test_case", + ( + UndecodableSelectorTestCase( + description="text output", + output_flags=(), + expected_code="S007", + expected_escaped_selector="orders\\xff", + ), + UndecodableSelectorTestCase( + description="json output", + output_flags=("--json",), + expected_code="S007", + expected_escaped_selector="orders\\xff", + ), + ), + ids=lambda case: case.description, +) +def test_given_undecodable_selector_when_compiling_then_unknown_selector_error_is_printed( + test_case: UndecodableSelectorTestCase, tmp_path: Path +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="selectors", + repo_files={ + "sqlbuild_project.toml": 'name = "selectors"\nadapter = "duckdb"\n', + "models/orders.sql": ( + "MODEL (description 'Test model orders.', materialized view);\n\n" + "SELECT 1 AS order_id\n" + ), + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=( + "--no-color", + "compile", + *test_case.output_flags, + "--select", + _UNDECODABLE_SELECTOR, + ), + project_dir=project_dir, + ) + + assert result.returncode == 1, result.stdout + result.stderr + assert "UnicodeEncodeError" not in result.stderr + assert test_case.expected_code in result.stdout + result.stderr + assert test_case.expected_escaped_selector in result.stdout + result.stderr From 80f52f4afc3904564a889bed06037c5e30471c3c Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:22:19 +0100 Subject: [PATCH 07/15] fix(rules): stop treating # as a Snowflake line comment The Rules scanner skipped Snowflake code after #, which Snowflake does not treat as a comment (only -- and //), so dependency calls there were missed. The scanner now matches the Snowflake tokenizer. --- .../src/engine/tests/test_query_facts.rs | 9 ------- .../src/engine/tests/test_sql_scanning.rs | 27 ++++++++++++++++++- .../src/engine/tests/test_types.rs | 6 +++++ .../src/query_facts/_helpers/shape.rs | 2 +- 4 files changed, 33 insertions(+), 11 deletions(-) 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 e7d29e80f8..cf0c987e90 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs @@ -31,15 +31,6 @@ fn given_dialect_sql_when_reading_query_facts_then_findings_match_the_authored_s columns: json!([]), expected_faults: &[], }, - QueryFactsTestCase { - description: "an apostrophe in a Snowflake # comment opens no string", - dialect: "snowflake", - 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: &[], - }, QueryFactsTestCase { description: "an apostrophe in a BigQuery # comment opens no string", dialect: "bigquery", diff --git a/crates/sqlbuild-rules/src/engine/tests/test_sql_scanning.rs b/crates/sqlbuild-rules/src/engine/tests/test_sql_scanning.rs index b39a5d3784..90f232919f 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_sql_scanning.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_sql_scanning.rs @@ -1,5 +1,5 @@ use crate::engine::tests::test_types::{ - DollarQuoteSiteTestCase, LintDialectSiteTestCase, SqlScannerTestCase, + DialectCallsTestCase, DollarQuoteSiteTestCase, LintDialectSiteTestCase, SqlScannerTestCase, }; use crate::query_facts::main::model_shape::model_shape; use crate::rules::_helpers::numeric_decisions::compact_sql; @@ -323,3 +323,28 @@ fn given_dollar_quoted_apostrophe_when_preparing_lint_sites_then_later_reference } Ok(()) } + +#[test] +fn given_hash_before_reference_when_shaping_then_only_hash_comment_dialects_skip_it() +-> Result<(), String> { + let test_cases = [ + DialectCallsTestCase { + description: "bigquery treats # as a line comment", + dialect: "bigquery", + expected_calls: &["__ref(\"o\")"], + }, + DialectCallsTestCase { + description: "snowflake reads code after #", + dialect: "snowflake", + expected_calls: &["__ref(\"o\")", "__ref(\"p\")"], + }, + ]; + let sql = "SELECT o.order_id FROM __ref(\"o\") AS o\nWHERE o.order_id > 0 # AND o.order_id IN (SELECT order_id FROM __ref(\"p\"))\n"; + for test_case in test_cases { + let calls: Vec = model_shape(sql, test_case.dialect) + .map(|shape| shape.calls.into_iter().map(|call| call.text).collect()) + .unwrap_or_default(); + assert_eq!(calls, test_case.expected_calls, "{}", test_case.description); + } + Ok(()) +} diff --git a/crates/sqlbuild-rules/src/engine/tests/test_types.rs b/crates/sqlbuild-rules/src/engine/tests/test_types.rs index e631ce4e0c..c18a15c5ad 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_types.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_types.rs @@ -318,3 +318,9 @@ pub(crate) struct StaleRelationExceptionTestCase { pub(crate) exceptions: Value, pub(crate) expected_error: &'static str, } + +pub(crate) struct DialectCallsTestCase { + pub(crate) description: &'static str, + pub(crate) dialect: &'static str, + pub(crate) expected_calls: &'static [&'static str], +} diff --git a/crates/sqlbuild-rules/src/query_facts/_helpers/shape.rs b/crates/sqlbuild-rules/src/query_facts/_helpers/shape.rs index 066543e71f..d000202686 100644 --- a/crates/sqlbuild-rules/src/query_facts/_helpers/shape.rs +++ b/crates/sqlbuild-rules/src/query_facts/_helpers/shape.rs @@ -91,7 +91,7 @@ fn lexical_syntax(dialect: &str) -> LexicalSyntax { }, "snowflake" => LexicalSyntax { backslash_escape_quotes: strings(&["'"]), - line_comment_prefixes: lines(&["--", "//", "#"]), + line_comment_prefixes: lines(&["--", "//"]), ..LexicalSyntax::default() }, "clickhouse" => LexicalSyntax { From 9afa54741d4c0e5267933ca957000a71fbe397f8 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:49:56 +0100 Subject: [PATCH 08/15] fix(rules): report SELECT * in plain set-operation branches SQBRMODEL102 only inspected a query's body when it was a single SELECT, so a star in any branch of UNION, INTERSECT or EXCEPT went unreported. The rule now checks every plain SELECT branch of each query body. --- .../src/engine/tests/test_query_facts.rs | 12 ++++++ .../sqlbuild-rules/src/query_facts/models.rs | 17 ++++++++ .../src/rules/_helpers/evaluation.rs | 13 ++++-- .../sqlbuild/cli/commands/main/_test_types.py | 10 +++++ .../sqlbuild/cli/commands/main/test_rules.py | 43 +++++++++++++++++++ 5 files changed, 91 insertions(+), 4 deletions(-) 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 cf0c987e90..b7b3673829 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs @@ -76,6 +76,18 @@ fn given_dialect_sql_when_reading_query_facts_then_findings_match_the_authored_s columns: json!([]), expected_faults: &[("SQBRMODEL102", 2, 8, "SELECT * is allowed only")], }, + QueryFactsTestCase { + description: "stars in plain set-operation branches are each reported", + dialect: "duckdb", + query_sql: "WITH imported AS (SELECT * FROM __ref(\"orders\"))\nSELECT * FROM (SELECT order_id FROM imported)\nUNION ALL\nSELECT * FROM (SELECT order_id FROM imported) AS s", + select: &["SQBRMODEL102"], + enum_columns: &[], + columns: json!([]), + expected_faults: &[ + ("SQBRMODEL102", 2, 8, "SELECT * is allowed only"), + ("SQBRMODEL102", 4, 8, "SELECT * is allowed only"), + ], + }, QueryFactsTestCase { description: "a comparison of two literals keeps its source position", dialect: "duckdb", diff --git a/crates/sqlbuild-rules/src/query_facts/models.rs b/crates/sqlbuild-rules/src/query_facts/models.rs index 2deb0ea0a7..8f77a5c64b 100644 --- a/crates/sqlbuild-rules/src/query_facts/models.rs +++ b/crates/sqlbuild-rules/src/query_facts/models.rs @@ -162,6 +162,23 @@ impl QueryFacts { values } + /// The plain SELECTs of this query's body: the body itself or every branch of its set operation. + pub(crate) fn body_selects(&self) -> Vec<&SelectFacts> { + fn collect<'a>(body: &'a SetFacts, values: &mut Vec<&'a SelectFacts>) { + match body { + SetFacts::Select(select) => values.push(select), + SetFacts::SetOperation { left, right, .. } => { + collect(left, values); + collect(right, values); + } + SetFacts::Query(_) | SetFacts::Other => {} + } + } + let mut values: Vec<&SelectFacts> = Vec::new(); + collect(&self.body, &mut values); + values + } + pub(crate) fn root_select(&self) -> Option<&SelectFacts> { match &self.body { SetFacts::Select(select) => Some(select), diff --git a/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs b/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs index d44680e3cf..8dc1d6c4e4 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs @@ -502,9 +502,6 @@ fn select_star( } } for query in parsed.facts.all_queries() { - let Some(select) = query.root_select() else { - continue; - }; if import_queries .iter() .any(|imported| std::ptr::eq(*imported, query)) @@ -513,7 +510,15 @@ fn select_star( } let dynamic_output_star = dynamic_contracts::allows_output_star(query, parsed.facts, parsed.model); - for star in stars_in_select(select) { + let mut stars: Vec<(&SelectFacts, Option)> = Vec::new(); + for select in query.body_selects() { + stars.extend( + stars_in_select(select) + .into_iter() + .map(|star| (select, star)), + ); + } + for (select, star) in stars { if dynamic_output_star { continue; } diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 1a30877064..29c810f9ed 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -1266,3 +1266,13 @@ class FormatWriteFailureTestCase: expected_replaced: tuple[bool, ...] expected_error: type[BaseException] expected_status: str + + +@dataclass(frozen=True) +class SetOperationStarRuleTestCase: + """A model whose set-operation branches select a star and the findings they must raise.""" + + description: str + model_sql: str + expected_codes: tuple[str, ...] + expected_distinct_lines: int 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 e44a6298cc..b0f81453b2 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -37,6 +37,7 @@ NumericRangeDecisionIntegrationTestCase, RulePassIntegrationTestCase, RulesIntegrationTestCase, + SetOperationStarRuleTestCase, SqlRuleCacheSequenceTestCase, TypedContractRuleIntegrationTestCase, ) @@ -3231,3 +3232,45 @@ def test_given_custom_rule_outcome_when_compiling_then_result_and_cleanup_are_re assert exit_code == test_case.expected_exit_code assert tuple(item["message"] for item in output["diagnostics"]) == test_case.expected_messages assert not tuple((tmp_path / "target" / "rules-cache" / "host-inputs").iterdir()) + + +@pytest.mark.parametrize( + "test_case", + [ + SetOperationStarRuleTestCase( + description="unaliased and aliased derived-table branches", + model_sql=( + "MODEL (description 'Test model order_ids.', materialized view);\n" + 'SELECT * FROM (SELECT order_id FROM __ref("orders"))\n' + "UNION ALL\n" + 'SELECT * FROM (SELECT order_id FROM __ref("orders")) AS s\n' + ), + expected_codes=("SQBRMODEL102", "SQBRMODEL102"), + expected_distinct_lines=2, + ), + ], + ids=lambda case: case.description, +) +def test_given_star_in_set_operation_branches_when_compiling_then_each_branch_is_reported( + test_case: SetOperationStarRuleTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n\n[rules]\nselect = ["SQBRMODEL102"]\n', + encoding="utf-8", + ) + (tmp_path / "models").mkdir() + (tmp_path / "models" / "orders.sql").write_text( + "MODEL (description 'Test model orders.', materialized view);\nSELECT 1 AS order_id\n", + encoding="utf-8", + ) + (tmp_path / "models" / "order_ids.sql").write_text(test_case.model_sql, encoding="utf-8") + + exit_code: int = main(["--project-dir", str(tmp_path), "compile", "--json", "--no-cache"]) + result: dict[str, object] = json.loads(capsys.readouterr().out) + diagnostics: list[dict[str, object]] = cast(list[dict[str, object]], result["diagnostics"]) + + assert exit_code == 1 + assert tuple(item["code"] for item in diagnostics) == test_case.expected_codes + assert len({item["line"] for item in diagnostics}) == test_case.expected_distinct_lines From d515b55321972b4a4f4b67a377d95b08061b215f Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:51:32 +0100 Subject: [PATCH 09/15] test: pin the S101 missing-database message to one settings example --- .../compile/test_missing_target_database.py | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/compile/test_missing_target_database.py diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_missing_target_database.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_missing_target_database.py new file mode 100644 index 0000000000..025221c974 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_missing_target_database.py @@ -0,0 +1,58 @@ +"""CLI e2e coverage: the S101 missing-database error and its single settings example.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.compile._test_types import ( + MissingTargetDatabaseTestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb + + +@pytest.mark.parametrize( + "test_case", + ( + MissingTargetDatabaseTestCase( + description="project defaults without a database", + project_toml=( + 'name = "orders"\nadapter = "snowflake"\n\n[defaults]\nschema = "analytics"\n' + ), + expected_error=( + "error[S101]: snowflake execution requires explicit target database. model " + "'orders' resolved to database=None schema='analytics'. Set them in the model " + "config, the environment config, or the project defaults.\n" + " = help: to set project-wide defaults, set this in sqlbuild_project.toml:\n" + " [defaults]\n" + ' database = "my_database"\n' + ), + expected_example_count=1, + ), + ), + ids=lambda case: case.description, +) +def test_given_snowflake_model_without_database_when_compiling_then_s101_shows_one_example( + test_case: MissingTargetDatabaseTestCase, tmp_path: Path +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders", + repo_files={ + "sqlbuild_project.toml": test_case.project_toml, + "models/orders.sql": ( + "MODEL (description 'Test model orders.', materialized view);\n\n" + "SELECT 1 AS order_id\n" + ), + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "compile"), project_dir=project_dir + ) + + assert result.returncode == 1, result.stdout + result.stderr + assert test_case.expected_error in result.stderr + assert result.stderr.count("[defaults]") == test_case.expected_example_count From 11d0814627ff2b5cbeb42b7f4bfc55e8be8e605d Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 11:19:21 +0100 Subject: [PATCH 10/15] fix: keep an audit's other options in the 64-bit option help The help for an oversized minimum_samples or evidence_limit showed the audit call with only the offending option, so copying it dropped the audit's name, severity and other options. It now repeats the whole call with only the offending value replaced. --- .../src/header_metadata/_helpers/audits.rs | 4 +- .../src/header_metadata/_helpers/text.rs | 57 ++++++++++++++++--- .../_helpers/corpus/config_failure_cases.py | 10 +++- ...-audit-minimum-samples-beyond-64-bits.json | 2 +- 4 files changed, 61 insertions(+), 12 deletions(-) diff --git a/crates/sqlbuild-model-config/src/header_metadata/_helpers/audits.rs b/crates/sqlbuild-model-config/src/header_metadata/_helpers/audits.rs index b89eb3456f..d8adeedd1f 100644 --- a/crates/sqlbuild-model-config/src/header_metadata/_helpers/audits.rs +++ b/crates/sqlbuild-model-config/src/header_metadata/_helpers/audits.rs @@ -90,12 +90,12 @@ fn configured_audit( let minimum_samples = optional_count( entry(options, "minimum_samples"), option_site, - (definition, "minimum_samples"), + (definition, "minimum_samples", options), )?; let evidence_limit = optional_count( entry(options, "evidence_limit"), option_site, - (definition, "evidence_limit"), + (definition, "evidence_limit", options), )?; let meta = optional_meta(entry(options, "meta"), option_site)?; let arguments: Vec<(N, N)> = options diff --git a/crates/sqlbuild-model-config/src/header_metadata/_helpers/text.rs b/crates/sqlbuild-model-config/src/header_metadata/_helpers/text.rs index 17528ee610..668c13397d 100644 --- a/crates/sqlbuild-model-config/src/header_metadata/_helpers/text.rs +++ b/crates/sqlbuild-model-config/src/header_metadata/_helpers/text.rs @@ -57,13 +57,13 @@ pub(crate) fn optional_bool( } } -/// Read a non-negative non-boolean integer option of audit `definition` that fits in 64 bits. +/// Read a non-negative integer option of audit `definition` that fits in 64 bits; help repeats `options`. pub(crate) fn optional_count( node: Option<&N>, site: Site<'_>, - option: (&str, &str), + option: (&str, &str, &[(N, N)]), ) -> Result, ConfigError> { - let (definition, key) = option; + let (definition, key, options) = option; match ( node.map(AuthoredNode::kind), node.and_then(AuthoredNode::integer), @@ -75,14 +75,29 @@ pub(crate) fn optional_count( "'{key}' {} is larger than a 64-bit integer", node.map(AuthoredNode::python_str).unwrap_or_default() )) - .with_help(count_help(site, definition, key))), + .with_help(count_help(site, (definition, options), key))), (Some(_), _) => Err(site.error(&format!("'{key}' must be a non-negative integer"))), } } -/// The MODEL header help that sets audit option `key` to the largest 64-bit integer. -fn count_help(site: Site<'_>, definition: &str, key: &str) -> String { - let audits: String = format!("audits [{definition} ({key} {})]", i64::MAX); +/// The MODEL header help repeating the audit call with option `key` set to the largest 64-bit integer. +fn count_help( + site: Site<'_>, + (definition, options): (&str, &[(N, N)]), + key: &str, +) -> String { + let rendered: Vec = options + .iter() + .map(|(name, value)| { + let name: String = name.text().unwrap_or_else(|| name.python_str()); + if name == key { + format!("{name} {}", i64::MAX) + } else { + format!("{name} {}", header_value(value)) + } + }) + .collect(); + let audits: String = format!("audits [{definition} ({})]", rendered.join(", ")); let entry: String = site.column.map_or_else( || audits.clone(), |column| format!("columns ({column} ({audits}))"), @@ -94,6 +109,34 @@ fn count_help(site: Site<'_>, definition: &str, key: &str) -> String { ) } +/// One authored value as MODEL header syntax: quoted strings, `[...]` lists and `(key value)` maps. +fn header_value(node: &N) -> String { + match node.kind() { + NodeKind::Null => "null".to_owned(), + NodeKind::Bool(value) => value.to_string(), + NodeKind::Str => { + let text: String = node.text().unwrap_or_default(); + format!("\"{}\"", text.replace('\\', "\\\\").replace('"', "\\\"")) + } + NodeKind::List | NodeKind::Tuple => { + let items: Vec = node.items().iter().map(header_value).collect(); + format!("[{}]", items.join(", ")) + } + NodeKind::Map => { + let entries: Vec = node + .entries() + .iter() + .map(|(key, value)| { + let key: String = key.text().unwrap_or_else(|| key.python_str()); + format!("{key} {}", header_value(value)) + }) + .collect(); + format!("({})", entries.join(", ")) + } + NodeKind::Int { .. } | NodeKind::Other => node.python_str(), + } +} + /// Return the mapping entry whose key is the string `key`. pub(crate) fn entry<'entries, N: AuthoredNode>( entries: &'entries [(N, N)], diff --git a/scripts/compiler_differential/_helpers/corpus/config_failure_cases.py b/scripts/compiler_differential/_helpers/corpus/config_failure_cases.py index b04d0252f0..028f67c483 100644 --- a/scripts/compiler_differential/_helpers/corpus/config_failure_cases.py +++ b/scripts/compiler_differential/_helpers/corpus/config_failure_cases.py @@ -373,9 +373,15 @@ def _integer_range_cases() -> tuple[FailureCase, ...]: ), expected_help=_header_help( purpose="set minimum_samples to a value that fits in 64 bits", - entry=f"audits [not_null (minimum_samples {_INT64_MAX})]", + entry=( + 'audits [not_null (name "order_ids_present", ' + f'minimum_samples {_INT64_MAX}, severity "warn", meta (tiers ["gold"]))]' + ), + ), + files=_staging_header( + ' audits [not_null (name "order_ids_present", ' + f"minimum_samples {_BEYOND_64_BITS}, severity warn, meta (tiers [gold]))]," ), - files=_staging_header(f" audits [not_null (minimum_samples {_BEYOND_64_BITS})],"), ), failure_case( name="header-column-audit-evidence-limit-beyond-64-bits", diff --git a/tests/goldens/compiler/failure/header-audit-minimum-samples-beyond-64-bits.json b/tests/goldens/compiler/failure/header-audit-minimum-samples-beyond-64-bits.json index 513f3f6137..303aac38a0 100644 --- a/tests/goldens/compiler/failure/header-audit-minimum-samples-beyond-64-bits.json +++ b/tests/goldens/compiler/failure/header-audit-minimum-samples-beyond-64-bits.json @@ -9,7 +9,7 @@ "severity": "error", "code": "P001", "message": "models/staging/stg_orders.sql model audit 'not_null' 'minimum_samples' 18446744073709551616 is larger than a 64-bit integer", - "help": "set minimum_samples to a value that fits in 64 bits, add this to the MODEL header:\n MODEL (\n audits [not_null (minimum_samples 9223372036854775807)],\n ...\n );" + "help": "set minimum_samples to a value that fits in 64 bits, add this to the MODEL header:\n MODEL (\n audits [not_null (name \"order_ids_present\", minimum_samples 9223372036854775807, severity \"warn\", meta (tiers [\"gold\"]))],\n ...\n );" } ] } From 1faa2c7132b403ac656b3a71b1b549b45a0bed8b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:09:29 +0100 Subject: [PATCH 11/15] fix(rules): exempt each dynamic-pivot SELECT * branch of a set operation SQBRMODEL102 now checks every set-operation branch, but the dynamic-pivot exemption was decided once per query and never matched a set-operation body, so a union of dynamic-pivot star branches was reported per branch although a lone dynamic-pivot star is exempt. The exemption now applies to each branch. --- .../src/engine/tests/test_query_facts.rs | 18 ++++++++++++++++++ .../src/rules/_helpers/dynamic_contracts.rs | 16 ++++++++++------ .../src/rules/_helpers/evaluation.rs | 2 +- 3 files changed, 29 insertions(+), 7 deletions(-) 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 b7b3673829..9c73c0ffc0 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_query_facts.rs @@ -88,6 +88,24 @@ fn given_dialect_sql_when_reading_query_facts_then_findings_match_the_authored_s ("SQBRMODEL102", 4, 8, "SELECT * is allowed only"), ], }, + QueryFactsTestCase { + description: "stars over dynamic pivots in every set-operation branch are exempt", + dialect: "snowflake", + query_sql: "SELECT * FROM orders PIVOT(MAX(amount) FOR category IN (ANY))\nUNION ALL\nSELECT * FROM orders PIVOT(MAX(amount) FOR category IN (ANY))", + select: &["SQBRMODEL102"], + enum_columns: &[], + columns: json!([]), + expected_faults: &[], + }, + QueryFactsTestCase { + description: "only the plain star branch beside a dynamic-pivot branch is reported", + dialect: "snowflake", + query_sql: "SELECT * FROM orders PIVOT(MAX(amount) FOR category IN (ANY))\nUNION ALL\nSELECT * FROM (SELECT order_id FROM orders)", + select: &["SQBRMODEL102"], + enum_columns: &[], + columns: json!([]), + expected_faults: &[("SQBRMODEL102", 3, 8, "SELECT * is allowed only")], + }, QueryFactsTestCase { description: "a comparison of two literals keeps its source position", dialect: "duckdb", diff --git a/crates/sqlbuild-rules/src/rules/_helpers/dynamic_contracts.rs b/crates/sqlbuild-rules/src/rules/_helpers/dynamic_contracts.rs index 7ad6246b76..4143496870 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/dynamic_contracts.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/dynamic_contracts.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use crate::constants::{ENFORCED_CONTRACT, SELECT_STAR_MODEL_RULE_CODE}; use crate::models::{Fault, Model, RuleMetadata, RulesConfig}; -use crate::query_facts::models::{QueryFacts, RelationFacts}; +use crate::query_facts::models::{QueryFacts, RelationFacts, SelectFacts}; use crate::rules::_helpers::evaluation::sole_table_name; use crate::rules::_helpers::{contract_name_types, explicit_output_types, typed_contract_columns}; use crate::rules::models::FaultCollector; @@ -49,15 +49,19 @@ pub(crate) fn evaluate_without_query( Some(faults.into_inner()) } +/// Whether `select` is a lone star over one dynamic pivot, whose columns only the pivot knows. +pub(crate) fn is_dynamic_pivot_star(select: &SelectFacts) -> bool { + select.items.len() == 1 + && select.from.len() == 1 + && select.joins.is_empty() + && matches!(select.from[0], RelationFacts::DynamicPivot) +} + pub(crate) fn allows_output_star(query: &QueryFacts, root: &QueryFacts, model: &Model) -> bool { let Some(select) = query.root_select() else { return false; }; - let dynamic_pivot_star = select.items.len() == 1 - && select.from.len() == 1 - && select.joins.is_empty() - && matches!(select.from[0], RelationFacts::DynamicPivot); - dynamic_pivot_star + is_dynamic_pivot_star(select) || (model.dynamic_columns_proven && std::ptr::eq(query, root) && root.ctes.last().is_some_and(|cte| { diff --git a/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs b/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs index 8dc1d6c4e4..8c9135be8d 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/evaluation.rs @@ -519,7 +519,7 @@ fn select_star( ); } for (select, star) in stars { - if dynamic_output_star { + if dynamic_output_star || dynamic_contracts::is_dynamic_pivot_star(select) { continue; } let lone = select.items.len() == 1; From 38b92de0051d3cfe3d79c9adf5d86082b1c3dc4c Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:09:30 +0100 Subject: [PATCH 12/15] fix: keep delivering lifecycle events when the stall warning cannot be written A failing stderr write of the blocked-sink warning ended the dispatcher thread, leaving queued events undelivered and uncounted. The warning is now best-effort. --- .../_helpers/event_exporting/workers.rs | 10 ++-- .../classes/test_dispatcher.py | 50 +++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs index 533e25d678..89d8ef6872 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs @@ -452,15 +452,17 @@ fn cancel(claims: &Bound<'_, PyDict>, token: u64) -> PyResult<()> { } /// Tell the user, once per stall, that one sink call outlived its timeout and what that costs. -fn warn_blocked(py: Python<'_>, name: &str, timeout: Duration) -> PyResult<()> { - write_stderr( +/// +/// Best-effort: a failing stderr must not stop the dispatcher delivering and counting events. +fn warn_blocked(py: Python<'_>, name: &str, timeout: Duration) { + drop(write_stderr( py, &format!( "Warning: lifecycle sink '{name}' did not return from one call within {}s; \ its events are dropped until that call returns.\n", timeout.as_secs_f64() ), - ) + )); } impl DispatcherWorker { @@ -546,7 +548,7 @@ impl DispatcherWorker { let blocked: Vec = std::mem::take(&mut state.newly_blocked); drop(state); for name in blocked { - warn_blocked(py, &name, shared.invocation_timeout)?; + warn_blocked(py, &name, shared.invocation_timeout); } if notify { shared.notify_all(py)?; diff --git a/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py b/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py index 6ca4da21b6..a833b20db4 100644 --- a/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py +++ b/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py @@ -1,5 +1,6 @@ from __future__ import annotations +import sys import threading import time from collections.abc import Callable, Mapping @@ -940,3 +941,52 @@ def record(*, event: LifecycleEvent) -> None: assert summary.delivered == test_case.expected_delivered assert (summary.failed, summary.dropped) == (1, 1) assert summary.delivered + summary.failed + summary.dropped == summary.accepted + + +class _BrokenStream: + def write(self, text: str) -> int: + raise OSError(f"stderr is closed: {len(text)}") + + def flush(self) -> None: + raise OSError("stderr is closed") + + +@pytest.mark.parametrize( + "test_case", + (EventExporterDispatcherTestCase("a failing stderr does not stop delivery", 2),), + ids=lambda case: case.description, +) +def test_given_failing_stderr_when_sink_stalls_then_delivery_and_counts_continue( + test_case: EventExporterDispatcherTestCase, monkeypatch: pytest.MonkeyPatch +) -> None: + release: threading.Event = threading.Event() + slow_started: threading.Event = threading.Event() + delivered: list[str] = [] + + def record(*, event: LifecycleEvent) -> None: + delivered.append(event.event_id) + if event.event_id == "event-1": + slow_started.set() + _ = release.wait() + + monkeypatch.setattr(sys, "stderr", _BrokenStream()) + dispatcher: EventExporterDispatcher = EventExporterDispatcher( + exporters=(BoundEventExporter("recorder", record, {}),), + shutdown_timeout_seconds=5.0, + invocation_timeout_seconds=0.1, + ) + dispatcher.enqueue(correlated_event(1, "statement_completed")) + assert slow_started.wait(timeout=1.0) + time.sleep(0.3) + dispatcher.enqueue(correlated_event(2, "statement_completed")) + time.sleep(0.05) + release.set() + time.sleep(0.1) + dispatcher.enqueue(correlated_event(3, "statement_completed")) + dispatcher.enqueue(lifecycle_event(4, "invocation_completed")) + + summary: EventExportSummary = dispatcher.shutdown() + + assert delivered == ["event-1", "event-3", "event-4"] + assert summary.delivered == test_case.expected_delivered + assert summary.delivered + summary.failed + summary.dropped == summary.accepted From 3d1308f6c3a9c95c0b5ca30092d0b7ffb16a8fc5 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:09:30 +0100 Subject: [PATCH 13/15] fix: keep a unit test in its folder when a move would change what it resolves Test SQL resolves macros, enums and constants from the test file's own folder, so moving a model's test to the destination's mirrored folder could hide a folder-scoped test declaration, failing verification. A test file now moves only when every scoped declaration it uses resolves to the same definition from the destination folder; otherwise it is renamed in place and the output's new notes section names the file and the declaration. The JSON output gains a notes array. --- .../_helpers/refactor/refactor_command.rs | 11 ++- .../_helpers/refactor/refactor_facts.rs | 17 +++- .../bindings/_helpers/scopes/scope_reports.rs | 16 ++++ .../main/render_refactor_json.rs | 4 + .../main/render_refactor_text.rs | 5 ++ .../src/refactor_command/tests/helpers.rs | 1 + .../refactoring/_helpers/edits/text_edits.rs | 3 + .../_helpers/planning/model_planning.rs | 6 +- .../_helpers/planning/unit_test_moves.rs | 37 ++++++++- .../src/refactoring/main/plan_refactor.rs | 8 +- .../src/refactoring/models.rs | 3 + .../src/refactoring/tests/helpers.rs | 29 +++++-- .../src/refactoring/tests/test_planning.rs | 41 +++++++++- .../src/refactoring/tests/test_types.rs | 4 + .../src/refactoring/types.rs | 5 ++ .../src/scope_report/_helpers/moves.rs | 69 +++++++++++++++- .../src/scope_report/main/mod.rs | 1 + .../scope_report/main/rebound_scope_usages.rs | 13 +++ .../sqlbuild/references/docs/cli/rename.md | 3 +- .../cli/commands/main/refactor/_test_types.py | 9 +++ .../refactor/test_unit_tests_follow_models.py | 81 +++++++++++++++++++ .../compiler/failure/refactor-mv-model.json | 1 + .../refactor-rename-column-cascade.json | 1 + .../refactor-rename-column-star-refused.json | 1 + .../failure/refactor-rename-column.json | 1 + ...factor-rename-model-collision-refused.json | 1 + .../refactor-rename-model-dry-run.json | 1 + .../refactor-rename-model-yaml-anchors.json | 1 + .../failure/refactor-rename-model.json | 1 + website/src/content/docs/docs/cli/rename.mdx | 3 +- 30 files changed, 357 insertions(+), 20 deletions(-) create mode 100644 crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs diff --git a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs index e10f8a1198..46dad3bdc0 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs @@ -42,7 +42,7 @@ use crate::bindings::_helpers::records::fields::mapping_items; use crate::bindings::_helpers::records::ids::ErrorId; use crate::bindings::_helpers::refactor::offline_compile::{OfflineCompile, compile_for_refactor}; use crate::bindings::_helpers::refactor::refactor_facts::{ - declaration_placement, model_facts, refactor_facts, + declaration_placement, model_facts, refactor_facts, unit_test_rebindings, }; const STAGING_PREFIX: &str = "sqb-refactor-"; @@ -238,7 +238,14 @@ fn plan( RefactorError::internal("the declaration-move host failed") }) }; - let planned: Result = plan_refactor(&facts, request, &host); + let test_host = |path: &str, destination: &str| -> Result>, RefactorError> { + unit_test_rebindings(project, (path, destination)).map_err(|error| { + host_error.replace(Some(error)); + RefactorError::internal("the unit-test scope host failed") + }) + }; + let planned: Result = + plan_refactor(&facts, request, &host, &test_host); if let Some(error) = host_error.into_inner() { return Err(error); } diff --git a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs index 31d1ee4be7..38e849f3e6 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs @@ -17,7 +17,7 @@ use crate::bindings::_helpers::records::fields::field as native_field; use crate::bindings::_helpers::records::ids::RecordId; use crate::bindings::_helpers::refactor::python_strings::find_python_string_locations; use crate::bindings::_helpers::scopes::scope_lookup::build_scope_lookup; -use crate::bindings::_helpers::scopes::scope_reports::scope_move_preview; +use crate::bindings::_helpers::scopes::scope_reports::{scope_move_preview, scope_rebound_usages}; use crate::bindings::_helpers::scopes::scope_validation::scope_relocated_declarations; const GENERIC_DIALECT: &str = "generic"; @@ -357,6 +357,21 @@ fn identity_key(identity: &Bound<'_, PyAny>) -> PyResult { identity.repr()?.extract() } +/// The scoped declarations the unit test file at `path` uses that would resolve differently +/// from `destination`, or `None` when that cannot be worked out. +pub(crate) fn unit_test_rebindings( + project: &Bound<'_, PyAny>, + paths: (&str, &str), +) -> PyResult>> { + let py: Python<'_> = project.py(); + let index: Bound<'_, PyAny> = native_field( + &native_field(&native_field(project, "graph")?, "project")?, + "scope_index", + )?; + let lookup: Bound<'_, PyAny> = build_scope_lookup(py, &index)?.into_bound(py); + scope_rebound_usages(py, &lookup, paths) +} + /// Where the shared scope placement puts declarations when `model` moves to `destination`. pub(crate) fn declaration_placement( project: &Bound<'_, PyAny>, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs b/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs index 82496de146..eb1b6d9b68 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs @@ -17,6 +17,7 @@ use sqlbuild_scopes::scope_report::main::cached_projection_index::cached_project use sqlbuild_scopes::scope_report::main::explain_scope_declaration::explain_scope_declaration; use sqlbuild_scopes::scope_report::main::list_scope_declarations::list_scope_declarations; use sqlbuild_scopes::scope_report::main::preview_scope_move::preview_scope_move; +use sqlbuild_scopes::scope_report::main::rebound_scope_usages::rebound_scope_usages; use sqlbuild_scopes::scope_report::main::scope_metadata_projection::scope_metadata_projection; use sqlbuild_scopes::scope_report::main::serialize_scope_result::serialize_scope_result; use sqlbuild_scopes::scope_report::models::{ @@ -306,6 +307,21 @@ pub(crate) fn scope_move_preview( } } +/// The scoped declarations the resources at `path` use that resolve differently from +/// `destination`, or `None` when `destination` cannot hold them. +pub(crate) fn scope_rebound_usages( + py: Python<'_>, + lookup: &Bound<'_, PyAny>, + paths: (&str, &str), +) -> PyResult>> { + let reports: LookupReports = lookup_reports(lookup)?; + let native: &NativeScopeLookup = reports.lookup.get(); + match rebound_scope_usages(reports.source(native), paths) { + Ok(rebound) => Ok(rebound), + Err(failure) => Err(failure_error(py, &failure)?), + } +} + fn serialized(classes: &ScopeClasses<'_>, result: &Bound<'_, PyAny>) -> PyResult { let result: ScopeResult = read_result(classes, result)?; serialize_scope_result(&result).map_err(|failure| match failure { diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs index 58f53d4544..0ea303cdbd 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs @@ -73,6 +73,10 @@ pub fn render_refactor_json( "blocking", JsonValue::Array(plan.blocking.iter().map(location).collect()), ), + ( + "notes", + JsonValue::Array(plan.notes.iter().map(location).collect()), + ), ( "compile", object(vec![ diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs index 471e5903f4..a10de1e462 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs @@ -26,6 +26,11 @@ pub fn render_refactor_text( lines.push(style.section("Migrations")); lines.extend(migration_lines(style, &plan.migrations)); } + if !plan.notes.is_empty() && !blocked { + lines.push(style.section("Notes")); + let notes: Vec<&ManualLocation> = plan.notes.iter().collect(); + lines.extend(location_lines(style, ¬es)); + } if blocked { lines.push(style.error_strong(&format!( "Cannot {} {} automatically", diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs index b991b82e9a..708440bb4a 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs @@ -23,6 +23,7 @@ pub(super) fn rename_plan(old_path: &str, new_name: &str) -> RefactorPlan { migrations: Vec::new(), renamed_columns: Vec::new(), help: None, + notes: Vec::new(), } } diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs index 7c3ff1d089..0411ec1229 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs @@ -19,6 +19,7 @@ pub(crate) struct RefactorParts { pub(crate) migrations: Vec, pub(crate) cascaded: Vec, pub(crate) moves: Vec<(String, String)>, + pub(crate) notes: Vec, } /// The 1-based line and column of an offset. @@ -118,6 +119,7 @@ pub(crate) fn merge_parts(parts: Vec) -> RefactorParts { merged.blocking.extend(part.blocking); merged.migrations.extend(part.migrations); merged.cascaded.extend(part.cascaded); + merged.notes.extend(part.notes); } merged } @@ -188,6 +190,7 @@ pub(crate) fn build_plan( migrations: parts.migrations, renamed_columns, help, + notes: parts.notes, } } diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs index e277dc8676..75f372debb 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs @@ -25,7 +25,7 @@ use crate::refactoring::errors::RefactorError; use crate::refactoring::models::{ ManualLocation, ModelFacts, RefactorFacts, RefactorOperation, RefactorRequest, TextEdit, }; -use crate::refactoring::types::DeclarationMoveHost; +use crate::refactoring::types::{DeclarationMoveHost, UnitTestScopeHost}; /// The model a request names, and the request with its name and destination resolved. pub(crate) struct ModelTarget<'a> { @@ -168,7 +168,7 @@ pub(crate) fn model_parts( facts: &RefactorFacts, target: &ModelTarget<'_>, context: &ScanContext, - host: &DeclarationMoveHost<'_>, + (host, test_host): (&DeclarationMoveHost<'_>, &UnitTestScopeHost<'_>), ) -> Result { let files = project_sql_files(facts); let old = target.model.name.as_str(); @@ -193,6 +193,7 @@ pub(crate) fn model_parts( names: (old, new), paths: (&target.source_path, &destination), }, + test_host, )?; edits.extend(tests.edits); let mut manual = macro_reference_locations(&facts.models, &files, old, context); @@ -208,6 +209,7 @@ pub(crate) fn model_parts( manual, blocking, moves, + notes: tests.notes, ..RefactorParts::default() }) } diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs index e9b0715319..02124df9a8 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs @@ -12,6 +12,7 @@ use crate::refactoring::constants::{ }; use crate::refactoring::errors::RefactorError; use crate::refactoring::models::{ManualLocation, UnitTestFacts}; +use crate::refactoring::types::UnitTestScopeHost; /// The model's names and project-relative paths before and after the refactoring. pub(crate) struct FollowedModel<'a> { @@ -20,11 +21,15 @@ pub(crate) struct FollowedModel<'a> { } /// Rename `TEST` names of tests that target only the model, and move files holding only them. +/// +/// A file moves only when every macro, enum and constant it uses resolves to the same +/// declaration from the destination folder; otherwise it is renamed in place, with a note. pub(crate) fn unit_test_parts( project_dir: &Path, tests: &[UnitTestFacts], files: &[ProjectSqlFile], model: &FollowedModel<'_>, + host: &UnitTestScopeHost<'_>, ) -> Result { let (old, _) = model.names; let mut by_path: BTreeMap<&str, Vec<&UnitTestFacts>> = BTreeMap::new(); @@ -51,7 +56,18 @@ pub(crate) fn unit_test_parts( if !blocks.iter().all(|test| targets_only_model(test)) { continue; } - let destination = test_destination(path, model); + let mut destination = test_destination(path, model); + if parent(&destination) != parent(path) + && let Some(reason) = kept_in_folder(path, &destination, host)? + { + destination = format!("{}/{}", parent(path), name(&destination)); + parts.notes.push(ManualLocation { + path: destination.clone(), + line: None, + column: None, + reason, + }); + } if destination == path { continue; } @@ -74,6 +90,25 @@ pub(crate) fn unit_test_parts( Ok(parts) } +/// Why the test file at `path` must stay in its folder instead of moving to `destination`. +fn kept_in_folder( + path: &str, + destination: &str, + host: &UnitTestScopeHost<'_>, +) -> Result, RefactorError> { + let folder = parent(destination); + Ok(match host(path, destination)? { + Some(rebound) if rebound.is_empty() => None, + Some(rebound) => Some(format!( + "kept in its folder: {} would not resolve to the same declaration from {folder}", + rebound.join(", ") + )), + None => Some(format!( + "kept in its folder: its declarations could not be checked from {folder}" + )), + }) +} + /// Where a test file of the model belongs after the refactoring. fn test_destination(path: &str, model: &FollowedModel<'_>) -> String { let (old, new) = model.names; diff --git a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs index d7547864b1..b1874567ce 100644 --- a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs +++ b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs @@ -10,13 +10,15 @@ use crate::refactoring::constants::{COLUMN_MANUAL_HELP, MODEL_MANUAL_HELP}; use crate::refactoring::errors::RefactorError; use crate::refactoring::models::{RefactorFacts, RefactorOperation, RefactorPlan, RefactorRequest}; -use crate::refactoring::types::DeclarationMoveHost; +use crate::refactoring::types::{DeclarationMoveHost, UnitTestScopeHost}; -/// Plan one refactoring without touching files; `host` finds declaration-file moves. +/// Plan one refactoring without touching files; `host` finds declaration-file moves and +/// `test_host` the declarations that keep a unit test file in its folder. pub fn plan_refactor( facts: &RefactorFacts, request: &RefactorRequest, host: &DeclarationMoveHost<'_>, + test_host: &UnitTestScopeHost<'_>, ) -> Result { let context = ScanContext::for_facts(facts); if request.operation == RefactorOperation::RenameColumn { @@ -35,7 +37,7 @@ pub fn plan_refactor( )); } let target = model_target(facts, request)?; - let parts = model_parts(facts, &target, &context, host)?; + let parts = model_parts(facts, &target, &context, (host, test_host))?; let mut moves = parts.moves.clone(); let destination = target.destination(); if destination != target.source_path { diff --git a/crates/sqlbuild-refactor/src/refactoring/models.rs b/crates/sqlbuild-refactor/src/refactoring/models.rs index f16c24efd1..08ff608936 100644 --- a/crates/sqlbuild-refactor/src/refactoring/models.rs +++ b/crates/sqlbuild-refactor/src/refactoring/models.rs @@ -131,6 +131,9 @@ pub struct RefactorPlan { pub migrations: Vec, pub renamed_columns: Vec<(String, String, String)>, pub help: Option, + /// Files the refactoring deliberately leaves in place, and why. + #[serde(default)] + pub notes: Vec, } impl RefactorPlan { diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index 1c182230b8..2758be1e81 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -328,7 +328,8 @@ pub(super) fn blocking_reasons( ..DeclarationPlacement::default() }) }; - let parts = model_parts(facts, &target, &context, &host)?; + let test_host = |_: &str, _: &str| Ok(Some(Vec::new())); + let parts = model_parts(facts, &target, &context, (&host, &test_host))?; Ok(parts .blocking .into_iter() @@ -648,11 +649,13 @@ pub(super) fn owner_header_parts( Ok((described(&edits), manual)) } -/// Each planned file as `original -> path`, with the replacements made in it. +/// Each planned test file as `original -> path` with the replacements made in it, and each +/// `path: reason` note, when the scope host reports `rebound` for every test file. pub(super) fn followed_unit_tests( request: &RefactorRequest, tests: &[(&str, &str, &[&str])], -) -> Result)>, RefactorError> { + rebound: &[&str], +) -> Result<(Vec<(String, Vec)>, Vec), RefactorError> { let mut orders: ModelFacts = model("orders", Some("view"), "analytics"); orders.path = "models/marts/orders.sql".to_owned(); let mut project_facts = facts("/nonexistent/project", vec![orders]); @@ -675,8 +678,22 @@ pub(super) fn followed_unit_tests( ..DeclarationPlacement::default() }) }; - let plan = - crate::refactoring::main::plan_refactor::plan_refactor(&project_facts, request, &host)?; + let test_host = |_: &str, _: &str| { + Ok(Some( + rebound.iter().map(|item| (*item).to_owned()).collect(), + )) + }; + let plan = crate::refactoring::main::plan_refactor::plan_refactor( + &project_facts, + request, + &host, + &test_host, + )?; + let notes: Vec = plan + .notes + .iter() + .map(|note| format!("{}: {}", note.path, note.reason)) + .collect(); let mut changes: Vec<(String, Vec)> = plan .changes .into_iter() @@ -693,7 +710,7 @@ pub(super) fn followed_unit_tests( }) .collect(); changes.sort(); - Ok(changes) + Ok((changes, notes)) } /// Expected `original -> path` test file changes with their replacements, sorted by path. diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs index 09858464d9..821cbacb02 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs @@ -349,6 +349,7 @@ fn given_unit_tests_of_model_when_renaming_or_moving_then_tests_follow() -> Resu FollowedUnitTestTestCase { description: "a rename renames owned files and TEST names in place", request: request(RefactorOperation::RenameModel, "order_lines", None), + rebound: &["macro:mock_orders"], expected_changes: followed_test_changes(&[ ( "tests/unit/marts/test_orders__keeps_rows.sql -> tests/unit/marts/test_order_lines__keeps_rows.sql", @@ -359,23 +360,59 @@ fn given_unit_tests_of_model_when_renaming_or_moving_then_tests_follow() -> Resu &["\"order_lines__flat_layout\""], ), ]), + expected_notes: &[], }, FollowedUnitTestTestCase { description: "a move takes mirrored tests to the mirrored folder and leaves others", request: request(RefactorOperation::MoveModel, "", Some("models/finance/")), + rebound: &[], expected_changes: followed_test_changes(&[( "tests/unit/marts/test_orders__keeps_rows.sql -> tests/unit/finance/test_orders__keeps_rows.sql", &[] as &[&str], )]), + expected_notes: &[], + }, + FollowedUnitTestTestCase { + description: "a move leaves a test whose declarations would rebind in its folder", + request: request(RefactorOperation::MoveModel, "", Some("models/finance/")), + rebound: &["macro:mock_orders"], + expected_changes: Vec::new(), + expected_notes: &[ + "tests/unit/marts/test_orders__keeps_rows.sql: kept in its folder: macro:mock_orders would not resolve to the same declaration from tests/unit/finance", + ], + }, + FollowedUnitTestTestCase { + description: "a move with a new name renames a kept test in its folder", + request: request( + RefactorOperation::MoveModel, + "", + Some("models/finance/order_lines.sql"), + ), + rebound: &["enum:order_status", "macro:mock_orders"], + expected_changes: followed_test_changes(&[ + ( + "tests/unit/marts/test_orders__keeps_rows.sql -> tests/unit/marts/test_order_lines__keeps_rows.sql", + &["\"order_lines__keeps_rows\""] as &[&str], + ), + ( + "tests/unit/test_orders.sql -> tests/unit/test_order_lines.sql", + &["\"order_lines__flat_layout\""], + ), + ]), + expected_notes: &[ + "tests/unit/marts/test_order_lines__keeps_rows.sql: kept in its folder: enum:order_status, macro:mock_orders would not resolve to the same declaration from tests/unit/finance", + ], }, ]; for test_case in test_cases { + let (changes, notes) = + followed_unit_tests(&test_case.request, tests, test_case.rebound)?; assert_eq!( - followed_unit_tests(&test_case.request, tests)?, - test_case.expected_changes, + changes, test_case.expected_changes, "{}", test_case.description ); + assert_eq!(notes, test_case.expected_notes, "{}", test_case.description); } Ok(()) } diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs index 1b31da41f5..f22c0c54f1 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs @@ -221,6 +221,10 @@ pub(super) struct HeaderPartsTestCase { pub(super) struct FollowedUnitTestTestCase { pub(super) description: &'static str, pub(super) request: RefactorRequest, + /// Declarations the scope host reports as resolving differently from any destination. + pub(super) rebound: &'static [&'static str], /// Each `original -> path` test file change with its replacements, sorted by path. pub(super) expected_changes: Vec<(String, Vec)>, + /// Each `path: reason` note of a test file kept in its folder. + pub(super) expected_notes: &'static [&'static str], } diff --git a/crates/sqlbuild-refactor/src/refactoring/types.rs b/crates/sqlbuild-refactor/src/refactoring/types.rs index fc910016d4..e31b53ebb9 100644 --- a/crates/sqlbuild-refactor/src/refactoring/types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/types.rs @@ -7,6 +7,11 @@ use crate::refactoring::models::DeclarationPlacement; pub type DeclarationMoveHost<'a> = dyn Fn(&str, &str) -> Result + 'a; +/// The scoped declarations a unit test file uses that would resolve differently, or not at all, +/// if it moved: `(test path, destination path)`; `None` when that cannot be worked out. +pub type UnitTestScopeHost<'a> = + dyn Fn(&str, &str) -> Result>, RefactorError> + 'a; + /// Original texts keyed by original path, in plan order. pub type Originals = Vec<(String, String)>; diff --git a/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs b/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs index 66304077ca..d17b3e40e3 100644 --- a/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs +++ b/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs @@ -5,7 +5,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use crate::scope_index::main::classify_report_resource::classify_report_resource; use crate::scope_index::main::format_identity::format_identity; use crate::scope_index::models::{ - DeclarationIdentity, OwnershipRoot, QualifiedIdentity, ReportClassification, ResourceRecord, + DeclarationIdentity, DeclarationKind, OwnershipRoot, QualifiedIdentity, ReportClassification, ResourceRecord, ScopeKind, }; use crate::scope_report::_helpers::context::Context; @@ -206,3 +206,70 @@ pub(crate) fn move_preview( Vec::new(), )) } + +/// Directly visible positions of each scoped (macro, enum, constant) declaration identity. +fn scoped_bindings(context: &Context<'_>, values: Visible) -> BTreeMap> { + let mut bindings: BTreeMap> = BTreeMap::new(); + for (position, _) in values { + let identity: &DeclarationIdentity = &context.declaration(position).identity; + if matches!( + identity.kind, + DeclarationKind::Macro | DeclarationKind::Enum | DeclarationKind::Constant + ) { + bindings.entry(qualified(identity)).or_default().push(position); + } + } + for positions in bindings.values_mut() { + positions.sort_unstable(); + } + bindings +} + +/// The scoped declarations the resources at `path` use that would resolve to different +/// definitions, or to none, from `destination`; `None` when `destination` cannot hold them. +pub(crate) fn rebound_usages( + source: ReportSource<'_>, + (path, destination): (&str, &str), +) -> Result>, ReportFailure> { + let context: Context<'_> = Context::new(source); + let Ok(destination) = normalized(destination) else { + return Ok(None); + }; + let mut rebound: Vec = Vec::new(); + for position in context.resources_at(path) { + let moving: &ResourceRecord = context.resource(*position); + let roots: Vec = destination_roots(&context, moving, &destination)?; + let [root] = roots.as_slice() else { + return Ok(None); + }; + let moved: ResourceRecord = ResourceRecord { + identity: moving.identity.clone(), + path: destination.clone(), + ownership_root: root.clone(), + }; + let (old_direct, _) = visible( + classify_report_resource(source.records, source.lookup, moving) + .map_err(ReportFailure::Path)?, + ); + let (new_direct, _) = visible( + classify_report_resource(source.records, source.lookup, &moved) + .map_err(ReportFailure::Path)?, + ); + let old: BTreeMap> = scoped_bindings(&context, old_direct); + let new: BTreeMap> = scoped_bindings(&context, new_direct); + let consumer: QualifiedIdentity = QualifiedIdentity::Resource(moving.identity.clone()); + for usage in context.usages_of_consumer(&consumer) { + let usage = context.usage(*usage); + let text: String = qualified(&usage.declaration); + if usage.through.is_none() + && old.contains_key(&text) + && old.get(&text) != new.get(&text) + && !rebound.contains(&text) + { + rebound.push(text); + } + } + } + rebound.sort(); + Ok(Some(rebound)) +} diff --git a/crates/sqlbuild-scopes/src/scope_report/main/mod.rs b/crates/sqlbuild-scopes/src/scope_report/main/mod.rs index 3d0b07f168..d53615bb8f 100644 --- a/crates/sqlbuild-scopes/src/scope_report/main/mod.rs +++ b/crates/sqlbuild-scopes/src/scope_report/main/mod.rs @@ -8,5 +8,6 @@ pub mod cached_report_records; pub mod explain_scope_declaration; pub mod list_scope_declarations; pub mod preview_scope_move; +pub mod rebound_scope_usages; pub mod scope_metadata_projection; pub mod serialize_scope_result; diff --git a/crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs b/crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs new file mode 100644 index 0000000000..fe76237ca0 --- /dev/null +++ b/crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs @@ -0,0 +1,13 @@ +//! `rebound_scope_usages`: which used declarations a resource file would lose or rebind if moved. + +use crate::scope_report::_helpers::moves::rebound_usages; +use crate::scope_report::models::{ReportFailure, ReportSource}; + +/// Qualified identities of the scoped macros, enums and constants the resources at `path` use +/// that resolve differently from `destination`, or `None` when `destination` cannot hold them. +pub fn rebound_scope_usages( + source: ReportSource<'_>, + paths: (&str, &str), +) -> Result>, ReportFailure> { + rebound_usages(source, paths) +} diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md index 38bf749fcc..8a817edcba 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md @@ -69,6 +69,7 @@ The model's own unit tests, the test files whose every `TEST` block targets only - a file named `test_.sql` or `test___.sql` is renamed to the new name; - `TEST (name "__")` takes the new name as its subject; - a test file in the folder that mirrors the model, `tests/unit//` for `models//`, moves to the folder that mirrors the destination. Test files kept elsewhere stay where they are. +- a test file stays in its folder, still renamed, when a macro, enum, or constant it uses would resolve to a different declaration, or to none, from the destination folder, such as a test macro in `tests/unit//_sqlbuild/_macros/`. The output's notes name the file and the declaration. Tests that also target other models keep their file and name; only their fixtures and references change. @@ -150,7 +151,7 @@ Migrations Compiled: ok, 3 files changed ``` -With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, and `compile.errors`. +With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, `notes` (files deliberately kept in place, with the reason), and `compile.errors`. ## Exit codes diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py index 5497a0df56..4a8f9d1bd0 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py @@ -148,3 +148,12 @@ class FollowedUnitTestDryRunE2ETestCase: command: tuple[str, ...] expected_status: str expected_path: str + + +@dataclass(frozen=True) +class ScopedUnitTestMoveE2ETestCase: + description: str + extra_files: dict[str, str] + amount_sql: str + expected_path: str + expected_note: str | None diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py index 60bf0b9572..1fe2fc18ec 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py @@ -11,8 +11,10 @@ from tests.e2e.src.sqlbuild.cli.commands.main.refactor._test_types import ( FollowedUnitTestDryRunE2ETestCase, FollowedUnitTestE2ETestCase, + ScopedUnitTestMoveE2ETestCase, ) from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( + CUSTOMER_TOTALS, FACT_ORDERS_TEST, fragments_present, project_text, @@ -130,3 +132,82 @@ def test_given_dry_run_when_renaming_model_with_unit_test_then_reports_move_and_ assert payload["status"] == test_case.expected_status assert test_case.expected_path in [str(item["path"]) for item in payload["files"]] assert project_text(project_dir) == before + + +_SAMPLE_AMOUNT_MACRO: str = 'def sample_amount() -> str:\n return "{amount}"\n' +_MARTS_MACRO: str = "tests/unit/marts/_sqlbuild/_macros/sample_amount.py" + + +@pytest.mark.parametrize( + "test_case", + [ + ScopedUnitTestMoveE2ETestCase( + description="a folder-scoped test macro keeps the test in its folder", + extra_files={_MARTS_MACRO: _SAMPLE_AMOUNT_MACRO.format(amount=25)}, + amount_sql="@sample_amount()", + expected_path="tests/unit/marts/test_order_facts__keeps_orders.sql", + expected_note="macro:sample_amount", + ), + ScopedUnitTestMoveE2ETestCase( + description="a folder-scoped test constant keeps the test in its folder", + extra_files={ + "tests/unit/marts/_sqlbuild/_constants/amounts.sql": ( + "CONSTANT (name sample_amount, value 25);\n" + ), + }, + amount_sql='@const("sample_amount")', + expected_path="tests/unit/marts/test_order_facts__keeps_orders.sql", + expected_note="constant:sample_amount", + ), + ScopedUnitTestMoveE2ETestCase( + description="a project-wide constant lets the test move", + extra_files={ + "constants/orders.sql": "CONSTANT (name sample_amount, value 25);\n", + "models/marts/customer_totals.sql": CUSTOMER_TOTALS.replace( + "GROUP BY customer_id\n", + 'GROUP BY customer_id\nHAVING SUM(amount) >= @const("sample_amount")\n', + ), + }, + amount_sql='@const("sample_amount")', + expected_path="tests/unit/reporting/test_order_facts__keeps_orders.sql", + expected_note=None, + ), + ], + ids=lambda case: case.description, +) +def test_given_scoped_declarations_when_moving_model_then_test_moves_only_if_they_resolve_alike( + tmp_path: Path, test_case: ScopedUnitTestMoveE2ETestCase +) -> None: + """A test moves only when its macros, enums and constants resolve alike from the destination. + + Without SQBRTEST103 selected, a test kept outside the destination's mirrored folder compiles. + """ + + test_sql: str = ( + _FILES[_MIRRORED_TEST] + .replace("25 AS amount", f"{test_case.amount_sql} AS amount", 1) + ) + project_dir: Path = write_orders_project( + tmp_path=tmp_path, + files={_MIRRORED_TEST: test_sql, **test_case.extra_files}, + ) + remove_files(project_dir=project_dir, paths=("tests/unit/test_fact_orders.sql",)) + + result: subprocess.CompletedProcess[str] = sqb( + project_dir, "mv", "model:fact_orders", "models/reporting/order_facts.sql" + ) + tested: subprocess.CompletedProcess[str] = sqb(project_dir, "test") + + assert result.returncode == 0, result.stdout + result.stderr + assert not (project_dir / _MIRRORED_TEST).exists() + moved: str = (project_dir / test_case.expected_path).read_text(encoding="utf-8") + assert 'TEST (name "order_facts__keeps_orders");' in moved + assert test_case.amount_sql in moved + notes: list[str] = [line for line in result.stdout.splitlines() if "kept in its folder" in line] + if test_case.expected_note is None: + assert notes == [], result.stdout + else: + assert len(notes) == 1, result.stdout + assert test_case.expected_path in notes[0] + assert test_case.expected_note in notes[0] + assert tested.returncode == 0, tested.stdout + tested.stderr diff --git a/tests/goldens/compiler/failure/refactor-mv-model.json b/tests/goldens/compiler/failure/refactor-mv-model.json index 71e83421bd..6bf7d16bc7 100644 --- a/tests/goldens/compiler/failure/refactor-mv-model.json +++ b/tests/goldens/compiler/failure/refactor-mv-model.json @@ -23,6 +23,7 @@ "renamed_columns": [], "manual": [], "blocking": [], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/tests/goldens/compiler/failure/refactor-rename-column-cascade.json b/tests/goldens/compiler/failure/refactor-rename-column-cascade.json index dbab17a01c..50248b4ecf 100644 --- a/tests/goldens/compiler/failure/refactor-rename-column-cascade.json +++ b/tests/goldens/compiler/failure/refactor-rename-column-cascade.json @@ -42,6 +42,7 @@ ], "manual": [], "blocking": [], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json b/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json index 676dd69f70..fa264ffbf2 100644 --- a/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json +++ b/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json @@ -44,6 +44,7 @@ } ], "blocking": [], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/tests/goldens/compiler/failure/refactor-rename-column.json b/tests/goldens/compiler/failure/refactor-rename-column.json index 9746f5f81c..ee6aba0795 100644 --- a/tests/goldens/compiler/failure/refactor-rename-column.json +++ b/tests/goldens/compiler/failure/refactor-rename-column.json @@ -50,6 +50,7 @@ ], "manual": [], "blocking": [], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json b/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json index cbe35bb606..028a84732f 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json +++ b/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json @@ -43,6 +43,7 @@ "reason": "model:customer_totals already exists" } ], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json b/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json index 3f50115661..1be2e24417 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json +++ b/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json @@ -36,6 +36,7 @@ "renamed_columns": [], "manual": [], "blocking": [], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json b/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json index efc3507a2c..d2bdae8b76 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json +++ b/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json @@ -56,6 +56,7 @@ "renamed_columns": [], "manual": [], "blocking": [], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/tests/goldens/compiler/failure/refactor-rename-model.json b/tests/goldens/compiler/failure/refactor-rename-model.json index ecb2e359f0..c12a99df9c 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model.json +++ b/tests/goldens/compiler/failure/refactor-rename-model.json @@ -57,6 +57,7 @@ "renamed_columns": [], "manual": [], "blocking": [], + "notes": [], "compile": { "ok": true, "errors": [] diff --git a/website/src/content/docs/docs/cli/rename.mdx b/website/src/content/docs/docs/cli/rename.mdx index 01d053e8e4..fa5226d2b2 100644 --- a/website/src/content/docs/docs/cli/rename.mdx +++ b/website/src/content/docs/docs/cli/rename.mdx @@ -54,6 +54,7 @@ The model's own unit tests, the test files whose every `TEST` block targets only - a file named `test_.sql` or `test___.sql` is renamed to the new name; - `TEST (name "__")` takes the new name as its subject; - a test file in the folder that mirrors the model, `tests/unit//` for `models//`, moves to the folder that mirrors the destination. Test files kept elsewhere stay where they are. +- a test file stays in its folder, still renamed, when a macro, enum, or constant it uses would resolve to a different declaration, or to none, from the destination folder, such as a test macro in `tests/unit//_sqlbuild/_macros/`. The output's notes name the file and the declaration. Tests that also target other models keep their file and name; only their fixtures and references change. @@ -135,7 +136,7 @@ Migrations Compiled: ok, 3 files changed ``` -With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, and `compile.errors`. +With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, `notes` (files deliberately kept in place, with the reason), and `compile.errors`. ## Exit codes From 97f28f0314122c7fb0f3cba28ee6b82a6dea1de3 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 17:02:58 +0100 Subject: [PATCH 14/15] style: satisfy the doc-comment, parameter and test-shape checks for the batch --- .../_helpers/event_exporting/workers.rs | 4 +-- .../_helpers/refactor/refactor_facts.rs | 3 +- .../bindings/_helpers/scopes/scope_reports.rs | 3 +- .../_helpers/edits/header_edits.rs | 12 +++---- .../_helpers/planning/model_planning.rs | 3 +- .../_helpers/planning/unit_test_moves.rs | 8 ++--- .../src/refactoring/main/plan_refactor.rs | 3 +- .../src/refactoring/tests/helpers.rs | 8 +++-- .../src/refactoring/tests/test_planning.rs | 3 +- .../src/refactoring/types.rs | 3 +- .../src/scope_report/_helpers/moves.rs | 12 ++++--- .../scope_report/main/rebound_scope_usages.rs | 3 +- .../cli/commands/main/refactor/_test_types.py | 2 +- .../refactor/test_unit_tests_follow_models.py | 32 +++++++++---------- .../classes/test_dispatcher.py | 14 +++++--- 15 files changed, 51 insertions(+), 62 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs index 89d8ef6872..3d3a8169d6 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/event_exporting/workers.rs @@ -451,9 +451,7 @@ fn cancel(claims: &Bound<'_, PyDict>, token: u64) -> PyResult<()> { claims.set_item(token, false) } -/// Tell the user, once per stall, that one sink call outlived its timeout and what that costs. -/// -/// Best-effort: a failing stderr must not stop the dispatcher delivering and counting events. +/// Warn once per stall that a sink call outlived its timeout; best-effort, ignoring write errors. fn warn_blocked(py: Python<'_>, name: &str, timeout: Duration) { drop(write_stderr( py, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs index 38e849f3e6..743f5b6a6a 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs @@ -357,8 +357,7 @@ fn identity_key(identity: &Bound<'_, PyAny>) -> PyResult { identity.repr()?.extract() } -/// The scoped declarations the unit test file at `path` uses that would resolve differently -/// from `destination`, or `None` when that cannot be worked out. +/// Scoped declarations the test file at `path` uses that resolve differently from `destination`. pub(crate) fn unit_test_rebindings( project: &Bound<'_, PyAny>, paths: (&str, &str), diff --git a/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs b/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs index eb1b6d9b68..85665749d3 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_reports.rs @@ -307,8 +307,7 @@ pub(crate) fn scope_move_preview( } } -/// The scoped declarations the resources at `path` use that resolve differently from -/// `destination`, or `None` when `destination` cannot hold them. +/// Scoped declarations the resources at `path` use that resolve differently from `destination`. pub(crate) fn scope_rebound_usages( py: Python<'_>, lookup: &Bound<'_, PyAny>, diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs index ddae3f23cf..c1c0a50c3e 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs @@ -653,8 +653,7 @@ fn line_indent(text: &[char], offset: usize) -> String { /// Code-point spans of every `SCHEMA (...)` header body, as `SCHEMA_STATEMENT_PATTERN.finditer`. fn schema_header_spans(contents: &str, text: &[char]) -> Vec<(usize, usize)> { statement_header_spans( - contents, - text, + (contents, text), SCHEMA_KEYWORD, |index| { (index == 0 || !is_word_char(text[index - 1])) @@ -664,11 +663,9 @@ fn schema_header_spans(contents: &str, text: &[char]) -> Vec<(usize, usize)> { ) } -/// Code-point spans of `keyword (...)` header bodies starting where `opens` allows, scanning on -/// from `resume` of each body's end. +/// Code-point spans of `keyword (...)` bodies opening where `opens` allows, resuming at `resume`. fn statement_header_spans( - contents: &str, - text: &[char], + (contents, text): (&str, &[char]), keyword: &str, opens: impl Fn(usize) -> bool, resume: impl Fn(usize) -> usize, @@ -726,8 +723,7 @@ pub(crate) fn schema_model_name_edits( /// Code-point spans of every `TEST (...)` header body that starts a line, as discovery splits blocks. fn test_header_spans(contents: &str, text: &[char]) -> Vec<(usize, usize)> { statement_header_spans( - contents, - text, + (contents, text), TEST_KEYWORD, |index| index == 0 || text[index - 1] == '\n', |end| end, diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs index 75f372debb..26370cf44c 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs @@ -187,8 +187,7 @@ pub(crate) fn model_parts( let destination = target.destination(); let tests = unit_test_parts( Path::new(&facts.project_dir), - &facts.unit_tests, - &files, + (&facts.unit_tests, &files), &FollowedModel { names: (old, new), paths: (&target.source_path, &destination), diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs index 02124df9a8..f7f461c5a2 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/unit_test_moves.rs @@ -20,14 +20,10 @@ pub(crate) struct FollowedModel<'a> { pub(crate) paths: (&'a str, &'a str), } -/// Rename `TEST` names of tests that target only the model, and move files holding only them. -/// -/// A file moves only when every macro, enum and constant it uses resolves to the same -/// declaration from the destination folder; otherwise it is renamed in place, with a note. +/// Rename `TEST` names of the model's own tests, and move their files when scopes allow. pub(crate) fn unit_test_parts( project_dir: &Path, - tests: &[UnitTestFacts], - files: &[ProjectSqlFile], + (tests, files): (&[UnitTestFacts], &[ProjectSqlFile]), model: &FollowedModel<'_>, host: &UnitTestScopeHost<'_>, ) -> Result { diff --git a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs index b1874567ce..b82d806464 100644 --- a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs +++ b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs @@ -12,8 +12,7 @@ use crate::refactoring::models::{RefactorFacts, RefactorOperation, RefactorPlan, use crate::refactoring::types::{DeclarationMoveHost, UnitTestScopeHost}; -/// Plan one refactoring without touching files; `host` finds declaration-file moves and -/// `test_host` the declarations that keep a unit test file in its folder. +/// Plan one refactoring without touching files, asking the hosts about declaration scopes. pub fn plan_refactor( facts: &RefactorFacts, request: &RefactorRequest, diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index 2758be1e81..5b8a72e3dd 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -649,13 +649,15 @@ pub(super) fn owner_header_parts( Ok((described(&edits), manual)) } -/// Each planned test file as `original -> path` with the replacements made in it, and each -/// `path: reason` note, when the scope host reports `rebound` for every test file. +/// Planned test file changes with their replacements, and the plan's notes. +type FollowedTests = (Vec<(String, Vec)>, Vec); + +/// Planned test file changes and `path: reason` notes when the scope host reports `rebound`. pub(super) fn followed_unit_tests( request: &RefactorRequest, tests: &[(&str, &str, &[&str])], rebound: &[&str], -) -> Result<(Vec<(String, Vec)>, Vec), RefactorError> { +) -> Result { let mut orders: ModelFacts = model("orders", Some("view"), "analytics"); orders.path = "models/marts/orders.sql".to_owned(); let mut project_facts = facts("/nonexistent/project", vec![orders]); diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs index 821cbacb02..d2f54be8ab 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs @@ -405,8 +405,7 @@ fn given_unit_tests_of_model_when_renaming_or_moving_then_tests_follow() -> Resu }, ]; for test_case in test_cases { - let (changes, notes) = - followed_unit_tests(&test_case.request, tests, test_case.rebound)?; + let (changes, notes) = followed_unit_tests(&test_case.request, tests, test_case.rebound)?; assert_eq!( changes, test_case.expected_changes, "{}", diff --git a/crates/sqlbuild-refactor/src/refactoring/types.rs b/crates/sqlbuild-refactor/src/refactoring/types.rs index e31b53ebb9..37d5259965 100644 --- a/crates/sqlbuild-refactor/src/refactoring/types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/types.rs @@ -7,8 +7,7 @@ use crate::refactoring::models::DeclarationPlacement; pub type DeclarationMoveHost<'a> = dyn Fn(&str, &str) -> Result + 'a; -/// The scoped declarations a unit test file uses that would resolve differently, or not at all, -/// if it moved: `(test path, destination path)`; `None` when that cannot be worked out. +/// Used scoped declarations a `(test path, destination)` move would rebind; `None` if unknown. pub type UnitTestScopeHost<'a> = dyn Fn(&str, &str) -> Result>, RefactorError> + 'a; diff --git a/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs b/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs index d17b3e40e3..f88597be5e 100644 --- a/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs +++ b/crates/sqlbuild-scopes/src/scope_report/_helpers/moves.rs @@ -5,8 +5,8 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use crate::scope_index::main::classify_report_resource::classify_report_resource; use crate::scope_index::main::format_identity::format_identity; use crate::scope_index::models::{ - DeclarationIdentity, DeclarationKind, OwnershipRoot, QualifiedIdentity, ReportClassification, ResourceRecord, - ScopeKind, + DeclarationIdentity, DeclarationKind, OwnershipRoot, QualifiedIdentity, ReportClassification, + ResourceRecord, ScopeKind, }; use crate::scope_report::_helpers::context::Context; use crate::scope_report::_helpers::declarations::declaration_report; @@ -216,7 +216,10 @@ fn scoped_bindings(context: &Context<'_>, values: Visible) -> BTreeMap, values: Visible) -> BTreeMap, (path, destination): (&str, &str), diff --git a/crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs b/crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs index fe76237ca0..efa84e554d 100644 --- a/crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs +++ b/crates/sqlbuild-scopes/src/scope_report/main/rebound_scope_usages.rs @@ -3,8 +3,7 @@ use crate::scope_report::_helpers::moves::rebound_usages; use crate::scope_report::models::{ReportFailure, ReportSource}; -/// Qualified identities of the scoped macros, enums and constants the resources at `path` use -/// that resolve differently from `destination`, or `None` when `destination` cannot hold them. +/// Used scoped declarations that resolve differently from `destination`; `None` if it is invalid. pub fn rebound_scope_usages( source: ReportSource<'_>, paths: (&str, &str), diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py index 4a8f9d1bd0..2c802d8726 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py @@ -156,4 +156,4 @@ class ScopedUnitTestMoveE2ETestCase: extra_files: dict[str, str] amount_sql: str expected_path: str - expected_note: str | None + expected_notes: tuple[str, ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py index 1fe2fc18ec..d536a0f145 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_unit_tests_follow_models.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re import subprocess from pathlib import Path from typing import Any @@ -41,6 +42,14 @@ ).replace(")\nSELECT 1\n", f"),\n{_CUSTOMER_TOTALS_FIXTURE}SELECT 1\n"), } +_SAMPLE_AMOUNT_MACRO: str = 'def sample_amount() -> str:\n return "{amount}"\n' +_MARTS_MACRO: str = "tests/unit/marts/_sqlbuild/_macros/sample_amount.py" +_KEPT_NOTE: str = ( + "└── tests/unit/marts/test_order_facts__keeps_orders.sql kept in its folder: {declaration} " + "would not resolve to the same declaration from tests/unit/reporting" +) +_NOTE_LINE: re.Pattern[str] = re.compile(r"^.*kept in its folder.*$", re.MULTILINE) + @pytest.mark.parametrize( "test_case", @@ -134,10 +143,6 @@ def test_given_dry_run_when_renaming_model_with_unit_test_then_reports_move_and_ assert project_text(project_dir) == before -_SAMPLE_AMOUNT_MACRO: str = 'def sample_amount() -> str:\n return "{amount}"\n' -_MARTS_MACRO: str = "tests/unit/marts/_sqlbuild/_macros/sample_amount.py" - - @pytest.mark.parametrize( "test_case", [ @@ -146,7 +151,7 @@ def test_given_dry_run_when_renaming_model_with_unit_test_then_reports_move_and_ extra_files={_MARTS_MACRO: _SAMPLE_AMOUNT_MACRO.format(amount=25)}, amount_sql="@sample_amount()", expected_path="tests/unit/marts/test_order_facts__keeps_orders.sql", - expected_note="macro:sample_amount", + expected_notes=(_KEPT_NOTE.format(declaration="macro:sample_amount"),), ), ScopedUnitTestMoveE2ETestCase( description="a folder-scoped test constant keeps the test in its folder", @@ -157,7 +162,7 @@ def test_given_dry_run_when_renaming_model_with_unit_test_then_reports_move_and_ }, amount_sql='@const("sample_amount")', expected_path="tests/unit/marts/test_order_facts__keeps_orders.sql", - expected_note="constant:sample_amount", + expected_notes=(_KEPT_NOTE.format(declaration="constant:sample_amount"),), ), ScopedUnitTestMoveE2ETestCase( description="a project-wide constant lets the test move", @@ -170,7 +175,7 @@ def test_given_dry_run_when_renaming_model_with_unit_test_then_reports_move_and_ }, amount_sql='@const("sample_amount")', expected_path="tests/unit/reporting/test_order_facts__keeps_orders.sql", - expected_note=None, + expected_notes=(), ), ], ids=lambda case: case.description, @@ -183,9 +188,8 @@ def test_given_scoped_declarations_when_moving_model_then_test_moves_only_if_the Without SQBRTEST103 selected, a test kept outside the destination's mirrored folder compiles. """ - test_sql: str = ( - _FILES[_MIRRORED_TEST] - .replace("25 AS amount", f"{test_case.amount_sql} AS amount", 1) + test_sql: str = _FILES[_MIRRORED_TEST].replace( + "25 AS amount", f"{test_case.amount_sql} AS amount", 1 ) project_dir: Path = write_orders_project( tmp_path=tmp_path, @@ -203,11 +207,5 @@ def test_given_scoped_declarations_when_moving_model_then_test_moves_only_if_the moved: str = (project_dir / test_case.expected_path).read_text(encoding="utf-8") assert 'TEST (name "order_facts__keeps_orders");' in moved assert test_case.amount_sql in moved - notes: list[str] = [line for line in result.stdout.splitlines() if "kept in its folder" in line] - if test_case.expected_note is None: - assert notes == [], result.stdout - else: - assert len(notes) == 1, result.stdout - assert test_case.expected_path in notes[0] - assert test_case.expected_note in notes[0] + assert tuple(_NOTE_LINE.findall(result.stdout)) == test_case.expected_notes, result.stdout assert tested.returncode == 0, tested.stdout + tested.stderr diff --git a/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py b/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py index a833b20db4..40b5dbbd81 100644 --- a/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py +++ b/tests/unit/src/sqlbuild/runtime/event_exporting/classes/test_dispatcher.py @@ -959,15 +959,19 @@ def flush(self) -> None: def test_given_failing_stderr_when_sink_stalls_then_delivery_and_counts_continue( test_case: EventExporterDispatcherTestCase, monkeypatch: pytest.MonkeyPatch ) -> None: - release: threading.Event = threading.Event() - slow_started: threading.Event = threading.Event() + event_ids: tuple[str, ...] = tuple(f"event-{index}" for index in range(1, 5)) + started: dict[str, threading.Event] = {event_id: threading.Event() for event_id in event_ids} + gates: dict[str, threading.Event] = {event_id: threading.Event() for event_id in event_ids} + for event_id in event_ids[1:]: + gates[event_id].set() + slow_started: threading.Event = started["event-1"] + release: threading.Event = gates["event-1"] delivered: list[str] = [] def record(*, event: LifecycleEvent) -> None: delivered.append(event.event_id) - if event.event_id == "event-1": - slow_started.set() - _ = release.wait() + started[event.event_id].set() + _ = gates[event.event_id].wait() monkeypatch.setattr(sys, "stderr", _BrokenStream()) dispatcher: EventExporterDispatcher = EventExporterDispatcher( From c78fbc1b02f8f649e8ea3e18588b4aee250d348a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 19:06:49 +0100 Subject: [PATCH 15/15] test: expect escaped errors for undecodable selectors Two integration tests pinned the previous crash (a TypeError) for a selector with undecodable bytes. Expected CLI errors now print that text escaped, so the tests assert the escaped S007 error and a valid ASCII JSON report. --- .../main/compile_outputs/test_native_json_report.py | 11 ++++++----- .../src/sqlbuild/cli/recall/test_local_recall.py | 8 +++++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/tests/integration/src/sqlbuild/cli/commands/main/compile_outputs/test_native_json_report.py b/tests/integration/src/sqlbuild/cli/commands/main/compile_outputs/test_native_json_report.py index 529e6cd6f0..9b92118407 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/compile_outputs/test_native_json_report.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/compile_outputs/test_native_json_report.py @@ -190,12 +190,12 @@ def test_given_lone_surrogate_in_full_report_when_compiling_json_then_native_wri CliJsonReportTestCase( description="selector with undecodable bytes", args=("--no-cache", "--select", SURROGATE_SELECTOR), - expected_fragment="str is not valid UTF-8: surrogates not allowed", + expected_fragment="unknown selector name 'orders\\\\xff'", ) ], ids=lambda case: case.description, ) -def test_given_lone_surrogate_in_error_report_when_compiling_json_then_orjson_error_propagates( +def test_given_lone_surrogate_in_error_report_when_compiling_json_then_writes_escaped_json( test_case: CliJsonReportTestCase, tmp_path: Path, capsys: pytest.CaptureFixture[str], @@ -206,11 +206,12 @@ def test_given_lone_surrogate_in_error_report_when_compiling_json_then_orjson_er write_files(project_dir=tmp_path, files=SURROGATE_HOOK_FILES) prepare_output_compiles(monkeypatch=monkeypatch, reuse_disabled="1") - with pytest.raises(TypeError) as raised: - _ = compile_json_text(project_dir=tmp_path, args=test_case.args, capsys=capsys) + text: str = compile_json_text(project_dir=tmp_path, args=test_case.args, capsys=capsys) _ = capsys.readouterr() - assert str(raised.value) == test_case.expected_fragment + assert test_case.expected_fragment in text + assert text.isascii() + assert text == json.dumps(json.loads(text), indent=2) + "\n" @pytest.mark.parametrize( diff --git a/tests/integration/src/sqlbuild/cli/recall/test_local_recall.py b/tests/integration/src/sqlbuild/cli/recall/test_local_recall.py index 9677a44ee9..802145907f 100644 --- a/tests/integration/src/sqlbuild/cli/recall/test_local_recall.py +++ b/tests/integration/src/sqlbuild/cli/recall/test_local_recall.py @@ -830,15 +830,17 @@ def test_given_an_invalid_output_limit_when_running_then_the_command_fails_with_ "test_case", [ UndecodableArgumentTestCase( - description="an undecodable selector is recorded escaped; the command fails as before", + description="an undecodable selector is recorded escaped; the error prints it escaped", selector="orders\udcff", - expected_error=b"TypeError: str is not valid UTF-8: surrogates not allowed", + expected_error=( + b"error[S007]: unknown selector name 'orders\\xff'\n = help: did you mean 'orders'?" + ), expected_recorded=[['["orders\\\\xff"]', 1]], ), ], ids=lambda case: case.description, ) -def test_given_an_undecodable_argument_when_recorded_then_it_is_escaped_and_the_command_unchanged( +def test_given_an_undecodable_argument_when_recorded_then_it_is_escaped_in_history_and_error( test_case: UndecodableArgumentTestCase, tmp_path: Path ) -> None: project_dir: Path = write_project(tmp_path / "orders_shop")