mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-05 16:03:47 +00:00
fix: review round 1 — dbt-script materialize, set-form rule, doc
- Refuse `// materialize` on a dbt script, the producer half of the rule the trigger loop already applies to `// on`: the graph ingest republishes that path's asset rows wholesale, so a declared write is wiped by the deploy that accepted it while its runs keep stamping the relation. - `dormant_dbt_subscriptions` now spells the same predicate its singular sibling does: the producer set has to be non-empty (nothing produces it yet is deploy order, not a dormant edge) and excludes the subscriber's own path (a script never wakes itself). Both divergences are pinned by tests. - The docs no longer claim the dbt deploy log covers a native producer that drops its `// materialize`; it does not, and nothing else reports that case. - An integration test over the deploy contract, since only a real deploy proves the handler feeds `sole_dbt_producer` the canonical key `asset.path` holds — the spelling that has to agree across the materialize target, the `// on` ref and the refusal that joins them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
73fc580766
commit
addcb4c525
-23
@@ -1,23 +0,0 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT DISTINCT st.trigger_ref || ' → ' || st.runnable_path AS \"edge!\"\n FROM script_trigger st\n WHERE st.workspace_id = $1 AND st.trigger_kind = 'asset'\n AND st.trigger_ref = ANY($2)\n AND NOT EXISTS (\n SELECT 1 FROM asset a\n JOIN script s ON s.workspace_id = a.workspace_id AND s.path = a.usage_path\n AND s.archived = false AND s.deleted = false\n AND s.language <> 'dbt'\n WHERE a.workspace_id = st.workspace_id AND a.kind = 'dbt'\n AND 'dbt://' || a.path = st.trigger_ref\n AND a.usage_kind = 'script'\n AND a.usage_access_type IN ('w', 'rw'))\n ORDER BY 1",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "edge!",
|
||||
"type_info": "Text"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"TextArray"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "00e2aeac5766f2748ac2438befb21fcb3fc769dc368195c3a2de58f6719e5851"
|
||||
}
|
||||
+12
@@ -0,0 +1,12 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "UPDATE workspace_settings\n SET dbt_warehouses = '{\"main\": {\"resource_path\": \"u/test-user/wh\"}}'::jsonb\n WHERE workspace_id = 'test-workspace'",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": []
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "1d8effff5dd1e4a177efed7366e84411c125cbfada861992e3d6032de635bad6"
|
||||
}
|
||||
+12
@@ -0,0 +1,12 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "INSERT INTO asset (workspace_id, path, kind, usage_access_type, usage_path, usage_kind)\n VALUES ('test-workspace', 'main/analytics/marts', 'dbt', 'w', 'u/test-user/project',\n 'script')",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": []
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "1f7608defb5748db687145750520cb1059f63c1ea51db3471c213018b2983704"
|
||||
}
|
||||
+20
@@ -0,0 +1,20 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT path FROM asset WHERE workspace_id = 'test-workspace' AND kind = 'dbt' AND usage_path = 'u/test-user/ingest' AND usage_access_type = 'w'",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "path",
|
||||
"type_info": "Varchar"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": []
|
||||
},
|
||||
"nullable": [
|
||||
false
|
||||
]
|
||||
},
|
||||
"hash": "33aa15957f56281947963cd696f7f518433741305d891d99059184eaf39fa2db"
|
||||
}
|
||||
+23
@@ -0,0 +1,23 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "WITH producer AS (\n SELECT 'dbt://' || a.path AS trigger_ref, a.usage_path, s.language\n FROM asset a\n JOIN script s ON s.workspace_id = a.workspace_id AND s.path = a.usage_path\n AND s.archived = false AND s.deleted = false\n WHERE a.workspace_id = $1 AND a.kind = 'dbt'\n AND a.usage_kind = 'script' AND a.usage_access_type IN ('w', 'rw')\n AND 'dbt://' || a.path = ANY($2)\n )\n SELECT DISTINCT st.trigger_ref || ' → ' || st.runnable_path AS \"edge!\"\n FROM script_trigger st\n WHERE st.workspace_id = $1 AND st.trigger_kind = 'asset'\n AND st.trigger_ref = ANY($2)\n AND EXISTS (SELECT 1 FROM producer p\n WHERE p.trigger_ref = st.trigger_ref\n AND p.usage_path <> st.runnable_path\n AND p.language = 'dbt')\n AND NOT EXISTS (SELECT 1 FROM producer p\n WHERE p.trigger_ref = st.trigger_ref\n AND p.usage_path <> st.runnable_path\n AND p.language <> 'dbt')\n ORDER BY 1",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "edge!",
|
||||
"type_info": "Text"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"TextArray"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "49d905cae6ba42a3df62bad9385e613381cb4dcf7e851197150a3771ab43c99e"
|
||||
}
|
||||
+12
@@ -0,0 +1,12 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "INSERT INTO script (workspace_id, hash, path, summary, description, content, created_by,\n language)\n VALUES ('test-workspace', 1, 'u/test-user/project', '', '', '', 'test-user', 'dbt')",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": []
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "6890e2be43ff8c653ce62f8cde1b9877190091923a2615d7d94bdf89a12e43c7"
|
||||
}
|
||||
+20
@@ -0,0 +1,20 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT trigger_ref FROM script_trigger WHERE workspace_id = 'test-workspace' AND runnable_path = 'u/test-user/consumer' AND trigger_kind = 'asset'",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "trigger_ref",
|
||||
"type_info": "Text"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": []
|
||||
},
|
||||
"nullable": [
|
||||
false
|
||||
]
|
||||
},
|
||||
"hash": "ae4c0e8aeeef90d08a9b9c919b881b0a03e44beea80da732a4ad7166cf4c06cf"
|
||||
}
|
||||
@@ -0,0 +1,135 @@
|
||||
use serde_json::json;
|
||||
use sqlx::{Pool, Postgres};
|
||||
|
||||
use windmill_test_utils::*;
|
||||
|
||||
fn client() -> reqwest::Client {
|
||||
reqwest::Client::new()
|
||||
}
|
||||
|
||||
fn authed(builder: reqwest::RequestBuilder) -> reqwest::RequestBuilder {
|
||||
builder.header("Authorization", "Bearer SECRET_TOKEN")
|
||||
}
|
||||
|
||||
async fn deploy(port: u16, path: &str, content: &str) -> reqwest::Response {
|
||||
authed(client().post(format!(
|
||||
"http://localhost:{port}/api/w/test-workspace/scripts/create"
|
||||
)))
|
||||
.json(&json!({
|
||||
"path": path,
|
||||
"summary": "",
|
||||
"description": "",
|
||||
"content": content,
|
||||
"language": "deno",
|
||||
"schema": { "type": "object", "properties": {}, "required": [] }
|
||||
}))
|
||||
.send()
|
||||
.await
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
/// A `dbt://` relation is one graph node only while every side spells it the same
|
||||
/// way, and three sides derive that spelling independently: the `// materialize`
|
||||
/// target becomes an `asset.path`, a `// on` ref becomes a `script_trigger`, and
|
||||
/// the deploy-time refusal joins the two. The unit tests on `sole_dbt_producer`
|
||||
/// prove the predicate; only a deploy proves the handler feeds it the key the
|
||||
/// table actually holds — so a canonicalization that drifted on one side would
|
||||
/// pass those and split the node here.
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
|
||||
async fn test_dbt_materialize_target_deploy_contract(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
sqlx::query!(
|
||||
r#"UPDATE workspace_settings
|
||||
SET dbt_warehouses = '{"main": {"resource_path": "u/test-user/wh"}}'::jsonb
|
||||
WHERE workspace_id = 'test-workspace'"#
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
// Nothing generates warehouse DDL, so a managed target is refused rather than
|
||||
// degraded into the track-only mode it would silently become.
|
||||
let resp = deploy(
|
||||
port,
|
||||
"u/test-user/managed",
|
||||
"// materialize dbt://main/analytics/orders\nexport async function main() {}",
|
||||
)
|
||||
.await;
|
||||
assert_eq!(resp.status(), 400);
|
||||
assert!(resp.text().await?.contains("must be `manual`"));
|
||||
|
||||
// The warehouse segment is the identity a dbt model keys on; a name the
|
||||
// workspace does not configure strands the write on an unreachable node.
|
||||
let resp = deploy(
|
||||
port,
|
||||
"u/test-user/unknown_wh",
|
||||
"// materialize manual dbt://nope/analytics/orders\nexport async function main() {}",
|
||||
)
|
||||
.await;
|
||||
assert_eq!(resp.status(), 400);
|
||||
assert!(resp.text().await?.contains("does not configure"));
|
||||
|
||||
// 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.
|
||||
let resp = deploy(
|
||||
port,
|
||||
"u/test-user/ingest",
|
||||
"// materialize manual dbt://main/ANALYTICS/Orders\nexport async function main() {}",
|
||||
)
|
||||
.await;
|
||||
assert_eq!(resp.status(), 201);
|
||||
let write = sqlx::query_scalar!(
|
||||
"SELECT path FROM asset WHERE workspace_id = 'test-workspace' AND kind = 'dbt' \
|
||||
AND usage_path = 'u/test-user/ingest' AND usage_access_type = 'w'"
|
||||
)
|
||||
.fetch_one(&db)
|
||||
.await?;
|
||||
assert_eq!(write, "main/analytics/orders");
|
||||
|
||||
// That producer is native, so subscribing to what it writes is accepted — and
|
||||
// the `// on` ref has to canonicalize identically, or the row it stores names
|
||||
// a relation nothing produces.
|
||||
let resp = deploy(
|
||||
port,
|
||||
"u/test-user/consumer",
|
||||
"// on dbt://main/\"Analytics\"/\"Orders\"\nexport async function main() {}",
|
||||
)
|
||||
.await;
|
||||
assert_eq!(resp.status(), 201);
|
||||
let trigger_ref = sqlx::query_scalar!(
|
||||
"SELECT trigger_ref FROM script_trigger WHERE workspace_id = 'test-workspace' \
|
||||
AND runnable_path = 'u/test-user/consumer' AND trigger_kind = 'asset'"
|
||||
)
|
||||
.fetch_one(&db)
|
||||
.await?;
|
||||
assert_eq!(trigger_ref, "dbt://main/analytics/orders");
|
||||
|
||||
// With dbt as the only producer the same subscription can never be woken — a
|
||||
// dbt run does not dispatch — so the deploy refuses it and names the project.
|
||||
sqlx::query!(
|
||||
"INSERT INTO script (workspace_id, hash, path, summary, description, content, created_by,
|
||||
language)
|
||||
VALUES ('test-workspace', 1, 'u/test-user/project', '', '', '', 'test-user', 'dbt')"
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
sqlx::query!(
|
||||
"INSERT INTO asset (workspace_id, path, kind, usage_access_type, usage_path, usage_kind)
|
||||
VALUES ('test-workspace', 'main/analytics/marts', 'dbt', 'w', 'u/test-user/project',
|
||||
'script')"
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
let resp = deploy(
|
||||
port,
|
||||
"u/test-user/mart_consumer",
|
||||
"// on dbt://main/analytics/MARTS\nexport async function main() {}",
|
||||
)
|
||||
.await;
|
||||
assert_eq!(resp.status(), 400);
|
||||
assert!(resp.text().await?.contains("u/test-user/project"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -1582,6 +1582,18 @@ async fn create_script_internal<'c>(
|
||||
// a `manual` script owns its DDL and skips them.
|
||||
if let Some(m) = pipeline_annotations.materialize.as_ref() {
|
||||
use windmill_parser::asset_parser::AssetKind as PAssetKind;
|
||||
// The producer half of the rule the trigger loop below applies to `// on`:
|
||||
// a dbt project's writes come from its manifest, and the graph ingest
|
||||
// republishes this path's asset rows wholesale, so a declared one would be
|
||||
// wiped by the very deploy that accepted it while its runs kept stamping
|
||||
// the relation.
|
||||
if ns.language == ScriptLang::Dbt {
|
||||
return Err(Error::BadRequest(
|
||||
"a dbt script cannot declare `// materialize`: what a project builds is read \
|
||||
from its manifest and published by the graph ingest, not annotated."
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
match m.target_kind {
|
||||
PAssetKind::Ducklake => {
|
||||
if ns.language != ScriptLang::DuckDb {
|
||||
@@ -2441,8 +2453,7 @@ async fn create_script_internal<'c>(
|
||||
)));
|
||||
}
|
||||
if let Some(dbt_owner) =
|
||||
windmill_common::assets::sole_dbt_producer(&db, &w_id, relation, &ns.path)
|
||||
.await?
|
||||
windmill_common::assets::sole_dbt_producer(&db, &w_id, relation, &ns.path).await?
|
||||
{
|
||||
return Err(Error::BadRequest(format!(
|
||||
"`{trigger_ref}` cannot be subscribed to: it is built by the dbt project at \
|
||||
|
||||
@@ -298,15 +298,18 @@ pub async fn sole_dbt_producer<'e>(
|
||||
}
|
||||
|
||||
/// The set form of [`sole_dbt_producer`], for asking about many relations at
|
||||
/// once: every `// on dbt://<relation>` edge among `relations` that no non-dbt
|
||||
/// script writes, rendered as `dbt://<relation> → <subscriber path>`.
|
||||
/// once: every `// on dbt://<relation>` edge among `relations` whose producers
|
||||
/// are all dbt scripts, rendered as `dbt://<relation> → <subscriber path>`.
|
||||
///
|
||||
/// A dbt deploy asks this about the relations it just ingested, because that
|
||||
/// ingest is what can retroactively leave a subscription accepted earlier — when
|
||||
/// nothing produced the relation — with dbt as its only producer. Kept beside its
|
||||
/// singular sibling and phrased against the same predicate: two spellings of "is
|
||||
/// dbt the sole producer" that drift apart would silence the warning without
|
||||
/// anything failing.
|
||||
/// nothing produced the relation — with dbt as its only producer.
|
||||
///
|
||||
/// Spells the predicate the same way its singular sibling does, per subscriber:
|
||||
/// the producer set excludes the subscriber's own path (a script never wakes
|
||||
/// itself) and has to be non-empty (nothing produces it yet is deploy order, not
|
||||
/// a dormant edge). Two "is dbt the sole producer" rules that drifted apart would
|
||||
/// silence this warning with nothing failing.
|
||||
///
|
||||
/// Same disclosure and executor contract as [`sole_dbt_producer`]: workspace
|
||||
/// pool, caller already scoped to `workspace_id`.
|
||||
@@ -323,19 +326,27 @@ pub async fn dormant_dbt_subscriptions<'e>(
|
||||
.map(|r| format!("dbt://{r}"))
|
||||
.collect::<Vec<_>>();
|
||||
Ok(sqlx::query_scalar!(
|
||||
r#"SELECT DISTINCT st.trigger_ref || ' → ' || st.runnable_path AS "edge!"
|
||||
r#"WITH producer AS (
|
||||
SELECT 'dbt://' || a.path AS trigger_ref, a.usage_path, s.language
|
||||
FROM asset a
|
||||
JOIN script s ON s.workspace_id = a.workspace_id AND s.path = a.usage_path
|
||||
AND s.archived = false AND s.deleted = false
|
||||
WHERE a.workspace_id = $1 AND a.kind = 'dbt'
|
||||
AND a.usage_kind = 'script' AND a.usage_access_type IN ('w', 'rw')
|
||||
AND 'dbt://' || a.path = ANY($2)
|
||||
)
|
||||
SELECT DISTINCT st.trigger_ref || ' → ' || st.runnable_path AS "edge!"
|
||||
FROM script_trigger st
|
||||
WHERE st.workspace_id = $1 AND st.trigger_kind = 'asset'
|
||||
AND st.trigger_ref = ANY($2)
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM asset a
|
||||
JOIN script s ON s.workspace_id = a.workspace_id AND s.path = a.usage_path
|
||||
AND s.archived = false AND s.deleted = false
|
||||
AND s.language <> 'dbt'
|
||||
WHERE a.workspace_id = st.workspace_id AND a.kind = 'dbt'
|
||||
AND 'dbt://' || a.path = st.trigger_ref
|
||||
AND a.usage_kind = 'script'
|
||||
AND a.usage_access_type IN ('w', 'rw'))
|
||||
AND EXISTS (SELECT 1 FROM producer p
|
||||
WHERE p.trigger_ref = st.trigger_ref
|
||||
AND p.usage_path <> st.runnable_path
|
||||
AND p.language = 'dbt')
|
||||
AND NOT EXISTS (SELECT 1 FROM producer p
|
||||
WHERE p.trigger_ref = st.trigger_ref
|
||||
AND p.usage_path <> st.runnable_path
|
||||
AND p.language <> 'dbt')
|
||||
ORDER BY 1"#,
|
||||
workspace_id,
|
||||
&refs
|
||||
|
||||
@@ -147,3 +147,30 @@ async fn the_set_form_agrees_with_the_singular_one(db: Pool<Postgres>) {
|
||||
"a native producer wakes it, so the edge is live"
|
||||
);
|
||||
}
|
||||
|
||||
/// The two ways the set form could stop meaning what the singular one means: a
|
||||
/// relation nothing produces is deploy order rather than a dormant edge, and a
|
||||
/// subscriber's own write is not a producer that can wake it — the dispatcher
|
||||
/// skips self-loops, so that edge is dormant and has to be named.
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
|
||||
async fn the_set_form_matches_on_the_edge_cases_too(db: Pool<Postgres>) {
|
||||
let relations = vec![RELATION.to_string()];
|
||||
plant_subscriber(&db, SUBSCRIBER).await;
|
||||
assert!(
|
||||
dormant_dbt_subscriptions(&db, WS, &relations)
|
||||
.await
|
||||
.unwrap()
|
||||
.is_empty(),
|
||||
"nothing produces it yet, so nothing is dormant"
|
||||
);
|
||||
|
||||
plant_producer(&db, "u/test-user/project", "dbt", 1).await;
|
||||
plant_producer(&db, SUBSCRIBER, "postgresql", 2).await;
|
||||
assert_eq!(
|
||||
dormant_dbt_subscriptions(&db, WS, &relations)
|
||||
.await
|
||||
.unwrap(),
|
||||
vec![format!("dbt://{RELATION} → {SUBSCRIBER}")],
|
||||
"the subscriber's own write cannot wake it, so dbt is still the sole producer"
|
||||
);
|
||||
}
|
||||
|
||||
+13
-8
@@ -711,9 +711,11 @@ A `# on dbt://<relation>` subscription is therefore refused at deploy in exactly
|
||||
one shape: when every script that writes that relation is a dbt one. Nothing
|
||||
produces it yet is NOT that shape — a subscriber may be deployed before its
|
||||
producer, as for every other asset kind, and refusing there would break
|
||||
deploy-order-independent syncs. A dbt script may not subscribe at all: its graph
|
||||
ingest clears its own `dbt://` trigger rows, so accepting one would deploy an edge
|
||||
the dependency job then silently removes.
|
||||
deploy-order-independent syncs. A dbt script may neither subscribe nor declare a
|
||||
`// materialize`: its graph ingest republishes that path's trigger and asset rows
|
||||
wholesale, so either annotation would deploy something the dependency job then
|
||||
silently removes — while the declared write would still stamp the relation on
|
||||
every run.
|
||||
|
||||
The producer set is read as it stands committed, minus the deploying script's own
|
||||
rows — those describe the version being replaced, so a script dropping its
|
||||
@@ -721,11 +723,14 @@ rows — those describe the version being replaced, so a script dropping its
|
||||
producer that wakes it, which it could not be anyway (the dispatcher skips
|
||||
self-loops).
|
||||
|
||||
What that leaves is a subscription accepted while it was live and later orphaned:
|
||||
a dbt project deployed afterwards that claims the relation, or a native producer
|
||||
that stops writing it. A dbt deploy names those edges in its own log rather than
|
||||
leaving them silently dormant — the same "an edge that can never fire is worse
|
||||
than saying so" the refusal is for, at the only other point where it is knowable.
|
||||
What that leaves is a subscription accepted while it was live and later orphaned.
|
||||
A dbt project deployed afterwards that claims the relation names those edges in
|
||||
its own log rather than leaving them silently dormant — the same "an edge that can
|
||||
never fire is worse than saying so" the refusal is for, at the other point where
|
||||
it is knowable. The remaining case, a native producer that drops its
|
||||
`// materialize` and leaves dbt alone on the relation, is reported nowhere: the
|
||||
deploy that causes it does not touch the subscriber, and the canvas is where it
|
||||
shows.
|
||||
|
||||
A plain READ still renders the consumer beside the model, which is what makes
|
||||
the lineage one graph — but it is written in the script's own code, not in a
|
||||
|
||||
Reference in New Issue
Block a user