mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-07 08:02:40 +00:00
fix: hold both halves of the deploy to one dbt:// relation validator
A subscription checked the relation's shape but not its warehouse, so `# on dbt://<unconfigured>/<schema>/<name>` deployed and persisted a trigger row for something no producer can ever write: the write side refuses that exact string, and a dbt project's `profile.warehouse` resolves against the same config, so no later deploy fixes it and the dormant-edge warning cannot report it either. The shape rule and the warehouse rule now live in one `validate_dbt_relation` that both halves call, rather than being spelled per site — the previous two rounds each closed one half of one rule, which is the drift that invites. Also moves the parser test out from between a comment and the test it documents, and names both refusals in the doc's list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
7b3c545ce3
commit
870fc15b68
@@ -1752,17 +1752,6 @@ 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())));
|
||||
@@ -1787,6 +1776,17 @@ mod pipeline_annotation_tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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");
|
||||
}
|
||||
}
|
||||
|
||||
// A relation that overrode its database carries `<database>.<schema>` in
|
||||
// one segment, and each half can be quoted independently. Stripping only
|
||||
// the outer pair leaves a key the manifest ingest never produces, so the
|
||||
|
||||
@@ -85,16 +85,30 @@ 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"));
|
||||
// Both halves are held to the same relation: every producer is a whole
|
||||
// `<warehouse>/<schema>/<name>` under a configured warehouse, so a
|
||||
// subscription to anything else names something nothing can ever write.
|
||||
for (path, ref_, expected) in [
|
||||
(
|
||||
"u/test-user/partial_sub",
|
||||
"dbt://main/analytics",
|
||||
"not a whole warehouse relation",
|
||||
),
|
||||
(
|
||||
"u/test-user/unknown_wh_sub",
|
||||
"dbt://nope/analytics/orders",
|
||||
"does not configure",
|
||||
),
|
||||
] {
|
||||
let resp = deploy(
|
||||
port,
|
||||
path,
|
||||
&format!("// on {ref_}\nexport async function main() {{}}"),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(resp.status(), 400);
|
||||
assert!(resp.text().await?.contains(expected));
|
||||
}
|
||||
|
||||
// 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
|
||||
|
||||
@@ -1081,6 +1081,39 @@ fn lock_hash_entry(path: &str, lock: &str) -> [(String, i64); 1] {
|
||||
[(path.to_string(), hash_script(lock))]
|
||||
}
|
||||
|
||||
/// The `dbt://` relation both halves of a deploy have to agree on: a whole
|
||||
/// `<warehouse>/<schema>/<name>`, under a warehouse this workspace configures.
|
||||
///
|
||||
/// Every producer is held to exactly this — a `// materialize` target here, a
|
||||
/// descriptor's `profile.warehouse` in the worker — so a subscription to anything
|
||||
/// else names a relation nothing can ever write. No later deploy fixes that and
|
||||
/// no dormant-edge warning reports it, since the warning fires on a dbt project's
|
||||
/// ingest and no project can claim a relation under a warehouse that isn't there.
|
||||
/// Asking here rather than at each site is what keeps the two from drifting into
|
||||
/// refusing and accepting the same string.
|
||||
async fn validate_dbt_relation(
|
||||
db: &sqlx::Pool<Postgres>,
|
||||
w_id: &str,
|
||||
relation: &str,
|
||||
what: &str,
|
||||
) -> Result<()> {
|
||||
if !windmill_parser::asset_parser::is_full_relation_path(relation) {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"{what} `dbt://{relation}` is not a whole warehouse relation \
|
||||
(`dbt://<warehouse>/<schema>/<name>`), so nothing can produce it."
|
||||
)));
|
||||
}
|
||||
let warehouse = relation.split('/').next().unwrap_or_default();
|
||||
windmill_common::workspaces::dbt_warehouse_exists(db, w_id, warehouse)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
Error::BadRequest(format!(
|
||||
"{what} `dbt://{relation}` names a warehouse this workspace does not \
|
||||
configure: {e}"
|
||||
))
|
||||
})
|
||||
}
|
||||
|
||||
async fn create_script_internal<'c>(
|
||||
mut ns: NewScript,
|
||||
w_id: String,
|
||||
@@ -1638,26 +1671,8 @@ async fn create_script_internal<'c>(
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
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, warehouse)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
Error::BadRequest(format!(
|
||||
"`// materialize dbt://{warehouse}/…` names a warehouse this \
|
||||
workspace does not configure: {e}"
|
||||
))
|
||||
})?;
|
||||
validate_dbt_relation(&db, &w_id, &m.target_path, "`// materialize` target")
|
||||
.await?;
|
||||
}
|
||||
_ => {
|
||||
return Err(Error::BadRequest(
|
||||
@@ -2467,15 +2482,7 @@ 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."
|
||||
)));
|
||||
}
|
||||
validate_dbt_relation(&db, &w_id, relation, "subscription target").await?;
|
||||
// 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) {
|
||||
|
||||
+5
-2
@@ -714,8 +714,11 @@ fan-out reads the deploy-time `asset` rows) but records no row, so the relation
|
||||
shows no last writer. Fixing it is one change for all of those annotations, not
|
||||
this one.
|
||||
|
||||
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
|
||||
A `# on dbt://<relation>` subscription is held to the same relation a producer
|
||||
is — a whole `<warehouse>/<schema>/<name>` under a configured warehouse, checked
|
||||
by the validator the `// materialize` target goes through, since two spellings of
|
||||
that rule would refuse and accept the same string. Beyond that it is refused 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 neither subscribe nor declare a
|
||||
|
||||
Reference in New Issue
Block a user