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

fix: the other app kind collides whoever owns it, and session tabs get a head to compare

Browse Source 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-09-15 19:51:55 +02:00
co-authored by Claude Opus 5
parent 8e10e5a091
commit dfbc696ddb
8 changed files with 110 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/.sqlx/query-cb135f028eb579ee912c618f48f38d99f84311ac3297f04bc19425a4bddceb33.json → backend/.sqlx/query-0d9a83d77632bc0de19a48eddb7e6f80b2a41db4f1d352920d0c295ea3a018b1.json
Generated
+2 -2
View File
@@ -1,6 +1,6 @@
{
"db_name": "PostgreSQL",
"query": "UPDATE draft\n SET path = $3,\n -- Both path keys, not just the typed one: the editors mirror the\n -- typed path into the other while it differs from the row's path,\n -- and the loaders prefer the mirror — left naming the old location\n -- it un-does this move on the next save. `create_missing = false`\n -- on both, so a draft carrying only one keeps only one.\n value = to_json(\n jsonb_set(\n jsonb_set(\n CASE WHEN $7::text IS NULL THEN to_jsonb(value)\n ELSE jsonb_set(to_jsonb(value), ARRAY['summary'], to_jsonb($7::text))\n END,\n ARRAY[$5::text], to_jsonb($3::text), false\n ),\n ARRAY[$8::text], to_jsonb($3::text), false\n )\n )\n WHERE workspace_id = $1\n AND path = $2\n AND typ = $4\n AND email = $6\n -- A pre-sanitizer NUL escape makes `to_jsonb` raise 22P05. Excluded\n -- here so the statement can't 500; reported below instead. Unlike the\n -- passive carry, rewriting the value IS this operation, so skipping it\n -- silently would move the row and leave its typed path stale.\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) = 0\n -- Skipped on a summary-only edit, where the \"target\" row is this\n -- row and the guard would refuse the update against itself.\n AND ($2 = $3 OR NOT EXISTS (\n SELECT 1 FROM draft o\n WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[])\n -- The legacy row counts: a deploy at that path wipes it together with\n -- the caller's, so parking a second draft there discards edits the\n -- caller never saw.\n AND (o.email = $6 OR o.email IS NULL)\n ))\n RETURNING id",
"query": "UPDATE draft\n SET path = $3,\n -- Both path keys, not just the typed one: the editors mirror the\n -- typed path into the other while it differs from the row's path,\n -- and the loaders prefer the mirror — left naming the old location\n -- it un-does this move on the next save. `create_missing = false`\n -- on both, so a draft carrying only one keeps only one.\n value = to_json(\n jsonb_set(\n jsonb_set(\n CASE WHEN $7::text IS NULL THEN to_jsonb(value)\n ELSE jsonb_set(to_jsonb(value), ARRAY['summary'], to_jsonb($7::text))\n END,\n ARRAY[$5::text], to_jsonb($3::text), false\n ),\n ARRAY[$8::text], to_jsonb($3::text), false\n )\n )\n WHERE workspace_id = $1\n AND path = $2\n AND typ = $4\n AND email = $6\n -- A pre-sanitizer NUL escape makes `to_jsonb` raise 22P05. Excluded\n -- here so the statement can't 500; reported below instead. Unlike the\n -- passive carry, rewriting the value IS this operation, so skipping it\n -- silently would move the row and leave its typed path stale.\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) = 0\n -- Skipped on a summary-only edit, where the \"target\" row is this\n -- row and the guard would refuse the update against itself.\n AND ($2 = $3 OR NOT EXISTS (\n SELECT 1 FROM draft o\n WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[])\n -- Of this kind only the caller's own row and the legacy one collide:\n -- teammates' drafts of one item share its path by design, but a deploy\n -- there wipes those two together, so a second would discard edits the\n -- caller never saw. The other app kind is a different item on the same\n -- deployed path, so it collides whoever owns it.\n AND (o.typ <> $4 OR o.email = $6 OR o.email IS NULL)\n ))\n RETURNING id",
"describe": {
"columns": [
{
@@ -60,5 +60,5 @@
false
]
},
"hash": "cb135f028eb579ee912c618f48f38d99f84311ac3297f04bc19425a4bddceb33"
"hash": "0d9a83d77632bc0de19a48eddb7e6f80b2a41db4f1d352920d0c295ea3a018b1"
}
backend/.sqlx/query-07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8.json → backend/.sqlx/query-a1ed94ef0a6d4f7bc89ac4a2341bc06dc95d60ec92469523935f5a18a4d3f907.json
Generated
+9 -3
View File
@@ -1,6 +1,6 @@
{
"db_name": "PostgreSQL",
"query": "SELECT\n -- Own row first: with both an own and a legacy row at the destination,\n -- a bare LIMIT 1 would name an arbitrary one and the two need different\n -- remedies (discard your own vs. ask an admin).\n (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND (email = $4 OR email IS NULL)\n ORDER BY email NULLS LAST LIMIT 1) as \"at_target\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email = $4) as \"at_target_own!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email IS NULL) as \"legacy!\" ",
"query": "SELECT\n -- The guard's own predicate, ordered own row, then legacy, then another\n -- user's other-kind row: each needs a different remedy, and a bare\n -- LIMIT 1 would name an arbitrary one.\n (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[])\n AND (typ <> $2 OR email = $4 OR email IS NULL)\n ORDER BY CASE WHEN email = $4 THEN 0 WHEN email IS NULL THEN 1 ELSE 2 END\n LIMIT 1) as \"at_target\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email = $4) as \"at_target_own!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email IS NULL) as \"at_target_legacy!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email IS NULL) as \"legacy!\" ",
"describe": {
"columns": [
{
@@ -15,11 +15,16 @@
},
{
"ordinal": 2,
"name": "poisoned!",
"name": "at_target_legacy!",
"type_info": "Bool"
},
{
"ordinal": 3,
"name": "poisoned!",
"type_info": "Bool"
},
{
"ordinal": 4,
"name": "legacy!",
"type_info": "Bool"
}
@@ -72,8 +77,9 @@
null,
null,
null,
null,
null
]
},
"hash": "07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8"
"hash": "a1ed94ef0a6d4f7bc89ac4a2341bc06dc95d60ec92469523935f5a18a4d3f907"
}
backend/tests/drafts_move_taken.rs
+51
View File
@@ -181,3 +181,54 @@ async fn test_draft_move_refuses_the_other_app_kind(db: Pool<Postgres>) -> anyho
);
Ok(())
}
/// Teammates' drafts of one item share its path by design, so another user's row is no
/// obstacle — except across the app pair, where the two kinds are different items on one
/// deployed path: deploying either strands the other, and deleting the app takes both.
#[sqlx::test(fixtures("base", "drafts_move_taken"))]
async fn test_draft_move_refuses_another_users_other_app_kind(
db: Pool<Postgres>,
) -> anyhow::Result<()> {
initialize_tracing().await;
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let move_to = |kind: &'static str, from: &'static str, to: &'static str| async move {
let resp = reqwest::Client::new()
.post(format!(
"http://localhost:{port}/api/w/test-workspace/drafts/move/{kind}/{from}"
))
.header("Authorization", "Bearer SECRET_TOKEN")
.json(&json!({ "new_path": to }))
.send()
.await?;
Ok::<_, anyhow::Error>((resp.status(), resp.text().await?))
};
let (status, body) = move_to(
"app",
"u/test-user/mvtaken_app",
"u/test-user/mvtaken_theirs",
)
.await?;
assert_eq!(
status, 400,
"a classic app was moved onto another user's raw app: {body}"
);
assert!(
body.contains("Another user has a raw app draft"),
"the refusal did not name the occupant: {body}"
);
// The same-kind case is the ordinary one: two users' drafts of one raw app.
let (status, body) = move_to(
"raw_app",
"u/test-user/mvtaken_raw",
"u/test-user/mvtaken_theirs",
)
.await?;
assert!(
status.is_success(),
"a raw app was refused beside another user's raw-app draft: {body}"
);
Ok(())
}
backend/tests/fixtures/drafts_move_taken.sql
Vendored
+5 -2
View File
@@ -24,9 +24,12 @@ VALUES ('test-workspace', 'u/test-user/mvtaken_legacy', 'script',
NULL);
-- A draft-only classic app and a draft-only raw app of the same owner. They share
-- the `app` table, so one occupies the other's path.
-- the `app` table, so one occupies the other's path. The third is another user's raw
-- app: a different item at its own path, not a second copy of anyone's.
INSERT INTO draft (workspace_id, path, typ, value, email) VALUES
('test-workspace', 'u/test-user/mvtaken_app', 'app',
'{"summary": "classic", "value": {}}', 'test@windmill.dev'),
('test-workspace', 'u/test-user/mvtaken_raw', 'raw_app',
'{"summary": "raw", "files": {}}', 'test@windmill.dev');
'{"summary": "raw", "files": {}}', 'test@windmill.dev'),
('test-workspace', 'u/test-user/mvtaken_theirs', 'raw_app',
'{"summary": "theirs", "files": {}}', 'test2@windmill.dev');
backend/windmill-api/src/drafts.rs
+23 -10
View File
@@ -762,10 +762,12 @@ async fn move_draft(
AND ($2 = $3 OR NOT EXISTS (
SELECT 1 FROM draft o
WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[])
-- The legacy row counts: a deploy at that path wipes it together with
-- the caller's, so parking a second draft there discards edits the
-- caller never saw.
AND (o.email = $6 OR o.email IS NULL)
-- Of this kind only the caller's own row and the legacy one collide:
-- teammates' drafts of one item share its path by design, but a deploy
-- there wipes those two together, so a second would discard edits the
-- caller never saw. The other app kind is a different item on the same
-- deployed path, so it collides whoever owns it.
AND (o.typ <> $4 OR o.email = $6 OR o.email IS NULL)
))
RETURNING id"#,
&w_id,
@@ -796,14 +798,18 @@ async fn move_draft(
if moved.is_none() {
let row = sqlx::query!(
r#"SELECT
-- Own row first: with both an own and a legacy row at the destination,
-- a bare LIMIT 1 would name an arbitrary one and the two need different
-- remedies (discard your own vs. ask an admin).
-- The guard's own predicate, ordered own row, then legacy, then another
-- user's other-kind row: each needs a different remedy, and a bare
-- LIMIT 1 would name an arbitrary one.
(SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3
AND typ::text = ANY($6::text[]) AND (email = $4 OR email IS NULL)
ORDER BY email NULLS LAST LIMIT 1) as "at_target",
AND typ::text = ANY($6::text[])
AND (typ <> $2 OR email = $4 OR email IS NULL)
ORDER BY CASE WHEN email = $4 THEN 0 WHEN email IS NULL THEN 1 ELSE 2 END
LIMIT 1) as "at_target",
EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3
AND typ::text = ANY($6::text[]) AND email = $4) as "at_target_own!",
EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3
AND typ::text = ANY($6::text[]) AND email IS NULL) as "at_target_legacy!",
EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5
AND typ = $2 AND email = $4
AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0
@@ -841,7 +847,7 @@ async fn move_draft(
let occupant = occupant.replace('_', " ");
if row.at_target_own {
format!("You already have a draft at '{new_path}' ({occupant})")
} else {
} else if row.at_target_legacy {
// An ownerless row the caller cannot clear themselves, so send them to
// the one place it can be resolved rather than to "discard your draft".
format!(
@@ -849,6 +855,13 @@ async fn move_draft(
({occupant}). A workspace admin can claim or discard it on the Review & \
deploy drafts page."
)
} else {
// The other app kind, owned by someone else: one deployed path cannot hold
// both, so this is the other item's path, not a teammate's copy of this one.
format!(
"Another user has a {occupant} draft at '{new_path}', and an app and a \
raw app cannot share a path."
)
}
} else {
format!("You have no draft at '{path}'")
frontend/src/lib/components/sessions/FlowEditorView.svelte
+4 -3
View File
@@ -104,9 +104,9 @@
cell.store), reactive state owned by the SessionRuntime class (via
flowCell), not by a component ancestor — so Svelte's ownership check
flags a false positive here. -->
<!-- draftBaseVersion: the base the session's draft carries, so the deploy guard
compares the same pair the full-page editor does; without it every deploy
from a session tab reads as up to date. -->
<!-- draftBaseVersion / version: the base the session's draft carries and the head at
load time, so the deploy guard compares the same pair the full-page editor does;
without them every deploy from a session tab reads as up to date. -->
<!-- svelte-ignore ownership_invalid_binding -->
<FlowBuilder
flowStore={cell.store}
@@ -121,6 +121,7 @@
draftBaseVersion={cell.store.val?.version_id != null
? String(cell.store.val.version_id)
: undefined}
version={cell.saved.val?.version_id}
{diffDrawer}
{onNavigate}
condensedHeader={true}
frontend/src/lib/components/sessions/RawAppEditorView.svelte
+1 -2
View File
@@ -256,8 +256,7 @@
autosavePath={path}
policy={cell.store.val.policy}
bind:savedApp={cell.saved.val}
version={cell.store.val?.parent_version ??
(cell.saved.val as { versions?: number[] } | undefined)?.versions?.at(-1)}
version={cell.store.val?.parent_version ?? cell.saved.val?.deployed_version}
draftBaseVersion={cell.store.val?.parent_version != null
? String(cell.store.val.parent_version)
: undefined}
frontend/src/lib/components/sessions/sessionRuntime.svelte.ts
+15 -4
View File
@@ -146,6 +146,9 @@ export interface RawAppSavedValue {
/** No deployed counterpart (draft-only); disables the topbar Diff. */
no_deployed?: boolean
custom_path?: string
/** The deployed head at load time, which the editor's deploy guard falls back to when
* the draft carries no base of its own. */
deployed_version?: number
}
// One editor cell per (kind, path) the session loads: the load slot plus the
@@ -605,7 +608,9 @@ function createRuntime(session: Session): SessionRuntime {
// yet on the backend — draft-only flows are a valid state.
try {
const result = await FlowService.getFlowByPath({ workspace, path, getDraft: true })
saved.val = result as SavedFlow
// `getDraft` omits `version_id`; the editor's deploy guard compares
// against it, so put the head fetched above back on the baseline.
saved.val = { ...(result as SavedFlow), version_id: deployedVersionId }
} catch {
saved.val = undefined
}
@@ -617,7 +622,7 @@ function createRuntime(session: Session): SessionRuntime {
// No local draft yet — seed from `result.draft ?? result`.
const result = await FlowService.getFlowByPath({ workspace, path, getDraft: true })
saved.val = result as SavedFlow
saved.val = { ...(result as SavedFlow), version_id: deployedVersionId }
const serverDraft = (result as SavedFlow).draft as Flow | undefined
const flow: Flow = (serverDraft ?? (result as Flow)) as Flow
// Seed the per-tab last_sync from the server draft's timestamp so the
@@ -783,7 +788,10 @@ function createRuntime(session: Session): SessionRuntime {
path: result.path,
policy: result.policy,
custom_path: result.custom_path,
no_deployed: result.no_deployed
no_deployed: result.no_deployed,
deployed_version: Array.isArray(result.versions)
? result.versions[result.versions.length - 1]
: undefined
}
} catch {
saved.val = undefined
@@ -819,7 +827,10 @@ function createRuntime(session: Session): SessionRuntime {
path: result.path,
policy: result.policy,
custom_path: result.custom_path,
no_deployed: result.no_deployed
no_deployed: result.no_deployed,
deployed_version: Array.isArray(result.versions)
? result.versions[result.versions.length - 1]
: undefined
}
// Prefer the server draft over the deployed value (mirrors the
// flow/script `result.draft ?? result`). A raw-app draft is already