refactor(python-notebook-migration): key the notebook mapping to the workflow's own version instead of a hardcoded vid=1

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.

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 {

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" });
   });
 

frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts

@@ -228,18 +228,18 @@ export class NotebookMigrationService {
   public storeNotebookAndMapping(
     wid: number | undefined,
     mappingContent: any,
-    notebookContent: any,
-    vid: number = 1
+    notebookContent: any
   ): Observable<StoreNotebookResponse> {
     if (!this.enabled) {
       return of({ success: false, message: "Notebook migration feature is disabled" });
     }
     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,
     };

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()

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
   }