You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "zyratlo (via GitHub)" <gi...@apache.org> on 2026/05/28 03:58:08 UTC
[PR] feat(python-notebook-migration, backend): add notebook-migration-service microservice in backend [texera]
zyratlo opened a new pull request, #5258:
URL: https://github.com/apache/texera/pull/5258
### What changes were proposed in this PR?
Introduces the microservice that mediates between Texera and the JupyterLab docker stack landed in `migration-tool-jupyter-docker`. Adds a new SBT subproject `notebook-migration-service` plus shared config and a frontend dev-proxy route.
**New SBT subproject `notebook-migration-service/`:**
- **`build.sbt`** and **`project/build.properties`** — module SBT setup; module depends on the existing `Auth`, `Config`, and `DAO` projects
- **`src/main/scala/.../NotebookMigrationService.scala`** — Dropwizard `Application` entry point; sets Jersey URL pattern to `/api/*`, registers the resource class, initializes the shared SQL connection via
`SqlServer.initConnection(StorageConfig.jdbcUrl, …)`, and wires in `RequestLoggingFilter`.
- **`src/main/scala/.../NotebookMigrationServiceConfiguration.scala`** — Dropwizard `Configuration` subclass.
- **`src/main/scala/.../resource/NotebookMigrationResource.scala`** — five REST endpoints under `/notebook-migration`:
- `GET /get-jupyter-url` — health-checks the Jupyter container and returns its base URL.
- `GET /get-jupyter-iframe-url` — returns the iframe-ready URL for `notebook.ipynb`.
- `POST /set-notebook` — receives a notebook JSON, PUTs it into JupyterLab via its `/api/contents/work/{name}` API.
- `POST /store-notebook-and-mapping` — persists a notebook + workflow-notebook mapping into Postgres in a single transaction (writes to the `notebook` and `workflow_notebook_mapping` tables added by `migration-tool-database-tables`).
- `POST /fetch-notebook-and-mapping` — returns the most recent notebook + mapping for a given (wid, vid).
- **`src/main/resources/logback.xml`** — logging config.
- **`src/main/resources/notebook-migration-service-web-config.yaml`** — Dropwizard server config (HTTP port `9098`, DB connection refs).
**Root build wiring:**
- **`build.sbt`** — declares the new `NotebookMigrationService` SBT subproject and adds it to the `TexeraProject` aggregation.
**Shared config:**
- **`common/config/src/main/resources/storage.conf`** — new `jupyter { url = "http://localhost:9100" }` block, overridable via `STORAGE_JUPYTER_URL`.
- **`common/config/src/main/scala/.../StorageConfig.scala`** — adds the `jupyterURL` accessor.
**Frontend dev proxy:**
- **`frontend/proxy.config.json`** — routes `/api/notebook-migration/*` to `http://localhost:9098`.
### Any related issues, documentation, discussions?
Closes #5257
Parent issue #4301
- Hard dependency: **must be merged after `migration-tool-database-tables`** #5055 — the resource imports jOOQ-generated `Notebook` / `WorkflowNotebookMapping` classes that only exist once the schema PR is merged.
- Soft dependency: the JupyterLab container from `migration-tool-jupyter-docker` is what `StorageConfig.jupyterURL` points to. Without it running, the Jupyter-related endpoints return a 500 with `"Cannot connect to Jupyter server"`. Service still starts and the DB-persistence endpoints work in isolation.
### How was this PR tested?
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.7)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3431919217
##########
notebook-migration-service/build.sbt:
##########
@@ -0,0 +1,77 @@
+// 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"
+
+
+enablePlugins(JavaAppPackaging)
Review Comment:
I added the `distMappings` block after `enablePlugins(JavaAppPackaging)`, matching the other services. The dist zip now ships LICENSE, NOTICE, DISCLAIMER, and licenses/ at its top level.
Two per-module files are added:
- NOTICE-binary
- LICENSE-binary
I also registered the service in `PER_MODULE_LICENSE_BINARIES` and added it to the `build.yml` per-service matrix. CI now runs this service's dist + the `check_binary_deps/NOTICE-binary` drift check (and its tests) like the other services.
Commits: 01b731b (files), 0fe0df8 (register + CI matrix), 3d112c9 (UTF-8 fix).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5258:
URL: https://github.com/apache/texera/pull/5258#issuecomment-4711637715
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 10 worse · ⚪ 5 noise (<±5%) · 0 without baseline
> Compared against main `891d2ad` benchmarked on **this same runner**, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/27570699028)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 405 | 0.247 | 23,413/36,190/36,190 us | 🔴 +9.3% / ⚪ within ±5% |
| 🔴 | bs=100 sw=10 sl=64 | 920 | 0.562 | 113,140/143,208/143,208 us | 🔴 +19.6% / ⚪ within ±5% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,099 | 0.671 | 909,583/949,827/949,827 us | ⚪ within ±5% / 🟢 -7.2% |
<details><summary>Baseline details</summary>
Latest main `891d2ad` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 405 tuples/sec | 434 tuples/sec | 410.82 tuples/sec | -6.7% | -1.4% |
| bs=10 sw=10 sl=64 | MB/s | 0.247 MB/s | 0.265 MB/s | 0.251 MB/s | -6.8% | -1.5% |
| bs=10 sw=10 sl=64 | p50 | 23,413 us | 21,414 us | 23,785 us | +9.3% | -1.6% |
| bs=10 sw=10 sl=64 | p95 | 36,190 us | 33,824 us | 34,980 us | +7.0% | +3.5% |
| bs=10 sw=10 sl=64 | p99 | 36,190 us | 33,824 us | 34,980 us | +7.0% | +3.5% |
| bs=100 sw=10 sl=64 | throughput | 920 tuples/sec | 971 tuples/sec | 891.94 tuples/sec | -5.3% | +3.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.562 MB/s | 0.593 MB/s | 0.544 MB/s | -5.2% | +3.2% |
| bs=100 sw=10 sl=64 | p50 | 113,140 us | 102,487 us | 112,277 us | +10.4% | +0.8% |
| bs=100 sw=10 sl=64 | p95 | 143,208 us | 119,696 us | 139,802 us | +19.6% | +2.4% |
| bs=100 sw=10 sl=64 | p99 | 143,208 us | 119,696 us | 139,802 us | +19.6% | +2.4% |
| bs=1000 sw=10 sl=64 | throughput | 1,099 tuples/sec | 1,085 tuples/sec | 1,041 tuples/sec | +1.3% | +5.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.671 MB/s | 0.663 MB/s | 0.635 MB/s | +1.2% | +5.6% |
| bs=1000 sw=10 sl=64 | p50 | 909,583 us | 919,939 us | 972,714 us | -1.1% | -6.5% |
| bs=1000 sw=10 sl=64 | p95 | 949,827 us | 967,952 us | 1,023,057 us | -1.9% | -7.2% |
| bs=1000 sw=10 sl=64 | p99 | 949,827 us | 967,952 us | 1,023,057 us | -1.9% | -7.2% |
</details>
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,493.24,200,128000,405,0.247,23412.50,36190.37,36190.37
1,100,10,64,20,2173.26,2000,1280000,920,0.562,113139.63,143207.60,143207.60
2,1000,10,64,20,18194.27,20000,12800000,1099,0.671,909583.22,949827.03,949827.03
```
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3461814259
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
Review Comment:
This value is already regex verified to be valid so there wouldn't be a case of invalid JSON. However, to future-proof for possible refactoring of this function it is OK to make this change as it doesn't affect anything now. Added in [319f0a3](https://github.com/apache/texera/pull/5258/commits/319f0a32abebd9060c4d81786a5a8040d62ad044)
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
Review Comment:
This value is already regex verified to be valid so there wouldn't be a case of invalid JSON. However, to future-proof for possible refactoring of this function it is OK to make this change as it doesn't affect anything now. Added in [319f0a3](https://github.com/apache/texera/pull/5258/commits/319f0a32abebd9060c4d81786a5a8040d62ad044)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3430685977
##########
notebook-migration-service/src/main/resources/logback.xml:
##########
@@ -0,0 +1,55 @@
+<!--
+ 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.
+-->
+
+<configuration>
+ <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
Review Comment:
Inconsistency: 5/6 existing services in this repo (agent-service, config-service, file-service, workflow-compiling-service, computing-unit-managing-service) configure logging only through the Dropwizard YAML — no separate logback.xml. Only access-control-service has both, which looks like a historical copy-paste. Dropwizard's official docs recommend against having both, since Dropwizard's YAML config reapplies logback configuration at startup and logback.xml becomes either dead weight or a source of duplicate appenders. Suggest dropping this logback.xml and keeping logging config solely in notebook-migration-service-web-config.yaml. Could you check on this?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3431303135
##########
notebook-migration-service/build.sbt:
##########
@@ -0,0 +1,77 @@
+// 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"
+
+
+enablePlugins(JavaAppPackaging)
Review Comment:
All **6** other dist-producing services in this repo (`access-control-service`, `config-service`, `file-service`, `workflow-compiling-service`, `computing-unit-managing-service`, and `amber`) include a `Universal / mappings := AddMetaInfLicenseFiles.distMappings(...)` block right after `enablePlugins(JavaAppPackaging)`. This service is the only one without it.
The root `build.sbt` comment (lines 58-59) is explicit: "Dist-producing modules additionally override `Universal / mappings` in their own build.sbt (not here) — see `AddMetaInfLicenseFiles.distMappings`." Without this block, the `notebook-migration-service` dist zip won't have `LICENSE-binary`, `NOTICE-binary`, `DISCLAIMER`, and `licenses/` at its top level — an ASF binary-distribution compliance requirement for the incubating project. The `asfLicensingSettings` applied at root covers META-INF inside each JAR, but not the dist zip's top-level files. The common-library modules (`common/auth`, `common/config`, `common/dao`, etc.) correctly don't have this block — they don't ship dist zips — but this is a service, so it should.
##########
build.sbt:
##########
@@ -169,6 +169,16 @@ lazy val WorkflowExecutionService = (project in file("amber"))
)
.configs(Test)
.dependsOn(DAO % "test->test", Auth % "test->test") // test scope dependency
+lazy val NotebookMigrationService = (project in file("notebook-migration-service"))
+ .dependsOn(Auth, Config, DAO)
+ .settings(asfLicensingSettings)
+ .settings(
+ dependencyOverrides ++= Seq(
+ // override it as io.dropwizard 4 require 2.16.1 or higher
+ "com.fasterxml.jackson.module" %% "jackson-module-scala" % "2.17.0"
Review Comment:
This override hardcodes `jackson-module-scala = 2.17.0`, but the rest of the repo uses `val jacksonVersion = "2.18.6"` (defined at root `build.sbt:63`) via `% jacksonVersion`. Other services (`ConfigService`, `FileService`, `AccessControlService`, etc.) all reference `jacksonVersion`. Plus `notebook-migration-service/build.sbt:76` declares the local dep as `2.18.6` — but since the root `dependencyOverrides` wins, this service actually runs with 2.17.0 while the rest of the repo runs with 2.18.6. Probably a copy-paste; should be `% jacksonVersion` to match convention and avoid silent version skew across services.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,378 @@
+// 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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
+
+ var conn: HttpURLConnection = null
+ 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)
+ conn = url.openConnection().asInstanceOf[HttpURLConnection]
+
+ conn.setRequestMethod("PUT")
+ conn.setDoOutput(true)
+ conn.setRequestProperty("Content-Type", "application/json")
+ // The Jupyter Contents API requires authentication; send the configured token.
+ conn.setRequestProperty("Authorization", s"token $jupyterToken")
+
+ 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/$notebookName?token=$jupyterToken"
+
+ 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(errorJson(e.getMessage))
+ .build()
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
Review Comment:
The `notebook.wid` UNIQUE constraint from #5055 means at most one notebook row per workflow. But this method unconditionally INSERTs into both tables, so:
- First call for (wid=X, vid=Y): both inserts succeed ✓
- Second call for (wid=X, vid=Z) — saving a mapping for the same workflow at a different version: the `notebook` INSERT violates the UNIQUE constraint → SQL exception → 500.
But `workflow_notebook_mapping`'s PK is `(wid, vid, nid)`, which implies the schema is designed to hold one mapping row per workflow version. The current code can't actually produce more than one — only the first version per workflow can be stored.
Is the intent (a) "one notebook per workflow, only ever stored once" or (b) "support multiple versions, each with its own mapping"?
- If (a): better to detect the existing row and return an explicit "notebook already stored for workflow X" 4xx rather than letting the unique violation bubble up as a 500.
- If (b): the notebook INSERT needs to be conditional (e.g., upsert with `ON CONFLICT (wid) DO NOTHING` then re-fetch the nid), and the mapping INSERT just appends a new (wid, vid, nid) row.
The test suite doesn't currently exercise this — all tests do a single store-then-fetch. Adding a "store twice" test case would pin the intended behavior.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3461753529
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
Review Comment:
@volatile only fixes visibility, not the shared-state design, and in the per-user-pod model there's effectively one user per process. The value it produces is deterministic ($jupyterUrl/notebooks/work/<name>?token=), so a "stale" read is just a different-but-valid URL to the same server. This change would satisfy static analysis, but is not a real bug fix. Ok to commit.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on PR #5258:
URL: https://github.com/apache/texera/pull/5258#issuecomment-4724324509
/request-review @mengw15
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3431357079
##########
build.sbt:
##########
@@ -169,6 +169,16 @@ lazy val WorkflowExecutionService = (project in file("amber"))
)
.configs(Test)
.dependsOn(DAO % "test->test", Auth % "test->test") // test scope dependency
+lazy val NotebookMigrationService = (project in file("notebook-migration-service"))
+ .dependsOn(Auth, Config, DAO)
+ .settings(asfLicensingSettings)
+ .settings(
+ dependencyOverrides ++= Seq(
+ // override it as io.dropwizard 4 require 2.16.1 or higher
+ "com.fasterxml.jackson.module" %% "jackson-module-scala" % "2.17.0"
Review Comment:
Fixed to use `%jacksonVersion` in 6f69e91
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3431304857
##########
common/config/src/main/resources/storage.conf:
##########
@@ -150,4 +150,15 @@ storage {
password = "postgres"
password = ${?STORAGE_JDBC_PASSWORD}
}
+
+ # Configurations of the JupyterLab service
+ # Default values are provided for each field, which you don't need to change if you deployed Jupyter via docker-compose.yml in notebook-migration-service/src/main/resources/docker-compose.yml
+ jupyter {
+ url = "http://localhost:9100"
+ url = ${?STORAGE_JUPYTER_URL}
+ # Must match JUPYTER_TOKEN in notebook-migration-service/src/main/resources/docker-compose.yml.
+ # The backend sends this token when loading the iframe and uploading notebooks.
+ token = "texera"
+ token = ${?STORAGE_JUPYTER_TOKEN}
Review Comment:
I've modified the environment variable to `JUPYTER_TOKEN` to be consistent with the Jupyter container in 733dcfc
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3430786723
##########
common/config/src/main/resources/storage.conf:
##########
@@ -150,4 +150,15 @@ storage {
password = "postgres"
password = ${?STORAGE_JDBC_PASSWORD}
}
+
+ # Configurations of the JupyterLab service
+ # Default values are provided for each field, which you don't need to change if you deployed Jupyter via docker-compose.yml in notebook-migration-service/src/main/resources/docker-compose.yml
+ jupyter {
+ url = "http://localhost:9100"
+ url = ${?STORAGE_JUPYTER_URL}
+ # Must match JUPYTER_TOKEN in notebook-migration-service/src/main/resources/docker-compose.yml.
+ # The backend sends this token when loading the iframe and uploading notebooks.
+ token = "texera"
+ token = ${?STORAGE_JUPYTER_TOKEN}
Review Comment:
The "must match" coupling between this `STORAGE_JUPYTER_TOKEN` and the Jupyter container's `JUPYTER_TOKEN` is currently maintained only by a comment. In production:
- **K8s Helm:** the chart author has to remember to template BOTH env vars from the same value (e.g., `{{ .Values.texera.jupyterToken }}`). If they template only one, the other silently defaults to `"texera"` and every backend → Jupyter call returns 403 — but `isJupyterAvailable()` treats 403 as "available" (`NotebookMigrationResource.scala:65`: `status == 200 || status == 403`), so the misconfiguration doesn't trip the health check and surfaces only at first user request.
- **Token rotation:** changing the prod token requires updating both sides atomically — no rolling update.
- **`bin/single-node/.env`:** same drift risk for whoever maintains that file.
Could both sides read from the **same** env var (e.g., rename to `JUPYTER_TOKEN` without the `STORAGE_` prefix, consumed by `docker-compose.yml` and `storage.conf` alike)? That removes the human-maintained "must match" obligation. If that's a bigger refactor than fits here, at minimum consider tightening the healthcheck to fail-fast on 403 so a token mismatch surfaces immediately rather than at first user request.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3444142888
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,391 @@
+// 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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
+
+ var conn: HttpURLConnection = null
+ 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"
Review Comment:
`notebookName` is taken from the request body and interpolated into two unsafe contexts, both without any validation:
1. **URL path (line 153):** `s"$jupyterUrl/api/contents/work/$notebookName"`. A `notebookName=../../etc/notebook.ipynb` after HTTP path normalization escapes `work/` and hits a different Jupyter contents endpoint. Jupyter's contents API has scope checks but defense in depth would catch this before the request even goes out.
2. **JSON response injection (line 193 → line 94):** `setNotebook` writes `notebookName` into the process-global `jupyterIframeURL`. Subsequent `getJupyterIframeURL()` calls then do raw JSON string concat at line 94: `"url": "$jupyterIframeURL"`. If `notebookName` ever contains a `"` character, the response is malformed JSON — frontend `JSON.parse` throws on iframe reload.
The error path already uses `errorJson` for safe JSON building (fixed in `f0cf26172`); the success paths here still use raw `s"..."` interpolation. A simple regex check on `notebookName` at the top of `setNotebook` (e.g., reject anything outside `[A-Za-z0-9._-]+\.ipynb`) closes the URL-path angle. The success-path JSON could either use `errorJson`-style ObjectMapper construction, or just sanitize the URL before storing it in `jupyterIframeURL`.
In the per-user-pod deployment model described elsewhere in this file, the real-world impact of the URL-path angle is bounded (a user can only attack their own pod's Jupyter, which they already control via the iframe). But the fix is cheap (a few lines), survives a future shared-deployment change, and is the kind of pattern static-analysis / security audits will flag.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3454034278
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,391 @@
+// 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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
+
+ var conn: HttpURLConnection = null
+ 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"
Review Comment:
Addressed in fd0d472
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3431495754
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,378 @@
+// 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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
+
+ var conn: HttpURLConnection = null
+ 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)
+ conn = url.openConnection().asInstanceOf[HttpURLConnection]
+
+ conn.setRequestMethod("PUT")
+ conn.setDoOutput(true)
+ conn.setRequestProperty("Content-Type", "application/json")
+ // The Jupyter Contents API requires authentication; send the configured token.
+ conn.setRequestProperty("Authorization", s"token $jupyterToken")
+
+ 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/$notebookName?token=$jupyterToken"
+
+ 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(errorJson(e.getMessage))
+ .build()
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
Review Comment:
Since the intent is (a), the method will check for duplicate wid and reject early with a 409 instead of generic 500. The test suite also includes a new test to verify this behavior. Added in dc08534.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3454656458
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/NotebookMigrationService.scala:
##########
@@ -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.
+
+package org.apache.texera.service
+
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+import com.typesafe.scalalogging.LazyLogging
+import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider}
+import io.dropwizard.core.Application
+import io.dropwizard.core.setup.{Bootstrap, Environment}
+import org.apache.texera.common.config.StorageConfig
+import org.apache.texera.auth.RequestLoggingFilter
+import org.apache.texera.dao.SqlServer
+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])
+
+ // Route request logs through SLF4J, controlled by TEXERA_SERVICE_LOG_LEVEL
+ RequestLoggingFilter.register(environment.getApplicationContext)
+ }
Review Comment:
I added auth and workflow write access checks in 8bed0f9 and new tests that verify their behavior. Please let me know if I should change anything in regards to convention and/or formatting.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration, backend): add notebook-migration-service microservice in backend [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5258:
URL: https://github.com/apache/texera/pull/5258#issuecomment-4560733138
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/5258?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 48.95%. Comparing base ([`d8c254c`](https://app.codecov.io/gh/apache/texera/commit/d8c254c75a024f00106528dfb66f4ccc4084f012?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0c3af5e`](https://app.codecov.io/gh/apache/texera/commit/0c3af5e620fa3b6ad684ab311da5da6cd7ee859a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #5258 +/- ##
=========================================
Coverage 48.95% 48.95%
Complexity 2377 2377
=========================================
Files 1048 1048
Lines 40270 40270
Branches 4272 4272
=========================================
Hits 19714 19714
Misses 19402 19402
Partials 1154 1154
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.53% <ø> (ø)` | | |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.76% <ø> (ø)` | | Carriedforward from [d8c254c](https://app.codecov.io/gh/apache/texera/commit/d8c254c75a024f00106528dfb66f4ccc4084f012?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `51.57% <ø> (ø)` | | Carriedforward from [d8c254c](https://app.codecov.io/gh/apache/texera/commit/d8c254c75a024f00106528dfb66f4ccc4084f012?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | | |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.99% <ø> (ø)` | | Carriedforward from [d8c254c](https://app.codecov.io/gh/apache/texera/commit/d8c254c75a024f00106528dfb66f4ccc4084f012?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.64% <ø> (ø)` | | Carriedforward from [d8c254c](https://app.codecov.io/gh/apache/texera/commit/d8c254c75a024f00106528dfb66f4ccc4084f012?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [python](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.79% <ø> (ø)` | | Carriedforward from [d8c254c](https://app.codecov.io/gh/apache/texera/commit/d8c254c75a024f00106528dfb66f4ccc4084f012?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/5258/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.81% <ø> (ø)` | | |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/texera/pull/5258?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3431213726
##########
common/config/src/main/resources/storage.conf:
##########
@@ -150,4 +150,15 @@ storage {
password = "postgres"
password = ${?STORAGE_JDBC_PASSWORD}
}
+
+ # Configurations of the JupyterLab service
+ # Default values are provided for each field, which you don't need to change if you deployed Jupyter via docker-compose.yml in notebook-migration-service/src/main/resources/docker-compose.yml
Review Comment:
I removed the `# Default values...` comment in ce69dfd but kept the `# Configurations of...` comment as other config blocks include this style of comment too. Let me know if you want this comment also removed.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3455189226
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
Review Comment:
This endpoint builds JSON via raw string interpolation. If `StorageConfig.jupyterURL` or `jupyterToken` ever contains characters that need escaping (e.g., quotes/newlines), the response becomes invalid JSON. Prefer constructing the JSON with Jackson (same approach as `errorJson`).
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
Review Comment:
`jupyterIframeURL` is mutable process-wide state that’s read/written by different request threads, but it’s not marked `@volatile` or otherwise synchronized. Without a memory barrier, `get-jupyter-iframe-url` can observe a stale value after `set-notebook` updates it.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
Review Comment:
This endpoint builds JSON via raw string interpolation. If `StorageConfig.jupyterURL` contains characters that need escaping (e.g., quotes/newlines), the response becomes invalid JSON. Prefer constructing the JSON with Jackson (same approach as `errorJson`).
##########
common/config/src/main/scala/org/apache/texera/common/config/StorageConfig.scala:
##########
@@ -133,4 +133,8 @@ 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")
+ val jupyterToken: String = conf.getString("storage.jupyter.token")
Review Comment:
`StorageConfig` defines `ENV_*` constants for most env-overridable settings, but the new Jupyter settings don’t have matching constants. Adding them keeps the config surface self-documented and consistent with the rest of this file (and the env var names used in `storage.conf`).
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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 = {
+ var conn: HttpURLConnection = null
+ try {
+ val json = mapper.readTree(body)
+
+ val notebookName = json.get("notebookName").asText()
+ val notebookData = json.get("notebookData")
+
+ // Allow only a plain ".ipynb" filename. Validated before any network call so a
+ // bad name is rejected with a 400 up front. This blocks path traversal in the
+ // Jupyter contents URL (e.g. "../../etc/x.ipynb") and keeps notebookName out of
+ // the raw-interpolated jupyterIframeURL JSON (no quotes/control chars).
+ if (!notebookName.matches("[A-Za-z0-9._-]+\\.ipynb")) {
+ return Response
+ .status(Response.Status.BAD_REQUEST)
+ .entity(errorJson(s"Invalid notebook name: $notebookName"))
+ .build()
+ }
+
+ if (!isJupyterAvailable(jupyterUrl)) {
+ return Response
+ .status(500)
+ .entity(
+ """
+ {
+ "success": false,
+ "message": "Cannot connect to Jupyter server"
+ }
+ """
+ )
+ .build()
+ }
+
+ // Construct Jupyter API URL
+ val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName"
+
+ val url = new URL(apiUrl)
+ conn = url.openConnection().asInstanceOf[HttpURLConnection]
+
+ conn.setRequestMethod("PUT")
+ conn.setDoOutput(true)
+ conn.setRequestProperty("Content-Type", "application/json")
+ // The Jupyter Contents API requires authentication; send the configured token.
+ conn.setRequestProperty("Authorization", s"token $jupyterToken")
+
+ 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/$notebookName?token=$jupyterToken"
+
+ 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(errorJson(e.getMessage))
+ .build()
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // Store notebook + mapping in database
+ def storeNotebookAndMapping(body: String, uid: java.lang.Integer): 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")
+
+ // Only a user with write access to the workflow may store its notebook.
+ if (!WorkflowAccessResource.hasWriteAccess(wid, uid)) {
+ return Response
+ .status(Response.Status.FORBIDDEN)
+ .entity(errorJson(s"No write access to workflow $wid"))
+ .build()
+ }
+
+ val dsl = SqlServer.getInstance().createDSLContext()
+
+ // 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.
+ val alreadyStored = dsl.fetchExists(
+ dsl.selectFrom(Notebook.NOTEBOOK).where(Notebook.NOTEBOOK.WID.eq(wid))
+ )
+ if (alreadyStored) {
+ return Response
+ .status(Response.Status.CONFLICT)
+ .entity(errorJson(s"A notebook is already stored for workflow $wid"))
+ .build()
+ }
+
+ 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(errorJson(e.getMessage))
+ .build()
+ }
Review Comment:
`storeNotebookAndMapping` pre-checks for an existing notebook, but concurrent requests can still race and hit the UNIQUE constraint on `notebook.wid`, which would currently fall into the generic `NonFatal` handler and return a 500. Translating Postgres unique-violation (SQLState 23505) to an explicit 409 avoids spurious 500s and makes the API behavior consistent under retries/concurrency.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 merged PR #5258:
URL: https://github.com/apache/texera/pull/5258
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3431186865
##########
notebook-migration-service/src/main/resources/logback.xml:
##########
@@ -0,0 +1,55 @@
+<!--
+ 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.
+-->
+
+<configuration>
+ <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
Review Comment:
Thanks for pointing that out, the logback.xml was an artifact from when I was learning how to write a back-end microservice. I've deleted the file in c05c35b.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3430788231
##########
common/config/src/main/resources/storage.conf:
##########
@@ -150,4 +150,15 @@ storage {
password = "postgres"
password = ${?STORAGE_JDBC_PASSWORD}
}
+
+ # Configurations of the JupyterLab service
+ # Default values are provided for each field, which you don't need to change if you deployed Jupyter via docker-compose.yml in notebook-migration-service/src/main/resources/docker-compose.yml
Review Comment:
remove this comment
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3453979043
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/NotebookMigrationService.scala:
##########
@@ -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.
+
+package org.apache.texera.service
+
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+import com.typesafe.scalalogging.LazyLogging
+import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider}
+import io.dropwizard.core.Application
+import io.dropwizard.core.setup.{Bootstrap, Environment}
+import org.apache.texera.common.config.StorageConfig
+import org.apache.texera.auth.RequestLoggingFilter
+import org.apache.texera.dao.SqlServer
+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])
+
+ // Route request logs through SLF4J, controlled by TEXERA_SERVICE_LOG_LEVEL
+ RequestLoggingFilter.register(environment.getApplicationContext)
+ }
Review Comment:
I added HealthCheckResource in 8bed0f9
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on PR #5258:
URL: https://github.com/apache/texera/pull/5258#issuecomment-4772565625
> also please add more test cases
Added more test cases, summary of leftover lines:
NotebookMigrationResource.scala — 100% line coverage. Every reachable branch is tested, including the Jupyter HTTP paths (via a stub HttpServer on the configured port), the write-access gate, and the malformed-input and unreachable-server failure paths. The only branches Codecov still flags as partial are unreachable by design:
- The NonFatal catch blocks — the missed branch is the fatal Throwable path that NonFatal deliberately does not catch.
- The @Auth resource-method wrappers — synthetic Scala/jacoco branches (parameter/entry artifacts), not real code paths.
- Object initialization and the conn == null guard in isJupyterAvailable (reached only if openConnection throws before assignment).
NotebookMigrationService.scala — run() and registerAuthFeatures are tested (mocked Dropwizard Environment, mirroring AccessControlServiceRunSpec). initialize() and main() are left untested: they stand up the real application and open the live JDBC connection, so they aren't unit-testable — consistent with the other Dropwizard services, which test run() only.
StorageConfig.scala / NotebookMigrationServiceConfiguration.scala — the flagged lines are config field declarations; their coverage is attributed to other modules' flags and can't be exercised from this service's tests.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3461778250
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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()
+ }
Review Comment:
This value is already regex verified to be valid so there wouldn't be a case of invalid JSON. However, to future-proof for possible refactoring of this function it is OK to make this change as it doesn't affect anything now.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5258:
URL: https://github.com/apache/texera/pull/5258#issuecomment-4770722034
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@Ma77Ball`, `@Yicong-Huang`, `@kunwp1`
You can notify them by mentioning `@Ma77Ball`, `@Yicong-Huang`, `@kunwp1` in a comment.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3461753529
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
Review Comment:
`@volatile` only fixes visibility, not the shared-state design, and in the per-user-pod model there's effectively one user per process. The value it produces is deterministic ($jupyterUrl/notebooks/work/<name>?token=), so a "stale" read is just a different-but-valid URL to the same server. This change would satisfy static analysis, but is not a real bug fix. Ok to commit.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3461824726
##########
common/config/src/main/scala/org/apache/texera/common/config/StorageConfig.scala:
##########
@@ -133,4 +133,8 @@ 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")
+ val jupyterToken: String = conf.getString("storage.jupyter.token")
Review Comment:
I'm going to leave this as-is. StorageConfig's ENV_* constants aren't actually referenced anywhere — the env-var registry that's consumed at runtime is common/config/.../EnvironmentalVariable.scala (used by ComputingUnitManagingResource to pass settings into spawned pods). So adding ENV_JUPYTER_* constants here would only grow an unused, duplicative block rather than improve the config surface.
If/when the Jupyter settings need to propagate to spawned pods, the right place to add them is EnvironmentalVariable — but that wiring doesn't exist yet, so I'd add the constant when it's actually needed rather than speculatively. The env var names themselves are already documented where they're consumed: storage.conf (STORAGE_JUPYTER_URL, JUPYTER_TOKEN).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3461851689
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -0,0 +1,422 @@
+// 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 io.dropwizard.auth.Auth
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.auth.SessionUser
+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.common.config.StorageConfig
+
+object NotebookMigrationResource extends LazyLogging {
+
+ private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ // Build an error response body via the mapper so the message is JSON-escaped; interpolating
+ // e.getMessage directly produces malformed JSON when it contains quotes, backslashes, or newlines.
+ private def errorJson(message: String): String =
+ mapper.writeValueAsString(mapper.createObjectNode().put("error", message))
+
+ private val jupyterUrl = StorageConfig.jupyterURL
+ private val jupyterToken = StorageConfig.jupyterToken
+ // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
+ // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
+ // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
+ // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
+ // per-user keying here, or one user's upload would overwrite another's iframe URL.
+ private var jupyterIframeURL = s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ private def isJupyterAvailable(jupyterUrl: String): Boolean = {
+ var conn: java.net.HttpURLConnection = null
+ try {
+ 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
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // 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 = {
+ var conn: HttpURLConnection = null
+ try {
+ val json = mapper.readTree(body)
+
+ val notebookName = json.get("notebookName").asText()
+ val notebookData = json.get("notebookData")
+
+ // Allow only a plain ".ipynb" filename. Validated before any network call so a
+ // bad name is rejected with a 400 up front. This blocks path traversal in the
+ // Jupyter contents URL (e.g. "../../etc/x.ipynb") and keeps notebookName out of
+ // the raw-interpolated jupyterIframeURL JSON (no quotes/control chars).
+ if (!notebookName.matches("[A-Za-z0-9._-]+\\.ipynb")) {
+ return Response
+ .status(Response.Status.BAD_REQUEST)
+ .entity(errorJson(s"Invalid notebook name: $notebookName"))
+ .build()
+ }
+
+ if (!isJupyterAvailable(jupyterUrl)) {
+ return Response
+ .status(500)
+ .entity(
+ """
+ {
+ "success": false,
+ "message": "Cannot connect to Jupyter server"
+ }
+ """
+ )
+ .build()
+ }
+
+ // Construct Jupyter API URL
+ val apiUrl = s"$jupyterUrl/api/contents/work/$notebookName"
+
+ val url = new URL(apiUrl)
+ conn = url.openConnection().asInstanceOf[HttpURLConnection]
+
+ conn.setRequestMethod("PUT")
+ conn.setDoOutput(true)
+ conn.setRequestProperty("Content-Type", "application/json")
+ // The Jupyter Contents API requires authentication; send the configured token.
+ conn.setRequestProperty("Authorization", s"token $jupyterToken")
+
+ 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/$notebookName?token=$jupyterToken"
+
+ 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(errorJson(e.getMessage))
+ .build()
+ } finally {
+ if (conn != null) conn.disconnect()
+ }
+ }
+
+ // Store notebook + mapping in database
+ def storeNotebookAndMapping(body: String, uid: java.lang.Integer): 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")
+
+ // Only a user with write access to the workflow may store its notebook.
+ if (!WorkflowAccessResource.hasWriteAccess(wid, uid)) {
+ return Response
+ .status(Response.Status.FORBIDDEN)
+ .entity(errorJson(s"No write access to workflow $wid"))
+ .build()
+ }
+
+ val dsl = SqlServer.getInstance().createDSLContext()
+
+ // 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.
+ val alreadyStored = dsl.fetchExists(
+ dsl.selectFrom(Notebook.NOTEBOOK).where(Notebook.NOTEBOOK.WID.eq(wid))
+ )
+ if (alreadyStored) {
+ return Response
+ .status(Response.Status.CONFLICT)
+ .entity(errorJson(s"A notebook is already stored for workflow $wid"))
+ .build()
+ }
+
+ 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(errorJson(e.getMessage))
+ .build()
+ }
Review Comment:
Added in [7f90d74](https://github.com/apache/texera/pull/5258/commits/7f90d74b75837881eeaaf3ed926aa96bb696fb6a)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #5258:
URL: https://github.com/apache/texera/pull/5258#discussion_r3443952454
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/NotebookMigrationService.scala:
##########
@@ -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.
+
+package org.apache.texera.service
+
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+import com.typesafe.scalalogging.LazyLogging
+import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider}
+import io.dropwizard.core.Application
+import io.dropwizard.core.setup.{Bootstrap, Environment}
+import org.apache.texera.common.config.StorageConfig
+import org.apache.texera.auth.RequestLoggingFilter
+import org.apache.texera.dao.SqlServer
+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])
+
+ // Route request logs through SLF4J, controlled by TEXERA_SERVICE_LOG_LEVEL
+ RequestLoggingFilter.register(environment.getApplicationContext)
+ }
Review Comment:
**All 5 existing Dropwizard services in this repo register the same auth stack in `run()`:**
| Service | Registers JWT auth at |
|---|---|
| `AccessControlService` | line 80 (`AuthDynamicFeature(classOf[JwtAuthFilter])`) |
| `ConfigService` | line 71 |
| `FileService` | line 92 |
| `WorkflowCompilingService` | line 101 |
| `ComputingUnitManagingService` | line 86 |
Same 5 components every time: `AuthDynamicFeature(classOf[JwtAuthFilter])` + `RolesAllowedDynamicFeature` + `AuthValueFactoryProvider.Binder(classOf[SessionUser])` + `UnauthorizedExceptionMapper` + corresponding `@RolesAllowed` / `@Auth user: SessionUser` on the resources.
**This service is the only one (1 of 6) without any of that.** Every endpoint is fully unauthenticated. `POST /store-notebook-and-mapping` takes `wid` from the request body and writes the user-uploaded notebook to that workflow, with no check that the caller owns workflow `wid`. Same for `fetch` and `set-notebook`. Any client with network access to `:9098` can read/write any user's workflow's notebook.
The `NotebookMigrationResource` companion-object comment mentions a "per-user pod" k8s deployment model. But (a) the other 5 services run in k8s too and still do auth at the service layer, (b) it relies on gateway routing being airtight (no internal-cluster client ever talking directly to a pod), (c) even with per-user pods, `wid` ownership isn't enforced inside the pod either — so a misrouted request could clobber another user's notebook.
Is the intent to add the auth stack in a later PR in the series, or is the design genuinely "gateway is the only auth boundary"? Worth either wiring auth here to match the other 5 services, or documenting the gateway-trusts model in a top-level service comment so the next contributor doesn't assume it's an oversight.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/NotebookMigrationService.scala:
##########
@@ -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.
+
+package org.apache.texera.service
+
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+import com.typesafe.scalalogging.LazyLogging
+import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider}
+import io.dropwizard.core.Application
+import io.dropwizard.core.setup.{Bootstrap, Environment}
+import org.apache.texera.common.config.StorageConfig
+import org.apache.texera.auth.RequestLoggingFilter
+import org.apache.texera.dao.SqlServer
+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])
+
+ // Route request logs through SLF4J, controlled by TEXERA_SERVICE_LOG_LEVEL
+ RequestLoggingFilter.register(environment.getApplicationContext)
+ }
Review Comment:
**All 5 existing Dropwizard services in this repo register `HealthCheckResource` in their `run()`:**
| Service | Line |
|---|---|
| `AccessControlService` | 74 |
| `ConfigService` | 69 |
| `FileService` | 88 |
| `WorkflowCompilingService` | 59 |
| `ComputingUnitManagingService` | 64 |
This service is the only one (1 of 6) without it. Combined with `adminConnectors: []` in `notebook-migration-service-web-config.yaml` (which disables Dropwizard's built-in admin connector), the service has no external liveness/readiness endpoint — the future k8s Deployment manifest (later PR in the series) won't have a probe target without back-fill. Easy to add now (`environment.jersey.register(classOf[HealthCheckResource])`) to match the convention.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python-notebook-migration): add notebook-migration-service microservice in backend [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on PR #5258:
URL: https://github.com/apache/texera/pull/5258#issuecomment-4754034828
also please add more test cases
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org