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 b8ebbb9f195..ac41ccd28d1 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": } 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 35c3f473773..ea6505a86b7 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 {