diff --git a/services/admin/app/supplier_facilities_routes.py b/services/admin/app/supplier_facilities_routes.py index 4783dcc9f..250f7f5c3 100644 --- a/services/admin/app/supplier_facilities_routes.py +++ b/services/admin/app/supplier_facilities_routes.py @@ -7,11 +7,11 @@ from __future__ import annotations import uuid as uuid_module -from typing import Any +from typing import Any, NoReturn from fastapi import APIRouter, Depends, HTTPException, Query from shared.pagination import PaginationParams -from sqlalchemy import delete, func, select +from sqlalchemy import delete, exists, func, select from sqlalchemy.orm import Session from .database import get_session @@ -134,6 +134,73 @@ async def create_supplier_facility( ) +_LINKED_RECORD_MODELS = ( + SupplierTraceabilityLotModel, + SupplierCTEEventModel, + SupplierFunnelEventModel, +) + + +def _linked_record_counts( + db: Session, + tenant_id: uuid_module.UUID, + facility_id: uuid_module.UUID, +) -> tuple[int, int, int]: + counts: list[int] = [] + for model in _LINKED_RECORD_MODELS: + counts.append( + db.execute( + select(func.count()) + .select_from(model) + .where(model.tenant_id == tenant_id, model.facility_id == facility_id) + ).scalar() + or 0 + ) + return counts[0], counts[1], counts[2] + + +def _no_linked_records(tenant_id: uuid_module.UUID, facility_id: uuid_module.UUID) -> tuple[Any, ...]: + """WHERE clauses that match only a facility with no linked rows.""" + return tuple( + ~exists( + select(model.id).where(model.tenant_id == tenant_id, model.facility_id == facility_id) + ) + for model in _LINKED_RECORD_MODELS + ) + + +def _load_facility_for_removal( + db: Session, + facility_id: uuid_module.UUID, + tenant_id: uuid_module.UUID, +) -> SupplierFacilityModel | None: + stmt = select(SupplierFacilityModel).where( + SupplierFacilityModel.id == facility_id, + SupplierFacilityModel.tenant_id == tenant_id, + ) + bind = db.get_bind() + if bind is not None and bind.dialect.name == "postgresql": + stmt = stmt.with_for_update() + return db.execute(stmt).scalar_one_or_none() + + +def _reject_linked_facility( + facility_name: str, + lots_count: int, + cte_events_count: int, + funnel_events_count: int, +) -> NoReturn: + raise HTTPException( + status_code=409, + detail=( + f"Cannot remove facility '{facility_name}' with linked records: " + f"{lots_count} lots/TLCs, {cte_events_count} CTE events, " + f"{funnel_events_count} other records. " + f"Delete only when zero linked rows." + ), + ) + + @router.get("/facilities/{facility_id}/removal-check", response_model=FacilityRemovalCheckResponse) async def check_supplier_facility_removal( facility_id: str, @@ -154,26 +221,9 @@ async def check_supplier_facility_removal( if facility is None or facility.tenant_id != tenant_id: raise HTTPException(status_code=404, detail="Facility not found") - lots_count = db.execute( - select(func.count()).select_from(SupplierTraceabilityLotModel).where( - SupplierTraceabilityLotModel.tenant_id == tenant_id, - SupplierTraceabilityLotModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - cte_events_count = db.execute( - select(func.count()).select_from(SupplierCTEEventModel).where( - SupplierCTEEventModel.tenant_id == tenant_id, - SupplierCTEEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - funnel_events_count = db.execute( - select(func.count()).select_from(SupplierFunnelEventModel).where( - SupplierFunnelEventModel.tenant_id == tenant_id, - SupplierFunnelEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 + lots_count, cte_events_count, funnel_events_count = _linked_record_counts( + db, tenant_id, facility_uuid + ) scoping_config_count = db.execute( select(func.count()).select_from(SupplierFacilityFTLCategoryModel).where( @@ -225,56 +275,57 @@ async def delete_supplier_facility( except ValueError as exc: raise HTTPException(status_code=400, detail="Invalid facility id") from exc - facility = db.get(SupplierFacilityModel, facility_uuid) - if facility is None or facility.tenant_id != tenant_id: + # Lock the facility on PostgreSQL before counting. A lot or CTE insert + # takes a key share lock on this row, so it cannot commit until this + # transaction ends. Lots and CTE events are ON DELETE CASCADE: a count + # in an earlier statement would miss a row that commits before the + # DELETE, and the cascade would destroy it. + facility = _load_facility_for_removal(db, facility_uuid, tenant_id) + if facility is None: raise HTTPException(status_code=404, detail="Facility not found") - lots_count = db.execute( - select(func.count()).select_from(SupplierTraceabilityLotModel).where( - SupplierTraceabilityLotModel.tenant_id == tenant_id, - SupplierTraceabilityLotModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - cte_events_count = db.execute( - select(func.count()).select_from(SupplierCTEEventModel).where( - SupplierCTEEventModel.tenant_id == tenant_id, - SupplierCTEEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - funnel_events_count = db.execute( - select(func.count()).select_from(SupplierFunnelEventModel).where( - SupplierFunnelEventModel.tenant_id == tenant_id, - SupplierFunnelEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - if lots_count > 0 or cte_events_count > 0 or funnel_events_count > 0: - raise HTTPException( - status_code=409, - detail=( - f"Cannot remove facility '{facility.name}' with linked records: " - f"{lots_count} lots/TLCs, {cte_events_count} CTE events, " - f"{funnel_events_count} other records. " - f"Delete only when zero linked rows." - ), + lots_count, cte_events_count, funnel_events_count = _linked_record_counts( + db, tenant_id, facility_uuid + ) + if lots_count or cte_events_count or funnel_events_count: + _reject_linked_facility(facility.name, lots_count, cte_events_count, funnel_events_count) + + facility_name = facility.name + # The DELETE re-checks linked rows in the same statement. A row that + # arrived after the count above makes this match nothing, so the + # cascade never runs. + result = db.execute( + delete(SupplierFacilityModel).where( + SupplierFacilityModel.id == facility_uuid, + SupplierFacilityModel.tenant_id == tenant_id, + *_no_linked_records(tenant_id, facility_uuid), + ), + execution_options={"synchronize_session": "fetch"}, + ) + if result.rowcount != 1: + db.rollback() + if _load_facility_for_removal(db, facility_uuid, tenant_id) is None: + raise HTTPException(status_code=404, detail="Facility not found") + lots_count, cte_events_count, funnel_events_count = _linked_record_counts( + db, tenant_id, facility_uuid ) + _reject_linked_facility(facility_name, lots_count, cte_events_count, funnel_events_count) + # FTL scoping is configuration, not traceability evidence. Postgres + # already cascades it with the facility; this covers databases that + # are not enforcing that foreign key. db.execute( delete(SupplierFacilityFTLCategoryModel).where( SupplierFacilityFTLCategoryModel.tenant_id == tenant_id, SupplierFacilityFTLCategoryModel.facility_id == facility_uuid, ) ) - - db.delete(facility) db.commit() return { "status": "deleted", "id": str(facility_uuid), - "facility_name": facility.name, + "facility_name": facility_name, } diff --git a/services/admin/tests/test_supplier_facilities_routes_1342.py b/services/admin/tests/test_supplier_facilities_routes_1342.py index 114ed8081..f43d76b3d 100644 --- a/services/admin/tests/test_supplier_facilities_routes_1342.py +++ b/services/admin/tests/test_supplier_facilities_routes_1342.py @@ -26,6 +26,7 @@ from __future__ import annotations +from datetime import datetime, timezone from unittest.mock import MagicMock, patch from uuid import UUID @@ -519,7 +520,7 @@ def test_lots_with_facility_filter(self, client: TestClient, db_session: Session supplier_user_id=TEST_USER_ID, facility_id=f.id, tlc_code="TLC-FILTER-001", - product_description="Spinach", + product_description="", status="active", ) db_session.add(lot) @@ -596,3 +597,98 @@ def _deny_owner(): resp = member_client.delete("/v1/supplier/facilities/00000000-0000-0000-0000-000000000000") assert resp.status_code == 403 + def test_delete_empty_facility(self, client: TestClient, db_session: Session): + facility = _make_facility(db_session) + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 200 + assert resp.json()["status"] == "deleted" + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is None + + def test_linked_lot_returns_409(self, client: TestClient, db_session: Session): + facility = _make_facility(db_session) + db_session.add( + SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=facility.id, + tlc_code="", + product_description="", + status="active", + ) + ) + db_session.commit() + + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is not None + + def test_stale_empty_count_does_not_cascade_lot(self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch): + """A lot committed after the pre-check must not be deleted with the facility. + + Lots use ON DELETE CASCADE. The pre-check and the DELETE used to be + separate statements, so a lot that landed in between was cascaded away. + """ + import app.supplier_facilities_routes as fac_routes + + facility = _make_facility(db_session) + lot = SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=facility.id, + tlc_code="", + product_description="", + status="active", + ) + db_session.add(lot) + db_session.commit() + monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) + + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is not None + assert db_session.get(SupplierTraceabilityLotModel, lot.id) is not None + + def test_stale_empty_count_does_not_cascade_cte_event( + self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch + ): + """An event filed on this facility must survive even when its lot lives elsewhere.""" + import app.supplier_facilities_routes as fac_routes + + target = _make_facility(db_session, "Target") + other = _make_facility(db_session, "Other") + lot = SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=other.id, + tlc_code="", + product_description="", + status="active", + ) + db_session.add(lot) + db_session.commit() + event = SupplierCTEEventModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=target.id, + lot_id=lot.id, + cte_type="shipping", + event_time=datetime.now(timezone.utc), + kde_data={}, + payload_sha256="", + merkle_hash="", + sequence_number=1, + obligation_ids=[], + ) + db_session.add(event) + db_session.commit() + monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) + + resp = client.delete(f"/v1/supplier/facilities/{target.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, target.id) is not None + assert db_session.get(SupplierCTEEventModel, event.id) is not None +