From 1894826725fbeb8911d3380254cf248b990df914 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 21 May 2026 11:45:35 -0400 Subject: [PATCH] ENG-3324: Add system pre-delete hook for cross-repo cascade cleanup 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) --- CHANGELOG.md | 3 + ...tem_connection_config_link_change_hooks.py | 2 +- src/fides/api/system_pre_delete_hooks.py | 70 +++++++++++++++++++ src/fides/api/system_steward_change_hooks.py | 4 +- src/fides/api/v1/endpoints/system.py | 7 ++ .../fides/api/test_system_pre_delete_hooks.py | 55 +++++++++++++++ .../fides/ops/api/v1/endpoints/test_system.py | 47 +++++++++++++ 7 files changed, 185 insertions(+), 3 deletions(-) create mode 100644 src/fides/api/system_pre_delete_hooks.py create mode 100644 tests/fides/api/test_system_pre_delete_hooks.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 06d55f56d51..0a950267c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ Changes can also be flagged with a GitHub label for tracking purposes. The URL o ## [Unreleased](https://github.com/ethyca/fides/compare/2.86.0..main) +### Added +- Added `system_pre_delete_hooks` registry so cross-repo consumers can snapshot dependent state before a system delete cascades. Fired from the single and bulk system-delete routes. + ## [2.86.0](https://github.com/ethyca/fides/compare/2.85.1..2.86.0) ### Added diff --git a/src/fides/api/system_connection_config_link_change_hooks.py b/src/fides/api/system_connection_config_link_change_hooks.py index c0b008673ac..b15ee60fc88 100644 --- a/src/fides/api/system_connection_config_link_change_hooks.py +++ b/src/fides/api/system_connection_config_link_change_hooks.py @@ -9,7 +9,7 @@ recomputed when that linkage changes. Like ``system_steward_change_hooks``, this module exists ONLY because the -event framework in fides PR #8096 is not yet merged. When it lands: +event framework is not yet merged. When it lands: - Call sites of ``notify_system_connection_config_link_changed`` (in the ``SystemIntegrationLinkService`` mutators) are replaced with diff --git a/src/fides/api/system_pre_delete_hooks.py b/src/fides/api/system_pre_delete_hooks.py new file mode 100644 index 00000000000..64abe5e0983 --- /dev/null +++ b/src/fides/api/system_pre_delete_hooks.py @@ -0,0 +1,70 @@ +""" +System pre-delete hook — TEMPORARY EVENT-FRAMEWORK SHIM. + +Fires synchronously immediately BEFORE a ``System`` is deleted, while the +system row and its dependent rows (``system_manager``, +``system_connection_config_link``, ``monitorsteward.source_system_id`` refs) +still exist. Registered callbacks may snapshot whatever state they need +before FK cascade wipes the joins they depend on. + +Distinct from ``system_steward_change_hooks``: that fires on add/remove +against the ``system_manager`` join, 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. +Hooks here therefore execute their snapshot synchronously and defer only +follow-up work via ``background_tasks``. + +Like the adjacent change-hook shims, this module exists ONLY because the +event framework is not yet merged. When it lands: + + - Call sites of ``notify_system_about_to_be_deleted`` (in the two v1 + system-delete routes) are replaced with + ``publish_before_commit(session, SystemDeleted(...))``. + - Fidesplus's registered callback is replaced with a + ``@subscribes_to(SystemDeleted)`` handler (with the snapshot moved + into a pre-commit listener). + - This file is deleted. + +DO NOT use this as a precedent for adding more cross-repo hooks. If you need +a similar shim before the framework lands, create a separate single-purpose +module — do not generalize this into a ``hooks`` package. +""" + +from typing import Callable, List + +from fastapi import BackgroundTasks +from loguru import logger + +SystemPreDeleteHook = Callable[[BackgroundTasks, str], None] + +_HOOKS: List[SystemPreDeleteHook] = [] + + +def register_system_pre_delete_hook(hook: SystemPreDeleteHook) -> None: + """Register a callback invoked synchronously before a system is deleted. + + Idempotent: registering the same hook twice is a no-op. + """ + if hook not in _HOOKS: + _HOOKS.append(hook) + + +def notify_system_about_to_be_deleted( + background_tasks: BackgroundTasks, system_id: str +) -> None: + """Invoke every registered hook BEFORE the system row is deleted. + + Each hook runs synchronously in the request thread and must complete + its snapshot quickly; heavy work should be deferred via + ``background_tasks``. Failures are logged and swallowed — a misbehaving + consumer must not block the delete. + """ + for hook in _HOOKS: + try: + hook(background_tasks, system_id) + except Exception: + logger.exception( + "System pre-delete hook {} raised for system_id={}", + hook, + system_id, + ) diff --git a/src/fides/api/system_steward_change_hooks.py b/src/fides/api/system_steward_change_hooks.py index 808a2c5218c..cdcf17a6fc5 100644 --- a/src/fides/api/system_steward_change_hooks.py +++ b/src/fides/api/system_steward_change_hooks.py @@ -5,8 +5,8 @@ added/removed as a data steward (system manager) of a system. The only known consumer today is fidesplus's monitor-stewardship-inheritance propagation. -This module exists ONLY because the event framework in fides PR #8096 is -not yet merged. When it lands: +This module exists ONLY because the event framework is not yet merged. When +it lands: - Call sites of ``notify_system_stewards_changed`` (in the three v1 system-manager routes) are replaced with diff --git a/src/fides/api/v1/endpoints/system.py b/src/fides/api/v1/endpoints/system.py index 9443408e412..26b264fb7c9 100644 --- a/src/fides/api/v1/endpoints/system.py +++ b/src/fides/api/v1/endpoints/system.py @@ -57,6 +57,7 @@ BasicSystemResponse, SystemResponse, ) +from fides.api.system_pre_delete_hooks import notify_system_about_to_be_deleted from fides.api.system_steward_change_hooks import notify_system_stewards_changed from fides.api.util.api_router import APIRouter from fides.api.util.connection_util import ( @@ -338,6 +339,7 @@ async def upsert( ) async def delete( fides_key: str, + background_tasks: BackgroundTasks, # to retrieve the System and also return a value db: AsyncSession = Depends(get_async_db), ) -> Dict: @@ -346,6 +348,9 @@ async def delete( to add additional "system manager" permission checks. """ system_to_delete = await get_resource(System, fides_key, db) + # Pre-delete hook runs while dependent rows still exist so consumers can + # snapshot state before FK cascade wipes joins they depend on. + notify_system_about_to_be_deleted(background_tasks, system_to_delete.id) async with db.begin(): await db.delete(system_to_delete) # Convert the resource to a dict explicitly for the response @@ -370,6 +375,7 @@ async def delete( ) async def system_bulk_delete( fides_keys: List[str], + background_tasks: BackgroundTasks, db: AsyncSession = Depends(get_async_db), ) -> Dict: """Delete multiple systems by their fides_keys.""" @@ -383,6 +389,7 @@ async def system_bulk_delete( systems_to_delete = result.scalars().all() for system in systems_to_delete: + notify_system_about_to_be_deleted(background_tasks, system.id) await db.delete(system) deleted.append(SystemSchema.model_validate(system).model_dump(mode="json")) diff --git a/tests/fides/api/test_system_pre_delete_hooks.py b/tests/fides/api/test_system_pre_delete_hooks.py new file mode 100644 index 00000000000..0868751d944 --- /dev/null +++ b/tests/fides/api/test_system_pre_delete_hooks.py @@ -0,0 +1,55 @@ +"""Tests for the system pre-delete hook registry.""" + +from unittest.mock import MagicMock + +import pytest +from fastapi import BackgroundTasks + +from fides.api import system_pre_delete_hooks as pre_delete + + +@pytest.fixture(autouse=True) +def isolated_registry(monkeypatch): + """Each test starts with an empty registry.""" + monkeypatch.setattr(pre_delete, "_HOOKS", []) + yield + + +def test_register_is_idempotent(): + hook = MagicMock() + pre_delete.register_system_pre_delete_hook(hook) + pre_delete.register_system_pre_delete_hook(hook) + assert pre_delete._HOOKS == [hook] + + +def test_notify_dispatches_to_all_hooks(): + first = MagicMock() + second = MagicMock() + pre_delete.register_system_pre_delete_hook(first) + pre_delete.register_system_pre_delete_hook(second) + + bg = BackgroundTasks() + pre_delete.notify_system_about_to_be_deleted(bg, "sys-1") + + first.assert_called_once_with(bg, "sys-1") + second.assert_called_once_with(bg, "sys-1") + + +def test_notify_isolates_hook_failures(): + """A raising hook must not prevent later hooks from firing.""" + raising = MagicMock(side_effect=RuntimeError("boom")) + survivor = MagicMock() + pre_delete.register_system_pre_delete_hook(raising) + pre_delete.register_system_pre_delete_hook(survivor) + + bg = BackgroundTasks() + pre_delete.notify_system_about_to_be_deleted(bg, "sys-1") + + raising.assert_called_once_with(bg, "sys-1") + survivor.assert_called_once_with(bg, "sys-1") + + +def test_notify_empty_registry_is_noop(): + """No hooks registered → no error, no side effects.""" + bg = BackgroundTasks() + pre_delete.notify_system_about_to_be_deleted(bg, "sys-1") diff --git a/tests/fides/ops/api/v1/endpoints/test_system.py b/tests/fides/ops/api/v1/endpoints/test_system.py index ac34aa67977..fdeaa4b77b5 100644 --- a/tests/fides/ops/api/v1/endpoints/test_system.py +++ b/tests/fides/ops/api/v1/endpoints/test_system.py @@ -13,6 +13,7 @@ ) from starlette.testclient import TestClient +from fides.api import system_pre_delete_hooks from fides.api.models.connectionconfig import ( AccessLevel, ConnectionConfig, @@ -1298,3 +1299,49 @@ def test_delete_system_without_staged_resources( assert resp.status_code == HTTP_200_OK assert db.query(System).filter_by(id=system_id).first() is None + + +class TestDeleteSystemFiresPreDeleteHook: + """Verify both delete routes invoke the pre-delete hook before the row is removed.""" + + @pytest.fixture(scope="function") + def registered_hook(self): + hook = mock.MagicMock() + system_pre_delete_hooks.register_system_pre_delete_hook(hook) + yield hook + system_pre_delete_hooks._HOOKS.remove(hook) + + def test_single_delete_fires_pre_delete_hook( + self, + api_client: TestClient, + generate_auth_header, + db: Session, + system, + registered_hook, + ) -> None: + system_id = system.id + url = V1_URL_PREFIX + f"/system/{system.fides_key}" + auth_header = generate_auth_header(scopes=[SYSTEM_DELETE]) + resp = api_client.delete(url, headers=auth_header) + + assert resp.status_code == HTTP_200_OK + registered_hook.assert_called_once() + # Second positional arg is system_id. + assert registered_hook.call_args.args[1] == system_id + + def test_bulk_delete_fires_pre_delete_hook_per_system( + self, + api_client: TestClient, + generate_auth_header, + db: Session, + system, + registered_hook, + ) -> None: + system_id = system.id + bulk_url = V1_URL_PREFIX + "/system/bulk-delete" + auth_header = generate_auth_header(scopes=[SYSTEM_DELETE]) + resp = api_client.post(bulk_url, headers=auth_header, json=[system.fides_key]) + + assert resp.status_code == HTTP_200_OK + registered_hook.assert_called_once() + assert registered_hook.call_args.args[1] == system_id