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-7132-2ec3b0a10b4e1f83e9c20f9075d4946c13c23f50 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 51b6af3574f41fa3f477a50eee7c7c0c5bc7bee4 Author: Ryan Zhang <[email protected]> AuthorDate: Thu Jul 30 15:58:13 2026 -0700 feat(python-notebook-migration): add backend endpoint to delete a workflow's stored notebook and mapping (#7132) ### What changes were proposed in this PR? Adds a single backend REST endpoint to the notebook-migration service that deletes a workflow's stored notebook and its workflow-to-notebook mapping. Until now the service could store and fetch a workflow's notebook (`store-notebook-and-mapping`, `fetch-notebook-and-mapping`) but had no way to remove one. Once stored, a notebook and its mapping stayed in the database with no server-side operation to clear them. This left the persisted rows behind whenever a user closed or discarded a migrated notebook, and it blocked the end-to-end deletion flow, which needs a backend call to remove the notebook before the UI can reset the state that reflects whether a notebook exists. New endpoint: `POST /notebook-migration/delete-notebook-and-mapping`. Behavior: - Reads `wid` from the JSON body. `vid` is not required: `notebook.wid` is UNIQUE (one notebook per workflow), so `wid` alone identifies the row. - Requires write access to the workflow. Returns 403 when the caller lacks it, consistent with the store and fetch endpoints. - Deletes the `notebook` row for the workflow. The `workflow_notebook_mapping` rows are removed by the existing `ON DELETE CASCADE` foreign key, so a single delete clears both tables and cannot leave them inconsistent. - Returns `{"success": true, "deleted": <count>}`, where the count is 1 when a notebook was removed and 0 when nothing was stored, so the caller can tell a real deletion from a no-op. - Idempotent: deleting when nothing is stored returns success with `deleted: 0`. POST with a JSON body is used to stay consistent with the existing store and fetch endpoints, which already read `wid` from a JSON body. This is backend only. The frontend service method, the close-panel wiring, and any Jupyter-server cleanup are intentionally left to the follow-on end-to-end deletion work that consumes this endpoint. Note: if the workflow itself is deleted, the notebook and mapping are already removed by existing cascades, so this endpoint targets the case where the workflow survives but its notebook should be discarded. ### Any related issues, documentation, discussions? Closes #7131 Parent issue #4301 ### How was this PR tested? Added resource spec cases in `NotebookMigrationResourceSpec` covering: - Deleting a workflow that has a stored notebook removes both the notebook row and its mapping rows (cascade verified) and reports `deleted: 1`. - Idempotent delete: returns success with `deleted: 0` when nothing is stored. - Returns 403 and deletes nothing when the caller has only read access. - Returns 500 on a malformed JSON body. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) --- .../resource/NotebookMigrationResource.scala | 86 +++++++++++++++++++++- .../resource/NotebookMigrationResourceSpec.scala | 78 ++++++++++++++++++++ 2 files changed, 161 insertions(+), 3 deletions(-) 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 b8ebbb9f19..ac41ccd28d 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 @@ -17,7 +17,7 @@ package org.apache.texera.service.resource -import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} import com.fasterxml.jackson.module.scala.DefaultScalaModule import com.typesafe.scalalogging.LazyLogging import io.dropwizard.auth.Auth @@ -49,6 +49,30 @@ object NotebookMigrationResource extends LazyLogging { private def successUrlJson(url: String): String = mapper.writeValueAsString(mapper.createObjectNode().put("success", true).put("url", url)) + // Build a {"success": true, "deleted": <count>} body via the mapper. The count lets the + // caller distinguish a real deletion (1) from a no-op when nothing was stored (0). + private def successDeletedJson(deleted: Int): String = + mapper.writeValueAsString( + mapper.createObjectNode().put("success", true).put("deleted", deleted) + ) + + // 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(). + private def readWid(json: JsonNode): Either[Response, java.lang.Integer] = { + val widNode = json.get("wid") + if (widNode == null || !widNode.isInt) { + Left( + Response + .status(Response.Status.BAD_REQUEST) + .entity(errorJson("Missing or invalid 'wid'")) + .build() + ) + } else { + Right(widNode.asInt()) + } + } + private val jupyterUrl = StorageConfig.jupyterURL private val jupyterToken = StorageConfig.jupyterToken // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook. @@ -223,7 +247,10 @@ object NotebookMigrationResource extends LazyLogging { try { val json = mapper.readTree(body) - val wid: java.lang.Integer = json.get("wid").asInt() + val wid: java.lang.Integer = readWid(json) match { + case Left(badRequest) => return badRequest + case Right(w) => w + } val vid: java.lang.Integer = json.get("vid").asInt() val mappingNode = json.get("mapping") val notebookNode = json.get("notebook") @@ -311,7 +338,10 @@ object NotebookMigrationResource extends LazyLogging { try { val json = mapper.readTree(body) - val wid: java.lang.Integer = json.get("wid").asInt() + val wid: java.lang.Integer = readWid(json) match { + case Left(badRequest) => return badRequest + case Right(w) => w + } val vid: java.lang.Integer = json.get("vid").asInt() // Only a user with write access to the workflow may fetch its notebook. @@ -374,6 +404,49 @@ object NotebookMigrationResource extends LazyLogging { .build() } } + + // Delete notebook + mapping for a workflow. The notebook -> workflow_notebook_mapping FK is + // ON DELETE CASCADE, so deleting the notebook row removes its mapping rows too. notebook.wid + // 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 wid: java.lang.Integer = readWid(json) match { + case Left(badRequest) => return badRequest + case Right(w) => w + } + + // Only a user with write access to the workflow may delete its notebook. + if (!WorkflowAccessResource.hasWriteAccess(wid, uid)) { + return Response + .status(Response.Status.FORBIDDEN) + .entity(errorJson(s"No write access to workflow $wid")) + .build() + } + + val dsl = SqlServer.getInstance().createDSLContext() + + // execute() returns the affected row count: 1 when a notebook was removed, 0 when the + // workflow had nothing stored (idempotent no-op). + val deleted: Int = SqlServer.withTransaction(dsl) { ctx => + ctx + .deleteFrom(Notebook.NOTEBOOK) + .where(Notebook.NOTEBOOK.WID.eq(wid)) + .execute() + } + + Response.ok(successDeletedJson(deleted)).build() + + } catch { + case NonFatal(e) => + logger.error("Error deleting notebook and mapping", e) + Response + .status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(errorJson(e.getMessage)) + .build() + } + } } @Path("/notebook-migration") @@ -416,4 +489,11 @@ class NotebookMigrationResource extends LazyLogging { logger.info("Fetching notebook and mapping") NotebookMigrationResource.fetchNotebookAndMapping(body, user.getUid) } + + @POST + @Path("/delete-notebook-and-mapping") + def deleteNotebookAndMapping(body: String, @Auth user: SessionUser): Response = { + logger.info("Deleting notebook and mapping") + NotebookMigrationResource.deleteNotebookAndMapping(body, user.getUid) + } } 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 35c3f47377..ea6505a86b 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 @@ -161,6 +161,9 @@ class NotebookMigrationResourceSpec private def fetchPayload(vid: Integer = seededVid): String = s"""{"wid": $testWid, "vid": $vid}""" + private def deletePayload(): String = + s"""{"wid": $testWid}""" + private val resource = new NotebookMigrationResource() private def sessionUser(uid: Integer): SessionUser = { @@ -326,6 +329,81 @@ class NotebookMigrationResourceSpec entity should include("\"v1\"") } + // -- deleteNotebookAndMapping ----------------------------------------------- + + "deleteNotebookAndMapping" should "remove the notebook and cascade to its mapping, reporting deleted=1" in { + NotebookMigrationResource.storeNotebookAndMapping(storePayload(), writerUid) + getDSLContext.fetchCount(NOTEBOOK) shouldBe 1 + getDSLContext.fetchCount(WORKFLOW_NOTEBOOK_MAPPING) shouldBe 1 + + val response = NotebookMigrationResource.deleteNotebookAndMapping(deletePayload(), writerUid) + response.getStatus shouldBe Response.Status.OK.getStatusCode + response.getEntity.toString should include("\"deleted\":1") + + // Deleting the notebook row cascades to workflow_notebook_mapping via the FK. + getDSLContext.fetchCount(NOTEBOOK) shouldBe 0 + getDSLContext.fetchCount(WORKFLOW_NOTEBOOK_MAPPING) shouldBe 0 + } + + it should "be idempotent, returning success with deleted=0 when nothing is stored" in { + val response = NotebookMigrationResource.deleteNotebookAndMapping(deletePayload(), writerUid) + response.getStatus shouldBe Response.Status.OK.getStatusCode + response.getEntity.toString should include("\"deleted\":0") + } + + it should "return 403 Forbidden and delete nothing when the user lacks write access" in { + NotebookMigrationResource.storeNotebookAndMapping(storePayload(), writerUid) + + // readerUid holds only READ access; delete requires WRITE. + NotebookMigrationResource + .deleteNotebookAndMapping(deletePayload(), readerUid) + .getStatus shouldBe Response.Status.FORBIDDEN.getStatusCode + + getDSLContext.fetchCount(NOTEBOOK) shouldBe 1 + 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. + resource + .deleteNotebookAndMapping("not json", sessionUser(writerUid)) + .getStatus shouldBe 500 + } + + // -- wid validation --------------------------------------------------------- + + "store/fetch/delete" should "return 400 Bad Request when 'wid' is missing from the body" in { + // A missing wid must be a client error, not a 500 from the null.asInt() NPE. + val noWid = s"""{"vid": $seededVid}""" + NotebookMigrationResource + .storeNotebookAndMapping(noWid, writerUid) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + NotebookMigrationResource + .fetchNotebookAndMapping(noWid, writerUid) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + NotebookMigrationResource + .deleteNotebookAndMapping("""{}""", writerUid) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + + getDSLContext.fetchCount(NOTEBOOK) shouldBe 0 + } + + it should "return 400 Bad Request when 'wid' is not an integer" in { + // A non-integer wid must be rejected rather than silently coerced to 0 by asInt(). + val badWid = s"""{"wid": "not-an-int", "vid": $seededVid}""" + NotebookMigrationResource + .storeNotebookAndMapping(badWid, writerUid) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + NotebookMigrationResource + .fetchNotebookAndMapping(badWid, writerUid) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + NotebookMigrationResource + .deleteNotebookAndMapping(badWid, writerUid) + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + + getDSLContext.fetchCount(NOTEBOOK) shouldBe 0 + } + // -- workflow write-access enforcement -------------------------------------- "store/fetch" should "return 403 Forbidden when the user lacks write access to the workflow" in {
