This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7611-6e56294657faaa5fd1271b093460568b7b41c415 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 310ab88e4c78da14182284199bde34a1d22d489b Author: Eugene Gu <[email protected]> AuthorDate: Thu Aug 13 04:06:19 2026 +0000 chore(frontend): downloading workflows as a ZIP no longer saves each one individually (#7611) ### What changes were proposed in this PR? Selecting several workflows and using the toolbar's "Download added workflow as a ZIP file" action saved one loose `.json` file per selected workflow **in addition to** the archive, so N selected workflows produced N+1 downloads. Root cause is in `frontend/src/app/dashboard/service/user/download/download.service.ts`. `downloadWorkflow(id, name)` both retrieves a workflow and saves it to disk — the save is the `tap(this.saveFile.bind(this))` at the end of the pipe — which is exactly what the per-row download action needs. `createWorkflowsZip` reused that same method purely to obtain each blob, so the save fired for every entry before the blob was added to the archive. This is a regression rather than intended behaviour: before #2920 (`57984370c`, "Refactor Frontend to Centralize Downloads Using DownloadService") the bulk path assembled the zip inline and called `saveAs` exactly once. That refactor moved the logic into `DownloadService` and reused `downloadWorkflow` for retrieval, inheriting its save side effect. The fix splits retrieval from saving: - new private `retrieveWorkflowItem(id, name)` returns the `DownloadableItem` (blob + file name) **without** saving — it is the former body of `downloadWorkflow` minus the `tap`; - `downloadWorkflow` is now `retrieveWorkflowItem(...).pipe(tap(this.saveFile.bind(this)))`, i.e. behaviour is unchanged for the three per-row callers (`user-workflow-list-item.component.ts`, `list-item.component.ts`, `card-item.component.ts`), which subscribe without a value handler and rely solely on that side effect; - `createWorkflowsZip` calls `retrieveWorkflowItem` directly. `A.pipe(map, tap)` and `A.pipe(map).pipe(tap)` compose the same chain, so the emitted value, timing, subscription semantics and error propagation of `downloadWorkflow` are unchanged. The public API is untouched and no call site needed updating. **Before** — three workflows selected, then "Download as ZIP": the archive plus `test1.json`, `test2.json`, `test3.json`, four files in total. <img width="1493" height="755" alt="Screenshot 2026-08-12 at 3 48 45 PM" src="https://github.com/user-attachments/assets/7a7b8cb0-330a-452f-b853-b7aabfc04acd"; /> **After** — same three workflows, same action: only `workflowExports-*.zip`. <img width="1482" height="750" alt="Screenshot 2026-08-12 at 4 43 38 PM" src="https://github.com/user-attachments/assets/bdc64245-91bc-4fa0-a06d-2b80e2871706"; /> ### Any related issues, documentation, discussions? Closes #7608 ### How was this PR tested? Seven cases were added to `frontend/src/app/dashboard/service/user/download/download.service.spec.ts` (23 → 30). Three of them fail on `main` and pass with this change. Pinning the fix: - `saves only the zip, not one JSON per workflow, when several workflows are zipped` — three workflows, asserts `saveAs` is called exactly once with the archive, and that no `Alpha.json` / `Beta.json` / `Gamma.json` was saved - `saves only the zip for a single-workflow selection` — the N=1 boundary, which a plain "one extra file" check would miss Guarding the other direction, so the bug cannot be "fixed" by deleting the save: - `still saves the file when a single workflow is downloaded on its own` — passes before and after; it fails if `downloadWorkflow` stops saving, which would break the per-row download action Edge cases: - `saves nothing when one of the workflows fails to retrieve` — the archive aborts as a whole, so the workflows that did come back must not be left behind as loose files (previously they were already saved by the time `forkJoin` errored) - `writes the workflow content into the zip entries` — reads an entry back out of the produced archive and parses it, since the pre-existing tests only asserted entry *names* - `does not save anything when the standalone workflow download fails` — error propagates, nothing is written; mirrors the existing dataset / single-file error cases - `saves nothing for an empty selection` — `forkJoin([])` completes without emitting, so nothing is retrieved and no empty archive is written (the toolbar already guards this case) ``` cd frontend node --max-old-space-size=8192 ./node_modules/@angular/cli/bin/ng test --watch=false \ --include="src/app/dashboard/service/user/download/download.service.spec.ts" ``` `Test Files 1 passed (1)` / `Tests 30 passed (30)`. Reverting only the source change makes exactly three of them fail. The specs of the three components that depend on `downloadWorkflow` still saving were also run and pass unchanged (`list-item`, `card-item`, `user-workflow-list-item`). Manually verified against a local stack, as shown in the screenshots above: create three workflows, select them, click the ZIP download action, and compare the browser's download list before and after the change. ### Was this PR authored or co-authored using generative AI tooling? Co-authored by: Claude Code (Claude Opus 5) --- .../service/user/download/download.service.spec.ts | 106 ++++++++++++++++++++- .../service/user/download/download.service.ts | 27 ++++-- 2 files changed, 122 insertions(+), 11 deletions(-) diff --git a/frontend/src/app/dashboard/service/user/download/download.service.spec.ts b/frontend/src/app/dashboard/service/user/download/download.service.spec.ts index 7bc180e737..0cbdf9ee57 100644 --- a/frontend/src/app/dashboard/service/user/download/download.service.spec.ts +++ b/frontend/src/app/dashboard/service/user/download/download.service.spec.ts @@ -227,7 +227,7 @@ describe("DownloadService", () => { // ─── createWorkflowsZip / nameWorkflow (real zip assembly) ──────────────── // These drive downloadWorkflowsAsZip through the real (un-mocked) private - // createWorkflowsZip → downloadWorkflow → nameWorkflow chain, so the produced + // createWorkflowsZip → retrieveWorkflowItem → nameWorkflow chain, so the produced // blob is a genuine zip we can load back and inspect. it("assembles a real zip with one JSON entry per workflow", async () => { @@ -266,6 +266,110 @@ describe("DownloadService", () => { expect(Object.keys(loaded.files).sort()).toEqual(["Dup-1.json", "Dup-2.json", "Dup.json"]); }); + // ─── zip download must not also save each workflow individually ─────────── + // Regression: createWorkflowsZip used to reuse downloadWorkflow purely to obtain + // the blob, and downloadWorkflow also saves to disk, so a zip of N workflows + // wrote N+1 files. + + it("saves only the zip, not one JSON per workflow, when several workflows are zipped", async () => { + workflowPersistServiceSpy.retrieveWorkflow.mockReturnValue(of({ content: { op: "x" } } as any)); + + const result = await firstValueFrom( + downloadService.downloadWorkflowsAsZip([ + { id: 1, name: "Alpha" }, + { id: 2, name: "Beta" }, + { id: 3, name: "Gamma" }, + ]) + ); + + expect(fileSaverServiceSpy.saveAs).toHaveBeenCalledTimes(1); + expect(fileSaverServiceSpy.saveAs).toHaveBeenCalledWith(result, expect.stringMatching(/^workflowExports-.*\.zip$/)); + + const savedNames = fileSaverServiceSpy.saveAs.mock.calls.map(([, fileName]) => fileName); + expect(savedNames).not.toContain("Alpha.json"); + expect(savedNames).not.toContain("Beta.json"); + expect(savedNames).not.toContain("Gamma.json"); + }); + + it("saves only the zip for a single-workflow selection", async () => { + workflowPersistServiceSpy.retrieveWorkflow.mockReturnValue(of({ content: { op: "x" } } as any)); + + await firstValueFrom(downloadService.downloadWorkflowsAsZip([{ id: 1, name: "Solo" }])); + + expect(fileSaverServiceSpy.saveAs).toHaveBeenCalledTimes(1); + expect(fileSaverServiceSpy.saveAs.mock.calls[0][1]).toMatch(/^workflowExports-.*\.zip$/); + }); + + // The other direction: the standalone single-workflow download action must + // keep saving, so the fix cannot simply drop the save from downloadWorkflow. + it("still saves the file when a single workflow is downloaded on its own", async () => { + workflowPersistServiceSpy.retrieveWorkflow.mockReturnValue(of({ content: { op: "x" } } as any)); + + const item = await firstValueFrom(downloadService.downloadWorkflow(42, "MyWorkflow")); + + expect(fileSaverServiceSpy.saveAs).toHaveBeenCalledTimes(1); + expect(fileSaverServiceSpy.saveAs).toHaveBeenCalledWith(item.blob, "MyWorkflow.json"); + }); + + it("saves nothing when one of the workflows fails to retrieve", async () => { + // The zip aborts as a whole, so the workflows that did come back must not be + // left behind as loose files. + workflowPersistServiceSpy.retrieveWorkflow.mockImplementation((id: number) => + id === 2 ? throwError(() => new Error("retrieve fail")) : (of({ content: { op: "x" } }) as any) + ); + + await expect( + firstValueFrom( + downloadService.downloadWorkflowsAsZip([ + { id: 1, name: "Alpha" }, + { id: 2, name: "Beta" }, + { id: 3, name: "Gamma" }, + ]) + ) + ).rejects.toThrow("retrieve fail"); + + expect(fileSaverServiceSpy.saveAs).not.toHaveBeenCalled(); + expect(notificationServiceSpy.error).toHaveBeenCalledWith("Error downloading workflows as ZIP"); + }); + + it("saves nothing for an empty selection", async () => { + // The toolbar never calls this with an empty selection, but forkJoin([]) + // completes without emitting, so the chain must end without writing an + // empty zip to disk. + let completed = false; + await new Promise<void>(resolve => + downloadService.downloadWorkflowsAsZip([]).subscribe({ + complete: () => { + completed = true; + resolve(); + }, + }) + ); + + expect(completed).toBe(true); + expect(fileSaverServiceSpy.saveAs).not.toHaveBeenCalled(); + expect(workflowPersistServiceSpy.retrieveWorkflow).not.toHaveBeenCalled(); + }); + + it("writes the workflow content into the zip entries", async () => { + workflowPersistServiceSpy.retrieveWorkflow.mockReturnValue(of({ content: { op: "x" } } as any)); + + const result = await firstValueFrom(downloadService.downloadWorkflowsAsZip([{ id: 1, name: "Alpha" }])); + + // Reading the entry back proves the blob handed to JSZip is the serialized + // workflow, not an empty or misrouted payload. + const loaded = await JSZip.loadAsync(result); + expect(JSON.parse(await loaded.files["Alpha.json"].async("string"))).toEqual({ op: "x" }); + }); + + it("does not save anything when the standalone workflow download fails", async () => { + workflowPersistServiceSpy.retrieveWorkflow.mockReturnValue(throwError(() => new Error("nope"))); + + await expect(firstValueFrom(downloadService.downloadWorkflow(42, "MyWorkflow"))).rejects.toThrow("nope"); + + expect(fileSaverServiceSpy.saveAs).not.toHaveBeenCalled(); + }); + // ─── downloadOperatorsResult ────────────────────────────────────────────── it("downloads a single operator file directly when there's exactly one file", async () => { diff --git a/frontend/src/app/dashboard/service/user/download/download.service.ts b/frontend/src/app/dashboard/service/user/download/download.service.ts index 2b7e5544e2..fb5d67fb70 100644 --- a/frontend/src/app/dashboard/service/user/download/download.service.ts +++ b/frontend/src/app/dashboard/service/user/download/download.service.ts @@ -62,15 +62,7 @@ export class DownloadService { ) {} downloadWorkflow(id: number, name: string): Observable<DownloadableItem> { - return this.workflowPersistService.retrieveWorkflow(id).pipe( - map(({ content }) => { - const workflowJson = JSON.stringify(content, null, 2); - const fileName = `${name}.json`; - const blob = new Blob([workflowJson], { type: "text/plain;charset=utf-8" }); - return { blob, fileName }; - }), - tap(this.saveFile.bind(this)) - ); + return this.retrieveWorkflowItem(id, name).pipe(tap(this.saveFile.bind(this))); } downloadDataset(id: number, name: string): Observable<Blob> { @@ -278,10 +270,25 @@ export class DownloadService { ); } + /** + * Builds the downloadable JSON item of a workflow without saving it, so callers that + * only need the blob (e.g. zip assembly) do not trigger a file save as a side effect. + */ + private retrieveWorkflowItem(id: number, name: string): Observable<DownloadableItem> { + return this.workflowPersistService.retrieveWorkflow(id).pipe( + map(({ content }) => { + const workflowJson = JSON.stringify(content, null, 2); + const fileName = `${name}.json`; + const blob = new Blob([workflowJson], { type: "text/plain;charset=utf-8" }); + return { blob, fileName }; + }) + ); + } + private createWorkflowsZip(workflowEntries: Array<{ id: number; name: string }>): Observable<Blob> { const zip = new JSZip(); const downloadObservables = workflowEntries.map(entry => - this.downloadWorkflow(entry.id, entry.name).pipe( + this.retrieveWorkflowItem(entry.id, entry.name).pipe( tap(({ blob, fileName }) => { zip.file(this.nameWorkflow(fileName, zip), blob); })
