UN-151 [FIX] Block workflow deletion at backend when in use by pipelines/APIs#1969
UN-151 [FIX] Block workflow deletion at backend when in use by pipelines/APIs#1969pk-zipstack wants to merge 2 commits into
Conversation
…nes/APIs Adds a perform_destroy guard on WorkflowViewSet that calls can_update_workflow and raises WorkflowDeletionError (400) with the same detailed message used by the frontend. Prevents direct API callers from silently cascade-deleting Pipelines/APIDeployments that reference the workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis PR blocks workflow deletion when the workflow is still in use, adds a dedicated API exception, and formats the usage details included in the delete error message. ChangesWorkflow deletion guard
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/workflow_manager/workflow_v2/workflow_helper.py (1)
996-1033: ⚡ Quick winAdd defensive handling for None or empty pipeline fields.
If
pipeline_nameorpipeline_typeisNoneor an empty string, the formatted message will displayNoneor empty backticks, which degrades the user experience.🛡️ Proposed fix to handle None/empty values gracefully
if pipelines: shown = list(pipelines)[:limit] for p in shown: - name = p.get("pipeline_name") - p_type = p.get("pipeline_type") + name = p.get("pipeline_name") or "Unknown" + p_type = p.get("pipeline_type") or "Unknown" lines.append(f"- `{name}` ({p_type} Pipeline)")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/workflow_manager/workflow_v2/workflow_helper.py` around lines 996 - 1033, The build_workflow_in_use_message function currently inserts raw pipeline_name/pipeline_type (and api_names entries) into the user message, which can render "None" or empty backticks; update build_workflow_in_use_message to defensively coerce missing or empty values to a friendly placeholder (e.g. "<unknown>" or "unnamed") before formatting: for pipelines iterate over p in shown and compute name = (p.get("pipeline_name") or "<unknown>").strip() and p_type = (p.get("pipeline_type") or "Pipeline").strip() (or similar), and for api_names map any None/empty to a placeholder before adding backticked entries; keep usage of WorkflowHelper.USAGE_MESSAGE_DISPLAY_LIMIT and the existing usage dict keys (pipelines, api_names, pipeline_count, api_count) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/workflow_manager/workflow_v2/workflow_helper.py`:
- Around line 996-1033: The build_workflow_in_use_message function currently
inserts raw pipeline_name/pipeline_type (and api_names entries) into the user
message, which can render "None" or empty backticks; update
build_workflow_in_use_message to defensively coerce missing or empty values to a
friendly placeholder (e.g. "<unknown>" or "unnamed") before formatting: for
pipelines iterate over p in shown and compute name = (p.get("pipeline_name") or
"<unknown>").strip() and p_type = (p.get("pipeline_type") or "Pipeline").strip()
(or similar), and for api_names map any None/empty to a placeholder before
adding backticked entries; keep usage of
WorkflowHelper.USAGE_MESSAGE_DISPLAY_LIMIT and the existing usage dict keys
(pipelines, api_names, pipeline_count, api_count) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9778eca5-e3ea-414b-b814-ab21fe1bdaa5
📒 Files selected for processing (3)
backend/workflow_manager/workflow_v2/exceptions.pybackend/workflow_manager/workflow_v2/views.pybackend/workflow_manager/workflow_v2/workflow_helper.py
|
Unstract test resultsPer-group results
Critical paths
|



What
perform_destroyoverride onWorkflowViewSetthat callsWorkflowHelper.can_update_workflowbefore deletion and raises a newWorkflowDeletionError(HTTP 400) with the same detailed message the UI shows when a workflow is in use.WorkflowHelper.build_workflow_in_use_messageso the rejection detail lists the dependent pipelines and API deployments, matching the frontend format.Why
Workflows.jsx) already gates the delete button via thecan_updateendpoint and shows a descriptive error when a workflow is referenced by pipelines/API deployments (PR UN-3217 [FEAT] Show specific pipeline/API names in workflow deletion error message #1784).WorkflowViewSet.destroyhad no equivalent guard. BecausePipeline.workflowandAPIDeployment.workfloware declared withon_delete=models.CASCADE, a directDELETE /workflow/<id>/API call (or a UI bypass / race) would silently cascade-delete every dependent pipeline and API deployment. That was the original concern in UN-151.How
WorkflowDeletionError(status 400) inworkflow_manager/workflow_v2/exceptions.py.build_workflow_in_use_message(workflow_name, usage)inworkflow_helper.pymirrors the frontend formatting (truncates after 3 names per category, appends...and N moresummary).WorkflowViewSet.perform_destroyinvokescan_update_workflow; oncan_update=falseit raisesWorkflowDeletionError(detail=...). Otherwise it falls through to the defaultModelViewSetdestroy.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
can_updatebefore issuingDELETE, so end users won't notice a change. The new guard only fires for direct API callers and any TOCTOU race between the UI check and the actual delete — both cases where the previous behavior was unsafe (silent cascade delete). Workflows with no pipeline/API references continue to delete as before.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
DELETE /workflow/<id>/for a workflow that is not referenced by any pipeline/API → should succeed (200).DELETE /workflow/<id>/for a workflow referenced by 1+ Pipelines and/or APIDeployments via curl or Postman → should return 400 with a body like:Pipeline/APIDeploymentrows are intact after the rejection.can_updatefirst) continues to work unchanged.Screenshots
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code