Copilot commented on code in PR #4437: URL: https://github.com/apache/texera/pull/4437#discussion_r3126654903
########## sql/updates/23.sql: ########## @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +-- ============================================ +-- 1. Connect to the texera_db database +-- ============================================ +\c texera_db + +SET search_path TO texera_db; + +-- ============================================ +-- 2. Delete tables if they already exist +-- ============================================ + +BEGIN; + +DROP TABLE IF EXISTS notebook CASCADE; +DROP TABLE IF EXISTS workflow_notebook_mapping CASCADE; + +-- ============================================ +-- 3. Create the tables to store notebook and mapping +-- ============================================ + +CREATE TABLE notebook ( + nid SERIAL NOT NULL PRIMARY KEY, + wid INT NOT NULL, + notebook JSONB NOT NULL, + FOREIGN KEY (wid) REFERENCES workflow(wid) ON DELETE CASCADE +); + +CREATE TABLE workflow_notebook_mapping ( + wid INT NOT NULL, + vid INT NOT NULL, + nid INT NOT NULL, + mapping JSONB NOT NULL, + PRIMARY KEY (wid, vid, nid), + FOREIGN KEY (wid) REFERENCES workflow(wid) ON DELETE CASCADE, + FOREIGN KEY (vid) REFERENCES workflow_version(vid) ON DELETE CASCADE, + FOREIGN KEY (nid) REFERENCES notebook(nid) ON DELETE CASCADE +); Review Comment: This migration adds new tables, but the base schema file `sql/texera_ddl.sql` (used by the test EmbeddedPostgres setup in `MockTexeraDB`) does not include them. That can break local/dev/test DB initialization and any tooling/codegen that assumes the base DDL represents the full schema. Please also update `sql/texera_ddl.sql` to include these table definitions (or document/automate applying update scripts during DB initialization). ########## notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala: ########## @@ -0,0 +1,324 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.dao.SqlServer +import org.jooq.JSONB +import org.apache.texera.dao.jooq.generated.tables.Notebook +import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import java.net.{HttpURLConnection, URL} +import java.nio.charset.StandardCharsets +import scala.util.control.NonFatal +import org.apache.texera.amber.config.StorageConfig + +object NotebookMigrationResource extends LazyLogging { + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) + private val jupyterUrl = StorageConfig.jupyterURL + private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + private def isJupyterAvailable(jupyterUrl: String): Boolean = { + try { + val conn = new java.net.URL(s"$jupyterUrl/api") + .openConnection() + .asInstanceOf[java.net.HttpURLConnection] + + conn.setRequestMethod("GET") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) + + val status = conn.getResponseCode + + status == 200 || status == 403 + } catch { + case _: Exception => false + } + } + + // Returns the Jupyter iframe reference URL + def getJupyterIframeURL(): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterIframeURL" + } + """ + ).build() + } + + // 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() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterUrl" + } + """ + ).build() + } + + // Set the notebook in Jupyter + def setNotebook(body: String): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + try { + val json = mapper.readTree(body) + + val notebookName = json.get("notebookName").asText() + val notebookData = json.get("notebookData") + + // Construct Jupyter API URL + val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName" + + val url = new URL(apiUrl) + val conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("PUT") + conn.setDoOutput(true) + conn.setRequestProperty("Content-Type", "application/json") + + val requestBody = + s""" + { + "type": "notebook", + "content": $notebookData + } + """ + + val os = conn.getOutputStream + os.write(requestBody.getBytes(StandardCharsets.UTF_8)) + os.flush() + os.close() + Review Comment: The Jupyter upload request uses `HttpURLConnection` without any connect/read timeouts. If the Jupyter server stalls, the request thread can hang indefinitely and exhaust the service thread pool. Please set reasonable `connectTimeout`/`readTimeout` (similar to `isJupyterAvailable`) and consider reading/closing the response stream to ensure the connection is fully released. ########## notebook-migration-service/project/build.properties: ########## @@ -0,0 +1,18 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +sbt.version = 1.9.9 Review Comment: This service-specific `project/build.properties` pins SBT to 1.9.9, but the repo root uses SBT 1.12.9. Having multiple SBT versions in one repo can cause confusing build behavior depending on where SBT is invoked. Please align this to the root SBT version (or remove the subproject build.properties if it isn't needed). ```suggestion sbt.version = 1.12.9 ``` ########## notebook-migration-service/src/main/resources/docker-compose.yml: ########## @@ -0,0 +1,34 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: texera-jupyter +services: + + jupyter: + build: + context: . + dockerfile: Dockerfile + container_name: texera-jupyter + ports: + - "9100:8888" + command: > + start-notebook.sh + --NotebookApp.token='' + --NotebookApp.password='' + --NotebookApp.disable_check_xsrf=True + --NotebookApp.tornado_settings="{'headers': {'Content-Security-Policy': 'frame-ancestors http://localhost:*'}}" + --NotebookApp.default_url=/tree Review Comment: This compose config disables Jupyter authentication (`token=''`, `password=''`) and disables XSRF checks. If this is used outside strictly-local development, it exposes the Jupyter server to trivial takeover (including executing arbitrary code in the container). Please gate these settings behind an explicit dev-only profile / env flags, and keep auth + XSRF enabled by default. ```suggestion sh -c ' if [ "$${JUPYTER_INSECURE_DEV_MODE:-false}" = "true" ]; then exec start-notebook.sh --NotebookApp.token="" --NotebookApp.password="" --NotebookApp.disable_check_xsrf=True --NotebookApp.tornado_settings="{\"headers\": {\"Content-Security-Policy\": \"frame-ancestors http://localhost:*\"}}"; --NotebookApp.default_url=/tree; else exec start-notebook.sh --NotebookApp.tornado_settings="{\"headers\": {\"Content-Security-Policy\": \"frame-ancestors http://localhost:*\"}}"; --NotebookApp.default_url=/tree; fi' ``` ########## notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala: ########## @@ -0,0 +1,324 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.dao.SqlServer +import org.jooq.JSONB +import org.apache.texera.dao.jooq.generated.tables.Notebook +import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import java.net.{HttpURLConnection, URL} +import java.nio.charset.StandardCharsets +import scala.util.control.NonFatal +import org.apache.texera.amber.config.StorageConfig + +object NotebookMigrationResource extends LazyLogging { + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) + private val jupyterUrl = StorageConfig.jupyterURL + private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + private def isJupyterAvailable(jupyterUrl: String): Boolean = { + try { + val conn = new java.net.URL(s"$jupyterUrl/api") + .openConnection() + .asInstanceOf[java.net.HttpURLConnection] + + conn.setRequestMethod("GET") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) + + val status = conn.getResponseCode + + status == 200 || status == 403 + } catch { + case _: Exception => false + } + } + + // Returns the Jupyter iframe reference URL + def getJupyterIframeURL(): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterIframeURL" + } + """ + ).build() + } + + // 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() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterUrl" + } + """ + ).build() + } + + // Set the notebook in Jupyter + def setNotebook(body: String): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + try { + val json = mapper.readTree(body) + + val notebookName = json.get("notebookName").asText() + val notebookData = json.get("notebookData") + + // Construct Jupyter API URL + val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName" + + val url = new URL(apiUrl) + val conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("PUT") + conn.setDoOutput(true) + conn.setRequestProperty("Content-Type", "application/json") + + val requestBody = + s""" + { + "type": "notebook", + "content": $notebookData + } + """ + + val os = conn.getOutputStream + os.write(requestBody.getBytes(StandardCharsets.UTF_8)) + os.flush() + os.close() + + val status = conn.getResponseCode + + if (status != 200 && status != 201) { + return Response.status(500).entity( + s""" + { + "success": false, + "message": "Failed to upload notebook to Jupyter (status $status)" + } + """ + ).build() + } + + jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + Response.ok( + s""" + { + "success": true, + "message": "Notebook successfully sent to Jupyter." + } + """ + ).build() + + } catch { + case NonFatal(e) => + logger.error("Error sending notebook to Jupyter", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(s"""{"error":"${e.getMessage}"}""") + .build() + } + } + + // Store notebook + mapping in database + def storeNotebookAndMapping(body: String): Response = { + try { + val json = mapper.readTree(body) + + val wid: java.lang.Integer = json.get("wid").asInt() + val vid: java.lang.Integer = json.get("vid").asInt() + val mappingNode = json.get("mapping") + val notebookNode = json.get("notebook") + Review Comment: Request JSON parsing uses `json.get("...")` for required fields (`wid`, `vid`, `mapping`, `notebook`) without validation. If the client sends malformed JSON or omits fields, this will throw and return a 500. Please validate inputs and return a 400 Bad Request with a clear error when required fields are missing/invalid. ########## notebook-migration-service/build.sbt: ########## @@ -0,0 +1,80 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +import scala.collection.Seq + +name := "notebook-migration-service" +organization := "org.apache" +version := "1.0.0" + +scalaVersion := "2.13.12" + +enablePlugins(JavaAppPackaging) + +// Enable semanticdb for Scalafix +ThisBuild / semanticdbEnabled := true +ThisBuild / semanticdbVersion := scalafixSemanticdb.revision + +// Manage dependency conflicts by always using the latest revision +ThisBuild / conflictManager := ConflictManager.latestRevision + +// Restrict parallel execution of tests to avoid conflicts +Global / concurrentRestrictions += Tags.limit(Tags.Test, 1) + +///////////////////////////////////////////////////////////////////////////// +// Compiler Options +///////////////////////////////////////////////////////////////////////////// + +// Scala compiler options +Compile / scalacOptions ++= Seq( + "-Xelide-below", "WARNING", // Turn on optimizations with "WARNING" as the threshold + "-feature", // Check feature warnings + "-deprecation", // Check deprecation warnings + "-Ywarn-unused:imports" // Check for unused imports +) + +///////////////////////////////////////////////////////////////////////////// +// Version Variables +///////////////////////////////////////////////////////////////////////////// + +val dropwizardVersion = "4.0.7" +val mockitoVersion = "5.4.0" +val assertjVersion = "3.24.2" + +///////////////////////////////////////////////////////////////////////////// +// Test-related Dependencies +///////////////////////////////////////////////////////////////////////////// + +libraryDependencies ++= Seq( + "org.scalamock" %% "scalamock" % "5.2.0" % Test, // ScalaMock + "org.scalatest" %% "scalatest" % "3.2.17" % Test, // ScalaTest + "io.dropwizard" % "dropwizard-testing" % dropwizardVersion % Test, // Dropwizard Testing + "org.mockito" % "mockito-core" % mockitoVersion % Test, // Mockito for mocking + "org.assertj" % "assertj-core" % assertjVersion % Test, // AssertJ for assertions + "com.novocode" % "junit-interface" % "0.11" % Test // SBT interface for JUnit +) + +///////////////////////////////////////////////////////////////////////////// +// Dependencies +///////////////////////////////////////////////////////////////////////////// + +// Core Dependencies +libraryDependencies ++= Seq( + "io.dropwizard" % "dropwizard-core" % dropwizardVersion, + "io.dropwizard" % "dropwizard-auth" % dropwizardVersion, // Dropwizard Authentication module + "com.fasterxml.jackson.module" %% "jackson-module-scala" % "2.15.2" Review Comment: This service declares `jackson-module-scala` 2.15.2, but other services use newer Jackson (e.g., 2.18.x) and the root build adds an override for this project. To avoid runtime incompatibilities with Dropwizard 4 and to keep dependency management consistent, please bump this dependency to match the repo-wide Jackson version and remove the need for a special-case override. ```suggestion "com.fasterxml.jackson.module" %% "jackson-module-scala" % "2.18.2" ``` ########## notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala: ########## @@ -0,0 +1,324 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.dao.SqlServer +import org.jooq.JSONB +import org.apache.texera.dao.jooq.generated.tables.Notebook +import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import java.net.{HttpURLConnection, URL} +import java.nio.charset.StandardCharsets +import scala.util.control.NonFatal +import org.apache.texera.amber.config.StorageConfig + +object NotebookMigrationResource extends LazyLogging { + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) + private val jupyterUrl = StorageConfig.jupyterURL + private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + private def isJupyterAvailable(jupyterUrl: String): Boolean = { + try { + val conn = new java.net.URL(s"$jupyterUrl/api") + .openConnection() + .asInstanceOf[java.net.HttpURLConnection] + + conn.setRequestMethod("GET") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) + + val status = conn.getResponseCode + + status == 200 || status == 403 + } catch { + case _: Exception => false + } + } + + // Returns the Jupyter iframe reference URL + def getJupyterIframeURL(): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterIframeURL" + } + """ + ).build() + } + + // 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() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterUrl" + } + """ + ).build() + } + + // Set the notebook in Jupyter + def setNotebook(body: String): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + try { + val json = mapper.readTree(body) + + val notebookName = json.get("notebookName").asText() + val notebookData = json.get("notebookData") + + // Construct Jupyter API URL + val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName" + + val url = new URL(apiUrl) + val conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("PUT") + conn.setDoOutput(true) + conn.setRequestProperty("Content-Type", "application/json") + + val requestBody = + s""" + { + "type": "notebook", + "content": $notebookData + } + """ + + val os = conn.getOutputStream + os.write(requestBody.getBytes(StandardCharsets.UTF_8)) + os.flush() + os.close() + + val status = conn.getResponseCode + + if (status != 200 && status != 201) { + return Response.status(500).entity( + s""" + { + "success": false, + "message": "Failed to upload notebook to Jupyter (status $status)" + } + """ + ).build() + } + + jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + Response.ok( + s""" + { + "success": true, + "message": "Notebook successfully sent to Jupyter." + } + """ + ).build() + + } catch { + case NonFatal(e) => + logger.error("Error sending notebook to Jupyter", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(s"""{"error":"${e.getMessage}"}""") + .build() + } + } + + // Store notebook + mapping in database + def storeNotebookAndMapping(body: String): Response = { + try { + val json = mapper.readTree(body) + + val wid: java.lang.Integer = json.get("wid").asInt() + val vid: java.lang.Integer = json.get("vid").asInt() + val mappingNode = json.get("mapping") + val notebookNode = json.get("notebook") + + val dsl = SqlServer.getInstance().createDSLContext() + + val nid: java.lang.Integer = SqlServer.withTransaction(dsl) { ctx => + // Insert notebook + val notebookRecord = ctx.insertInto(Notebook.NOTEBOOK) + .set(Notebook.NOTEBOOK.WID, wid) + .set(Notebook.NOTEBOOK.NOTEBOOK_, JSONB.valueOf(notebookNode.toString)) + .returning(Notebook.NOTEBOOK.NID) + .fetchOne() + + val nidInside: java.lang.Integer = notebookRecord.getValue(Notebook.NOTEBOOK.NID) + + // Insert workflow-notebook mapping + ctx.insertInto(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.WID, wid) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.VID, vid) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.NID, nidInside) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.MAPPING, JSONB.valueOf(mappingNode.toString)) + .execute() + + nidInside + } + + Response.ok( + s""" + { + "success": true, + "message": "Notebook and mapping successfully stored. wid: $wid, vid: $vid, nid: $nid" + } + """ + ).build() + + } catch { + case NonFatal(e) => + logger.error("Error storing mapping and workflow", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(s"""{"error":"${e.getMessage}"}""") + .build() + } + } + + + // Fetch notebook + mapping + def fetchNotebookAndMapping(body: String): Response = { + try { + val json = mapper.readTree(body) + + val wid: java.lang.Integer = json.get("wid").asInt() + val vid: java.lang.Integer = json.get("vid").asInt() + + val dsl = SqlServer.getInstance().createDSLContext() + + // Fetch the most recent notebook (highest nid) for this workflow version + val result = dsl.select( + Notebook.NOTEBOOK.NID, + Notebook.NOTEBOOK.NOTEBOOK_, + WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.MAPPING + ) + .from(Notebook.NOTEBOOK) + .join(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING) + .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() + + if (result == null) { + Response.ok("""{"exists": false}""").build() + } else { + val nid: Int = result.getValue(Notebook.NOTEBOOK.NID) Review Comment: `val nid: Int = ...` is assigned but never used in the response. Please remove it (or include `nid` in the returned JSON if callers need it) to avoid dead code and keep the method focused. ```suggestion ``` ########## notebook-migration-service/src/main/scala/org/apache/texera/service/NotebookMigrationService.scala: ########## @@ -0,0 +1,88 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service + +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import io.dropwizard.auth.AuthDynamicFeature +import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider} +import io.dropwizard.core.Application +import io.dropwizard.core.setup.{Bootstrap, Environment} +import org.apache.texera.amber.config.StorageConfig +import org.apache.texera.auth.{JwtAuthFilter, RequestLoggingFilter, SessionUser} +import org.apache.texera.dao.SqlServer +import org.eclipse.jetty.server.session.SessionHandler +import java.nio.file.Path +import org.apache.texera.service.resource.NotebookMigrationResource + +class NotebookMigrationService extends Application[NotebookMigrationServiceConfiguration] with LazyLogging { + override def initialize(bootstrap: Bootstrap[NotebookMigrationServiceConfiguration]): Unit = { + // enable environment variable substitution in YAML config + bootstrap.setConfigurationSourceProvider( + new SubstitutingSourceProvider( + bootstrap.getConfigurationSourceProvider, + new EnvironmentVariableSubstitutor(false) + ) + ) + // Register Scala module to Dropwizard default object mapper + bootstrap.getObjectMapper.registerModule(DefaultScalaModule) + + SqlServer.initConnection( + StorageConfig.jdbcUrl, + StorageConfig.jdbcUsername, + StorageConfig.jdbcPassword + ) + } + + override def run( + configuration: NotebookMigrationServiceConfiguration, + environment: Environment + ): Unit = { + // Serve backend at /api + environment.jersey.setUrlPattern("/api/*") + + environment.jersey.register(classOf[NotebookMigrationResource]) + + // Register JWT authentication filter + // environment.jersey.register(new AuthDynamicFeature(classOf[JwtAuthFilter])) + + // Enable @Auth annotation for injecting SessionUser + // environment.jersey.register( + // new io.dropwizard.auth.AuthValueFactoryProvider.Binder(classOf[SessionUser]) + // ) Review Comment: The service currently does not register the JWT auth filter / `@Auth` binder (unlike other backend services such as `AccessControlService`). That means all notebook migration endpoints are unauthenticated by default, including DB writes and Jupyter upload calls. Please enable JWT auth here and update the resource methods to require `@Auth SessionUser` (and enforce workflow-level authorization on `wid`/`vid`). ```suggestion environment.jersey.register(new AuthDynamicFeature(classOf[JwtAuthFilter])) // Enable @Auth annotation for injecting SessionUser environment.jersey.register( new io.dropwizard.auth.AuthValueFactoryProvider.Binder(classOf[SessionUser]) ) ``` ########## notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala: ########## @@ -0,0 +1,324 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.dao.SqlServer +import org.jooq.JSONB +import org.apache.texera.dao.jooq.generated.tables.Notebook +import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import java.net.{HttpURLConnection, URL} +import java.nio.charset.StandardCharsets +import scala.util.control.NonFatal +import org.apache.texera.amber.config.StorageConfig + +object NotebookMigrationResource extends LazyLogging { + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) + private val jupyterUrl = StorageConfig.jupyterURL + private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + private def isJupyterAvailable(jupyterUrl: String): Boolean = { + try { + val conn = new java.net.URL(s"$jupyterUrl/api") + .openConnection() + .asInstanceOf[java.net.HttpURLConnection] + + conn.setRequestMethod("GET") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) + + val status = conn.getResponseCode + + status == 200 || status == 403 + } catch { + case _: Exception => false + } + } + + // Returns the Jupyter iframe reference URL + def getJupyterIframeURL(): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterIframeURL" + } + """ + ).build() + } + + // 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() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterUrl" + } + """ + ).build() + } + + // Set the notebook in Jupyter + def setNotebook(body: String): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + try { + val json = mapper.readTree(body) + + val notebookName = json.get("notebookName").asText() + val notebookData = json.get("notebookData") + + // Construct Jupyter API URL + val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName" + + val url = new URL(apiUrl) + val conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("PUT") + conn.setDoOutput(true) + conn.setRequestProperty("Content-Type", "application/json") + + val requestBody = + s""" + { + "type": "notebook", + "content": $notebookData + } + """ + + val os = conn.getOutputStream + os.write(requestBody.getBytes(StandardCharsets.UTF_8)) + os.flush() + os.close() + + val status = conn.getResponseCode + + if (status != 200 && status != 201) { + return Response.status(500).entity( + s""" + { + "success": false, + "message": "Failed to upload notebook to Jupyter (status $status)" + } + """ + ).build() + } + + jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + Response.ok( + s""" + { + "success": true, + "message": "Notebook successfully sent to Jupyter." + } + """ + ).build() + + } catch { + case NonFatal(e) => + logger.error("Error sending notebook to Jupyter", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(s"""{"error":"${e.getMessage}"}""") + .build() + } + } + + // Store notebook + mapping in database + def storeNotebookAndMapping(body: String): Response = { + try { + val json = mapper.readTree(body) Review Comment: No tests are added for this new resource/service. Since the repo uses Dropwizard resource specs in other services, please add tests covering at least: Jupyter unavailable (returns 5xx/appropriate status), valid notebook upload, invalid/missing fields (400), and DB store/fetch roundtrip. ########## common/config/src/main/scala/org/apache/texera/amber/config/StorageConfig.scala: ########## @@ -129,4 +129,7 @@ object StorageConfig { val ENV_S3_REGION = "STORAGE_S3_REGION" val ENV_S3_AUTH_USERNAME = "STORAGE_S3_AUTH_USERNAME" val ENV_S3_AUTH_PASSWORD = "STORAGE_S3_AUTH_PASSWORD" + + // Jupyter + val jupyterURL: String = conf.getString("storage.jupyter.url") Review Comment: `StorageConfig` uses `jdbcUrl`/`jdbcUrlForTestCases` naming, but this introduces `jupyterURL` with inconsistent casing. Consider renaming to `jupyterUrl` to match the established style and avoid awkward call sites. ```suggestion val jupyterUrl: String = conf.getString("storage.jupyter.url") ``` ########## sql/updates/23.sql: ########## @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +-- ============================================ +-- 1. Connect to the texera_db database +-- ============================================ +\c texera_db + +SET search_path TO texera_db; + +-- ============================================ +-- 2. Delete tables if they already exist +-- ============================================ + +BEGIN; + +DROP TABLE IF EXISTS notebook CASCADE; +DROP TABLE IF EXISTS workflow_notebook_mapping CASCADE; + +-- ============================================ +-- 3. Create the tables to store notebook and mapping +-- ============================================ + +CREATE TABLE notebook ( + nid SERIAL NOT NULL PRIMARY KEY, + wid INT NOT NULL, + notebook JSONB NOT NULL, + FOREIGN KEY (wid) REFERENCES workflow(wid) ON DELETE CASCADE +); + +CREATE TABLE workflow_notebook_mapping ( Review Comment: `DROP TABLE IF EXISTS ... CASCADE` in a numbered SQL update is destructive and will erase any existing notebook/mapping data if the migration is re-run (or if a deployment mistakenly applies it on an environment with existing data). SQL update scripts in this repo are generally additive/transformative (e.g., `ALTER TABLE ... ADD COLUMN IF NOT EXISTS`) rather than dropping tables. Please remove the drops and make the migration idempotent via `CREATE TABLE IF NOT EXISTS` (and `ALTER TABLE` for any future changes). ```suggestion -- 2. Create tables if they do not already exist -- ============================================ BEGIN; -- ============================================ -- 3. Create the tables to store notebook and mapping -- ============================================ CREATE TABLE IF NOT EXISTS notebook ( nid SERIAL NOT NULL PRIMARY KEY, wid INT NOT NULL, notebook JSONB NOT NULL, FOREIGN KEY (wid) REFERENCES workflow(wid) ON DELETE CASCADE ); CREATE TABLE IF NOT EXISTS workflow_notebook_mapping ( ``` ########## notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala: ########## @@ -0,0 +1,324 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.dao.SqlServer +import org.jooq.JSONB +import org.apache.texera.dao.jooq.generated.tables.Notebook +import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import java.net.{HttpURLConnection, URL} +import java.nio.charset.StandardCharsets +import scala.util.control.NonFatal +import org.apache.texera.amber.config.StorageConfig + +object NotebookMigrationResource extends LazyLogging { + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) + private val jupyterUrl = StorageConfig.jupyterURL + private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + Review Comment: `jupyterIframeURL` is a mutable singleton (`var`) shared across all requests/users. In a multi-user service this can cause cross-user notebook URL leakage and race conditions (last writer wins). Please avoid global mutable state here; compute the iframe URL per request (based on the notebook name) or persist it per workflow/user in the DB. ########## notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala: ########## @@ -0,0 +1,324 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.dao.SqlServer +import org.jooq.JSONB +import org.apache.texera.dao.jooq.generated.tables.Notebook +import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import java.net.{HttpURLConnection, URL} +import java.nio.charset.StandardCharsets +import scala.util.control.NonFatal +import org.apache.texera.amber.config.StorageConfig + +object NotebookMigrationResource extends LazyLogging { + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) + private val jupyterUrl = StorageConfig.jupyterURL + private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + private def isJupyterAvailable(jupyterUrl: String): Boolean = { + try { + val conn = new java.net.URL(s"$jupyterUrl/api") + .openConnection() + .asInstanceOf[java.net.HttpURLConnection] + + conn.setRequestMethod("GET") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) + + val status = conn.getResponseCode + + status == 200 || status == 403 + } catch { + case _: Exception => false + } + } + + // Returns the Jupyter iframe reference URL + def getJupyterIframeURL(): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterIframeURL" + } + """ + ).build() + } + + // 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() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterUrl" + } + """ + ).build() + } + + // Set the notebook in Jupyter + def setNotebook(body: String): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + try { + val json = mapper.readTree(body) + + val notebookName = json.get("notebookName").asText() + val notebookData = json.get("notebookData") + + // Construct Jupyter API URL + val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName" + + val url = new URL(apiUrl) + val conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("PUT") + conn.setDoOutput(true) + conn.setRequestProperty("Content-Type", "application/json") + + val requestBody = + s""" + { + "type": "notebook", + "content": $notebookData + } + """ + + val os = conn.getOutputStream + os.write(requestBody.getBytes(StandardCharsets.UTF_8)) + os.flush() + os.close() + + val status = conn.getResponseCode + + if (status != 200 && status != 201) { + return Response.status(500).entity( + s""" + { + "success": false, + "message": "Failed to upload notebook to Jupyter (status $status)" + } + """ + ).build() + } + + jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + Response.ok( + s""" + { + "success": true, + "message": "Notebook successfully sent to Jupyter." + } + """ + ).build() + + } catch { + case NonFatal(e) => + logger.error("Error sending notebook to Jupyter", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(s"""{"error":"${e.getMessage}"}""") + .build() + } + } + + // Store notebook + mapping in database + def storeNotebookAndMapping(body: String): Response = { + try { + val json = mapper.readTree(body) + + val wid: java.lang.Integer = json.get("wid").asInt() + val vid: java.lang.Integer = json.get("vid").asInt() + val mappingNode = json.get("mapping") + val notebookNode = json.get("notebook") + + val dsl = SqlServer.getInstance().createDSLContext() + + val nid: java.lang.Integer = SqlServer.withTransaction(dsl) { ctx => + // Insert notebook + val notebookRecord = ctx.insertInto(Notebook.NOTEBOOK) + .set(Notebook.NOTEBOOK.WID, wid) + .set(Notebook.NOTEBOOK.NOTEBOOK_, JSONB.valueOf(notebookNode.toString)) + .returning(Notebook.NOTEBOOK.NID) + .fetchOne() + + val nidInside: java.lang.Integer = notebookRecord.getValue(Notebook.NOTEBOOK.NID) + + // Insert workflow-notebook mapping + ctx.insertInto(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.WID, wid) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.VID, vid) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.NID, nidInside) + .set(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.MAPPING, JSONB.valueOf(mappingNode.toString)) + .execute() + + nidInside + } + + Response.ok( + s""" + { + "success": true, + "message": "Notebook and mapping successfully stored. wid: $wid, vid: $vid, nid: $nid" + } + """ + ).build() + + } catch { + case NonFatal(e) => + logger.error("Error storing mapping and workflow", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(s"""{"error":"${e.getMessage}"}""") + .build() + } + } + + + // Fetch notebook + mapping + def fetchNotebookAndMapping(body: String): Response = { + try { + val json = mapper.readTree(body) + + val wid: java.lang.Integer = json.get("wid").asInt() + val vid: java.lang.Integer = json.get("vid").asInt() + + val dsl = SqlServer.getInstance().createDSLContext() + + // Fetch the most recent notebook (highest nid) for this workflow version + val result = dsl.select( + Notebook.NOTEBOOK.NID, + Notebook.NOTEBOOK.NOTEBOOK_, + WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.MAPPING + ) + .from(Notebook.NOTEBOOK) + .join(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING) + .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() + + if (result == null) { + Response.ok("""{"exists": false}""").build() + } else { + val nid: Int = result.getValue(Notebook.NOTEBOOK.NID) + val notebookJson: String = result.get(Notebook.NOTEBOOK.NOTEBOOK_).asInstanceOf[JSONB].data() + val mappingJson: String = result.get(WorkflowNotebookMapping.WORKFLOW_NOTEBOOK_MAPPING.MAPPING).asInstanceOf[JSONB].data() + + Response.ok( + s""" + { + "exists": true, + "notebook": $notebookJson, + "mapping": $mappingJson + } + """ + ).build() + } + + } catch { + case NonFatal(e) => + logger.error("Database error retrieving mapping", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(s"""{"error":"${e.getMessage}"}""") + .build() + } + } +} + +@Path("/notebook-migration") +@Produces(Array(MediaType.APPLICATION_JSON)) +@Consumes(Array(MediaType.APPLICATION_JSON)) +class NotebookMigrationResource extends LazyLogging { + + @GET + @Path("/get-jupyter-iframe-url") + def getJupyterIframeURL: Response = { + logger.info("Getting Jupyter iframe URL") + NotebookMigrationResource.getJupyterIframeURL() + } + + @GET + @Path("/get-jupyter-url") + def getJupyterURL: Response = { + logger.info("Getting Jupyter API URL") + NotebookMigrationResource.getJupyterURL() + } + + @POST + @Path("/set-notebook") + def setNotebook(body: String): Response = { + logger.info("Setting notebook, request body: " + body) + NotebookMigrationResource.setNotebook(body) + } + + @POST + @Path("/store-notebook-and-mapping") + def storeNotebookAndMapping(body: String): Response = { + logger.info("Storing notebook and mapping, request body: " + body) + NotebookMigrationResource.storeNotebookAndMapping(body) + } + + @POST + @Path("/fetch-notebook-and-mapping") + def fetchNotebookAndMapping(body: String): Response = { + logger.info("Fetching notebook and mapping, request body: " + body) + NotebookMigrationResource.fetchNotebookAndMapping(body) Review Comment: `NotebookMigrationResource` logs full request bodies (including notebook JSON and potentially user code / secrets). This can bloat logs and leak sensitive content into log storage. Please remove body logging or log only non-sensitive metadata (e.g., wid/vid and notebookName) with truncation. ########## notebook-migration-service/src/main/scala/org/apache/texera/service/NotebookMigrationService.scala: ########## @@ -0,0 +1,88 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service + +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import io.dropwizard.auth.AuthDynamicFeature +import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider} +import io.dropwizard.core.Application +import io.dropwizard.core.setup.{Bootstrap, Environment} +import org.apache.texera.amber.config.StorageConfig +import org.apache.texera.auth.{JwtAuthFilter, RequestLoggingFilter, SessionUser} +import org.apache.texera.dao.SqlServer +import org.eclipse.jetty.server.session.SessionHandler Review Comment: Several imports are currently unused because the authentication/session handler registration is commented out (`AuthDynamicFeature`, `JwtAuthFilter`, `SessionUser`, `SessionHandler`). The repo runs Scalafix `RemoveUnused`, so this will likely fail formatting/lint checks. Please either enable the auth code (preferred) or remove the unused imports. ```suggestion import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider} import io.dropwizard.core.Application import io.dropwizard.core.setup.{Bootstrap, Environment} import org.apache.texera.amber.config.StorageConfig import org.apache.texera.auth.RequestLoggingFilter import org.apache.texera.dao.SqlServer ``` ########## notebook-migration-service/src/main/resources/docker-compose.yml: ########## @@ -0,0 +1,34 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: texera-jupyter +services: + + jupyter: + build: + context: . + dockerfile: Dockerfile Review Comment: `docker-compose.yml` references `dockerfile: Dockerfile`, but the added file is named `dockerfile` (lowercase). On case-sensitive filesystems this will fail to build. Please rename the file to `Dockerfile` or update the compose file to match the actual filename. ```suggestion dockerfile: dockerfile ``` ########## notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala: ########## @@ -0,0 +1,324 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.texera.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import com.typesafe.scalalogging.LazyLogging +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.dao.SqlServer +import org.jooq.JSONB +import org.apache.texera.dao.jooq.generated.tables.Notebook +import org.apache.texera.dao.jooq.generated.tables.WorkflowNotebookMapping +import java.net.{HttpURLConnection, URL} +import java.nio.charset.StandardCharsets +import scala.util.control.NonFatal +import org.apache.texera.amber.config.StorageConfig + +object NotebookMigrationResource extends LazyLogging { + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) + private val jupyterUrl = StorageConfig.jupyterURL + private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" + + private def isJupyterAvailable(jupyterUrl: String): Boolean = { + try { + val conn = new java.net.URL(s"$jupyterUrl/api") + .openConnection() + .asInstanceOf[java.net.HttpURLConnection] + + conn.setRequestMethod("GET") + conn.setConnectTimeout(2000) + conn.setReadTimeout(2000) + + val status = conn.getResponseCode + + status == 200 || status == 403 + } catch { + case _: Exception => false + } + } + + // Returns the Jupyter iframe reference URL + def getJupyterIframeURL(): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterIframeURL" + } + """ + ).build() + } + + // 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() + } + + Response.ok( + s""" + { + "success": true, + "url": "$jupyterUrl" + } + """ + ).build() + } + + // Set the notebook in Jupyter + def setNotebook(body: String): Response = { + if (!isJupyterAvailable(jupyterUrl)) { + return Response.status(500).entity( + """ + { + "success": false, + "message": "Cannot connect to Jupyter server" + } + """ + ).build() + } + + try { + val json = mapper.readTree(body) + + val notebookName = json.get("notebookName").asText() + val notebookData = json.get("notebookData") + + // Construct Jupyter API URL + val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName" + + val url = new URL(apiUrl) + val conn = url.openConnection().asInstanceOf[HttpURLConnection] + + conn.setRequestMethod("PUT") + conn.setDoOutput(true) + conn.setRequestProperty("Content-Type", "application/json") + + val requestBody = + s""" + { + "type": "notebook", + "content": $notebookData + } + """ + + val os = conn.getOutputStream + os.write(requestBody.getBytes(StandardCharsets.UTF_8)) + os.flush() + os.close() + + val status = conn.getResponseCode + + if (status != 200 && status != 201) { + return Response.status(500).entity( + s""" + { + "success": false, + "message": "Failed to upload notebook to Jupyter (status $status)" + } + """ + ).build() + } + + jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb" Review Comment: After uploading a notebook named `notebookName` to Jupyter, the iframe URL is always reset to `/notebooks/work/notebook.ipynb` rather than reflecting the uploaded name. This will produce the wrong iframe link when `notebookName` is not exactly `notebook.ipynb`. Please set the iframe URL using the actual notebook name/path (or enforce a single canonical name consistently on both upload and iframe URL generation). ```suggestion jupyterIframeURL = s"$jupyterUrl/notebooks/work/$notebookName" ``` ########## notebook-migration-service/build.sbt: ########## @@ -0,0 +1,80 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +import scala.collection.Seq + +name := "notebook-migration-service" +organization := "org.apache" +version := "1.0.0" + +scalaVersion := "2.13.12" Review Comment: This module sets `scalaVersion := "2.13.12"`, while other services in this repo use Scala 2.13.18. Mixing patch versions across subprojects can lead to confusing dependency/ABI issues. Please align this service's Scala version with the rest of the build (2.13.18). ```suggestion scalaVersion := "2.13.18" ``` ########## notebook-migration-service/src/main/resources/custom.js: ########## @@ -0,0 +1,95 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Use Jupyter's event system to ensure the notebook is fully loaded +require(["base/js/events"], function (events) { + events.on("kernel_ready.Kernel", function () { + + // Attach click event listener to cells + $("#notebook-container").on("click", ".cell", function (event) { + const cell = $(this); + const index = $(".cell").index(cell); + const cellContent = cell.find(".input_area").text(); + + // Get the UUID from the cell's metadata, or use "N/A" if it doesn't exist + const cellUUID = Jupyter.notebook.get_cell(index).metadata.uuid || 'N/A'; + + // Send a message to the parent window (Texera app) + window.parent.postMessage( + { action: "cellClicked", cellIndex: index, cellContent: cellContent, cellUUID: cellUUID }, + "http://localhost:4200"; + ); + }); + }); +}); + +// Listen for messages from the Texera app (or parent window) +window.addEventListener("message", function (event) { + // Verify the message origin + if (event.origin !== 'http://localhost:4200') { + console.warn("Message received from unrecognized origin:", event.origin); + return; Review Comment: `custom.js` hardcodes the parent origin (`http://localhost:4200`) both for `postMessage` and for origin verification. This will break when Texera is served from a different host/port (and makes it easy to misconfigure in deployments). Please make the allowed origin configurable (e.g., via an env var injected into Jupyter, or derive from `document.referrer` with a strict allowlist) and avoid hardcoding localhost. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
