From 4f4ecba9ccad483bdc5d2c4ffb838a65031c4f32 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 10 Sep 2026 08:01:25 +0200 Subject: [PATCH] fix(datatables): clone a fork's pointer instead of failing after the copy Forking a fork with cloning left an orphan database. The preflight resolves the pointer and sees the governing entry, so both endpoints ran and filled the new database; `apply_forked_datatable` then refused the inherited pointer and rolled the fork back, stranding a registered `wm_fork_*` that no entry names and whose name blocks the retry. Refusing earlier would have been the smaller change, but forking a fork and cloning worked before pointers existed, so it would trade an orphan for a regression. Resolve what the pointer names and write the terminal entry the clone needs: the whole `database` object rather than a patch of its `resource_path`, since a pointer has none, and `reference` removed with it. Also accepts `-- role=x` and `-- Role = x`, two more spellings that fell through to the default role. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb --- ...b9d1f440692f183c7c378f59e4b73f1c6e241.json | 17 ++++++ .../windmill-api-workspaces/src/workspaces.rs | 53 ++++++++++++------- backend/windmill-common/src/worker.rs | 18 ++++--- 3 files changed, 63 insertions(+), 25 deletions(-) create mode 100644 backend/.sqlx/query-ebaf3ed3097621da59dd201b5a4b9d1f440692f183c7c378f59e4b73f1c6e241.json diff --git a/backend/.sqlx/query-ebaf3ed3097621da59dd201b5a4b9d1f440692f183c7c378f59e4b73f1c6e241.json b/backend/.sqlx/query-ebaf3ed3097621da59dd201b5a4b9d1f440692f183c7c378f59e4b73f1c6e241.json new file mode 100644 index 0000000000..e5de67e73f --- /dev/null +++ b/backend/.sqlx/query-ebaf3ed3097621da59dd201b5a4b9d1f440692f183c7c378f59e4b73f1c6e241.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(\n jsonb_set(\n datatable #- ARRAY['datatables', $2, 'reference'],\n ARRAY['datatables', $2, 'database'], $3::jsonb),\n ARRAY['datatables', $2, 'forked_from'], $4::jsonb\n )\n WHERE workspace_id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Jsonb", + "Jsonb" + ] + }, + "nullable": [] + }, + "hash": "ebaf3ed3097621da59dd201b5a4b9d1f440692f183c7c378f59e4b73f1c6e241" +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 296922fbd7..596376fe0e 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -45,12 +45,12 @@ use windmill_common::workspaces::GitRepositorySettings; #[cfg(feature = "enterprise")] use windmill_common::workspaces::WorkspaceDeploymentUISettings; use windmill_common::workspaces::{ - check_deploy_rules, check_user_against_rule, get_datatable_resource_from_db, - datatable_ref_name, get_datatable_resource_from_db_unchecked, resolve_governing_datatable, - validate_dev_workspace_id, validate_fork_workspace_id, validate_workspace_name, DataTable, - DataTableCatalogResourceType, DataTableForkBehavior, DatatableAccess, ProtectionRuleKind, - ProtectionRules, ProtectionRuleset, RuleCheckResult, WorkspaceGitSyncSettings, - DEV_WORKSPACE_LOCK_RULE_NAME, + check_deploy_rules, check_user_against_rule, datatable_ref_name, + get_datatable_resource_from_db, get_datatable_resource_from_db_unchecked, + resolve_governing_datatable, validate_dev_workspace_id, validate_fork_workspace_id, + validate_workspace_name, DataTable, DataTableCatalogResourceType, DataTableForkBehavior, + DatatableAccess, ProtectionRuleKind, ProtectionRules, ProtectionRuleset, RuleCheckResult, + WorkspaceGitSyncSettings, DEV_WORKSPACE_LOCK_RULE_NAME, }; use windmill_common::workspaces::{Ducklake, DucklakeCatalogResourceType}; use windmill_common::PgDatabase; @@ -7915,29 +7915,44 @@ async fn apply_forked_datatable( let dt: DataTable = serde_json::from_value(config_val) .map_err(|e| Error::internal_err(format!("Failed to parse datatable config: {}", e)))?; - // A cloned data table owns its copy, so the fork's entry must be terminal. It arrived that way - // from the settings clone; a pointer here would mean the parent's own entry was one, and the - // clone has to name its new database rather than follow anything. - let database = dt.database.as_ref().ok_or_else(|| { - Error::BadRequest(format!( - "Data table '{}' points at another workspace's data table and cannot be cloned; \ - fork it from the workspace that owns it.", - fdt.name - )) - })?; + // A clone owns its copy, so the fork's entry has to be terminal. When the parent was itself a + // fork the settings clone hands down a pointer instead, and the database this clone just + // created is already filled — so resolve what it points at and name the copy, rather than + // refusing after the fact. Pointers are only ever written for instance databases + // (`point_kept_datatables_at_parent`), so the resolved entry is one. + let database = match dt.database.clone() { + Some(database) => database, + None => resolve_governing_datatable(db, parent_w_id, &fdt.name) + .await? + .datatable + .database + .ok_or_else(|| { + Error::internal_err(format!( + "Data table '{}' resolves to an entry that owns no database", + fdt.name + )) + })?, + }; if database.resource_type == DataTableCatalogResourceType::Instance { - // Instance: update resource_path to the new dbname + // The whole `database` object, not just its `resource_path`: a pointer entry has none to + // patch. `reference` goes with it — exactly one of the two may be set. + let new_database = serde_json::json!({ + "resource_type": "instance", + "resource_path": &fdt.new_dbname, + }); sqlx::query!( r#"UPDATE workspace_settings SET datatable = jsonb_set( - jsonb_set(datatable, ARRAY['datatables', $2, 'database', 'resource_path'], to_jsonb($3::text)), + jsonb_set( + datatable #- ARRAY['datatables', $2, 'reference'], + ARRAY['datatables', $2, 'database'], $3::jsonb), ARRAY['datatables', $2, 'forked_from'], $4::jsonb ) WHERE workspace_id = $1"#, forked_w_id, &fdt.name, - &fdt.new_dbname, + new_database, forked_from, ) .execute(&mut **tx) diff --git a/backend/windmill-common/src/worker.rs b/backend/windmill-common/src/worker.rs index 6b3c63e3c0..c742a3efcb 100644 --- a/backend/windmill-common/src/worker.rs +++ b/backend/windmill-common/src/worker.rs @@ -1105,9 +1105,10 @@ impl SqlAnnotations { if !line.starts_with("--") { break; } - // `role`, `Role`, `role:` and `role:name` all open an attempt; `rolexyz` does not. - // The colon is worth accepting rather than skipping past: `-- role: x` is the likelier - // spelling, and skipping it is exactly the silent fallback this refuses. + // The keyword may be followed by whitespace, `:` or `=` — `role x`, `role: x`, + // `role=x`, `Role = x` all open an attempt, while `rolexyz` does not. Each accepted + // separator is one spelling that would otherwise take the `continue` below and run the + // query as the data table's default role, which is the silence this exists to remove. let body = line[2..].trim_start(); let Some(after) = body .get(..4) @@ -1116,15 +1117,18 @@ impl SqlAnnotations { else { continue; }; - let colon = after.starts_with(':'); - let after = after.strip_prefix(':').unwrap_or(after); - if !after.is_empty() && !colon && !after.starts_with(char::is_whitespace) { + if !after.is_empty() + && !after.starts_with(char::is_whitespace) + && !after.starts_with([':', '=']) + { continue; } // Past this point the line is an attempt to name a role, so a malformed one is an // error rather than a miss. Falling through would run the query as the data table's // default role — quietly, and under a login the author did not choose. + let after = after.trim_start(); + let after = after.strip_prefix([':', '=']).unwrap_or(after); let mut tokens = after.split_whitespace(); let role = tokens .next() @@ -2750,6 +2754,8 @@ mod tests { "-- role operator;\nSELECT 1", "-- role: operator\nSELECT 1", "-- role:operator\nSELECT 1", + "-- role=operator\nSELECT 1", + "-- Role = operator\nSELECT 1", ] { assert_eq!( role(accepted).unwrap(),