From 63cc0181ded257f2f73369bd1d72dacc1022de37 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 16 Sep 2026 08:41:26 +0200 Subject: [PATCH] fix: a reused destination retires the routes pointing at it, and draft_base stays out of diffs Co-Authored-By: Claude Opus 5 (1M context) --- ...8fa7d0b650b5cfd80c099f785f5e411062e5.json} | 5 +- ...d46af4419dda9410ffc46f19bc38e56efe609.json | 18 ++++++ backend/tests/drafts_save_follows_move.rs | 55 +++++++++++++++++++ backend/windmill-api-flows/src/flows.rs | 1 + backend/windmill-api-scripts/src/scripts.rs | 1 + backend/windmill-api/src/apps.rs | 1 + backend/windmill-common/src/user_drafts.rs | 27 ++++++++- frontend/src/lib/utils.ts | 1 + frontend/src/lib/utils_draft_deploy.ts | 4 +- 9 files changed, 107 insertions(+), 6 deletions(-) rename backend/.sqlx/{query-b6af374e5043a207eae6bce80d3eccda82aec0ae80ccdf2445570e369e0d815a.json => query-9723edfd6de38cf0bf21123a46d88fa7d0b650b5cfd80c099f785f5e411062e5.json} (56%) create mode 100644 backend/.sqlx/query-9d1b202844e6935e636d1570a92d46af4419dda9410ffc46f19bc38e56efe609.json diff --git a/backend/.sqlx/query-b6af374e5043a207eae6bce80d3eccda82aec0ae80ccdf2445570e369e0d815a.json b/backend/.sqlx/query-9723edfd6de38cf0bf21123a46d88fa7d0b650b5cfd80c099f785f5e411062e5.json similarity index 56% rename from backend/.sqlx/query-b6af374e5043a207eae6bce80d3eccda82aec0ae80ccdf2445570e369e0d815a.json rename to backend/.sqlx/query-9723edfd6de38cf0bf21123a46d88fa7d0b650b5cfd80c099f785f5e411062e5.json index fd930f89c4..4704ddd84e 100644 --- a/backend/.sqlx/query-b6af374e5043a207eae6bce80d3eccda82aec0ae80ccdf2445570e369e0d815a.json +++ b/backend/.sqlx/query-9723edfd6de38cf0bf21123a46d88fa7d0b650b5cfd80c099f785f5e411062e5.json @@ -1,16 +1,17 @@ { "db_name": "PostgreSQL", - "query": "DELETE FROM draft_move WHERE workspace_id = $1 AND typ::text = ANY($2::text[]) AND old_path = $3", + "query": "DELETE FROM draft_move WHERE workspace_id = $1 AND typ::text = ANY($2::text[])\n AND (old_path = $3 OR (new_path = $3 AND $4::text IS NULL))", "describe": { "columns": [], "parameters": { "Left": [ "Text", "TextArray", + "Text", "Text" ] }, "nullable": [] }, - "hash": "b6af374e5043a207eae6bce80d3eccda82aec0ae80ccdf2445570e369e0d815a" + "hash": "9723edfd6de38cf0bf21123a46d88fa7d0b650b5cfd80c099f785f5e411062e5" } diff --git a/backend/.sqlx/query-9d1b202844e6935e636d1570a92d46af4419dda9410ffc46f19bc38e56efe609.json b/backend/.sqlx/query-9d1b202844e6935e636d1570a92d46af4419dda9410ffc46f19bc38e56efe609.json new file mode 100644 index 0000000000..f882ce2986 --- /dev/null +++ b/backend/.sqlx/query-9d1b202844e6935e636d1570a92d46af4419dda9410ffc46f19bc38e56efe609.json @@ -0,0 +1,18 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM draft_move\n WHERE workspace_id = $1 AND typ::text = ANY($2::text[])\n AND new_path = $4 AND old_path <> $3 AND ($5::text IS NULL OR email = $5)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "TextArray", + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "9d1b202844e6935e636d1570a92d46af4419dda9410ffc46f19bc38e56efe609" +} diff --git a/backend/tests/drafts_save_follows_move.rs b/backend/tests/drafts_save_follows_move.rs index c87ad9adf7..afbec56c75 100644 --- a/backend/tests/drafts_save_follows_move.rs +++ b/backend/tests/drafts_save_follows_move.rs @@ -530,3 +530,58 @@ async fn test_a_moved_draft_deploys_at_its_new_path(db: Pool) -> anyho assert_eq!(status, 201, "the moved draft could not be deployed: {body}"); Ok(()) } + +/// A route is only as good as the item it points at: when an unrelated item claims the +/// destination, a save still addressed to the old path must stay where it is rather than +/// land on that item's draft. +#[sqlx::test(fixtures("base", "drafts_save_follows_move"))] +async fn test_a_reused_destination_ends_the_route(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + sqlx::query("UPDATE script SET archived = true WHERE path = 'u/test-user/follow_a'") + .execute(&db) + .await?; + let resp = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/move/script/u/test-user/follow_a" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ "new_path": "u/test-user/follow_b" })) + .send() + .await?; + assert!( + resp.status().is_success(), + "move failed: {}", + resp.text().await? + ); + // Someone else's item takes the destination, and the moved draft goes with the + // deploy that consumes it. + sqlx::query("DELETE FROM draft WHERE workspace_id = 'test-workspace' AND path = 'u/test-user/follow_b'") + .execute(&db) + .await?; + let resp = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/scripts/create" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ + "path": "u/test-user/follow_b", + "summary": "unrelated", + "description": "", + "content": "export function main() { return 3 }", + "language": "deno", + "schema": {} + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "create failed: {}", resp.text().await?); + + assert_eq!( + save_at(port, "u/test-user/follow_a", "after the destination was reused").await?, + "u/test-user/follow_a", + "a save was routed onto the item that now owns the destination" + ); + Ok(()) +} diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index 92d291344b..9a154ee2d1 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -719,6 +719,7 @@ async fn create_flow( &w_id, &[UserDraftItemKind::Flow], &nf.path, + None, ) .await?; diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 1eebfc1389..0400cbc91d 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -2426,6 +2426,7 @@ async fn create_script_internal<'c>( &w_id, &[UserDraftItemKind::Script], &ns.path, + p_path_opt.as_deref(), ) .await?; if p_hashes.is_some() && !p_hashes.unwrap().is_empty() { diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index bc22837f63..fabf9fd0cd 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -2611,6 +2611,7 @@ async fn create_app_internal<'a>( &w_id, &[UserDraftItemKind::App, UserDraftItemKind::RawApp], &app.path, + None, ) .await?; let id = sqlx::query_scalar!( diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index 99ec3cce5e..ebd0ce363f 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -790,6 +790,21 @@ pub async fn record_draft_move( ) .execute(&mut **tx) .await?; + // Routes that ended at the destination before this move describe drafts that were + // carried there for an item this one is replacing: left alive, a save addressed to + // the start of that chain would land on this move's draft instead. + sqlx::query!( + "DELETE FROM draft_move + WHERE workspace_id = $1 AND typ::text = ANY($2::text[]) + AND new_path = $4 AND old_path <> $3 AND ($5::text IS NULL OR email = $5)", + w_id, + &typs as &[&str], + old_path, + new_path, + email, + ) + .execute(&mut **tx) + .await?; sqlx::query!( "UPDATE draft_move SET new_path = $4 WHERE workspace_id = $1 AND typ::text = ANY($2::text[]) @@ -842,8 +857,11 @@ pub async fn record_draft_move( Ok(()) } -/// Drop the move records leaving `path`: an item was just created there, and saves -/// addressed to it are its own. +/// Drop the move records at `path`: an item was just created there, so saves addressed to +/// it are its own (records leaving `path`). A deploy that is not a rename also drops the +/// records arriving, which point at an item that no longer owns the path; `keep_from`, +/// the path a rename came from, suppresses that, since the chain ending here is the one +/// this very deploy just wrote. /// /// **The caller must have authorized the deploy that created the item first.** /// Dropping a record sends later draft writes at `path` back to `path`. @@ -852,13 +870,16 @@ pub async fn clear_draft_moves_from( w_id: &str, kinds: &[UserDraftItemKind], path: &str, + keep_from: Option<&str>, ) -> Result<()> { let typs = kinds.iter().map(|k| k.as_str()).collect::>(); sqlx::query!( - "DELETE FROM draft_move WHERE workspace_id = $1 AND typ::text = ANY($2::text[]) AND old_path = $3", + "DELETE FROM draft_move WHERE workspace_id = $1 AND typ::text = ANY($2::text[]) + AND (old_path = $3 OR (new_path = $3 AND $4::text IS NULL))", w_id, &typs as &[&str], path, + keep_from, ) .execute(&mut **tx) .await?; diff --git a/frontend/src/lib/utils.ts b/frontend/src/lib/utils.ts index 635fc6f14e..d6a0dd0bdc 100644 --- a/frontend/src/lib/utils.ts +++ b/frontend/src/lib/utils.ts @@ -1442,6 +1442,7 @@ const CLEANED_VALUE_KEYS = new Set([ 'draft_only', 'draft_saved_at', 'draft_created_at', + 'draft_base', 'is_draft', 'other_drafts_users', 'created_at', diff --git a/frontend/src/lib/utils_draft_deploy.ts b/frontend/src/lib/utils_draft_deploy.ts index eae28b2466..31b973f9a5 100644 --- a/frontend/src/lib/utils_draft_deploy.ts +++ b/frontend/src/lib/utils_draft_deploy.ts @@ -262,7 +262,7 @@ export async function getDraftDiffValues( // draft-table row (e.g. a flow created via createFlow(draft_only: true), like // `u/admin/new`). There `draft` is null, so the draft side must fall back to // the row's own value — otherwise the diff "after" is empty and nothing shows. - // Strip overlay metadata (is_draft / draft_saved_at / no_deployed / + // Strip overlay metadata (is_draft / draft_saved_at / draft_base / no_deployed / // other_drafts_users) from the deployed side so the diff doesn't show the // per-user markers as noise. if (kind === 'script') { @@ -271,6 +271,7 @@ export async function getDraftDiffValues( draft, is_draft: _i, draft_saved_at: _c, + draft_base: _b, no_deployed, other_drafts_users: _o, hash: _h, @@ -289,6 +290,7 @@ export async function getDraftDiffValues( draft, is_draft: _i, draft_saved_at: _c, + draft_base: _b, no_deployed, other_drafts_users: _o, version_id: _v,