From 46f27cdc6ab9f66c3abf30b1d73c5f608cc311c3 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Thu, 10 Sep 2026 16:59:04 -0700 Subject: [PATCH] fix(workflow): a plain save no longer clobbers is_public Since #8125 the persist endpoint wrote is_public from the request. The frontend feeds the saved row back as its metadata, and that row names the flag isPublic while the rest of the frontend calls it isPublished, so the very next autosave went out without the flag, the update wrote NULL into a NOT NULL column, and every second save failed with 500 (silently on the canvas, "Could not save" on the Form View). A stale false on a save could likewise un-publish a workflow. persist now writes name, description and content only; publishing stays with /public and /private, and default_view with /set-default-view. The frontend stops sending the flag on a save, and parseWorkflowInfo carries a persist response's isPublic over to isPublished so metadata fed back from a save keeps the publish state. Closes #8496. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../user/workflow/WorkflowResource.scala | 12 +++--- .../dashboard/file/WorkflowResourceSpec.scala | 40 +++++++++++++++++-- .../workflow-persist.service.spec.ts | 10 +++-- .../workflow-persist.service.ts | 5 ++- .../util/workflow-util.service.spec.ts | 15 +++++++ .../util/workflow-util.service.ts | 8 ++++ 6 files changed, 77 insertions(+), 13 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala index b8bead4b0ea..17cd7a11fad 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala @@ -489,10 +489,13 @@ class WorkflowResource extends LazyLogging { } /** - * Persists a plain save by updating only the fields the client sends - * (name/description/content/is_public). It deliberately leaves `default_view` untouched -- - * that column is owned by /set-default-view alone -- so a save can never clobber a - * concurrent change. Timestamps are likewise not rewritten here. + * Persists a plain save by updating only what a save is: name, description and content. + * `is_public` is not written here. Publishing has its own endpoints (/public, /private), and + * a save payload does not reliably carry the flag: the frontend feeds the saved row straight + * back as its metadata, where the flag has another name, so the very next autosave arrives + * without it. Writing that null violated the column's NOT NULL constraint and every second + * save failed with 500. `default_view` is likewise owned by /set-default-view alone, and the + * timestamps are not rewritten here, so a save can never clobber a concurrent change to either. */ private def saveWorkflowFields(workflow: Workflow): Unit = { context @@ -500,7 +503,6 @@ class WorkflowResource extends LazyLogging { .set(WORKFLOW.NAME, workflow.getName) .set(WORKFLOW.DESCRIPTION, workflow.getDescription) .set(WORKFLOW.CONTENT, workflow.getContent) - .set(WORKFLOW.IS_PUBLIC, workflow.getIsPublic) .where(WORKFLOW.WID.eq(workflow.getWid)) .execute() } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala index 88f429a3e69..d197808d00e 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala @@ -1340,9 +1340,9 @@ class WorkflowResourceSpec assert(defaultView(wid) == DefaultViewEnum.CANVAS) } - // A plain save (persistWorkflow) only writes the fields the client sends -- name, - // description, content, is_public -- and never `default_view`, so saving the canvas must - // not reset the default view. The edit payload mirrors what the frontend sends. + // A plain save (persistWorkflow) only writes name, description and content -- never + // `default_view`, so saving the canvas must not reset the default view. The edit payload mirrors + // what the frontend sends. it should "survive a subsequent save of the workflow" in { val wid = persistFreshWorkflow("param_survives_save") workflowResource.setDefaultView(wid, DefaultViewRequest("FORM"), sessionUser1) @@ -1351,7 +1351,6 @@ class WorkflowResourceSpec edit.setWid(wid) edit.setName("param_survives_save_edited") edit.setContent("{\"operators\":[],\"links\":[]}") - edit.setIsPublic(false) workflowResource.persistWorkflow(edit, sessionUser1) assert( @@ -1360,6 +1359,39 @@ class WorkflowResourceSpec ) } + // The frontend feeds the saved row back as its metadata, where the publish flag has another + // name, so the very next autosave arrives with isPublic unset. A save must neither fail on that + // (the column is NOT NULL, and writing the null made every second save a 500) nor rewrite the + // flag: publishing is /public and /private's job alone. + "/persist API" should "neither fail nor change is_public when the save carries no flag" in { + val wid = persistFreshWorkflow("persist_keeps_public") + workflowResource.makePublic(wid, sessionUser1) + + val edit = new Workflow() + edit.setWid(wid) + edit.setName("persist_keeps_public_edited") + edit.setContent("{\"operators\":[],\"links\":[]}") + // isPublic deliberately left null, exactly as the frontend's second save sends it + val saved = workflowResource.persistWorkflow(edit, sessionUser1) + + assert(saved.getName == "persist_keeps_public_edited") + assert(saved.getIsPublic, "a plain save must not touch the publish flag") + } + + it should "not un-publish a workflow when the save says isPublic = false" in { + val wid = persistFreshWorkflow("persist_ignores_flag") + workflowResource.makePublic(wid, sessionUser1) + + val edit = new Workflow() + edit.setWid(wid) + edit.setName("persist_ignores_flag_edited") + edit.setContent("{\"operators\":[],\"links\":[]}") + edit.setIsPublic(false) + val saved = workflowResource.persistWorkflow(edit, sessionUser1) + + assert(saved.getIsPublic, "a stale flag on a save must not un-publish the workflow") + } + // A biologist's path is hub -> clone -> use, so a copy has to stay usable. it should "be inherited by a duplicated workflow" in { val wid = persistFreshWorkflow("param_source") diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts index a5e4463037d..1ae30d331cf 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts +++ b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts @@ -202,19 +202,23 @@ describe("WorkflowPersistService", () => { const req = httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`); expect(req.request.method).toBe("POST"); + // The publish flag is not part of a save: the endpoint does not read it, and sending a + // stale copy is what used to null the column after the first save. expect(req.request.body).toEqual({ wid: 9, name: "my wf", description: "a description", content: JSON.stringify(validContent), - isPublic: true, }); - req.flush({ wid: 9, name: "my wf", content: '{"operators":[]}' }); + // The saved row comes back with the flag under the backend's name; the response the + // caller sees carries it as isPublished, so metadata fed back from a save stays complete. + req.flush({ wid: 9, name: "my wf", content: '{"operators":[]}', isPublic: true }); // valid workflow -> no error notification, and string content is parsed expect(errorSpy).not.toHaveBeenCalled(); expect(result?.content).toEqual({ operators: [] }); + expect(result?.isPublished).toBe(1); }); it("persistWorkflow notifies the user when the workflow is broken but still POSTs", () => { @@ -235,7 +239,7 @@ describe("WorkflowPersistService", () => { ); const req = httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`); - expect(req.request.body.isPublic).toBe(false); + expect("isPublic" in req.request.body).toBe(false); req.flush({ wid: 1, name: "broken", content: '{"operators":[]}' }); }); diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts index 8e2203addd9..9b8f4741bd1 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts +++ b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts @@ -75,13 +75,16 @@ export class WorkflowPersistService { ); } + // A save carries name, description and content only. The publish flag is not sent: the + // backend does not read it on this endpoint (publishing goes through /public and /private), + // and it is not reliably known here anyway, since the metadata fed back after a save names + // it differently (see WorkflowUtilService.parseWorkflowInfo). return this.http .post(`${AppSettings.getApiEndpoint()}/${WORKFLOW_PERSIST_URL}`, { wid: workflow.wid, name: workflow.name, description: workflow.description, content: JSON.stringify(workflow.content), - isPublic: workflow.isPublished, }) .pipe( filter((updatedWorkflow: Workflow) => updatedWorkflow != null), diff --git a/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.spec.ts b/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.spec.ts index 4146e0704ad..61c04a0825e 100644 --- a/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.spec.ts @@ -219,6 +219,21 @@ describe("WorkflowUtilService", () => { expect(parsed.content).toBe(content); }); + // The persist endpoint returns the stored row, which names the publish flag isPublic; the rest + // of the frontend knows it as isPublished. Without the carry-over, a save fed back as metadata + // lost the flag, and the next save went out without it. + it("should carry the persist response's isPublic over to isPublished", () => { + const fromPersist = { wid: 1, name: "n", content: "{}", isPublic: true } as unknown as Workflow; + + expect(WorkflowUtilService.parseWorkflowInfo(fromPersist).isPublished).toBe(1); + }); + + it("should leave an isPublished the payload already carries alone", () => { + const fromRetrieve = { wid: 1, name: "n", content: "{}", isPublished: 0, isPublic: true } as unknown as Workflow; + + expect(WorkflowUtilService.parseWorkflowInfo(fromRetrieve).isPublished).toBe(0); + }); + it("should create a fresh comment box at the default position", () => { const commentBox = workflowUtilService.getNewCommentBox(); diff --git a/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.ts b/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.ts index 64681965b42..0760b8841d9 100644 --- a/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.ts +++ b/frontend/src/app/workspace/service/workflow-graph/util/workflow-util.service.ts @@ -182,6 +182,14 @@ export class WorkflowUtilService { if (workflow != null && typeof workflow.content === "string") { workflow.content = jsonCast(workflow.content); } + // The persist endpoint answers with the stored row, whose publish flag is named isPublic; + // every other workflow endpoint, and the Workflow type, call it isPublished. Carry it across, + // or the metadata a save feeds back would silently drop the publish state until the next + // full load. + const stored = workflow as Workflow & { isPublic?: boolean }; + if (workflow != null && workflow.isPublished === undefined && stored.isPublic !== undefined) { + workflow.isPublished = Number(stored.isPublic); + } return workflow; }