From 793e4dba6b3a8e563a9ba0ec7fd9d76812103579 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Fri, 11 Sep 2026 11:42:44 +0200 Subject: [PATCH] feat: refuse a rename onto a path that already holds a draft A draft occupies its path the way a deployed item does: a never-deployed item, or a draft left on an archived script. Renaming onto it would either merge two items or leave the losing row stranded at a path its item has left. The move now refuses with a BadRequest inside the deploy's transaction, so the rename itself fails and the source stays deployed. Every draft on the item then moves; there is no longer a left-behind count to report. Co-Authored-By: Claude Fable 5.1 --- ...8e76030efff8805bf2fb53061e4cbc8eed4a7.json | 17 +++++ ...c55f4c17a91c279f2b736e98ea9a0ac1ecaf9.json | 25 ------- backend/tests/drafts_move_taken.rs | 75 +++++++++++++++++++ backend/tests/fixtures/drafts_move_taken.sql | 17 +++++ backend/windmill-api-flows/src/flows.rs | 10 +-- backend/windmill-api-scripts/src/scripts.rs | 10 +-- backend/windmill-api/src/apps.rs | 9 +-- backend/windmill-common/src/user_drafts.rs | 72 +++++++----------- 8 files changed, 139 insertions(+), 96 deletions(-) create mode 100644 backend/.sqlx/query-af0400dcf733b00629c24118b218e76030efff8805bf2fb53061e4cbc8eed4a7.json delete mode 100644 backend/.sqlx/query-d336ad22e1f7ddc06fd9a49ad3ac55f4c17a91c279f2b736e98ea9a0ac1ecaf9.json create mode 100644 backend/tests/drafts_move_taken.rs create mode 100644 backend/tests/fixtures/drafts_move_taken.sql diff --git a/backend/.sqlx/query-af0400dcf733b00629c24118b218e76030efff8805bf2fb53061e4cbc8eed4a7.json b/backend/.sqlx/query-af0400dcf733b00629c24118b218e76030efff8805bf2fb53061e4cbc8eed4a7.json new file mode 100644 index 0000000000..2a991142f6 --- /dev/null +++ b/backend/.sqlx/query-af0400dcf733b00629c24118b218e76030efff8805bf2fb53061e4cbc8eed4a7.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft\n SET path = $3\n WHERE workspace_id = $1\n AND path = $2\n AND typ::text = ANY($4::text[])", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Varchar", + "TextArray" + ] + }, + "nullable": [] + }, + "hash": "af0400dcf733b00629c24118b218e76030efff8805bf2fb53061e4cbc8eed4a7" +} diff --git a/backend/.sqlx/query-d336ad22e1f7ddc06fd9a49ad3ac55f4c17a91c279f2b736e98ea9a0ac1ecaf9.json b/backend/.sqlx/query-d336ad22e1f7ddc06fd9a49ad3ac55f4c17a91c279f2b736e98ea9a0ac1ecaf9.json deleted file mode 100644 index ab4bc45fa8..0000000000 --- a/backend/.sqlx/query-d336ad22e1f7ddc06fd9a49ad3ac55f4c17a91c279f2b736e98ea9a0ac1ecaf9.json +++ /dev/null @@ -1,25 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE draft AS d\n SET path = $3\n WHERE d.workspace_id = $1\n AND d.path = $2\n AND d.typ::text = ANY($4::text[])\n AND NOT EXISTS (\n SELECT 1 FROM draft o\n WHERE o.workspace_id = d.workspace_id\n AND o.path = $3\n AND o.typ = d.typ\n AND o.email IS NOT DISTINCT FROM d.email\n )\n RETURNING d.id", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "id", - "type_info": "Int8" - } - ], - "parameters": { - "Left": [ - "Text", - "Text", - "Text", - "TextArray" - ] - }, - "nullable": [ - false - ] - }, - "hash": "d336ad22e1f7ddc06fd9a49ad3ac55f4c17a91c279f2b736e98ea9a0ac1ecaf9" -} diff --git a/backend/tests/drafts_move_taken.rs b/backend/tests/drafts_move_taken.rs new file mode 100644 index 0000000000..df29d9f9e7 --- /dev/null +++ b/backend/tests/drafts_move_taken.rs @@ -0,0 +1,75 @@ +//! A rename onto a path that already holds a draft is refused. +//! +//! Nothing deployed can sit at a rename's destination (the deploy conflicts on +//! that), but a draft can: a never-deployed item, or a draft left on an archived +//! script. Moving onto it would merge two items or strand a row, so the rename +//! itself fails, in its own transaction, and the source stays deployed. The +//! destination draft here is the deployer's own, which is the same collision. + +use serde_json::{json, Value}; +use sqlx::{Pool, Postgres}; + +use windmill_test_utils::*; + +/// Hex form of script hash 7010, the way the API takes a parent hash. +const HEAD_HASH: &str = "0000000000001b62"; + +#[sqlx::test(fixtures("base", "drafts_move_taken"))] +async fn test_rename_onto_a_draft_is_refused(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let client = reqwest::Client::new(); + + let resp = client + .post(format!( + "http://localhost:{port}/api/w/test-workspace/scripts/create" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ + "path": "u/test-user/mvtaken_b", + "parent_hash": HEAD_HASH, + "summary": "A", + "description": "", + "content": "export function main() { return 1 }", + "language": "deno", + "schema": {} + })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 400, "rename onto a draft was not refused: {body}"); + assert!( + body.contains("already has a draft"), + "unexpected refusal: {body}" + ); + + // The whole deploy rolled back: the source is still the live head, and the + // draft at the destination is untouched. + let head: Value = client + .get(format!( + "http://localhost:{port}/api/w/test-workspace/scripts/get/p/u/test-user/mvtaken_a" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .send() + .await? + .json() + .await?; + assert_eq!(head["hash"], HEAD_HASH, "source was replaced: {head}"); + assert_eq!(head["archived"], false, "source was archived: {head}"); + + let draft: Value = client + .get(format!("http://localhost:{port}/api/w/test-workspace/drafts/get_own/script/u/test-user/mvtaken_b")) + .header("Authorization", "Bearer SECRET_TOKEN") + .send() + .await? + .json() + .await?; + assert_eq!( + draft["value"]["summary"], "B", + "destination draft changed: {draft}" + ); + + Ok(()) +} diff --git a/backend/tests/fixtures/drafts_move_taken.sql b/backend/tests/fixtures/drafts_move_taken.sql new file mode 100644 index 0000000000..f278036e56 --- /dev/null +++ b/backend/tests/fixtures/drafts_move_taken.sql @@ -0,0 +1,17 @@ +-- Fixture for refusing a rename onto a path a draft already occupies. +-- +-- A deployed script at `u/test-user/mvtaken_a` (hash 7010 = 0x1b62), and a +-- never-deployed draft of test-user's own at `u/test-user/mvtaken_b`, the path +-- the rename will target. Nothing deployed lives at the target, so only the +-- draft can refuse the move. + +INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, + schema, summary, description, lock, extra_perms) +VALUES ('test-workspace', 7010, 'u/test-user/mvtaken_a', + 'export function main() { return 1 }', + 'deno', 'script', 'test-user', '{}', 'A', '', '', '{}'); + +INSERT INTO draft (workspace_id, path, typ, value, email) +VALUES ('test-workspace', 'u/test-user/mvtaken_b', 'script', + '{"path": "u/test-user/mvtaken_b", "summary": "B", "content": ""}', + 'test@windmill.dev'); diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index e43dea86d5..4170c46392 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -1390,7 +1390,7 @@ async fn update_flow( // Everything left at the old path is a draft this deploy didn't consume // — teammates' rows, and the deployer's own when the caller asked us to // keep it. Carry them rather than strand them. - let outcome = windmill_common::user_drafts::move_drafts_for_path( + windmill_common::user_drafts::move_drafts_for_path( &mut tx, &w_id, &[UserDraftItemKind::Flow], @@ -1398,14 +1398,6 @@ async fn update_flow( &nf.path, ) .await?; - if outcome.left_behind > 0 { - tracing::warn!( - "{} of {} flow draft(s) stranded at {flow_path}: their owner already has a draft at {}", - outcome.left_behind, - outcome.moved + outcome.left_behind, - &nf.path - ); - } } audit_log( diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 57af5e8d31..c2e9a6f4e7 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -2138,7 +2138,7 @@ async fn create_script_internal<'c>( // Everything left at the old path is a draft this deploy didn't // consume — teammates' rows, and the deployer's own when the caller // asked us to keep it. Carry them rather than strand them. - let outcome = windmill_common::user_drafts::move_drafts_for_path( + windmill_common::user_drafts::move_drafts_for_path( &mut tx, &w_id, &[UserDraftItemKind::Script], @@ -2146,14 +2146,6 @@ async fn create_script_internal<'c>( &ns.path, ) .await?; - if outcome.left_behind > 0 { - tracing::warn!( - "{} of {} script draft(s) stranded at {p_path}: their owner already has a draft at {}", - outcome.left_behind, - outcome.moved + outcome.left_behind, - &ns.path - ); - } } sqlx::query!( diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 7b646d9ded..bf6cbcbc14 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -3463,7 +3463,7 @@ async fn update_app_internal<'a>( // Everything left at the old path is a draft this deploy didn't consume // — teammates' rows, and the deployer's own when the caller asked us to // keep it. Carry them rather than strand them. - let outcome = windmill_common::user_drafts::move_drafts_for_path( + windmill_common::user_drafts::move_drafts_for_path( &mut tx, &w_id, &[UserDraftItemKind::App, UserDraftItemKind::RawApp], @@ -3471,13 +3471,6 @@ async fn update_app_internal<'a>( &npath, ) .await?; - if outcome.left_behind > 0 { - tracing::warn!( - "{} of {} app draft(s) stranded at {path}: their owner already has a draft at {npath}", - outcome.left_behind, - outcome.moved + outcome.left_behind - ); - } } audit_log( &mut *tx, diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index bfab248d23..3c24f85b4a 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -599,71 +599,53 @@ pub async fn delete_own_draft_for_path( /// and guessing wrong silently either strands the user's staged rename or /// clears a staleness warning they needed. /// -/// A row whose owner already has a draft at `new_path` stays put: the target -/// draft is work in its own right and is never overwritten. Those rows are -/// reported as `left_behind` rather than swallowed — they are exactly the -/// orphans this function exists to prevent, so a caller that ignores the count -/// is choosing to strand them silently. +/// A draft already at `new_path` (a never-deployed item, or one left on an +/// archived script there) occupies that path the way a deployed item does, and +/// the move is refused with `BadRequest` — inside the deploy's transaction, so +/// the rename itself is what gets refused. Moving onto it would either merge two +/// items or strand the row that lost the collision. pub async fn move_drafts_for_path( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, w_id: &str, kinds: &[UserDraftItemKind], old_path: &str, new_path: &str, -) -> Result { +) -> Result<()> { let typs = kinds.iter().map(|k| k.as_str()).collect::>(); + let taken = sqlx::query_scalar!( + r#"SELECT count(*) as "n!" FROM draft + WHERE workspace_id = $1 AND path = $2 AND typ::text = ANY($3::text[])"#, + w_id, + new_path, + &typs as &[&str], + ) + .fetch_one(&mut **tx) + .await?; + if taken > 0 { + return Err(crate::error::Error::BadRequest(format!( + "'{new_path}' already has a draft on it — move it or discard it first" + ))); + } // Only the row's path column. The value is the user's payload and this deploy // is not their edit, so nothing in it is rewritten — including the path it // would deploy to, which stays whatever they last typed. A carried draft // therefore reads as out of date against the version this deploy created, // which is true, and the editor's stale prompt shows the diff that says // whether it matters. - let moved = sqlx::query_scalar!( - r#"UPDATE draft AS d + sqlx::query!( + r#"UPDATE draft SET path = $3 - WHERE d.workspace_id = $1 - AND d.path = $2 - AND d.typ::text = ANY($4::text[]) - AND NOT EXISTS ( - SELECT 1 FROM draft o - WHERE o.workspace_id = d.workspace_id - AND o.path = $3 - AND o.typ = d.typ - AND o.email IS NOT DISTINCT FROM d.email - ) - RETURNING d.id"#, + WHERE workspace_id = $1 + AND path = $2 + AND typ::text = ANY($4::text[])"#, w_id, old_path, new_path, &typs as &[&str], ) - .fetch_all(&mut **tx) + .execute(&mut **tx) .await?; - - // Anything still sitting at the old path was blocked by the collision - // guard. Counted rather than inferred from `moved`, so a concurrent insert - // at the old path is reported too. - let left_behind = sqlx::query_scalar!( - r#"SELECT count(*) as "n!" FROM draft - WHERE workspace_id = $1 AND path = $2 AND typ::text = ANY($3::text[])"#, - w_id, - old_path, - &typs as &[&str], - ) - .fetch_one(&mut **tx) - .await?; - - Ok(MoveDraftsOutcome { moved: moved.len(), left_behind: left_behind as usize }) -} - -/// What `move_drafts_for_path` did. `left_behind` is non-zero only when the -/// target owner already held a draft at the new path; those rows are now -/// orphaned at a path their item has left, so a caller that sees a non-zero -/// count owes the user a warning. -#[derive(Debug, Clone, Copy)] -pub struct MoveDraftsOutcome { - pub moved: usize, - pub left_behind: usize, + Ok(()) } /// Fetch the authed user's draft as a standalone payload, for "get by path"