From 15b79eee966683e5f2d31353aeec701094ffd5bd Mon Sep 17 00:00:00 2001 From: Thomas Date: Thu, 23 Nov 2023 02:42:13 +0800 Subject: [PATCH] Gracefully handle Foreign Key Constraint Errors (#4406) Co-authored-by: Adam Sachs --- CHANGELOG.md | 3 +++ src/fides/api/db/crud.py | 18 +++++++++++++++++- tests/ctl/database/test_crud.py | 16 +++++++++++++++- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d8d81bf2c..9f797b2334 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,7 +45,9 @@ The types of changes are: ## [2.24.0](https://github.com/ethyca/fides/compare/2.23.3...2.24.0) ### Added + - Adds fides_disable_banner config option to Fides.js [#4378](https://github.com/ethyca/fides/pull/4378) +- Deletions that fail due to foreign key constraints will now be more clearly communicated [#4406](https://github.com/ethyca/fides/pull/4378) - Added support for a custom get preferences API call provided through Fides.init [#4375](https://github.com/ethyca/fides/pull/4375) - Hidden custom privacy request fields in the Privacy Center [#4370](https://github.com/ethyca/fides/pull/4370) - Backend System-level Cookie Support [#4383](https://github.com/ethyca/fides/pull/4383) @@ -59,6 +61,7 @@ The types of changes are: - Adds more granularity to tracking consent method, updates custom savePreferencesFn and FidesUpdated event to take consent method [#4419](https://github.com/ethyca/fides/pull/4419) ### Changed + - Add filtering and pagination to bulk vendor add table [#4351](https://github.com/ethyca/fides/pull/4351) - Determine if the TCF overlay needs to surface based on backend calculated version hash [#4356](https://github.com/ethyca/fides/pull/4356) - Moved Experiences and Preferences endpoints to Plus to take advantage of dynamic GVL [#4367](https://github.com/ethyca/fides/pull/4367) diff --git a/src/fides/api/db/crud.py b/src/fides/api/db/crud.py index eb13fa38de..e9feeb8880 100644 --- a/src/fides/api/db/crud.py +++ b/src/fides/api/db/crud.py @@ -5,15 +5,17 @@ from collections import defaultdict from typing import Any, Dict, List, Tuple +from fastapi import HTTPException from loguru import logger as log from sqlalchemy import and_, column from sqlalchemy import delete as _delete from sqlalchemy import or_ from sqlalchemy import update as _update from sqlalchemy.dialects.postgresql import Insert as _insert -from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.exc import IntegrityError, SQLAlchemyError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.future import select +from starlette.status import HTTP_422_UNPROCESSABLE_ENTITY from fides.api.db.base import Base # type: ignore[attr-defined] from fides.api.models.sql_models import ( # type: ignore[attr-defined] @@ -308,6 +310,7 @@ async def delete_resource( async with async_session.begin(): try: + # Automatically delete related resources if they are CTL objects if hasattr(sql_model, "parent_key"): log.debug("Deleting resource and its children") query = ( @@ -324,6 +327,19 @@ async def delete_resource( log.debug("Deleting resource") query = _delete(sql_model).where(sql_model.fides_key == fides_key) await async_session.execute(query) + except IntegrityError as err: + raw_error_text: str = err.orig.args[0] + + if "violates foreign key constraint" in raw_error_text: + error_message = "Failed to delete resource! Foreign key constraint found, try deleting related resources first." + else: + error_message = "Failed to delete resource!" + + log.bind(error="SQL Query integrity error!").error(raw_error_text) + raise HTTPException( + status_code=HTTP_422_UNPROCESSABLE_ENTITY, + detail=error_message, + ) except SQLAlchemyError: error = errors.QueryError() log.bind(error=error.detail["error"]).info( # type: ignore[index] diff --git a/tests/ctl/database/test_crud.py b/tests/ctl/database/test_crud.py index a16f1e03b5..77d4ac202c 100644 --- a/tests/ctl/database/test_crud.py +++ b/tests/ctl/database/test_crud.py @@ -3,6 +3,7 @@ from uuid import uuid4 import pytest +from fastapi import HTTPException from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.ext.asyncio import AsyncSession @@ -31,7 +32,6 @@ def fixture_created_resources( created_keys = [] resource_type = request.param to_create = ["foo", "foo.bar", "foo.bar.baz", "foo.boo"] - for key in to_create: parent_key = ".".join(key.split(".")[:-1]) if "." in key else None @@ -73,6 +73,20 @@ async def test_cascade_delete_taxonomy_children( assert len(set(keys).intersection(remaining_keys)) == 0 +@pytest.mark.integration +async def test_delete_fails_linked_resources( + linked_dataset, + async_session: AsyncSession, +) -> None: + """Deleting a resource should fail if a linked resource (without cascade delete relationship) still exists.""" + with pytest.raises(HTTPException) as e: + await delete_resource( + sql_models.Dataset, linked_dataset.fides_key, async_session + ) + + assert "try deleting related resources first" in str(e) + + @pytest.fixture(scope="function") def custom_field_definition_data_use(db): custom_field_definition_data = {