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