Copilot commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3780828080
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -457,9 +463,15 @@ class NotebookMigrationResource extends LazyLogging {
@GET
@Path("/get-jupyter-iframe-url")
- def getJupyterIframeURL(@Auth user: SessionUser): Response = {
+ def getJupyterIframeURL(
+ @QueryParam("notebookName") notebookName: String,
Review Comment:
This is new user-facing API support: callers can now select a notebook via a
query parameter. The repository's contribution guidance says support that did
not previously exist is a `feat`, while `refactor` asserts identical
user-facing behavior. Please rename the PR/commit to
`feat(notebook-migration-service): compute jupyter iframe url per request`.
##########
notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala:
##########
@@ -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 = NotebookMigrationResource.getJupyterIframeURL("other.ipynb")
+ resp.getStatus shouldBe Response.Status.OK.getStatusCode
+ resp.getEntity.toString should include("/notebooks/work/other.ipynb")
+ }
+ }
Review Comment:
This positive test calls the companion helper directly, so it would still
pass if the JAX-RS wrapper ignored every explicit `notebookName` and always
used the default. Exercise `resource.getJupyterIframeURL` here so the newly
added endpoint forwarding is covered.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -75,13 +75,10 @@ object NotebookMigrationResource extends LazyLogging {
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"
Review Comment:
Removing the process-global iframe state leaves
`frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts:130-133`
factually stale: it still says a global service needs keying for the backend's
`process-global jupyterIframeURL`. Update that comment as part of this refactor
so future work does not rely on state that no longer exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
