Skip to content

Commit 620bca0

Browse files
authored
fix(test): rewrite references in helper CTEs and order them after their dependencies (#1019)
* fix(test): rewrite references in helper CTEs and order them after their dependencies Helper CTEs in SQL tests now resolve __ref(), __source(), __seed() and the other reference calls exactly like __assert__ CTEs, are emitted after the mocks and models they read, and count their unmocked __ref() targets when selecting the test. Helper references compile cannot resolve (unknown models, cycles, and mocks or fixtures reading a referencing helper) fail at compile with P013. * fix(test): limit helper CTE references to helpers the test reads Resolve, check and count references only in helpers reachable from the test's expected rows and assertions, so unread helpers and mocks behave as before. Inlined helpers now resolve in expected rows too, relations resolved helpers read win over a step's own copy so assertions and helpers share one mock CTE, nested CTEs shadowing a helper keep the existing redefinition error, and the mock check only covers mocks and fixtures the test uses.
1 parent dc8cdcd commit 620bca0

18 files changed

Lines changed: 1794 additions & 85 deletions

File tree

‎crates/sqlbuild-analysis/src/compiler/_helpers/sql_tests/helper_scope.rs‎

Lines changed: 134 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use polyglot_sql::Dialect;
66

77
use crate::compiler::_helpers::sql_tests::cte_rename::defined_cte_keys;
88
use crate::compiler::_helpers::sql_tests::cte_slices::SliceDialect;
9-
use crate::compiler::_helpers::sql_tests::cte_sql::with_leading_ctes;
9+
use crate::compiler::_helpers::sql_tests::cte_sql::{with_leading_ctes, with_unique_ctes};
1010
use crate::compiler::_helpers::sql_tests::helper_names::{
1111
helper_cte_name, rename_helper_references,
1212
};
@@ -24,6 +24,8 @@ use crate::constants::{
2424
pub(crate) struct HelperScope {
2525
pub(crate) ctes: Vec<(String, String)>,
2626
pub(crate) reached_mocks: HashSet<String>,
27+
/// Mock and model CTEs resolved helpers read, which defer to the reader's own copy.
28+
pub(crate) generated: HashSet<String>,
2729
}
2830

2931
/// One mock CTE other test CTEs can read by its generated name.
@@ -41,6 +43,16 @@ struct ScopeHelper {
4143
cte_name: String,
4244
sql: String,
4345
tokens: Vec<String>,
46+
resolved: Option<ResolvedHelper>,
47+
}
48+
49+
/// A helper whose relation references were resolved like an assertion's, after the model chain.
50+
pub(crate) struct ResolvedHelper {
51+
/// The helper body with every reference replaced by the CTE that stands in for it.
52+
pub(crate) sql: String,
53+
/// The mock and model CTEs the resolved body reads, dependencies first.
54+
pub(crate) lifted_ctes: Vec<(String, String)>,
55+
pub(crate) reached_mocks: HashSet<String>,
4456
}
4557

4658
/// One test CTE another test CTE reads: a mock, or a helper when `mock_name` is `None`.
@@ -161,6 +173,7 @@ impl ScopeGraph {
161173
cte_name: cte.name.clone(),
162174
sql: cte.sql_body.clone(),
163175
tokens: identifier_tokens(&cte.sql_body, patterns),
176+
resolved: None,
164177
});
165178
}
166179
let readers = test_readers(fixtures);
@@ -281,6 +294,76 @@ impl ScopeGraph {
281294
order.ordered
282295
}
283296

297+
/// Helpers the test's expected rows and assertions read, except those a mock it uses reads.
298+
pub(crate) fn read_helpers(
299+
&self,
300+
readers: &[&str],
301+
used_mocks: &HashSet<String>,
302+
patterns: &SqlTestPatterns,
303+
) -> HashSet<usize> {
304+
let helpers = |nodes: Vec<ScopeNode>| -> HashSet<usize> {
305+
nodes
306+
.into_iter()
307+
.filter_map(|node| match node {
308+
ScopeNode::Helper(index) => Some(index),
309+
ScopeNode::Mock(_) => None,
310+
})
311+
.collect()
312+
};
313+
let mut read: HashSet<usize> = HashSet::new();
314+
for sql in readers {
315+
read.extend(helpers(
316+
self.closure(&identifier_tokens(sql, patterns), true),
317+
));
318+
}
319+
for mock in &self.mocks {
320+
if used_mocks.contains(&mock.mock_name) {
321+
for index in helpers(self.closure(&mock.tokens, true)) {
322+
read.remove(&index);
323+
}
324+
}
325+
}
326+
read
327+
}
328+
329+
/// Read helpers whose SQL, as placed in the test query, calls a reference such as `__ref()`.
330+
pub(crate) fn referencing_helpers(
331+
&self,
332+
read: &HashSet<usize>,
333+
patterns: &SqlTestPatterns,
334+
) -> Vec<(usize, String)> {
335+
self.helpers
336+
.iter()
337+
.enumerate()
338+
.filter(|(index, helper)| helper.resolved.is_none() && read.contains(index))
339+
.filter_map(|(index, _)| {
340+
let sql = self.placed_helper_sql(index);
341+
(patterns.test_reference.is_match(sql) || patterns.udf.is_match(sql))
342+
.then(|| (index, sql.to_string()))
343+
})
344+
.collect()
345+
}
346+
347+
/// Replace a helper's references with the CTEs a reader must define ahead of it.
348+
pub(crate) fn resolve_helper(&mut self, index: usize, resolved: ResolvedHelper) {
349+
let placement = self.placement;
350+
let Some(helper) = self.helpers.get_mut(index) else {
351+
return;
352+
};
353+
match placement {
354+
HelperPlacement::TopLevel => helper.sql.clone_from(&resolved.sql),
355+
HelperPlacement::Inline => helper.original.clone_from(&resolved.sql),
356+
}
357+
helper.resolved = Some(resolved);
358+
}
359+
360+
fn placed_helper_sql(&self, index: usize) -> &str {
361+
match self.placement {
362+
HelperPlacement::TopLevel => &self.helpers[index].sql,
363+
HelperPlacement::Inline => &self.helpers[index].original,
364+
}
365+
}
366+
284367
/// Whether a CTE name is a test helper placed at the top level of the test query.
285368
pub(crate) fn is_top_level_helper(&self, name: &str) -> bool {
286369
self.placement == HelperPlacement::TopLevel
@@ -343,18 +426,39 @@ impl ScopeGraph {
343426
let mut scope = HelperScope {
344427
ctes: Vec::new(),
345428
reached_mocks: HashSet::new(),
429+
generated: HashSet::new(),
346430
};
347431
for node in nodes {
348432
if let ScopeNode::Mock(index) = node {
349433
scope
350434
.reached_mocks
351435
.insert(self.mocks[index].mock_name.clone());
352436
}
437+
if let ScopeNode::Helper(index) = node
438+
&& let Some(resolved) = &self.helpers[index].resolved
439+
{
440+
scope
441+
.reached_mocks
442+
.extend(resolved.reached_mocks.iter().cloned());
443+
scope.generated.extend(
444+
resolved
445+
.lifted_ctes
446+
.iter()
447+
.map(|(name, _)| name.to_ascii_lowercase()),
448+
);
449+
scope.ctes = with_unique_ctes(
450+
std::mem::take(&mut scope.ctes),
451+
resolved.lifted_ctes.iter().cloned(),
452+
);
453+
}
353454
if let Some(dependency) = self.dependency(node, None) {
354-
scope.ctes.push((
355-
dependency.generated_name.to_string(),
356-
dependency.sql.to_string(),
357-
));
455+
scope.ctes = with_unique_ctes(
456+
std::mem::take(&mut scope.ctes),
457+
[(
458+
dependency.generated_name.to_string(),
459+
dependency.sql.to_string(),
460+
)],
461+
);
358462
}
359463
}
360464
scope
@@ -428,6 +532,7 @@ pub(crate) fn helper_scope_ctes(
428532
return Ok(HelperScope {
429533
ctes: Vec::new(),
430534
reached_mocks: HashSet::new(),
535+
generated: HashSet::new(),
431536
});
432537
}
433538
let nodes = graph.closure(&identifier_tokens(sql, patterns), true);
@@ -444,14 +549,31 @@ pub(crate) fn helper_scope_ctes(
444549
Ok(graph.collect_scope(nodes))
445550
}
446551

447-
/// Append scoped CTEs after a step's generated CTEs, keeping one definition per name.
552+
/// Merge scoped CTEs into a step's CTEs; relations resolved helpers read come first and win.
448553
pub(crate) fn merged_scoped_ctes(
449-
mut lifted: Vec<(String, String)>,
450-
scoped: Vec<(String, String)>,
554+
lifted: Vec<(String, String)>,
555+
scoped: HelperScope,
451556
file_label: &str,
452557
) -> Result<Vec<(String, String)>, String> {
453-
for (name, sql) in scoped {
454-
match lifted
558+
type Ctes = Vec<(String, String)>;
559+
let (generated, authored): (Ctes, Ctes) = scoped
560+
.ctes
561+
.into_iter()
562+
.partition(|(name, _)| scoped.generated.contains(&name.to_ascii_lowercase()));
563+
let mut merged: Vec<(String, String)> = lifted;
564+
if !generated.is_empty() {
565+
let own = std::mem::replace(&mut merged, generated);
566+
for (name, sql) in own {
567+
if !merged
568+
.iter()
569+
.any(|(existing, _)| existing.eq_ignore_ascii_case(&name))
570+
{
571+
merged.push((name, sql));
572+
}
573+
}
574+
}
575+
for (name, sql) in authored {
576+
match merged
455577
.iter()
456578
.find(|(existing, _)| existing.eq_ignore_ascii_case(&name))
457579
{
@@ -461,10 +583,10 @@ pub(crate) fn merged_scoped_ctes(
461583
"SQL test '{file_label}' defines CTE '{name}', which conflicts with the generated CTE"
462584
)));
463585
}
464-
None => lifted.push((name, sql)),
586+
None => merged.push((name, sql)),
465587
}
466588
}
467-
Ok(lifted)
589+
Ok(merged)
468590
}
469591

470592
/// Names the comparison renderer generates, which a scoped helper CTE must not shadow.

0 commit comments

Comments
 (0)