fix(addresses): preserve complete import rollback evidence and batch previews

Release v0.1.23. Coordinated integrity review: GovOPlaN/govoplan-core#298.

src/govoplan_addresses/backend/imports.py

@@ -3,19 +3,28 @@ from __future__ import annotations
 import base64
 import binascii
 import csv
+import copy
 import hashlib
 import json
 from collections import Counter
+from datetime import UTC, datetime
 from io import BytesIO, StringIO
 from typing import Any
 
-from sqlalchemy import and_, false, or_
-from sqlalchemy.orm import Session
+from sqlalchemy import and_, false, func, or_, select
+from sqlalchemy.orm import Session, selectinload
+from sqlalchemy.orm.attributes import set_committed_value
 
 from govoplan_addresses.backend.db.models import (
     AddressImportProfile,
     AddressImportRun,
+    AddressListEntry,
     Contact,
+    ContactChannelRule,
+    ContactEmail,
+    ContactPhone,
+    ContactPointQualityDecision,
+    ContactPostalAddress,
 )
 from govoplan_addresses.backend.import_schemas import (
     AddressImportConfiguration,
@@ -34,6 +43,12 @@ from govoplan_addresses.backend.schemas import (
 )
 from govoplan_addresses.backend.service import (
     AddressBookError,
+    _contact_change_payload,
+    _record_address_contact_change,
+    _require_mutable_book,
+    _replace_emails,
+    _replace_phones,
+    _replace_postal_addresses,
     create_contact,
     delete_contact,
     get_visible_address_book,
@@ -47,6 +62,16 @@ from govoplan_core.db.base import utcnow
 
 MAX_IMPORT_BYTES = 10_000_000
 MAX_IMPORT_COLUMNS = 200
+CONTACT_LOOKUP_BATCH_SIZE = 250
+ROLLBACK_SNAPSHOT_VERSION = 2
+CONTACT_POINTS = {
+    "emails": (ContactEmail, ("email",), _replace_emails),
+    "phones": (ContactPhone, ("phone",), _replace_phones),
+    "postal_addresses": (
+        ContactPostalAddress, ("street", "postal_code", "locality", "region", "country"),
+        _replace_postal_addresses,
+    ),
+}
 
 
 def _account_id(principal: ApiPrincipal) -> str:
@@ -241,15 +266,19 @@ def get_import_run(
     session: Session,
     principal: ApiPrincipal,
     run_id: str,
+    *,
+    lock: bool = False,
 ) -> AddressImportRun:
     visible_book_ids = [book.id for book in _visible_import_books(session, principal)]
     if not visible_book_ids:
         raise AddressBookError("Address import run not found.")
-    run = (
+    query = (
         session.query(AddressImportRun)
         .filter(AddressImportRun.id == run_id, AddressImportRun.address_book_id.in_(visible_book_ids))
-        .one_or_none()
     )
+    if lock:
+        query = query.populate_existing().with_for_update()
+    run = query.one_or_none()
     if run is None:
         raise AddressBookError("Address import run not found.")
     return run
@@ -262,13 +291,13 @@ def apply_address_import(
     *,
     expected_plan_hash: str,
 ) -> AddressImportRun:
-    run = get_import_run(session, principal, run_id)
+    run = get_import_run(session, principal, run_id, lock=True)
+    if run.plan_hash != expected_plan_hash:
+        raise AddressBookError("The reviewed import plan changed; create a new preview.")
     if run.status == "applied":
         return run
     if run.status != "previewed":
         raise AddressBookError(f"Import run cannot be applied from status {run.status!r}.")
-    if run.plan_hash != expected_plan_hash:
-        raise AddressBookError("The reviewed import plan changed; create a new preview.")
     if any(item.get("severity") == "error" for item in run.diagnostics or []):
         raise AddressBookError("Import plans with error diagnostics cannot be applied.")
     if any(item.get("action") == "conflict" for item in run.plan_data or []):
@@ -276,12 +305,15 @@ def apply_address_import(
 
     created_ids: list[str] = []
     updated: list[dict[str, Any]] = []
-    for item in run.plan_data or []:
+    # SQLAlchemy JSON columns do not track nested mutations. Keep the persisted
+    # preview untouched until assigning a genuinely changed complete plan.
+    applied_plan = copy.deepcopy(run.plan_data or [])
+    for item in applied_plan:
         action = item.get("action")
         if action in {"ignored", "unchanged"}:
             continue
         source_ref = str(item["source_ref"])
-        existing = _contact_by_source_ref(session, run.address_book_id, source_ref)
+        existing = _contact_by_source_ref(session, run.address_book_id, source_ref, lock=True)
         if action == "create":
             if existing is not None and existing.deleted_at is None:
                 raise AddressBookError("A target contact appeared after preview; preview the import again.")
@@ -299,18 +331,27 @@ def apply_address_import(
         elif action == "update":
             if existing is None:
                 raise AddressBookError("An import target disappeared after preview; preview the import again.")
+            _lock_contact_points(session, existing)
             if _contact_hash(existing) != item.get("expected_contact_hash"):
                 raise AddressBookError(
                     f'Contact "{existing.display_name}" changed after preview; preview the import again.'
                 )
-            before = _contact_snapshot(existing)
+            _require_mutable_book(existing.address_book)
+            before = {
+                "version": ROLLBACK_SNAPSHOT_VERSION,
+                "contact": copy.deepcopy(_contact_snapshot(existing)),
+                "deleted_at": _deleted_at_value(existing.deleted_at),
+            }
+            update_payload = ContactUpdateRequest.model_validate(item["payload"])
+            points = _prepare_import_points(session, existing, update_payload)
             if existing.deleted_at is not None:
                 restore_contact(session, principal, existing.id)
+            _apply_import_points(existing, points)
             contact = update_contact(
                 session,
                 principal,
                 existing.id,
-                ContactUpdateRequest.model_validate(item["payload"]),
+                _without_contact_points(update_payload),
             )
             _stamp_import_contact(contact, run=run, item=item)
             session.flush()
@@ -319,7 +360,7 @@ def apply_address_import(
 
     run.status = "applied"
     run.applied_at = utcnow()
-    run.plan_data = list(run.plan_data or [])
+    run.plan_data = applied_plan
     run.result_evidence = {
         "input_hash": run.input_hash,
         "plan_hash": run.plan_hash,
@@ -337,7 +378,7 @@ def rollback_address_import(
     run_id: str,
     payload: AddressImportRollbackRequest,
 ) -> AddressImportRun:
-    run = get_import_run(session, principal, run_id)
+    run = get_import_run(session, principal, run_id, lock=True)
     if run.plan_hash != payload.expected_plan_hash:
         raise AddressBookError("The reviewed import plan changed; reload the import run.")
     if run.status == "rolled_back":
@@ -347,6 +388,12 @@ def rollback_address_import(
     evidence = dict(run.result_evidence or {})
     updated = list(evidence.get("updated_contacts") or [])
     created_ids = list(evidence.get("created_contact_ids") or [])
+    # Validate every before-image before touching any contact. Older runs did
+    # not record deletion state, so automatic recovery cannot infer it safely.
+    before_images = {
+        str(item["contact_id"]): _validated_rollback_snapshot(item.get("before"))
+        for item in updated
+    }
 
     expected_hashes = {
         str(item["contact_id"]): str(item["after_hash"])
@@ -359,12 +406,20 @@ def rollback_address_import(
             if item.get("contact_id") in created_ids and item.get("after_hash")
         }
     )
-    for contact_id, expected_hash in expected_hashes.items():
-        contact = get_visible_contact(session, principal, contact_id, include_deleted=True)
+    if not set(created_ids).issubset(expected_hashes):
+        raise AddressBookError("Import rollback evidence is incomplete; automatic rollback is unsafe.")
+    for contact_id, expected_hash in sorted(expected_hashes.items()):
+        contact = get_visible_contact(session, principal, contact_id, include_deleted=True, lock=True)
+        _lock_contact_points(session, contact)
+        if contact.address_book_id != run.address_book_id:
+            raise AddressBookError("An import target moved to another address book; automatic rollback is unsafe.")
+        _require_mutable_book(contact.address_book)
         if _contact_hash(contact) != expected_hash:
             raise AddressBookError(
                 f'Contact "{contact.display_name}" changed after import; automatic rollback is unsafe.'
             )
+        if contact_id in before_images:
+            _validate_point_restoration(session, contact, before_images[contact_id][0]["points"])
 
     for contact_id in created_ids:
         contact = get_visible_contact(session, principal, contact_id, include_deleted=True)
@@ -372,22 +427,23 @@ def rollback_address_import(
             delete_contact(session, principal, contact.id)
     for item in updated:
         contact = get_visible_contact(session, principal, str(item["contact_id"]), include_deleted=True)
-        snapshot = dict(item["before"])
+        snapshot, previous_deleted_at = before_images[str(item["contact_id"])]
         if contact.deleted_at is not None:
             restore_contact(session, principal, contact.id)
-        update_contact(
-            session,
-            principal,
-            contact.id,
-            ContactUpdateRequest.model_validate(snapshot["payload"]),
-        )
-        contact.source_kind = snapshot.get("source_kind") or "local"
-        contact.source_ref = snapshot.get("source_ref")
-        contact.source_revision = snapshot.get("source_revision")
-        contact.source_payload_kind = snapshot.get("source_payload_kind")
-        contact.source_payload_raw = snapshot.get("source_payload_raw")
-        contact.provenance = dict(snapshot.get("provenance") or {})
-        contact.metadata_ = dict(snapshot.get("metadata") or {})
+        previous = _contact_change_payload(contact, prefix="previous_")
+        _restore_contact_points(contact, snapshot["points"])
+        # A validated stored before-image is not a fresh user edit: do not trim,
+        # normalize or coerce it through the generic update path a second time.
+        for field in ("display_name", "given_name", "family_name", "organization", "role_title", "note", "tags"):
+            setattr(contact, field, copy.deepcopy(snapshot["payload"][field]))
+        for field in ("source_kind", "source_ref", "source_revision", "source_payload_kind", "source_payload_raw", "provenance"):
+            setattr(contact, field, copy.deepcopy(snapshot[field]))
+        contact.metadata_ = copy.deepcopy(snapshot["metadata"])
+        contact.updated_by_account_id = _account_id(principal)
+        _record_address_contact_change(session, principal, contact=contact, operation="updated", previous=previous)
+        if previous_deleted_at is not None:
+            delete_contact(session, principal, contact.id)
+            contact.deleted_at = previous_deleted_at
 
     run.status = "rolled_back"
     run.rolled_back_at = utcnow()
@@ -701,6 +757,12 @@ def _plan_rows(
         first_index.setdefault(key, index)
         last_index[key] = index
 
+    existing_contacts = _contacts_by_source_refs(
+        session,
+        book_id,
+        [f"import:{profile.profile_key}:{key}" for key in key_counts],
+    )
+
     for index, (row_number, row, key) in enumerate(keyed_rows):
         if key_counts[key] > 1:
             if config.duplicate_source_key_policy == "reject":
@@ -721,7 +783,7 @@ def _plan_rows(
         )
         diagnostics.extend(row_diagnostics)
         source_ref = f"import:{profile.profile_key}:{key}"
-        existing = _contact_by_source_ref(session, book_id, source_ref)
+        existing = existing_contacts.get(source_ref)
         payload = _payload_from_mapped(
             mapped,
             profile=profile,
@@ -951,13 +1013,79 @@ def _diagnostic(
     }
 
 
-def _contact_by_source_ref(session: Session, book_id: str, source_ref: str) -> Contact | None:
-    return (
+def _contacts_by_source_refs(session: Session, book_id: str, source_refs: list[str]) -> dict[str, Contact]:
+    contacts: dict[str, Contact] = {}
+    for offset in range(0, len(source_refs), CONTACT_LOOKUP_BATCH_SIZE):
+        candidates = (
+            select(
+                Contact.id,
+                func.row_number().over(
+                    partition_by=Contact.source_ref,
+                    order_by=(Contact.created_at.asc(), Contact.id.asc()),
+                ).label("source_position"),
+            )
+            .where(
+                Contact.address_book_id == book_id,
+                Contact.source_ref.in_(source_refs[offset : offset + CONTACT_LOOKUP_BATCH_SIZE]),
+            )
+            .subquery()
+        )
+        rows = (
+            session.query(Contact)
+            .join(candidates, candidates.c.id == Contact.id)
+            .filter(candidates.c.source_position == 1)
+            .options(selectinload(Contact.emails), selectinload(Contact.phones), selectinload(Contact.postal_addresses))
+            .populate_existing()
+            .order_by(Contact.created_at.asc(), Contact.id.asc())
+            .all()
+        )
+        for contact in rows:
+            # Preserve the historical first-match choice for duplicate stored
+            # source references, independently of batch/database row order.
+            contacts.setdefault(str(contact.source_ref), contact)
+    return contacts
+
+
+def _contact_by_source_ref(session: Session, book_id: str, source_ref: str, *, lock: bool = False) -> Contact | None:
+    query = (
         session.query(Contact)
         .filter(Contact.address_book_id == book_id, Contact.source_ref == source_ref)
         .order_by(Contact.created_at.asc(), Contact.id.asc())
-        .first()
     )
+    if lock:
+        query = query.populate_existing().with_for_update()
+    return query.first()
+
+
+def _validated_rollback_snapshot(value: object) -> tuple[dict[str, Any], datetime | None]:
+    if (
+        not isinstance(value, dict)
+        or value.get("version") != ROLLBACK_SNAPSHOT_VERSION
+        or "deleted_at" not in value
+        or not isinstance(value.get("contact"), dict)
+    ):
+        raise AddressBookError(
+            "This import has incomplete legacy rollback evidence; automatic rollback is unsafe. "
+            "Review and reconcile the affected contacts manually."
+        )
+    snapshot = value["contact"]
+    required_fields = {
+        "display_name", "given_name", "family_name", "organization", "role_title",
+        "note", "tags", "emails", "phones", "postal_addresses", "provenance",
+    }
+    if (
+        not isinstance(snapshot.get("payload"), dict)
+        or not required_fields.issubset(snapshot["payload"])
+        or not {"source_kind", "source_ref", "source_revision", "source_payload_kind", "source_payload_raw", "provenance", "metadata"}.issubset(snapshot)
+    ):
+        raise AddressBookError("Import rollback evidence is incomplete; automatic rollback is unsafe.")
+    try:
+        deleted_at = datetime.fromisoformat(value["deleted_at"]) if value["deleted_at"] is not None else None
+        ContactUpdateRequest.model_validate(snapshot["payload"])
+        _validate_point_snapshot(snapshot.get("points"))
+    except (TypeError, ValueError) as exc:
+        raise AddressBookError("Import rollback evidence is invalid; automatic rollback is unsafe.") from exc
+    return snapshot, deleted_at
 
 
 def _stamp_import_contact(contact: Contact, *, run: AddressImportRun, item: dict[str, Any]) -> None:
@@ -1007,12 +1135,176 @@ def _contact_snapshot(contact: Contact) -> dict[str, Any]:
         "source_payload_kind": contact.source_payload_kind,
         "source_payload_raw": contact.source_payload_raw,
         "provenance": dict(contact.provenance or {}),
-        "metadata": dict(contact.metadata_ or {}),
+        "metadata": copy.deepcopy(contact.metadata_),
+        "points": {
+            name: [_point_snapshot(point) for point in getattr(contact, name)]
+            for name in CONTACT_POINTS
+        },
     }
 
 
 def _contact_hash(contact: Contact) -> str:
-    return _hash_json({**_contact_snapshot(contact), "deleted_at": contact.deleted_at.isoformat() if contact.deleted_at else None})
+    return _hash_json({**_contact_snapshot(contact), "deleted_at": _deleted_at_value(contact.deleted_at)})
+
+
+def _point_snapshot(point: ContactEmail | ContactPhone | ContactPostalAddress) -> dict[str, Any]:
+    # Include every persisted evidence field, including identity, originals,
+    # normalization, provenance, ordering and timestamps, but never the parent FK.
+    return {
+        column.key: (
+            _deleted_at_value(getattr(point, column.key))
+            if column.key in {"created_at", "updated_at"}
+            else copy.deepcopy(getattr(point, column.key))
+        )
+        for column in point.__table__.columns if column.key != "contact_id"
+    }
+
+
+def _lock_contact_points(session: Session, contact: Contact) -> None:
+    # The parent is locked by the caller. Point-only evidence updates need their
+    # own row locks; they need not update the parent row on PostgreSQL.
+    for name, (model, _identity, _replace) in CONTACT_POINTS.items():
+        positions = {point.id: index for index, point in enumerate(getattr(contact, name))}
+        points = session.query(model).filter(model.contact_id == contact.id).order_by(
+            model.id.asc(),
+        ).populate_existing().with_for_update().all()
+        # Lock acquisition order is not presentation order. Preserve the
+        # relationship's existing order for tied indexes; new identities still
+        # enter the hash and therefore cannot evade the reviewed-state guard.
+        points.sort(key=lambda point: (point.order_index, positions.get(point.id, len(positions)), point.id))
+        set_committed_value(contact, name, points)
+
+
+def _validate_point_snapshot(value: object) -> None:
+    if not isinstance(value, dict) or set(value) != set(CONTACT_POINTS):
+        raise ValueError("Missing complete contact-point evidence")
+    for name, (model, _identity, _replace) in CONTACT_POINTS.items():
+        points = value[name]
+        columns = {column.key: column for column in model.__table__.columns if column.key != "contact_id"}
+        ids: set[str] = set()
+        if not isinstance(points, list):
+            raise ValueError("Invalid contact-point collection")
+        for point in points:
+            if not isinstance(point, dict) or set(point) != set(columns):
+                raise ValueError("Incomplete contact-point evidence")
+            for key, column in columns.items():
+                field = point[key]
+                if field is None and column.nullable:
+                    continue
+                if key in {"created_at", "updated_at"}:
+                    datetime.fromisoformat(field)
+                elif type(field) is not column.type.python_type:
+                    raise ValueError("Invalid contact-point evidence type")
+            if not point["id"] or point["id"] in ids:
+                raise ValueError("Invalid contact-point identity")
+            ids.add(point["id"])
+
+
+def _without_contact_points(payload: ContactUpdateRequest) -> ContactUpdateRequest:
+    return ContactUpdateRequest.model_validate(
+        payload.model_dump(exclude=set(CONTACT_POINTS), exclude_unset=True)
+    )
+
+
+def _require_unreferenced_points(session: Session, contact: Contact, removed: set[str]) -> None:
+    if not removed:
+        return
+    # Do not detach address-list selections or silently retarget point-specific
+    # consent/quality decisions when an import removes or replaces a value.
+    referenced = session.query(AddressListEntry.id).filter(
+        AddressListEntry.contact_id == contact.id,
+        or_(AddressListEntry.contact_email_id.in_(removed), AddressListEntry.contact_postal_address_id.in_(removed)),
+    ).first()
+    for model in (ContactChannelRule, ContactPointQualityDecision):
+        if referenced is not None:
+            break
+        referenced = session.query(model.id).filter(
+            model.contact_id == contact.id, model.contact_point_id.in_(removed),
+        ).first()
+    if referenced is not None:
+        raise AddressBookError(
+            "An affected contact point has address-list or governance references; "
+            "review and reconcile those references before applying or rolling back this import."
+        )
+
+
+def _prepare_import_points(session: Session, contact: Contact, payload: ContactUpdateRequest):
+    staged = Contact(
+        source_kind=contact.source_kind, source_ref=contact.source_ref,
+        source_revision=contact.source_revision,
+        created_by_account_id=contact.created_by_account_id,
+        updated_by_account_id=contact.updated_by_account_id,
+    )
+    result = {}
+    removed: set[str] = set()
+    for name, (_model, identity, replace) in CONTACT_POINTS.items():
+        if name not in payload.model_fields_set:
+            continue
+        replace(staged, getattr(payload, name) or [])
+        available = list(getattr(contact, name))
+        matches = []
+        for candidate in list(getattr(staged, name)):
+            # Detach the normalization-only parent before a new point enters the
+            # persistent collection; otherwise save-update cascade can enlist it.
+            candidate.contact = None
+            original = next((point for point in available if all(
+                getattr(point, key) == getattr(candidate, key) for key in identity
+            )), None)
+            if original is not None:
+                available.remove(original)
+            matches.append((original, candidate))
+        removed.update(point.id for point in available)
+        result[name] = matches
+    _require_unreferenced_points(session, contact, removed)
+    return result
+
+
+def _apply_import_points(contact: Contact, prepared) -> None:
+    for name, matches in prepared.items():
+        points = []
+        for original, candidate in matches:
+            if original is None:
+                points.append(candidate)
+            else:
+                # Same value retains its exact original/normalized evidence and
+                # identity; only explicitly imported presentation fields change.
+                for field in ("label", "is_primary", "order_index"):
+                    setattr(original, field, getattr(candidate, field))
+                points.append(original)
+        setattr(contact, name, points)
+
+
+def _validate_point_restoration(session: Session, contact: Contact, points: dict[str, Any]) -> None:
+    removed: set[str] = set()
+    for name, (model, _identity, _replace) in CONTACT_POINTS.items():
+        desired = {point["id"] for point in points[name]}
+        removed.update(point.id for point in getattr(contact, name) if point.id not in desired)
+        # A deleted original identity must never be reclaimed from another contact.
+        if desired and session.query(model.id).filter(
+            model.id.in_(desired), model.contact_id != contact.id,
+        ).first() is not None:
+            raise AddressBookError("Contact-point identity changed; automatic rollback is unsafe.")
+    _require_unreferenced_points(session, contact, removed)
+
+
+def _restore_contact_points(contact: Contact, snapshots: dict[str, Any]) -> None:
+    for name, (model, _identity, _replace) in CONTACT_POINTS.items():
+        existing = {point.id: point for point in getattr(contact, name)}
+        restored = []
+        for snapshot in snapshots[name]:
+            point = existing.get(snapshot["id"])
+            if point is None:
+                point = model()
+            for key, value in snapshot.items():
+                setattr(point, key, datetime.fromisoformat(value) if key in {"created_at", "updated_at"} else copy.deepcopy(value))
+            restored.append(point)
+        setattr(contact, name, restored)
+
+
+def _deleted_at_value(value: datetime | None) -> str | None:
+    if value is None:
+        return None
+    return (value.replace(tzinfo=UTC) if value.tzinfo is None else value.astimezone(UTC)).isoformat()
 
 
 def _hash_json(value: object) -> str: