starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-21 00:02:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat: refuse a rename onto a path that already holds a draft

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Guilhem Lemouel
2026-09-11 11:42:44 +02:00
co-authored by Claude Fable 5.1
parent 5de97332d3
commit 793e4dba6b
8 changed files with 139 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/.sqlx/query-af0400dcf733b00629c24118b218e76030efff8805bf2fb53061e4cbc8eed4a7.json
Generated
+17
View File
@@ -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"
}
backend/.sqlx/query-d336ad22e1f7ddc06fd9a49ad3ac55f4c17a91c279f2b736e98ea9a0ac1ecaf9.json
Generated
-25
View File
@@ -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"
}
backend/tests/drafts_move_taken.rs
+75
View File
@@ -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<Postgres>) -> 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(())
}
backend/tests/fixtures/drafts_move_taken.sql
Vendored
+17
View File
@@ -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');
backend/windmill-api-flows/src/flows.rs
+1 -9
View File
@@ -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(
backend/windmill-api-scripts/src/scripts.rs
+1 -9
View File
@@ -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!(
backend/windmill-api/src/apps.rs
+1 -8
View File
@@ -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,
backend/windmill-common/src/user_drafts.rs
+27 -45
View File
@@ -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<MoveDraftsOutcome> {
) -> Result<()> {
let typs = kinds.iter().map(|k| k.as_str()).collect::<Vec<_>>();
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"