diff --git a/backend/.sqlx/query-d3686c73788b866090f25383da507e9a7d9f90d5f86fa4a1b89026cb088d0a58.json b/backend/.sqlx/query-0a8e315e6a808c9a8704d5cbdc607a011dba4472741df120c2a556c87fe2825f.json similarity index 83% rename from backend/.sqlx/query-d3686c73788b866090f25383da507e9a7d9f90d5f86fa4a1b89026cb088d0a58.json rename to backend/.sqlx/query-0a8e315e6a808c9a8704d5cbdc607a011dba4472741df120c2a556c87fe2825f.json index f4edb8a807..3cef79cd00 100644 --- a/backend/.sqlx/query-d3686c73788b866090f25383da507e9a7d9f90d5f86fa4a1b89026cb088d0a58.json +++ b/backend/.sqlx/query-0a8e315e6a808c9a8704d5cbdc607a011dba4472741df120c2a556c87fe2825f.json @@ -1,10 +1,20 @@ { "db_name": "PostgreSQL", - "query": "INSERT INTO draft (workspace_id, email, path, typ, value, created_at, base)\n VALUES ($1, $2, $3, $4, $5::text::json, COALESCE($8::timestamptz, now()), $9)\n ON CONFLICT (workspace_id, path, typ, email) WHERE email IS NOT NULL\n DO UPDATE SET value = EXCLUDED.value, created_at = EXCLUDED.created_at,\n base = EXCLUDED.base\n WHERE $7::bool = true\n OR $6::timestamptz IS NULL\n OR draft.created_at <= $6::timestamptz\n RETURNING created_at", + "query": "INSERT INTO draft (workspace_id, email, path, typ, value, created_at, base)\n VALUES ($1, $2, $3, $4, $5::text::json, COALESCE($8::timestamptz, now()), $9)\n ON CONFLICT (workspace_id, path, typ, email) WHERE email IS NOT NULL\n DO UPDATE SET value = EXCLUDED.value, created_at = EXCLUDED.created_at,\n base = EXCLUDED.base\n WHERE $7::bool = true\n OR $6::timestamptz IS NULL\n OR draft.created_at <= $6::timestamptz\n RETURNING id, path, created_at", "describe": { "columns": [ { "ordinal": 0, + "name": "id", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "path", + "type_info": "Varchar" + }, + { + "ordinal": 2, "name": "created_at", "type_info": "Timestamptz" } @@ -57,8 +67,10 @@ ] }, "nullable": [ + false, + false, false ] }, - "hash": "d3686c73788b866090f25383da507e9a7d9f90d5f86fa4a1b89026cb088d0a58" + "hash": "0a8e315e6a808c9a8704d5cbdc607a011dba4472741df120c2a556c87fe2825f" } diff --git a/backend/.sqlx/query-0cc6770a81ecaecafe0d9b7100f94b329c4ec7978b016af4c964415e0c38396c.json b/backend/.sqlx/query-10e85bd008777fe980bba2515ce7d927f3832b0de8b7a1b45223d09406c39a16.json similarity index 74% rename from backend/.sqlx/query-0cc6770a81ecaecafe0d9b7100f94b329c4ec7978b016af4c964415e0c38396c.json rename to backend/.sqlx/query-10e85bd008777fe980bba2515ce7d927f3832b0de8b7a1b45223d09406c39a16.json index 23d3b51c76..8c35c5eaa3 100644 --- a/backend/.sqlx/query-0cc6770a81ecaecafe0d9b7100f94b329c4ec7978b016af4c964415e0c38396c.json +++ b/backend/.sqlx/query-10e85bd008777fe980bba2515ce7d927f3832b0de8b7a1b45223d09406c39a16.json @@ -1,20 +1,25 @@ { "db_name": "PostgreSQL", - "query": "SELECT value as \"value!: sqlx::types::Json>\",\n created_at, base\n FROM draft\n WHERE workspace_id = $1\n AND (email = $2 OR email IS NULL)\n AND path = $3\n AND typ = $4\n ORDER BY email NULLS LAST\n LIMIT 1", + "query": "SELECT id, value as \"value!: sqlx::types::Json>\",\n created_at, base\n FROM draft\n WHERE workspace_id = $1\n AND (email = $2 OR email IS NULL)\n AND path = $3\n AND typ = $4\n ORDER BY email NULLS LAST\n LIMIT 1", "describe": { "columns": [ { "ordinal": 0, + "name": "id", + "type_info": "Int8" + }, + { + "ordinal": 1, "name": "value!: sqlx::types::Json>", "type_info": "Json" }, { - "ordinal": 1, + "ordinal": 2, "name": "created_at", "type_info": "Timestamptz" }, { - "ordinal": 2, + "ordinal": 3, "name": "base", "type_info": "Text" } @@ -62,10 +67,11 @@ ] }, "nullable": [ + false, false, false, true ] }, - "hash": "0cc6770a81ecaecafe0d9b7100f94b329c4ec7978b016af4c964415e0c38396c" + "hash": "10e85bd008777fe980bba2515ce7d927f3832b0de8b7a1b45223d09406c39a16" } diff --git a/backend/.sqlx/query-14a475e8d090d3caca0b76ab3b2e7b48417a376412d7ab7ecd2a324a307897b7.json b/backend/.sqlx/query-14a475e8d090d3caca0b76ab3b2e7b48417a376412d7ab7ecd2a324a307897b7.json new file mode 100644 index 0000000000..bc3b154d84 --- /dev/null +++ b/backend/.sqlx/query-14a475e8d090d3caca0b76ab3b2e7b48417a376412d7ab7ecd2a324a307897b7.json @@ -0,0 +1,59 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT path FROM draft WHERE id = $1 AND workspace_id = $2 AND typ = $3 AND email = $4", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Int8", + "Text", + { + "Custom": { + "name": "draft_kind", + "kind": { + "Enum": [ + "script", + "flow", + "app", + "raw_app", + "resource", + "variable", + "trigger_schedule", + "trigger_webhook", + "trigger_default_email", + "trigger_email", + "trigger_http", + "trigger_websocket", + "trigger_postgres", + "trigger_kafka", + "trigger_nats", + "trigger_mqtt", + "trigger_sqs", + "trigger_gcp", + "trigger_azure", + "trigger_poll", + "trigger_cli", + "trigger_nextcloud", + "trigger_google", + "trigger_github", + "data_pipeline", + "trigger_amqp" + ] + } + } + }, + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "14a475e8d090d3caca0b76ab3b2e7b48417a376412d7ab7ecd2a324a307897b7" +} diff --git a/backend/.sqlx/query-189a8c5a3ac2d9ece750aa1e01c08120b04670bf7261b2f5f5b61b1bb7fde5c8.json b/backend/.sqlx/query-189a8c5a3ac2d9ece750aa1e01c08120b04670bf7261b2f5f5b61b1bb7fde5c8.json deleted file mode 100644 index 9fa0e33c2d..0000000000 --- a/backend/.sqlx/query-189a8c5a3ac2d9ece750aa1e01c08120b04670bf7261b2f5f5b61b1bb7fde5c8.json +++ /dev/null @@ -1,35 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT path, created_by, hash FROM script\n WHERE workspace_id = $1 AND $2 = ANY(parent_hashes)\n AND NOT archived AND NOT deleted\n ORDER BY created_at DESC LIMIT 1", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "path", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "created_by", - "type_info": "Varchar" - }, - { - "ordinal": 2, - "name": "hash", - "type_info": "Int8" - } - ], - "parameters": { - "Left": [ - "Text", - "Int8" - ] - }, - "nullable": [ - false, - false, - false - ] - }, - "hash": "189a8c5a3ac2d9ece750aa1e01c08120b04670bf7261b2f5f5b61b1bb7fde5c8" -} diff --git a/backend/.sqlx/query-323f975428149815f3a9bc5c47396c1ad11eab84d03dd8c435b824e2e1b03ff1.json b/backend/.sqlx/query-323f975428149815f3a9bc5c47396c1ad11eab84d03dd8c435b824e2e1b03ff1.json deleted file mode 100644 index 53f0ca1d34..0000000000 --- a/backend/.sqlx/query-323f975428149815f3a9bc5c47396c1ad11eab84d03dd8c435b824e2e1b03ff1.json +++ /dev/null @@ -1,35 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT a.path, head.id as \"head!\", head.created_by as \"head_by!\"\n FROM app_version av\n JOIN app a ON a.id = av.app_id\n JOIN LATERAL (\n SELECT id, created_by FROM app_version\n WHERE app_id = a.id ORDER BY created_at DESC LIMIT 1\n ) head ON true\n WHERE av.id = $2 AND a.workspace_id = $1", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "path", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "head!", - "type_info": "Int8" - }, - { - "ordinal": 2, - "name": "head_by!", - "type_info": "Varchar" - } - ], - "parameters": { - "Left": [ - "Text", - "Int8" - ] - }, - "nullable": [ - false, - false, - false - ] - }, - "hash": "323f975428149815f3a9bc5c47396c1ad11eab84d03dd8c435b824e2e1b03ff1" -} diff --git a/backend/.sqlx/query-394fea4bb407ef3f800df87032aaf5d00d2fbcc50cb3615dacc85831a6fdf813.json b/backend/.sqlx/query-394fea4bb407ef3f800df87032aaf5d00d2fbcc50cb3615dacc85831a6fdf813.json deleted file mode 100644 index 55d9a4458a..0000000000 --- a/backend/.sqlx/query-394fea4bb407ef3f800df87032aaf5d00d2fbcc50cb3615dacc85831a6fdf813.json +++ /dev/null @@ -1,35 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT fv.path, f.edited_by, f.versions[array_upper(f.versions, 1)] as \"head!\"\n FROM flow_version fv\n JOIN flow f ON f.workspace_id = fv.workspace_id AND f.path = fv.path\n WHERE fv.id = $2 AND fv.workspace_id = $1", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "path", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "edited_by", - "type_info": "Varchar" - }, - { - "ordinal": 2, - "name": "head!", - "type_info": "Int8" - } - ], - "parameters": { - "Left": [ - "Text", - "Int8" - ] - }, - "nullable": [ - false, - false, - null - ] - }, - "hash": "394fea4bb407ef3f800df87032aaf5d00d2fbcc50cb3615dacc85831a6fdf813" -} diff --git a/backend/.sqlx/query-9ab0ebbe787d71d8a36618e8fbcab5de58f58d3d7bb9f4cc94a98fd138c28114.json b/backend/.sqlx/query-9ab0ebbe787d71d8a36618e8fbcab5de58f58d3d7bb9f4cc94a98fd138c28114.json deleted file mode 100644 index eead73a429..0000000000 --- a/backend/.sqlx/query-9ab0ebbe787d71d8a36618e8fbcab5de58f58d3d7bb9f4cc94a98fd138c28114.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM app WHERE workspace_id = $1 AND path = $2) as \"e!\"", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "e!", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "9ab0ebbe787d71d8a36618e8fbcab5de58f58d3d7bb9f4cc94a98fd138c28114" -} diff --git a/backend/.sqlx/query-c451a6d05b4aeb21ecbe29a355b8943c2b738ead4b870ef75c0d64a20bd83ecc.json b/backend/.sqlx/query-c451a6d05b4aeb21ecbe29a355b8943c2b738ead4b870ef75c0d64a20bd83ecc.json deleted file mode 100644 index ab97a26990..0000000000 --- a/backend/.sqlx/query-c451a6d05b4aeb21ecbe29a355b8943c2b738ead4b870ef75c0d64a20bd83ecc.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM flow WHERE workspace_id = $1 AND path = $2) as \"e!\"", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "e!", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "c451a6d05b4aeb21ecbe29a355b8943c2b738ead4b870ef75c0d64a20bd83ecc" -} diff --git a/backend/.sqlx/query-d5f06d5bca13ae5a22ef1290de4ebef6725238cea5ffbdc7a21b157b27d4c4dd.json b/backend/.sqlx/query-d5f06d5bca13ae5a22ef1290de4ebef6725238cea5ffbdc7a21b157b27d4c4dd.json deleted file mode 100644 index 9afa90e619..0000000000 --- a/backend/.sqlx/query-d5f06d5bca13ae5a22ef1290de4ebef6725238cea5ffbdc7a21b157b27d4c4dd.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2\n AND NOT archived AND NOT deleted) as \"e!\"", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "e!", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "d5f06d5bca13ae5a22ef1290de4ebef6725238cea5ffbdc7a21b157b27d4c4dd" -} diff --git a/backend/tests/drafts_moved_rls.rs b/backend/tests/drafts_moved_rls.rs deleted file mode 100644 index 1af90e0840..0000000000 --- a/backend/tests/drafts_moved_rls.rs +++ /dev/null @@ -1,61 +0,0 @@ -//! `POST /drafts/update` must never name a destination the caller cannot see. -//! -//! When an item moves, the saver's editor is still bound to the old path and its -//! next save is answered with `status: "moved"` plus the new path and the mover's -//! username. That answer is resolved twice — once before the write, once after it -//! on the write's own connection — and both must read under RLS. A post-write -//! re-assert on a raw pool connection reads rows the pre-check cannot see, so it -//! discloses the destination and permanently refuses a save that should land. - -use serde_json::{json, Value}; -use sqlx::{Pool, Postgres}; - -use windmill_test_utils::*; - -/// Hex form of script hash 7001, the way a script draft stores `parent_hash`. -const OLD_HASH: &str = "0000000000001b59"; - -async fn save_draft(port: u16, token: &str) -> anyhow::Result { - Ok(reqwest::Client::new() - .post(format!( - "http://localhost:{port}/api/w/test-workspace/drafts/update/script/f/mvrls_visible/s1" - )) - .header("Authorization", format!("Bearer {token}")) - .json(&json!({ - "value": { - "path": "f/mvrls_visible/s1", - "parent_hash": OLD_HASH, - "content": "export function main() { return 2 }", - "language": "deno", - "summary": "S1", - "description": "", - "schema": {} - } - })) - .send() - .await? - .json() - .await?) -} - -#[sqlx::test(fixtures("base", "drafts_moved_rls"))] -async fn test_moved_answer_is_rls_scoped(db: Pool) -> anyhow::Result<()> { - initialize_tracing().await; - let server = ApiServer::start(db.clone()).await?; - let port = server.addr.port(); - - // test-user-2 writes in `mvrls_visible` but has nothing on `mvrls_secret`. - // They must be told the save landed, not where the item went. - let res = save_draft(port, "SECRET_TOKEN_2").await?; - assert_eq!(res["status"], "saved", "non-admin save was refused: {res}"); - assert_eq!(res["moved_to"], Value::Null, "destination disclosed: {res}"); - assert_eq!(res["moved_by"], Value::Null, "mover disclosed: {res}"); - - // The admin sees the destination, so they get the real answer. - let res = save_draft(port, "SECRET_TOKEN").await?; - assert_eq!(res["status"], "moved", "admin was not told it moved: {res}"); - assert_eq!(res["moved_to"], "f/mvrls_secret/s1"); - assert_eq!(res["moved_by"], "test-user"); - - Ok(()) -} diff --git a/backend/tests/drafts_save_by_id.rs b/backend/tests/drafts_save_by_id.rs new file mode 100644 index 0000000000..c1484fc89f --- /dev/null +++ b/backend/tests/drafts_save_by_id.rs @@ -0,0 +1,107 @@ +//! A draft saved by row id lands where the row is, not where the editor was. +//! +//! A rename carries every draft on the item to the new path. An editor left open +//! across it is still bound to the old path; saving by the row's id writes at the +//! item's current path and the response names it, so the editor can follow. A +//! save by path would instead have planted a phantom draft at the old location. + +use serde_json::{json, Value}; +use sqlx::{Pool, Postgres}; + +use windmill_test_utils::*; + +/// Hex form of script hash 7030, the way the API takes a parent hash. +const HEAD_HASH: &str = "0000000000001b76"; +const DRAFT_ID: i64 = 9001; + +async fn own_draft_paths(port: u16) -> anyhow::Result> { + let list: Vec = reqwest::Client::new() + .get(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/list" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .send() + .await? + .json() + .await?; + Ok(list + .iter() + .filter(|d| d["kind"] == "script") + .filter_map(|d| d["path"].as_str().map(String::from)) + .filter(|p| p.starts_with("u/test-user/byid_")) + .collect()) +} + +#[sqlx::test(fixtures("base", "drafts_save_by_id"))] +async fn test_save_by_id_follows_a_rename(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let client = reqwest::Client::new(); + + // Rename from Home: a redeploy of the deployed content at the new path that + // keeps the deployer's own draft, so the draft is carried rather than consumed. + 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/byid_b", + "parent_hash": HEAD_HASH, + "summary": "A", + "description": "", + "content": "export function main() { return 1 }", + "language": "deno", + "schema": {}, + "skip_draft_deletion": true + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "rename failed: {}", resp.text().await?); + assert_eq!(own_draft_paths(port).await?, vec!["u/test-user/byid_b"]); + + // The editor is still on the old path but saves by id. + let saved: Value = client + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/update/script/u/test-user/byid_a" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ + "id": DRAFT_ID, + "value": { + "path": "u/test-user/byid_a", + "parent_hash": HEAD_HASH, + "summary": "A", + "content": "edited after the move" + } + })) + .send() + .await? + .json() + .await?; + assert_eq!(saved["status"], "saved", "save refused: {saved}"); + assert_eq!(saved["id"], DRAFT_ID); + assert_eq!( + saved["path"], "u/test-user/byid_b", + "save did not follow the row: {saved}" + ); + + // The write landed on the carried row; nothing reappeared at the old path. + assert_eq!(own_draft_paths(port).await?, vec!["u/test-user/byid_b"]); + let draft: Value = client + .get(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/get_own/script/u/test-user/byid_b" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .send() + .await? + .json() + .await?; + assert_eq!( + draft["value"]["content"], "edited after the move", + "{draft}" + ); + + Ok(()) +} diff --git a/backend/tests/fixtures/drafts_moved_rls.sql b/backend/tests/fixtures/drafts_moved_rls.sql deleted file mode 100644 index 3dea7a8d64..0000000000 --- a/backend/tests/fixtures/drafts_moved_rls.sql +++ /dev/null @@ -1,30 +0,0 @@ --- Fixture for the "moved" draft answer under RLS. --- --- Models a script that has already been moved out of a folder the saver can --- reach (`mvrls_visible`, where test-user-2 is a writer) and into one they have --- no permission on (`mvrls_secret`). A script move goes through `create_script`, --- which archives the row in place and inserts a successor carrying the old hash --- in `parent_hashes` — the state reproduced here. - -INSERT INTO folder (workspace_id, name, display_name, owners, extra_perms, created_by) -VALUES ('test-workspace', 'mvrls_visible', 'Visible', '{"u/test-user"}', - '{"u/test-user": true, "u/test-user-2": true}', 'test-user'); - --- No entry for test-user-2: the destination is invisible to them. -INSERT INTO folder (workspace_id, name, display_name, owners, extra_perms, created_by) -VALUES ('test-workspace', 'mvrls_secret', 'Secret', '{"u/test-user"}', - '{"u/test-user": true}', 'test-user'); - --- The pre-move row, archived in place at the old path. Hash 7001 = 0x1b59. -INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, - schema, summary, description, lock, extra_perms, archived) -VALUES ('test-workspace', 7001, 'f/mvrls_visible/s1', - 'export function main() { return 1 }', - 'deno', 'script', 'test-user', '{}', 'S1', '', '', '{}', true); - --- The post-move row, at the destination, pointing back at 7001. -INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, - schema, summary, description, lock, extra_perms, parent_hashes) -VALUES ('test-workspace', 7002, 'f/mvrls_secret/s1', - 'export function main() { return 1 }', - 'deno', 'script', 'test-user', '{}', 'S1', '', '', '{}', '{7001}'); diff --git a/backend/tests/fixtures/drafts_save_by_id.sql b/backend/tests/fixtures/drafts_save_by_id.sql new file mode 100644 index 0000000000..10759cb45a --- /dev/null +++ b/backend/tests/fixtures/drafts_save_by_id.sql @@ -0,0 +1,16 @@ +-- Fixture for saving a draft by row id across a rename. +-- +-- A deployed script at `u/test-user/byid_a` (hash 7030 = 0x1b76) with the +-- deployer's own draft on it. The test renames the script and then saves the +-- draft by id from an editor still bound to the old path. + +INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, + schema, summary, description, lock, extra_perms) +VALUES ('test-workspace', 7030, 'u/test-user/byid_a', + 'export function main() { return 1 }', + 'deno', 'script', 'test-user', '{}', 'A', '', '', '{}'); + +INSERT INTO draft (id, workspace_id, path, typ, value, email, base) +VALUES (9001, 'test-workspace', 'u/test-user/byid_a', 'script', + '{"path": "u/test-user/byid_a", "parent_hash": "0000000000001b76", "summary": "A", "content": "draft"}', + 'test@windmill.dev', '0000000000001b76'); diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index f62abc708e..436fee424f 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -9430,6 +9430,10 @@ paths: type: string format: date-time description: Upsert-only override for the stored creation timestamp. Normal saves omit it (stamped server-side); the localStorage→DB migration passes the draft's original write time so migrated drafts keep their age. + id: + type: integer + format: int64 + description: The draft row to write, from a previous save's `id` or the get-by-path `draft_id`. Addresses the row wherever a move took it; the URL path only applies when the id names no row of the caller's any more. responses: "200": description: save result @@ -9440,21 +9444,17 @@ paths: properties: status: type: string - enum: [saved, conflict, moved] - description: "`moved`: the item left this path, so nothing was written — see `moved_to`." + enum: [saved, conflict] current_timestamp: type: string format: date-time - moved_to: + id: + type: integer + format: int64 + description: "`saved` upserts only: the row's id, to save by from now on." + path: type: string - description: "`moved` only: the path the item lives at now." - moved_by: - type: string - description: "`moved` only: who last deployed it there. Best-effort." - moved_patch: - type: object - additionalProperties: true - description: "`moved` only: fields to merge into the refused draft before re-saving it at `moved_to` — the typed target path and the version the item now sits at. Sent as a patch so the client does not reproduce the per-kind key names." + description: "`saved` upserts only: where the row is. Differs from the URL path once a move has carried the row elsewhere; the editor follows it there." required: [status, current_timestamp] /w/{workspace}/drafts/move/{kind}/{path}: @@ -26105,6 +26105,10 @@ components: draft_saved_at: type: string format: date-time + draft_id: + type: integer + format: int64 + description: The draft row's id. Pass it as `id` on updateDraft so saves follow the row through a move. draft_base: type: string description: | diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index 5fe98559a5..cd6d964416 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -14,11 +14,9 @@ use axum::{ Json, Router, }; use serde::{Deserialize, Serialize}; -use serde_json::json; use windmill_common::{ db::UserDB, error::{Error, Result}, - scripts::ScriptHash, user_drafts::{DraftUserRef, UserDraftItemKind, ENCRYPTED_DRAFT_PREFIX}, users::resolve_username_to_email, utils::{check_proper_path, strip_json_nul}, @@ -314,6 +312,11 @@ pub struct SaveDraftRequest { /// keep their age instead of all resurfacing to the top as freshly created. #[serde(default)] pub created_at: Option>, + /// The draft row to write, when the client has saved or loaded it before. + /// Addresses the row wherever a move took it; the URL path is only the + /// fallback when the id names no row of the caller's any more. + #[serde(default)] + pub id: Option, } #[derive(Serialize, Debug)] @@ -321,10 +324,6 @@ pub struct SaveDraftRequest { pub enum SaveDraftStatus { Saved, Conflict, - /// The item this draft belongs to was moved away from this path. Nothing - /// was written — writing would plant a phantom draft-only item at a path - /// the item has left. - Moved, } #[derive(Serialize, Debug)] @@ -332,20 +331,14 @@ pub struct SaveDraftResponse { pub status: SaveDraftStatus, /// On `saved`: when the change was applied (client remembers it as the /// next `last_sync`). On `conflict`: the existing row's `created_at`. - /// On `moved`: the server's now(). pub current_timestamp: chrono::DateTime, - /// `moved` only: where the item lives now. + /// `saved` upserts only: the row's id, to save by from now on. #[serde(skip_serializing_if = "Option::is_none")] - pub moved_to: Option, - /// `moved` only: who last deployed it at its new path. Best-effort. + pub id: Option, + /// `saved` upserts only: where the row is. Differs from the URL path once a + /// move has carried the row elsewhere; the editor follows it there. #[serde(skip_serializing_if = "Option::is_none")] - pub moved_by: Option, - /// `moved` only: the path keys to merge into the refused draft before - /// re-saving it at `moved_to`. Sent as a patch so the client never has to - /// reproduce the per-kind key names (`UserDraftItemKind::typed_path_field` - /// and `mirror_path_field`). - #[serde(skip_serializing_if = "Option::is_none")] - pub moved_patch: Option, + pub path: Option, } /// The version a draft forked from, as the editors write it into `draft.value`. @@ -377,11 +370,9 @@ impl DraftBaseVersion { } } -/// The version this draft forked from, or `None` when it has no lineage to -/// follow — a kind that keeps none, a malformed payload, or a draft that was -/// never forked from a deploy. Pure: no queries. This is the escape hatch the -/// autosave path is built around, so every caller that is about to spend a -/// round-trip on move detection should consult it first. +/// The version this draft forked from, or `None` when it has none — a kind +/// that keeps no lineage, a malformed payload, or a draft that was never forked +/// from a deploy. Pure: no queries. fn draft_lineage(kind: UserDraftItemKind, value: &str) -> Option { use UserDraftItemKind::*; if !matches!(kind, Script | Flow | App | RawApp) { @@ -400,228 +391,15 @@ fn draft_lineage(kind: UserDraftItemKind, value: &str) -> Option Result, serde_json::Value)>> { - // `UserDB::begin` is not a bare BEGIN — it also issues `set_session_context` - // and, under `PG_SCHEMA`, `SET LOCAL search_path`, so this wraps one indexed - // existence check in 3-4 round-trips. Callers keep that off a draft with - // nothing to follow by having no `DraftBaseVersion` to pass. - let mut tx = user_db.clone().begin(authed).await?; - let moved = resolve_moved_to_in(&mut tx, w_id, kind, path, base).await; - tx.commit().await?; - moved -} - -/// The body of `resolve_moved_to`, on a caller-supplied transaction. Split out -/// so the post-write re-assert can reuse the connection it already holds rather -/// than acquiring a second one from the same pool while holding an open -/// transaction — that pattern stalls under pool pressure. The transaction it is -/// handed must be RLS-scoped: it reports a path and a username the caller may -/// have no access to, and passing the write gate at the old path says nothing -/// about what the caller may see at the new one. -async fn resolve_moved_to_in( - tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, - w_id: &str, - kind: UserDraftItemKind, - path: &str, - base: &DraftBaseVersion, -) -> Result, serde_json::Value)>> { - use UserDraftItemKind::*; - let script_hash = base - .parent_hash - .as_deref() - .and_then(|h| windmill_common::scripts::to_i64(h).ok()); - - let moved = match kind { - Script => { - // An archived row keeps sitting at the old path, so a plain - // existence check would miss every script move. - let still_here = sqlx::query_scalar!( - r#"SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2 - AND NOT archived AND NOT deleted) as "e!""#, - w_id, - path, - ) - .fetch_one(&mut **tx) - .await?; - if still_here { - None - } else { - // Only reached once the item is gone. `parent_hashes` carries no - // index, so this is a workspace-wide scan — keeping it behind the - // cheap check above is what leaves the autosave hot path at one - // indexed lookup. - sqlx::query!( - r#"SELECT path, created_by, hash FROM script - WHERE workspace_id = $1 AND $2 = ANY(parent_hashes) - AND NOT archived AND NOT deleted - ORDER BY created_at DESC LIMIT 1"#, - w_id, - script_hash, - ) - .fetch_optional(&mut **tx) - .await? - // Hex text, the way the API serializes a hash and the way a - // script draft stores `parent_hash`. - .map(|r| { - ( - r.path, - Some(r.created_by), - json!(ScriptHash(r.hash).to_string()), - ) - }) - } - } - Flow => { - let still_here = sqlx::query_scalar!( - r#"SELECT EXISTS(SELECT 1 FROM flow WHERE workspace_id = $1 AND path = $2) as "e!""#, - w_id, - path, - ) - .fetch_one(&mut **tx) - .await?; - if still_here { - None - } else { - sqlx::query!( - r#"SELECT fv.path, f.edited_by, f.versions[array_upper(f.versions, 1)] as "head!" - FROM flow_version fv - JOIN flow f ON f.workspace_id = fv.workspace_id AND f.path = fv.path - WHERE fv.id = $2 AND fv.workspace_id = $1"#, - w_id, - base.version_id, - ) - .fetch_optional(&mut **tx) - .await? - .map(|r| (r.path, Some(r.edited_by), json!(r.head))) - } - } - App | RawApp => { - let still_here = sqlx::query_scalar!( - r#"SELECT EXISTS(SELECT 1 FROM app WHERE workspace_id = $1 AND path = $2) as "e!""#, - w_id, - path, - ) - .fetch_one(&mut **tx) - .await?; - if still_here { - None - } else { - // `created_by` must come from the HEAD row, not from `av` — `av` - // is the version the draft forked from, whose author is usually - // the person now reading this. Naming them would make - // `moved_by_me` true for the wrong user and restamp a draft that - // has never seen the head's content. - sqlx::query!( - r#"SELECT a.path, head.id as "head!", head.created_by as "head_by!" - FROM app_version av - JOIN app a ON a.id = av.app_id - JOIN LATERAL ( - SELECT id, created_by FROM app_version - WHERE app_id = a.id ORDER BY created_at DESC LIMIT 1 - ) head ON true - WHERE av.id = $2 AND a.workspace_id = $1"#, - w_id, - base.parent_version, - ) - .fetch_optional(&mut **tx) - .await? - .map(|r| (r.path, Some(r.head_by), json!(r.head))) - } - } - _ => None, - }; - - // Same path back ⇒ nothing moved (a stale read, or a path reused). - let moved = moved.filter(|(new_path, _, _)| new_path != path); - - // "Continue at the new path" is the user explicitly relocating their edits, so - // both path keys are set to the destination — unlike the passive carry, which - // leaves the value alone because it is not the user's action. The version is - // NOT restamped: the draft really is behind the version the move created, and - // the editor's stale prompt shows the diff that says whether that matters. - // `create_missing` semantics are the client's: it merges this over the value, - // so a key the draft never had is added, which is what relocating means here. - Ok(moved.map(|(new_path, new_by, _head)| { - let mut patch = serde_json::Map::new(); - for field in [kind.typed_path_field(), kind.mirror_path_field()] - .into_iter() - .flatten() - { - patch.insert(field.to_string(), json!(&new_path)); - } - (new_path, new_by, serde_json::Value::Object(patch)) - })) -} - -/// Is the item still deployed at `path`? One indexed existence check, used to -/// NARROW — not close — the window between the `moved` pre-check and the write -/// that follows it. A residual remains: the deploy that moves an item calls -/// `move_drafts_for_path` inside its own transaction and then does more work -/// before committing, so a first save with no prior row at the old path can read -/// the pre-move snapshot under READ COMMITTED, take no lock, and commit a stray -/// row. That row is bounded — the editor's next autosave hits the pre-check and -/// is told the item moved. A save that DOES have a row there serialises behind -/// the mover's own UPDATE on that tuple and detects the move correctly. -/// Runs on the write's own RLS-scoped connection. An item hidden by RLS reads as -/// `false` here, which asks `resolve_moved_to_in`, which answers `None`, so the -/// save lands — the same outcome as the `true` this would return if the row were -/// visible and unmoved. -/// -/// `false` covers three different situations — moved, deleted, and a genuinely -/// draft-only item that never had a deployed row — so it is only ever a cue to -/// ask `resolve_moved_to`, never a verdict on its own. -async fn deployed_still_at( - tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, - w_id: &str, - kind: UserDraftItemKind, - path: &str, -) -> Result { - let Some(table) = kind.deployed_table() else { - return Ok(false); - }; - // A script move archives its parent in place, so the row stays at the old - // path and a bare existence check would report "still here" for every script - // move. `table` comes from the closed `deployed_table()` enum, never input. - let archived_filter = if table == "script" { - " AND NOT archived AND NOT deleted" - } else { - "" - }; - let q = format!( - "SELECT EXISTS(SELECT 1 FROM {table} WHERE workspace_id = $1 AND path = $2{archived_filter})" - ); - Ok(sqlx::query_scalar::<_, bool>(&q) - .bind(w_id) - .bind(path) - .fetch_one(&mut **tx) - .await?) -} - -/// Apply the current user's draft at (workspace, kind, path): non-null `value` -/// upserts, `null` (or omitted) deletes. Either way, when the existing row is -/// newer than `last_sync` (and `force` is false) the op is skipped and the -/// response is `status = conflict` + the server's current timestamp. +/// The row is addressed by `id` when the client has one, and by the URL path +/// otherwise. An id follows the row wherever a move took it, so an editor left +/// open across a rename writes at the item's current path instead of planting +/// a phantom draft at the one it left; the response names that path. async fn update_draft( authed: ApiAuthed, Extension(db): Extension, @@ -630,7 +408,7 @@ async fn update_draft( Json(req): Json, ) -> Result> { let email = &authed.email; - let path = path.to_path(); + let url_path = path.to_path(); // Saving a draft requires write permission on the underlying path. Deleting // (discarding) one's OWN draft does not: the email-scoped row belongs to the // authed user, so they can always discard it even after losing write access @@ -640,57 +418,47 @@ async fn update_draft( // — they keep the write gate. let is_own_discard = req.value.is_none() && !req.legacy; - // Rejected before the moved branch below, not with the rest of the write gate - // after it. An operator can never save a draft, so a `moved` answer is no use - // to them — and letting them past would hand a read-only role the unindexed - // `parent_hashes` lineage scan, repeatable with any fabricated base version. if !is_own_discard && authed.is_operator { return Err(Error::NotAuthorized( "operators cannot save drafts".to_string(), )); } - // An editor left open across someone else's move is still bound to the old - // path and would re-plant its draft there. Refuse and answer with where the - // item went; the carried draft is already waiting at the new path. - // - // Answered BEFORE the write gate, and deliberately: that gate resolves against - // the OLD path, where a move has left no deployed row, so a collaborator whose - // write access came from the item's own `extra_perms` would be told - // "unauthorized" for an item they still have permission on — and never learn - // where it went. Nothing is written on this branch, and `resolve_moved_to` - // reads under RLS, so it can only name an item the caller can already see. - // - // Parsed once here and threaded to all three sites that need it: an app draft - // runs to hundreds of KB, and serde tokenizes the whole document even to skip - // the keys it does not want. - let lineage = req - .value - .as_ref() - .and_then(|value| draft_lineage(kind, value.0.get())); - if let Some(base) = &lineage { - if let Some((moved_to, moved_by, moved_patch)) = - resolve_moved_to(&authed, &user_db, &w_id, kind, path, base).await? - { - let now = sqlx::query_scalar!(r#"SELECT now() as "now!""#) - .fetch_one(&db) - .await?; - return Ok(Json(SaveDraftResponse { - status: SaveDraftStatus::Moved, - current_timestamp: now, - moved_to: Some(moved_to), - moved_by, - moved_patch: Some(moved_patch), - })); + // Where the row is now. `None` when the id names no row of ours any more + // (discarded elsewhere, or deleted with its item): the URL path then applies, + // as it does for a first save. + let row_path = match req.id { + Some(id) => { + sqlx::query_scalar!( + "SELECT path FROM draft WHERE id = $1 AND workspace_id = $2 AND typ = $3 AND email = $4", + id, + &w_id, + kind as UserDraftItemKind, + email, + ) + .fetch_optional(&db) + .await? + } + None => None, + }; + let followed = row_path.is_some(); + let path: &str = row_path.as_deref().unwrap_or(url_path); + + // Everything past here writes, so the gate applies from here on. Answered + // without the path when the row was followed: the move may have taken it + // somewhere the caller cannot see. + if !is_own_discard { + match require_can_write_path(&authed, &db, &user_db, &w_id, kind, path).await { + Err(Error::NotAuthorized(_)) if followed => { + return Err(Error::NotAuthorized( + "this draft's item was moved to a path you cannot write".to_string(), + )); + } + other => other?, } } - // Everything past here writes, so the gate applies from here on. - if !is_own_discard { - require_can_write_path(&authed, &db, &user_db, &w_id, kind, path).await?; - } - - let applied_at = if let Some(value) = &req.value { + let applied = if let Some(value) = &req.value { // Secret variable values must never sit in `draft.value` in plaintext // (see `encrypt_secret_variable_value`). let serialized = if kind == UserDraftItemKind::Variable { @@ -702,33 +470,14 @@ async fn update_draft( // escape and later make any `->>`/`to_jsonb` extraction raise `22P05`. // Strip it here so a NUL never reaches the column. let serialized = strip_json_nul(&serialized); + // `base` is derived here from the value's per-kind field rather than sent + // by the client, so every writer (editors, chat, CLI) fills it the same way. + let base = draft_lineage(kind, value.0.get()).and_then(|l| l.as_text(kind)); // Upsert. The conflict check rides on the DO UPDATE WHERE clause — // when the row is newer than `last_sync`, RETURNING yields nothing. // `created_at` defaults to `now()` but the migration overrides it ($8) // so a migrated draft keeps its original age instead of jumping to top. - // The re-assert below needs a transaction to be able to undo the write, - // but only a draft with lineage can ever be told it moved — so a draft - // with none (a draft-only item, a variable, a trigger) keeps the plain - // single-statement write instead of paying BEGIN/COMMIT for a check that - // cannot fire. - // RLS-scoped, because the re-assert below reads the item tables through - // this same connection and must see exactly what the pre-check saw. `draft` - // itself carries no policies and grants `windmill_user` full access, so the - // upsert is unaffected by the role. - let mut tx = match &lineage { - Some(_) => Some(user_db.clone().begin(&authed).await?), - None => None, - }; - // One owned connection for the no-transaction case, so the executor below - // borrows from a binding that outlives the call. - let mut plain = match tx { - Some(_) => None, - None => Some(db.acquire().await?), - }; - // `base` is derived here from the value's per-kind field rather than sent - // by the client, so every writer (editors, chat, CLI) fills it the same way. - let base = lineage.as_ref().and_then(|l| l.as_text(kind)); - let applied = sqlx::query_scalar!( + sqlx::query!( r#"INSERT INTO draft (workspace_id, email, path, typ, value, created_at, base) VALUES ($1, $2, $3, $4, $5::text::json, COALESCE($8::timestamptz, now()), $9) ON CONFLICT (workspace_id, path, typ, email) WHERE email IS NOT NULL @@ -737,7 +486,7 @@ async fn update_draft( WHERE $7::bool = true OR $6::timestamptz IS NULL OR draft.created_at <= $6::timestamptz - RETURNING created_at"#, + RETURNING id, path, created_at"#, &w_id, email, path, @@ -748,45 +497,9 @@ async fn update_draft( req.created_at, base.as_deref(), ) - .fetch_optional(match (tx.as_mut(), plain.as_mut()) { - (Some(tx), _) => &mut **tx as &mut sqlx::PgConnection, - (None, Some(conn)) => &mut **conn, - (None, None) => unreachable!("exactly one of tx/plain is set"), - }) - .await?; - if let Some(mut tx) = tx { - // Cheap on the path that matters: one indexed existence check when the - // write landed. Only when it says the item is gone do we pay for the - // lineage lookup — and that answer is what distinguishes a move (roll - // back, report it) from a delete or a never-deployed draft-only item - // (both legitimate saves, which resolve to `None`). - // - // Run on the connection we already hold: acquiring a second from the - // same pool while this transaction is open is the two-connection stall. - // `tx` exists only because `lineage` did, so this is the same draft the - // pre-check consulted. - if let Some(base) = lineage.as_ref().filter(|_| applied.is_some()) { - if !deployed_still_at(&mut tx, &w_id, kind, path).await? { - if let Some((moved_to, moved_by, moved_patch)) = - resolve_moved_to_in(&mut tx, &w_id, kind, path, base).await? - { - tx.rollback().await?; - let now = sqlx::query_scalar!(r#"SELECT now() as "now!""#) - .fetch_one(&db) - .await?; - return Ok(Json(SaveDraftResponse { - status: SaveDraftStatus::Moved, - current_timestamp: now, - moved_to: Some(moved_to), - moved_by, - moved_patch: Some(moved_patch), - })); - } - } - } - tx.commit().await?; - } - applied + .fetch_optional(&db) + .await? + .map(|r| (r.created_at, Some(r.id), Some(r.path))) } else { // Delete, same conflict rule in the WHERE clause. Returns NULL when // the row was too new (conflict) OR already absent (idempotent) — @@ -811,15 +524,15 @@ async fn update_draft( ) .fetch_optional(&db) .await? + .map(|ts| (ts, None, None)) }; - if let Some(ts) = applied_at { + if let Some((ts, id, path)) = applied { return Ok(Json(SaveDraftResponse { status: SaveDraftStatus::Saved, current_timestamp: ts, - moved_to: None, - moved_by: None, - moved_patch: None, + id, + path, })); } @@ -844,9 +557,8 @@ async fn update_draft( Some(ts) => Ok(Json(SaveDraftResponse { status: SaveDraftStatus::Conflict, current_timestamp: ts, - moved_to: None, - moved_by: None, - moved_patch: None, + id: None, + path: None, })), // Delete + nothing-was-there ⇒ report success with server's NOW(). None => { @@ -856,9 +568,8 @@ async fn update_draft( Ok(Json(SaveDraftResponse { status: SaveDraftStatus::Saved, current_timestamp: now, - moved_to: None, - moved_by: None, - moved_patch: None, + id: None, + path: None, })) } } @@ -878,12 +589,8 @@ pub struct MoveDraftRequest { /// keys inside its value — there is no deployed row, schedule or trigger to /// cascade to. /// -/// The owner's OWN open editor is not notified, and cannot be: the moved-item -/// handshake in `update_draft` resolves through a deployed version chain, and a -/// draft-only item has none by definition. An editor still open on the old path -/// re-plants a row there on its next autosave, leaving two items. Closing that -/// needs a stable identity for a draft-only item — a tombstone or a surrogate id -/// — which is a larger change than this endpoint. +/// The owner's own open editor follows: it saves by the row's id, so its next +/// autosave lands at the new path and it is told where that is. /// /// Scoped to the caller's own row on purpose: two users can each have a draft /// at the same never-deployed path, and those are two separate items. diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index 3c24f85b4a..9489e20806 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -240,6 +240,10 @@ pub struct WithDraftOverlay { pub is_draft: bool, #[serde(skip_serializing_if = "Option::is_none")] pub draft_saved_at: Option>, + /// The draft row's id. The editor saves by it from then on, so its writes + /// follow the row through a move. + #[serde(skip_serializing_if = "Option::is_none")] + pub draft_id: Option, /// The deployed version the draft forked from (`draft.base`), as text /// whatever the kind. The editor compares it to the head it loaded to tell /// a draft that is behind. Absent when there is no draft or it was never @@ -331,6 +335,7 @@ where inner: Box::new(deployed), is_draft: false, draft_saved_at: None, + draft_id: None, draft_base: None, no_deployed: false, draft: None, @@ -351,7 +356,7 @@ where // NULL-email workspace draft. `NULLS LAST` + `LIMIT 1` drops the legacy // row when an owned one exists. let row = sqlx::query!( - r#"SELECT value as "value!: sqlx::types::Json>", + r#"SELECT id, value as "value!: sqlx::types::Json>", created_at, base FROM draft WHERE workspace_id = $1 @@ -373,6 +378,7 @@ where inner: Box::new(deployed), is_draft: false, draft_saved_at: None, + draft_id: None, draft_base: None, no_deployed: false, draft: None, @@ -386,6 +392,7 @@ where inner: Box::new(deployed), is_draft: true, draft_saved_at: Some(row.created_at), + draft_id: Some(row.id), draft_base: row.base, no_deployed: false, draft: Some(draft_json), @@ -665,7 +672,7 @@ pub async fn fetch_draft_only( ) -> Result> { // Own draft first, legacy NULL-email row as fallback (see `maybe_overlay_draft`). let row = sqlx::query!( - r#"SELECT value as "value!: sqlx::types::Json>", + r#"SELECT id, value as "value!: sqlx::types::Json>", created_at, base FROM draft WHERE workspace_id = $1 @@ -697,6 +704,7 @@ pub async fn fetch_draft_only( inner: Box::new(draft_json.clone()), is_draft: true, draft_saved_at: Some(row.created_at), + draft_id: Some(row.id), draft_base: row.base, no_deployed: true, draft: Some(draft_json), diff --git a/frontend/src/lib/components/ScriptBuilder.svelte b/frontend/src/lib/components/ScriptBuilder.svelte index 8f81a36d71..7be89beafd 100644 --- a/frontend/src/lib/components/ScriptBuilder.svelte +++ b/frontend/src/lib/components/ScriptBuilder.svelte @@ -786,7 +786,7 @@ // + `tick()` first so the last keystrokes reach the bindable before the // syncer flushes. No toast — the AutosaveIndicator narrates the result, and // `flush` never rejects (postSave routes errors to the failures map). - async function saveDraft(): Promise { + export async function saveDraft(): Promise { if (!opWorkspace || !userDraftPath) return editor?.flushPendingChanges() await tick() diff --git a/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte b/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte index 816f4e8634..5b3fcff72f 100644 --- a/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte +++ b/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte @@ -2,7 +2,6 @@ /** * The draft modals every editor route mounts at its trailer: * - DraftSyncConflictModal: surfaces a 409 from the autosave pipeline. - * - DraftMovedModal: the item was moved away from this path mid-edit. * - OtherUsersDraftsModal: other users' drafts at this path, for forking. * - StaleDraftModal: prompts when the user's draft predates the latest * deploy; open-state is computed here from the route's timestamps. @@ -13,8 +12,11 @@ */ import type { UserDraftItemKind } from '$lib/gen' import DraftSyncConflictModal from './DraftSyncConflictModal.svelte' - import DraftMovedModal from './DraftMovedModal.svelte' import OtherUsersDraftsModal, { type OtherDraftUser } from './OtherUsersDraftsModal.svelte' + import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' + import { sendUserToast } from '$lib/toast' + import { base } from '$app/paths' + import { goto } from '$app/navigation' import StaleDraftModal from './StaleDraftModal.svelte' import ConfirmationModal from './ConfirmationModal.svelte' import { OtherUserDraftLoad } from '$lib/components/otherUserDraftLoad.svelte' @@ -54,6 +56,11 @@ * choice between keeping and discarding is informed. Omit where the editor * has no diff drawer; the action is then not rendered. */ onViewDiff?: () => void | Promise + /** Runs before this editor follows its draft to the item's new path: the + * editor's own draft save, which materializes text the code editor still + * holds. Without it, keystrokes typed since the relocating save are lost + * to the navigation. */ + onBeforeRelocate?: () => void | Promise /** Defaults to true; set to false to suppress all modals. */ enabled?: boolean } @@ -74,6 +81,7 @@ deployedHeadVersion = undefined, onLoadLatestDeploy, onViewDiff, + onBeforeRelocate, enabled = true }: Props = $props() @@ -113,6 +121,30 @@ } }) }) + + const EDITOR_SEGMENT: Partial> = { + script: 'scripts/edit', + flow: 'flows/edit', + app: 'apps/edit', + raw_app: 'apps_raw/edit' + } + + // The item was moved while this editor was open: the draft row followed it + // and the save just landed there. Follow it too — the route reloads the item + // at its new path, and the stale prompt above then says what changed. Edits + // typed since that save are flushed first, so leaving this path drops none. + $effect(() => { + if (!enabled || !workspace || !path) return + const seg = EDITOR_SEGMENT[itemKind] + if (!seg) return + const query = { workspace, itemKind, path } + return UserDraftDbSyncer.onRelocated(query, async (newPath) => { + await onBeforeRelocate?.() + await UserDraftDbSyncer.flush(query) + sendUserToast(`This item was moved to ${newPath}. You are now editing it there.`) + await goto(`${base}/${seg}/${newPath}`) + }) + }) {#if enabled && workspace && path} @@ -121,7 +153,6 @@ {onLoadFromServer} {getLocalDraft} /> - {#if otherDraftsUsers.length > 0} {#key path} - /** - * Surfaces the "moved" verdict left by `UserDraftDbSyncer.postSave`: someone - * moved this item while the editor was open, so the server refused the - * autosave rather than plant a phantom draft-only item at the path the item - * has left. - * - * Continuing pushes the current in-memory draft to the new path (force, since - * the draft carried over by the move is older) and follows it there, so edits - * made after the move aren't lost to the relocation. - */ - import { base } from '$app/paths' - import { goto } from '$app/navigation' - import { UserDraftDbSyncer, type UserDraftLastSyncQuery } from '$lib/userDraftDbSyncer.svelte' - import Modal2 from '$lib/components/common/modal/Modal2.svelte' - import Button from '$lib/components/common/button/Button.svelte' - import Alert from '$lib/components/common/alert/Alert.svelte' - import { FolderInput } from 'lucide-svelte' - - type Props = { - query: UserDraftLastSyncQuery - /** Current local draft value, re-pointed at the new path before it is - * pushed there. */ - getLocalDraft: () => unknown - } - - let { query, getLocalDraft }: Props = $props() - - const moveHandle = $derived(UserDraftDbSyncer.getMove(query)) - let isOpen = $derived(moveHandle.move !== undefined) - let busy = $state(false) - - const EDITOR_SEGMENT: Partial> = { - script: 'scripts/edit', - flow: 'flows/edit', - app: 'apps/edit', - raw_app: 'apps_raw/edit' - } - - /** Applies the server's `moved_patch` — the typed target path plus the version - * the item now sits at. Both come from the server precisely so this file - * doesn't reproduce `typed_path_field` / `base_version_field`; re-pointing the - * path without the version restamp would land the draft at the new path still - * claiming the pre-move version, and greet the user with a stale-draft prompt - * offering to discard the edits they just chose to carry. */ - function repointed(value: unknown, patch: Record | undefined): unknown { - if (value == undefined || typeof value !== 'object' || patch == undefined) return value - return { ...(value as Record), ...patch } - } - - // Tagged with the destination it was raised for, because this component is - // mounted for the editor's lifetime rather than per prompt: an untagged error - // would still be rendered when the next move verdict opens the modal. Tagging - // also survives the A→B→C case, where we deliberately re-point the verdict and - // then raise an error about the new destination. - let carryError = $state<{ title: string; detail: string; forMovedTo: string } | undefined>( - undefined - ) - const shownError = $derived( - carryError && carryError.forMovedTo === moveHandle.move?.movedTo ? carryError : undefined - ) - - async function continueThere() { - const move = moveHandle.move - if (!move) return - busy = true - carryError = undefined - try { - const local = getLocalDraft() - if (local != undefined) { - const target = { workspace: query.workspace, itemKind: query.itemKind, path: move.movedTo } - await UserDraftDbSyncer.overwrite({ ...target, value: repointed(local, move.patch) }) - // `overwrite` resolves whether or not the write landed — a network - // failure and a second move both park state instead of throwing. Leaving - // here regardless would drop the editor's edits on the floor and, for an - // A→B→C move, land on a B that no longer holds the item. Stay put and say - // so; the draft is still in this editor, so the user can retry. - const failed = UserDraftDbSyncer.getState(target).failureMessage - const movedAgain = UserDraftDbSyncer.getMove(target).move - if (movedAgain) { - // It moved again while we were carrying (A→B→C). Re-point this - // editor's own move record at C so the modal now offers C and a - // retry makes progress — without this the retry would keep - // overwriting at B, be refused again, and loop with no way out. - UserDraftDbSyncer.recordMove(query, movedAgain) - carryError = { - title: 'It moved again while saving', - detail: `It is now at ${movedAgain.movedTo}. Your edits are still in this editor — continue to follow it there.`, - forMovedTo: movedAgain.movedTo - } - return - } - if (failed) { - carryError = { - title: 'Could not save at the new path', - detail: `${failed.replace(/\.?$/, '.')} Your edits are still in this editor.`, - forMovedTo: move.movedTo - } - return - } - } - UserDraftDbSyncer.clearMove(query) - const seg = EDITOR_SEGMENT[query.itemKind] - if (seg) await goto(`${base}/${seg}/${move.movedTo}`) - } finally { - busy = false - } - } - - - -
-
- -
-

- {#if moveHandle.move?.movedBy} - {moveHandle.move.movedBy} moved this to - {:else} - This was moved to - {/if} - {moveHandle.move?.movedTo}. Nothing was saved here - — this path no longer holds the item. -

-

- Continuing takes your current edits to the new path, replacing any draft already there. - Staying here leaves them unsaved. -

- {#if shownError} - {shownError.detail} - {/if} -
-
- -
- - -
-
- diff --git a/frontend/src/lib/components/usePageDraftSync.svelte.ts b/frontend/src/lib/components/usePageDraftSync.svelte.ts index 8ca2b0beee..cbf3ea7702 100644 --- a/frontend/src/lib/components/usePageDraftSync.svelte.ts +++ b/frontend/src/lib/components/usePageDraftSync.svelte.ts @@ -47,8 +47,10 @@ export interface PageDraftSync { seedBaseline(value: V): void /** After a backend load, record the server's `draft_saved_at` so the * next autosave attaches a matching `last_sync` and the server can - * reject stale writes. `undefined` clears it (no draft existed). */ - recordRemoteSync(draftSavedAt: string | undefined): void + * reject stale writes. `undefined` clears it (no draft existed). The + * row's `draft_id` makes later saves address the row, so they follow a + * move. */ + recordRemoteSync(draftSavedAt: string | undefined, draftId?: number): void /** Drop the draft (server row + local cell) — restore-to-deployed and * post-deploy cleanup. */ remove(): void @@ -99,14 +101,15 @@ export function usePageDraftSync(opts: PageDraftSyncOptions): Pa if (!ws || !p) return UserDraft.seed(opts.itemKind, p, value, { workspace: ws }) }, - recordRemoteSync(draftSavedAt: string | undefined) { + recordRemoteSync(draftSavedAt: string | undefined, draftId?: number) { const ws = opts.workspace() const p = opts.path() if (!ws || !p) return untrack(() => UserDraftDbSyncer.recordRemoteSync( { workspace: ws, itemKind: opts.itemKind, path: p }, - draftSavedAt + draftSavedAt, + draftId ) ) }, diff --git a/frontend/src/lib/userDraftDbMigration.ts b/frontend/src/lib/userDraftDbMigration.ts index 0868876e06..1496e595cb 100644 --- a/frontend/src/lib/userDraftDbMigration.ts +++ b/frontend/src/lib/userDraftDbMigration.ts @@ -262,36 +262,6 @@ export async function migrateUserDraftsToDb(): Promise { path, requestBody: { value, last_sync: writtenAt, created_at: writtenAt } }) - if (res.status === 'moved' && res.moved_to) { - // Nothing was written: the item left this path. Retrying here would - // never resolve — `resolve_moved_to` answers from the item's lineage, - // so the old path reports `moved` for as long as the item exists — and - // dropping the entry would destroy the only copy there is. Follow the - // item instead, applying the patch the server just handed us so the - // draft lands pointing at where it now lives. - const moved = await DraftService.updateDraft({ - workspace: parsed.workspace, - kind: parsed.itemKind, - path: res.moved_to, - requestBody: { - value: { ...(value as object), ...((res.moved_patch as object) ?? {}) }, - last_sync: writtenAt, - created_at: writtenAt - } - }) - if (moved.status !== 'saved' && moved.status !== 'conflict') { - // Still not landed. Surface it rather than looping: the modal is the - // user's only way to see or discard an un-migratable draft. - reportDraftMigrationError({ - key, - path, - workspace: parsed.workspace, - itemKind: parsed.itemKind, - value - }) - continue - } - } if (res.status === 'conflict') { console.info( `UserDraft LS→DB migration: server draft for ${path} is fresher, dropping LS copy` diff --git a/frontend/src/lib/userDraftDbSyncer.svelte.ts b/frontend/src/lib/userDraftDbSyncer.svelte.ts index 558afc3b82..a7e3bf6297 100644 --- a/frontend/src/lib/userDraftDbSyncer.svelte.ts +++ b/frontend/src/lib/userDraftDbSyncer.svelte.ts @@ -129,17 +129,6 @@ export type DraftConflictInfo = { localLastSync: string | null } -/** Where an item went after someone moved it, as reported by a refused save. */ -export type DraftMovedInfo = { - movedTo: string - movedBy: string | undefined - /** Fields the server says to merge into the draft before re-saving it at - * `movedTo` — the typed target path and the version the item now sits at. - * The server owns these key names (`UserDraftItemKind::typed_path_field` / - * `base_version_field`), so nothing here reproduces them. */ - patch: Record | undefined -} - export type UserDraftLastSyncQuery = { workspace: string itemKind: UserDraftItemKind @@ -226,12 +215,18 @@ const syncLocked = new Map void) | undefined>() const conflicts = new SvelteMap() /** - * Keys whose item was MOVED out from under an editor still bound to the old - * path. The server refuses the write (saving would plant a phantom draft-only - * item where the item no longer is) and answers with where it went; read via - * `getMove(query)` to prompt the user over there. + * The server row id per key, learned from the load (`draft_id`) or the first + * save. Sent with every later save so the write addresses the row wherever a + * move took it, rather than the path this editor was opened on. */ -const moves = new SvelteMap() +const draftIds = new Map() + +/** + * Per-key listeners fired when a save lands at a path other than the key's: + * the item was moved while the editor was open, and the row followed it. The + * editor follows too (see `onRelocated`). + */ +const relocationListeners = new Map void>>() /** * Draft keys whose last save threw (network / 5xx) → extracted error @@ -310,21 +305,10 @@ async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { // `last_sync` (first-ever save) hits the backend's "treat as // fresh" branch. last_sync: opts.force ? undefined : lastSync, - force: opts.force ?? false + force: opts.force ?? false, + id: draftIds.get(key) } }) - if (resp.status === 'moved') { - // Nothing was written. Like a conflict, `lastSync` stays put so the - // state survives every retry until the user acts on it. - if (resp.moved_to) { - moves.set(key, { - movedTo: resp.moved_to, - movedBy: resp.moved_by, - patch: resp.moved_patch as Record | undefined - }) - } - return - } if (resp.status === 'conflict') { // Someone advanced the row past our `last_sync`. Park the // snapshot for the UI; do NOT touch `lastSync` — the next save @@ -339,8 +323,10 @@ async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { // resp.status === 'saved' — advance lastSync (or drop on delete). if (opts.value === null) { clearLastSync(opts.workspace, opts.itemKind, opts.path) + draftIds.delete(key) } else { setLastSync(opts.workspace, opts.itemKind, opts.path, resp.current_timestamp) + if (resp.id != null) draftIds.set(key, resp.id) } // postSave is the only place a draft's server-side existence changes, // so it's the single source for the list pages' `*` hint @@ -348,7 +334,6 @@ async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { // free instead of maintaining a separate source of truth. setLocalDraftHint(opts.workspace, opts.itemKind, opts.path, opts.value !== null) conflicts.delete(key) - moves.delete(key) failures.delete(key) // Clear pending only if it's still the opts we just saved — a // newer `save()` that arrived during the POST replaces the entry @@ -365,6 +350,12 @@ async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { // cached state the same way an upsert does. Listener errors must never // make a committed save read as failed. notifyAnySaved({ workspace: opts.workspace, itemKind: opts.itemKind, path: opts.path }) + // The row was carried elsewhere by a move and the save followed it. Last, + // so the editor that reacts (by leaving this path) sees a settled key. + if (opts.value !== null && resp.path && resp.path !== opts.path) { + const listeners = relocationListeners.get(key) + if (listeners) for (const l of [...listeners]) l(resp.path) + } } catch (e) { console.error('UserDraftDbSyncer.save failed', e) // Leave pending opts in place so the next attempt retries the same @@ -558,18 +549,28 @@ export const UserDraftDbSyncer = { * Seed the per-tab `last_sync` after an editor reads a draft from the * server. Pass the response's `draft_saved_at` so the next save sends a * matching `last_sync`; pass `undefined` when no draft existed (next - * save omits `last_sync`, the backend's first-push branch). + * save omits `last_sync`, the backend's first-push branch). `draftId` is + * the row's id from the same response; with it the next save addresses + * the row rather than the path, so it follows a move. */ - recordRemoteSync(query: UserDraftLastSyncQuery, draftSavedAt: string | undefined): void { + recordRemoteSync( + query: UserDraftLastSyncQuery, + draftSavedAt: string | undefined, + draftId?: number + ): void { const key = draftKey(query.workspace, query.itemKind, query.path) if (draftSavedAt) { setLastSync(query.workspace, query.itemKind, query.path, draftSavedAt) } else { clearLastSync(query.workspace, query.itemKind, query.path) } + if (draftId != null) { + draftIds.set(key, draftId) + } else { + draftIds.delete(key) + } // Back in sync with the server: clear any conflict / failure. conflicts.delete(key) - moves.delete(key) failures.delete(key) }, @@ -647,27 +648,26 @@ export const UserDraftDbSyncer = { conflicts.delete(draftKey(query.workspace, query.itemKind, query.path)) }, - /** Reactive "the item moved away from this path" snapshot, if any. */ - getMove(query: UserDraftLastSyncQuery): { - readonly move: DraftMovedInfo | undefined - } { + /** + * Subscribe to saves for a draft key that landed at another path: the item + * was moved while this editor was open and the row followed it. Fired with + * the row's current path, after the save is fully accounted for. Returns an + * unsubscribe. + */ + onRelocated(query: UserDraftLastSyncQuery, listener: (newPath: string) => void): () => void { const key = draftKey(query.workspace, query.itemKind, query.path) - return { - get move() { - return moves.get(key) - } + let set = relocationListeners.get(key) + if (!set) { + set = new Set() + relocationListeners.set(key, set) + } + set.add(listener) + return () => { + const s = relocationListeners.get(key) + if (!s) return + s.delete(listener) + if (s.size === 0) relocationListeners.delete(key) } - }, - - clearMove(query: UserDraftLastSyncQuery): void { - moves.delete(draftKey(query.workspace, query.itemKind, query.path)) - }, - - /** Re-point a key's move verdict, for an item that moved again while its - * carry was in flight (A→B→C). Without this the prompt keeps naming B, and - * every retry is refused for the same reason. */ - recordMove(query: UserDraftLastSyncQuery, move: DraftMovedInfo): void { - moves.set(draftKey(query.workspace, query.itemKind, query.path), move) }, /** diff --git a/frontend/src/routes/(root)/(logged)/apps/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/apps/edit/[...path]/+page.svelte index d3c8155a3c..998de1f657 100644 --- a/frontend/src/routes/(root)/(logged)/apps/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/apps/edit/[...path]/+page.svelte @@ -240,7 +240,8 @@ if ($workspaceStore && path) { UserDraftDbSyncer.recordRemoteSync( { workspace: $workspaceStore, itemKind: 'app', path }, - backendApp.draft_saved_at + backendApp.draft_saved_at, + backendApp.draft_id ) } // The app autosave stores a raw `App`, but this loader (and AppEditor's diff --git a/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte index ed1e86c508..fe9018d955 100644 --- a/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte @@ -302,7 +302,10 @@ if (getDraft) { otherDraftsUsers = (backendApp.other_drafts_users ?? []) as OtherDraftUser[] } - draftSync.recordRemoteSync(backendApp.draft_saved_at as string | undefined) + draftSync.recordRemoteSync( + backendApp.draft_saved_at as string | undefined, + backendApp.draft_id as number | undefined + ) isNewApp = !!backendApp.no_deployed // Per-response, NOT sticky: a later no-own-draft load in the same editor // must reset this so it can't wrongly force overlay mode. diff --git a/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte index 5027f5fd52..de0b831453 100644 --- a/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte @@ -358,7 +358,10 @@ if (getDraft) { otherDraftsUsers = (backendFlow.other_drafts_users ?? []) as OtherDraftUser[] } - draftSync.recordRemoteSync(backendFlow.draft_saved_at as string | undefined) + draftSync.recordRemoteSync( + backendFlow.draft_saved_at as string | undefined, + backendFlow.draft_id + ) // Re-evaluate per load: true for draft-only paths, false once deployed. isNewFlow = !!backendFlow.no_deployed // Per-response, NOT sticky: a later no-own-draft load in the same editor @@ -522,6 +525,7 @@ {draftBaseVersion} deployedHeadVersion={version != null ? String(version) : undefined} onViewDiff={() => flowBuilder?.openDiffDrawer()} + onBeforeRelocate={() => flowBuilder?.saveDraft()} onLoadLatestDeploy={async () => { // stopSync-bracketed; see /scripts/edit's restoreDeployed for the race. if (!$workspaceStore) return diff --git a/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte index a404dbe9e8..98fc9227c2 100644 --- a/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte @@ -317,7 +317,10 @@ // Seed the per-tab `last_sync` so the next autosave attaches a matching // timestamp the backend can stale-check. `undefined` (no draft) clears // it, making the next save take the "first push" branch. - draftSync.recordRemoteSync(backendScript.draft_saved_at as string | undefined) + draftSync.recordRemoteSync( + backendScript.draft_saved_at as string | undefined, + backendScript.draft_id + ) // Per-response, NOT sticky: navigating to another path in the same editor // must reset this, else a later no-own-draft load wrongly enters overlay. const hasOwnDraft = !!backendScript.is_draft @@ -483,6 +486,7 @@ draftBaseVersion={draftBaseHash} deployedHeadVersion={deployedHeadHash} onViewDiff={() => scriptBuilder?.openDiffDrawer()} + onBeforeRelocate={() => scriptBuilder?.saveDraft()} onLoadLatestDeploy={async () => { // stopSync-bracketed; see restoreDeployed for the race. if (!$workspaceStore) return