Skip to content

feat(python-notebook-migration): add backend endpoint to delete a workflow's stored notebook and mapping - #7132

Merged
mengw15 merged 4 commits into
apache:mainfrom
zyratlo:migration-tool-delete-notebook-backend
Jul 30, 2026
Merged

feat(python-notebook-migration): add backend endpoint to delete a workflow's stored notebook and mapping#7132
mengw15 merged 4 commits into
apache:mainfrom
zyratlo:migration-tool-delete-notebook-backend

Conversation

@zyratlo

@zyratlo zyratlo commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Adds a single backend REST endpoint to the notebook-migration service that deletes a workflow's stored notebook and its workflow-to-notebook mapping.

Until now the service could store and fetch a workflow's notebook (store-notebook-and-mapping, fetch-notebook-and-mapping) but had no way to remove one. Once stored, a notebook and its mapping stayed in the database with no server-side operation to clear them. This left the persisted rows behind whenever a user closed or discarded a migrated notebook, and it blocked the end-to-end deletion flow, which needs a backend call to remove the notebook before the UI can reset the state that reflects whether a notebook exists.

New endpoint: POST /notebook-migration/delete-notebook-and-mapping.

Behavior:

  • Reads wid from the JSON body. vid is not required: notebook.wid is UNIQUE (one notebook per workflow), so wid alone identifies the row.
  • Requires write access to the workflow. Returns 403 when the caller lacks it, consistent with the store and fetch endpoints.
  • Deletes the notebook row for the workflow. The workflow_notebook_mapping rows are removed by the existing ON DELETE CASCADE foreign key, so a single delete clears both tables and cannot leave them inconsistent.
  • Returns {"success": true, "deleted": <count>}, where the count is 1 when a notebook was removed and 0 when nothing was stored, so the caller can tell a real deletion from a no-op.
  • Idempotent: deleting when nothing is stored returns success with deleted: 0.

POST with a JSON body is used to stay consistent with the existing store and fetch endpoints, which already read wid from a JSON body.

This is backend only. The frontend service method, the close-panel wiring, and any Jupyter-server cleanup are intentionally left to the follow-on end-to-end deletion work that consumes this endpoint.

Note: if the workflow itself is deleted, the notebook and mapping are already removed by existing cascades, so this endpoint targets the case where the workflow survives but its notebook should be discarded.

Any related issues, documentation, discussions?

Closes #7131
Parent issue #4301

How was this PR tested?

Added resource spec cases in NotebookMigrationResourceSpec covering:

  • Deleting a workflow that has a stored notebook removes both the notebook row and its mapping rows (cascade verified) and reports deleted: 1.
  • Idempotent delete: returns success with deleted: 0 when nothing is stored.
  • Returns 403 and deletes nothing when the caller has only read access.
  • Returns 500 on a malformed JSON body.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8)

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @Neilk1021
    You can notify them by mentioning @Neilk1021 in a comment.

@zyratlo

zyratlo commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @mengw15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new REST endpoint to the notebook-migration backend service to delete a workflow’s stored notebook and rely on FK cascade to remove the associated workflow→notebook mapping, enabling an end-to-end “discard migrated notebook” flow.

Changes:

  • Added POST /notebook-migration/delete-notebook-and-mapping that deletes the notebook row by wid and returns {"success": true, "deleted": <count>}.
  • Added resource-spec coverage for successful deletion (including cascade), idempotent no-op deletion, 403 on insufficient access, and 500 on malformed JSON.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala Adds the new delete endpoint implementation and JAX-RS route.
notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala Adds tests validating delete behavior, authorization, idempotency, and error path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Yicong-Huang

Copy link
Copy Markdown
Contributor

wait wait this PR has no CI ran. I think this python-notebook-migration service is not in labeler system correctly. Can we fix that first?

@zyratlo

zyratlo commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

wait wait this PR has no CI ran. I think this python-notebook-migration service is not in labeler system correctly. Can we fix that first?

@Yicong-Huang I think the CI is not running because we added this service in the labeler system in PR #6945, which is waiting for your input to be merged.

@Yicong-Huang

Copy link
Copy Markdown
Contributor

ah. let's merge that one first. I will change this to draft. this PR CI has to be retriggered.

@github-actions github-actions Bot added the platform Non-amber Scala service paths label Jul 30, 2026
@codecov-commenter

codecov-commenter commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.46%. Comparing base (2ec3b0a) to head (b15fff6).

Files with missing lines Patch % Lines
...a/service/resource/NotebookMigrationResource.scala 80.00% 0 Missing and 6 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7132      +/-   ##
============================================
- Coverage     79.57%   79.46%   -0.11%     
+ Complexity     3829     3783      -46     
============================================
  Files          1160     1159       -1     
  Lines         46164    45878     -286     
  Branches       5135     5111      -24     
============================================
- Hits          36736    36458     -278     
- Misses         7795     7801       +6     
+ Partials       1633     1619      -14     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 77.42% <ø> (ø) Carriedforward from 6cb4a04
amber 72.55% <ø> (-0.50%) ⬇️ Carriedforward from 6cb4a04
computing-unit-managing-service 20.49% <ø> (ø)
config-service 66.66% <ø> (ø)
file-service 66.59% <ø> (-0.22%) ⬇️ Carriedforward from 6cb4a04
frontend 83.25% <ø> (+0.16%) ⬆️ Carriedforward from 6cb4a04
notebook-migration-service 78.89% <80.00%> (-0.06%) ⬇️
pyamber 97.38% <ø> (+0.02%) ⬆️ Carriedforward from 6cb4a04
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 6cb4a04

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mengw15
mengw15 added this pull request to the merge queue Jul 30, 2026
Merged via the queue into apache:main with commit 51b6af3 Jul 30, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Notebook Migration] Add a delete endpoint to notebook migration service

5 participants