Skip to content

ENG-3324: System pre-delete hook for cross-repo cascade cleanup#8257

Closed
adamsachs wants to merge 1 commit into
mainfrom
ENG-3324/system-delete-fires-steward-change-hook
Closed

ENG-3324: System pre-delete hook for cross-repo cascade cleanup#8257
adamsachs wants to merge 1 commit into
mainfrom
ENG-3324/system-delete-fires-steward-change-hook

Conversation

@adamsachs
Copy link
Copy Markdown
Contributor

@adamsachs adamsachs commented May 21, 2026

Ticket ENG-3324

🔗 Coupled with fidesplus #3632 — the consumer that registers a callback on the new hook to clean up ClientDetail.monitors after a system delete. Must merge before / together with this one.

Description Of Changes

Adds a synchronous-dispatch hook registry, system_pre_delete_hooks, fired immediately before a System row is deleted in both the single and bulk system-delete routes. Lets cross-repo consumers snapshot dependent state while the system + its FK-cascaded dependents (system_manager, system_connection_config_link, monitorsteward.source_system_id refs) still exist.

Distinct from the existing system_steward_change_hooks / system_connection_config_link_change_hooks shims: those fire on add/remove against join tables, where the BG-task model works because the post-commit state is still queryable. Deletion is different — by the time a BG task would run, FK cascades have erased the rows consumers need. The new hook therefore executes hooks synchronously in the request thread; consumers do their snapshot inline and defer follow-up work via background_tasks themselves.

Why this exists: fidesplus's stewardship-inheritance propagation maintains a denormalized ClientDetail.monitors array (used as an authorization gate for monitor-steward scope). When a System is deleted, FK cascade wipes the inherited MonitorSteward rows correctly, but ClientDetail.monitors is left with stale monitor IDs and no convergence path. The paired fidesplus PR registers a callback that snapshots the affected (monitor_id, user_id) pairs synchronously, then schedules a BG task to clean the array post-commit.

Also strips a stale "fides PR #8096" reference from the two adjacent change-hook shims so the docstrings track the event-framework migration generically rather than pointing at a moving target.

Code Changes

  • src/fides/api/system_pre_delete_hooks.py — new single-purpose registry, ~70 LOC, mirrors the existing shim posture. register_system_pre_delete_hook(callback) and notify_system_about_to_be_deleted(background_tasks, system_id). Failure-isolated (each hook wrapped in try/except, logged not raised). Idempotent registration.
  • src/fides/api/v1/endpoints/system.py:delete — injects BackgroundTasks, fires notify_system_about_to_be_deleted before db.delete(...).
  • src/fides/api/v1/endpoints/system.py:system_bulk_delete — same pattern, per-iteration emit before each db.delete(system).
  • src/fides/api/system_steward_change_hooks.py / system_connection_config_link_change_hooks.py — drop stale "fides PR Proposal: in-application event framework — design + Phase 1 plumbing #8096" references from the module docstrings; behavior unchanged.
  • Tests:
    • tests/fides/api/test_system_pre_delete_hooks.py — 4 unit tests for the registry (idempotency, dispatch-to-all, failure isolation, empty-registry no-op).
    • tests/fides/ops/api/v1/endpoints/test_system.py — 2 route-integration tests confirming the hook fires once per deleted system on both the single and bulk delete paths.

Steps to Confirm

  1. pytest tests/fides/api/test_system_pre_delete_hooks.py — 4 tests pass.
  2. pytest tests/fides/ops/api/v1/endpoints/test_system.py -k PreDeleteHook — 2 tests pass.
  3. Confirm the hook is fired with the correct system_id when a system is deleted via either route (BackgroundTasks param, system_id positional arg).

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

🤖 Generated with Claude Code

Adds a synchronous-dispatch hook registry that fires immediately before
the system delete commits, letting cross-repo consumers (fidesplus
stewardship inheritance) snapshot dependent state before FK cascade wipes
the joins they depend on. Wired into both the single and bulk system-
delete routes.

Also strips a stale PR # reference from the two adjacent change-hook
shims so the docstrings track the event-framework migration generically.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored May 21, 2026 4:30pm
fides-privacy-center Ignored Ignored May 21, 2026 4:30pm

Request Review

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.17%. Comparing base (38b3296) to head (1894826).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/v1/endpoints/system.py 33.33% 2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (88.23%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8257      +/-   ##
==========================================
+ Coverage   85.15%   85.17%   +0.01%     
==========================================
  Files         670      671       +1     
  Lines       43497    43514      +17     
  Branches     5093     5095       +2     
==========================================
+ Hits        37039    37062      +23     
+ Misses       5351     5348       -3     
+ Partials     1107     1104       -3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@adamsachs
Copy link
Copy Markdown
Contributor Author

[Claude] Superseded by fidesplus #3782. These changes were combined with the paired fidesplus PR #3632 into a single PR against the new dev branch, now that the fides OSS repo has been merged into the fidesplus monorepo (this repo's src/fides tree). Closing in favor of that consolidated PR.

@adamsachs adamsachs closed this Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant