mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-11 08:07:15 +00:00
fix: refuse a dbt:// subscription that is not a whole relation
`# on dbt://main/analytics` deployed and persisted a trigger row. Every producer spells `<warehouse>/<schema>/<name>` — the manifest ingest derives it from `relation_name`, a `// materialize` target is checked against it — so a partial one is an edge nothing can ever wake, which is what the dbt-only refusal exists to prevent. The shape now has one definition (`is_full_relation_path`) that both halves of the deploy ask, rather than a segment count spelled twice: a subscription and a write that disagreed would refuse and accept the same string. Also rewrites the canvas test's comment as a current constraint per AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
dff574d3d9
commit
7b3c545ce3
@@ -806,6 +806,18 @@ pub fn canonicalize_table_asset_path(path: &str) -> String {
|
||||
)
|
||||
}
|
||||
|
||||
/// Whether a `dbt://` path names a whole relation, `<warehouse>/<schema>/<name>`.
|
||||
///
|
||||
/// Every producer spells one that way — the manifest ingest derives it from
|
||||
/// `relation_name`, a `// materialize` target is checked against it — so anything
|
||||
/// else can be produced by nothing and read by nothing. Both sides of the deploy
|
||||
/// ask here rather than counting segments themselves: a subscription and a write
|
||||
/// that disagreed on the shape would refuse and accept the same string.
|
||||
pub fn is_full_relation_path(path: &str) -> bool {
|
||||
let mut segments = path.split('/');
|
||||
segments.clone().count() == 3 && !segments.any(str::is_empty)
|
||||
}
|
||||
|
||||
/// A doubled delimiter inside a quoted identifier is that delimiter, literally —
|
||||
/// the same rule the worker's `split_relation` applies to `relation_name`. Both
|
||||
/// have to decode it or one spelling of a table becomes two graph nodes: the dbt
|
||||
@@ -1740,6 +1752,17 @@ mod pipeline_annotation_tests {
|
||||
// annotation is hand-written, and the warehouses fold case in opposite
|
||||
// directions. A regression here is invisible: both nodes still render, they
|
||||
// just stop being the same node and the cross-boundary cascade never fires.
|
||||
/// The shape both halves of the deploy check against: a subscription and a
|
||||
/// write that disagreed on it would refuse and accept the same string.
|
||||
#[test]
|
||||
fn a_whole_relation_is_three_non_empty_segments() {
|
||||
assert!(is_full_relation_path("main/analytics/orders"));
|
||||
assert!(is_full_relation_path("main/archive.sales/orders"));
|
||||
for partial in ["main", "main/analytics", "main/analytics/orders/x", "", "main//orders"] {
|
||||
assert!(!is_full_relation_path(partial), "{partial} is not a relation");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn table_paths_from_every_spelling_canonicalize_to_one_key() {
|
||||
let canonical = Some((AssetKind::Dbt, Cow::Owned("main/analytics/orders".into())));
|
||||
|
||||
@@ -85,6 +85,17 @@ async fn test_dbt_materialize_target_deploy_contract(db: Pool<Postgres>) -> anyh
|
||||
.await?
|
||||
.contains("`// data_test` is not supported"));
|
||||
|
||||
// Every producer spells a whole relation, so a partial one is an edge nothing
|
||||
// can ever wake.
|
||||
let resp = deploy(
|
||||
port,
|
||||
"u/test-user/partial_sub",
|
||||
"// on dbt://main/analytics\nexport async function main() {}",
|
||||
)
|
||||
.await;
|
||||
assert_eq!(resp.status(), 400);
|
||||
assert!(resp.text().await?.contains("not a whole warehouse relation"));
|
||||
|
||||
// Any language may declare the write — the DuckLake write engine is DuckDB's,
|
||||
// this declaration is not — and the target is canonicalized on the way into
|
||||
// `asset`, so a hand-written mixed-case spelling lands on the model's key.
|
||||
|
||||
@@ -1638,25 +1638,24 @@ async fn create_script_internal<'c>(
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
let segments = m.target_path.split('/').collect::<Vec<_>>();
|
||||
if segments.len() != 3 || segments.iter().any(|s| s.is_empty()) {
|
||||
if !windmill_parser::asset_parser::is_full_relation_path(&m.target_path) {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"`// materialize` needs a full warehouse relation in the target: \
|
||||
`dbt://<warehouse>/<schema>/<name>` (got `dbt://{}`).",
|
||||
m.target_path
|
||||
)));
|
||||
}
|
||||
let warehouse = m.target_path.split('/').next().unwrap_or_default();
|
||||
// The warehouse segment IS the identity a dbt model reading this
|
||||
// relation keys on, so a name no warehouse answers to is not a
|
||||
// namespace — it strands this write on a node nothing reaches.
|
||||
// Same resolution a dbt descriptor's `profile.warehouse` gets.
|
||||
windmill_common::workspaces::dbt_warehouse_exists(&db, &w_id, segments[0])
|
||||
windmill_common::workspaces::dbt_warehouse_exists(&db, &w_id, warehouse)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
Error::BadRequest(format!(
|
||||
"`// materialize dbt://{}/…` names a warehouse this workspace does \
|
||||
not configure: {e}",
|
||||
segments[0]
|
||||
"`// materialize dbt://{warehouse}/…` names a warehouse this \
|
||||
workspace does not configure: {e}"
|
||||
))
|
||||
})?;
|
||||
}
|
||||
@@ -2468,6 +2467,15 @@ async fn create_script_internal<'c>(
|
||||
and a project is run on its schedule, not woken by an asset cascade."
|
||||
)));
|
||||
}
|
||||
// Every producer spells a whole relation, so a partial one is an edge
|
||||
// nothing can ever wake — refused on the same terms as the dbt-only
|
||||
// case rather than persisted.
|
||||
if !windmill_parser::asset_parser::is_full_relation_path(relation) {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"`{trigger_ref}` is not a whole warehouse relation \
|
||||
(`dbt://<warehouse>/<schema>/<name>`), so nothing can produce it."
|
||||
)));
|
||||
}
|
||||
// Both paths under a rename: the old one's committed write row is
|
||||
// still there and this transaction is about to remove it.
|
||||
let deploying_paths = match p_path_opt.as_deref().filter(|old| *old != ns.path) {
|
||||
|
||||
@@ -319,11 +319,9 @@ describe('resolveGraph', () => {
|
||||
expect(assetTrigKeys(r, 'f/x/open')).toEqual(['ducklake:main.orders'])
|
||||
})
|
||||
|
||||
// A `dbt://` subscription used to be suppressed here because every one of them
|
||||
// was refused at deploy. A relation a native `// materialize manual dbt://…`
|
||||
// script writes now wakes its subscribers, so hiding the edge would leave the
|
||||
// author's own annotation off the canvas; the deploy is what refuses the case
|
||||
// that still cannot fire (a relation dbt alone builds).
|
||||
// A relation a native `// materialize manual dbt://…` script writes wakes its
|
||||
// subscribers, so hiding the edge would leave the author's own annotation off
|
||||
// the canvas. The deploy refuses the ones that cannot fire.
|
||||
it('draws an explicit dbt:// subscription as an unsaved trigger overlay', () => {
|
||||
const liveAnnotations = {
|
||||
scriptPath: 'f/x/open',
|
||||
|
||||
Reference in New Issue
Block a user