This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7602-3cb3479ec9201e43ca9e803da2a2a26d59a480a7 in repository https://gitbox.apache.org/repos/asf/texera.git
commit d94581ade4f421adcf3e8b58ae4be73afe04bdf1 Author: Ryan Zhang <[email protected]> AuthorDate: Fri Aug 14 23:09:31 2026 +0000 feat(notebook-migration-service): compute jupyter iframe url per request (#7602) ### What changes were proposed in this PR? Makes `notebook-migration-service` stateless so it can later run as a single global instance instead of one instance per user. This is the first backend stage of moving the service onto Texera's "orchestrator services are global, stateful resources are per user" pattern. Today the service keeps a shared `@volatile jupyterIframeURL`: `set-notebook` writes it and `get-jupyter-iframe-url` reads it back. That shared state is only safe because each user happens to run their own pod, and even within one user it lets two browser tabs race. This PR removes the shared state and builds the URL from the request instead. **`NotebookMigrationResource.scala`** - Removes the `@volatile var jupyterIframeURL` singleton and the warning comment that documented its per-user-pod assumption. Adds a `defaultNotebookName` constant (`notebook.ipynb`). - `getJupyterIframeURL` now takes a `notebookName` argument and builds the URL on each call. The name is validated with the same plain `.ipynb` regex `setNotebook` uses, since it now flows straight into the returned URL (blocks path traversal). The argument defaults to `defaultNotebookName`. - `setNotebook` no longer mutates any shared state; the assignment that wrote the singleton is gone. Its upload behavior is unchanged. - The `/get-jupyter-iframe-url` endpoint accepts an optional `notebookName` query parameter and falls back to the default when it is absent. The change is backward compatible. The existing frontend calls the endpoint with no query parameter, which resolves to `notebook.ipynb`, exactly the URL it received before. No frontend, config, or deployment change is needed in this PR, and no other service or branch consumes the removed state. ### Any related issues, documentation, discussions? Closes #7390 Parent-issue #4301 ### How was this PR tested? Extends the existing suite in `NotebookMigrationResourceSpec.scala`: - Fixed the two call sites that pass through the new endpoint signature. - Added a test that an explicit `notebookName` is honored in the returned URL. - Added a test that an invalid `notebookName` is rejected with 400 before any Jupyter call. - Added a test pinning the refactor: after `setNotebook` uploads `other.ipynb`, a parameter-less `getJupyterIframeURL` returns the default `notebook.ipynb`, proving the result no longer depends on state left by `setNotebook`. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) --- .../resource/NotebookMigrationResource.scala | 47 ++++++++++++++-------- .../resource/NotebookMigrationResourceSpec.scala | 39 ++++++++++++++++-- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala index ac41ccd28d..17a0a989d7 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala @@ -73,15 +73,16 @@ object NotebookMigrationResource extends LazyLogging { } } + // jupyterUrl and jupyterToken are single process-wide values, so this service still + // targets one Jupyter per process (the per-user-pod model) and must not be deployed as a + // shared global instance yet: every user would get the same Jupyter and the same token. + // Resolving these per user is a later stage of the migration (#7665). 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. - @volatile private var jupyterIframeURL = - s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken" + + // Default notebook name used when a request does not specify one, so a param-less + // getJupyterIframeURL call reproduces the URL from before this service became stateless. + private val defaultNotebookName = "notebook.ipynb" private def isJupyterAvailable(jupyterUrl: String): Boolean = { var conn: java.net.HttpURLConnection = null @@ -104,8 +105,17 @@ object NotebookMigrationResource extends LazyLogging { } } - // Returns the Jupyter iframe reference URL - def getJupyterIframeURL(): Response = { + // Returns the Jupyter iframe reference URL for the given notebook. + def getJupyterIframeURL(notebookName: String): Response = { + // notebookName flows into the returned URL, so validate it the same way setNotebook does: + // block path traversal and keep it to a plain .ipynb filename. + 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) @@ -120,7 +130,9 @@ object NotebookMigrationResource extends LazyLogging { .build() } - Response.ok(successUrlJson(jupyterIframeURL)).build() + Response + .ok(successUrlJson(s"$jupyterUrl/notebooks/work/$notebookName?token=$jupyterToken")) + .build() } // Returns the URL of Jupyter @@ -153,8 +165,7 @@ object NotebookMigrationResource extends LazyLogging { // 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). + // Jupyter contents URL (e.g. "../../etc/x.ipynb"). if (!notebookName.matches("[A-Za-z0-9._-]+\\.ipynb")) { return Response .status(Response.Status.BAD_REQUEST) @@ -217,8 +228,6 @@ object NotebookMigrationResource extends LazyLogging { .build() } - jupyterIframeURL = s"$jupyterUrl/notebooks/work/$notebookName?token=$jupyterToken" - Response .ok( s""" @@ -457,9 +466,15 @@ class NotebookMigrationResource extends LazyLogging { @GET @Path("/get-jupyter-iframe-url") - def getJupyterIframeURL(@Auth user: SessionUser): Response = { + def getJupyterIframeURL( + @QueryParam("notebookName") notebookName: String, + @Auth user: SessionUser + ): Response = { logger.info("Getting Jupyter iframe URL") - NotebookMigrationResource.getJupyterIframeURL() + val name = Option(notebookName) + .filter(_.nonEmpty) + .getOrElse(NotebookMigrationResource.defaultNotebookName) + NotebookMigrationResource.getJupyterIframeURL(name) } @GET diff --git a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala index bb1a29c7e5..15ad27eff1 100644 --- a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala +++ b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala @@ -449,7 +449,7 @@ class NotebookMigrationResourceSpec resource.setNotebook(validNotebook, user).getStatus shouldBe 500 resource.getJupyterURL(user).getStatus shouldBe 500 - resource.getJupyterIframeURL(user).getStatus shouldBe 500 + resource.getJupyterIframeURL(null, user).getStatus shouldBe 500 } it should "return 500 when the request body is malformed JSON" in { @@ -481,9 +481,42 @@ class NotebookMigrationResourceSpec urlResp.getStatus shouldBe Response.Status.OK.getStatusCode urlResp.getEntity.toString should include("localhost:9100") - val iframeResp = resource.getJupyterIframeURL(sessionUser(writerUid)) + val iframeResp = resource.getJupyterIframeURL(null, sessionUser(writerUid)) iframeResp.getStatus shouldBe Response.Status.OK.getStatusCode - iframeResp.getEntity.toString should include("/notebooks/work/") + iframeResp.getEntity.toString should include("/notebooks/work/notebook.ipynb") + } + } + + it should "build the iframe URL from an explicit notebook name" in { + withFakeJupyter(contentsStatus = 201) { + val resp = resource.getJupyterIframeURL("other.ipynb", sessionUser(writerUid)) + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("/notebooks/work/other.ipynb") + } + } + + it should "reject an invalid notebook name for the iframe URL with 400" in { + // notebookName flows into the URL, so it is validated before any Jupyter call and + // rejected without a running server. + NotebookMigrationResource + .getJupyterIframeURL("../../etc/evil.ipynb") + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + } + + it should "not be affected by a prior setNotebook call (no shared iframe state)" in { + // Pins the stateless refactor: getJupyterIframeURL builds its URL from the request, not + // from state left by setNotebook. A param-less iframe request after uploading other.ipynb + // must return the default notebook, not the just-uploaded name. + withFakeJupyter(contentsStatus = 201) { + val user = sessionUser(writerUid) + resource + .setNotebook("""{"notebookName": "other.ipynb", "notebookData": {"cells": []}}""", user) + .getStatus shouldBe Response.Status.OK.getStatusCode + + val iframe = resource.getJupyterIframeURL(null, user) + iframe.getStatus shouldBe Response.Status.OK.getStatusCode + iframe.getEntity.toString should include("/notebooks/work/notebook.ipynb") + iframe.getEntity.toString should not include "other.ipynb" } }
