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-7741-c927890160a8701c1b8ecb4fc4cc17711ea011ad in repository https://gitbox.apache.org/repos/asf/texera.git
commit ee06e4ba05f937f43041f7dc7faaca11d6759f9b Author: Ryan Zhang <[email protected]> AuthorDate: Wed Aug 19 22:55:59 2026 +0000 feat(notebook-migration): remove a workflow's notebook file from the Jupyter pod on delete (#7741) ### What changes were proposed in this PR? Removes a workflow's notebook file from the Jupyter pod when the notebook is deleted, so the pod's `work/` directory no longer accumulates notebooks for workflows the user has removed. Before #7671 this was self limiting: every workflow uploaded to the same `work/notebook.ipynb`, so there was only ever one file and it was overwritten on reuse. Now that each workflow uploads to `work/notebook_<wid>.ipynb`, the file survives both a notebook delete and a workflow delete, because those paths only touched the database. The database is authoritative for whether a notebook exists. The pod file is a per user artifact only the frontend can reach, since the notebook migration service targets one Jupyter per process under the per user pod model. So the file cleanup is best effort from the frontend after the authoritative database delete, and a failure is logged rather than surfaced. **`NotebookMigrationResource` (new `delete-notebook` endpoint)** - Adds `POST /notebook-migration/delete-notebook`, the counterpart to `set-notebook`: it takes a `notebookName`, validates it with the same `[A-Za-z0-9._-]+\.ipynb` pattern (blocking path traversal before any network call), and issues `DELETE /api/contents/work/<name>` against the Jupyter Contents API, bounded by a 2s connect and read timeout so a stalled pod cannot wedge the request thread. - A 204 or 200 reports `deleted: 1`. A 404 is treated as a no op with `deleted: 0`, so a workflow whose notebook was never uploaded still deletes cleanly, consistent with how the database delete reports `deleted: 0` when nothing was stored. Any other status is a 500. - Extracts the shared `jupyterUnavailableResponse` so the four endpoints that need a reachable Jupyter cannot drift in status or body. **`NotebookMigrationService` (frontend)** - Adds `deleteNotebookForWorkflow(wid)`, the single seam both delete paths call. It derives the filename from the wid, so the `notebook_<wid>.ipynb` convention lives only in the service that owns it, posts it to the new endpoint, and returns nothing: pod cleanup is best effort, so a failure is logged, not surfaced, and no caller acts on the outcome. The parameter is a concrete wid so it can never fall back to the shared default filename. **`JupyterPanelService` (panel delete button)** - `deleteJupyterNotebook()` captures the current wid up front (so a mid flight workflow switch cannot retarget the delete), then calls `deleteNotebookForWorkflow(wid)` after the database delete succeeds. The unsaved workflow path (wid undefined or the default 0) still resets local state only, since no file was ever uploaded for it. **`UserWorkflowComponent` (dashboard delete)** - Adds a private `cleanupNotebookFiles(wids)` that calls `deleteNotebookForWorkflow(wid)` per wid, and calls it from the success handler of both single delete (`deleteWorkflow`) and bulk delete (`handleConfirmDeleteSelectedWorkflows`). It runs only after the backend delete succeeds, so a failed delete leaves the pod file in place. A deleted workflow with no notebook produces a harmless 404. ### Any related issues, documentation, discussions? Closes #7737 Parent issue #4301 ### How was this PR tested? - `NotebookMigrationResourceSpec.scala`: `delete-notebook` issues a DELETE against the `work/<name>` contents path (verb and path pinned), reports `deleted=1` on 204 and on 200, treats 404 as `deleted=0`, returns 500 when Jupyter rejects the delete or is unreachable, and returns 400 on an invalid name, a missing or non string name, or a malformed body. - `notebook-migration.service.spec.ts`: `deleteNotebookForWorkflow` posts the wid-derived name to `delete-notebook`, swallows a transport failure without notifying, and makes no HTTP call when the feature flag is off. - `jupyter-panel.service.spec.ts`: the panel delete calls `deleteNotebookForWorkflow` with the current wid, does not touch the pod when the database delete fails or for the unsaved default wid, and no ops when the flag is off. - `user-workflow.component.spec.ts`: single delete cleans up wid 5, bulk delete cleans up each checked wid in order, and neither the no wid path nor a backend delete error touches the pod. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) --- .../user-workflow/user-workflow.component.spec.ts | 19 ++- .../user/user-workflow/user-workflow.component.ts | 20 ++- .../jupyter-panel/jupyter-panel.service.spec.ts | 17 ++- .../service/jupyter-panel/jupyter-panel.service.ts | 12 +- .../notebook-migration.service.spec.ts | 36 +++++ .../notebook-migration.service.ts | 18 +++ .../resource/NotebookMigrationResource.scala | 153 ++++++++++++++++----- .../resource/NotebookMigrationResourceSpec.scala | 110 +++++++++++++-- 8 files changed, 323 insertions(+), 62 deletions(-) diff --git a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts index db2c2317b8..25f544da39 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.spec.ts @@ -654,9 +654,12 @@ describe("SavedWorkflowSectionComponent", () => { }); describe("deleteWorkflow", () => { - it("deletes an entry with a wid and removes it from the results", () => { + it("deletes an entry with a wid, removes it from the results, and cleans up its pod notebook", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(of(null)); + const cleanup = vi + .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow") + .mockResolvedValue(undefined); const target = makeEntry(5, "to delete"); setEntries([target, makeEntry(6, "keep")]); @@ -664,15 +667,18 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).toHaveBeenCalledWith([5]); expect(component.searchResultsComponent.entries.map(e => e.name)).toEqual(["keep"]); + expect(cleanup).toHaveBeenCalledWith(5); }); it("does nothing when the entry has no wid", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn(); + const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow"); component.deleteWorkflow(makeEntry(undefined, "no wid")); expect(persist.deleteWorkflow).not.toHaveBeenCalled(); + expect(cleanup).not.toHaveBeenCalled(); }); }); @@ -732,9 +738,12 @@ describe("SavedWorkflowSectionComponent", () => { }); describe("handleConfirmDeleteSelectedWorkflows", () => { - it("deletes checked wids and keeps undefined-wid entries", () => { + it("deletes checked wids, keeps undefined-wid entries, and cleans up each pod notebook", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(of(null)); + const cleanup = vi + .spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow") + .mockResolvedValue(undefined); setEntries([ makeEntry(1, "a", true), makeEntry(2, "b", true), @@ -746,6 +755,7 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).toHaveBeenCalledWith([1, 2]); expect(component.searchResultsComponent.entries.map(e => e.name)).toEqual(["c", "d"]); + expect(cleanup.mock.calls.map(c => c[0])).toEqual([1, 2]); }); it("early-returns when a checked entry has no wid", () => { @@ -758,15 +768,18 @@ describe("SavedWorkflowSectionComponent", () => { expect(persist.deleteWorkflow).not.toHaveBeenCalled(); }); - it("alerts on a deletion error", () => { + it("alerts on a deletion error and does not touch the pod", () => { const persist = TestBed.inject(WorkflowPersistService) as any; persist.deleteWorkflow = vi.fn().mockReturnValue(throwError(() => "delfail")); const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}); + const cleanup = vi.spyOn(TestBed.inject(NotebookMigrationService), "deleteNotebookForWorkflow"); setEntries([makeEntry(1, "a", true)]); component.handleConfirmDeleteSelectedWorkflows(); expect(alertSpy).toHaveBeenCalledWith("delfail"); + // The backend delete failed, so the pod file must be left in place. + expect(cleanup).not.toHaveBeenCalled(); }); }); diff --git a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts index 4bd6acda2b..fad38708e0 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts +++ b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.ts @@ -504,19 +504,32 @@ export class UserWorkflowComponent implements AfterViewInit, OnDestroy { */ public deleteWorkflow(entry: DashboardEntry): void { - if (entry.workflow.workflow.wid == undefined) { + const wid = entry.workflow.workflow.wid; + if (wid == undefined) { return; } this.workflowPersistService - .deleteWorkflow([entry.workflow.workflow.wid]) + .deleteWorkflow([wid]) .pipe(untilDestroyed(this)) .subscribe(_ => { this.searchResultsComponent.entries = this.searchResultsComponent.entries.filter( - workflowEntry => workflowEntry.workflow.workflow.wid !== entry.workflow.workflow.wid + workflowEntry => workflowEntry.workflow.workflow.wid !== wid ); + this.cleanupNotebookFiles([wid]); }); } + // Best-effort removal of the deleted workflows' notebook files from the Jupyter pod. + // The workflow delete already cascades the notebook DB rows, but the pod's per-workflow + // notebook_<wid>.ipynb only the frontend can reach, so clean it up here. Not awaited and + // never surfaced: a workflow with no notebook is a harmless 404, and an unreachable pod + // must not affect a delete that already succeeded. + private cleanupNotebookFiles(wids: number[]): void { + for (const wid of wids) { + void this.notebookMigrationService.deleteNotebookForWorkflow(wid); + } + } + /** * Verify Uploaded file name and upload the file */ @@ -696,6 +709,7 @@ export class UserWorkflowComponent implements AfterViewInit, OnDestroy { // Check if wid is defined and if it's not included in targetWids return entryWid === undefined || !targetWids.includes(entryWid); }); + this.cleanupNotebookFiles(targetWids); }, // TODO: fix this with notification component error: (err: unknown) => alert(err), diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index cd4abcdabc..256e8f2cba 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -88,6 +88,9 @@ describe("JupyterPanelService", () => { setMapping: vi.fn(), getJupyterURL: vi.fn().mockResolvedValue("http://jupyter";), deleteNotebookAndMapping: vi.fn().mockReturnValue(of({ success: true, deleted: 1 })), + // In the base mock because every successful delete fires it; a missing stub would + // throw inside the delete subscription rather than failing a targeted assertion. + deleteNotebookForWorkflow: vi.fn().mockResolvedValue(undefined), }; mockGuiConfig = { env: { pythonNotebookMigrationEnabled: true } }; @@ -146,6 +149,13 @@ describe("JupyterPanelService", () => { expect(mockWorkflow.unhighlightLinks).toHaveBeenCalled(); }); + it("deleteJupyterNotebook removes the pod's copy for the current workflow", () => { + service.deleteJupyterNotebook(); + + // The service derives the filename from the wid, so the panel just passes the wid. + expect(mockNotebook.deleteNotebookForWorkflow).toHaveBeenCalledWith(1); + }); + it("deleteJupyterNotebook keeps the panel open and notifies on failure", () => { mockNotebook.deleteNotebookAndMapping.mockReturnValueOnce(throwError(() => new Error("boom"))); let visible: boolean | null = null; @@ -157,6 +167,8 @@ describe("JupyterPanelService", () => { expect(mockNotification.error).toHaveBeenCalled(); expect(visible).toBe(true); expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); + // The notebook is still stored, so its file must stay in the pod. + expect(mockNotebook.deleteNotebookForWorkflow).not.toHaveBeenCalled(); }); it("deleteJupyterNotebook only resets local state for the default wid 0 (no backend call)", () => { @@ -170,8 +182,10 @@ describe("JupyterPanelService", () => { service.deleteJupyterNotebook(); - // wid 0 is the unsaved default workflow, so no backend delete should fire. + // wid 0 is the unsaved default workflow, so neither backend delete should fire: + // nothing is stored and no notebook file was ever uploaded for it. expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); + expect(mockNotebook.deleteNotebookForWorkflow).not.toHaveBeenCalled(); expect(visible).toBe(false); expect(exists).toBe(false); }); @@ -715,6 +729,7 @@ describe("JupyterPanelService", () => { service.deleteJupyterNotebook(); expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); + expect(mockNotebook.deleteNotebookForWorkflow).not.toHaveBeenCalled(); }); it("minimizeJupyterNotebookPanel does not flip visibility", () => { diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index ecb5c90eca..2a1681155d 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -237,15 +237,13 @@ export class JupyterPanelService { } } - // Delete the current workflow's stored notebook from the backend, then hide the - // panel and clear all local notebook state. This is the user-initiated action - // behind the panel's delete button, and is distinct from the workflow-switch - // cleanup (hideAndClearLocalState), which must never touch the backend. + // Delete the current workflow's stored notebook from the migration database and its file + // from the Jupyter pod, then hide the panel and clear all local notebook state. public deleteJupyterNotebook(): void { if (!this.enabled) return; const wid = this.workflowActionService.getWorkflow().wid; - // Unsaved workflow (wid undefined or the default wid 0): nothing is persisted, - // and a delete POST with such a wid would 500, so just reset local state. + // Unsaved workflow (wid undefined or the default wid 0): nothing is persisted and no + // notebook file was uploaded for it (the upload path needs a wid) if (!wid) { this.hideAndClearLocalState(); this.jupyterNotebookExists.next(false); @@ -257,6 +255,8 @@ export class JupyterPanelService { this.hideAndClearLocalState(); this.jupyterNotebookExists.next(false); this.clearHighlights(); + // wid is captured above, so a mid-flight workflow switch can't retarget this. + void this.notebookMigrationService.deleteNotebookForWorkflow(wid); }, error: (err: unknown) => { // Keep the panel open on failure so the user sees the notebook wasn't removed. diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts index 05699cfb21..26fa39ee4a 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts @@ -142,6 +142,37 @@ describe("NotebookMigrationService", () => { expect(mockNotificationService.error).toHaveBeenCalledWith(expect.stringContaining("network down")); }); + // deleteNotebookForWorkflow + it("posts the wid-derived notebook name to delete-notebook", async () => { + const promise = service.deleteNotebookForWorkflow(1); + + const req = httpMock.expectOne(req => req.url.endsWith("/notebook-migration/delete-notebook")); + + expect(req.request.method).toBe("POST"); + expect(req.request.body).toEqual({ notebookName: "notebook_1.ipynb" }); + + req.flush({ success: true, deleted: 1 }); + + await promise; + }); + + it("makes no HTTP call for wid 0, which would map to the shared default filename", async () => { + await service.deleteNotebookForWorkflow(0); + httpMock.expectNone(req => req.url.endsWith("/notebook-migration/delete-notebook")); + }); + + it("swallows the failure and shows no notification when the notebook file delete fails", async () => { + // Pod cleanup is best effort, so a failure is logged rather than surfaced: the + // database delete it follows has already succeeded. + const promise = service.deleteNotebookForWorkflow(1); + + const req = httpMock.expectOne(req => req.url.endsWith("/notebook-migration/delete-notebook")); + req.error(new ErrorEvent("Server error")); + + await promise; + expect(mockNotificationService.error).not.toHaveBeenCalled(); + }); + // jupyter URL methods (HttpClient so the JwtModule interceptor attaches the auth token) it("should return Jupyter URL when the request succeeds", async () => { const promise = service.getJupyterURL(); @@ -343,6 +374,11 @@ describe("NotebookMigrationService", () => { httpMock.expectNone(req => req.url.includes("/notebook-migration/set-notebook")); }); + it("deleteNotebookForWorkflow makes no HTTP call", async () => { + await service.deleteNotebookForWorkflow(1); + httpMock.expectNone(req => req.url.endsWith("/notebook-migration/delete-notebook")); + }); + it("getJupyterURL returns null without making an HTTP call", async () => { const result = await service.getJupyterURL(); expect(result).toBeNull(); diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts index 6fb73ba316..1cb5b92b8e 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts @@ -163,6 +163,24 @@ export class NotebookMigrationService { } } + // Remove a workflow's notebook file from the Jupyter pod. Takes a concrete wid so it can + // never fall back to the shared default filename and delete the wrong file; callers guard + // out unsaved workflows before calling. Best effort by design: the database rows are the + // source of truth for whether a workflow has a notebook, so a failure here is logged, not + // surfaced, and nothing acts on the outcome. + public async deleteNotebookForWorkflow(wid: number): Promise<void> { + if (!this.enabled) return; + if (!Number.isInteger(wid) || wid <= 0) return; + const jupyterAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/delete-notebook`; + const headers = new HttpHeaders({ "Content-Type": "application/json" }); + + try { + await firstValueFrom(this.http.post(jupyterAPIUrl, { notebookName: notebookFileName(wid) }, { headers })); + } catch (error) { + console.error("Error deleting notebook from pod: ", error); + } + } + public async getJupyterURL(): Promise<string | null> { if (!this.enabled) return null; try { diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala index 17a0a989d7..1600c4c61a 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala @@ -56,6 +56,37 @@ object NotebookMigrationResource extends LazyLogging { mapper.createObjectNode().put("success", true).put("deleted", deleted) ) + private def jupyterUnavailableResponse: Response = + Response + .status(500) + .entity( + mapper.writeValueAsString( + mapper + .createObjectNode() + .put("success", false) + .put("message", "Cannot connect to Jupyter server") + ) + ) + .build() + + // Parse a request body into a JSON object. Returns Left(400) on malformed JSON or a + // non-object root so a bad request is reported as a client error. + private def parseBody(body: String): Either[Response, JsonNode] = { + val json = + try mapper.readTree(body) + catch { case NonFatal(_) => null } + if (json == null || !json.isObject) { + Left( + Response + .status(Response.Status.BAD_REQUEST) + .entity(errorJson("Request body must be a JSON object")) + .build() + ) + } else { + Right(json) + } + } + // Read the required integer `wid` from a request body. Returns Left(400) when the field is // missing or not an integer so the caller can short-circuit. Without this a missing wid NPEs // into a 500 and a non-integer wid silently coerces to 0 via asInt(). @@ -117,17 +148,7 @@ object NotebookMigrationResource extends LazyLogging { } if (!isJupyterAvailable(jupyterUrl)) { - return Response - .status(500) - .entity( - """ - { - "success": false, - "message": "Cannot connect to Jupyter server" - } - """ - ) - .build() + return jupyterUnavailableResponse } Response @@ -138,17 +159,7 @@ object NotebookMigrationResource extends LazyLogging { // Returns the URL of Jupyter def getJupyterURL(): Response = { if (!isJupyterAvailable(jupyterUrl)) { - return Response - .status(500) - .entity( - """ - { - "success": false, - "message": "Cannot connect to Jupyter server" - } - """ - ) - .build() + return jupyterUnavailableResponse } Response.ok(successUrlJson(jupyterUrl)).build() @@ -158,7 +169,10 @@ object NotebookMigrationResource extends LazyLogging { def setNotebook(body: String): Response = { var conn: HttpURLConnection = null try { - val json = mapper.readTree(body) + val json = parseBody(body) match { + case Left(badRequest) => return badRequest + case Right(j) => j + } val notebookName = json.get("notebookName").asText() val notebookData = json.get("notebookData") @@ -174,17 +188,7 @@ object NotebookMigrationResource extends LazyLogging { } if (!isJupyterAvailable(jupyterUrl)) { - return Response - .status(500) - .entity( - """ - { - "success": false, - "message": "Cannot connect to Jupyter server" - } - """ - ) - .build() + return jupyterUnavailableResponse } // Construct Jupyter API URL @@ -251,10 +255,72 @@ object NotebookMigrationResource extends LazyLogging { } } + // Delete the notebook file from Jupyter's work/ directory: + def deleteNotebook(body: String): Response = { + var conn: HttpURLConnection = null + try { + val json = parseBody(body) match { + case Left(badRequest) => return badRequest + case Right(j) => j + } + + // Read the name defensively + val notebookName = + Option(json.get("notebookName")).filter(_.isTextual).map(_.asText()).getOrElse("") + + if (!notebookName.matches("[A-Za-z0-9._-]+\\.ipynb")) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity(errorJson(s"Invalid notebook name: $notebookName")) + .build() + } + + if (!isJupyterAvailable(jupyterUrl)) { + return jupyterUnavailableResponse + } + + val url = new URL(s"$jupyterUrl/api/contents/work/$notebookName") + conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("DELETE") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) + conn.setRequestProperty("Authorization", s"token $jupyterToken") + + val status = conn.getResponseCode + + // Jupyter answers 204 on a successful delete, or 200 when it echoes the deleted entry. + // A 404 means the file is already gone, which is the requested end state, so report it + // as a no-op (deleted=0) rather than an error: a workflow whose notebook was never + // uploaded must still delete cleanly. + if (status != 204 && status != 200 && status != 404) { + return Response + .status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(errorJson(s"Failed to delete notebook from Jupyter (status $status)")) + .build() + } + + Response.ok(successDeletedJson(if (status == 404) 0 else 1)).build() + + } catch { + case NonFatal(e) => + logger.error("Error deleting notebook from Jupyter", e) + Response + .status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(errorJson(e.getMessage)) + .build() + } finally { + if (conn != null) conn.disconnect() + } + } + // Store notebook + mapping in database def storeNotebookAndMapping(body: String, uid: java.lang.Integer): Response = { try { - val json = mapper.readTree(body) + val json = parseBody(body) match { + case Left(badRequest) => return badRequest + case Right(j) => j + } val wid: java.lang.Integer = readWid(json) match { case Left(badRequest) => return badRequest @@ -345,7 +411,10 @@ object NotebookMigrationResource extends LazyLogging { // Fetch notebook + mapping def fetchNotebookAndMapping(body: String, uid: java.lang.Integer): Response = { try { - val json = mapper.readTree(body) + val json = parseBody(body) match { + case Left(badRequest) => return badRequest + case Right(j) => j + } val wid: java.lang.Integer = readWid(json) match { case Left(badRequest) => return badRequest @@ -419,7 +488,10 @@ object NotebookMigrationResource extends LazyLogging { // is UNIQUE (one notebook per workflow), so wid alone identifies the row and vid is not needed. def deleteNotebookAndMapping(body: String, uid: java.lang.Integer): Response = { try { - val json = mapper.readTree(body) + val json = parseBody(body) match { + case Left(badRequest) => return badRequest + case Right(j) => j + } val wid: java.lang.Integer = readWid(json) match { case Left(badRequest) => return badRequest @@ -491,6 +563,13 @@ class NotebookMigrationResource extends LazyLogging { NotebookMigrationResource.setNotebook(body) } + @POST + @Path("/delete-notebook") + def deleteNotebook(body: String, @Auth user: SessionUser): Response = { + logger.info("Deleting notebook from Jupyter") + NotebookMigrationResource.deleteNotebook(body) + } + @POST @Path("/store-notebook-and-mapping") def storeNotebookAndMapping(body: String, @Auth user: SessionUser): Response = { diff --git a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala index 15ad27eff1..90b24f5349 100644 --- a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala +++ b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala @@ -73,6 +73,10 @@ class NotebookMigrationResourceSpec private var writerUid: Integer = _ // holds WRITE access to testWid private var readerUid: Integer = _ // holds READ access to testWid + // Method and path of the last /api/contents request the fake Jupyter saw, so a test can pin + // the verb and URL a Jupyter call uses. A var is safe here because the spec runs sequentially. + private var lastContentsRequest: Option[(String, String)] = None + private val sampleNotebook = """{"cells":[{"cell_type":"code","metadata":{},"source":"print(1)"}]}""" private val sampleMapping = @@ -87,6 +91,7 @@ class NotebookMigrationResourceSpec workflowVersionDao = new WorkflowVersionDao(cfg) userDao = new UserDao(cfg) workflowUserAccessDao = new WorkflowUserAccessDao(cfg) + lastContentsRequest = None cleanup() val workflow = new Workflow @@ -163,6 +168,9 @@ class NotebookMigrationResourceSpec private def deletePayload(): String = s"""{"wid": $testWid}""" + private def deleteNotebookPayload(name: String = "notebook.ipynb"): String = + s"""{"notebookName": "$name"}""" + private val resource = new NotebookMigrationResource() private def sessionUser(uid: Integer): SessionUser = { @@ -194,11 +202,17 @@ class NotebookMigrationResourceSpec "/api/contents", (exchange: com.sun.net.httpserver.HttpExchange) => { exchange.getRequestBody.readAllBytes() - val body = "{}".getBytes("UTF-8") - exchange.sendResponseHeaders(contentsStatus, body.length) - val os = exchange.getResponseBody - os.write(body) - os.close() + lastContentsRequest = Some((exchange.getRequestMethod, exchange.getRequestURI.getPath)) + if (contentsStatus == 204) { + // 204 carries no body, so send the headers with a -1 length. + exchange.sendResponseHeaders(contentsStatus, -1) + } else { + val body = "{}".getBytes("UTF-8") + exchange.sendResponseHeaders(contentsStatus, body.length) + val os = exchange.getResponseBody + os.write(body) + os.close() + } } ) server.start() @@ -362,11 +376,11 @@ class NotebookMigrationResourceSpec getDSLContext.fetchCount(WORKFLOW_NOTEBOOK_MAPPING) shouldBe 1 } - it should "return 500 when the request body is malformed JSON" in { - // Exercises the NonFatal catch path in deleteNotebookAndMapping. + it should "return 400 when the request body is malformed JSON" in { + // Malformed input is a client error, caught by parseBody before the generic 500 handler. resource .deleteNotebookAndMapping("not json", sessionUser(writerUid)) - .getStatus shouldBe 500 + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode } // -- wid validation --------------------------------------------------------- @@ -450,13 +464,15 @@ class NotebookMigrationResourceSpec resource.setNotebook(validNotebook, user).getStatus shouldBe 500 resource.getJupyterURL(user).getStatus shouldBe 500 resource.getJupyterIframeURL(null, user).getStatus shouldBe 500 + resource.deleteNotebook(deleteNotebookPayload(), user).getStatus shouldBe 500 } - it should "return 500 when the request body is malformed JSON" in { - // Exercises the NonFatal catch paths in setNotebook and fetchNotebookAndMapping. + it should "return 400 when the request body is malformed JSON" in { + // Malformed input is a client error: parseBody rejects it before any downstream work. val user = sessionUser(writerUid) - resource.setNotebook("not json", user).getStatus shouldBe 500 - resource.fetchNotebookAndMapping("not json", user).getStatus shouldBe 500 + val badRequest = Response.Status.BAD_REQUEST.getStatusCode + resource.setNotebook("not json", user).getStatus shouldBe badRequest + resource.fetchNotebookAndMapping("not json", user).getStatus shouldBe badRequest } it should "upload the notebook and return success when Jupyter accepts it" in { @@ -545,6 +561,76 @@ class NotebookMigrationResourceSpec } } + // -- deleteNotebook (Jupyter file) ------------------------------------------ + + "deleteNotebook" should "DELETE the notebook's contents path and report deleted=1" in { + withFakeJupyter(contentsStatus = 204) { + val name = s"notebook_$testWid.ipynb" + val resp = resource.deleteNotebook(deleteNotebookPayload(name), sessionUser(writerUid)) + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("\"deleted\":1") + // Pins the verb and the work/ path, the two things that make this the counterpart + // of setNotebook's PUT rather than a delete of some other file. + lastContentsRequest shouldBe Some(("DELETE", s"/api/contents/work/$name")) + } + } + + it should "treat a 200 from Jupyter as a successful delete, reporting deleted=1" in { + // Some Jupyter versions answer 200 instead of 204 on a delete; both mean success. + withFakeJupyter(contentsStatus = 200) { + val resp = resource.deleteNotebook(deleteNotebookPayload(), sessionUser(writerUid)) + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("\"deleted\":1") + } + } + + it should "treat a 404 from Jupyter as a no-op, reporting deleted=0" in { + // A workflow whose notebook was never uploaded must still delete cleanly. + withFakeJupyter(contentsStatus = 404) { + val resp = resource.deleteNotebook(deleteNotebookPayload(), sessionUser(writerUid)) + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("\"deleted\":0") + } + } + + it should "return 500 when Jupyter rejects the delete" in { + withFakeJupyter(contentsStatus = 500) { + resource + .deleteNotebook(deleteNotebookPayload(), sessionUser(writerUid)) + .getStatus shouldBe 500 + } + } + + it should "reject a notebook name that is not a plain .ipynb filename with 400" in { + // Validated before any Jupyter call, so no server is needed. Covers path traversal, + // a wrong extension, and an embedded subpath. + Seq("../../etc/evil.ipynb", "notebook.txt", "work/notebook.ipynb").foreach { name => + withClue(s"name=$name: ") { + NotebookMigrationResource + .deleteNotebook(deleteNotebookPayload(name)) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + } + lastContentsRequest shouldBe None + } + } + + it should "return 400 when 'notebookName' is missing or not a string" in { + // A missing name must be a client error, not a 500 from null.asText(). + Seq("""{}""", """{"notebookName": 7}""").foreach { body => + withClue(s"body=$body: ") { + NotebookMigrationResource + .deleteNotebook(body) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + } + } + } + + it should "return 400 when the request body is malformed JSON" in { + resource + .deleteNotebook("not json", sessionUser(writerUid)) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + } + // -- setNotebook ------------------------------------------------------------ "setNotebook" should "reject a notebook name that is not a plain .ipynb filename with 400" in {
