From a0ae63057608870d9ce66f2f2c81708c7cad2f7e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:38:51 +0100 Subject: [PATCH 01/14] fix(rename): rename contract-declared columns and audit arguments with the column A MODEL columns block without commas between entries hid every entry after the first from sqb rename, so the model's own declaration was left for a hand edit that the next run then refused because the project no longer compiled. Column entries now split where the compiler splits them. String arguments of attached audits are renamed only when the generic audit's SQL definition splices them in as SQL (`@where_condition`), and only where they reference the audited model: a subquery reading another model keeps that model's column, and one reading the audited model is renamed too. Arguments the audit quotes (`@'prefix'`), arguments of audits with no SQL definition, unparseable ones and ones reading relations of unknown columns are listed for a hand edit. Refs: CHI-640 --- .../_helpers/edits/header_edits.rs | 105 ++++++++++- .../_helpers/planning/audit_arguments.rs | 75 ++++++++ .../_helpers/planning/column_planning.rs | 163 +++++++++++++++--- .../_helpers/planning/column_references.rs | 57 ++++++ .../src/refactoring/_helpers/planning/mod.rs | 1 + .../src/refactoring/constants.rs | 12 ++ .../src/refactoring/tests/helpers.rs | 65 ++++++- .../refactoring/tests/test_header_edits.rs | 114 +++++++++++- .../src/refactoring/tests/test_types.rs | 7 + .../sqlbuild/references/docs/cli/rename.md | 3 +- .../cli/commands/main/refactor/helpers.py | 75 ++++++++ .../main/refactor/test_rename_columns.py | 59 +++++++ website/src/content/docs/docs/cli/rename.mdx | 3 +- 13 files changed, 706 insertions(+), 33 deletions(-) create mode 100644 crates/sqlbuild-refactor/src/refactoring/_helpers/planning/audit_arguments.rs 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 56f0c50fdc..ef6ec15a1f 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/header_edits.rs @@ -1,6 +1,7 @@ //! Token-exact MODEL and SCHEMA header edits, as `header_edits.py` and `schema_edits.py` make. use sqlbuild_core::text::main::is_word_char::is_word_char; +use sqlbuild_model_config::header_metadata::constants::AUDIT_OPTION_KEYS; use sqlbuild_sqltext::compiler::main::model_header_matching::match_batch; use sqlbuild_sqltext::compiler::main::model_header_tokenizing::tokenize_one; use sqlbuild_sqltext::compiler::main::statement_header_matching::match_statement_header; @@ -9,10 +10,11 @@ use crate::refactoring::_helpers::edits::text_edits::{text_edit, whole_word_offs use crate::refactoring::_helpers::scanning::chars::{find, rfind, slice, starts_with}; use crate::refactoring::_helpers::scanning::sql_sites::embedded_ref_spans; use crate::refactoring::constants::{ - COLUMN_VALUED_CONFIG_KEYS, COLUMNS_KEY, CURSOR_INPUTS_KEY, HEADER_CLOSERS, + AUDITS_KEY, COLUMN_VALUED_CONFIG_KEYS, COLUMNS_KEY, CURSOR_INPUTS_KEY, HEADER_CLOSERS, HEADER_DESCRIPTION_KEY, HEADER_INDENT, HEADER_KEY_AND_VALUE_TOKENS, HEADER_OPEN_PAREN, - HEADER_OPENERS, HEADER_SEPARATOR, MIGRATE_FROM_KEY, PARENTHESIZED_EMPTY_TOKENS, REF_FUNCTION, - RELATIONSHIPS_AUDIT, RELATIONSHIPS_FIELD_KEY, RELATIONSHIPS_TO_KEY, SCHEMA_KEYWORD, + HEADER_OPENERS, HEADER_SEPARATOR, LIST_OPENER, MIGRATE_FROM_KEY, PARENTHESIZED_EMPTY_TOKENS, + REF_FUNCTION, RELATIONSHIPS_AUDIT, RELATIONSHIPS_FIELD_KEY, RELATIONSHIPS_TO_KEY, + SCHEMA_KEYWORD, }; use crate::refactoring::errors::RefactorError; use crate::refactoring::models::{EditKind, TextEdit}; @@ -159,6 +161,40 @@ fn nested_entries<'a>(tokens: &[&'a HeaderToken], depth: i64) -> Vec(tokens: &[&'a HeaderToken]) -> Vec> { + let mut groups: Vec> = vec![Vec::new()]; + let mut previous: Option<&HeaderToken> = None; + for token in tokens { + if token.depth < 1 { + continue; + } + let closed_entry = token.depth == 1 + && is_value_kind(token.kind) + && previous.is_some_and(|item| { + item.depth == 1 + && item.kind == HeaderTokenKind::Symbol + && HEADER_CLOSERS.contains(&item.value.as_str()) + }); + previous = Some(token); + if token.depth == 1 && is_separator(token) { + groups.push(Vec::new()); + continue; + } + if closed_entry { + groups.push(Vec::new()); + } + if let Some(group) = groups.last_mut() { + group.push(token); + } + } + groups + .into_iter() + .filter(|group| group.first().is_some_and(|first| is_value_kind(first.kind))) + .map(|group| (group[0], group[1..].to_vec())) + .collect() +} + /// Top-level `key value` entries keyed by their first word. fn header_entries(tokens: &[HeaderToken]) -> Vec<(String, Vec<&HeaderToken>)> { let all: Vec<&HeaderToken> = tokens.iter().collect(); @@ -298,7 +334,7 @@ pub(crate) fn column_entry_edits( if key != COLUMNS_KEY { continue; } - for (name_token, values) in nested_entries(&entry[1..], 1) { + for (name_token, values) in column_entries(&entry[1..]) { if !is_value(name_token, old) { continue; } @@ -383,6 +419,67 @@ pub(crate) fn insert_header_entry_edit( ) } +/// One keyed string argument of an attached audit instance. +pub(crate) struct AuditArgument<'a> { + pub(crate) token: &'a HeaderToken, + /// The generic audit the instance attaches, such as `accepted_values`. + pub(crate) audit: String, + pub(crate) key: String, +} + +/// Keyed string arguments of `audits [...]` instances, with their audit and argument names. +pub(crate) fn audit_argument_strings(tokens: &[HeaderToken]) -> Vec> { + let mut found: Vec> = Vec::new(); + let mut audit_depths: Vec = Vec::new(); + let mut instance: String = String::new(); + for (index, token) in tokens.iter().enumerate() { + let list_opener = index > 0 && tokens[index - 1].value == AUDITS_KEY; + while !list_opener + && audit_depths + .last() + .is_some_and(|depth| token.depth < *depth) + { + audit_depths.pop(); + } + let opens_audits = token.kind == HeaderTokenKind::Word + && token.value == AUDITS_KEY + && tokens.get(index + 1).is_some_and(|next| { + next.kind == HeaderTokenKind::Symbol && next.value == LIST_OPENER + }); + if opens_audits { + audit_depths.push(token.depth + 1); + continue; + } + if token.kind == HeaderTokenKind::Word && audit_depths.last() == Some(&token.depth) { + instance.clone_from(&token.value); + } + let preceding = index + .checked_sub(HEADER_KEY_AND_VALUE_TOKENS) + .map(|before| (&tokens[before], &tokens[before + 1])); + let Some((before_key, key)) = preceding else { + continue; + }; + if audit_depths.is_empty() || token.kind != HeaderTokenKind::String { + continue; + } + let keyed = key.kind == HeaderTokenKind::Word + && key.depth == token.depth + && before_key.kind == HeaderTokenKind::Symbol + && (before_key.value == HEADER_OPEN_PAREN || before_key.value == HEADER_SEPARATOR); + let option = AUDIT_OPTION_KEYS.contains(&key.value.as_str()) + || key.value == RELATIONSHIPS_TO_KEY + || key.value == RELATIONSHIPS_FIELD_KEY; + if keyed && !option { + found.push(AuditArgument { + token, + audit: instance.clone(), + key: key.value.clone(), + }); + } + } + found +} + /// Header offsets that still mention a word outside descriptions. pub(crate) fn unhandled_word_offsets( text: &[char], diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/audit_arguments.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/audit_arguments.rs new file mode 100644 index 0000000000..cc6cd98a64 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/audit_arguments.rs @@ -0,0 +1,75 @@ +//! How a generic audit's SQL uses one argument: spliced in as SQL, quoted, or unknown. + +use std::path::Path; + +use crate::refactoring::_helpers::scanning::chars::chars; +use crate::refactoring::constants::{ + AUDIT_PARAMETER_SIGIL, GENERIC_AUDIT_DIRECTORY, SQL_STRING_QUOTE, +}; +use crate::refactoring::models::{AuthoredFile, SqlFileRole}; + +/// How every definition of a generic audit renders one argument. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum ArgumentUsage { + /// Every definition splices the argument in as SQL with `@name`. + Sql, + /// A definition quotes the argument as a string literal with `@'name'`. + Literal, + /// No project SQL definition shows how the argument is used. + Unknown, +} + +/// How the project's SQL definitions of `audit` render argument `key`. +pub(crate) fn argument_usage(files: &[AuthoredFile], audit: &str, key: &str) -> ArgumentUsage { + let definitions: Vec<&AuthoredFile> = files + .iter() + .filter(|file| file.role == SqlFileRole::Audit && defines_generic_audit(&file.path, audit)) + .collect(); + let mut usage = ArgumentUsage::Unknown; + for definition in definitions { + let (spliced, quoted) = parameter_uses(&chars(&definition.contents), key); + if quoted { + return ArgumentUsage::Literal; + } + if !spliced { + return ArgumentUsage::Unknown; + } + usage = ArgumentUsage::Sql; + } + usage +} + +/// Whether `path` is a generic audit definition file named `audit`. +fn defines_generic_audit(path: &str, audit: &str) -> bool { + let path = Path::new(path); + path.file_stem().is_some_and(|stem| stem == audit) + && path + .parent() + .and_then(Path::file_name) + .is_some_and(|directory| directory == GENERIC_AUDIT_DIRECTORY) +} + +/// Whether the SQL uses `@key` unquoted and whether it uses `@'key'`. +fn parameter_uses(text: &[char], key: &str) -> (bool, bool) { + let key: Vec = chars(key); + let mut spliced = false; + let mut quoted = false; + for (index, character) in text.iter().enumerate() { + if *character != AUDIT_PARAMETER_SIGIL { + continue; + } + let rest = &text[index + 1..]; + if rest.starts_with(&key) && !rest.get(key.len()).is_some_and(|next| is_word(*next)) { + spliced = true; + } + let quoted_name = rest.first() == Some(&SQL_STRING_QUOTE) + && rest[1..].starts_with(&key) + && rest.get(key.len() + 1) == Some(&SQL_STRING_QUOTE); + quoted = quoted || quoted_name; + } + (spliced, quoted) +} + +fn is_word(character: char) -> bool { + character.is_alphanumeric() || character == '_' +} diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_planning.rs index 0a15a1168a..c11fe3cd76 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_planning.rs @@ -3,29 +3,31 @@ use std::collections::{BTreeSet, HashSet, VecDeque}; use crate::refactoring::_helpers::edits::header_edits::{ - add_column_entry_edit, column_config_edits, column_entry_edits, consumer_column_header_edits, - header_tokens, schema_column_edits, unhandled_word_offsets, + HeaderToken, add_column_entry_edit, audit_argument_strings, column_config_edits, + column_entry_edits, consumer_column_header_edits, header_tokens, schema_column_edits, + unhandled_word_offsets, }; use crate::refactoring::_helpers::edits::text_edits::{ - RefactorParts, manual_at, merge_parts, path_edits, + RefactorParts, manual_at, merge_parts, path_edits, whole_word_offsets, }; use crate::refactoring::_helpers::edits::yaml_edits::yaml_column_edits; use crate::refactoring::_helpers::files::project_files::{ AuthoredBody, ProjectSqlFile, authored_bodies, project_sql_files, yaml_files, }; +use crate::refactoring::_helpers::planning::audit_arguments::{ArgumentUsage, argument_usage}; use crate::refactoring::_helpers::planning::column_references::{ - BodyContext, BodyMapping, ColumnQuery, ResourceColumns, analyze_column, columns_of, - consumer_edits, output_edits, resource_columns, + BodyContext, BodyEdits, BodyMapping, ColumnFacts, ColumnQuery, ExpressionSite, ResourceColumns, + analyze_column, columns_of, consumer_edits, expression_edits, output_edits, resource_columns, }; use crate::refactoring::_helpers::planning::model_planning::{ find_model, needs_column_migration, validate_identifier, }; -use crate::refactoring::_helpers::scanning::chars::chars; +use crate::refactoring::_helpers::scanning::chars::{chars, slice}; use crate::refactoring::_helpers::scanning::scan_context::ScanContext; -use crate::refactoring::_helpers::scanning::sql_sites::{analysis_sql, model_body}; +use crate::refactoring::_helpers::scanning::sql_sites::{AnalysisSql, analysis_sql, model_body}; use crate::refactoring::constants::{ - EXPECTED_FIXTURE_PREFIX, MIGRATE_FROM_KEY, MODEL_RESOURCE_TYPE, REF_FIXTURE_PREFIX, REF_KIND, - ROOT_SCOPE, + AUDIT_ARGUMENT_TABLE, ESCAPE_CHARACTER, EXPECTED_FIXTURE_PREFIX, MIGRATE_FROM_KEY, + MODEL_RESOURCE_TYPE, REF_FIXTURE_PREFIX, REF_KIND, ROOT_SCOPE, }; use crate::refactoring::errors::RefactorError; use crate::refactoring::models::{ @@ -272,7 +274,7 @@ fn declarations( ])) } -fn header_parts( +pub(crate) fn header_parts( context: &ColumnRenameContext<'_>, model: &ModelFacts, ) -> Result { @@ -293,18 +295,23 @@ fn header_parts( let mut edits = column_config_edits(&text, &tokens, old, new); edits.extend(entry_edits.unwrap_or_default()); edits.extend(added); - let handled: Vec = edits.iter().map(|edit| edit.start).collect(); - let manual: Vec = unhandled_word_offsets(&text, &tokens, old, &handled) - .into_iter() - .map(|offset| { - manual_at( - path, - &text, - Some(offset), - format!("the MODEL header mentions {old} here; update it by hand"), - ) - }) - .collect(); + let mut handled: Vec = edits.iter().map(|edit| edit.start).collect(); + let arguments = audit_argument_parts(context, model, (path, &text), &tokens)?; + handled.extend(arguments.handled); + edits.extend(arguments.edits.edits); + let mut manual: Vec = arguments.edits.manual; + manual.extend( + unhandled_word_offsets(&text, &tokens, old, &handled) + .into_iter() + .map(|offset| { + manual_at( + path, + &text, + Some(offset), + format!("the MODEL header mentions {old} here; update it by hand"), + ) + }), + ); let migrations = if migrate { vec![MigrationDeclaration { model_name: model.name.clone(), @@ -322,6 +329,118 @@ fn header_parts( }) } +/// Audit argument edits and the header string tokens they settle. +struct ArgumentParts { + edits: BodyEdits, + handled: Vec, +} + +/// Rename the column in audit string arguments the audit's SQL splices in as expressions. +fn audit_argument_parts( + context: &ColumnRenameContext<'_>, + model: &ModelFacts, + file: (&str, &[char]), + tokens: &[HeaderToken], +) -> Result { + let (path, text) = file; + let (old, _) = context.names(); + let mut parts = ArgumentParts { + edits: BodyEdits::default(), + handled: Vec::new(), + }; + for argument in audit_argument_strings(tokens) { + let token = argument.token; + let expression = slice(text, token.start + 1, token.end.saturating_sub(1)); + let mentions = whole_word_offsets(&chars(&expression), old); + if mentions.is_empty() { + continue; + } + parts.handled.push(token.start); + let usage = argument_usage( + &context.facts.authored_files, + &argument.audit, + &argument.key, + ); + if usage != ArgumentUsage::Sql { + let reason = non_sql_argument_reason(old, &argument.audit, &argument.key, usage); + parts + .edits + .manual + .extend(mentions.into_iter().map(|offset| { + manual_at(path, text, Some(token.start + 1 + offset), reason.clone()) + })); + continue; + } + let prefix = format!("SELECT * FROM {AUDIT_ARGUMENT_TABLE} WHERE "); + let site = ExpressionSite { + path, + contents: text, + prefix: prefix.chars().count(), + offset: token.start + 1, + }; + let rendered: AnalysisSql = analysis_sql(&chars(&expression), context.scan); + let unknown_relation = rendered + .tables + .iter() + .find(|((kind, name), _)| columns_of(&context.columns, kind, name).is_empty()) + .map(|((_, name), _)| name.clone()); + if let Some(relation) = unknown_relation { + let reason = format!( + "this audit argument reads {relation}, whose columns SQLBuild does not know, so {old} here is not renamed; update it by hand" + ); + parts + .edits + .manual + .extend(mentions.into_iter().map(|offset| { + manual_at(path, text, Some(token.start + 1 + offset), reason.clone()) + })); + continue; + } + let mut target_tables: BTreeSet = rendered.placeholders(REF_KIND, &model.name); + target_tables.insert(AUDIT_ARGUMENT_TABLE.to_owned()); + let mut tables = vec![( + (REF_KIND.to_owned(), model.name.clone()), + BTreeSet::from([AUDIT_ARGUMENT_TABLE.to_owned()]), + )]; + tables.extend(rendered.tables); + let analysis = AnalysisSql { + sql: format!("{prefix}{}", rendered.sql), + tables, + }; + let analysed = if expression.contains(ESCAPE_CHARACTER) { + ColumnFacts::default() + } else { + analyze_column( + &analysis, + &context.facts.dialect, + &context.columns, + &ColumnQuery { + column: old, + target_tables, + target_ctes: BTreeSet::new(), + output_scopes: BTreeSet::new(), + }, + )? + }; + let result = expression_edits(&analysed, &site, context.names()); + parts.edits.edits.extend(result.edits); + parts.edits.manual.extend(result.manual); + } + Ok(parts) +} + +/// Why an audit argument that is not known to be SQL is left for a hand edit. +fn non_sql_argument_reason(old: &str, audit: &str, key: &str, usage: ArgumentUsage) -> String { + match usage { + ArgumentUsage::Literal => format!( + "audit {audit} quotes argument {key} as a string, so {old} here is not renamed; update it by hand if it names the column" + ), + _ => format!( + "no SQL definition of audit {audit} shows whether argument {key} is SQL, so {old} here is not renamed; update it by hand if it names the column" + ), + } +} + fn consumers<'a>( context: &ColumnRenameContext<'a>, model: &ModelFacts, 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 38e6ad1a97..af00f40706 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_references.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/column_references.rs @@ -426,6 +426,63 @@ pub(crate) fn output_edits( result } +/// Where one SQL expression authored inside a header string sits in its analysis query and file. +pub(crate) struct ExpressionSite<'a> { + pub(crate) path: &'a str, + pub(crate) contents: &'a [char], + /// The length of the analysis query text before the expression. + pub(crate) prefix: usize, + /// The file offset of the expression's first character. + pub(crate) offset: usize, +} + +/// Rename every reference to the column in one analysed header-string expression. +pub(crate) fn expression_edits( + facts: &ColumnFacts, + site: &ExpressionSite<'_>, + names: (&str, &str), +) -> BodyEdits { + let (old, new) = names; + let locate = |start: usize| { + start + .checked_sub(site.prefix) + .map(|start| site.offset + start) + }; + if !facts.parsed { + return BodyEdits::manual(manual_at( + site.path, + site.contents, + Some(site.offset), + format!( + "this audit argument mentions {old} but is not a SQL expression SQLBuild can parse; update it by hand" + ), + )); + } + let mut result = BodyEdits::default(); + for reference in &facts.references { + let (Some(start), Some(end)) = (locate(reference.name_start), locate(reference.name_end)) + else { + continue; + }; + let authored = slice(site.contents, start, end); + result.edits.push(text_edit( + site.contents, + (start, end), + respell(&authored, new), + EditKind::Header, + )); + } + for unresolved in &facts.unresolved { + result.manual.push(manual_at( + site.path, + site.contents, + unresolved.start.and_then(locate).or(Some(site.offset)), + format!("cannot tell whether {old} in this audit argument is the renamed column; update it by hand"), + )); + } + result +} + fn scope_role(scope: &str) -> String { if scope == ROOT_SCOPE { scope.to_owned() diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs index 0554329300..3817d5ef15 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod audit_arguments; pub(crate) mod column_planning; pub(crate) mod column_references; pub(crate) mod declaration_moves; diff --git a/crates/sqlbuild-refactor/src/refactoring/constants.rs b/crates/sqlbuild-refactor/src/refactoring/constants.rs index e60ecdb0ad..84645fd5f6 100644 --- a/crates/sqlbuild-refactor/src/refactoring/constants.rs +++ b/crates/sqlbuild-refactor/src/refactoring/constants.rs @@ -13,6 +13,17 @@ pub const CTE_SCOPE_PREFIX: &str = "cte:"; pub const MIGRATE_FROM_KEY: &str = "migrate_from"; pub const CURSOR_INPUTS_KEY: &str = "cursor_inputs"; pub const COLUMNS_KEY: &str = "columns"; +pub const AUDITS_KEY: &str = "audits"; +/// The relation an audit string argument is analysed against, standing for the audited model. +pub const AUDIT_ARGUMENT_TABLE: &str = "__sqb_audit_target__"; +/// The escape character of quoted MODEL header values. +pub const ESCAPE_CHARACTER: char = '\\'; +/// The quote of SQL string literals. +pub const SQL_STRING_QUOTE: char = '\''; +/// The character that starts a generic audit parameter such as `@expression`. +pub const AUDIT_PARAMETER_SIGIL: char = '@'; +/// The directory holding generic audit definitions, as in `audits/generic/`. +pub const GENERIC_AUDIT_DIRECTORY: &str = "generic"; pub const RELATIONSHIPS_AUDIT: &str = "relationships"; pub const RELATIONSHIPS_TO_KEY: &str = "to"; pub const RELATIONSHIPS_FIELD_KEY: &str = "field"; @@ -39,6 +50,7 @@ pub const HEADER_DESCRIPTION_KEY: &str = "description"; pub const HEADER_INDENT: &str = " "; pub const HEADER_SEPARATOR: &str = ","; pub const HEADER_OPEN_PAREN: &str = "("; +pub const LIST_OPENER: &str = "["; pub const HEADER_OPENERS: [&str; 3] = ["(", "[", "{"]; pub const HEADER_CLOSERS: [&str; 3] = [")", "]", "}"]; pub const PARENTHESIZED_EMPTY_TOKENS: usize = 2; diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index dac3d1fcc5..7f01e62d17 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -12,6 +12,9 @@ use crate::refactoring::_helpers::edits::header_edits::{ use crate::refactoring::_helpers::edits::text_edits::{apply_text_edits, file_changes, text_edit}; use crate::refactoring::_helpers::edits::yaml_edits::{yaml_column_edits, yaml_model_edits}; use crate::refactoring::_helpers::files::workspace::commit_changes; +use crate::refactoring::_helpers::planning::column_planning::{ + column_rename_context, header_parts, +}; use crate::refactoring::_helpers::planning::column_references::{ BodyContext, BodyMapping, ColumnQuery, ResourceColumns, analyze_column, consumer_edits, }; @@ -22,9 +25,9 @@ use crate::refactoring::_helpers::scanning::scan_context::ScanContext; use crate::refactoring::_helpers::scanning::sql_sites::{ModelBody, analysis_sql, resource_sites}; use crate::refactoring::errors::{RefactorError, RefactorErrorKind}; use crate::refactoring::models::{ - DeclarationMoves, DeclarationPlacement, DestinationFacts, DiscoveredFile, EditKind, - ExpansionSpan, FileChange, ModelFacts, PlacedDeclaration, RefactorFacts, RefactorOperation, - RefactorRequest, RelocatedDeclaration, TextEdit, + AuthoredFile, DeclarationMoves, DeclarationPlacement, DestinationFacts, DiscoveredFile, + EditKind, ExpansionSpan, FileChange, ModelFacts, PlacedDeclaration, RefactorFacts, + RefactorOperation, RefactorRequest, RelocatedDeclaration, SqlFileRole, TextEdit, }; use crate::refactoring::tests::test_types::{ExpectedSpans, PlannedMoves, Spans}; @@ -587,3 +590,59 @@ pub(crate) fn physical_columns(columns: &[(&str, &str)]) -> Vec<(String, String) .map(|(name, column_type)| ((*name).to_owned(), (*column_type).to_owned())) .collect() } + +/// Header edits and manual locations of renaming `orders.is_flagged` to `is_marked`. +pub(super) fn owner_header_parts( + contents: &str, +) -> Result<(Vec, Vec), RefactorError> { + let mut orders = model("orders", Some("table"), "analytics"); + orders.inferred_columns = vec!["order_id".to_owned(), "is_flagged".to_owned()]; + let mut vip_flags = model("vip_flags", Some("table"), "analytics"); + vip_flags.inferred_columns = vec!["is_flagged".to_owned()]; + let unknown_flags = model("unknown_flags", Some("table"), "analytics"); + let mut project_facts = facts( + "/nonexistent/project", + vec![orders.clone(), vip_flags, unknown_flags], + ); + project_facts.model_files = vec![DiscoveredFile { + path: orders.path.clone(), + contents: contents.to_owned(), + }]; + project_facts.authored_files = [ + ( + "flag_check", + "SELECT * FROM @relation WHERE NOT (@where_condition)", + ), + ( + "prefix_check", + "SELECT * FROM @relation WHERE @'prefix' = ''", + ), + ] + .into_iter() + .map(|(name, body)| AuthoredFile { + role: SqlFileRole::Audit, + path: format!("audits/generic/{name}.sql"), + contents: format!("AUDIT ();\n{body}\n"), + texts: vec![body.to_owned()], + }) + .collect(); + let mut rename = request(RefactorOperation::RenameColumn, "is_marked", None); + rename.column_name = Some("is_flagged".to_owned()); + let scan = context("duckdb"); + let rename_context = column_rename_context(&project_facts, &rename, &scan)?; + let parts = header_parts(&rename_context, &orders)?; + let edits: Vec = parts.edits.into_iter().map(|(_, edit)| edit).collect(); + let manual = parts + .manual + .into_iter() + .map(|item| { + format!( + "{}:{} {}", + item.line.unwrap_or_default(), + item.column.unwrap_or_default(), + item.reason + ) + }) + .collect(); + Ok((described(&edits), manual)) +} diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_header_edits.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_header_edits.rs index d87bb0cb0d..5c5f7923ce 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_header_edits.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_header_edits.rs @@ -1,9 +1,11 @@ use crate::refactoring::errors::RefactorError; use crate::refactoring::tests::helpers::{ applied, consumer_model_edits, described, inserted_entry, owner_column_edits, - schema_field_edits, schema_model_edits, + owner_header_parts, schema_field_edits, schema_model_edits, +}; +use crate::refactoring::tests::test_types::{ + HeaderEditsTestCase, HeaderPartsTestCase, InsertEntryTestCase, }; -use crate::refactoring::tests::test_types::{HeaderEditsTestCase, InsertEntryTestCase}; const CONSUMER: &str = "MODEL (\n description 'Reads __ref(\"stg_orders\") rows.',\n cursor_inputs (stg_orders (order_date)),\n relationships (to stg_orders, field order_id),\n materialized incremental,\n);\n\nSELECT 1\n"; const OWNER: &str = "MODEL (\n materialized incremental,\n unique_key order_id,\n columns (\n order_id (description 'Order key.'),\n amount,\n ),\n);\n\nSELECT 1\n"; @@ -34,6 +36,16 @@ fn given_headers_when_renaming_then_token_exact_edits_are_planned() -> Result<() ], expected_contents: "MODEL (\n materialized incremental,\n unique_key order_key,\n columns (\n order_key (migrate_from order_id, description 'Order key.'),\n amount,\n ),\n);\n\nSELECT 1\n", }, + HeaderEditsTestCase { + description: "newline-separated columns entries without commas are found", + contents: "MODEL (\n materialized incremental,\n columns (\n amount (type INT)\n order_id (type VARCHAR)\n ),\n);\n\nSELECT 1\n", + plan: owner_column_edits, + expected_edits: &[ + "5:5 header \"order_id\" -> \"order_key\"", + "5:15 migration \"\" -> \"migrate_from order_id\"", + ], + expected_contents: "MODEL (\n materialized incremental,\n columns (\n amount (type INT)\n order_key (migrate_from order_id, type VARCHAR)\n ),\n);\n\nSELECT 1\n", + }, HeaderEditsTestCase { description: "a SCHEMA header target is renamed; a longer keyword is not a SCHEMA header", contents: SCHEMA_FILE, @@ -106,3 +118,101 @@ fn given_header_layouts_when_inserting_entry_then_python_spacing_is_kept() } Ok(()) } + +#[test] +fn given_owner_header_when_renaming_column_then_declarations_and_audit_arguments_follow() +-> Result<(), RefactorError> { + let test_cases = [ + HeaderPartsTestCase { + description: "a comma-less column declaration is renamed instead of left for a hand edit", + contents: "MODEL (\n columns (\n order_id (type VARCHAR)\n is_flagged (type BOOLEAN)\n ),\n);\nSELECT 1\n", + expected_edits: &["4:5 header \"is_flagged\" -> \"is_marked\""], + expected_manual: &[], + }, + HeaderPartsTestCase { + description: "column and model audit arguments rename column references, not string literals", + contents: "MODEL (\n columns (\n is_flagged (audits [flag_check (where_condition \"is_flagged AND 'is_flagged' <> ''\")]),\n ),\n audits [flag_check (name \"orders_flag\", where_condition \"NOT is_flagged\")],\n);\nSELECT 1\n", + expected_edits: &[ + "3:5 header \"is_flagged\" -> \"is_marked\"", + "3:54 header \"is_flagged\" -> \"is_marked\"", + "5:64 header \"is_flagged\" -> \"is_marked\"", + ], + expected_manual: &[], + }, + HeaderPartsTestCase { + description: "an audit argument that is not a parseable expression is listed for a hand edit", + contents: "MODEL (\n audits [flag_check (where_condition \"is_flagged ((\")],\n);\nSELECT 1\n", + expected_edits: &[], + expected_manual: &[ + "2:40 this audit argument mentions is_flagged but is not a SQL expression SQLBuild can parse; update it by hand", + ], + }, + HeaderPartsTestCase { + description: "a qualified reference the analysis cannot resolve is listed for a hand edit", + contents: "MODEL (\n audits [flag_check (where_condition \"NOT orders.is_flagged\")],\n);\nSELECT 1\n", + expected_edits: &[], + expected_manual: &[ + "2:44 cannot tell whether is_flagged in this audit argument is the renamed column; update it by hand", + ], + }, + HeaderPartsTestCase { + description: "a subquery reading another model keeps its own column", + contents: "MODEL (\n audits [flag_check (where_condition 'is_flagged IN (SELECT is_flagged FROM __ref(\"vip_flags\"))')],\n);\nSELECT 1\n", + expected_edits: &["2:40 header \"is_flagged\" -> \"is_marked\""], + expected_manual: &[], + }, + HeaderPartsTestCase { + description: "a subquery reading the audited model renames its column too", + contents: "MODEL (\n audits [flag_check (where_condition 'is_flagged IN (SELECT is_flagged FROM __ref(\"orders\"))')],\n);\nSELECT 1\n", + expected_edits: &[ + "2:40 header \"is_flagged\" -> \"is_marked\"", + "2:62 header \"is_flagged\" -> \"is_marked\"", + ], + expected_manual: &[], + }, + HeaderPartsTestCase { + description: "a subquery reading a model of unknown columns is not renamed", + contents: "MODEL (\n audits [flag_check (where_condition 'is_flagged IN (SELECT is_flagged FROM __ref(\"unknown_flags\"))')],\n);\nSELECT 1\n", + expected_edits: &[], + expected_manual: &[ + "2:40 this audit argument reads unknown_flags, whose columns SQLBuild does not know, so is_flagged here is not renamed; update it by hand", + "2:62 this audit argument reads unknown_flags, whose columns SQLBuild does not know, so is_flagged here is not renamed; update it by hand", + ], + }, + HeaderPartsTestCase { + description: "an argument the audit quotes as a string is listed, not renamed", + contents: "MODEL (\n audits [prefix_check (prefix \"is_flagged\")],\n);\nSELECT 1\n", + expected_edits: &[], + expected_manual: &[ + "2:33 audit prefix_check quotes argument prefix as a string, so is_flagged here is not renamed; update it by hand if it names the column", + ], + }, + HeaderPartsTestCase { + description: "an argument of an audit with no SQL definition is listed, not renamed", + contents: "MODEL (\n audits [python_check (where_condition \"NOT is_flagged\")],\n);\nSELECT 1\n", + expected_edits: &[], + expected_manual: &[ + "2:46 no SQL definition of audit python_check shows whether argument where_condition is SQL, so is_flagged here is not renamed; update it by hand if it names the column", + ], + }, + HeaderPartsTestCase { + description: "audit option values and list literals are not audit arguments", + contents: "MODEL (\n audits [accepted_values (name \"is_flagged\", values [\"is_flagged\"])],\n);\nSELECT 1\n", + expected_edits: &[], + expected_manual: &[ + "2:34 the MODEL header mentions is_flagged here; update it by hand", + "2:56 the MODEL header mentions is_flagged here; update it by hand", + ], + }, + ]; + for test_case in test_cases { + let (edits, manual) = owner_header_parts(test_case.contents)?; + assert_eq!(edits, test_case.expected_edits, "{}", test_case.description); + assert_eq!( + manual, test_case.expected_manual, + "{}", + 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 87f8028685..869adc4ac1 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs @@ -210,3 +210,10 @@ pub(crate) struct SourceContractTestCase { pub(crate) overwrite: bool, pub(crate) expected_edit: ContractEdit, } + +pub(super) struct HeaderPartsTestCase { + pub(super) description: &'static str, + pub(super) contents: &'static str, + pub(super) expected_edits: &'static [&'static str], + pub(super) expected_manual: &'static [&'static str], +} 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 85b3ab7a29..042f572a9e 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md @@ -76,7 +76,8 @@ 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 and column-valued config such as `unique_key`, `cursor`, and `partition_by`, and `cursor_inputs` and `relationships (field ...)` entries that point at the column; +- 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; - 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 968e55733b..a1b8b383a4 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py @@ -184,6 +184,81 @@ def order_amount_union(*, first: str, second: str) -> dict[str, str]: "materialized table,", "materialized table,\n model_schema order_shape," ), } +FLAGGED_ORDERS: str = ( + "MODEL (\n" + ' description "Orders with a flag",\n' + " materialized table,\n" + " columns (\n" + " order_id (type VARCHAR)\n" + " is_flagged (type BOOLEAN)\n" + " ),\n" + ");\n\n" + "SELECT\n" + " CAST('o1' AS VARCHAR) AS order_id,\n" + " CAST(TRUE AS BOOLEAN) AS is_flagged\n" +) +FLAGGED_ORDER_SUMMARY: str = ( + "MODEL (\n" + ' description "Orders summary",\n' + " materialized table,\n" + ");\n\n" + "WITH orders AS (\n" + " SELECT *\n" + ' FROM __ref("flagged_orders")\n' + ")\n\n" + "SELECT\n" + " order_id,\n" + " is_flagged\n" + "FROM orders\n" +) +FLAGGED_CONTRACT: dict[str, str] = { + "models/marts/flagged_orders.sql": FLAGGED_ORDERS, + "models/marts/flagged_order_summary.sql": FLAGGED_ORDER_SUMMARY, +} +FLAGGED_AUDITS: dict[str, str] = { + "models/marts/_sqlbuild/_audits/generic/flag_check.sql": ( + "AUDIT ();\n\nSELECT *\nFROM @relation\nWHERE @where_condition AND order_id IS NULL\n" + ), + "models/marts/flagged_orders.sql": FLAGGED_ORDERS.replace( + "is_flagged (type BOOLEAN)", + "is_flagged (type BOOLEAN, audits [flag_check " + "(where_condition \"is_flagged = TRUE AND 'is_flagged' <> ''\")])", + ).replace( + " ),\n);", + ' ),\n audits [flag_check (name "orders_flag", where_condition "NOT is_flagged")],\n);', + ), + "models/marts/flagged_order_summary.sql": FLAGGED_ORDER_SUMMARY.replace( + " materialized table,\n", + ' materialized table,\n audits [flag_check (where_condition "is_flagged IS NULL")],\n', + ), +} +_FLAG_CHECK: str = ( + "AUDIT ();\n\nSELECT *\nFROM @relation\nWHERE @where_condition AND order_id IS NULL\n" +) +FLAGGED_SUBQUERY_AUDITS: dict[str, str] = { + "models/marts/_sqlbuild/_audits/generic/flag_check.sql": _FLAG_CHECK, + "models/marts/vip_flags.sql": ( + 'MODEL (description "VIP flags", materialized table);\n' + "SELECT CAST(TRUE AS BOOLEAN) AS is_flagged\n" + ), + "models/marts/flagged_orders.sql": FLAGGED_ORDERS.replace( + " ),\n);", + " ),\n audits [\n" + ' flag_check (name "vip_flag", where_condition ' + "'is_flagged IN (SELECT is_flagged FROM __ref(\"vip_flags\"))'),\n" + ' flag_check (name "own_flag", where_condition ' + "'is_flagged IN (SELECT is_flagged FROM __ref(\"flagged_orders\"))'),\n" + " ],\n);", + ), +} +FLAGGED_LITERAL_AUDIT: dict[str, str] = { + "models/marts/_sqlbuild/_audits/generic/prefix_check.sql": ( + "AUDIT ();\n\nSELECT *\nFROM @relation\nWHERE @'prefix' = ''\n" + ), + "models/marts/flagged_orders.sql": FLAGGED_ORDERS.replace( + " ),\n);", ' ),\n audits [prefix_check (prefix "is_flagged")],\n);' + ), +} ORDERS_MACRO: dict[str, str] = { "sqlbuild_project.toml": _PROJECT_TOML + "\n[references]\nenforce_explicit = false\n", "models/staging/_sqlbuild/_macros/orders.py": ( 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 a46b665363..f96ce95eec 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 @@ -18,6 +18,10 @@ from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( CENTS_MACRO, CUSTOMER_DECLARATIONS, + FLAGGED_AUDITS, + FLAGGED_CONTRACT, + FLAGGED_LITERAL_AUDIT, + FLAGGED_SUBQUERY_AUDITS, ORDER_AMOUNT_CHECK, ORDER_EXPORT, ORDER_SHAPE, @@ -127,6 +131,53 @@ "order_export": ("order_id", "customer_id", "revenue", "order_date"), }, ), + ColumnRenameE2ETestCase( + description="a column declared in the model's own contract renames in one step", + command=("rename", "column:flagged_orders.is_flagged", "is_marked", "--cascade"), + extra_files=FLAGGED_CONTRACT, + expected_fragments={ + "models/marts/flagged_orders.sql": ( + " is_marked (type BOOLEAN)\n", + "CAST(TRUE AS BOOLEAN) AS is_marked\n", + ), + "models/marts/flagged_order_summary.sql": (" is_marked\nFROM orders",), + }, + expected_columns={ + "flagged_orders": ("order_id", "is_marked"), + "flagged_order_summary": ("order_id", "is_marked"), + }, + ), + ColumnRenameE2ETestCase( + description="audit where_condition arguments rename the column as SQL", + command=("rename", "column:flagged_orders.is_flagged", "is_marked", "--cascade"), + extra_files=FLAGGED_AUDITS, + expected_fragments={ + "models/marts/flagged_orders.sql": ( + "where_condition \"is_marked = TRUE AND 'is_flagged' <> ''\"", + 'where_condition "NOT is_marked"', + ), + "models/marts/flagged_order_summary.sql": ('where_condition "is_marked IS NULL"',), + }, + expected_columns={ + "flagged_orders": ("order_id", "is_marked"), + "flagged_order_summary": ("order_id", "is_marked"), + }, + ), + ColumnRenameE2ETestCase( + description="audit subqueries rename only columns of the audited model", + command=("rename", "column:flagged_orders.is_flagged", "is_marked"), + extra_files=FLAGGED_SUBQUERY_AUDITS, + expected_fragments={ + "models/marts/flagged_orders.sql": ( + "'is_marked IN (SELECT is_flagged FROM __ref(\"vip_flags\"))'", + "'is_marked IN (SELECT is_marked FROM __ref(\"flagged_orders\"))'", + ), + }, + expected_columns={ + "flagged_orders": ("order_id", "is_marked"), + "vip_flags": ("is_flagged",), + }, + ), ], ids=lambda case: case.description, ) @@ -207,6 +258,14 @@ def test_given_column_rename_when_applied_then_project_tests_and_builds( expected_reason="reference to stg_orders produced by a macro", expected_paths=("models/staging/stg_order_count.sql",), ), + RefusedRefactorE2ETestCase( + description="audit argument the audit quotes as a string needs a manual check", + command=("rename", "column:flagged_orders.is_flagged", "is_marked"), + extra_files=FLAGGED_LITERAL_AUDIT, + expected_status="refused", + expected_reason="audit prefix_check quotes argument prefix as a string", + 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 7431ffc6c2..4946eb90d2 100644 --- a/website/src/content/docs/docs/cli/rename.mdx +++ b/website/src/content/docs/docs/cli/rename.mdx @@ -61,7 +61,8 @@ 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 and column-valued config such as `unique_key`, `cursor`, and `partition_by`, and `cursor_inputs` and `relationships (field ...)` entries that point at the column; +- 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; - 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 1ad4a8e46b3d40787a7a0a8637d8ff9c074a888c Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:14:50 +0100 Subject: [PATCH 02/14] fix(cli): report an unreadable --select-file as a coded error Reading a missing or unreadable selector file raised a raw FileNotFoundError with a traceback from compile, plan and build. The read now fails with error[C114] naming the file, in text and --json output. Refs: CHI-501 --- .../src/bindings/_helpers/cli/dispatch.rs | 41 ++++++++-- .../cli/commands/main/compile/_test_types.py | 7 ++ .../cli/commands/main/compile/helpers.py | 7 ++ .../main/compile/test_compile_selection.py | 80 +++++++++++++++++++ 4 files changed, 130 insertions(+), 5 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs index 32abc9ad06..da8cc80927 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs @@ -38,7 +38,7 @@ use crate::bindings::_helpers::workspace_commands::commands::{ run_playground_command, run_skills_command, }; use pyo3::IntoPyObjectExt; -use pyo3::exceptions::{PyAttributeError, PyTypeError}; +use pyo3::exceptions::{PyAttributeError, PyOSError, PyTypeError, PyUnicodeError}; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyErr, PyResult, Python}; use pyo3::types::{PyDict, PyDictMethods, PyString, PyTuple}; use sqlbuild_cli::diff_report::errors::DiffUserError; @@ -60,6 +60,10 @@ use crate::bindings::_helpers::records::registry; const PATHLIB_MODULE: &str = "pathlib"; const COMMENT_PREFIX: &str = "#"; const UTF8: &str = "utf-8"; +/// The code of a `--select-file` that cannot be read. +const SELECTOR_FILE_CODE: &str = "C114"; +const SELECTOR_FILE_HELP: &str = + "Pass the path of a readable UTF-8 file, relative to the current directory or absolute."; /// The `diff` arguments that compare raw queries instead of project models. const RAW_QUERY_ATTRIBUTES: [&str; 4] = [ "left_query", @@ -255,10 +259,11 @@ fn selector_inputs<'py>( let path: Bound<'py, PyAny> = path_class(py)?.call1((raw_path?,))?; let keywords: Bound<'py, PyDict> = PyDict::new(py); keywords.set_item("encoding", UTF8)?; - let lines: Vec = path - .call_method("read_text", (), Some(&keywords))? - .call_method0("splitlines")? - .extract()?; + let text: Bound<'py, PyAny> = match path.call_method("read_text", (), Some(&keywords)) { + Ok(text) => text, + Err(error) => return Err(selector_file_error(py, &path, &error)?), + }; + let lines: Vec = text.call_method0("splitlines")?.extract()?; let mut count: usize = 0; for line in &lines { let stripped: &str = python_strip(line); @@ -280,6 +285,32 @@ fn selector_inputs<'py>( Ok((PyTuple::new(py, selectors)?, PyTuple::new(py, files)?)) } +/// The user error for a `--select-file` that cannot be read, or `error` when it is unexpected. +fn selector_file_error(py: Python<'_>, path: &Bound<'_, PyAny>, error: &PyErr) -> PyResult { + let readable = + error.is_instance_of::(py) || error.is_instance_of::(py); + if !readable { + return Ok(error.clone_ref(py)); + } + let value: Bound<'_, PyAny> = error.value(py).clone().into_any(); + let reason: Bound<'_, PyAny> = value + .getattr_opt("strerror")? + .unwrap_or_else(|| value.clone()); + let reason: String = if reason.is_none() { + value.str()?.to_string() + } else { + reason.str()?.to_string() + }; + Ok(user_error( + py, + DispatchFailure { + message: format!("cannot read selector file {}: {reason}", path.str()?), + code: SELECTOR_FILE_CODE, + help: Some(SELECTOR_FILE_HELP), + }, + )) +} + /// Call the route's command implementation, or the handler a test injected for its slot. fn call_route<'py>( invocation: &Invocation<'py>, 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 429b1573c5..ad7eacb81d 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 @@ -232,6 +232,13 @@ class CompileSelectionTestCase: unexpected_stdout_fragments: tuple[str, ...] +@dataclass(frozen=True) +class SelectorFileErrorTestCase: + description: str + command: tuple[str, ...] + expected_json_codes: tuple[str, ...] + + @dataclass(frozen=True) class DeepSqlAnalysisCompileTestCase: description: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/helpers.py index 4d584bdfcf..b852152b73 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/helpers.py @@ -5088,3 +5088,10 @@ def restore_default_interrupt() -> None: """Give a spawned command default Ctrl-C handling even when its parent ignores SIGINT.""" _ = signal.signal(signal.SIGINT, signal.SIG_DFL) + + +def json_diagnostic_codes(stdout: str) -> tuple[str, ...]: + """Return the diagnostic codes of a compile JSON document; empty stdout has none.""" + + document: dict[str, Any] = json.loads(stdout or "{}") + return tuple(str(diagnostic["code"]) for diagnostic in document.get("diagnostics", ())) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py index 18978d26f8..88940fd26e 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py @@ -9,9 +9,36 @@ from tests.e2e.src.sqlbuild.cli.commands.main.compile._test_types import ( CompileSelectionTestCase, + SelectorFileErrorTestCase, ) +from tests.e2e.src.sqlbuild.cli.commands.main.compile.helpers import json_diagnostic_codes from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb +_PROJECT_TOML: str = ( + 'name = "compile_selection"\n' + 'adapter = "duckdb"\n' + 'default_target = "dev"\n\n' + "[connection]\n" + 'database = "selection.duckdb"\n\n' + "[targets.dev]\n" + 'schema = "dev"\n' +) +_SOURCE_FILES: dict[str, str] = { + "sources/raw.yml": ( + "sources:\n" + " - name: raw__orders\n" + " description: Raw orders.\n" + " schema: main\n" + " table: raw_orders\n" + " columns:\n" + " - name: order_id\n" + " type: INTEGER\n" + ), + "models/stg_orders.sql": ( + "MODEL (description 'Staged orders.');\n\nSELECT order_id FROM __source(\"raw__orders\")\n" + ), +} + @pytest.mark.parametrize( "test_case", @@ -97,5 +124,58 @@ def test_given_model_selection_when_compiling_then_limits_offline_analysis_and_r assert fragment not in result.stdout +@pytest.mark.parametrize( + "test_case", + [ + SelectorFileErrorTestCase( + description="compile names the missing selector file", + command=("compile",), + expected_json_codes=(), + ), + SelectorFileErrorTestCase( + description="compile json reports the missing selector file as a diagnostic", + command=("compile", "--json"), + expected_json_codes=("C114",), + ), + SelectorFileErrorTestCase( + description="plan names the missing selector file", + command=("plan",), + expected_json_codes=(), + ), + SelectorFileErrorTestCase( + description="build names the missing selector file", + command=("build",), + expected_json_codes=(), + ), + ], + ids=lambda case: case.description, +) +def test_given_missing_selector_file_when_running_then_reports_a_coded_error( + test_case: SelectorFileErrorTestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="compile_selection", + repo_files={"sqlbuild_project.toml": _PROJECT_TOML, **_SOURCE_FILES}, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", *test_case.command, "--select-file", "missing.txt"), + project_dir=project_dir, + working_dir=project_dir, + ) + + output: str = result.stdout + result.stderr + assert result.returncode == 1, test_case.description + output + assert "Traceback" not in output, test_case.description + output + assert "error[C114]: cannot read selector file missing.txt" in result.stderr, ( + test_case.description + output + ) + assert json_diagnostic_codes(result.stdout) == test_case.expected_json_codes, ( + test_case.description + output + ) + + if __name__ == "__main__": pytest.main([__file__, "-vv"]) From 7457811b7fb72a593276fecf91697257ac4b01dd Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:14:50 +0100 Subject: [PATCH 03/14] fix(rules): evaluate model rules on a selection without models With only declaration rules selected, model rule evaluation took the first model unconditionally, so a source-only selection sliced an empty model list and panicked. The model limit is now capped by the number of models. Refs: CHI-530 --- .../src/engine/_helpers/evaluation.rs | 6 ++- .../cli/commands/main/compile/_test_types.py | 7 ++++ .../main/compile/test_compile_selection.py | 42 +++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs b/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs index 1c8e9e2bb0..383296000a 100644 --- a/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules/src/engine/_helpers/evaluation.rs @@ -112,7 +112,11 @@ fn evaluate_native_models( "SQBRDECLARATION201" | "SQBRDECLARATION301" | "SQBRTEST301" ) }); - let model_limit = if all_models_required { models.len() } else { 1 }; + let model_limit = if all_models_required { + models.len() + } else { + models.len().min(1) + }; let mut bucket = cache .map(|store| store.native_rules_bucket(model_rules_fingerprint)) .transpose()? 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 ad7eacb81d..0a68348ecf 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 @@ -239,6 +239,13 @@ class SelectorFileErrorTestCase: expected_json_codes: tuple[str, ...] +@dataclass(frozen=True) +class SourceSelectionRulesTestCase: + description: str + selection_args: tuple[str, ...] + expected_stdout_fragment: str + + @dataclass(frozen=True) class DeepSqlAnalysisCompileTestCase: description: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py index 88940fd26e..14a265e4f1 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_selection.py @@ -10,6 +10,7 @@ from tests.e2e.src.sqlbuild.cli.commands.main.compile._test_types import ( CompileSelectionTestCase, SelectorFileErrorTestCase, + SourceSelectionRulesTestCase, ) from tests.e2e.src.sqlbuild.cli.commands.main.compile.helpers import json_diagnostic_codes from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb @@ -177,5 +178,46 @@ def test_given_missing_selector_file_when_running_then_reports_a_coded_error( ) +@pytest.mark.parametrize( + "test_case", + [ + SourceSelectionRulesTestCase( + description="an exact source selection evaluates rules without models", + selection_args=("--select", "source:raw__orders"), + expected_stdout_fragment="Project compiled 0 models", + ), + SourceSelectionRulesTestCase( + description="a source glob selection evaluates rules without models", + selection_args=("--select", "source:raw_*"), + expected_stdout_fragment="Project compiled 0 models", + ), + ], + ids=lambda case: case.description, +) +def test_given_rules_and_source_only_selection_when_compiling_then_compiles( + test_case: SourceSelectionRulesTestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="compile_selection", + repo_files={ + "sqlbuild_project.toml": _PROJECT_TOML + '\n[rules]\nselect = ["SQBRDECLARATION201"]\n', + **_SOURCE_FILES, + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "compile", *test_case.selection_args), + project_dir=project_dir, + working_dir=project_dir, + ) + + output: str = result.stdout + result.stderr + assert result.returncode == 0, test_case.description + output + assert "panicked" not in output, test_case.description + output + assert test_case.expected_stdout_fragment in result.stdout, test_case.description + output + + if __name__ == "__main__": pytest.main([__file__, "-vv"]) From 37ed9929cff9895568078209d5caf75b2b672275 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:14:51 +0100 Subject: [PATCH 04/14] fix(janitor): refuse cleanly when confirmation cannot be read Without --auto-approve, janitor prompted on a non-interactive stdin and ended with an EOFError traceback. Like build confirmations, it now refuses before prompting with error[C503] and tells the user to pass --auto-approve or run it in a terminal. Refs: CHI-534 --- .../_helpers/janitor/janitor_command.rs | 28 ++++++++- .../cli/commands/main/janitor/_test_types.py | 11 ++++ .../cli/commands/main/janitor/test_janitor.py | 57 +++++++++++++++++++ 3 files changed, 93 insertions(+), 3 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_command.rs b/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_command.rs index 0b2d70a16e..48e4e1ded7 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_command.rs @@ -8,6 +8,7 @@ use pyo3::prelude::{ use pyo3::types::{PyDict, PyDictMethods}; use pyo3::{pyfunction, wrap_pyfunction}; use sqlbuild_analysis::janitor_planning::main::resolve_retention_days::resolve_retention_days; +use sqlbuild_cli::diff_report::errors::DiffUserError; use sqlbuild_core::cli_text::models::CliStyle; use sqlbuild_core::janitor_text::errors::JanitorSettingsError; use sqlbuild_core::janitor_text::main::janitor_completion_text::janitor_completion_text; @@ -24,6 +25,7 @@ use sqlbuild_core::janitor_text::models::JanitorPlanText; use crate::bindings::_helpers::command_runtime::preview_target::validate_preview_target; use crate::bindings::_helpers::compile_driver::discovery::discover_project_inputs; use crate::bindings::_helpers::contracts::spec_resolution::resolve_target_name; +use crate::bindings::_helpers::diff_report::support::cli_error; use crate::bindings::_helpers::janitor::janitor_phases::{ JanitorCompiled, build_janitor_execution_plan, close_janitor_warehouse, compile_janitor_project, connect_janitor_warehouse, execute_janitor_cleanup, @@ -35,8 +37,15 @@ use crate::bindings::_helpers::records::construct::error_class; use crate::bindings::_helpers::records::construct::new_record; use crate::bindings::_helpers::records::fields::field as native_field; use crate::bindings::_helpers::records::ids::{ErrorId, RecordId}; +use crate::bindings::_helpers::scenario_command::setup::stream_isatty; const COMMAND_NAME: &str = "janitor"; +/// The code of a cleanup that needs a confirmation stdin cannot give. +const CONFIRMATION_CODE: &str = "C503"; +const CONFIRMATION_REFUSAL: &str = + "janitor cleanup requires confirmation, but stdin is not a terminal"; +const CONFIRMATION_HELP: &str = "Run sqb janitor in a terminal to confirm, or pass --auto-approve."; +const LINE_ENDINGS: [char; 2] = ['\n', '\r']; fn imported<'py>(py: Python<'py>, module: &str, name: &str) -> PyResult> { py.import(module)?.getattr(name) @@ -157,12 +166,25 @@ fn settings<'py>( new_record(py, RecordId::JanitorSettings, (), Some(&options)) } -/// Ask for the exact confirmation phrase; an interrupt cancels. +/// Ask for the exact confirmation phrase; an interrupt cancels, and a non-interactive stdin refuses. fn confirmed(py: Python<'_>, plan: &JanitorPlanText) -> PyResult { + let stdin: Bound<'_, PyAny> = imported(py, "sys", "stdin")?; + if stdin.is_none() || !stream_isatty(&stdin)? { + return Err(cli_error( + py, + ErrorId::CliUser, + &DiffUserError { + message: CONFIRMATION_REFUSAL.to_owned(), + code: CONFIRMATION_CODE.to_owned(), + help: Some(CONFIRMATION_HELP.to_owned()), + }, + )); + } write(py, &janitor_confirmation_prompt(plan))?; flush(py)?; - match imported(py, "builtins", "input")?.call0() { - Ok(response) => Ok(response.extract::()? == janitor_confirmation_phrase(plan)), + match stdin.call_method0("readline") { + Ok(response) => Ok(response.extract::()?.trim_end_matches(LINE_ENDINGS) + == janitor_confirmation_phrase(plan)), Err(error) if error.is_instance_of::(py) => { write(py, "\n")?; Ok(false) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/janitor/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/janitor/_test_types.py index b96a3de3be..dfe682a71f 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/janitor/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/janitor/_test_types.py @@ -175,3 +175,14 @@ class JanitorTargetErrorE2ETestCase: janitor_command: tuple[str, ...] expected_exit_code: int expected_output_fragments: tuple[str, ...] + + +@dataclass(frozen=True) +class JanitorNonInteractiveE2ETestCase: + """Test case for a janitor confirmation that stdin cannot answer.""" + + description: str + janitor_command: tuple[str, ...] + expected_exit_code: int + expected_stderr_fragments: tuple[str, ...] + expected_existing_tables: tuple[str, ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/janitor/test_janitor.py b/tests/e2e/src/sqlbuild/cli/commands/main/janitor/test_janitor.py index 810db559b5..35f6ba4e68 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/janitor/test_janitor.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/janitor/test_janitor.py @@ -13,6 +13,7 @@ JanitorDirectStatePruningE2ETestCase, JanitorDisabledE2ETestCase, JanitorInvalidConfigE2ETestCase, + JanitorNonInteractiveE2ETestCase, JanitorSourceOverlapE2ETestCase, ) from tests.e2e.src.sqlbuild.cli.commands.main.janitor.helpers import ( @@ -153,6 +154,62 @@ def test_given_stale_relations_when_running_direct_janitor_then_it_archives_trac assert "Eligible expired locks" not in janitor_result.stdout +@pytest.mark.parametrize( + "test_case", + [ + JanitorNonInteractiveE2ETestCase( + description="janitor without auto-approve refuses when stdin is not a terminal", + janitor_command=("--no-color", "janitor"), + expected_exit_code=1, + expected_stderr_fragments=( + "error[C503]: janitor cleanup requires confirmation, but stdin is not a terminal", + "pass --auto-approve", + ), + expected_existing_tables=("janitor_tracked_extra", "janitor_untracked_extra"), + ) + ], + ids=lambda case: case.description, +) +def test_given_piped_stdin_when_janitor_needs_confirmation_then_it_refuses_cleanly( + test_case: JanitorNonInteractiveE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_janitor_project( + tmp_path=tmp_path, + project_name="janitor_non_interactive_project", + janitor_config=dedent( + """ + enabled = true + retention_days = 0 + """ + ), + ) + db_path: Path = project_dir / "janitor.duckdb" + build_result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "build", "--full-refresh"), + project_dir=project_dir, + ) + create_janitor_demo_relations(db_path=db_path) + + janitor_result: subprocess.CompletedProcess[str] = run_sqb( + command=test_case.janitor_command, + project_dir=project_dir, + input_text="", + ) + + output: str = janitor_result.stdout + janitor_result.stderr + assert build_result.returncode == 0, build_result.stdout + build_result.stderr + assert janitor_result.returncode == test_case.expected_exit_code, test_case.description + output + assert "Traceback" not in output, test_case.description + output + assert all( + fragment in janitor_result.stderr for fragment in test_case.expected_stderr_fragments + ), test_case.description + output + assert all( + table_exists(db_path=db_path, table_name=table_name) + for table_name in test_case.expected_existing_tables + ), test_case.description + + @pytest.mark.parametrize( "test_case", [ From 34da81c154dced344d70d3756022891aecaf5422 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:14:51 +0100 Subject: [PATCH 05/14] fix(macros): reject list and dict keys in macro argument dicts A dict key such as [1] or (1, [2]) parsed, then failed with a raw TypeError when the Python dict was built. The argument parser now rejects list and dict keys, also inside tuple keys, with a located P001 error and help. Refs: CHI-571 --- .../macro_arguments/_helpers/evaluation.rs | 18 +++++++++++-- .../src/macro_arguments/_helpers/failures.rs | 11 ++++++++ .../tests/test_parse_macro_arguments.rs | 18 +++++++++++++ .../_helpers/corpus/failure_cases.py | 18 +++++++++++++ ...e-error-macro-argument-unhashable-key.json | 27 +++++++++++++++++++ 5 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 tests/goldens/compiler/failure/engine-error-macro-argument-unhashable-key.json diff --git a/crates/sqlbuild-render/src/macro_arguments/_helpers/evaluation.rs b/crates/sqlbuild-render/src/macro_arguments/_helpers/evaluation.rs index bb2ecdd966..cbd6346417 100644 --- a/crates/sqlbuild-render/src/macro_arguments/_helpers/evaluation.rs +++ b/crates/sqlbuild-render/src/macro_arguments/_helpers/evaluation.rs @@ -4,7 +4,7 @@ use std::collections::VecDeque; use crate::macro_arguments::_helpers::failures::{ Failure, bytes_literal, complex_literal, dict_unpacking, ellipsis, formatted_string, - keyword_expansion, typed_reference, unary, unsupported, + keyword_expansion, typed_reference, unary, unhashable_key, unsupported, }; use crate::macro_arguments::_helpers::lexer::NumberToken; use crate::macro_arguments::_helpers::syntax_tree::{CallArguments, Expr, Keyword, Node}; @@ -68,7 +68,11 @@ fn evaluate(node: &Node) -> Result { let Some(key) = key else { return Err(dict_unpacking(value.position)); }; - values.push((evaluate(key)?, evaluate(value)?)); + let key_value: ArgumentValue = evaluate(key)?; + if !is_hashable(&key_value) { + return Err(unhashable_key(key.position)); + } + values.push((key_value, evaluate(value)?)); } Ok(ArgumentValue::Dict(values)) } @@ -87,6 +91,16 @@ fn evaluate(node: &Node) -> Result { } } +/// Whether Python can hash the value; a nested call's value is left for Python to check. +fn is_hashable(value: &ArgumentValue) -> bool { + match value { + ArgumentValue::List(_) | ArgumentValue::Dict(_) => false, + ArgumentValue::Tuple(items) => items.iter().all(is_hashable), + ArgumentValue::Negative(inner) | ArgumentValue::Positive(inner) => is_hashable(inner), + _ => true, + } +} + fn evaluate_all(items: &[Node]) -> Result, Failure> { items.iter().map(evaluate).collect() } diff --git a/crates/sqlbuild-render/src/macro_arguments/_helpers/failures.rs b/crates/sqlbuild-render/src/macro_arguments/_helpers/failures.rs index e1b52f9b76..c623fd2acc 100644 --- a/crates/sqlbuild-render/src/macro_arguments/_helpers/failures.rs +++ b/crates/sqlbuild-render/src/macro_arguments/_helpers/failures.rs @@ -11,6 +11,8 @@ const ELLIPSIS_HELP: &str = "Pass None, or a string the macro understands, inste const FORMATTED_HELP: &str = "Pass the parts as separate arguments and format them in the macro"; const REFERENCE_HELP: &str = "Write one quoted resource name, for example __ref(\"orders\")"; const UNPACKING_HELP: &str = "Write each argument or key out explicitly"; +const HASHABLE_KEY_HELP: &str = "Use a string, number, True, False, None, or a tuple of them as \ + a dict key"; const KEYWORD_HELP: &str = "Pass each keyword argument once, after every positional argument"; const SURROGATE_HELP: &str = "Lone surrogates are not valid Unicode text and cannot be sent to a \ warehouse; write the character itself or the escape of a valid code point"; @@ -108,6 +110,15 @@ pub(crate) fn dict_unpacking(position: usize) -> Failure { failure("must not use dict unpacking", UNPACKING_HELP, position) } +/// A dict key Python cannot hash: a list or dict, or a tuple containing one. +pub(crate) fn unhashable_key(position: usize) -> Failure { + failure( + "use a list or dict as a dict key", + HASHABLE_KEY_HELP, + position, + ) +} + pub(crate) fn keyword_order(detail: &str, position: usize) -> Failure { failure(detail, KEYWORD_HELP, position) } diff --git a/crates/sqlbuild-render/src/macro_arguments/tests/test_parse_macro_arguments.rs b/crates/sqlbuild-render/src/macro_arguments/tests/test_parse_macro_arguments.rs index eaf557532c..27dcafc30c 100644 --- a/crates/sqlbuild-render/src/macro_arguments/tests/test_parse_macro_arguments.rs +++ b/crates/sqlbuild-render/src/macro_arguments/tests/test_parse_macro_arguments.rs @@ -70,6 +70,24 @@ fn given_argument_text_when_parsing_then_values_match_python_literals() { nested: &[], expected_plan: "Err(ArgumentError { detail: \"use \'...\' (Ellipsis)\", help: \"Pass None, or a string the macro understands, instead of \'...\'\", line: 1, column: 1 })", }, + ParseMacroArgumentsTestCase { + description: "a list dict key is rejected at the key", + text: "{'a': 1, [1]: 2}", + nested: &[], + expected_plan: "Err(ArgumentError { detail: \"use a list or dict as a dict key\", help: \"Use a string, number, True, False, None, or a tuple of them as a dict key\", line: 1, column: 10 })", + }, + ParseMacroArgumentsTestCase { + description: "a tuple dict key holding a list is rejected", + text: "{(1, [2]): 2}", + nested: &[], + expected_plan: "Err(ArgumentError { detail: \"use a list or dict as a dict key\", help: \"Use a string, number, True, False, None, or a tuple of them as a dict key\", line: 1, column: 2 })", + }, + ParseMacroArgumentsTestCase { + description: "tuple, number and reference dict keys are accepted", + text: "{(1, 'a'): 2, -1: 3, __ref('a'): 4}", + nested: &[], + expected_plan: "Ok(MacroArguments { positional: [Dict([(Tuple([Int { radix: 10, digits: \"1\" }, Str(\"a\")]), Int { radix: 10, digits: \"2\" }), (Negative(Int { radix: 10, digits: \"1\" }), Int { radix: 10, digits: \"3\" }), (TypedReference { function: \"__ref\", name: \"a\" }, Int { radix: 10, digits: \"4\" })])], keywords: [], typed_references: [(\"__ref\", \"a\")] })", + }, ParseMacroArgumentsTestCase { description: "an f-string is rejected", text: "f'{x}'", diff --git a/scripts/compiler_differential/_helpers/corpus/failure_cases.py b/scripts/compiler_differential/_helpers/corpus/failure_cases.py index 8fa79fe75b..d58dab3217 100644 --- a/scripts/compiler_differential/_helpers/corpus/failure_cases.py +++ b/scripts/compiler_differential/_helpers/corpus/failure_cases.py @@ -818,6 +818,24 @@ def engine_error_cases() -> tuple[FailureCase, ...]: "b'orders'" ), ), + failure_case( + name="engine-error-macro-argument-unhashable-key", + expected_code="P001", + files={ + _ENGINE_ERROR_MACROS: _ENGINE_ERROR_CENTS, + **mart_body_files( + 'SELECT customer_id, @cents({[1]: 2}) AS cents\nFROM __ref("stg_orders")\n' + ), + }, + expected_message=( + f"Macro arguments of '@cents' in '/{FAILURE_MART_PATH}' " + "use a list or dict as a dict key at line 5, column 29" + ), + expected_location=(5, 29), + expected_help=( + "Use a string, number, True, False, None, or a tuple of them as a dict key" + ), + ), failure_case( name="engine-error-macro-argument-complex", expected_code="P001", diff --git a/tests/goldens/compiler/failure/engine-error-macro-argument-unhashable-key.json b/tests/goldens/compiler/failure/engine-error-macro-argument-unhashable-key.json new file mode 100644 index 0000000000..9477f06115 --- /dev/null +++ b/tests/goldens/compiler/failure/engine-error-macro-argument-unhashable-key.json @@ -0,0 +1,27 @@ +{ + "commands": [ + { + "label": "compile", + "exit_code": 1, + "diagnostics": [ + { + "phase": "compile", + "severity": "error", + "code": "P001", + "message": "Macro arguments of '@cents' in '/models/marts/customer_totals.sql' use a list or dict as a dict key at line 5, column 29", + "path": "models/marts/customer_totals.sql", + "line": 5, + "column": 29, + "location": { + "path": "models/marts/customer_totals.sql", + "line": 5, + "column": 29 + }, + "help": "Use a string, number, True, False, None, or a tuple of them as a dict key" + } + ] + } + ], + "compiled": {}, + "manifest": null +} From 78e9f7c12119a0d9e374115367d70e26701d868e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:15:02 +0100 Subject: [PATCH 06/14] fix(cli): report a warehouse connection failure as a coded error When the warehouse connection could not be opened, for example a DuckDB file in a missing directory, build, seed, audit, test, plan, load, query and diff printed the failed connection line and then the adapter's raw exception with a traceback. Connection failures now raise error[C115] naming the adapter and the reason, with help pointing at sqb debug. SQLBuild's own coded errors pass through unchanged; a client library error that carries its own `code`, such as a Google API error, is still reported as C115. Refs: CHI-596 --- .../bindings/_helpers/check/check_command.rs | 6 +- .../src/bindings/_helpers/cli/entry.rs | 3 +- .../bindings/_helpers/clone/clone_command.rs | 4 +- .../_helpers/compile_driver/plan_result.rs | 3 +- .../_helpers/contract_command/command.rs | 3 +- .../bindings/_helpers/diff_report/command.rs | 5 +- .../_helpers/freshness/freshness_command.rs | 5 +- .../_helpers/janitor/janitor_phases.rs | 3 +- .../bindings/_helpers/load/load_command.rs | 5 +- .../_helpers/native_host/connections.rs | 22 +++ .../bindings/_helpers/native_host/executor.rs | 12 ++ .../src/bindings/_helpers/native_host/mod.rs | 1 + .../bindings/_helpers/query_command/query.rs | 4 +- src/sqlbuild/_native_host.py | 8 + src/sqlbuild/_native_host.pyi | 6 + .../executor/load/_helpers/scheduling.py | 7 +- .../executor/pipeline/_helpers/auditing.py | 7 +- .../executor/pipeline/_helpers/scenario.py | 7 +- .../executor/pipeline/_helpers/seeding.py | 7 +- .../executor/pipeline/_helpers/testing.py | 7 +- src/sqlbuild/executor/pipeline/constants.py | 3 + src/sqlbuild/executor/pipeline/exceptions.py | 13 ++ .../raise_warehouse_connection_failure.py | 25 ++++ .../cli/commands/main/build/_test_types.py | 9 ++ .../main/build/test_lifecycle_commands.py | 137 ++++++++++++++++++ .../pipeline/_helpers/test_auditing.py | 11 +- .../sqlbuild/runtime/contracts/_test_types.py | 1 + .../runtime/contracts/test_open_connection.py | 13 +- 28 files changed, 310 insertions(+), 27 deletions(-) create mode 100644 crates/sqlbuild-python/src/bindings/_helpers/native_host/connections.rs create mode 100644 src/sqlbuild/executor/pipeline/constants.py create mode 100644 src/sqlbuild/executor/pipeline/main/raise_warehouse_connection_failure.py diff --git a/crates/sqlbuild-python/src/bindings/_helpers/check/check_command.rs b/crates/sqlbuild-python/src/bindings/_helpers/check/check_command.rs index 98d7fd7328..385086be84 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/check/check_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/check/check_command.rs @@ -11,6 +11,7 @@ use crate::bindings::_helpers::dbt_interop::reference_resolver::resolve_external use crate::bindings::_helpers::graph::algorithms::transitive_closure_many; use crate::bindings::_helpers::graph::execution_gates::sql_ref_key; use crate::bindings::_helpers::graph::python_lifecycle::build_python_sql_run_lifecycle; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::planner_entries::command_plans::build_relation_command_plan; use crate::bindings::_helpers::planner_phases::objects::object; use crate::bindings::_helpers::planner_selection::selectors::parse_project_selector; @@ -564,8 +565,9 @@ fn execute<'py>( ) -> PyResult>> { let py: Python<'py> = invocation.py(); let adapter: Bound<'py, PyAny> = native_field(invocation, "adapter")?; - let connection: Bound<'py, PyAny> = - adapter.call_method1("connect", (native_field(invocation, "connection_config")?,))?; + let connection: Bound<'py, PyAny> = adapter + .call_method1("connect", (native_field(invocation, "connection_config")?,)) + .map_err(|error| connection_failure(&adapter, &error))?; let event_writer: Bound<'py, PyAny> = Bound::new(py, ExecutionEventWriter::new(py, None)?)?.into_any(); let executed: PyResult>> = (|| { diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs index 6323521345..676a05d255 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs @@ -43,9 +43,10 @@ const DIFF_PUBLICATION_CODE: &str = "C257"; const DIFF_EXECUTION_STATUS: &str = "execution_failed"; const DIFF_OUTCOME_PREFIX: &str = "Query diff outcome "; /// Expected staying-Python command failures, loaded only once a command raised. -const EXPECTED_COMMAND_FAILURES: [ExecutorHost; 2] = [ +const EXPECTED_COMMAND_FAILURES: [ExecutorHost; 3] = [ ExecutorHost::AuditExecutionError, ExecutorHost::ExecutionDurationLimitError, + ExecutorHost::WarehouseConnectionError, ]; /// Expected native command failures. const EXPECTED_NATIVE_FAILURES: [ErrorId; 2] = [ErrorId::Discovery, ErrorId::SpecConfig]; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/clone/clone_command.rs b/crates/sqlbuild-python/src/bindings/_helpers/clone/clone_command.rs index de1ab3c393..098bca4a05 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/clone/clone_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/clone/clone_command.rs @@ -33,6 +33,7 @@ use crate::bindings::_helpers::contracts::spec_resolution::resolve_effective_ada use crate::bindings::_helpers::contracts::spec_resolution::resolve_target_config; use crate::bindings::_helpers::contracts::spec_resolution::resolve_target_name; use crate::bindings::_helpers::dbt_interop::reference_resolver::resolve_external_sql_reference_resolver; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::native_host::executor::ExecutorHost; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::planner_selection::unit_tests::reject_unit_test_selectors; @@ -214,7 +215,8 @@ fn connect<'py>( let connection: Bound<'py, PyAny> = within( &lifecycle(py, ("clone", "clone_target_connection"))?, |_| invocation.adapter.call_method1("connect", (&config,)), - )?; + ) + .map_err(|error| connection_failure(&invocation.adapter, &error))?; let summary: String = format!( "{} ({:.2}s)", invocation.adapter_name, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_result.rs b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_result.rs index c013edc2c1..407cf70237 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_result.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/plan_result.rs @@ -19,6 +19,7 @@ use pyo3::{pyfunction, wrap_pyfunction}; use crate::bindings::_helpers::compile_driver::phases::monotonic; use crate::bindings::_helpers::compile_driver::python_calls::KeywordArguments; use crate::bindings::_helpers::graph::python_node_graph::run_selection_from_inputs; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::native_host::user_code::UserCodeHost; use crate::bindings::_helpers::records::fields::field as native_field; @@ -280,7 +281,7 @@ pub(crate) fn connection_with_hooks<'py>( if error.is_instance_of::(hooks.py()) && !on_error.is_none() { on_error.call((SINGLE_CONNECTION,), Some(&elapsed(started)?))?; } - return Err(error); + return Err(connection_failure(adapter, &error)); } }; let on_complete: Bound<'py, PyAny> = native_field(hooks, "on_connection_complete")?; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/contract_command/command.rs b/crates/sqlbuild-python/src/bindings/_helpers/contract_command/command.rs index ff774e700f..56b69f6503 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/contract_command/command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/contract_command/command.rs @@ -27,6 +27,7 @@ use crate::bindings::_helpers::compile_driver::project::build_compiled_project; use crate::bindings::_helpers::compile_driver::python_calls::keywords; use crate::bindings::_helpers::compile_driver::python_calls::{call_with, imported}; use crate::bindings::_helpers::contract_command::write::{ContractWrite, write_contracts}; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::planner_selection::selectors::select_project_graph; use crate::bindings::_helpers::planner_selection::unit_tests::reject_unit_test_selectors; use crate::bindings::_helpers::presentation_text::terminal::stdout_supports_color; @@ -199,7 +200,7 @@ fn compare<'py>( if exception(py, &error) { connection_timing(&reporter, "on_connection_error", connection_started)?; } - return Err(error); + return Err(connection_failure(&adapter, &error)); } }; let compared: PyResult> = (|| { diff --git a/crates/sqlbuild-python/src/bindings/_helpers/diff_report/command.rs b/crates/sqlbuild-python/src/bindings/_helpers/diff_report/command.rs index 1a830d2dcf..4b0ff2b799 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/diff_report/command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/diff_report/command.rs @@ -37,6 +37,7 @@ use crate::bindings::_helpers::diff_report::support::{ use crate::bindings::_helpers::discovery::configuration::discover_project_configuration; use crate::bindings::_helpers::frontier::encoder::emit_error; use crate::bindings::_helpers::native_host::adapter::AdapterHost; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::native_host::executor::ExecutorHost; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::planner_selection::unit_tests::reject_unit_test_selectors; @@ -441,7 +442,9 @@ fn run_model_diff( ], )? }; - let connection: Bound<'_, PyAny> = adapter.call_method1("connect", (&connection_config,))?; + let connection: Bound<'_, PyAny> = adapter + .call_method1("connect", (&connection_config,)) + .map_err(|error| connection_failure(&adapter, &error))?; let extras: [(&str, Bound<'_, PyAny>); 5] = [ ("bounded", native_field(request, "bounded")?), ("max_column_examples", object(py, column_examples)?), diff --git a/crates/sqlbuild-python/src/bindings/_helpers/freshness/freshness_command.rs b/crates/sqlbuild-python/src/bindings/_helpers/freshness/freshness_command.rs index c4841c9d00..a26b38c856 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/freshness/freshness_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/freshness/freshness_command.rs @@ -22,6 +22,7 @@ use crate::bindings::_helpers::contracts::spec_resolution::resolve_effective_ada use crate::bindings::_helpers::freshness::freshness_observe::{PreviousState, observe_freshness}; use crate::bindings::_helpers::freshness::observation::Observer; use crate::bindings::_helpers::freshness::state_io::{StateRead, read_latest}; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::planner_selection::selectors::{ expand_project_upstream_keys, select_project_graph, }; @@ -221,7 +222,9 @@ fn observe( ], )?), )?; - let connection: Bound<'_, PyAny> = adapter.call_method1("connect", (connection_config,))?; + let connection: Bound<'_, PyAny> = adapter + .call_method1("connect", (connection_config,)) + .map_err(|error| connection_failure(adapter, &error))?; let observed = (|| { let project: Bound<'_, PyAny> = native_field(graph, "project")?; let by_identity: Option> = diff --git a/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_phases.rs b/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_phases.rs index 639e89515b..5fc363b8da 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_phases.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/janitor/janitor_phases.rs @@ -2,6 +2,7 @@ use crate::bindings::_helpers::compile_driver::project::build_compiled_project; use crate::bindings::_helpers::compile_driver::python_calls::keywords; +use crate::bindings::_helpers::native_host::connections::connection_failure; use std::time::Instant; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyErr, PyResult, Python}; @@ -172,7 +173,7 @@ pub(crate) fn connect_janitor_warehouse<'py>( } Err(error) => { connection_timing(&progress, "on_connection_error", started)?; - Err(error) + Err(connection_failure(&compiled.adapter, &error)) } } } diff --git a/crates/sqlbuild-python/src/bindings/_helpers/load/load_command.rs b/crates/sqlbuild-python/src/bindings/_helpers/load/load_command.rs index 40531e77e1..e7b86460f5 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/load/load_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/load/load_command.rs @@ -35,6 +35,7 @@ use crate::bindings::_helpers::load::load_selection::{ LoadProject, LoaderDefaults, loader_defaults, }; use crate::bindings::_helpers::native_host::adapter::AdapterHost; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::native_host::executor::ExecutorHost; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::native_host::user_code::UserCodeHost; @@ -168,7 +169,9 @@ fn check_reference_targets( .iter() .map(|source| native_field(source, "name")?.extract()) .collect::>()?; - let connection: Bound<'_, PyAny> = adapter.call_method1("connect", (connection_config,))?; + let connection: Bound<'_, PyAny> = adapter + .call_method1("connect", (connection_config,)) + .map_err(|error| connection_failure(adapter, &error))?; let checked: PyResult<()> = (|| { let locations: Vec> = invocation .references diff --git a/crates/sqlbuild-python/src/bindings/_helpers/native_host/connections.rs b/crates/sqlbuild-python/src/bindings/_helpers/native_host/connections.rs new file mode 100644 index 0000000000..447ddf442d --- /dev/null +++ b/crates/sqlbuild-python/src/bindings/_helpers/native_host/connections.rs @@ -0,0 +1,22 @@ +//! The coded error commands raise when the warehouse connection cannot be opened. + +use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyErr, Python}; +use pyo3::types::{PyDict, PyDictMethods}; + +use crate::bindings::_helpers::native_host::executor::ExecutorHost; +use crate::bindings::_helpers::native_host::host; + +/// The coded error to raise for a failed `adapter.connect`, or the failure itself when coded. +pub(crate) fn connection_failure(adapter: &Bound<'_, PyAny>, error: &PyErr) -> PyErr { + let py: Python<'_> = adapter.py(); + let raised = (|| { + let options: Bound<'_, PyDict> = PyDict::new(py); + options.set_item("adapter", adapter)?; + options.set_item("error", error.value(py))?; + host::entry(py, ExecutorHost::RaiseWarehouseConnectionFailure)?.call((), Some(&options)) + })(); + match raised { + Ok(_) => error.clone_ref(py), + Err(failure) => failure, + } +} diff --git a/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs b/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs index fe6bf9c1e5..c7528eda68 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs @@ -68,6 +68,8 @@ pub(crate) enum ExecutorHost { MigrationRelationForLocation, IsCompatibilityView, AuditExecutionError, + WarehouseConnectionError, + RaiseWarehouseConnectionFailure, AuditPipelineCallbacks, RunAuditPipelineWithCallbacks, RunBuildPipeline, @@ -486,6 +488,16 @@ pub(crate) const ENTRIES: &[HostEntry] = &[ kind: HostKind::Exception, edge: HostEdge::Executor, }, + HostEntry { + name: "WarehouseConnectionError", + kind: HostKind::Exception, + edge: HostEdge::Executor, + }, + HostEntry { + name: "raise_warehouse_connection_failure", + kind: HostKind::Function, + edge: HostEdge::Executor, + }, HostEntry { name: "AuditPipelineCallbacks", kind: HostKind::Class, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/native_host/mod.rs b/crates/sqlbuild-python/src/bindings/_helpers/native_host/mod.rs index c9c575a440..3efbda9aea 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/native_host/mod.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/native_host/mod.rs @@ -1,6 +1,7 @@ //! The one Python module native code imports by path, and the staying-Python entries it names. pub(crate) mod adapter; +pub(crate) mod connections; pub(crate) mod constants; pub(crate) mod executor; pub(crate) mod facade; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/query_command/query.rs b/crates/sqlbuild-python/src/bindings/_helpers/query_command/query.rs index 9b72b40611..0adfe98ad0 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/query_command/query.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/query_command/query.rs @@ -17,6 +17,7 @@ use crate::bindings::_helpers::command_runtime::connection::resolve_project_conn use crate::bindings::_helpers::compile_driver::python_calls::keywords; use crate::bindings::_helpers::compile_driver::python_calls::{KeywordArguments, call_with}; use crate::bindings::_helpers::native_host::adapter::AdapterHost; +use crate::bindings::_helpers::native_host::connections::connection_failure; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::query_command::project::{ProjectAdapter, project_adapter}; use crate::bindings::_helpers::query_command::query_values::{ @@ -164,7 +165,8 @@ fn run_sql<'py>( )?; let connection: Bound<'py, PyAny> = project .adapter - .call_method1("connect", (connection_config,))?; + .call_method1("connect", (connection_config,)) + .map_err(|error| connection_failure(&project.adapter, &error))?; let limit_value: Bound<'py, PyAny> = match limit { Some(limit) => PyInt::new(py, limit).into_any(), None => py.None().into_bound(py), diff --git a/src/sqlbuild/_native_host.py b/src/sqlbuild/_native_host.py index 3ee235e68e..4adc8800e5 100644 --- a/src/sqlbuild/_native_host.py +++ b/src/sqlbuild/_native_host.py @@ -381,6 +381,14 @@ "is_compatibility_view", ), "AuditExecutionError": ("sqlbuild.executor.pipeline.exceptions", "AuditExecutionError"), + "WarehouseConnectionError": ( + "sqlbuild.executor.pipeline.exceptions", + "WarehouseConnectionError", + ), + "raise_warehouse_connection_failure": ( + "sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure", + "raise_warehouse_connection_failure", + ), "AuditPipelineCallbacks": ("sqlbuild.executor.pipeline.main.run", "AuditPipelineCallbacks"), "run_audit_pipeline_with_callbacks": ( "sqlbuild.executor.pipeline.main.run", diff --git a/src/sqlbuild/_native_host.pyi b/src/sqlbuild/_native_host.pyi index 1dde9af711..7e222cc35e 100644 --- a/src/sqlbuild/_native_host.pyi +++ b/src/sqlbuild/_native_host.pyi @@ -270,6 +270,12 @@ from sqlbuild.executor.old_name_views._helpers.janitor import ( is_compatibility_view as is_compatibility_view, ) from sqlbuild.executor.pipeline.exceptions import AuditExecutionError as AuditExecutionError +from sqlbuild.executor.pipeline.exceptions import ( + WarehouseConnectionError as WarehouseConnectionError, +) +from sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure import ( + raise_warehouse_connection_failure as raise_warehouse_connection_failure, +) from sqlbuild.executor.pipeline.main.run import AuditPipelineCallbacks as AuditPipelineCallbacks from sqlbuild.executor.pipeline.main.run import ( run_audit_pipeline_with_callbacks as run_audit_pipeline_with_callbacks, diff --git a/src/sqlbuild/executor/load/_helpers/scheduling.py b/src/sqlbuild/executor/load/_helpers/scheduling.py index bedf7263e1..e7737c7d6e 100644 --- a/src/sqlbuild/executor/load/_helpers/scheduling.py +++ b/src/sqlbuild/executor/load/_helpers/scheduling.py @@ -35,6 +35,9 @@ LoadRuntimeParams, ) from sqlbuild.executor.load.types import LoadProgressCallback +from sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure import ( + raise_warehouse_connection_failure, +) from sqlbuild.executor.scheduling.types import ExecutionStatus from sqlbuild.runtime.contracts.types import ConnectionElapsedCallback @@ -150,13 +153,13 @@ def open_load_connections( try: for _ in range(effective_concurrency): connections.append(adapter.connect(connection_config)) - except Exception: + except Exception as error: if on_connection_error is not None: on_connection_error(effective_concurrency, elapsed_seconds=time.monotonic() - start) connection: Any for connection in connections: adapter.close(connection) - raise + raise_warehouse_connection_failure(adapter=adapter, error=error) if on_connection_complete is not None: on_connection_complete(effective_concurrency, elapsed_seconds=time.monotonic() - start) return tuple(connections) diff --git a/src/sqlbuild/executor/pipeline/_helpers/auditing.py b/src/sqlbuild/executor/pipeline/_helpers/auditing.py index 91696eb327..1065a65c81 100644 --- a/src/sqlbuild/executor/pipeline/_helpers/auditing.py +++ b/src/sqlbuild/executor/pipeline/_helpers/auditing.py @@ -39,6 +39,9 @@ AuditExecutionError, AuditOutcomeError, ) +from sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure import ( + raise_warehouse_connection_failure, +) from sqlbuild.executor.pipeline.models import AuditExecutionFailure, AuditPipelineCallbacks from sqlbuild.executor.scheduling.main._run_worker import run_worker_with_completion from sqlbuild.runtime.contracts.types import ConnectionElapsedCallback @@ -289,7 +292,7 @@ def _run_audits( connection_config=connection_config, connection_count=worker_count, ) - except BaseException: + except BaseException as error: if callbacks.on_connection_error is not None: try: callbacks.on_connection_error( @@ -297,7 +300,7 @@ def _run_audits( ) except BaseException: pass - raise + raise_warehouse_connection_failure(adapter=adapter, error=error) active_error: BaseException | None = None try: if callbacks.on_connection_complete is not None: diff --git a/src/sqlbuild/executor/pipeline/_helpers/scenario.py b/src/sqlbuild/executor/pipeline/_helpers/scenario.py index 918f030374..decc58ee66 100644 --- a/src/sqlbuild/executor/pipeline/_helpers/scenario.py +++ b/src/sqlbuild/executor/pipeline/_helpers/scenario.py @@ -42,6 +42,9 @@ timed_out_scenario_result, ) from sqlbuild.executor.pipeline._helpers.settings import resolve_promotion_mode +from sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure import ( + raise_warehouse_connection_failure, +) from sqlbuild.executor.scenario.classes.prepared_scenario_schemas import PreparedScenarioSchemas from sqlbuild.executor.scenario.classes.running_scenarios import RunningScenarios from sqlbuild.executor.scenario.classes.scenario_interrupts import ScenarioInterrupts @@ -421,10 +424,10 @@ def _scenario_target_connections( connection_config=connection_config, connection_count=connection_count, ) - except Exception: + except Exception as error: if hooks.on_connection_error is not None: hooks.on_connection_error(connection_count, elapsed_seconds=time.monotonic() - start) - raise + raise_warehouse_connection_failure(adapter=adapter, error=error) if hooks.on_connection_complete is not None: hooks.on_connection_complete(connection_count, elapsed_seconds=time.monotonic() - start) recovery: ConnectionRecovery = ConnectionRecovery( diff --git a/src/sqlbuild/executor/pipeline/_helpers/seeding.py b/src/sqlbuild/executor/pipeline/_helpers/seeding.py index e608c77529..71bfa9083f 100644 --- a/src/sqlbuild/executor/pipeline/_helpers/seeding.py +++ b/src/sqlbuild/executor/pipeline/_helpers/seeding.py @@ -12,6 +12,9 @@ from sqlbuild.adapter.contract.classes.base_adapter import BaseAdapter from sqlbuild.adapter.contract.classes.statement_recorder import StatementRecorder from sqlbuild.executor.build.models import SeedExecutionResult +from sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure import ( + raise_warehouse_connection_failure, +) from sqlbuild.executor.scheduling.types import ExecutionStatus from sqlbuild.executor.seed.main._execute import execute_seed from sqlbuild.runtime.contracts.types import ConnectionElapsedCallback @@ -46,13 +49,13 @@ def run_seed_pipeline( try: for _ in range(effective_concurrency): connections.append(adapter.connect(connection_config)) - except Exception: + except Exception as error: if on_connection_error is not None: on_connection_error(effective_concurrency, elapsed_seconds=time.monotonic() - start) connection: Any for connection in connections: adapter.close(connection) - raise + raise_warehouse_connection_failure(adapter=adapter, error=error) if on_connection_complete is not None: on_connection_complete(effective_concurrency, elapsed_seconds=time.monotonic() - start) diff --git a/src/sqlbuild/executor/pipeline/_helpers/testing.py b/src/sqlbuild/executor/pipeline/_helpers/testing.py index a1eb9145ff..da9881cdde 100644 --- a/src/sqlbuild/executor/pipeline/_helpers/testing.py +++ b/src/sqlbuild/executor/pipeline/_helpers/testing.py @@ -30,6 +30,9 @@ close_connections, open_worker_connections, ) +from sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure import ( + raise_warehouse_connection_failure, +) from sqlbuild.executor.pipeline.models import TestPipelineCallbacks from sqlbuild.executor.scheduling.classes.recovering_connection_pool import RecoveringConnectionPool from sqlbuild.executor.scheduling.types import ExecutionStatus @@ -85,12 +88,12 @@ def run_test_pipeline( connection_config=connection_config, connection_count=worker_count, ) - except BaseException: + except BaseException as error: if resolved_callbacks.on_connection_error is not None: resolved_callbacks.on_connection_error( worker_count, elapsed_seconds=time.monotonic() - start ) - raise + raise_warehouse_connection_failure(adapter=adapter, error=error) if resolved_callbacks.on_connection_complete is not None: resolved_callbacks.on_connection_complete( worker_count, elapsed_seconds=time.monotonic() - start diff --git a/src/sqlbuild/executor/pipeline/constants.py b/src/sqlbuild/executor/pipeline/constants.py new file mode 100644 index 0000000000..fd137bcba7 --- /dev/null +++ b/src/sqlbuild/executor/pipeline/constants.py @@ -0,0 +1,3 @@ +"""Execution pipeline constants.""" + +SQLBUILD_PACKAGE: str = "sqlbuild" diff --git a/src/sqlbuild/executor/pipeline/exceptions.py b/src/sqlbuild/executor/pipeline/exceptions.py index 1e0500db42..889f24321c 100644 --- a/src/sqlbuild/executor/pipeline/exceptions.py +++ b/src/sqlbuild/executor/pipeline/exceptions.py @@ -32,3 +32,16 @@ class AuditConcurrencyError(RuntimeError): class AuditOutcomeError(RuntimeError): """Raised when audit execution returns an unknown quality outcome.""" + + +class WarehouseConnectionError(Exception): + """Raised when a command cannot open its warehouse connection.""" + + code: str = "C115" + + def __init__(self, *, adapter_name: str, reason: str) -> None: + self.message: str = f"could not connect to the {adapter_name} warehouse: {reason}" + self.help: str = ( + "Check the connection settings of the target; `sqb debug` tests the connection." + ) + super().__init__(self.message) diff --git a/src/sqlbuild/executor/pipeline/main/raise_warehouse_connection_failure.py b/src/sqlbuild/executor/pipeline/main/raise_warehouse_connection_failure.py new file mode 100644 index 0000000000..d1b2f3951b --- /dev/null +++ b/src/sqlbuild/executor/pipeline/main/raise_warehouse_connection_failure.py @@ -0,0 +1,25 @@ +"""Public entry for the coded failure of a warehouse connection a command cannot open.""" + +from __future__ import annotations + +from typing import NoReturn + +from sqlbuild.adapter.contract.classes.base_adapter import BaseAdapter +from sqlbuild.executor.pipeline.constants import SQLBUILD_PACKAGE +from sqlbuild.executor.pipeline.exceptions import WarehouseConnectionError + + +def raise_warehouse_connection_failure(*, adapter: BaseAdapter, error: BaseException) -> NoReturn: + """Raise the coded connection error for `error`, or `error` itself when SQLBuild coded it.""" + + if not isinstance(error, Exception) or _is_sqlbuild_coded(error): + raise error + raise WarehouseConnectionError( + adapter_name=getattr(adapter, "adapter_name", None) or type(adapter).__name__, + reason=str(error) or type(error).__name__, + ) from error + + +def _is_sqlbuild_coded(error: Exception) -> bool: + package: str = type(error).__module__.partition(".")[0] + return package == SQLBUILD_PACKAGE and isinstance(getattr(error, "code", None), str) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py index 692244d217..118451fa56 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py @@ -1336,3 +1336,12 @@ class ModelConfigEnginesBuildE2ETestCase: expected_exit_code: int expected_output_fragments: tuple[str, ...] expected_relations: tuple[tuple[str, str], ...] + + +@dataclass(frozen=True) +class WarehouseConnectionFailureE2ETestCase: + description: str + command: tuple[str, ...] + expected_exit_code: int + expected_stderr_fragments: tuple[str, ...] + extra_files: dict[str, str] = field(default_factory=dict) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/build/test_lifecycle_commands.py b/tests/e2e/src/sqlbuild/cli/commands/main/build/test_lifecycle_commands.py index b6d5e77e67..d71e77b767 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/build/test_lifecycle_commands.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/build/test_lifecycle_commands.py @@ -10,13 +10,68 @@ from tests.e2e.src.sqlbuild.cli.commands.main.build._test_types import ( LifecycleCommandsBuildE2ETestCase, + WarehouseConnectionFailureE2ETestCase, ) from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import ( assert_fragments_in_order, + prepare_inline_project, prepare_waffle_shop, run_sqb, ) +_UNREACHABLE_WAREHOUSE_FILES: dict[str, str] = { + "sqlbuild_project.toml": ( + 'name = "unreachable_warehouse"\n' + 'adapter = "duckdb"\n' + 'default_target = "dev"\n\n' + "[connection]\n" + 'database = "missing_directory/nested/orders.duckdb"\n\n' + "[targets.dev]\n" + 'schema = "dev"\n' + ), + "seeds/order_statuses.csv": "status_id,status\n1,placed\n", + "seeds/order_statuses.yml": ( + "seeds:\n" + " - name: order_statuses\n" + " description: Order statuses.\n" + " columns:\n" + " - name: status_id\n" + " type: INTEGER\n" + " audits: [not_null]\n" + " - name: status\n" + " type: VARCHAR\n" + ), + "models/statuses.sql": ( + "MODEL (description 'Statuses.', materialized table, " + "columns (status_id (audits [not_null])));\n\n" + 'SELECT status_id FROM __seed("order_statuses")\n' + ), + "tests/unit/test_statuses.sql": ( + "TEST();\n\n" + "WITH __seed__order_statuses AS (SELECT 1 AS status_id, 'placed' AS status),\n" + "__expected__statuses AS (SELECT 1 AS status_id)\n" + "SELECT 1\n" + ), +} +_FORBIDDEN_ADAPTER_FILES: dict[str, str] = { + "sqlbuild_project.toml": _UNREACHABLE_WAREHOUSE_FILES["sqlbuild_project.toml"] + .replace('adapter = "duckdb"', 'adapter = "duckdb_forbidden"') + .replace("missing_directory/nested/orders.duckdb", "orders.duckdb"), + "adapters/duckdb_forbidden.py": ( + "from sqlbuild.adapters.duckdb.classes.duckdb_adapter import DuckDbAdapter\n\n\n" + "class Forbidden(Exception):\n" + " code = 403\n\n\n" + "class DuckDbForbiddenAdapter(DuckDbAdapter):\n" + ' adapter_name = "duckdb_forbidden"\n\n' + " def connect(self, config):\n" + ' raise Forbidden("403 access denied for the orders project")\n' + ), +} +_CONNECTION_FAILURE: tuple[str, ...] = ( + "error[C115]: could not connect to the duckdb warehouse", + "`sqb debug` tests the connection", +) + @pytest.mark.parametrize( "test_case", @@ -165,3 +220,85 @@ def test_given_waffle_shop_when_running_core_lifecycle_commands_then_outputs_are full_refresh_build.stdout + full_refresh_build.stderr ) assert test_case.expected_full_refresh_fragment in full_refresh_build.stdout + + +@pytest.mark.parametrize( + "test_case", + [ + WarehouseConnectionFailureE2ETestCase( + description=f"{' '.join(command)} reports an unreachable warehouse as a coded error", + command=command, + expected_exit_code=1, + expected_stderr_fragments=_CONNECTION_FAILURE, + ) + for command in ( + ("build",), + ("seed",), + ("audit",), + ("plan",), + ("test",), + ("query", "SELECT 1"), + ) + ], + ids=lambda case: case.description, +) +def test_given_unreachable_warehouse_when_running_command_then_reports_coded_error( + test_case: WarehouseConnectionFailureE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="unreachable_warehouse", + repo_files=_UNREACHABLE_WAREHOUSE_FILES, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", *test_case.command), + project_dir=project_dir, + ) + + output: str = result.stdout + result.stderr + assert result.returncode == test_case.expected_exit_code, test_case.description + output + assert "Traceback" not in output, test_case.description + output + assert all(fragment in result.stderr for fragment in test_case.expected_stderr_fragments), ( + test_case.description + output + ) + + +@pytest.mark.parametrize( + "test_case", + [ + WarehouseConnectionFailureE2ETestCase( + description="a client error carrying its own code is still a coded connection error", + command=("build",), + expected_exit_code=1, + expected_stderr_fragments=( + "error[C115]: could not connect to the duckdb_forbidden warehouse: " + "403 access denied for the orders project", + ), + extra_files=_FORBIDDEN_ADAPTER_FILES, + ) + ], + ids=lambda case: case.description, +) +def test_given_client_error_with_own_code_when_connecting_then_reports_coded_error( + test_case: WarehouseConnectionFailureE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="unreachable_warehouse", + repo_files={**_UNREACHABLE_WAREHOUSE_FILES, **test_case.extra_files}, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", *test_case.command), + project_dir=project_dir, + ) + + output: str = result.stdout + result.stderr + assert result.returncode == test_case.expected_exit_code, test_case.description + output + assert "Traceback" not in output, test_case.description + output + assert all(fragment in result.stderr for fragment in test_case.expected_stderr_fragments), ( + test_case.description + output + ) diff --git a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_auditing.py b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_auditing.py index 5c02f6ec76..a248f7b307 100644 --- a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_auditing.py +++ b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_auditing.py @@ -42,7 +42,11 @@ from sqlbuild.cost.models import CostResourceContext from sqlbuild.executor.auditing.models import AuditExecutionResult from sqlbuild.executor.pipeline._helpers import auditing -from sqlbuild.executor.pipeline.exceptions import AuditExecutionError, AuditOutcomeError +from sqlbuild.executor.pipeline.exceptions import ( + AuditExecutionError, + AuditOutcomeError, + WarehouseConnectionError, +) from sqlbuild.executor.pipeline.main.run import ( AuditPipelineCallbacks, run_audit_pipeline, @@ -907,10 +911,13 @@ def test_given_partial_connection_open_when_later_open_fails_then_opened_connect test_case: AuditConcurrencyTestCase, ) -> None: adapter: Mock = Mock(spec=BaseAdapter) + adapter.adapter_name = "duckdb" opened: object = object() adapter.connect.side_effect = (opened, LookupError("open failure")) - with pytest.raises(LookupError, match="open failure"): + with pytest.raises( + WarehouseConnectionError, match="could not connect to the duckdb warehouse: open failure" + ): run_audit_pipeline( plan=PlanOutput(audit_entries=(audit_entry("first"), audit_entry("second"))), connection_config={}, diff --git a/tests/unit/src/sqlbuild/runtime/contracts/_test_types.py b/tests/unit/src/sqlbuild/runtime/contracts/_test_types.py index 7b431a5856..8c0b00e543 100644 --- a/tests/unit/src/sqlbuild/runtime/contracts/_test_types.py +++ b/tests/unit/src/sqlbuild/runtime/contracts/_test_types.py @@ -17,6 +17,7 @@ class OpenConnectionFailureTestCase: connection_config: dict[str, object] monotonic_times: tuple[float, float] expected_error: BaseException + expected_raised_type: type[BaseException] expected_event_order: tuple[str, ...] expected_elapsed_seconds: tuple[float, ...] expected_connection_counts: tuple[int, ...] diff --git a/tests/unit/src/sqlbuild/runtime/contracts/test_open_connection.py b/tests/unit/src/sqlbuild/runtime/contracts/test_open_connection.py index 5e8083d0e7..77f2ef8aa6 100644 --- a/tests/unit/src/sqlbuild/runtime/contracts/test_open_connection.py +++ b/tests/unit/src/sqlbuild/runtime/contracts/test_open_connection.py @@ -6,6 +6,7 @@ import pytest from sqlbuild._native import open_connection_with_hooks +from sqlbuild.executor.pipeline.exceptions import WarehouseConnectionError from sqlbuild.runtime.contracts.models import ConnectionHooks from tests.unit.src.sqlbuild.runtime.contracts._test_types import ( OpenConnectionFailureTestCase, @@ -75,10 +76,11 @@ def on_complete(connection_count: int, *, elapsed_seconds: int | float) -> None: "test_case", [ OpenConnectionFailureTestCase( - description="failed connection reports the original exception", + description="failed connection reports a coded error caused by the original", connection_config={"database": "unavailable.duckdb"}, monotonic_times=(20.0, 21.25), expected_error=RuntimeError("connection unavailable"), + expected_raised_type=WarehouseConnectionError, expected_event_order=("start", "connect", "error"), expected_elapsed_seconds=(1.25,), expected_connection_counts=(1, 1), @@ -88,6 +90,7 @@ def on_complete(connection_count: int, *, elapsed_seconds: int | float) -> None: connection_config={"database": "unavailable.duckdb"}, monotonic_times=(20.0, 21.25), expected_error=KeyboardInterrupt(), + expected_raised_type=KeyboardInterrupt, expected_event_order=("start", "connect"), expected_elapsed_seconds=(), expected_connection_counts=(1,), @@ -95,7 +98,7 @@ def on_complete(connection_count: int, *, elapsed_seconds: int | float) -> None: ], ids=lambda case: case.description, ) -def test_given_connection_hooks_when_opening_fails_then_reports_error_and_reraises_identity( +def test_given_connection_hooks_when_opening_fails_then_reports_error_and_raises_original_cause( test_case: OpenConnectionFailureTestCase, monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -118,7 +121,7 @@ def on_error(connection_count: int, *, elapsed_seconds: int | float) -> None: connection_counts.append(connection_count) elapsed_seconds_values.append(float(elapsed_seconds)) - with pytest.raises(type(test_case.expected_error)) as exc_info: + with pytest.raises(test_case.expected_raised_type) as exc_info: open_connection_with_hooks( adapter=adapter, connection_config=test_case.connection_config, @@ -128,7 +131,9 @@ def on_error(connection_count: int, *, elapsed_seconds: int | float) -> None: ), ) - assert exc_info.value is test_case.expected_error + assert test_case.expected_error in (exc_info.value, exc_info.value.__cause__), ( + test_case.description + ) assert adapter.connected_config is test_case.connection_config assert tuple(events) == test_case.expected_event_order assert tuple(elapsed_seconds_values) == test_case.expected_elapsed_seconds From fb7e68e1cbf3c1a8faa2bb9ce900130b341426ba Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:15:03 +0100 Subject: [PATCH 07/14] fix(diff): report unknown key columns and missing relations as coded errors sqb diff with a --key column that does not exist, or with the relation missing on either side, failed with a raw warehouse error and a traceback. Key columns are now checked against both relations (X311), and a failed schema read whose relation does not exist reports which side lacks it (X310). Refs: CHI-600 --- .../executor/diff/_helpers/execution.py | 109 +++++++++++++++++- .../cli/commands/main/diff/test_diff.py | 38 ++++++ 2 files changed, 142 insertions(+), 5 deletions(-) diff --git a/src/sqlbuild/executor/diff/_helpers/execution.py b/src/sqlbuild/executor/diff/_helpers/execution.py index 72d630ed94..fea5d32897 100644 --- a/src/sqlbuild/executor/diff/_helpers/execution.py +++ b/src/sqlbuild/executor/diff/_helpers/execution.py @@ -50,11 +50,21 @@ def execute_model_diff( unique_key = options.unique_key_override elif _model_has_unique_key(right_model): unique_key = get_unique_key(right_model) - schema_result: SchemaDiffResult = adapter.diff_schema( - connection=connection, - left=left_relation, - right=right_relation, - ) + try: + schema_result: SchemaDiffResult = adapter.diff_schema( + connection=connection, + left=left_relation, + right=right_relation, + ) + except Exception as error: + _raise_missing_relation( + adapter=adapter, + connection=connection, + name=name, + sides=(("FROM", left_model, left_relation), ("TO", right_model, right_relation)), + error=error, + ) + raise if options.max_columns is not None: observed_columns: int = max( schema_result.left_column_count, @@ -367,6 +377,12 @@ def _execute_model_row_diff( (*get_row_diff_exclude_columns(right_model), *options.excluded_columns_override) ) ) + _require_key_columns( + adapter=adapter, + connection=connection, + relations=(left_relation, right_relation), + keys=resolved_unique_key, + ) intersection: tuple[str, ...] = tuple( key for key in resolved_unique_key if key in excluded_columns ) @@ -516,3 +532,86 @@ def _model_has_unique_key(model: Any) -> bool: if isinstance(raw, list | tuple): return any(isinstance(item, str) and item for item in raw) return False + + +def _raise_missing_relation( + *, + adapter: BaseAdapter, + connection: Any, + name: str, + sides: tuple[tuple[str, Any, str], tuple[str, Any, str]], + error: Exception, +) -> None: + """Raise a coded error when a schema diff failed because one side's relation is missing.""" + + left, right = sides + left_missing: bool = _relation_missing( + adapter=adapter, connection=connection, model=left[1], error=error + ) + right_missing: bool = not left_missing and _relation_missing( + adapter=adapter, connection=connection, model=right[1], error=error + ) + missing: tuple[tuple[str, Any, str], ...] = tuple( + side for side, absent in ((left, left_missing), (right, right_missing)) if absent + ) + if not missing: + return + label, _, relation = missing[0] + raise ExecutorInputError( + f"model '{name}' has no relation {relation} in the {label} target", + code="X310", + help="Build the model in that target first, or leave it out of --select.", + ) from error + + +def _relation_missing( + *, adapter: BaseAdapter, connection: Any, model: Any, error: Exception +) -> bool: + """Whether the model's relation is absent; a failed check re-raises the diff's own error.""" + + try: + exists: bool = adapter.relation_exists( + connection=connection, + database=model.destination.database, + schema=model.destination.schema, + name=model.destination.name, + ) + except Exception: + raise error # noqa: B904 + return not exists + + +def _require_key_columns( + *, + adapter: BaseAdapter, + connection: Any, + relations: tuple[str, str], + keys: tuple[str, ...], +) -> None: + """Reject row diff key columns that one of the relations does not have.""" + + if not keys: + return + left, right = relations + _require_relation_keys(adapter=adapter, connection=connection, relation=left, keys=keys) + _require_relation_keys(adapter=adapter, connection=connection, relation=right, keys=keys) + + +def _require_relation_keys( + *, adapter: BaseAdapter, connection: Any, relation: str, keys: tuple[str, ...] +) -> None: + columns: set[str] = { + column.name.lower() + for column in adapter.describe_relation(connection=connection, relation=relation) + } + missing: tuple[str, ...] = tuple(key for key in keys if key.lower() not in columns) + if not missing: + return + noun: str = "column" if len(missing) == 1 else "columns" + verb: str = "does" if len(missing) == 1 else "do" + raise ExecutorInputError( + f"row diff key {noun} {', '.join(repr(key) for key in missing)} " + f"{verb} not exist in {relation}", + code="X311", + help="Pass --key with columns both relations have, or fix the model's unique_key.", + ) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/diff/test_diff.py b/tests/e2e/src/sqlbuild/cli/commands/main/diff/test_diff.py index 18bbf2c089..3e0e6b856f 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/diff/test_diff.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/diff/test_diff.py @@ -432,6 +432,44 @@ expected_exit_code=1, expected_stderr_fragments=("model 'daily_revenue' requires unique_key for row diff",), ), + DiffCommandE2ETestCase( + description="an unknown cli key column fails with a coded error", + command=( + "--no-color", + "diff", + "prod:dev", + "--full", + "--key", + "nope", + "--select", + "daily_revenue", + ), + expected_exit_code=1, + expected_stderr_fragments=( + "error[X311]", + "row diff key column 'nope' does not exist in prod.daily_revenue", + ), + ), + DiffCommandE2ETestCase( + description="a relation missing on the FROM side fails with a coded error", + mutation_sql=("DROP TABLE prod.daily_revenue",), + command=("--no-color", "diff", "prod:dev", "--full", "--select", "daily_revenue"), + expected_exit_code=1, + expected_stderr_fragments=( + "error[X310]", + "model 'daily_revenue' has no relation prod.daily_revenue in the FROM target", + ), + ), + DiffCommandE2ETestCase( + description="a relation missing on the TO side fails with a coded error", + mutation_sql=("DROP TABLE dev.daily_revenue",), + command=("--no-color", "diff", "prod:dev", "--full", "--select", "daily_revenue"), + expected_exit_code=1, + expected_stderr_fragments=( + "error[X310]", + "model 'daily_revenue' has no relation dev.daily_revenue in the TO target", + ), + ), DiffCommandE2ETestCase( description="cli key enables model without declared unique key", command=( From 6ea7906769b84aa4b05caf5dfa0080d70959e674 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:15:16 +0100 Subject: [PATCH 08/14] fix(plan): draw the tree bar under detail rows of non-last entries Detail rows under every plan entry used a blank prefix, so the vertical bar of a non-last entry broke under its cursor, bounds, reason and diff rows. Detail and nested rows now carry the bar when their entry has a later sibling. Refs: CHI-533 --- .../src/cli_text/_helpers/tree_rows.rs | 49 ++++++++++++++++--- .../sqlbuild-core/src/cli_text/constants.rs | 5 ++ .../src/cli_text/tests/test_cli_text.rs | 12 +++++ .../sqlbuild/references/docs/cli/plan.md | 10 ++-- .../test_identity_handover.py | 10 ++-- .../pre_build/test_pre_build_work_display.py | 4 +- website/src/content/docs/docs/cli/plan.mdx | 10 ++-- 7 files changed, 76 insertions(+), 24 deletions(-) diff --git a/crates/sqlbuild-core/src/cli_text/_helpers/tree_rows.rs b/crates/sqlbuild-core/src/cli_text/_helpers/tree_rows.rs index db02ee17d0..ff47cb2eb3 100644 --- a/crates/sqlbuild-core/src/cli_text/_helpers/tree_rows.rs +++ b/crates/sqlbuild-core/src/cli_text/_helpers/tree_rows.rs @@ -3,7 +3,8 @@ use crate::cli_text::_helpers::bounded_sections::strip_ansi; use crate::cli_text::_helpers::layout::tree_connector; use crate::cli_text::constants::{ - ENTRY_ROW_PREFIX, LEAF_KEY_SEPARATOR, LEAF_ROW_PREFIX, NESTED_ROW_PREFIX, TREE_EXEMPT_CHARS, + ENTRY_ROW_PREFIX, LEAF_KEY_SEPARATOR, LEAF_ROW_PREFIX, NESTED_ROW_PREFIX, STYLE_ESCAPE_END, + STYLE_ESCAPE_START, TREE_CONTINUATION_GLYPH, TREE_EXEMPT_CHARS, }; use crate::cli_text::models::{CliStyle, RowKind}; @@ -42,19 +43,49 @@ fn is_last(kinds: &[RowKind], index: usize, row_kind: RowKind) -> bool { true } +/// A nested row whose leaf indentation becomes a non-last entry's bar, keeping style escapes. +fn continued(line: String, continuation: Option<&str>) -> String { + let Some(continuation) = continuation else { + return line; + }; + let mut kept: String = String::with_capacity(line.len()); + let mut removed: usize = 0; + let mut characters = line.chars().peekable(); + while removed < LEAF_ROW_PREFIX.len() { + match characters.next() { + Some(STYLE_ESCAPE_START) => { + kept.push(STYLE_ESCAPE_START); + for character in characters.by_ref() { + kept.push(character); + if character == STYLE_ESCAPE_END { + break; + } + } + } + Some(' ') => removed += 1, + _ => return line, + } + } + kept.extend(characters); + format!("{continuation}{kept}") +} + /// `_treeify_plan_lines`: entry rows and their detail rows drawn with tree connectors. pub(crate) fn treeify(lines: Vec) -> Vec { let style: CliStyle = CliStyle { use_color: true }; let kinds: Vec = lines.iter().map(|line| classify(line)).collect(); let mut out: Vec = Vec::with_capacity(lines.len()); + let mut bar: Option = None; for (index, line) in lines.into_iter().enumerate() { match kinds[index] { RowKind::Entry => { - let connector: String = - tree_connector(style, is_last(&kinds, index, RowKind::Entry)); + let last: bool = is_last(&kinds, index, RowKind::Entry); + bar = (!last).then(|| style.muted(TREE_CONTINUATION_GLYPH)); + let connector: String = tree_connector(style, last); out.push(format!("{connector} {}", &line[ENTRY_ROW_PREFIX.len()..])); } RowKind::Leaf => { + let continuation: &str = bar.as_deref().unwrap_or(LEAF_ROW_PREFIX); let connector: String = tree_connector(style, is_last(&kinds, index, RowKind::Leaf)); let content: String = match line.strip_prefix(LEAF_ROW_PREFIX) { @@ -63,19 +94,23 @@ pub(crate) fn treeify(lines: Vec) -> Vec { }; if let Some((key, value)) = content.split_once(LEAF_KEY_SEPARATOR) { out.push(format!( - "{LEAF_ROW_PREFIX}{connector} {} {value}", + "{continuation}{connector} {} {value}", style.muted(key) )); } else if content.ends_with(':') { out.push(format!( - "{LEAF_ROW_PREFIX}{connector} {}", + "{continuation}{connector} {}", style.muted(&content) )); } else { - out.push(format!("{LEAF_ROW_PREFIX}{connector} {content}")); + out.push(format!("{continuation}{connector} {content}")); } } - RowKind::Nested | RowKind::Other => out.push(line), + RowKind::Nested => out.push(continued(line, bar.as_deref())), + RowKind::Other => { + bar = None; + out.push(line); + } } } out diff --git a/crates/sqlbuild-core/src/cli_text/constants.rs b/crates/sqlbuild-core/src/cli_text/constants.rs index c356a7ab7b..483b68b65d 100644 --- a/crates/sqlbuild-core/src/cli_text/constants.rs +++ b/crates/sqlbuild-core/src/cli_text/constants.rs @@ -16,6 +16,11 @@ pub(crate) const PHASE_OK_GLYPH: &str = "\u{2713}"; pub(crate) const PHASE_FAIL_GLYPH: &str = "\u{2717}"; pub(crate) const TREE_BRANCH_GLYPH: &str = "\u{251c}\u{2500}\u{2500}"; pub(crate) const TREE_LAST_GLYPH: &str = "\u{2514}\u{2500}\u{2500}"; +/// The bar a non-last entry's detail rows carry under its connector. +pub(crate) const TREE_CONTINUATION_GLYPH: &str = "\u{2502} "; +/// The start of a terminal style escape and the character that ends it. +pub(crate) const STYLE_ESCAPE_START: char = '\u{1b}'; +pub(crate) const STYLE_ESCAPE_END: char = 'm'; /// The human report lists at most this many models. pub(crate) const HUMAN_MODEL_LIMIT: usize = 100; pub(crate) const MIN_MODEL_NAME_WIDTH: usize = 24; diff --git a/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs b/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs index 3e2632fd59..5531a03b4e 100644 --- a/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs +++ b/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs @@ -158,6 +158,18 @@ fn given_plan_report_when_rendering_then_groups_models_by_reason() { expected_text: "Plan ready 3 selected\n\nQuery changed (1)\n\u{2514}\u{2500}\u{2500} orders full rebuild\n \u{251c}\u{2500}\u{2500} policy replay_on_change=full\n \u{2514}\u{2500}\u{2500} query diff:\n -SELECT 1\n +SELECT 2\n\nModels (1)\n\u{2514}\u{2500}\u{2500} customers table\n\nWarnings (1)\n\u{2514}\u{2500}\u{2500} orders\n \u{251c}\u{2500}\u{2500} orders lag\n \u{2514}\u{2500}\u{2500} second line", expected_completion: "\u{2713} Plan complete 3 selected", }, + PlanTextTestCase { + description: "detail rows under a non-last entry keep the tree's vertical bar", + report: plan_report(vec![ + ModelPlanText { + query_diff: Some(vec![" -SELECT 1".to_owned()]), + ..table_model("orders", "query_changed") + }, + table_model("customers", "query_changed"), + ]), + expected_text: "Plan ready 2 selected\n\nQuery changed (2)\n\u{251c}\u{2500}\u{2500} orders full rebuild\n\u{2502} \u{251c}\u{2500}\u{2500} policy replay_on_change=full\n\u{2502} \u{2514}\u{2500}\u{2500} query diff:\n\u{2502} -SELECT 1\n\u{2514}\u{2500}\u{2500} customers full rebuild\n \u{2514}\u{2500}\u{2500} policy replay_on_change=full", + expected_completion: "\u{2713} Plan complete 2 selected", + }, PlanTextTestCase { description: "every active model is a full refresh", report: plan_report(vec![ diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md index 808e85b2f5..d094dda1e6 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md @@ -60,7 +60,7 @@ Sources to load (2) Changed functions (2) ├── table_fn__customer_orders table function - └── reason first run +│ └── reason first run └── udf__is_completed_order sql udf └── reason first run @@ -73,11 +73,11 @@ First run (12) ├── dim_customers table ├── fact_orders table ├── customer_status_snapshot merge (timestamp) - ├── cursor last_ordered_at (timestamp) - └── bounds computed at run time, once model fact_orders is up to date +│ ├── cursor last_ordered_at (timestamp) +│ └── bounds computed at run time, once model fact_orders is up to date ├── hourly_order_activity delete_insert (timestamp, microbatch) - ├── cursor activity_hour (timestamp) - ├── mode microbatch +│ ├── cursor activity_hour (timestamp) +│ ├── mode microbatch ... └── order_status_index delete_insert (integer) ├── cursor order_id (integer) diff --git a/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/test_identity_handover.py b/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/test_identity_handover.py index 7802a14a9d..3e3b1b0c4b 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/test_identity_handover.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/test_identity_handover.py @@ -148,12 +148,12 @@ def test_given_renamed_chain_with_full_replay_when_building_then_migrated_histor description="plan lists a renamed view as a migration that recreates it", expected_fragment=( "├── customer_orders_enriched migrate main.orders_enriched -> " - "main.customer_orders_enriched\n ├── transfer recreate (view)\n" - " ├── discovery automatic\n" - " ├── hint matched by unchanged definition; sqb rename " + "main.customer_orders_enriched\n│ ├── transfer recreate (view)\n" + "│ ├── discovery automatic\n" + "│ ├── hint matched by unchanged definition; sqb rename " "rewrites references and declares migrate_from so the move does not depend on discovery\n" - " └── old name main.orders_enriched\n" - " └── left for janitor old_name_view false" + "│ └── old name main.orders_enriched\n" + "│ └── left for janitor old_name_view false" ), ) ], diff --git a/tests/unit/src/sqlbuild/compiler/planner/main/pre_build/test_pre_build_work_display.py b/tests/unit/src/sqlbuild/compiler/planner/main/pre_build/test_pre_build_work_display.py index f3c1786018..83936e3c0c 100644 --- a/tests/unit/src/sqlbuild/compiler/planner/main/pre_build/test_pre_build_work_display.py +++ b/tests/unit/src/sqlbuild/compiler/planner/main/pre_build/test_pre_build_work_display.py @@ -207,8 +207,8 @@ def test_given_colour_off_when_formatting_plan_then_migration_text_is_plain( expected_text=( "Column migrations (3)\n" "├── fact_orders migrate columns\n" - " ├── amount -> revenue rename in place (automatic)\n" - " └── tax -> tax_amount already renamed, record\n" + "│ ├── amount -> revenue rename in place (automatic)\n" + "│ └── tax -> tax_amount already renamed, record\n" "└── order_history migrate columns\n" " └── status -> order_status rename in place" ), diff --git a/website/src/content/docs/docs/cli/plan.mdx b/website/src/content/docs/docs/cli/plan.mdx index 4cb337be29..582c72b52c 100644 --- a/website/src/content/docs/docs/cli/plan.mdx +++ b/website/src/content/docs/docs/cli/plan.mdx @@ -51,7 +51,7 @@ Sources to load (2) Changed functions (2) ├── table_fn__customer_orders table function - └── reason first run +│ └── reason first run └── udf__is_completed_order sql udf └── reason first run @@ -64,11 +64,11 @@ First run (12) ├── dim_customers table ├── fact_orders table ├── customer_status_snapshot merge (timestamp) - ├── cursor last_ordered_at (timestamp) - └── bounds computed at run time, once model fact_orders is up to date +│ ├── cursor last_ordered_at (timestamp) +│ └── bounds computed at run time, once model fact_orders is up to date ├── hourly_order_activity delete_insert (timestamp, microbatch) - ├── cursor activity_hour (timestamp) - ├── mode microbatch +│ ├── cursor activity_hour (timestamp) +│ ├── mode microbatch ... └── order_status_index delete_insert (integer) ├── cursor order_id (integer) From a60b41b511b1bd626697f4449991f04407833f0b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:15:16 +0100 Subject: [PATCH 09/14] fix(build): show target and outcome on standalone audit rows Seed, source and end audits printed as 'audit not_null OK', without their target, and a failing warn-severity audit still showed OK. Their rows now name the target and column, as in 'not_null (customers.customer_id) PASS', and show PASS, WARN, FAIL or INSUFFICIENT from the audit outcome. The partial semantic checks notice also says '1 model' instead of '1 models'. Refs: CHI-519 --- .../cli_text/main/semantic_coverage_notice.rs | 7 ++- .../src/cli_text/tests/test_cli_text.rs | 7 +++ .../src/progress/_helpers/resources.rs | 44 +++++++++++++- .../sqlbuild-core/src/progress/constants.rs | 10 ++++ crates/sqlbuild-core/src/progress/models.rs | 8 +++ .../src/progress/tests/helpers.rs | 33 ++++++++++- .../src/progress/tests/test_progress.rs | 58 ++++++++++++++++++- .../src/progress/tests/test_types.rs | 8 +++ .../src/bindings/_helpers/cli/progress.rs | 8 +++ src/sqlbuild/_native.pyi | 1 + .../main/_record_audit_progress_outcome.py | 18 ++++++ .../executor/build/_helpers/end_audits.py | 6 ++ .../executor/build/_helpers/source_audits.py | 6 ++ .../executor/pipeline/_helpers/auditing.py | 6 ++ .../cli/commands/main/build/_test_types.py | 8 +++ .../cli/commands/main/build/helpers.py | 27 +++++++++ .../main/build/test_audit_failures.py | 48 +++++++++++++++ .../classes/test_native_progress_projector.py | 12 ++-- 18 files changed, 301 insertions(+), 14 deletions(-) create mode 100644 src/sqlbuild/executor/auditing/main/_record_audit_progress_outcome.py diff --git a/crates/sqlbuild-core/src/cli_text/main/semantic_coverage_notice.rs b/crates/sqlbuild-core/src/cli_text/main/semantic_coverage_notice.rs index e8cfe029d2..e3b5712f1d 100644 --- a/crates/sqlbuild-core/src/cli_text/main/semantic_coverage_notice.rs +++ b/crates/sqlbuild-core/src/cli_text/main/semantic_coverage_notice.rs @@ -2,6 +2,8 @@ use std::collections::BTreeMap; +use crate::cli_text::_helpers::summary_text::count_noun; + use crate::cli_text::constants::{ DEFAULT_ACTION, INVALID_UPSTREAM, NOTICE_DISPLAY_LIMIT, OPEN_INPUT, OPEN_SOURCE, }; @@ -53,9 +55,10 @@ pub fn semantic_coverage_notice(reasons: &[(String, Vec)]) -> Option = vec![format!( - "Semantic checks were partial for {} models:", - reasons.len() + "Semantic checks were partial for {}:", + count_noun(count, "model", None) )]; for (reason, models) in groups.iter().take(NOTICE_DISPLAY_LIMIT) { let suffix: String = if models.len() > NOTICE_DISPLAY_LIMIT { diff --git a/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs b/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs index 5531a03b4e..4bc1ad7077 100644 --- a/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs +++ b/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs @@ -78,6 +78,13 @@ fn given_partial_reasons_when_building_notice_then_groups_by_reason() { "Semantic checks were partial for 2 models:\n zeta: SQL analysis disabled; enable sql_analysis, then run `sqb compile --no-cache`.\n alpha, zeta: reads open source raw_orders; run `sqb contract generate --from --select source:raw_orders --write` and enforce the complete source contract.\n alpha: weird; declare the input shape, then run `sqb compile --no-cache`.", ), }, + NoticeTestCase { + description: "one partial model is counted in the singular", + reasons: vec![("alpha".to_owned(), vec!["SQL analysis disabled".to_owned()])], + expected_notice: Some( + "Semantic checks were partial for 1 model:\n alpha: SQL analysis disabled; enable sql_analysis, then run `sqb compile --no-cache`.", + ), + }, NoticeTestCase { description: "complete checks print no notice", reasons: Vec::new(), diff --git a/crates/sqlbuild-core/src/progress/_helpers/resources.rs b/crates/sqlbuild-core/src/progress/_helpers/resources.rs index f1ca1aeaca..32c55fa84a 100644 --- a/crates/sqlbuild-core/src/progress/_helpers/resources.rs +++ b/crates/sqlbuild-core/src/progress/_helpers/resources.rs @@ -4,7 +4,8 @@ use std::collections::VecDeque; use crate::cli_text::models::CliStyle; use crate::progress::constants::{ - FAILED_SUFFIX, KIND_WIDTH, SCENARIO_RESOURCE_PREFIX, SKIPPED_SUFFIX, + AUDIT_OUTCOME_STATUSES, AUDIT_RESOURCE_KIND, AUDIT_RESOURCE_PREFIX, FAILED_SUFFIX, KIND_WIDTH, + SCENARIO_RESOURCE_PREFIX, SKIPPED_SUFFIX, }; use crate::progress::models::{ProgressEvent, ProgressProjection}; use crate::terminal_events::models::TerminalRequest; @@ -19,6 +20,27 @@ fn display_resource_name(resource_id: Option<&str>, resource_name: &str) -> Stri format!("{scenario_name}/{resource_name}") } +/// `name (target.column)` from an audit identifier `audit::[:[:]]`. +fn audit_display_name(resource_id: Option<&str>, resource_name: &str) -> String { + let Some(identity) = resource_id.and_then(|id| id.strip_prefix(AUDIT_RESOURCE_PREFIX)) else { + return resource_name.to_owned(); + }; + let attached: Vec<&str> = identity.split(':').skip(2).collect(); + if attached.is_empty() { + return resource_name.to_owned(); + } + format!("{resource_name} ({})", attached.join(".")) +} + +/// The row name of one resource: audits name their target, scenario resources their scenario. +fn resource_display_name(kind: &str, resource_id: Option<&str>, resource_name: &str) -> String { + if kind == AUDIT_RESOURCE_KIND { + audit_display_name(resource_id, resource_name) + } else { + display_resource_name(resource_id, resource_name) + } +} + /// Remember an attempt, keeping the position of its first sighting. fn store_attempt( mut projection: ProgressProjection, @@ -84,7 +106,8 @@ pub(crate) fn consume_resource_start( use_color: projection.settings.use_color, } .status("START"); - let display_name: String = display_resource_name(Some(resource_id), resource_name); + let display_name: String = + resource_display_name(resource_kind, Some(resource_id), resource_name); lines.push(format!( " {counter}{resource_kind: = projection + .audit_outcomes + .get(attempt_id) + .and_then(|outcome| { + AUDIT_OUTCOME_STATUSES + .iter() + .find(|(value, _)| value == outcome) + .map(|(_, status)| *status) + }); let status_text: &str = if event.event_type.ends_with(FAILED_SUFFIX) { "FAIL" } else if event.event_type.ends_with(SKIPPED_SUFFIX) { "SKIP" + } else if let Some(status) = recorded { + status + } else if audit { + "PASS" } else { "OK" }; @@ -148,7 +185,8 @@ pub(crate) fn render_resource_terminal( .duration_ms .map(|duration| format!(" {:.2}s", duration / 1000.0)) .unwrap_or_default(); - let display_name: String = display_resource_name(event.resource_id.as_deref(), resource_name); + let display_name: String = + resource_display_name(resource_kind, event.resource_id.as_deref(), resource_name); let line: String = format!(" {resource_kind:, pub(crate) quiet_operation_ids: HashSet, pub(crate) enriched_resource_names: HashSet, + /// Audit outcomes by attempt, which the audit's own terminal row shows. + pub(crate) audit_outcomes: HashMap, pub(crate) resource_names: HashMap, pub(crate) statement_starts: HashMap, pub(crate) statement_query_ids: HashMap, @@ -108,6 +110,12 @@ impl ProgressProjection { .insert(resource_name.to_owned()); } + /// Record how one audit attempt ended, before its terminal row renders. + pub fn record_audit_outcome(&mut self, resource_attempt_id: &str, outcome: &str) { + self.audit_outcomes + .insert(resource_attempt_id.to_owned(), outcome.to_owned()); + } + /// Whether a visible operation with this name currently owns progress. #[must_use] pub fn is_operation_active(&self, operation_name: &str) -> bool { diff --git a/crates/sqlbuild-core/src/progress/tests/helpers.rs b/crates/sqlbuild-core/src/progress/tests/helpers.rs index a654c48407..bdbd5d6d28 100644 --- a/crates/sqlbuild-core/src/progress/tests/helpers.rs +++ b/crates/sqlbuild-core/src/progress/tests/helpers.rs @@ -4,7 +4,7 @@ use crate::progress::main::consume_progress_event::consume_progress_event; use crate::progress::models::{ ProgressEvent, ProgressPayload, ProgressProjection, ProgressSettings, }; -use crate::progress::tests::test_types::ProjectionTestCase; +use crate::progress::tests::test_types::{AuditRowTestCase, ProjectionTestCase}; use crate::terminal_events::models::TerminalRequest; /// One `orders` resource attempt event. @@ -84,3 +84,34 @@ pub(super) fn orders_claim() -> TerminalRequest { pub(super) fn plain() -> ProgressSettings { ProgressSettings::default() } + +/// The start and terminal rows of one standalone `not_null` audit attempt. +pub(super) fn audit_rows(test_case: &AuditRowTestCase) -> Vec { + let mut projection: ProgressProjection = ProgressProjection::new(plain()); + for (attempt_id, outcome) in &test_case.recorded_outcomes { + projection.record_audit_outcome(attempt_id, outcome); + } + let mut lines: Vec = Vec::new(); + for (event_id, event_type) in [ + ("e1", "resource_attempt_started"), + ("e2", test_case.terminal_event), + ] { + let event: ProgressEvent = ProgressEvent { + event_id: event_id.to_owned(), + event_type: event_type.to_owned(), + resource_id: Some(test_case.resource_id.to_owned()), + resource_attempt_id: Some("a1".to_owned()), + payload: ProgressPayload { + resource_name: Some("not_null".to_owned()), + resource_kind: Some("audit".to_owned()), + duration_ms: Some(10.0), + ..ProgressPayload::default() + }, + ..ProgressEvent::default() + }; + let (consumed, event_lines) = consume_progress_event(projection, &event); + projection = consumed; + lines.extend(event_lines); + } + lines +} diff --git a/crates/sqlbuild-core/src/progress/tests/test_progress.rs b/crates/sqlbuild-core/src/progress/tests/test_progress.rs index a3ff9d8de3..9ffb9e3b11 100644 --- a/crates/sqlbuild-core/src/progress/tests/test_progress.rs +++ b/crates/sqlbuild-core/src/progress/tests/test_progress.rs @@ -1,8 +1,8 @@ use crate::progress::models::{ProgressEvent, ProgressPayload, ProgressSettings}; use crate::progress::tests::helpers::{ - attempt_event, operation_event, orders_claim, plain, project_case, + attempt_event, audit_rows, operation_event, orders_claim, plain, project_case, }; -use crate::progress::tests::test_types::ProjectionTestCase; +use crate::progress::tests::test_types::{AuditRowTestCase, ProjectionTestCase}; #[test] fn given_lifecycle_facts_when_projecting_then_progress_lines_match() { @@ -211,3 +211,57 @@ fn given_lifecycle_facts_when_projecting_then_progress_lines_match() { ); } } + +#[test] +fn given_standalone_audit_when_projecting_then_row_names_target_and_outcome() { + let test_cases = [ + AuditRowTestCase { + description: "a seed column audit names its target and passes", + resource_id: "audit:not_null:seed:customers:customer_id", + terminal_event: "resource_attempt_completed", + recorded_outcomes: vec![("a1", "pass")], + expected_lines: vec![ + " audit not_null (customers.customer_id) START", + " audit not_null (customers.customer_id) PASS 0.01s", + ], + }, + AuditRowTestCase { + description: "a completed audit with a warn outcome shows WARN", + resource_id: "audit:not_null:source:raw_orders:order_id", + terminal_event: "resource_attempt_completed", + recorded_outcomes: vec![("a1", "warn")], + expected_lines: vec![ + " audit not_null (raw_orders.order_id) START", + " audit not_null (raw_orders.order_id) WARN 0.01s", + ], + }, + AuditRowTestCase { + description: "a failed singular audit has no target and shows FAIL", + resource_id: "audit:not_null:singular", + terminal_event: "resource_attempt_failed", + recorded_outcomes: vec![("a1", "error")], + expected_lines: vec![ + " audit not_null START", + " audit not_null FAIL 0.01s", + ], + }, + AuditRowTestCase { + description: "a completed audit without its own recorded outcome passes", + resource_id: "audit:not_null:model:orders", + terminal_event: "resource_attempt_completed", + recorded_outcomes: vec![("a2", "warn")], + expected_lines: vec![ + " audit not_null (orders) START", + " audit not_null (orders) PASS 0.01s", + ], + }, + ]; + for test_case in test_cases { + assert_eq!( + audit_rows(&test_case), + test_case.expected_lines, + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-core/src/progress/tests/test_types.rs b/crates/sqlbuild-core/src/progress/tests/test_types.rs index 0a9e9d687d..1606615f18 100644 --- a/crates/sqlbuild-core/src/progress/tests/test_types.rs +++ b/crates/sqlbuild-core/src/progress/tests/test_types.rs @@ -49,3 +49,11 @@ pub(super) struct StatusSpinnerLineTestCase { pub(super) style: CliStyle, pub(super) expected_line: &'static str, } + +pub(super) struct AuditRowTestCase { + pub(super) description: &'static str, + pub(super) resource_id: &'static str, + pub(super) terminal_event: &'static str, + pub(super) recorded_outcomes: Vec<(&'static str, &'static str)>, + pub(super) expected_lines: Vec<&'static str>, +} diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/progress.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/progress.rs index e7d1df94fb..5878950bff 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/progress.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/progress.rs @@ -268,6 +268,14 @@ impl NativeProgressProjector { Ok(()) } + /// Record how one audit attempt ended, so its terminal row shows the outcome. + #[pyo3(signature = (*, resource_attempt_id, outcome))] + fn record_audit_outcome(&self, resource_attempt_id: &str, outcome: &str) -> PyResult<()> { + self.locked()? + .record_audit_outcome(resource_attempt_id, outcome); + Ok(()) + } + /// Return whether a visible canonical operation currently owns progress. #[pyo3(name = "is_operation_active", signature = (*, operation_name))] pub(crate) fn operation_active(&self, operation_name: &str) -> PyResult { diff --git a/src/sqlbuild/_native.pyi b/src/sqlbuild/_native.pyi index a95aa3d592..5acdf8889c 100644 --- a/src/sqlbuild/_native.pyi +++ b/src/sqlbuild/_native.pyi @@ -3045,6 +3045,7 @@ class NativeProgressProjector: def restore(self, token: Token[NativeProgressProjector | None]) -> None: ... def configure_resources(self, *, ordinals: Mapping[str, int], total: int) -> None: ... def expect_resource_enrichment(self, *, resource_name: str) -> None: ... + def record_audit_outcome(self, *, resource_attempt_id: str, outcome: str) -> None: ... def is_operation_active(self, *, operation_name: str) -> bool: ... def consume(self, event: LifecycleEvent) -> None: ... def consume_resource_terminal( diff --git a/src/sqlbuild/executor/auditing/main/_record_audit_progress_outcome.py b/src/sqlbuild/executor/auditing/main/_record_audit_progress_outcome.py new file mode 100644 index 0000000000..a8b728d177 --- /dev/null +++ b/src/sqlbuild/executor/auditing/main/_record_audit_progress_outcome.py @@ -0,0 +1,18 @@ +"""Audit outcomes for the progress rows of standalone audits.""" + +from __future__ import annotations + +from sqlbuild._native import NativeProgressProjector, current_native_progress_projector +from sqlbuild.executor.auditing.models import AuditExecutionResult + + +def record_audit_progress_outcome( + *, resource_attempt_id: str, result: AuditExecutionResult +) -> None: + """Show the audit's outcome, such as WARN, on its terminal row in the active progress.""" + + projector: NativeProgressProjector | None = current_native_progress_projector() + if projector is not None: + projector.record_audit_outcome( + resource_attempt_id=resource_attempt_id, outcome=result.outcome.value + ) diff --git a/src/sqlbuild/executor/build/_helpers/end_audits.py b/src/sqlbuild/executor/build/_helpers/end_audits.py index b49ea7f544..db077aac03 100644 --- a/src/sqlbuild/executor/build/_helpers/end_audits.py +++ b/src/sqlbuild/executor/build/_helpers/end_audits.py @@ -15,6 +15,9 @@ from sqlbuild.executor.auditing.main._execute_reporting_errors import ( execute_audit_reporting_errors, ) +from sqlbuild.executor.auditing.main._record_audit_progress_outcome import ( + record_audit_progress_outcome, +) from sqlbuild.executor.auditing.main.publish_completed_audit_results import ( publish_completed_audit_results, ) @@ -62,6 +65,9 @@ def run_end_audits( run_scope_phase=AuditRunScope.FINAL, quality_scope=quality_scope, ) + record_audit_progress_outcome( + resource_attempt_id=lifecycle.resource_attempt_id, result=result + ) if result.outcome == AuditOutcome.ERROR: lifecycle.failed() results.append(result) diff --git a/src/sqlbuild/executor/build/_helpers/source_audits.py b/src/sqlbuild/executor/build/_helpers/source_audits.py index b43fa5aa2d..0942ce4e50 100644 --- a/src/sqlbuild/executor/build/_helpers/source_audits.py +++ b/src/sqlbuild/executor/build/_helpers/source_audits.py @@ -16,6 +16,9 @@ from sqlbuild.executor.auditing.main._execute_reporting_errors import ( execute_audit_reporting_errors, ) +from sqlbuild.executor.auditing.main._record_audit_progress_outcome import ( + record_audit_progress_outcome, +) from sqlbuild.executor.auditing.main.publish_completed_audit_results import ( publish_completed_audit_results, ) @@ -83,6 +86,9 @@ def run_pending_source_audits( run_scope_phase=AuditRunScope.FINAL, quality_scope="source", ) + record_audit_progress_outcome( + resource_attempt_id=lifecycle.resource_attempt_id, result=result + ) if result.outcome == AuditOutcome.ERROR: lifecycle.failed() audit_results.append(result) diff --git a/src/sqlbuild/executor/pipeline/_helpers/auditing.py b/src/sqlbuild/executor/pipeline/_helpers/auditing.py index 1065a65c81..ceae487286 100644 --- a/src/sqlbuild/executor/pipeline/_helpers/auditing.py +++ b/src/sqlbuild/executor/pipeline/_helpers/auditing.py @@ -22,6 +22,9 @@ from sqlbuild.diagnostics.main.diagnostics_context import diagnostics_context from sqlbuild.executor.auditing.main._execute import execute_audit from sqlbuild.executor.auditing.main._project_results import project_audit_result_batch +from sqlbuild.executor.auditing.main._record_audit_progress_outcome import ( + record_audit_progress_outcome, +) from sqlbuild.executor.auditing.main.audit_result_publication_scope import ( audit_result_publication_scope, ) @@ -580,6 +583,9 @@ def _execute_entry( run_scope_phase=AuditRunScope.FINAL, quality_scope="standalone", ) + record_audit_progress_outcome( + resource_attempt_id=lifecycle.resource_attempt_id, result=result + ) if result.outcome == AuditOutcome.ERROR: lifecycle.failed() return result diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py index 118451fa56..7b30f393d6 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py @@ -1345,3 +1345,11 @@ class WarehouseConnectionFailureE2ETestCase: expected_exit_code: int expected_stderr_fragments: tuple[str, ...] extra_files: dict[str, str] = field(default_factory=dict) + + +@dataclass(frozen=True) +class StandaloneAuditRowE2ETestCase: + description: str + status_values: tuple[str, ...] + expected_exit_code: int + expected_rows: tuple[str, ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/build/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/build/helpers.py index c3856bf578..98556b8bf9 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/build/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/build/helpers.py @@ -1115,3 +1115,30 @@ def dynamic_category_project(*, tmp_path: Path, name_pattern_entry: str) -> Path ), }, ) + + +def seed_audit_files(status_values: tuple[str, ...]) -> dict[str, str]: + """A seed with a not_null audit and a warn-severity accepted_values audit.""" + + return { + "sqlbuild_project.toml": ( + 'name = "seed_audit_rows"\nadapter = "duckdb"\ndefault_target = "dev"\n\n' + '[connection]\ndatabase = "seed_audits.duckdb"\n\n[targets.dev]\nschema = "dev"\n' + ), + "seeds/order_statuses.csv": "status_id,status\n1,placed\n2,shipped\n", + "seeds/order_statuses.yml": ( + "seeds:\n" + " - name: order_statuses\n" + " description: Order statuses.\n" + " columns:\n" + " - name: status_id\n" + " type: INTEGER\n" + " audits: [not_null]\n" + " - name: status\n" + " type: VARCHAR\n" + " audits:\n" + " - accepted_values:\n" + f" values: [{', '.join(status_values)}]\n" + " severity: warn\n" + ), + } diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/build/test_audit_failures.py b/tests/e2e/src/sqlbuild/cli/commands/main/build/test_audit_failures.py index 6a8674fac6..30e4ba2ce0 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/build/test_audit_failures.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/build/test_audit_failures.py @@ -11,7 +11,9 @@ from tests.e2e.src.sqlbuild.cli.commands.main.build._test_types import ( AuditFailureBuildE2ETestCase, MeasurementAuditCliE2ETestCase, + StandaloneAuditRowE2ETestCase, ) +from tests.e2e.src.sqlbuild.cli.commands.main.build.helpers import seed_audit_files from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import ( prepare_inline_project, run_sqb, @@ -265,3 +267,49 @@ def test_given_measurement_audit_when_running_cli_then_outcome_controls_exit_and assert result.returncode == test_case.expected_exit_code assert test_case.expected_output in result.stdout assert "FAIL" in result.stdout + + +@pytest.mark.parametrize( + "test_case", + [ + StandaloneAuditRowE2ETestCase( + description="passing seed audits name their column and show PASS", + status_values=("placed", "shipped"), + expected_exit_code=0, + expected_rows=( + " audit not_null (order_statuses.status_id) PASS", + " audit accepted_values (order_statuses.status) PASS", + ), + ), + StandaloneAuditRowE2ETestCase( + description="a failing warn seed audit shows WARN", + status_values=("placed",), + expected_exit_code=0, + expected_rows=( + " audit not_null (order_statuses.status_id) PASS", + " audit accepted_values (order_statuses.status) WARN", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_seed_audits_when_building_then_rows_show_target_and_outcome( + test_case: StandaloneAuditRowE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="seed_audit_rows", + repo_files=seed_audit_files(test_case.status_values), + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "build"), + project_dir=project_dir, + ) + + output: str = result.stdout + result.stderr + assert result.returncode == test_case.expected_exit_code, test_case.description + output + assert all(row in result.stdout for row in test_case.expected_rows), ( + test_case.description + output + ) diff --git a/tests/unit/src/sqlbuild/cli/progress/classes/test_native_progress_projector.py b/tests/unit/src/sqlbuild/cli/progress/classes/test_native_progress_projector.py index 856756ee1c..1cf91dea05 100644 --- a/tests/unit/src/sqlbuild/cli/progress/classes/test_native_progress_projector.py +++ b/tests/unit/src/sqlbuild/cli/progress/classes/test_native_progress_projector.py @@ -365,8 +365,8 @@ def test_given_retried_resource_when_final_result_claims_then_prior_attempt_rend RetryProjectionCase( description="same generic audit column on distinct targets", expected_lines=( - " 1/1 audit not_null (id) START", - " 1/1 audit not_null (id) START", + " 1/1 audit not_null (orders.id) START", + " 1/1 audit not_null (customers.id) START", " orders audit OK 0.10s rich", " customers audit OK 0.20s rich", ), @@ -381,7 +381,7 @@ def test_given_same_name_distinct_resources_when_claimed_by_id_then_terminals_do ) -> None: stream: _FlushRecordingStream = _FlushRecordingStream() projector: NativeProgressProjector = NativeProgressProjector(stream=stream, use_color=False) - projector.configure_resources(ordinals={"not_null (id)": 1}, total=1) + projector.configure_resources(ordinals={"not_null": 1}, total=1) first_start: LifecycleEvent = replace( lifecycle_event( "resource_attempt_started", @@ -390,7 +390,7 @@ def test_given_same_name_distinct_resources_when_claimed_by_id_then_terminals_do resource_attempt_id="first-attempt", payload={ "resource_kind": "audit", - "resource_name": "not_null (id)", + "resource_name": "not_null", "attempt_number": 1, }, ), @@ -418,11 +418,11 @@ def test_given_same_name_distinct_resources_when_claimed_by_id_then_terminals_do for event in (first_start, second_start, second_terminal, first_terminal): projector.consume(event) first_duration: float | None = projector.consume_resource_terminal( - resource_name="not_null (id)", resource_id="audit:not_null:model:orders:id" + resource_name="not_null", resource_id="audit:not_null:model:orders:id" ) stream.write(f" orders audit OK {cast(float, first_duration) / 1000.0:.2f}s rich\n") second_duration: float | None = projector.consume_resource_terminal( - resource_name="not_null (id)", resource_id="audit:not_null:model:customers:id" + resource_name="not_null", resource_id="audit:not_null:model:customers:id" ) stream.write(f" customers audit OK {cast(float, second_duration) / 1000.0:.2f}s rich\n") projector.close() From 97a1be34a2409bcf5ce02fe52e84b1fb0a4d9735 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:15:17 +0100 Subject: [PATCH 10/14] fix(plan): exit 1 when the plan has error findings sqb plan always exited 0 and printed 'Plan complete with errors' when the plan had error-severity findings that sqb build refuses, such as M114 name claims, so CI could not gate on it. Text and --json plans now exit 1 and the final line reads 'Plan failed with errors'; plans with only warnings still exit 0. The plan docs record the change as a release note. Refs: CHI-429 --- .../cli_text/main/format_plan_completion.rs | 8 +-- .../src/cli_text/tests/test_cli_text.rs | 13 ++++ .../bindings/_helpers/plan_command/plan.rs | 19 ++++- .../sqlbuild/references/docs/cli/plan.md | 10 +++ .../cli/commands/main/plan/_test_types.py | 9 +++ .../cli/commands/main/plan/helpers.py | 43 +++++++++++ .../main/plan/test_plan_exit_codes.py | 72 +++++++++++++++++++ .../main/column_migrations/helpers.py | 13 +++- .../commands/main/model_migrations/helpers.py | 7 +- website/src/content/docs/docs/cli/plan.mdx | 9 +++ 10 files changed, 193 insertions(+), 10 deletions(-) create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/plan/test_plan_exit_codes.py diff --git a/crates/sqlbuild-core/src/cli_text/main/format_plan_completion.rs b/crates/sqlbuild-core/src/cli_text/main/format_plan_completion.rs index b386b0bb0c..322bde9466 100644 --- a/crates/sqlbuild-core/src/cli_text/main/format_plan_completion.rs +++ b/crates/sqlbuild-core/src/cli_text/main/format_plan_completion.rs @@ -1,10 +1,10 @@ //! The completion line that closes text plan output. use crate::cli_text::_helpers::plan_sections::{full_refresh_layout, ready_summary}; -use crate::cli_text::constants::{ERROR_SEVERITY_VALUE, PHASE_OK_GLYPH}; +use crate::cli_text::constants::{ERROR_SEVERITY_VALUE, PHASE_FAIL_GLYPH, PHASE_OK_GLYPH}; use crate::cli_text::models::{CliStyle, PlanTextReport}; -/// `✓ Plan complete `, warning-styled when the plan has errors. +/// `✓ Plan complete `, or `✗ Plan failed` when the plan has errors a build refuses. #[must_use] pub fn format_plan_completion(report: &PlanTextReport) -> String { let style: CliStyle = CliStyle { @@ -16,8 +16,8 @@ pub fn format_plan_completion(report: &PlanTextReport) -> String { .any(|warning| warning.severity == ERROR_SEVERITY_VALUE); let (glyph, label) = if has_errors { ( - style.warning(PHASE_OK_GLYPH), - style.warning_strong("Plan complete with errors"), + style.error(PHASE_FAIL_GLYPH), + style.error_strong("Plan failed with errors"), ) } else { (style.success(PHASE_OK_GLYPH), "Plan complete".to_owned()) diff --git a/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs b/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs index 4bc1ad7077..60b9f89cc3 100644 --- a/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs +++ b/crates/sqlbuild-core/src/cli_text/tests/test_cli_text.rs @@ -177,6 +177,19 @@ fn given_plan_report_when_rendering_then_groups_models_by_reason() { expected_text: "Plan ready 2 selected\n\nQuery changed (2)\n\u{251c}\u{2500}\u{2500} orders full rebuild\n\u{2502} \u{251c}\u{2500}\u{2500} policy replay_on_change=full\n\u{2502} \u{2514}\u{2500}\u{2500} query diff:\n\u{2502} -SELECT 1\n\u{2514}\u{2500}\u{2500} customers full rebuild\n \u{2514}\u{2500}\u{2500} policy replay_on_change=full", expected_completion: "\u{2713} Plan complete 2 selected", }, + PlanTextTestCase { + description: "an error finding makes the final line state failure", + report: PlanTextReport { + warnings: vec![PlanWarningText { + severity: "error".to_owned(), + message: "would replace analytics.orders".to_owned(), + model_name: None, + }], + ..plan_report(vec![table_model("orders", "no_change")]) + }, + expected_text: "Plan ready 1 selected\n\nModels (1)\n\u{2514}\u{2500}\u{2500} orders table\n\nErrors (1)\n\u{2514}\u{2500}\u{2500} would replace analytics.orders", + expected_completion: "\u{2717} Plan failed with errors 1 selected", + }, PlanTextTestCase { description: "every active model is a full refresh", report: plan_report(vec![ diff --git a/crates/sqlbuild-python/src/bindings/_helpers/plan_command/plan.rs b/crates/sqlbuild-python/src/bindings/_helpers/plan_command/plan.rs index 7653cd894b..ddf569199b 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/plan_command/plan.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/plan_command/plan.rs @@ -30,6 +30,8 @@ use crate::bindings::_helpers::records::ids::RecordId; const COMMAND_NAME: &str = "plan"; const DEFAULT_SECTION_ENTRIES: i64 = 50; +/// The `severity` value of a plan finding that blocks a build. +const ERROR_SEVERITY: &str = "error"; const CONNECTION_TIMING: [&str; 2] = ["connection_count", "elapsed_seconds"]; /// Request fields passed to the compile pipeline under the same name. const PIPELINE_FIELDS: [&str; 11] = [ @@ -300,7 +302,22 @@ pub(crate) fn run_plan_command(request: &Bound<'_, PyAny>) -> PyResult { let pipeline: Bound<'_, PyAny> = with_verbose_inspection(py, verbose, || compile_pipeline(request, &invocation))?; write_output(request, &invocation, &pipeline)?; - Ok(0) + Ok(i32::from(plan_has_errors(&native_field( + &pipeline, + "plan_output", + )?)?)) +} + +/// Whether any plan finding has error severity, which `sqb build` refuses. +fn plan_has_errors(plan: &Bound<'_, PyAny>) -> PyResult { + for warning in native_field(plan, "warnings")?.try_iter()? { + let severity: Bound<'_, PyAny> = + native_field(&native_field(&warning?, "severity")?, "value")?; + if severity.extract::()? == ERROR_SEVERITY { + return Ok(true); + } + } + Ok(false) } pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> { diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md index d094dda1e6..fa6806eb22 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md @@ -11,6 +11,7 @@ Online: https://sqlbuild.com/docs/cli/plan/ - Usage - Flags - Output +- Exit codes - Missing upstream dependencies - Missing source tables @@ -132,6 +133,15 @@ Column migrations (1) When a removed column and an added column look related but are not an exact rename, the schema diff shows a hint such as `similar to amount; if this is a rename, add revenue (migrate_from amount)` until the change is built. +## Exit codes + +`sqb plan` exits `0` when the plan is ready, including plans that only have warnings. It exits `1` +when the plan has an error-severity finding that `sqb build` would refuse, such as an `M114` name +claim; the final line then reads `✗ Plan failed with errors`. `sqb plan --json` uses the same exit +codes, so CI can gate on either form. + +Release note: earlier versions printed `Plan complete with errors` and exited `0` for such plans. + ## Missing upstream dependencies Planning a scoped selection whose upstream inputs were never built in the target fails with `S301` rather than producing a plan that would break at build time: diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/plan/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/plan/_test_types.py index 810b51661a..067e857ff1 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/plan/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/plan/_test_types.py @@ -63,3 +63,12 @@ class MissingSourceTableE2ETestCase: expected_output_fragments: tuple[str, ...] = () expected_recorded_source_names: tuple[str, ...] = () expected_statuses: dict[str, str] = field(default_factory=dict) + + +@dataclass(frozen=True) +class PlanExitCodeE2ETestCase: + description: str + added_files: dict[str, str] + plan_args: tuple[str, ...] + expected_exit_code: int + expected_output_fragments: tuple[str, ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/plan/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/plan/helpers.py index 6dd49631fe..bce8148381 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/plan/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/plan/helpers.py @@ -173,3 +173,46 @@ def prepare_missing_source_tables_project(*, tmp_path: Path) -> Path: ): execute_duckdb(db_path=db_path, sql=statement) return project_dir + + +_EXIT_CODE_PROJECT_TOML: str = ( + 'name = "plan_exit_codes"\n' + 'adapter = "duckdb"\n' + 'default_target = "dev"\n\n' + "[connection]\n" + 'database = "plan.duckdb"\n\n' + "[targets.dev]\n" + 'schema = "analytics"\n' +) +_REVENUE_MODEL: str = "MODEL (description 'Revenue.', materialized table);\n\nSELECT 1 AS amount\n" +_DAILY_REVENUE_MODEL: str = ( + "MODEL (description 'Daily revenue.', materialized table, migrate_from revenue);\n\n" + "SELECT 1 AS amount\n" +) +RECLAIMED_NAME: dict[str, str] = {"models/revenue.sql": _REVENUE_MODEL} +REMOVABLE_MIGRATION_WARNING: str = "migrate_from can be removed from 'daily_revenue'" +NAME_CLAIM_ERROR: str = "model 'revenue' would replace analytics.revenue" + + +def migrated_project(tmp_path: Path) -> Path: + """A project whose `revenue` table was migrated to `daily_revenue` and built.""" + + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="plan_exit_codes", + repo_files={ + "sqlbuild_project.toml": _EXIT_CODE_PROJECT_TOML, + "models/revenue.sql": _REVENUE_MODEL, + }, + ) + first: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "build"), project_dir=project_dir + ) + assert first.returncode == 0, first.stdout + first.stderr + (project_dir / "models/revenue.sql").unlink() + (project_dir / "models/daily_revenue.sql").write_text(_DAILY_REVENUE_MODEL, encoding="utf-8") + migrated: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "build"), project_dir=project_dir + ) + assert migrated.returncode == 0, migrated.stdout + migrated.stderr + return project_dir diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/plan/test_plan_exit_codes.py b/tests/e2e/src/sqlbuild/cli/commands/main/plan/test_plan_exit_codes.py new file mode 100644 index 0000000000..d49308145d --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/plan/test_plan_exit_codes.py @@ -0,0 +1,72 @@ +"""E2E coverage for the exit code of `sqb plan`.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.plan._test_types import PlanExitCodeE2ETestCase +from tests.e2e.src.sqlbuild.cli.commands.main.plan.helpers import ( + NAME_CLAIM_ERROR, + RECLAIMED_NAME, + REMOVABLE_MIGRATION_WARNING, + migrated_project, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import run_sqb + + +@pytest.mark.parametrize( + "test_case", + [ + PlanExitCodeE2ETestCase( + description="a plan with only warnings exits 0", + added_files={}, + plan_args=(), + expected_exit_code=0, + expected_output_fragments=( + REMOVABLE_MIGRATION_WARNING, + "\u2713 Plan complete 1 selected", + ), + ), + PlanExitCodeE2ETestCase( + description="a plan with an error finding exits 1 and states failure", + added_files=RECLAIMED_NAME, + plan_args=(), + expected_exit_code=1, + expected_output_fragments=( + NAME_CLAIM_ERROR, + "\u2717 Plan failed with errors 2 selected", + ), + ), + PlanExitCodeE2ETestCase( + description="a json plan with an error finding exits 1", + added_files=RECLAIMED_NAME, + plan_args=("--json",), + expected_exit_code=1, + expected_output_fragments=(NAME_CLAIM_ERROR,), + ), + ], + ids=lambda case: case.description, +) +def test_given_plan_findings_when_planning_then_exit_code_reflects_errors( + test_case: PlanExitCodeE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = migrated_project(tmp_path) + relative_path: str + contents: str + for relative_path, contents in test_case.added_files.items(): + (project_dir / relative_path).write_text(contents, encoding="utf-8") + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "plan", *test_case.plan_args), + project_dir=project_dir, + ) + + output: str = result.stdout + result.stderr + assert result.returncode == test_case.expected_exit_code, test_case.description + output + assert all(fragment in output for fragment in test_case.expected_output_fragments), ( + test_case.description + output + ) diff --git a/tests/integration/src/sqlbuild/cli/commands/main/column_migrations/helpers.py b/tests/integration/src/sqlbuild/cli/commands/main/column_migrations/helpers.py index 7500593196..64177a1fcf 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/column_migrations/helpers.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/column_migrations/helpers.py @@ -246,11 +246,18 @@ def plan_text(*, project_dir: Path, capsys: pytest.CaptureFixture[str]) -> str: def plan_json(*, project_dir: Path, capsys: pytest.CaptureFixture[str]) -> dict[str, Any]: - """Run sqb plan --json and parse stdout.""" + """Run sqb plan --json, check its exit code against its findings, and parse stdout.""" result: CliRun = run_sqb(project_dir=project_dir, args=("plan", "--json"), capsys=capsys) - assert result.exit_code == 0, result.output - return json.loads(result.stdout) + payload: dict[str, Any] = json.loads(result.stdout) + assert result.exit_code == plan_exit_code(payload), result.output + return payload + + +def plan_exit_code(plan: dict[str, Any]) -> int: + """Return the exit code of a plan: 1 when any finding has error severity, else 0.""" + + return int(any(warning["severity"] == "error" for warning in plan["warnings"])) def planned_column_migrations(plan: dict[str, Any]) -> tuple[tuple[str, str, str, str], ...]: diff --git a/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/helpers.py b/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/helpers.py index 6828de306d..98ac4033b8 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/helpers.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/model_migrations/helpers.py @@ -302,8 +302,11 @@ def plan_json( ["--project-dir", str(project_dir), "--no-color", "plan", "--json", *args] ) captured: CaptureResult[str] = capsys.readouterr() - assert exit_code == 0, captured.out + captured.err - return json.loads(captured.out) + payload: dict[str, Any] = json.loads(captured.out) + assert exit_code == int( + any(warning["severity"] == "error" for warning in payload["warnings"]) + ), captured.out + captured.err + return payload def model_entry(*, plan: dict[str, Any], name: str) -> dict[str, Any]: diff --git a/website/src/content/docs/docs/cli/plan.mdx b/website/src/content/docs/docs/cli/plan.mdx index 582c72b52c..9340cfa2ef 100644 --- a/website/src/content/docs/docs/cli/plan.mdx +++ b/website/src/content/docs/docs/cli/plan.mdx @@ -123,6 +123,15 @@ Column migrations (1) When a removed column and an added column look related but are not an exact rename, the schema diff shows a hint such as `similar to amount; if this is a rename, add revenue (migrate_from amount)` until the change is built. +## Exit codes + +`sqb plan` exits `0` when the plan is ready, including plans that only have warnings. It exits `1` +when the plan has an error-severity finding that `sqb build` would refuse, such as an `M114` name +claim; the final line then reads `✗ Plan failed with errors`. `sqb plan --json` uses the same exit +codes, so CI can gate on either form. + +Release note: earlier versions printed `Plan complete with errors` and exited `0` for such plans. + ## Missing upstream dependencies Planning a scoped selection whose upstream inputs were never built in the target fails with `S301` rather than producing a plan that would break at build time: From 10ab7e8137345527902ad5e544be37ff9da1cffe Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:36:53 +0100 Subject: [PATCH 11/14] fix(test): run unit tests against temporary copies of project functions `sqb test` called the built warehouse function, so an edited UDF or table function was tested at its last-built definition, and a never-built one failed with "Run `sqb build` first". Before the tests run, SQLBuild now creates a session-scoped copy of every function the selected tests call, and of every function those call (the test plan now carries the transitive closure), then points calls in the tests' SQL code at the copies; calls in string literals and comments keep their text. No permanent DDL is issued. The copy set is decided on the first test connection and recreated on every other connection and on recovery reconnects; a connection that cannot create it stops the run with error[T006] naming the function and connection. Adapters opt in through the optional StrictAdapter method `render_create_temporary_function`: DuckDB and MotherDuck use TEMP MACRO, PostgreSQL uses pg_temp, Snowflake and Databricks use prefixed TEMPORARY FUNCTIONs. BigQuery and SQL Server keep calling the built function. When no copy can be created, tests fall back to the built function (a caller whose callee has no copy too), and a missing one reports the reason with the build hint. Refs: CHI-555 --- .../src/bindings/_helpers/cli/entry.rs | 3 +- .../bindings/_helpers/native_host/executor.rs | 6 + .../_helpers/planner_entries/command_plans.rs | 38 +- .../references/docs/concepts/adapters.md | 2 +- .../docs/concepts/adapters/bigquery.md | 5 + .../docs/concepts/adapters/databricks.md | 6 + .../docs/concepts/adapters/duckdb.md | 6 + .../docs/concepts/adapters/motherduck.md | 6 + .../docs/concepts/adapters/postgres.md | 7 + .../docs/concepts/adapters/snowflake.md | 8 + .../docs/concepts/adapters/sqlserver.md | 5 + .../references/docs/concepts/testing.md | 34 ++ src/sqlbuild/_native_host.py | 4 + src/sqlbuild/_native_host.pyi | 3 + .../contract/classes/duckdb_backed_adapter.py | 20 + .../contract/classes/strict_adapter.py | 10 + src/sqlbuild/adapter/contract/models.py | 8 + .../databricks/classes/databricks_adapter.py | 27 ++ src/sqlbuild/adapters/databricks/constants.py | 1 + .../postgres/classes/postgres_adapter.py | 20 + .../snowflake/classes/snowflake_adapter.py | 29 ++ src/sqlbuild/adapters/snowflake/constants.py | 1 + .../pipeline/_helpers/temporary_functions.py | 362 ++++++++++++++++++ .../executor/pipeline/_helpers/testing.py | 105 ++++- src/sqlbuild/executor/pipeline/exceptions.py | 17 + src/sqlbuild/executor/pipeline/models.py | 15 +- .../cli/commands/main/test/_test_types.py | 32 ++ .../main/test/test_temporary_functions.py | 312 +++++++++++++++ .../classes/strict_adapter/_test_types.py | 14 + .../strict_adapter/test_strict_adapter.py | 17 +- .../test_temporary_function_rendering.py | 146 +++++++ .../executor/pipeline/_helpers/_test_types.py | 17 + .../executor/pipeline/_helpers/helpers.py | 89 +++++ .../_helpers/test_temporary_functions.py | 75 ++++ .../pipeline/_helpers/test_testing.py | 126 ++++++ .../content/docs/docs/concepts/adapters.mdx | 2 +- .../docs/docs/concepts/adapters/bigquery.mdx | 5 + .../docs/concepts/adapters/databricks.mdx | 6 + .../docs/docs/concepts/adapters/duckdb.mdx | 6 + .../docs/concepts/adapters/motherduck.mdx | 6 + .../docs/docs/concepts/adapters/postgres.mdx | 7 + .../docs/docs/concepts/adapters/snowflake.mdx | 7 + .../docs/docs/concepts/adapters/sqlserver.mdx | 5 + .../content/docs/docs/concepts/testing.mdx | 34 ++ 44 files changed, 1637 insertions(+), 17 deletions(-) create mode 100644 src/sqlbuild/executor/pipeline/_helpers/temporary_functions.py create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/test/test_temporary_functions.py create mode 100644 tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_temporary_function_rendering.py create mode 100644 tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_temporary_functions.py diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs index 676a05d255..d4500c5832 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs @@ -43,10 +43,11 @@ const DIFF_PUBLICATION_CODE: &str = "C257"; const DIFF_EXECUTION_STATUS: &str = "execution_failed"; const DIFF_OUTCOME_PREFIX: &str = "Query diff outcome "; /// Expected staying-Python command failures, loaded only once a command raised. -const EXPECTED_COMMAND_FAILURES: [ExecutorHost; 3] = [ +const EXPECTED_COMMAND_FAILURES: [ExecutorHost; 4] = [ ExecutorHost::AuditExecutionError, ExecutorHost::ExecutionDurationLimitError, ExecutorHost::WarehouseConnectionError, + ExecutorHost::TemporaryTestFunctionError, ]; /// Expected native command failures. const EXPECTED_NATIVE_FAILURES: [ErrorId; 2] = [ErrorId::Discovery, ErrorId::SpecConfig]; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs b/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs index c7528eda68..2f31d6fa79 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/native_host/executor.rs @@ -69,6 +69,7 @@ pub(crate) enum ExecutorHost { IsCompatibilityView, AuditExecutionError, WarehouseConnectionError, + TemporaryTestFunctionError, RaiseWarehouseConnectionFailure, AuditPipelineCallbacks, RunAuditPipelineWithCallbacks, @@ -493,6 +494,11 @@ pub(crate) const ENTRIES: &[HostEntry] = &[ kind: HostKind::Exception, edge: HostEdge::Executor, }, + HostEntry { + name: "TemporaryTestFunctionError", + kind: HostKind::Exception, + edge: HostEdge::Executor, + }, HostEntry { name: "raise_warehouse_connection_failure", kind: HostKind::Function, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/planner_entries/command_plans.rs b/crates/sqlbuild-python/src/bindings/_helpers/planner_entries/command_plans.rs index 5ac80284d5..284d458d3b 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/planner_entries/command_plans.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/planner_entries/command_plans.rs @@ -23,7 +23,10 @@ use crate::bindings::_helpers::records::fields::field as native_field; use crate::bindings::_helpers::records::fields::mapping_get; use crate::bindings::_helpers::records::ids::RecordId; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyModule, PyModuleMethods, PyResult, Python}; -use pyo3::types::{PyDict, PyDictMethods, PyFrozenSet, PyList, PyListMethods, PyString, PyTuple}; +use pyo3::types::{ + PyDict, PyDictMethods, PyFrozenSet, PyList, PyListMethods, PySet, PySetMethods, PyString, + PyTuple, +}; use pyo3::{pyfunction, wrap_pyfunction}; use sqlbuild_analysis::fingerprint_storage::constants::NODE_TYPE_SEED; use sqlbuild_core::constants::FINGERPRINT_TABLE_NAME; @@ -190,6 +193,37 @@ pub(crate) fn build_audit_command_plan<'py>( ) } +/// The tests' direct function dependencies and every project function those call. +fn called_function_keys<'py>( + project: &Bound<'py, PyAny>, + direct: Vec>, +) -> PyResult> { + let py: Python<'py> = project.py(); + let functions: Bound<'py, PyDict> = PyDict::new(py); + for function in native_field(project, "functions")?.try_iter()? { + let function = function?; + functions.set_item(native_field(&function, "key")?, function)?; + } + let called: Bound<'py, PySet> = PySet::empty(py)?; + let mut pending: Vec> = direct; + while let Some(key) = pending.pop() { + if called.contains(&key)? { + continue; + } + called.add(&key)?; + let Some(function) = functions.get_item(&key)? else { + continue; + }; + for dependency in native_field(&function, "deps")?.try_iter()? { + let dependency = dependency?; + if functions.contains(&dependency)? { + pending.push(dependency); + } + } + } + Ok(called) +} + /// Project selected SQL tests from canonical static state. #[pyfunction] #[pyo3(signature = (**kwargs))] @@ -235,7 +269,7 @@ pub(crate) fn build_test_command_plan<'py>( function_keys.push(key?); } } - let function_keys = PyFrozenSet::new(py, function_keys)?; + let function_keys = called_function_keys(&project, function_keys)?; let mut functions: Vec> = Vec::new(); for function in native_field(&project, "functions")?.try_iter()? { let function = function?; diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters.md index ab4ac91e34..648ff7e161 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters.md @@ -101,7 +101,7 @@ Set `sql_analysis_dialect_name` to the SQL dialect name that matches your engine For full control with no inherited defaults, subclass `StrictAdapter` instead. Every method is abstract and must be implemented explicitly. SQLBuild raises a clear error listing any unimplemented methods. -A few `StrictAdapter` methods are optional and have a documented default. `create_sql_test_output` and `drop_sql_test_output` let an adapter store a unit test's actual output once so every comparison reads it (see [Single-read comparison](testing.md#single-read-comparison)). By default `create_sql_test_output` returns `None`, and SQLBuild runs the test's inline comparison query instead, so adapters that do not implement them keep working unchanged. +A few `StrictAdapter` methods are optional and have a documented default. `create_sql_test_output` and `drop_sql_test_output` let an adapter store a unit test's actual output once so every comparison reads it (see [Single-read comparison](testing.md#single-read-comparison)). By default `create_sql_test_output` returns `None`, and SQLBuild runs the test's inline comparison query instead, so adapters that do not implement them keep working unchanged. `render_create_temporary_function` returns the call target and DDL for a session-scoped copy of a project function, which `sqb test` calls instead of the built function (see [Functions in `sqb test`](testing.md#functions-in-sqb-test)). By default it returns `None`, and tests call the built function. ### Discovery rules diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/bigquery.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/bigquery.md index 8d0141e523..b450317391 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/bigquery.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/bigquery.md @@ -65,3 +65,8 @@ NUMERIC '2.4700' BigQuery does not support arrays of arrays. A nested list or set requested with `render_as array` fails at compile time rather than producing invalid BigQuery SQL. Parenthesized `value_list` rendering remains available for ordinary scalar collections used with `IN`. See [Collections and Rendering](../constants/collections-and-rendering.md#native-array-rendering) for declarations, rendering precedence, and the cross-adapter matrix. + +## Functions in unit tests + +BigQuery temporary functions exist only inside one script, so `sqb test` calls the built function. +Run `sqb build` before testing a new or changed function. See [Functions in `sqb test`](../testing.md#functions-in-sqb-test). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/databricks.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/databricks.md index 61748c1a68..0053f2e4fc 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/databricks.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/databricks.md @@ -45,3 +45,9 @@ The target's `database` selects the Unity Catalog catalog, and its `schema` sele On connect, SQLBuild runs `USE CATALOG` and `USE SCHEMA` statements from the active target to set the authoritative session namespace. + +## Functions in unit tests + +`sqb test` creates each SQL UDF and table function the selected tests call as a session +`TEMPORARY FUNCTION __sqb_test__`. Python UDFs cannot be temporary, so tests call the built +Python function. See [Functions in `sqb test`](../testing.md#functions-in-sqb-test). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/duckdb.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/duckdb.md index 51a57dccb7..35506f687c 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/duckdb.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/duckdb.md @@ -60,3 +60,9 @@ DuckDB defaults to `staged` promotion: tables are materialized into a staging ta [settings] table_promotion_mode = "staged" ``` + +## Functions in unit tests + +`sqb test` copies each SQL UDF and table function the selected tests call into a `CREATE TEMP +MACRO` on every test connection, called as `temp.main.`, so tests use the current project +definition without a build. Python UDFs are registered on each test connection. See [Functions in `sqb test`](../testing.md#functions-in-sqb-test). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/motherduck.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/motherduck.md index d54f46af87..cf92a5859c 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/motherduck.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/motherduck.md @@ -77,3 +77,9 @@ schema = "dev" ``` This lets you develop and test locally with zero MotherDuck compute cost, then deploy to MotherDuck in production. SQLBuild's [scenario replay](../scenarios.md) also runs locally in DuckDB regardless of the production adapter. + +## Functions in unit tests + +`sqb test` copies each SQL UDF and table function the selected tests call into a session +`TEMP MACRO`, the same as [DuckDB](duckdb.md#functions-in-unit-tests), so the +functions in your MotherDuck database are not replaced. See [Functions in `sqb test`](../testing.md#functions-in-sqb-test). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/postgres.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/postgres.md index 41ff1723bf..97337b9aa1 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/postgres.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/postgres.md @@ -64,3 +64,10 @@ connection = "postgres" database = "analytics" schema = "dev" ``` + +## Functions in unit tests + +`sqb test` creates each SQL function the selected tests call in the session's `pg_temp` schema and +calls it as `pg_temp.`, so tests use the current project definition without a build. Temporary +functions belong to one server session: behind a transaction-mode pooler, run `sqb test` through a +direct connection or a session-mode pool. See [Functions in `sqb test`](../testing.md#functions-in-sqb-test). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/snowflake.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/snowflake.md index a48a472d01..ea93ec6ce1 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/snowflake.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/snowflake.md @@ -15,6 +15,7 @@ Online: https://sqlbuild.com/docs/concepts/adapters/snowflake/ - State tables - Shared connections across targets - Cost estimates +- Functions in unit tests Snowflake requires the optional `snowflake-connector-python` dependency: @@ -137,3 +138,10 @@ not reconstruct invisible concurrent work, warehouse resume or idle tail, the 60 cloud-services credits, contract adjustments, or multi-cluster billing. Run metadata and query IDs are stored under `target/executions//`; that statement ledger stores only an SQL digest, not SQL text. Executed SQL artifacts are stored separately under the sensitive `target/run/` tree. + +## Functions in unit tests + +`sqb test` creates each UDF and table function the selected tests call, SQL or Python, as a +`TEMPORARY FUNCTION __SQB_TEST__` in the session's current schema. The session needs a current +schema and permission to create functions there; otherwise tests call the built function. See +[Functions in `sqb test`](../testing.md#functions-in-sqb-test). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/sqlserver.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/sqlserver.md index 3bba118653..3939785771 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/sqlserver.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/sqlserver.md @@ -80,3 +80,8 @@ connection = "sqlserver" database = "analytics" schema = "dev" ``` + +## Functions in unit tests + +SQL Server has no session-scoped functions, so `sqb test` calls the built function. Run `sqb build` +before testing a new or changed function. See [Functions in `sqb test`](../testing.md#functions-in-sqb-test). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/testing.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/testing.md index 48dc842484..6b6a0b0fa1 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/testing.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/testing.md @@ -590,6 +590,40 @@ __udf_expected__ AS ( UDFs are warehouse objects, so the function is created before the test runs. During `sqb build`, UDF tests run after the function is created but before any model that uses it. +### Functions in `sqb test` + +`sqb test` does not need a build to test the current version of a function. Before the tests run, +SQLBuild creates a session-scoped temporary copy of every project UDF and table function the +selected tests call, including functions those functions call, from the definitions in your +project. The tests call the copies, so an edited function is tested without `sqb build`, and the +built function in the target schema is never created, replaced or dropped. The copies disappear +when the test connections close. + +| Adapter | Temporary copy | +|---|---| +| DuckDB, MotherDuck | `CREATE TEMP MACRO`, called as `temp.main.`. Python UDFs are registered on each test connection. | +| PostgreSQL | SQL function in the session's `pg_temp` schema, called as `pg_temp.` | +| Snowflake | `CREATE TEMPORARY FUNCTION __sqb_test__` in the session's current schema, for SQL and Python UDFs and table functions | +| Databricks | `CREATE TEMPORARY FUNCTION __sqb_test__` for SQL functions | +| BigQuery, SQL Server | Not supported; tests call the built function | + +When an adapter cannot create a copy (a Databricks Python UDF, for example), or the warehouse +refuses to create it (the role lacks permission, or the Snowflake session has no current schema), +the test calls the built function instead. If that function has not been built either, the test +fails with an error that names the function and the reason no copy was created, and asks you to run +`sqb build` first. +A function whose callee has no copy calls the built functions too, so one test never mixes copies +and built functions. + +SQLBuild decides which copies exist on the first test connection and then creates the same copies +on every other test connection, including a connection that replaces one a timeout left unusable. +If one of those connections cannot create a copy, `sqb test` stops with error `T006`, which names +the function and the connection. + +PostgreSQL temporary functions belong to one server session. Behind a transaction-mode connection +pooler, a test's statements can reach a different server session that does not hold the copies, so +run `sqb test` for projects with functions through a direct connection or a session-mode pool. + ### Table function tests Test table functions by calling them in `__table_fn_actual__` and comparing against `__table_fn_expected__`: diff --git a/src/sqlbuild/_native_host.py b/src/sqlbuild/_native_host.py index 4adc8800e5..634ab0f4fe 100644 --- a/src/sqlbuild/_native_host.py +++ b/src/sqlbuild/_native_host.py @@ -385,6 +385,10 @@ "sqlbuild.executor.pipeline.exceptions", "WarehouseConnectionError", ), + "TemporaryTestFunctionError": ( + "sqlbuild.executor.pipeline.exceptions", + "TemporaryTestFunctionError", + ), "raise_warehouse_connection_failure": ( "sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure", "raise_warehouse_connection_failure", diff --git a/src/sqlbuild/_native_host.pyi b/src/sqlbuild/_native_host.pyi index 7e222cc35e..08c7f6dee1 100644 --- a/src/sqlbuild/_native_host.pyi +++ b/src/sqlbuild/_native_host.pyi @@ -270,6 +270,9 @@ from sqlbuild.executor.old_name_views._helpers.janitor import ( is_compatibility_view as is_compatibility_view, ) from sqlbuild.executor.pipeline.exceptions import AuditExecutionError as AuditExecutionError +from sqlbuild.executor.pipeline.exceptions import ( + TemporaryTestFunctionError as TemporaryTestFunctionError, +) from sqlbuild.executor.pipeline.exceptions import ( WarehouseConnectionError as WarehouseConnectionError, ) diff --git a/src/sqlbuild/adapter/contract/classes/duckdb_backed_adapter.py b/src/sqlbuild/adapter/contract/classes/duckdb_backed_adapter.py index e57bbd8024..bb1ad30829 100644 --- a/src/sqlbuild/adapter/contract/classes/duckdb_backed_adapter.py +++ b/src/sqlbuild/adapter/contract/classes/duckdb_backed_adapter.py @@ -72,6 +72,7 @@ SnapshotChangeTarget, TableFreshnessMetadata, TableFreshnessRequest, + TemporaryFunction, ) from sqlbuild.adapter.contract.types import ( CursorKind, @@ -1330,6 +1331,25 @@ def render_create_function( return (f"CREATE OR REPLACE MACRO {destination}({argument_sql}) AS TABLE\n{body_sql}",) return (f"CREATE OR REPLACE MACRO {destination}({argument_sql}) AS (\n{body_sql}\n)",) + def render_create_temporary_function( + self, *, name: str, definition: FunctionDefinition + ) -> TemporaryFunction | None: + """Copy a SQL function into a temporary macro; Python functions register per connection.""" + + if definition.language != FunctionLanguage.SQL: + return None + identifier: str = self.render_identifier(name) + argument_sql: str = ", ".join(str(arg.name) for arg in definition.arguments) + body: str = ( + f"AS TABLE\n{definition.body_sql}" + if definition.return_columns + else f"AS (\n{definition.body_sql}\n)" + ) + return TemporaryFunction( + target=f"temp.main.{identifier}", + statements=(f"CREATE OR REPLACE TEMP MACRO {identifier}({argument_sql}) {body}",), + ) + def supports_python_functions(self) -> bool: return True diff --git a/src/sqlbuild/adapter/contract/classes/strict_adapter.py b/src/sqlbuild/adapter/contract/classes/strict_adapter.py index ac1bf07a68..558e0cf341 100644 --- a/src/sqlbuild/adapter/contract/classes/strict_adapter.py +++ b/src/sqlbuild/adapter/contract/classes/strict_adapter.py @@ -16,6 +16,7 @@ ColumnInfo, CursorValue, ExpressionInferenceProfile, + FunctionDefinition, MigrationStagePlan, RelationGrant, RelationReadProbe, @@ -24,6 +25,7 @@ SnapshotChangeTarget, TableFreshnessMetadata, TableFreshnessRequest, + TemporaryFunction, ) from sqlbuild.adapter.contract.types import ( FrameworkType, @@ -116,6 +118,14 @@ def drop_sql_test_output(self, *, connection: Any, relation: str) -> None: del connection, relation + def render_create_temporary_function( + self, *, name: str, definition: FunctionDefinition + ) -> TemporaryFunction | None: + """Optional: a session-scoped function copy for unit tests, or `None` if unsupported.""" + + del name, definition + return None + @abstractmethod def lists_relations_authoritatively(self, *, database: str | None, schema: str) -> bool: """Return whether ``list_relations`` reports every relation in this scope.""" diff --git a/src/sqlbuild/adapter/contract/models.py b/src/sqlbuild/adapter/contract/models.py index c6e237e612..3678afe53a 100644 --- a/src/sqlbuild/adapter/contract/models.py +++ b/src/sqlbuild/adapter/contract/models.py @@ -284,6 +284,14 @@ class FunctionDefinition: source_file_path: Path | None = None +@dataclass(frozen=True) +class TemporaryFunction: + """A session-scoped copy of a project function: its call target and creating DDL.""" + + target: str + statements: tuple[str, ...] + + @dataclass(frozen=True) class QueryResult: """Rows returned by an ad hoc warehouse query.""" diff --git a/src/sqlbuild/adapters/databricks/classes/databricks_adapter.py b/src/sqlbuild/adapters/databricks/classes/databricks_adapter.py index 91ff016fd2..9006a13ce2 100644 --- a/src/sqlbuild/adapters/databricks/classes/databricks_adapter.py +++ b/src/sqlbuild/adapters/databricks/classes/databricks_adapter.py @@ -80,6 +80,7 @@ SnapshotSqlDialect, TableFreshnessMetadata, TableFreshnessRequest, + TemporaryFunction, ) from sqlbuild.adapter.contract.types import ( BuiltinAdapter, @@ -128,6 +129,7 @@ NON_ROW_RESULT_COLUMN_NAMES, RELATION_NOT_FOUND_ERROR_CLASSES, TABLE_RELATION_METADATA_TYPES, + TEMPORARY_FUNCTION_PREFIX, UNDEFINED_TABLE_SQLSTATE, ) from sqlbuild.compiler.compile.types import FunctionLanguage @@ -1679,6 +1681,31 @@ def render_create_function( f"RETURN {body_sql}", ) + def render_create_temporary_function( + self, *, name: str, definition: FunctionDefinition + ) -> TemporaryFunction | None: + """Copy a SQL function into a prefixed session TEMPORARY function, called unqualified.""" + + if definition.language != FunctionLanguage.SQL: + return None + target: str = self.render_identifier(f"{TEMPORARY_FUNCTION_PREFIX}{name}") + statements: tuple[str, ...] = self.render_create_function( + destination=target, + arguments=definition.arguments, + returns=definition.returns, + body_sql=definition.body_sql, + return_columns=definition.return_columns, + ) + return TemporaryFunction( + target=target, + statements=tuple( + statement.replace( + "CREATE OR REPLACE FUNCTION ", "CREATE OR REPLACE TEMPORARY FUNCTION ", 1 + ) + for statement in statements + ), + ) + def _render_create_python_function( self, *, diff --git a/src/sqlbuild/adapters/databricks/constants.py b/src/sqlbuild/adapters/databricks/constants.py index 5401a62f46..3ab8be030e 100644 --- a/src/sqlbuild/adapters/databricks/constants.py +++ b/src/sqlbuild/adapters/databricks/constants.py @@ -16,3 +16,4 @@ UNDEFINED_TABLE_SQLSTATE: str = "42P01" DETAILED_TABLE_INFORMATION_MARKER: str = "# detailed table information" DESCRIBE_DETAIL_ROW_WIDTH: int = 2 +TEMPORARY_FUNCTION_PREFIX: str = "__sqb_test__" diff --git a/src/sqlbuild/adapters/postgres/classes/postgres_adapter.py b/src/sqlbuild/adapters/postgres/classes/postgres_adapter.py index 0d7ab58d6e..1346335f92 100644 --- a/src/sqlbuild/adapters/postgres/classes/postgres_adapter.py +++ b/src/sqlbuild/adapters/postgres/classes/postgres_adapter.py @@ -76,6 +76,7 @@ SnapshotSqlDialect, TableFreshnessMetadata, TableFreshnessRequest, + TemporaryFunction, ) from sqlbuild.adapter.contract.types import ( BuiltinAdapter, @@ -536,6 +537,25 @@ def render_create_function( f"LANGUAGE SQL AS $$\n{body_sql}\n$$", ) + def render_create_temporary_function( + self, *, name: str, definition: FunctionDefinition + ) -> TemporaryFunction | None: + """Copy a SQL function into the session's `pg_temp` schema, called schema-qualified.""" + + if definition.language != FunctionLanguage.SQL: + return None + target: str = f"pg_temp.{self.render_identifier(name)}" + return TemporaryFunction( + target=target, + statements=self.render_create_function( + destination=target, + arguments=definition.arguments, + returns=definition.returns, + body_sql=definition.body_sql, + return_columns=definition.return_columns, + ), + ) + def render_append( self, *, destination: str, sql: str, columns: tuple[str, ...] | None = None ) -> tuple[str, ...]: diff --git a/src/sqlbuild/adapters/snowflake/classes/snowflake_adapter.py b/src/sqlbuild/adapters/snowflake/classes/snowflake_adapter.py index 05a67ee557..0cb29d1848 100644 --- a/src/sqlbuild/adapters/snowflake/classes/snowflake_adapter.py +++ b/src/sqlbuild/adapters/snowflake/classes/snowflake_adapter.py @@ -79,6 +79,7 @@ SnapshotSqlDialect, TableFreshnessMetadata, TableFreshnessRequest, + TemporaryFunction, ) from sqlbuild.adapter.contract.types import ( BuiltinAdapter, @@ -152,6 +153,7 @@ SHOW_LIKE_ESCAPE_CHARACTER, STATUS_COLUMN_NAME, SUCCESS_STATUS_TOKENS, + TEMPORARY_FUNCTION_PREFIX, TRUE_METADATA_VALUE, VIEW_RELATION_TYPE_TOKEN, ) @@ -2829,6 +2831,33 @@ def render_create_function( f"AS $$\n{body_sql}\n$$", ) + def render_create_temporary_function( + self, *, name: str, definition: FunctionDefinition + ) -> TemporaryFunction | None: + """Copy a function into a session TEMPORARY function in the session's current schema.""" + + target: str = self.render_identifier(f"{TEMPORARY_FUNCTION_PREFIX}{name}") + statements: tuple[str, ...] = self.render_create_function( + destination=target, + arguments=definition.arguments, + returns=definition.returns, + body_sql=definition.body_sql, + return_columns=definition.return_columns, + language=definition.language, + runtime_version=definition.runtime_version, + entry_point=definition.entry_point, + packages=definition.packages, + ) + return TemporaryFunction( + target=target, + statements=tuple( + statement.replace( + "CREATE OR REPLACE FUNCTION ", "CREATE OR REPLACE TEMPORARY FUNCTION ", 1 + ) + for statement in statements + ), + ) + def render_cursor_bound_literal(self, *, value: str, cursor_type: str | None) -> str: if cursor_type == CursorKind.INTEGER: return value diff --git a/src/sqlbuild/adapters/snowflake/constants.py b/src/sqlbuild/adapters/snowflake/constants.py index 26a745d85c..3816e406b6 100644 --- a/src/sqlbuild/adapters/snowflake/constants.py +++ b/src/sqlbuild/adapters/snowflake/constants.py @@ -28,3 +28,4 @@ STATE_TABLE_FALLBACK_RETENTION_DAYS: int = 1 STATE_TABLE_RETENTION_PARAMETER: str = "DATA_RETENTION_TIME_IN_DAYS" INVALID_PARAMETER_VALUE_ERRNO: int = 1008 +TEMPORARY_FUNCTION_PREFIX: str = "__sqb_test__" diff --git a/src/sqlbuild/executor/pipeline/_helpers/temporary_functions.py b/src/sqlbuild/executor/pipeline/_helpers/temporary_functions.py new file mode 100644 index 0000000000..734d328678 --- /dev/null +++ b/src/sqlbuild/executor/pipeline/_helpers/temporary_functions.py @@ -0,0 +1,362 @@ +"""Session-scoped copies of project functions so unit tests call the current definitions.""" + +from __future__ import annotations + +import logging +import re +from collections.abc import Iterable +from dataclasses import replace +from typing import Any + +from sqlbuild._native import ( + ChainStep, + CompiledObjectKey, + FunctionPlanEntry, + SqlCodeScanner, + SqlTestAssertionStep, + SqlTestPlanEntry, +) +from sqlbuild.adapter.contract.classes.base_adapter import BaseAdapter +from sqlbuild.adapter.contract.models import FunctionDefinition, TemporaryFunction +from sqlbuild.diagnostics.classes.diagnostic_record_redactor import DiagnosticRecordRedactor +from sqlbuild.diagnostics.main.log_debug_event import log_debug_event +from sqlbuild.executor.pipeline.exceptions import TemporaryTestFunctionError +from sqlbuild.executor.pipeline.models import TemporaryTestFunctions +from sqlbuild.runtime.execution_limits.exceptions import ( + ExecutionDurationLimitError, + ItemTimeoutError, +) + +_LOGGER: logging.Logger = logging.getLogger("sqlbuild.execution") +_IDENTIFIER_QUOTES: frozenset[str] = frozenset({'"', "`", "["}) +_SCANNER_CONTEXT: str = "SQL test function call rewriting" + + +def create_temporary_test_functions( + *, + function_entries: tuple[FunctionPlanEntry, ...], + required_keys: Iterable[CompiledObjectKey], + adapter: BaseAdapter, + connection: Any, +) -> TemporaryTestFunctions: + """Create a temporary copy of each required function and the functions its body calls.""" + + syntax: dict[str, object] = adapter.sql_lexical_syntax.native_mapping + entries_by_key: dict[CompiledObjectKey, FunctionPlanEntry] = _entries_by_key(function_entries) + keys_by_target: dict[str, CompiledObjectKey] = _keys_by_target(entries_by_key) + replacements: dict[str, str] = {} + created_keys: set[CompiledObjectKey] = set() + statements: list[tuple[str, str]] = [] + failures: dict[CompiledObjectKey, str] = {} + key: CompiledObjectKey + for key in _dependency_order( + required_keys=required_keys, entries_by_key=entries_by_key, syntax=syntax + ): + entry: FunctionPlanEntry = entries_by_key[key] + uncopied: tuple[CompiledObjectKey, ...] = tuple( + callee + for callee in _called_function_keys( + sql=entry.body_sql, keys_by_target=keys_by_target, syntax=syntax + ) + if callee in failures + ) + if uncopied: + failures[key] = ( + f"function {uncopied[0].name}, which it calls, has no temporary copy: " + f"{failures[uncopied[0]]}" + ) + continue + deployed_target: str = str(entry.destination.qualified_name) + temporary: TemporaryFunction | None = adapter.render_create_temporary_function( + name=entry.name, + definition=FunctionDefinition( + destination=deployed_target, + arguments=entry.arguments, + returns=entry.returns, + body_sql=rewrite_function_calls( + sql=entry.body_sql, replacements=replacements, syntax=syntax + ), + return_columns=entry.return_columns, + language=entry.language, + runtime_version=entry.runtime_version, + entry_point=entry.entry_point, + packages=entry.packages, + source_file_path=entry.source_file_path, + ), + ) + if temporary is None: + failures[key] = ( + f"the {adapter.adapter_name} adapter cannot create a temporary copy of " + f"{entry.language} function {entry.name} for the test run" + ) + continue + failure: str | None = _execute_statements( + adapter=adapter, connection=connection, statements=temporary.statements + ) + if failure is not None: + failures[key] = f"its temporary copy for the test run could not be created: {failure}" + continue + replacements[deployed_target] = temporary.target + created_keys.add(key) + statements.extend((entry.name, statement) for statement in temporary.statements) + return TemporaryTestFunctions( + statements=tuple(statements), + created_keys=frozenset(created_keys), + replacements=replacements, + failures=failures, + ) + + +def install_temporary_test_functions( + *, + adapter: BaseAdapter, + connection: Any, + functions: TemporaryTestFunctions, + connection_label: str, +) -> None: + """Create the copies the first connection created on another one, or fail with T006.""" + + function_name: str + statement: str + for function_name, statement in functions.statements: + try: + _ = adapter.execute(connection=connection, sql=statement) + except (ItemTimeoutError, ExecutionDurationLimitError): + raise + except Exception as error: + raise TemporaryTestFunctionError( + function_name=function_name, + connection=connection_label, + reason=DiagnosticRecordRedactor.text(str(error)) or type(error).__name__, + ) from error + + +def called_function_keys( + *, + function_entries: tuple[FunctionPlanEntry, ...], + keys: Iterable[CompiledObjectKey], + adapter: BaseAdapter, +) -> tuple[CompiledObjectKey, ...]: + """The planned functions among `keys` and every planned function their bodies call.""" + + return _dependency_order( + required_keys=keys, + entries_by_key=_entries_by_key(function_entries), + syntax=adapter.sql_lexical_syntax.native_mapping, + ) + + +def rewrite_test_entry( + *, entry: SqlTestPlanEntry, replacements: dict[str, str], syntax: dict[str, object] +) -> SqlTestPlanEntry: + """Point a test's function calls at their temporary copies.""" + + if not replacements: + return entry + return replace( + entry, + chain=tuple( + _rewrite_chain_step(step=step, replacements=replacements, syntax=syntax) + for step in entry.chain + ), + assertions=tuple( + _rewrite_assertion_step(step=step, replacements=replacements, syntax=syntax) + for step in entry.assertions + ), + ) + + +def rewrite_function_calls( + *, sql: str, replacements: dict[str, str], syntax: dict[str, object] +) -> str: + """Replace each deployed function target called in SQL code with its temporary target.""" + + if not replacements: + return sql + pieces: list[str] = [] + copied_to: int = 0 + match: re.Match[str] + for match in _code_matches(pattern=_call_pattern(targets=replacements), sql=sql, syntax=syntax): + pieces.append(sql[copied_to : match.start()]) + pieces.append(replacements[match.group(1)]) + copied_to = match.end() + pieces.append(sql[copied_to:]) + return "".join(pieces) + + +def _execute_statements( + *, adapter: BaseAdapter, connection: Any, statements: tuple[str, ...] +) -> str | None: + try: + statement: str + for statement in statements: + _ = adapter.execute(connection=connection, sql=statement) + except (ItemTimeoutError, ExecutionDurationLimitError): + raise + except Exception as error: + reason: str = DiagnosticRecordRedactor.text(str(error)) + log_debug_event( + logger=_LOGGER, + message="Temporary test function could not be created; using the built function", + sqlbuild_error=reason, + ) + return reason + return None + + +def _entries_by_key( + function_entries: tuple[FunctionPlanEntry, ...], +) -> dict[CompiledObjectKey, FunctionPlanEntry]: + return { + entry.key: entry + for entry in function_entries + if entry.destination.qualified_name is not None + } + + +def _keys_by_target( + entries_by_key: dict[CompiledObjectKey, FunctionPlanEntry], +) -> dict[str, CompiledObjectKey]: + return {str(entry.destination.qualified_name): key for key, entry in entries_by_key.items()} + + +def _dependency_order( + *, + required_keys: Iterable[CompiledObjectKey], + entries_by_key: dict[CompiledObjectKey, FunctionPlanEntry], + syntax: dict[str, object], +) -> tuple[CompiledObjectKey, ...]: + keys_by_target: dict[str, CompiledObjectKey] = _keys_by_target(entries_by_key) + ordered: list[CompiledObjectKey] = [] + visiting: set[CompiledObjectKey] = set() + pending: list[tuple[CompiledObjectKey, bool]] = [ + (key, False) for key in reversed(tuple(required_keys)) if key in entries_by_key + ] + while pending: + key, expanded = pending.pop() + if key in ordered: + continue + if expanded: + ordered.append(key) + continue + if key in visiting: + continue + visiting.add(key) + pending.append((key, True)) + pending.extend( + (dependency, False) + for dependency in _called_function_keys( + sql=entries_by_key[key].body_sql, keys_by_target=keys_by_target, syntax=syntax + ) + if dependency != key and dependency not in ordered + ) + return tuple(ordered) + + +def _called_function_keys( + *, sql: str, keys_by_target: dict[str, CompiledObjectKey], syntax: dict[str, object] +) -> tuple[CompiledObjectKey, ...]: + if not keys_by_target: + return () + return tuple( + keys_by_target[match.group(1)] + for match in _code_matches( + pattern=_call_pattern(targets=keys_by_target), sql=sql, syntax=syntax + ) + ) + + +def _call_pattern(*, targets: Iterable[str]) -> re.Pattern[str]: + alternatives: str = "|".join( + re.escape(target) for target in sorted(targets, key=lambda value: len(value), reverse=True) + ) + return re.compile(rf"(? tuple[re.Match[str], ...]: + matches: tuple[re.Match[str], ...] = tuple(pattern.finditer(sql)) + if not matches: + return () + hidden: tuple[tuple[int, int], ...] = _comment_and_string_ranges(sql=sql, syntax=syntax) + return tuple(match for match in matches if not _inside(offset=match.start(), ranges=hidden)) + + +def _inside(*, offset: int, ranges: tuple[tuple[int, int], ...]) -> bool: + start: int + end: int + for start, end in ranges: + if start <= offset < end: + return True + return False + + +def _comment_and_string_ranges( + *, sql: str, syntax: dict[str, object] +) -> tuple[tuple[int, int], ...]: + scanner: SqlCodeScanner = SqlCodeScanner(sql, syntax, _SCANNER_CONTEXT) + ranges: list[tuple[int, int]] = [] + index: int = 0 + while index < len(sql): + end: int | None = scanner.non_code_end(index) + if end is None: + index += 1 + continue + if sql[index] not in _IDENTIFIER_QUOTES: + ranges.append((index, end)) + index = max(end, index + 1) + return tuple(ranges) + + +def _rewrite_chain_step( + *, step: ChainStep, replacements: dict[str, str], syntax: dict[str, object] +) -> ChainStep: + return replace( + step, + resolved_sql=rewrite_function_calls( + sql=step.resolved_sql, replacements=replacements, syntax=syntax + ), + expected_cte_sql=_rewrite_optional( + sql=step.expected_cte_sql, replacements=replacements, syntax=syntax + ), + lifted_ctes=_rewrite_ctes(ctes=step.lifted_ctes, replacements=replacements, syntax=syntax), + comparison_body_sql=_rewrite_optional( + sql=step.comparison_body_sql, replacements=replacements, syntax=syntax + ), + expected_lifted_ctes=_rewrite_ctes( + ctes=step.expected_lifted_ctes, replacements=replacements, syntax=syntax + ), + ) + + +def _rewrite_assertion_step( + *, step: SqlTestAssertionStep, replacements: dict[str, str], syntax: dict[str, object] +) -> SqlTestAssertionStep: + return replace( + step, + resolved_sql=rewrite_function_calls( + sql=step.resolved_sql, replacements=replacements, syntax=syntax + ), + lifted_ctes=_rewrite_ctes(ctes=step.lifted_ctes, replacements=replacements, syntax=syntax), + comparison_body_sql=_rewrite_optional( + sql=step.comparison_body_sql, replacements=replacements, syntax=syntax + ), + ) + + +def _rewrite_optional( + *, sql: str | None, replacements: dict[str, str], syntax: dict[str, object] +) -> str | None: + if sql is None: + return None + return rewrite_function_calls(sql=sql, replacements=replacements, syntax=syntax) + + +def _rewrite_ctes( + *, ctes: tuple[tuple[str, str], ...], replacements: dict[str, str], syntax: dict[str, object] +) -> tuple[tuple[str, str], ...]: + return tuple( + (name, rewrite_function_calls(sql=sql, replacements=replacements, syntax=syntax)) + for name, sql in ctes + ) diff --git a/src/sqlbuild/executor/pipeline/_helpers/testing.py b/src/sqlbuild/executor/pipeline/_helpers/testing.py index da9881cdde..7ff175af98 100644 --- a/src/sqlbuild/executor/pipeline/_helpers/testing.py +++ b/src/sqlbuild/executor/pipeline/_helpers/testing.py @@ -30,10 +30,16 @@ close_connections, open_worker_connections, ) +from sqlbuild.executor.pipeline._helpers.temporary_functions import ( + called_function_keys, + create_temporary_test_functions, + install_temporary_test_functions, + rewrite_test_entry, +) from sqlbuild.executor.pipeline.main.raise_warehouse_connection_failure import ( raise_warehouse_connection_failure, ) -from sqlbuild.executor.pipeline.models import TestPipelineCallbacks +from sqlbuild.executor.pipeline.models import TemporaryTestFunctions, TestPipelineCallbacks from sqlbuild.executor.scheduling.classes.recovering_connection_pool import RecoveringConnectionPool from sqlbuild.executor.scheduling.types import ExecutionStatus from sqlbuild.executor.testing.constants import SQL_TEST_EXECUTION_ERROR_CODE @@ -99,8 +105,14 @@ def run_test_pipeline( worker_count, elapsed_seconds=time.monotonic() - start ) active_error: BaseException | None = None + installed_functions: list[TemporaryTestFunctions] = [] recovery: ConnectionRecovery = ConnectionRecovery( - adapter=adapter, connect=lambda: adapter.connect(dict(connection_config)) + adapter=adapter, + connect=lambda: _connect_test_connection( + adapter=adapter, + connection_config=connection_config, + installed_functions=installed_functions, + ), ) try: preflight_start: float = time.monotonic() @@ -114,11 +126,27 @@ def run_test_pipeline( metadata={"item_count": len(plan.test_entries)}, attributes=OperationAttributes(phase="setup", target_kind="sql_test"), ): - missing_functions_by_test = _prepare_test_functions( + missing_functions_by_test, temporary_functions = _prepare_test_functions( plan=plan, adapter=adapter, connection=connections[0], ) + for ordinal, connection in enumerate(connections[1:], start=2): + install_temporary_test_functions( + adapter=adapter, + connection=connection, + functions=temporary_functions, + connection_label=f"test connection {ordinal} of {len(connections)}", + ) + installed_functions.append(temporary_functions) + entries = tuple( + rewrite_test_entry( + entry=entry, + replacements=temporary_functions.replacements, + syntax=adapter.sql_lexical_syntax.native_mapping, + ) + for entry in entries + ) if resolved_callbacks.on_progress is not None: resolved_callbacks.on_progress( f"Prepared test functions. ({time.monotonic() - preflight_start:.2f}s)" @@ -286,25 +314,67 @@ def _test_resource_name(entry: SqlTestPlanEntry) -> str: return entry.name +def _connect_test_connection( + *, + adapter: BaseAdapter, + connection_config: dict[str, object], + installed_functions: list[TemporaryTestFunctions], +) -> Any: + connection: Any = adapter.connect(dict(connection_config)) + try: + functions: TemporaryTestFunctions + for functions in installed_functions: + install_temporary_test_functions( + adapter=adapter, + connection=connection, + functions=functions, + connection_label="a replacement test connection", + ) + except BaseException: + adapter.close(connection) + raise + return connection + + def _prepare_test_functions( *, plan: PlanOutput, adapter: BaseAdapter, connection: Any, -) -> dict[CompiledObjectKey, tuple[str, ...]]: +) -> tuple[dict[CompiledObjectKey, tuple[str, ...]], TemporaryTestFunctions]: function_entries: dict[CompiledObjectKey, FunctionPlanEntry] = { entry.key: entry for entry in plan.function_entries } + distinct_deps: set[tuple[CompiledObjectKey, ...]] = { + test_entry.function_deps for test_entry in plan.test_entries + } + closures: dict[tuple[CompiledObjectKey, ...], tuple[CompiledObjectKey, ...]] = { + deps: _function_closure(plan=plan, keys=deps, adapter=adapter) for deps in distinct_deps + } + deps_by_test: dict[CompiledObjectKey, tuple[CompiledObjectKey, ...]] = { + test_entry.key: closures[test_entry.function_deps] for test_entry in plan.test_entries + } required_targets_by_key: dict[CompiledObjectKey, CompiledRelationLocation] = {} test_entry: SqlTestPlanEntry for test_entry in plan.test_entries: dep: CompiledObjectKey - for dep in test_entry.function_deps: + for dep in deps_by_test[test_entry.key]: function_target: CompiledRelationLocation | None = plan.function_locations.get(dep.name) if function_target is not None: required_targets_by_key[dep] = function_target if not required_targets_by_key: - return {} + return {}, TemporaryTestFunctions() + temporary_functions: TemporaryTestFunctions = create_temporary_test_functions( + function_entries=plan.function_entries, + required_keys=tuple(required_targets_by_key), + adapter=adapter, + connection=connection, + ) + created_key: CompiledObjectKey + for created_key in temporary_functions.created_keys: + _ = required_targets_by_key.pop(created_key, None) + if not required_targets_by_key: + return {}, temporary_functions existing_function_names: set[tuple[str | None, str | None, str]] = set() for database, schema, names in _group_function_names(required_targets_by_key.values()): @@ -333,7 +403,9 @@ def _prepare_test_functions( _normalize_required_name(target.name), ) if function_key not in existing_function_names: - missing_by_key[key] = key.name + missing_by_key[key] = _missing_function_label( + name=key.name, reason=temporary_functions.failures.get(key) + ) _register_connection_scoped_python_functions( missing_by_key=missing_by_key, @@ -345,11 +417,26 @@ def _prepare_test_functions( missing_by_test: dict[CompiledObjectKey, tuple[str, ...]] = {} for test_entry in plan.test_entries: missing_names: tuple[str, ...] = tuple( - missing_by_key[dep] for dep in test_entry.function_deps if dep in missing_by_key + missing_by_key[dep] for dep in deps_by_test[test_entry.key] if dep in missing_by_key ) if missing_names: missing_by_test[test_entry.key] = missing_names - return missing_by_test + return missing_by_test, temporary_functions + + +def _function_closure( + *, plan: PlanOutput, keys: tuple[CompiledObjectKey, ...], adapter: BaseAdapter +) -> tuple[CompiledObjectKey, ...]: + called: tuple[CompiledObjectKey, ...] = called_function_keys( + function_entries=plan.function_entries, keys=keys, adapter=adapter + ) + return tuple(dict.fromkeys((*keys, *called))) + + +def _missing_function_label(*, name: str, reason: str | None) -> str: + if reason is None: + return name + return f"{name} ({reason})" def _register_connection_scoped_python_functions( diff --git a/src/sqlbuild/executor/pipeline/exceptions.py b/src/sqlbuild/executor/pipeline/exceptions.py index 889f24321c..e56371e14d 100644 --- a/src/sqlbuild/executor/pipeline/exceptions.py +++ b/src/sqlbuild/executor/pipeline/exceptions.py @@ -45,3 +45,20 @@ def __init__(self, *, adapter_name: str, reason: str) -> None: "Check the connection settings of the target; `sqb debug` tests the connection." ) super().__init__(self.message) + + +class TemporaryTestFunctionError(Exception): + """Raised when a test connection cannot create the temporary function copies tests call.""" + + code: str = "T006" + + def __init__(self, *, function_name: str, connection: str, reason: str) -> None: + self.message: str = ( + f"could not create the temporary copy of function {function_name} on {connection}: " + f"{reason}" + ) + self.help: str = ( + "The tests call the temporary copies, which the first test connection created; " + "check that every connection of the target may create temporary functions." + ) + super().__init__(self.message) diff --git a/src/sqlbuild/executor/pipeline/models.py b/src/sqlbuild/executor/pipeline/models.py index 9190a261b0..c821813fe3 100644 --- a/src/sqlbuild/executor/pipeline/models.py +++ b/src/sqlbuild/executor/pipeline/models.py @@ -3,10 +3,10 @@ from __future__ import annotations from collections.abc import Callable -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any -from sqlbuild._native import AuditPlanEntry, SqlTestPlanEntry +from sqlbuild._native import AuditPlanEntry, CompiledObjectKey, SqlTestPlanEntry from sqlbuild.executor.auditing.models import AuditExecutionResult from sqlbuild.executor.build.models import ( BuildCallbacks, @@ -43,6 +43,17 @@ class TestPipelineCallbacks: on_test_complete: Callable[[SqlTestExecutionResult], None] | None = None +@dataclass(frozen=True) +class TemporaryTestFunctions: + """Temporary function copies created for one test run and the functions left without one.""" + + statements: tuple[tuple[str, str], ...] = () + """Each `(function name, statement)` that created a copy, in creation order.""" + created_keys: frozenset[CompiledObjectKey] = frozenset() + replacements: dict[str, str] = field(default_factory=dict) + failures: dict[CompiledObjectKey, str] = field(default_factory=dict) + + @dataclass(frozen=True) class AuditExecutionFailure: """Safe public description of one standalone audit runtime failure.""" diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/test/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/test/_test_types.py index 4a49c1b251..a8353abd4d 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/test/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/test/_test_types.py @@ -438,3 +438,35 @@ class TransactionTimeoutE2ETestCase: expected_output_fragments: tuple[str, ...] expected_published_rows: tuple[tuple[int, int], ...] expected_later_model_rows: tuple[tuple[int], ...] + + +@dataclass(frozen=True) +class TemporaryFunctionE2ETestCase: + """Test case for unit tests that call an edited project function.""" + + description: str + built_body: str + edited_body: str + expected_exit_code: int + expected_stdout_fragment: str + expected_built_result: bool + + +@dataclass(frozen=True) +class TemporaryCalleeE2ETestCase: + """Test case for unit tests whose function calls another project function.""" + + description: str + build_first: bool + callee_body: str + expected_exit_code: int + expected_stdout_fragment: str + + +@dataclass(frozen=True) +class TemporaryFunctionConnectionE2ETestCase: + """Test case for a test connection that cannot create the temporary copies.""" + + description: str + expected_exit_code: int + expected_stderr_fragment: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/test/test_temporary_functions.py b/tests/e2e/src/sqlbuild/cli/commands/main/test/test_temporary_functions.py new file mode 100644 index 0000000000..7ca584b869 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/test/test_temporary_functions.py @@ -0,0 +1,312 @@ +"""Unit tests call the project's current function definitions without a build.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.test._test_types import ( + TemporaryCalleeE2ETestCase, + TemporaryFunctionConnectionE2ETestCase, + TemporaryFunctionE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import ( + execute_duckdb, + prepare_inline_project, + query_duckdb, + run_sqb, +) + +_FUNCTION_PATH: str = "functions/sql/is_completed_order.sql" +_FUNCTION_HEADER: str = ( + "FUNCTION (description 'Whether an order status counts as completed.', " + "arguments (order_status VARCHAR), returns BOOLEAN);\n" +) +_PROJECT_FILES: dict[str, str] = { + "sqlbuild_project.toml": ( + 'name = "orders_temporary_functions"\nadapter = "duckdb"\n' + '[connection]\ndatabase = "orders.duckdb"\n' + ), + "models/order_statuses.sql": ( + "MODEL (description 'Test model order_statuses.', materialized table);\n" + "SELECT 'completed' AS order_status\n" + ), + "models/orders.sql": ( + "MODEL (description 'Test model orders.', materialized table);\n" + 'SELECT order_status, __udf("is_completed_order")(order_status) AS is_completed ' + 'FROM __ref("order_statuses")\n' + ), + "tests/unit/orders_case.sql": ( + 'TEST (name "orders_case");\nWITH\n' + "__ref__order_statuses AS (SELECT 'completed' AS order_status),\n" + "__expected__orders AS (SELECT 'completed' AS order_status, TRUE AS is_completed)\n" + "SELECT 1\n" + ), +} + +_CALLEE_PATH: str = "functions/sql/status_matches.sql" +_CALLEE_HEADER: str = ( + "FUNCTION (description 'Whether an order status is the completed one.', " + "arguments (order_status VARCHAR), returns BOOLEAN);\n" +) +_CALLER_FILES: dict[str, str] = { + **_PROJECT_FILES, + _FUNCTION_PATH: _FUNCTION_HEADER + '__udf("status_matches")(order_status)', +} +_QUOTED_CALL_FILES: dict[str, str] = { + **_PROJECT_FILES, + _FUNCTION_PATH: _FUNCTION_HEADER + "order_status = 'completed'", + "models/orders.sql": ( + "MODEL (description 'Test model orders.', materialized table);\n" + 'SELECT order_status, __udf("is_completed_order")(order_status) AS is_completed, ' + "length('main.is_completed_order(order_status)') AS formula_length " + "-- main.is_completed_order(order_status)\n" + 'FROM __ref("order_statuses")\n' + ), + "tests/unit/orders_case.sql": ( + 'TEST (name "orders_case");\nWITH\n' + "__ref__order_statuses AS (SELECT 'completed' AS order_status),\n" + "__expected__orders AS (SELECT 'completed' AS order_status, TRUE AS is_completed, " + "37 AS formula_length)\n" + "SELECT 1\n" + ), +} +_SECOND_CONNECTION_FAILS: dict[str, str] = { + **_PROJECT_FILES, + "sqlbuild_project.toml": _PROJECT_FILES["sqlbuild_project.toml"].replace( + 'adapter = "duckdb"', 'adapter = "duckdb_one_session"' + ), + _FUNCTION_PATH: _FUNCTION_HEADER + "order_status = 'completed'", + "tests/unit/orders_again.sql": _PROJECT_FILES["tests/unit/orders_case.sql"].replace( + "orders_case", "orders_again" + ), + "adapters/duckdb_one_session.py": ( + "from sqlbuild.adapters.duckdb.classes.duckdb_adapter import DuckDbAdapter\n\n\n" + "class DuckDbOneSessionAdapter(DuckDbAdapter):\n" + ' adapter_name = "duckdb_one_session"\n' + " macro_sessions = []\n\n" + " def execute(self, *, connection, sql):\n" + ' if sql.startswith("CREATE OR REPLACE TEMP MACRO"):\n' + " if not self.macro_sessions:\n" + " self.macro_sessions.append(connection)\n" + " if connection is not self.macro_sessions[0]:\n" + ' raise PermissionError("temporary macros are disabled on this session")\n' + " return super().execute(connection=connection, sql=sql)\n" + ), +} + + +@pytest.mark.parametrize( + "test_case", + [ + TemporaryFunctionE2ETestCase( + description="edited function body decides the test without a rebuild", + built_body="order_status = 'completed'", + edited_body="order_status = 'shipped'", + expected_exit_code=1, + expected_stdout_fragment="FAIL=1", + expected_built_result=True, + ), + TemporaryFunctionE2ETestCase( + description="fixed function body passes before the built copy is replaced", + built_body="order_status = 'shipped'", + edited_body="order_status = 'completed'", + expected_exit_code=0, + expected_stdout_fragment="PASS=1", + expected_built_result=False, + ), + ], + ids=lambda case: case.description, +) +def test_given_edited_function_when_testing_then_current_definition_runs_and_build_is_untouched( + test_case: TemporaryFunctionE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders_temporary_functions", + repo_files={**_PROJECT_FILES, _FUNCTION_PATH: _FUNCTION_HEADER + test_case.built_body}, + ) + built: subprocess.CompletedProcess[str] = run_sqb( + command=("build", "--no-tests", "--no-audits"), project_dir=project_dir + ) + _ = (project_dir / _FUNCTION_PATH).write_text( + _FUNCTION_HEADER + test_case.edited_body, encoding="utf-8" + ) + + tested: subprocess.CompletedProcess[str] = run_sqb(command=("test",), project_dir=project_dir) + + assert built.returncode == 0, built.stdout + built.stderr + built_rows: list[tuple[Any, ...]] = query_duckdb( + db_path=project_dir / "orders.duckdb", + sql="SELECT main.is_completed_order('completed')", + ) + assert tested.returncode == test_case.expected_exit_code, ( + test_case.description, + tested.stdout + tested.stderr, + ) + assert test_case.expected_stdout_fragment in tested.stdout, test_case.description + assert built_rows == [(test_case.expected_built_result,)], test_case.description + + +@pytest.mark.parametrize( + "test_case", + [ + TemporaryFunctionE2ETestCase( + description="never-built function runs from its temporary copy", + built_body="order_status = 'completed'", + edited_body="order_status = 'completed'", + expected_exit_code=0, + expected_stdout_fragment="PASS=1", + expected_built_result=False, + ), + ], + ids=lambda case: case.description, +) +def test_given_unbuilt_function_when_testing_then_test_passes_and_no_function_is_created( + test_case: TemporaryFunctionE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders_temporary_functions", + repo_files={**_PROJECT_FILES, _FUNCTION_PATH: _FUNCTION_HEADER + test_case.edited_body}, + ) + execute_duckdb(db_path=project_dir / "orders.duckdb", sql="SELECT 1") + + tested: subprocess.CompletedProcess[str] = run_sqb(command=("test",), project_dir=project_dir) + + function_rows: list[tuple[Any, ...]] = query_duckdb( + db_path=project_dir / "orders.duckdb", + sql=( + "SELECT count(*) > 0 FROM duckdb_functions() WHERE function_name = 'is_completed_order'" + ), + ) + assert tested.returncode == test_case.expected_exit_code, ( + test_case.description, + tested.stdout + tested.stderr, + ) + assert test_case.expected_stdout_fragment in tested.stdout, test_case.description + assert function_rows == [(test_case.expected_built_result,)], test_case.description + + +@pytest.mark.parametrize( + "test_case", + [ + TemporaryCalleeE2ETestCase( + description="edited callee decides the caller's test without a rebuild", + build_first=True, + callee_body="order_status = 'shipped'", + expected_exit_code=1, + expected_stdout_fragment="FAIL=1", + ), + TemporaryCalleeE2ETestCase( + description="never-built caller and callee run from temporary copies", + build_first=False, + callee_body="order_status = 'completed'", + expected_exit_code=0, + expected_stdout_fragment="PASS=1", + ), + ], + ids=lambda case: case.description, +) +def test_given_function_calling_another_when_testing_then_callee_current_definition_runs( + test_case: TemporaryCalleeE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders_temporary_callee", + repo_files={**_CALLER_FILES, _CALLEE_PATH: _CALLEE_HEADER + "order_status = 'completed'"}, + ) + commands: dict[bool, tuple[tuple[str, ...], ...]] = { + True: (("build", "--no-tests", "--no-audits"),), + False: (), + } + built: tuple[subprocess.CompletedProcess[str], ...] = tuple( + run_sqb(command=command, project_dir=project_dir) + for command in commands[test_case.build_first] + ) + _ = (project_dir / _CALLEE_PATH).write_text( + _CALLEE_HEADER + test_case.callee_body, encoding="utf-8" + ) + + tested: subprocess.CompletedProcess[str] = run_sqb(command=("test",), project_dir=project_dir) + + assert all(result.returncode == 0 for result in built), test_case.description + assert tested.returncode == test_case.expected_exit_code, ( + test_case.description, + tested.stdout + tested.stderr, + ) + assert test_case.expected_stdout_fragment in tested.stdout, test_case.description + + +@pytest.mark.parametrize( + "test_case", + [ + TemporaryFunctionConnectionE2ETestCase( + description="second test connection refusing the copies is a coded error", + expected_exit_code=1, + expected_stderr_fragment=( + "error[T006]: could not create the temporary copy of function " + "is_completed_order on test connection 2 of 2: " + "temporary macros are disabled on this session" + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_connection_refusing_copies_when_testing_concurrently_then_reports_coded_error( + test_case: TemporaryFunctionConnectionE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders_temporary_functions", + repo_files=_SECOND_CONNECTION_FAILS, + ) + + tested: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "test", "--concurrency", "2"), project_dir=project_dir + ) + + output: str = tested.stdout + tested.stderr + assert tested.returncode == test_case.expected_exit_code, (test_case.description, output) + assert test_case.expected_stderr_fragment in tested.stderr, (test_case.description, output) + assert "Traceback" not in output, (test_case.description, output) + + +@pytest.mark.parametrize( + "test_case", + [ + TemporaryFunctionE2ETestCase( + description="function calls quoted in strings and comments keep their text", + built_body="order_status = 'completed'", + edited_body="order_status = 'completed'", + expected_exit_code=0, + expected_stdout_fragment="PASS=1", + expected_built_result=False, + ), + ], + ids=lambda case: case.description, +) +def test_given_function_call_in_string_when_testing_then_only_code_calls_are_redirected( + test_case: TemporaryFunctionE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders_temporary_functions", + repo_files=_QUOTED_CALL_FILES, + ) + + tested: subprocess.CompletedProcess[str] = run_sqb(command=("test",), project_dir=project_dir) + + assert tested.returncode == test_case.expected_exit_code, ( + test_case.description, + tested.stdout + tested.stderr, + ) + assert test_case.expected_stdout_fragment in tested.stdout, test_case.description diff --git a/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/_test_types.py b/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/_test_types.py index 0bb976dcc0..099e407f97 100644 --- a/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/_test_types.py +++ b/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/_test_types.py @@ -1,5 +1,9 @@ from dataclasses import dataclass +from sqlbuild.adapter.contract.classes.strict_adapter import StrictAdapter +from sqlbuild.adapter.contract.models import TemporaryFunction +from sqlbuild.compiler.compile.types import FunctionLanguage + @dataclass(frozen=True) class StrictAdapterContractTestCase: @@ -25,6 +29,7 @@ class OptionalStrictMethodTestCase: description: str expected_optional_methods: frozenset[str] expected_output_relation: None + expected_temporary_function: None @dataclass(frozen=True) @@ -33,3 +38,12 @@ class StrictAdapterInlineSqlTestTestCase: expected_total: int expected_outcome: str expected_statement_count: int + + +@dataclass(frozen=True) +class TemporaryFunctionRenderingTestCase: + description: str + adapter: StrictAdapter + language: FunctionLanguage + return_columns: tuple[tuple[str, str], ...] + expected_temporary_function: TemporaryFunction | None diff --git a/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_strict_adapter.py b/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_strict_adapter.py index f1951d4d43..3abd653077 100644 --- a/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_strict_adapter.py +++ b/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_strict_adapter.py @@ -17,6 +17,7 @@ from sqlbuild.adapter.contract.classes.base_adapter import BaseAdapter from sqlbuild.adapter.contract.classes.strict_adapter import StrictAdapter from sqlbuild.adapter.contract.exceptions import AdapterUserError +from sqlbuild.adapter.contract.models import FunctionDefinition, TemporaryFunction from sqlbuild.adapters.duckdb.classes.duckdb_adapter import DuckDbAdapter from sqlbuild.executor.pipeline._helpers.testing import run_test_pipeline from sqlbuild.executor.testing.models import SqlTestExecutionResult @@ -84,8 +85,15 @@ def test_given_first_class_adapters_when_checking_contract_then_no_method_comes_ [ OptionalStrictMethodTestCase( description="unit-test output storage is optional and defaults to inline", - expected_optional_methods=frozenset({"create_sql_test_output", "drop_sql_test_output"}), + expected_optional_methods=frozenset( + { + "create_sql_test_output", + "drop_sql_test_output", + "render_create_temporary_function", + } + ), expected_output_relation=None, + expected_temporary_function=None, ) ], ids=lambda case: case.description, @@ -100,10 +108,17 @@ def test_given_strict_adapter_when_reading_optional_methods_then_defaults_keep_i connection=None, sql="SELECT 1", name="orders_output" ) adapter.drop_sql_test_output(connection=None, relation="orders_output") + temporary: TemporaryFunction | None = adapter.render_create_temporary_function( + name="is_completed_order", + definition=FunctionDefinition( + destination="main.is_completed_order", arguments=(), returns="BOOLEAN", body_sql="TRUE" + ), + ) assert test_case.expected_optional_methods.isdisjoint(abstract_methods) assert test_case.expected_optional_methods <= frozenset(StrictAdapter.__dict__) assert relation is test_case.expected_output_relation + assert temporary is test_case.expected_temporary_function @pytest.mark.parametrize( diff --git a/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_temporary_function_rendering.py b/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_temporary_function_rendering.py new file mode 100644 index 0000000000..3fe66e580b --- /dev/null +++ b/tests/unit/src/sqlbuild/adapter/contract/classes/strict_adapter/test_temporary_function_rendering.py @@ -0,0 +1,146 @@ +from __future__ import annotations + +import pytest + +from sqlbuild._native import FunctionArgument, FunctionReturnColumn +from sqlbuild.adapter.contract.models import FunctionDefinition, TemporaryFunction +from sqlbuild.adapters.bigquery.classes.bigquery_adapter import BigQueryAdapter +from sqlbuild.adapters.databricks.classes.databricks_adapter import DatabricksAdapter +from sqlbuild.adapters.duckdb.classes.duckdb_adapter import DuckDbAdapter +from sqlbuild.adapters.postgres.classes.postgres_adapter import PostgresAdapter +from sqlbuild.adapters.snowflake.classes.snowflake_adapter import SnowflakeAdapter +from sqlbuild.adapters.sqlserver.classes.sqlserver_adapter import SqlServerAdapter +from sqlbuild.compiler.compile.types import FunctionLanguage +from tests.unit.src.sqlbuild.adapter.contract.classes.strict_adapter._test_types import ( + TemporaryFunctionRenderingTestCase, +) + + +@pytest.mark.parametrize( + "test_case", + [ + TemporaryFunctionRenderingTestCase( + description="duckdb copies a sql function into a temporary macro", + adapter=DuckDbAdapter(), + language=FunctionLanguage.SQL, + return_columns=(), + expected_temporary_function=TemporaryFunction( + target='temp.main."is_completed_order"', + statements=( + 'CREATE OR REPLACE TEMP MACRO "is_completed_order"(order_status) AS (\n' + "order_status = 'completed'\n)", + ), + ), + ), + TemporaryFunctionRenderingTestCase( + description="duckdb copies a table function into a temporary table macro", + adapter=DuckDbAdapter(), + language=FunctionLanguage.SQL, + return_columns=(("order_id", "INTEGER"),), + expected_temporary_function=TemporaryFunction( + target='temp.main."is_completed_order"', + statements=( + 'CREATE OR REPLACE TEMP MACRO "is_completed_order"(order_status) AS TABLE\n' + "order_status = 'completed'", + ), + ), + ), + TemporaryFunctionRenderingTestCase( + description="duckdb leaves python functions to per-connection registration", + adapter=DuckDbAdapter(), + language=FunctionLanguage.PYTHON, + return_columns=(), + expected_temporary_function=None, + ), + TemporaryFunctionRenderingTestCase( + description="postgres creates the copy in the session pg_temp schema", + adapter=PostgresAdapter(), + language=FunctionLanguage.SQL, + return_columns=(), + expected_temporary_function=TemporaryFunction( + target='pg_temp."is_completed_order"', + statements=( + 'CREATE OR REPLACE FUNCTION pg_temp."is_completed_order"(order_status TEXT)\n' + "RETURNS BOOLEAN\n" + "LANGUAGE SQL AS $$\n" + "order_status = 'completed'\n" + "$$", + ), + ), + ), + TemporaryFunctionRenderingTestCase( + description="snowflake creates a prefixed temporary function in the current schema", + adapter=SnowflakeAdapter(), + language=FunctionLanguage.SQL, + return_columns=(), + expected_temporary_function=TemporaryFunction( + target='"__SQB_TEST__IS_COMPLETED_ORDER"', + statements=( + "CREATE OR REPLACE TEMPORARY FUNCTION " + '"__SQB_TEST__IS_COMPLETED_ORDER"(order_status TEXT)\n' + "RETURNS BOOLEAN\n" + "LANGUAGE SQL\n" + "AS $$\n" + "order_status = 'completed'\n" + "$$", + ), + ), + ), + TemporaryFunctionRenderingTestCase( + description="bigquery has no session to hold a temporary function", + adapter=BigQueryAdapter(), + language=FunctionLanguage.SQL, + return_columns=(), + expected_temporary_function=None, + ), + TemporaryFunctionRenderingTestCase( + description="databricks creates a prefixed session temporary function", + adapter=DatabricksAdapter(), + language=FunctionLanguage.SQL, + return_columns=(), + expected_temporary_function=TemporaryFunction( + target="`__sqb_test__is_completed_order`", + statements=( + "CREATE OR REPLACE TEMPORARY FUNCTION " + "`__sqb_test__is_completed_order`(order_status TEXT)\n" + "RETURNS BOOLEAN\n" + "RETURN order_status = 'completed'", + ), + ), + ), + TemporaryFunctionRenderingTestCase( + description="databricks keeps using built python functions", + adapter=DatabricksAdapter(), + language=FunctionLanguage.PYTHON, + return_columns=(), + expected_temporary_function=None, + ), + TemporaryFunctionRenderingTestCase( + description="sql server keeps using the built function", + adapter=SqlServerAdapter(), + language=FunctionLanguage.SQL, + return_columns=(), + expected_temporary_function=None, + ), + ], + ids=lambda case: case.description, +) +def test_given_project_function_when_rendering_temporary_copy_then_adapter_returns_session_ddl( + test_case: TemporaryFunctionRenderingTestCase, +) -> None: + temporary: TemporaryFunction | None = test_case.adapter.render_create_temporary_function( + name="is_completed_order", + definition=FunctionDefinition( + destination="analytics.is_completed_order", + arguments=(FunctionArgument(name="order_status", type="TEXT"),), + returns="BOOLEAN", + body_sql="order_status = 'completed'", + return_columns=tuple( + FunctionReturnColumn(name=name, type=type_name) + for name, type_name in test_case.return_columns + ), + language=test_case.language, + ), + ) + + assert temporary == test_case.expected_temporary_function, test_case.description diff --git a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/_test_types.py b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/_test_types.py index ea148bd256..7853c224f1 100644 --- a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/_test_types.py +++ b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/_test_types.py @@ -185,3 +185,20 @@ class ScenarioWorkerExitTestCase: description: str worker_count: int expected_returncode: int + + +@dataclass(frozen=True) +class TemporaryTestFunctionTestCase: + description: str + body_sql: str + test_count: int + max_concurrency: int + expected_outcomes: tuple[str, ...] + expected_error_fragments: tuple[str, ...] + + +@dataclass(frozen=True) +class FunctionCallRewriteTestCase: + description: str + sql: str + expected_sql: str diff --git a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/helpers.py b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/helpers.py index 4b292bd609..9c59b2342d 100644 --- a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/helpers.py +++ b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/helpers.py @@ -4,12 +4,14 @@ import threading from collections.abc import Callable +from dataclasses import replace from pathlib import Path from typing import Any, ClassVar from unittest.mock import MagicMock from sqlbuild._native import ( AuditPlanEntry, + ChainStep, CompiledObjectKey, CompiledProject, CompiledRelationLocation, @@ -17,6 +19,8 @@ CompiledSqlScenario, CompilePipelineResult, DiscoveredSqlScenarioFile, + FunctionArgument, + FunctionPlanEntry, PlanOutput, ScenarioExecutionPlan, ScenarioGraphPlan, @@ -25,6 +29,7 @@ SeedCsvSettings, SeedPlanEntry, SqlLexicalSyntax, + SqlTestPlanEntry, ) from sqlbuild.adapter.contract.classes.base_adapter import BaseAdapter from sqlbuild.adapter.contract.classes.statement_recorder import StatementRecorder @@ -320,3 +325,87 @@ def load_snapshot_failure(**_kwargs: object) -> ScenarioLocalSnapshotLoadResult: True: load_snapshot_success, False: load_snapshot_failure, }[test_case.load_error_message is None] + + +def order_status_function_test_plan(*, body_sql: str, test_count: int) -> PlanOutput: + """Plan of tests calling one unbuilt project function whose current body is `body_sql`.""" + + function_key: CompiledObjectKey = CompiledObjectKey( + resource_type=CompiledResourceType.UDF, name="is_completed_order" + ) + destination: CompiledRelationLocation = CompiledRelationLocation( + database=None, + schema="analytics", + name="is_completed_order", + qualified_name="analytics.is_completed_order", + ) + return PlanOutput( + test_entries=tuple( + SqlTestPlanEntry( + key=CompiledObjectKey( + resource_type=CompiledResourceType.SQL_TEST, name=f"test_orders_{index}" + ), + name=f"test_orders_{index}", + chain=( + ChainStep( + model_name="orders", + resolved_sql=( + "SELECT analytics.is_completed_order ('completed') AS is_completed" + ), + expected_cte_sql="SELECT TRUE AS is_completed", + ), + ), + function_deps=(function_key,), + ) + for index in range(test_count) + ), + function_entries=( + FunctionPlanEntry( + key=function_key, + name="is_completed_order", + relative_path=Path("functions/sql/is_completed_order.sql"), + destination=destination, + arguments=(FunctionArgument(name="order_status", type="VARCHAR"),), + returns="BOOLEAN", + body_sql=body_sql, + fingerprint_query_sql=body_sql, + fingerprint_destination=destination, + ), + ), + function_locations={"is_completed_order": destination}, + ) + + +def order_status_callee_test_plan(*, callee_body_sql: str, test_count: int) -> PlanOutput: + """Plan of tests calling `is_completed_order`, which calls `status_matches` with this body.""" + + plan: PlanOutput = order_status_function_test_plan( + body_sql="analytics.status_matches(order_status)", test_count=test_count + ) + callee_key: CompiledObjectKey = CompiledObjectKey( + resource_type=CompiledResourceType.UDF, name="status_matches" + ) + callee_destination: CompiledRelationLocation = CompiledRelationLocation( + database=None, + schema="analytics", + name="status_matches", + qualified_name="analytics.status_matches", + ) + return replace( + plan, + function_entries=( + *plan.function_entries, + FunctionPlanEntry( + key=callee_key, + name="status_matches", + relative_path=Path("functions/sql/status_matches.sql"), + destination=callee_destination, + arguments=(FunctionArgument(name="order_status", type="VARCHAR"),), + returns="BOOLEAN", + body_sql=callee_body_sql, + fingerprint_query_sql=callee_body_sql, + fingerprint_destination=callee_destination, + ), + ), + function_locations={**plan.function_locations, "status_matches": callee_destination}, + ) diff --git a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_temporary_functions.py b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_temporary_functions.py new file mode 100644 index 0000000000..b3057f1563 --- /dev/null +++ b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_temporary_functions.py @@ -0,0 +1,75 @@ +"""Tests for rewriting test SQL onto temporary function copies.""" + +from __future__ import annotations + +import pytest + +from sqlbuild.adapters.duckdb.classes.duckdb_adapter import DuckDbAdapter +from sqlbuild.executor.pipeline._helpers.temporary_functions import rewrite_function_calls +from tests.unit.src.sqlbuild.executor.pipeline._helpers._test_types import ( + FunctionCallRewriteTestCase, +) + +_REPLACEMENTS: dict[str, str] = { + "analytics.is_completed_order": 'temp.main."is_completed_order"', + '"warehouse"."analytics"."customer_orders"': 'temp.main."customer_orders"', +} + + +@pytest.mark.parametrize( + "test_case", + [ + FunctionCallRewriteTestCase( + description="scalar call moves to the temporary copy", + sql="SELECT analytics.is_completed_order(status) FROM orders", + expected_sql='SELECT temp.main."is_completed_order"(status) FROM orders', + ), + FunctionCallRewriteTestCase( + description="whitespace before the argument list still matches", + sql="SELECT analytics.is_completed_order (status)", + expected_sql='SELECT temp.main."is_completed_order" (status)', + ), + FunctionCallRewriteTestCase( + description="quoted table function target moves to the temporary copy", + sql='SELECT * FROM "warehouse"."analytics"."customer_orders"(7)', + expected_sql='SELECT * FROM temp.main."customer_orders"(7)', + ), + FunctionCallRewriteTestCase( + description="longer names and non-call mentions stay unchanged", + sql=( + "SELECT staging.analytics.is_completed_order(status), " + "analytics.is_completed_order_v2(status), " + "'analytics.is_completed_order' AS label" + ), + expected_sql=( + "SELECT staging.analytics.is_completed_order(status), " + "analytics.is_completed_order_v2(status), " + "'analytics.is_completed_order' AS label" + ), + ), + FunctionCallRewriteTestCase( + description="calls inside string literals and comments stay unchanged", + sql=( + "SELECT analytics.is_completed_order(status), " + "'analytics.is_completed_order(x)' AS label -- analytics.is_completed_order(y)\n" + "/* analytics.is_completed_order(z) */ FROM orders" + ), + expected_sql=( + 'SELECT temp.main."is_completed_order"(status), ' + "'analytics.is_completed_order(x)' AS label -- analytics.is_completed_order(y)\n" + "/* analytics.is_completed_order(z) */ FROM orders" + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_deployed_function_calls_when_rewriting_then_only_calls_move_to_temporary_copies( + test_case: FunctionCallRewriteTestCase, +) -> None: + rewritten: str = rewrite_function_calls( + sql=test_case.sql, + replacements=_REPLACEMENTS, + syntax=DuckDbAdapter.sql_lexical_syntax.native_mapping, + ) + + assert rewritten == test_case.expected_sql, test_case.description diff --git a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_testing.py b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_testing.py index 8690416f6c..386b1b60c2 100644 --- a/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_testing.py +++ b/tests/unit/src/sqlbuild/executor/pipeline/_helpers/test_testing.py @@ -32,9 +32,12 @@ SqlTestConcurrencyTestCase, SqlTestFunctionPreflightTestCase, SqlTestOperationLifecycleTestCase, + TemporaryTestFunctionTestCase, ) from tests.unit.src.sqlbuild.executor.pipeline._helpers.helpers import ( lifecycle_events_with_prefix, + order_status_callee_test_plan, + order_status_function_test_plan, ) @@ -132,6 +135,129 @@ def record_event(event: LifecycleEvent) -> None: ) +@pytest.mark.parametrize( + "test_case", + [ + TemporaryTestFunctionTestCase( + description="unbuilt function runs from its temporary copy", + body_sql="order_status = 'completed'", + test_count=1, + max_concurrency=1, + expected_outcomes=("pass",), + expected_error_fragments=(), + ), + TemporaryTestFunctionTestCase( + description="every worker connection holds the temporary copy", + body_sql="order_status = 'completed'", + test_count=2, + max_concurrency=2, + expected_outcomes=("pass", "pass"), + expected_error_fragments=(), + ), + TemporaryTestFunctionTestCase( + description="current definition decides the outcome", + body_sql="order_status = 'cancelled'", + test_count=1, + max_concurrency=1, + expected_outcomes=("fail",), + expected_error_fragments=(), + ), + TemporaryTestFunctionTestCase( + description="uncreatable copy reports why and asks for a build", + body_sql="order_status = = 'completed'", + test_count=1, + max_concurrency=1, + expected_outcomes=("error",), + expected_error_fragments=( + "its temporary copy for the test run could not be created", + "Run `sqb build` first", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_unbuilt_function_when_running_tests_then_temporary_copy_holds_current_definition( + test_case: TemporaryTestFunctionTestCase, + tmp_path: Path, +) -> None: + results: tuple[SqlTestExecutionResult, ...] = run_test_pipeline( + plan=order_status_function_test_plan( + body_sql=test_case.body_sql, test_count=test_case.test_count + ), + connection_config={"database": str(tmp_path / "orders.duckdb")}, + adapter=DuckDbAdapter(), + max_concurrency=test_case.max_concurrency, + ) + + messages: str = " ".join(str(result.error_message) for result in results) + assert tuple(str(result.outcome) for result in results) == test_case.expected_outcomes, ( + test_case.description, + messages, + ) + assert all(fragment in messages for fragment in test_case.expected_error_fragments), ( + test_case.description, + messages, + ) + + +@pytest.mark.parametrize( + "test_case", + [ + TemporaryTestFunctionTestCase( + description="callee's current definition decides the caller's test", + body_sql="order_status = 'cancelled'", + test_count=1, + max_concurrency=1, + expected_outcomes=("fail",), + expected_error_fragments=(), + ), + TemporaryTestFunctionTestCase( + description="callee copied on every worker connection", + body_sql="order_status = 'completed'", + test_count=2, + max_concurrency=2, + expected_outcomes=("pass", "pass"), + expected_error_fragments=(), + ), + TemporaryTestFunctionTestCase( + description="caller without a copyable callee reports the callee", + body_sql="order_status = = 'completed'", + test_count=1, + max_concurrency=1, + expected_outcomes=("error",), + expected_error_fragments=( + "function status_matches, which it calls, has no temporary copy", + "status_matches (its temporary copy for the test run could not be created", + "Run `sqb build` first", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_unbuilt_callee_when_running_tests_then_callee_copy_holds_current_definition( + test_case: TemporaryTestFunctionTestCase, + tmp_path: Path, +) -> None: + results: tuple[SqlTestExecutionResult, ...] = run_test_pipeline( + plan=order_status_callee_test_plan( + callee_body_sql=test_case.body_sql, test_count=test_case.test_count + ), + connection_config={"database": str(tmp_path / "orders.duckdb")}, + adapter=DuckDbAdapter(), + max_concurrency=test_case.max_concurrency, + ) + + messages: str = " ".join(str(result.error_message) for result in results) + assert tuple(str(result.outcome) for result in results) == test_case.expected_outcomes, ( + test_case.description, + messages, + ) + assert all(fragment in messages for fragment in test_case.expected_error_fragments), ( + test_case.description, + messages, + ) + + @pytest.mark.parametrize( "test_case", ( diff --git a/website/src/content/docs/docs/concepts/adapters.mdx b/website/src/content/docs/docs/concepts/adapters.mdx index d65f2b3a77..5523b9cf9e 100644 --- a/website/src/content/docs/docs/concepts/adapters.mdx +++ b/website/src/content/docs/docs/concepts/adapters.mdx @@ -94,7 +94,7 @@ Set `sql_analysis_dialect_name` to the SQL dialect name that matches your engine For full control with no inherited defaults, subclass `StrictAdapter` instead. Every method is abstract and must be implemented explicitly. SQLBuild raises a clear error listing any unimplemented methods. -A few `StrictAdapter` methods are optional and have a documented default. `create_sql_test_output` and `drop_sql_test_output` let an adapter store a unit test's actual output once so every comparison reads it (see [Single-read comparison](/docs/concepts/testing#single-read-comparison)). By default `create_sql_test_output` returns `None`, and SQLBuild runs the test's inline comparison query instead, so adapters that do not implement them keep working unchanged. +A few `StrictAdapter` methods are optional and have a documented default. `create_sql_test_output` and `drop_sql_test_output` let an adapter store a unit test's actual output once so every comparison reads it (see [Single-read comparison](/docs/concepts/testing#single-read-comparison)). By default `create_sql_test_output` returns `None`, and SQLBuild runs the test's inline comparison query instead, so adapters that do not implement them keep working unchanged. `render_create_temporary_function` returns the call target and DDL for a session-scoped copy of a project function, which `sqb test` calls instead of the built function (see [Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test)). By default it returns `None`, and tests call the built function. ### Discovery rules diff --git a/website/src/content/docs/docs/concepts/adapters/bigquery.mdx b/website/src/content/docs/docs/concepts/adapters/bigquery.mdx index 3c9ec55a5d..9bcc286861 100644 --- a/website/src/content/docs/docs/concepts/adapters/bigquery.mdx +++ b/website/src/content/docs/docs/concepts/adapters/bigquery.mdx @@ -66,3 +66,8 @@ NUMERIC '2.4700' BigQuery does not support arrays of arrays. A nested list or set requested with `render_as array` fails at compile time rather than producing invalid BigQuery SQL. Parenthesized `value_list` rendering remains available for ordinary scalar collections used with `IN`. See [Collections and Rendering](/docs/concepts/constants/collections-and-rendering#native-array-rendering) for declarations, rendering precedence, and the cross-adapter matrix. + +## Functions in unit tests + +BigQuery temporary functions exist only inside one script, so `sqb test` calls the built function. +Run `sqb build` before testing a new or changed function. See [Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test). diff --git a/website/src/content/docs/docs/concepts/adapters/databricks.mdx b/website/src/content/docs/docs/concepts/adapters/databricks.mdx index a1ef6f4f4a..c017c54238 100644 --- a/website/src/content/docs/docs/concepts/adapters/databricks.mdx +++ b/website/src/content/docs/docs/concepts/adapters/databricks.mdx @@ -46,3 +46,9 @@ The target's `database` selects the Unity Catalog catalog, and its `schema` sele On connect, SQLBuild runs `USE CATALOG` and `USE SCHEMA` statements from the active target to set the authoritative session namespace. + +## Functions in unit tests + +`sqb test` creates each SQL UDF and table function the selected tests call as a session +`TEMPORARY FUNCTION __sqb_test__`. Python UDFs cannot be temporary, so tests call the built +Python function. See [Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test). diff --git a/website/src/content/docs/docs/concepts/adapters/duckdb.mdx b/website/src/content/docs/docs/concepts/adapters/duckdb.mdx index c42ac865a7..90b0e70a7d 100644 --- a/website/src/content/docs/docs/concepts/adapters/duckdb.mdx +++ b/website/src/content/docs/docs/concepts/adapters/duckdb.mdx @@ -57,3 +57,9 @@ DuckDB defaults to `staged` promotion: tables are materialized into a staging ta [settings] table_promotion_mode = "staged" ``` + +## Functions in unit tests + +`sqb test` copies each SQL UDF and table function the selected tests call into a `CREATE TEMP +MACRO` on every test connection, called as `temp.main.`, so tests use the current project +definition without a build. Python UDFs are registered on each test connection. See [Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test). diff --git a/website/src/content/docs/docs/concepts/adapters/motherduck.mdx b/website/src/content/docs/docs/concepts/adapters/motherduck.mdx index 380ae3bf0c..40963c1323 100644 --- a/website/src/content/docs/docs/concepts/adapters/motherduck.mdx +++ b/website/src/content/docs/docs/concepts/adapters/motherduck.mdx @@ -74,3 +74,9 @@ schema = "dev" ``` This lets you develop and test locally with zero MotherDuck compute cost, then deploy to MotherDuck in production. SQLBuild's [scenario replay](/docs/concepts/scenarios) also runs locally in DuckDB regardless of the production adapter. + +## Functions in unit tests + +`sqb test` copies each SQL UDF and table function the selected tests call into a session +`TEMP MACRO`, the same as [DuckDB](/docs/concepts/adapters/duckdb#functions-in-unit-tests), so the +functions in your MotherDuck database are not replaced. See [Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test). diff --git a/website/src/content/docs/docs/concepts/adapters/postgres.mdx b/website/src/content/docs/docs/concepts/adapters/postgres.mdx index 66752859c7..e0bb8c2153 100644 --- a/website/src/content/docs/docs/concepts/adapters/postgres.mdx +++ b/website/src/content/docs/docs/concepts/adapters/postgres.mdx @@ -61,3 +61,10 @@ connection = "postgres" database = "analytics" schema = "dev" ``` + +## Functions in unit tests + +`sqb test` creates each SQL function the selected tests call in the session's `pg_temp` schema and +calls it as `pg_temp.`, so tests use the current project definition without a build. Temporary +functions belong to one server session: behind a transaction-mode pooler, run `sqb test` through a +direct connection or a session-mode pool. See [Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test). diff --git a/website/src/content/docs/docs/concepts/adapters/snowflake.mdx b/website/src/content/docs/docs/concepts/adapters/snowflake.mdx index 9bac82061c..76d70017ad 100644 --- a/website/src/content/docs/docs/concepts/adapters/snowflake.mdx +++ b/website/src/content/docs/docs/concepts/adapters/snowflake.mdx @@ -124,3 +124,10 @@ not reconstruct invisible concurrent work, warehouse resume or idle tail, the 60 cloud-services credits, contract adjustments, or multi-cluster billing. Run metadata and query IDs are stored under `target/executions//`; that statement ledger stores only an SQL digest, not SQL text. Executed SQL artifacts are stored separately under the sensitive `target/run/` tree. + +## Functions in unit tests + +`sqb test` creates each UDF and table function the selected tests call, SQL or Python, as a +`TEMPORARY FUNCTION __SQB_TEST__` in the session's current schema. The session needs a current +schema and permission to create functions there; otherwise tests call the built function. See +[Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test). diff --git a/website/src/content/docs/docs/concepts/adapters/sqlserver.mdx b/website/src/content/docs/docs/concepts/adapters/sqlserver.mdx index 13f0132d91..7d9ef18ff7 100644 --- a/website/src/content/docs/docs/concepts/adapters/sqlserver.mdx +++ b/website/src/content/docs/docs/concepts/adapters/sqlserver.mdx @@ -81,3 +81,8 @@ connection = "sqlserver" database = "analytics" schema = "dev" ``` + +## Functions in unit tests + +SQL Server has no session-scoped functions, so `sqb test` calls the built function. Run `sqb build` +before testing a new or changed function. See [Functions in `sqb test`](/docs/concepts/testing#functions-in-sqb-test). diff --git a/website/src/content/docs/docs/concepts/testing.mdx b/website/src/content/docs/docs/concepts/testing.mdx index 53bac26675..ef3f447579 100644 --- a/website/src/content/docs/docs/concepts/testing.mdx +++ b/website/src/content/docs/docs/concepts/testing.mdx @@ -565,6 +565,40 @@ __udf_expected__ AS ( UDFs are warehouse objects, so the function is created before the test runs. During `sqb build`, UDF tests run after the function is created but before any model that uses it. +### Functions in `sqb test` + +`sqb test` does not need a build to test the current version of a function. Before the tests run, +SQLBuild creates a session-scoped temporary copy of every project UDF and table function the +selected tests call, including functions those functions call, from the definitions in your +project. The tests call the copies, so an edited function is tested without `sqb build`, and the +built function in the target schema is never created, replaced or dropped. The copies disappear +when the test connections close. + +| Adapter | Temporary copy | +|---|---| +| DuckDB, MotherDuck | `CREATE TEMP MACRO`, called as `temp.main.`. Python UDFs are registered on each test connection. | +| PostgreSQL | SQL function in the session's `pg_temp` schema, called as `pg_temp.` | +| Snowflake | `CREATE TEMPORARY FUNCTION __sqb_test__` in the session's current schema, for SQL and Python UDFs and table functions | +| Databricks | `CREATE TEMPORARY FUNCTION __sqb_test__` for SQL functions | +| BigQuery, SQL Server | Not supported; tests call the built function | + +When an adapter cannot create a copy (a Databricks Python UDF, for example), or the warehouse +refuses to create it (the role lacks permission, or the Snowflake session has no current schema), +the test calls the built function instead. If that function has not been built either, the test +fails with an error that names the function and the reason no copy was created, and asks you to run +`sqb build` first. +A function whose callee has no copy calls the built functions too, so one test never mixes copies +and built functions. + +SQLBuild decides which copies exist on the first test connection and then creates the same copies +on every other test connection, including a connection that replaces one a timeout left unusable. +If one of those connections cannot create a copy, `sqb test` stops with error `T006`, which names +the function and the connection. + +PostgreSQL temporary functions belong to one server session. Behind a transaction-mode connection +pooler, a test's statements can reach a different server session that does not hold the copies, so +run `sqb test` for projects with functions through a direct connection or a session-mode pool. + ### Table function tests Test table functions by calling them in `__table_fn_actual__` and comparing against `__table_fn_expected__`: From 1c94906a935895e83497ff1f159a57e51cd63ea8 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:39:39 +0100 Subject: [PATCH 12/14] ci: fail check-ci when generated skill reference pages are stale The bundled skill reference pages under src/sqlbuild/.agents/skills/sqlbuild/references/docs drifted from the website docs and nothing caught it. `make check-docs-skill`, now part of `make check-ci`, regenerates the pages in memory with the same generator as `make skills` and fails, listing every changed, missing or retired page, with `make skills` as the fix. Refs: CHI-629 --- Makefile | 8 ++- scripts/check_docs_skill.py | 15 +++++ scripts/docs_skill/_helpers/output.py | 20 +++++++ scripts/docs_skill/main/check.py | 41 +++++++++++++ .../unit/scripts/docs_skill/main/__init__.py | 0 .../scripts/docs_skill/main/_test_types.py | 9 +++ .../scripts/docs_skill/main/test_check.py | 59 +++++++++++++++++++ 7 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 scripts/check_docs_skill.py create mode 100644 scripts/docs_skill/main/check.py create mode 100644 tests/unit/scripts/docs_skill/main/__init__.py create mode 100644 tests/unit/scripts/docs_skill/main/_test_types.py create mode 100644 tests/unit/scripts/docs_skill/main/test_check.py diff --git a/Makefile b/Makefile index 610731828c..a849a31c3e 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ SHELL := /bin/bash -.PHONY: verify verify-quick verify-pg coverage cli-preview rust-check check-native-layering check-python-boundary check-release-gate \ +.PHONY: verify verify-quick verify-pg coverage cli-preview rust-check check-native-layering check-python-boundary check-release-gate check-docs-skill \ check-e2e-shards test-e2e-duckdb test-e2e-duckdb-build-core \ test-e2e-duckdb-build-incremental \ test-e2e-duckdb-cli-data test-e2e-duckdb-cli test-e2e-duckdb-inspection \ @@ -625,7 +625,7 @@ verify-quick: exit $$status -check-ci: check-e2e-shards check-native-layering check-python-boundary check-release-gate rust-lint +check-ci: check-e2e-shards check-native-layering check-python-boundary check-release-gate check-docs-skill rust-lint uv run ruff format --check . uv run ruff check . uv run ty check src tests @@ -645,6 +645,10 @@ check-release-gate: uv run python -m scripts.check_release_gate +check-docs-skill: + uv run python -m scripts.check_docs_skill + + check-pr-metadata: @test -n "$(PR_TITLE)" || { echo "PR_TITLE is required" >&2; exit 2; } @test -n "$(PR_BODY_FILE)" || { echo "PR_BODY_FILE is required" >&2; exit 2; } diff --git a/scripts/check_docs_skill.py b/scripts/check_docs_skill.py new file mode 100644 index 0000000000..91d6a8d69a --- /dev/null +++ b/scripts/check_docs_skill.py @@ -0,0 +1,15 @@ +"""Direct wrapper for the docs skill freshness check.""" + +from __future__ import annotations + +from scripts.docs_skill.main.check import check_docs_skill + + +def main(argv: list[str] | None = None) -> int: + """Check that the SQLBuild documentation skill is up to date.""" + + return check_docs_skill(argv) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/docs_skill/_helpers/output.py b/scripts/docs_skill/_helpers/output.py index f036999ef0..4c07d7444b 100644 --- a/scripts/docs_skill/_helpers/output.py +++ b/scripts/docs_skill/_helpers/output.py @@ -25,3 +25,23 @@ def write_reference_pages(*, output_dir: Path, pages: dict[str, str]) -> None: if directory.is_dir() and not any(directory.iterdir()): directory.rmdir() print(f"Wrote {len(pages)} reference pages to {output_dir} ({len(stale_paths)} removed)") + + +def outdated_reference_pages(*, output_dir: Path, pages: dict[str, str]) -> tuple[str, ...]: + """Return generated pages that are missing, differ from `pages`, or are no longer generated.""" + + existing: dict[str, str] = { + path.relative_to(output_dir).as_posix(): path.read_text(encoding="utf-8") + for path in output_dir.rglob("*.md") + } + changed: set[str] = { + relative_path + for relative_path, contents in pages.items() + if existing.get(relative_path) != contents + } + stale: set[str] = { + relative_path + for relative_path, contents in existing.items() + if relative_path not in pages and GENERATED_MARKER in contents + } + return tuple(sorted(changed | stale)) diff --git a/scripts/docs_skill/main/check.py b/scripts/docs_skill/main/check.py new file mode 100644 index 0000000000..448ec6b068 --- /dev/null +++ b/scripts/docs_skill/main/check.py @@ -0,0 +1,41 @@ +"""Focused entry that fails when the bundled docs reference pages are out of date.""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + +from scripts.docs_skill._helpers.output import outdated_reference_pages +from scripts.docs_skill._helpers.render import build_reference_pages +from scripts.docs_skill.constants import DEFAULT_DOCS_ROOT, DEFAULT_OUTPUT_DIR + + +def check_docs_skill(argv: list[str] | None = None) -> int: + """Compare freshly generated reference pages with the committed ones.""" + + args: argparse.Namespace = _parse_args(argv) + outdated: tuple[str, ...] = outdated_reference_pages( + output_dir=args.output_dir, + pages=build_reference_pages(docs_root=args.docs_root), + ) + if not outdated: + print(f"Docs skill reference pages in {args.output_dir} are up to date") + return 0 + print( + f"{len(outdated)} docs skill reference page(s) in {args.output_dir} are out of date " + "with the website docs. Run `make skills` and commit the result:", + file=sys.stderr, + ) + for relative_path in outdated: + print(f" {relative_path}", file=sys.stderr) + return 1 + + +def _parse_args(argv: list[str] | None) -> argparse.Namespace: + parser: argparse.ArgumentParser = argparse.ArgumentParser( + description="Fail when bundled SQLBuild skill reference pages differ from the website docs." + ) + parser.add_argument("--docs-root", type=Path, default=DEFAULT_DOCS_ROOT) + parser.add_argument("--output-dir", type=Path, default=DEFAULT_OUTPUT_DIR) + return parser.parse_args(argv) diff --git a/tests/unit/scripts/docs_skill/main/__init__.py b/tests/unit/scripts/docs_skill/main/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unit/scripts/docs_skill/main/_test_types.py b/tests/unit/scripts/docs_skill/main/_test_types.py new file mode 100644 index 0000000000..81d866fbf7 --- /dev/null +++ b/tests/unit/scripts/docs_skill/main/_test_types.py @@ -0,0 +1,9 @@ +from dataclasses import dataclass + + +@dataclass(frozen=True) +class DocsSkillCheckTestCase: + description: str + output_edits: dict[str, str] + expected_exit_code: int + expected_outdated: tuple[str, ...] diff --git a/tests/unit/scripts/docs_skill/main/test_check.py b/tests/unit/scripts/docs_skill/main/test_check.py new file mode 100644 index 0000000000..e26bff16c7 --- /dev/null +++ b/tests/unit/scripts/docs_skill/main/test_check.py @@ -0,0 +1,59 @@ +from pathlib import Path + +import pytest + +from scripts.docs_skill.constants import GENERATED_MARKER +from scripts.docs_skill.main.check import check_docs_skill +from scripts.docs_skill.main.generate import generate_docs_skill +from tests.unit.scripts.docs_skill.main._test_types import DocsSkillCheckTestCase + +_SIDEBAR_JSON: str = '[{"label":"Concepts","items":["docs/concepts/orders"]}]' +_ORDERS_PAGE: str = '---\ntitle: "Orders"\n---\n\nOrders body.\n' + + +@pytest.mark.parametrize( + "test_case", + [ + DocsSkillCheckTestCase( + description="freshly generated pages pass", + output_edits={}, + expected_exit_code=0, + expected_outdated=(), + ), + DocsSkillCheckTestCase( + description="hand-edited generated page fails", + output_edits={"concepts/orders.md": f"{GENERATED_MARKER}\nold orders body\n"}, + expected_exit_code=1, + expected_outdated=("concepts/orders.md",), + ), + DocsSkillCheckTestCase( + description="generated page with no docs source fails", + output_edits={"concepts/retired.md": f"{GENERATED_MARKER}\nretired\n"}, + expected_exit_code=1, + expected_outdated=("concepts/retired.md",), + ), + ], + ids=lambda case: case.description, +) +def test_given_reference_pages_when_checking_docs_skill_then_outdated_pages_fail_with_fix_command( + test_case: DocsSkillCheckTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + docs_root: Path = tmp_path / "src/content/docs/docs" + output_dir: Path = tmp_path / "references/docs" + (docs_root / "concepts").mkdir(parents=True) + (tmp_path / "src/sidebar.json").write_text(_SIDEBAR_JSON, encoding="utf-8") + (docs_root / "concepts/orders.mdx").write_text(_ORDERS_PAGE, encoding="utf-8") + arguments: list[str] = ["--docs-root", str(docs_root), "--output-dir", str(output_dir)] + _ = generate_docs_skill(arguments) + for relative_path, contents in test_case.output_edits.items(): + (output_dir / relative_path).write_text(contents, encoding="utf-8") + _ = capsys.readouterr() + + result: int = check_docs_skill(arguments) + + errors: str = capsys.readouterr().err + assert result == test_case.expected_exit_code, (test_case.description, errors) + assert all(f" {path}\n" in errors for path in test_case.expected_outdated), errors + assert ("make skills" in errors) is bool(test_case.expected_outdated), errors From f0fa54b728a2523147a023dd25f01a82706dba6e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:45:10 +0100 Subject: [PATCH 13/14] fix(audits): point seed and source audits that read @model at @relation The seeds page attached `expression_is_true` without showing its definition, and the definition people copy from the playground reads `__ref("@model")`. An attached audit receives its owner kind's name argument (`model`, `source` or `seed`) plus `relation`, so on a seed it failed with a bare "missing argument 'model'". Passing the seed as `model` would not help: `__ref` resolves only models. The seeds page now shows a seed-compatible `@relation` definition and explains the owner arguments, the audits page documents them, and the missing-argument error names the argument the owner does receive and points at @relation. A DuckDB E2E builds the documented example. Refs: CHI-632 --- .../src/audits/_helpers/rendering.rs | 26 +++++- .../tests/test_render_attached_audit.rs | 17 ++++ .../references/docs/concepts/audits.md | 3 +- .../references/docs/concepts/seeds.md | 15 +++ .../cli/commands/main/seed/_test_types.py | 11 +++ .../main/seed/test_seed_yaml_audits.py | 93 +++++++++++++++++++ .../src/content/docs/docs/concepts/audits.mdx | 3 +- .../src/content/docs/docs/concepts/seeds.mdx | 15 +++ 8 files changed, 179 insertions(+), 4 deletions(-) create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/seed/test_seed_yaml_audits.py diff --git a/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs b/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs index 5e9481fe81..84c1e1fe2e 100644 --- a/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs +++ b/crates/sqlbuild-attachments/src/audits/_helpers/rendering.rs @@ -2,6 +2,7 @@ use crate::audits::_helpers::parameters::{RenderStop, 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}; /// Severity first, then run scope, as Python resolves them after expanding the SQL. @@ -39,8 +40,10 @@ pub(crate) fn rendered( ) -> Result { render_parameterized_sql(sql, arguments).map_err(|stop| match stop { RenderStop::MissingArgument(name) => format!( - "{} is missing argument '{name}' for generic audit '{}'", - attachment.owner_label, attachment.definition_name + "{} is missing argument '{name}' for generic audit '{}'{}", + attachment.owner_label, + attachment.definition_name, + owner_argument_hint(attachment, &name) ), RenderStop::UnsupportedValue(name) => format!( "{} generic audit '{}' argument '{name}' uses an unsupported value", @@ -49,6 +52,25 @@ pub(crate) fn rendered( }) } +/// Why `@model` is missing on a seed or source: each owner passes its own kind's name argument. +fn owner_argument_hint(attachment: &AuditAttachment, missing: &str) -> String { + let owner_arguments: [&str; 3] = [MODEL_RESOURCE, SOURCE_RESOURCE, SEED_RESOURCE]; + if !owner_arguments.contains(&missing) { + return String::new(); + } + let Some((owner_kind, _)) = attachment + .implicit_arguments + .iter() + .find(|(argument, _)| owner_arguments.contains(&argument.as_str())) + else { + return String::new(); + }; + format!( + "; a {owner_kind} audit receives '{owner_kind}' and 'relation', not '{missing}', so read \ + the attached relation with @relation" + ) +} + /// `merge_audit_arguments`: an explicit argument may repeat an implicit one only as equal text. pub(crate) fn merged_arguments( attachment: &AuditAttachment, 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 9b571785ff..9d9981a31b 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 @@ -88,6 +88,23 @@ fn given_attached_audits_when_rendering_then_python_rendering_is_returned() { 'accepted_values'", ), }, + AttachedAuditTestCase { + description: "a seed audit reading @model is told to read @relation", + attachment: AuditAttachment { + owner_label: "seeds/waffle_types.yml".to_owned(), + sql_body: "SELECT * FROM __ref(\"@model\") WHERE NOT (price_cents >= 0)".to_owned(), + implicit_arguments: vec![ + ("seed".to_owned(), "waffle_types".to_owned()), + ("relation".to_owned(), "__seed(\"waffle_types\")".to_owned()), + ], + ..attachment(Vec::new(), None, None) + }, + expected_rendering: failed( + "seeds/waffle_types.yml is missing argument 'model' for generic audit \ + 'accepted_values'; a seed audit receives 'seed' and 'relation', not 'model', \ + so read the attached relation with @relation", + ), + }, AttachedAuditTestCase { description: "an opaque value inside a referenced list is Python's value error", attachment: attachment( 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 46f21a727d..be6e5ae7f3 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/audits.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/audits.md @@ -230,7 +230,8 @@ Generic audit SQL uses `@name` for parameter placeholders. These are resolved by | Parameter | Description | |-----------|-------------| | `@column` | The column name (auto-populated for column-level audits) | -| `@relation` | The target relation (auto-populated from the attached model or source) | +| `@relation` | The target relation (auto-populated from the attached model, source or seed) | +| `@model`, `@source`, `@seed` | The attached resource's name. Each is set only for its own kind, so an audit that attaches to more than one kind reads `@relation` | | `@'name'` | A quoted parameter passed from the audit declaration (e.g. `@'values'`) | | `@name` | An unquoted parameter passed from the audit declaration (e.g. `@expression`) | diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/seeds.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/seeds.md index 34e3d3f4b1..bdf725e9be 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/seeds.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/seeds.md @@ -102,6 +102,21 @@ seeds: severity: error ``` +`not_null` and `unique` are built in. `expression_is_true` is a project generic audit; one that +works on seeds reads the attached relation through `@relation`: + +```sql +-- audits/generic/expression_is_true.sql +AUDIT (); + +SELECT * +FROM @relation +WHERE NOT (@expression) +``` + +A seed audit receives `@seed` (the seed name) and `@relation`, but not `@model`, so a generic +audit written as `FROM __ref("@model")` works only on models. + During `sqb build`, seed audits run right after the seed loads. An `error` failure blocks every model that depends on the seed. See [Audits](audits.md#seed-audits). diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/seed/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/seed/_test_types.py index 30e69d645b..ac948917f0 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/seed/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/seed/_test_types.py @@ -25,3 +25,14 @@ class SeedJsonOutputTestCase: expected_status: str expected_summary: dict[str, int] expected_assets: tuple[tuple[str, str], ...] + + +@dataclass(frozen=True) +class SeedYamlAuditE2ETestCase: + """Test case for the documented seed YAML audit example.""" + + description: str + audit_sql: str + seed_csv: str + expected_exit_code: int + expected_output_fragments: tuple[str, ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/seed/test_seed_yaml_audits.py b/tests/e2e/src/sqlbuild/cli/commands/main/seed/test_seed_yaml_audits.py new file mode 100644 index 0000000000..9eda44997d --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/seed/test_seed_yaml_audits.py @@ -0,0 +1,93 @@ +"""The seed YAML audit example in the seeds documentation builds as documented.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.seed._test_types import SeedYamlAuditE2ETestCase +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb + +_SEED_YAML: str = """\ +seeds: + - name: waffle_types + description: Waffle types + columns: + - name: waffle_type_id + type: INTEGER + audits: + - not_null + - unique + - name: price_cents + type: INTEGER + audits: + - expression_is_true: + expression: "price_cents >= 0" + severity: error +""" +_RELATION_AUDIT_SQL: str = "AUDIT ();\n\nSELECT *\nFROM @relation\nWHERE NOT (@expression)\n" + + +@pytest.mark.parametrize( + "test_case", + [ + SeedYamlAuditE2ETestCase( + description="documented seed audits pass on valid rows", + audit_sql=_RELATION_AUDIT_SQL, + seed_csv="waffle_type_id,price_cents\n1,450\n2,525\n", + expected_exit_code=0, + expected_output_fragments=( + "expression_is_true (waffle_types)", + "PASS=4", + "FAIL=0", + ), + ), + SeedYamlAuditE2ETestCase( + description="documented seed expression audit fails a negative price", + audit_sql=_RELATION_AUDIT_SQL, + seed_csv="waffle_type_id,price_cents\n1,450\n2,-1\n", + expected_exit_code=1, + expected_output_fragments=("expression_is_true (waffle_types)", "FAIL=1"), + ), + SeedYamlAuditE2ETestCase( + description="model-only audit on a seed points at @relation", + audit_sql='AUDIT ();\n\nSELECT *\nFROM __ref("@model")\nWHERE NOT (@expression)\n', + seed_csv="waffle_type_id,price_cents\n1,450\n", + expected_exit_code=1, + expected_output_fragments=( + "seeds/waffle_types.yml is missing argument 'model' for generic audit " + "'expression_is_true'; a seed audit receives 'seed' and 'relation', not 'model', " + "so read the attached relation with @relation", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_documented_seed_yaml_audits_when_building_then_audits_run_on_the_seed( + test_case: SeedYamlAuditE2ETestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="waffle_seed_audits", + repo_files={ + "sqlbuild_project.toml": ( + 'name = "waffle_seed_audits"\nadapter = "duckdb"\n' + '[connection]\ndatabase = "waffles.duckdb"\n' + ), + "seeds/waffle_types.csv": test_case.seed_csv, + "seeds/waffle_types.yml": _SEED_YAML, + "audits/generic/expression_is_true.sql": test_case.audit_sql, + }, + ) + + built: subprocess.CompletedProcess[str] = run_sqb(command=("build",), project_dir=project_dir) + + output: str = built.stdout + built.stderr + assert built.returncode == test_case.expected_exit_code, (test_case.description, output) + assert all(fragment in output for fragment in test_case.expected_output_fragments), ( + test_case.description, + output, + ) diff --git a/website/src/content/docs/docs/concepts/audits.mdx b/website/src/content/docs/docs/concepts/audits.mdx index 0dc17ff775..aed697b813 100644 --- a/website/src/content/docs/docs/concepts/audits.mdx +++ b/website/src/content/docs/docs/concepts/audits.mdx @@ -211,7 +211,8 @@ Generic audit SQL uses `@name` for parameter placeholders. These are resolved by | Parameter | Description | |-----------|-------------| | `@column` | The column name (auto-populated for column-level audits) | -| `@relation` | The target relation (auto-populated from the attached model or source) | +| `@relation` | The target relation (auto-populated from the attached model, source or seed) | +| `@model`, `@source`, `@seed` | The attached resource's name. Each is set only for its own kind, so an audit that attaches to more than one kind reads `@relation` | | `@'name'` | A quoted parameter passed from the audit declaration (e.g. `@'values'`) | | `@name` | An unquoted parameter passed from the audit declaration (e.g. `@expression`) | diff --git a/website/src/content/docs/docs/concepts/seeds.mdx b/website/src/content/docs/docs/concepts/seeds.mdx index cfbe20b4b9..bfbab4908e 100644 --- a/website/src/content/docs/docs/concepts/seeds.mdx +++ b/website/src/content/docs/docs/concepts/seeds.mdx @@ -90,6 +90,21 @@ seeds: severity: error ``` +`not_null` and `unique` are built in. `expression_is_true` is a project generic audit; one that +works on seeds reads the attached relation through `@relation`: + +```sql +-- audits/generic/expression_is_true.sql +AUDIT (); + +SELECT * +FROM @relation +WHERE NOT (@expression) +``` + +A seed audit receives `@seed` (the seed name) and `@relation`, but not `@model`, so a generic +audit written as `FROM __ref("@model")` works only on models. + During `sqb build`, seed audits run right after the seed loads. An `error` failure blocks every model that depends on the seed. See [Audits](/docs/concepts/audits#seed-audits). From df6014aa40a99cc486c1f2ae068a1e11eea63635 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 19 Oct 2026 05:57:23 +0100 Subject: [PATCH 14/14] test: expect sqb plan to exit 1 for an on_schema_change fail finding CHI-429 makes sqb plan exit 1 when the plan reports an error finding, so the schema backfill case that plans an on_schema_change fail model now expects that exit code. --- .../e2e/src/sqlbuild/cli/commands/main/build/_test_types.py | 1 + .../cli/commands/main/build/test_schema_backfill_behavior.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py index 7b30f393d6..7816b2f698 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/build/_test_types.py @@ -717,6 +717,7 @@ class SchemaBackfillBuildE2ETestCase: schema_after_text: str command: tuple[str, ...] expected_exit_code: int + expected_plan_exit_code: int expected_reason: str expected_backfill_action: str | None = None expected_backfill_duration: str | None = None diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/build/test_schema_backfill_behavior.py b/tests/e2e/src/sqlbuild/cli/commands/main/build/test_schema_backfill_behavior.py index d647093272..5a35f0df47 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/build/test_schema_backfill_behavior.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/build/test_schema_backfill_behavior.py @@ -35,6 +35,7 @@ ), command=("plan", "--json", "--select", "order_status_index"), expected_exit_code=0, + expected_plan_exit_code=0, expected_reason="schema_changed", expected_backfill_action="bounded", expected_backfill_duration="7d", @@ -55,6 +56,7 @@ ), command=("plan", "--json", "--select", "order_status_index"), expected_exit_code=0, + expected_plan_exit_code=1, expected_reason="schema_changed", expected_backfill_action="bounded", expected_backfill_duration="7d", @@ -77,6 +79,7 @@ ), command=("plan", "--json", "--select", "order_status_index"), expected_exit_code=0, + expected_plan_exit_code=0, expected_reason="schema_changed", expected_backfill_action="bounded", expected_backfill_duration="7d", @@ -119,7 +122,7 @@ def test_given_schema_backfill_mutation_when_planning_then_expected_metadata_is_ command=test_case.command, project_dir=project_dir, ) - assert plan_result.returncode == test_case.expected_exit_code, ( + assert plan_result.returncode == test_case.expected_plan_exit_code, ( plan_result.stdout + plan_result.stderr ) payload: dict[str, object] = json.loads(plan_result.stdout)