From 7b3c545ce32f3bff3a28e3e71a3f07647174cb55 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 5 Sep 2026 04:34:19 +0200 Subject: [PATCH] fix: refuse a `dbt://` subscription that is not a whole relation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `# on dbt://main/analytics` deployed and persisted a trigger row. Every producer spells `//` — 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) --- .../windmill-parser/src/asset_parser.rs | 23 +++++++++++++++++++ .../tests/dbt_materialize_target.rs | 11 +++++++++ backend/windmill-api-scripts/src/scripts.rs | 20 +++++++++++----- .../assets/AssetGraph/resolveGraph.test.ts | 8 +++---- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/backend/parsers/windmill-parser/src/asset_parser.rs b/backend/parsers/windmill-parser/src/asset_parser.rs index cebc31484d..da3ef9b700 100644 --- a/backend/parsers/windmill-parser/src/asset_parser.rs +++ b/backend/parsers/windmill-parser/src/asset_parser.rs @@ -806,6 +806,18 @@ pub fn canonicalize_table_asset_path(path: &str) -> String { ) } +/// Whether a `dbt://` path names a whole relation, `//`. +/// +/// 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()))); diff --git a/backend/windmill-api-integration-tests/tests/dbt_materialize_target.rs b/backend/windmill-api-integration-tests/tests/dbt_materialize_target.rs index d93a9562c4..ecc9add82b 100644 --- a/backend/windmill-api-integration-tests/tests/dbt_materialize_target.rs +++ b/backend/windmill-api-integration-tests/tests/dbt_materialize_target.rs @@ -85,6 +85,17 @@ async fn test_dbt_materialize_target_deploy_contract(db: Pool) -> 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. diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index fd4f986779..6c1a516fe0 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -1638,25 +1638,24 @@ async fn create_script_internal<'c>( .to_string(), )); } - let segments = m.target_path.split('/').collect::>(); - 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:////` (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:////`), 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) { diff --git a/frontend/src/lib/components/assets/AssetGraph/resolveGraph.test.ts b/frontend/src/lib/components/assets/AssetGraph/resolveGraph.test.ts index 02879399a9..c57687d78b 100644 --- a/frontend/src/lib/components/assets/AssetGraph/resolveGraph.test.ts +++ b/frontend/src/lib/components/assets/AssetGraph/resolveGraph.test.ts @@ -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',