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 256e8f2cba1..3abaddd342f 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 @@ -123,7 +123,7 @@ describe("JupyterPanelService", () => { service.jupyterNotebookPanelVisible$.subscribe(v => (state = v)); - service.openPanel("JupyterNotebookPanel"); + (service as any).jupyterNotebookPanelVisible.next(true); expect(state).toBe(true); service.deleteJupyterNotebook(); @@ -221,38 +221,14 @@ describe("JupyterPanelService", () => { expect(state).toBe(true); }); - // openPanel - it("should open panel only for correct name", () => { - let state: boolean | null = false; - - service.jupyterNotebookPanelVisible$.subscribe(v => (state = v)); - - service.openPanel("WrongPanel"); - expect(state).toBe(false); - - service.openPanel("JupyterNotebookPanel"); - expect(state).toBe(true); - }); - - it("openPanel flags jupyterNotebookExists$ so the toolbar expand button appears after an in-place import", () => { - const states: boolean[] = []; - service.jupyterNotebookExists$.subscribe(v => states.push(v)); - expect(states.at(-1)).toBe(false); - - // Wrong panel name does not flip the flag. - service.openPanel("WrongPanel"); - expect(states.at(-1)).toBe(false); - - // Opening the jupyter panel records that the workflow now has a notebook. - service.openPanel("JupyterNotebookPanel"); - expect(states.at(-1)).toBe(true); - }); - // HTTP fetchNotebookAndMapping it("should return 0 when exists=false", async () => { - const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1, 1)); + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1)); const req = httpMock.expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")); + // The fetch keys on wid only; no vid is sent (the backend ignores version). + expect(req.request.body.wid).toBe(1); + expect(req.request.body.vid).toBeUndefined(); req.flush({ exists: false }); expect(await resultPromise).toBe(0); @@ -271,7 +247,7 @@ describe("JupyterPanelService", () => { const mapping = { cell_to_operator: { cell1: ["A"] }, operator_to_cell: {} }; const notebook = { cells: [] }; - const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1, 1)); + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1)); httpMock .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) .flush({ exists: true, mapping, notebook }); @@ -289,7 +265,7 @@ describe("JupyterPanelService", () => { mockNotebook.sendNotebookToJupyter = vi.fn().mockResolvedValue(1); const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); - const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1, 1)); + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1)); httpMock .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) .flush("migration service down", { status: 500, statusText: "Server Error" }); @@ -307,7 +283,7 @@ describe("JupyterPanelService", () => { const mapping = { cell_to_operator: {}, operator_to_cell: {} }; const notebook = { cells: [] }; - const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(2, 1)); + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(2)); httpMock .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) .flush({ exists: true, mapping, notebook }); @@ -716,13 +692,6 @@ describe("JupyterPanelService", () => { expect(mockWorkflow.workflowMetaDataChanged).not.toHaveBeenCalled(); }); - it("openPanel does not flip the visibility stream", () => { - let state: boolean | null = false; - service.jupyterNotebookPanelVisible$.subscribe(v => (state = v)); - service.openPanel("JupyterNotebookPanel"); - expect(state).toBe(false); - }); - it("deleteJupyterNotebook does not call the backend or delete the mapping when disabled", () => { // When the feature is disabled the method returns early, so neither the // backend delete nor the local mapping drop should run. 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 2a1681155d5..8f4624867d8 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 @@ -125,16 +125,12 @@ export class JupyterPanelService { }); } - private fetchNotebookAndMapping( - workflowID: number | undefined = this.workflowActionService.getWorkflow().wid, - vId: number = 1 - ) { - // Fetch mapping and notebook from migration database if exists for wid + private fetchNotebookAndMapping(workflowID: number | undefined = this.workflowActionService.getWorkflow().wid) { + // Fetch mapping and notebook from migration database if exists for wid. const dbAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/fetch-notebook-and-mapping`; const headers = new HttpHeaders({ "Content-Type": "application/json" }); const payload = { wid: workflowID, - vid: vId, // Future work: add dynamic fetching of current workflow vId }; return this.http.post(dbAPIUrl, payload, { headers }).pipe( @@ -224,19 +220,6 @@ export class JupyterPanelService { return this.notebookMigrationService.getJupyterIframeURL(this.currentNotebookFileName()); } - // Open the Jupyter Notebook panel - public openPanel(panelName: string): void { - if (!this.enabled) return; - if (panelName === "JupyterNotebookPanel") { - this.jupyterNotebookPanelVisible.next(true); - // Opening the panel means the current workflow has an associated notebook, so - // surface the toolbar "expand" button (jupyterNotebookExists$) right away. Needed - // after an in-place import where the wid does not change and init() does not re-run - // to detect the notebook. - this.jupyterNotebookExists.next(true); - } - } - // 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 { 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 26fa39ee4a4..7ffa452a487 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 @@ -264,14 +264,14 @@ describe("NotebookMigrationService", () => { }); // storeNotebookAndMapping - it("should call storeNotebookAndMapping API with the default vid", () => { + it("should call storeNotebookAndMapping API without a vid (resolved server-side)", () => { service.storeNotebookAndMapping(1, {}, {}).subscribe(); const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/store-notebook-and-mapping")); expect(req.request.method).toBe("POST"); expect(req.request.body.wid).toBe(1); - expect(req.request.body.vid).toBe(1); + expect(req.request.body.vid).toBeUndefined(); req.flush({ success: true, message: "stored" }); }); 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 1cb5b92b8ec..ed3ffd5a6ee 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 @@ -228,8 +228,7 @@ export class NotebookMigrationService { public storeNotebookAndMapping( wid: number | undefined, mappingContent: any, - notebookContent: any, - vid: number = 1 + notebookContent: any ): Observable { if (!this.enabled) { return of({ success: false, message: "Notebook migration feature is disabled" }); @@ -237,9 +236,10 @@ export class NotebookMigrationService { const dbAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/store-notebook-and-mapping`; const headers = new HttpHeaders({ "Content-Type": "application/json" }); + // The mapping's version id (vid) is resolved server-side from the workflow's + // latest version to anchor its FK, so no vid is sent from here. const payload = { wid, - vid, mapping: mappingContent, notebook: notebookContent, }; 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 1600c4c61ab..3aa0c357c9f 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 @@ -28,8 +28,10 @@ import org.apache.texera.auth.SessionUser import org.apache.texera.dao.SqlServer import org.jooq.JSONB import org.jooq.exception.DataAccessException +import org.jooq.impl.DSL import org.apache.texera.dao.jooq.generated.tables.Notebook import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import org.apache.texera.dao.jooq.generated.tables.WorkflowVersion import java.net.{HttpURLConnection, URL} import java.nio.charset.StandardCharsets import scala.util.control.NonFatal @@ -326,7 +328,6 @@ object NotebookMigrationResource extends LazyLogging { 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") @@ -342,7 +343,8 @@ object NotebookMigrationResource extends LazyLogging { // notebook.wid is UNIQUE: a workflow has at most one notebook. If one already // exists, reject the re-store with a 409 rather than letting the INSERT trip the - // constraint and surface as a 500. + // constraint and surface as a 500. Checked before the version lookup so a re-store + // skips that query. val alreadyStored = dsl.fetchExists( dsl.selectFrom(Notebook.NOTEBOOK).where(Notebook.NOTEBOOK.WID.eq(wid)) ) @@ -353,6 +355,21 @@ object NotebookMigrationResource extends LazyLogging { .build() } + // The mapping's vid FK must reference a real workflow_version row. Anchor it to the + // workflow's own latest version (created alongside the workflow) rather than a + // hardcoded id, so an unrelated workflow's version can never own or cascade it. + val vid: java.lang.Integer = dsl + .select(DSL.max(WorkflowVersion.WORKFLOW_VERSION.VID)) + .from(WorkflowVersion.WORKFLOW_VERSION) + .where(WorkflowVersion.WORKFLOW_VERSION.WID.eq(wid)) + .fetchOne(0, classOf[java.lang.Integer]) + if (vid == null) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity(errorJson(s"No workflow version exists for workflow $wid")) + .build() + } + val nid: java.lang.Integer = SqlServer.withTransaction(dsl) { ctx => // Insert notebook val notebookRecord = ctx @@ -420,8 +437,6 @@ object NotebookMigrationResource extends LazyLogging { 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. if (!WorkflowAccessResource.hasWriteAccess(wid, uid)) { return Response @@ -432,7 +447,10 @@ object NotebookMigrationResource extends LazyLogging { val dsl = SqlServer.getInstance().createDSLContext() - // Fetch the most recent notebook (highest nid) for this workflow version + // Fetch the notebook for this workflow, regardless of its version. + // + // Future work: to support one notebook per workflow version, drop the notebook.wid + // UNIQUE constraint and add a vid filter here. val result = dsl .select( Notebook.NOTEBOOK.NID, @@ -444,7 +462,6 @@ object NotebookMigrationResource extends LazyLogging { .on(Notebook.NOTEBOOK.WID.eq(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.WID)) .and(Notebook.NOTEBOOK.NID.eq(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.NID)) .where(Notebook.NOTEBOOK.WID.eq(wid)) - .and(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.VID.eq(vid)) .orderBy(Notebook.NOTEBOOK.NID.desc()) // most recent nid first .limit(1) // only take the latest .fetchOne() 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 90b24f53495..09a978efcbd 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 @@ -155,15 +155,16 @@ class NotebookMigrationResourceSpec getDSLContext.deleteFrom(USER).where(USER.EMAIL.in(writerEmail, readerEmail)).execute() } + // The endpoints resolve the vid server-side (store) or ignore it (fetch), so the + // client sends only the wid — no vid field, matching the real frontend requests. private def storePayload( notebook: String = sampleNotebook, - mapping: String = sampleMapping, - vid: Integer = seededVid + mapping: String = sampleMapping ): String = - s"""{"wid": $testWid, "vid": $vid, "notebook": $notebook, "mapping": $mapping}""" + s"""{"wid": $testWid, "notebook": $notebook, "mapping": $mapping}""" - private def fetchPayload(vid: Integer = seededVid): String = - s"""{"wid": $testWid, "vid": $vid}""" + private def fetchPayload(): String = + s"""{"wid": $testWid}""" private def deletePayload(): String = s"""{"wid": $testWid}""" @@ -234,6 +235,7 @@ class NotebookMigrationResourceSpec val mappingRow = getDSLContext.selectFrom(WORKFLOW_NOTEBOOK_MAPPING).fetchOne() mappingRow.get(WORKFLOW_NOTEBOOK_MAPPING.WID) shouldBe testWid + // vid is resolved server-side to the workflow's latest version (the only seeded one here). mappingRow.get(WORKFLOW_NOTEBOOK_MAPPING.VID) shouldBe seededVid // The mapping row must reference the just-inserted notebook by its returned nid. mappingRow.get(WORKFLOW_NOTEBOOK_MAPPING.NID) shouldBe notebookRow.get(NOTEBOOK.NID) @@ -271,18 +273,26 @@ class NotebookMigrationResourceSpec storedMappingJson should include("\"cell1\"") } - it should "roll back the notebook insert when the mapping insert fails its FK constraint" in { - // workflow_notebook_mapping.vid has FK -> workflow_version(vid). Passing an - // unknown vid trips the mapping insert; because both inserts share a single - // SqlServer.withTransaction block, the notebook insert must roll back too. - // Without this guarantee, orphaned notebook rows would accumulate on every - // failed store. - val unknownVid: Integer = -1 - val response = NotebookMigrationResource.storeNotebookAndMapping( - storePayload(vid = unknownVid), - writerUid - ) - response.getStatus shouldBe Response.Status.INTERNAL_SERVER_ERROR.getStatusCode + it should "ignore a client-supplied vid and anchor the mapping to the workflow's latest version" in { + // The vid FK is resolved server-side to MAX(workflow_version.vid) for the wid, so a + // stale or bogus vid in the request body must never reach the mapping row. This pins + // the fix for the old hardcoded vid=1 behaviour. + val payload = + s"""{"wid": $testWid, "vid": 999999, "notebook": $sampleNotebook, "mapping": $sampleMapping}""" + val response = NotebookMigrationResource.storeNotebookAndMapping(payload, writerUid) + response.getStatus shouldBe Response.Status.OK.getStatusCode + + val mappingRow = getDSLContext.selectFrom(WORKFLOW_NOTEBOOK_MAPPING).fetchOne() + mappingRow.get(WORKFLOW_NOTEBOOK_MAPPING.VID) shouldBe seededVid + } + + it should "return 400 and store nothing when the workflow has no version to anchor the mapping" in { + // The mapping's vid FK needs a real workflow_version row. With none, the store must + // fail cleanly with a 400 before any insert, not a 500 from the FK constraint. + getDSLContext.deleteFrom(WORKFLOW_VERSION).where(WORKFLOW_VERSION.WID.eq(testWid)).execute() + + val response = NotebookMigrationResource.storeNotebookAndMapping(storePayload(), writerUid) + response.getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode getDSLContext.fetchCount(NOTEBOOK) shouldBe 0 getDSLContext.fetchCount(WORKFLOW_NOTEBOOK_MAPPING) shouldBe 0 } @@ -303,7 +313,7 @@ class NotebookMigrationResourceSpec // -- fetchNotebookAndMapping ------------------------------------------------ - "fetchNotebookAndMapping" should "return exists=false when no notebook is stored for the (wid, vid)" in { + "fetchNotebookAndMapping" should "return exists=false when no notebook is stored for the workflow" in { val response = NotebookMigrationResource.fetchNotebookAndMapping(fetchPayload(), writerUid) response.getStatus shouldBe Response.Status.OK.getStatusCode response.getEntity.toString should include("\"exists\": false") @@ -322,7 +332,7 @@ class NotebookMigrationResourceSpec entity should include("\"mapping\":") } - it should "return the stored notebook content for a (wid, vid) on fetch" in { + it should "return the stored notebook content for the workflow on fetch" in { // notebook.wid is UNIQUE — one notebook per workflow — so the endpoint's // orderBy(NID.desc).limit(1) resolves to that single row. This pins the // workflow-reopen path: after a store, fetch must return that notebook's content. @@ -342,6 +352,27 @@ class NotebookMigrationResourceSpec entity should include("\"v1\"") } + it should "return the notebook regardless of the workflow's current version" in { + // The mapping is stored under the version present at store time; the workflow may then + // advance to a newer version. Fetch keys on wid alone, so the notebook still reattaches. + // This mirrors the reopen-after-edit path. + NotebookMigrationResource.storeNotebookAndMapping(storePayload(), writerUid) + + val newerVersion = new WorkflowVersion + newerVersion.setWid(testWid) + newerVersion.setContent("{}") + newerVersion.setCreationTime(new Timestamp(System.currentTimeMillis())) + workflowVersionDao.insert(newerVersion) + newerVersion.getVid.intValue() should be > seededVid.intValue() + + val entity = + NotebookMigrationResource + .fetchNotebookAndMapping(fetchPayload(), writerUid) + .getEntity + .toString + entity should include("\"exists\": true") + } + // -- deleteNotebookAndMapping ----------------------------------------------- "deleteNotebookAndMapping" should "remove the notebook and cascade to its mapping, reporting deleted=1" in { @@ -387,7 +418,7 @@ class NotebookMigrationResourceSpec "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}""" + val noWid = """{"notebook": {}, "mapping": {}}""" NotebookMigrationResource .storeNotebookAndMapping(noWid, writerUid) .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode @@ -403,7 +434,7 @@ class NotebookMigrationResourceSpec 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}""" + val badWid = """{"wid": "not-an-int"}""" NotebookMigrationResource .storeNotebookAndMapping(badWid, writerUid) .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode @@ -472,6 +503,7 @@ class NotebookMigrationResourceSpec val user = sessionUser(writerUid) val badRequest = Response.Status.BAD_REQUEST.getStatusCode resource.setNotebook("not json", user).getStatus shouldBe badRequest + resource.storeNotebookAndMapping("not json", user).getStatus shouldBe badRequest resource.fetchNotebookAndMapping("not json", user).getStatus shouldBe badRequest }