From fd792f6d78f4b537276a92b0d77f5d150940838e Mon Sep 17 00:00:00 2001 From: Marcel Date: Sat, 6 Jun 2026 11:38:07 +0200 Subject: [PATCH] feat(person): enforce person-delete integrity at the DB layer (V71) (#684) Add ON DELETE behaviour to the two V1 FKs into persons (documents.sender_id -> SET NULL, document_receivers.person_id -> CASCADE) and a real FK with ON DELETE CASCADE on the transcription_block_mentioned_persons soft reference, cleaning up pre-existing orphan mention rows first. The cascade stays strictly at the join/reference layer and never reaches documents rows. Proven by new Postgres-backed PersonRepositoryTest cascade tests (AC-1/2/3/8 plus the cascade-boundary document-survival guard). Rewrites the now-stale V56 'no FK' comment. Co-Authored-By: Claude Opus 4.8 --- ..._transcription_block_mentioned_persons.sql | 11 +- .../V71__person_delete_on_delete_fk.sql | 50 +++++++ .../person/PersonRepositoryTest.java | 130 ++++++++++++++++++ 3 files changed, 186 insertions(+), 5 deletions(-) create mode 100644 backend/src/main/resources/db/migration/V71__person_delete_on_delete_fk.sql diff --git a/backend/src/main/resources/db/migration/V56__add_transcription_block_mentioned_persons.sql b/backend/src/main/resources/db/migration/V56__add_transcription_block_mentioned_persons.sql index 80a44ef4..5620ced6 100644 --- a/backend/src/main/resources/db/migration/V56__add_transcription_block_mentioned_persons.sql +++ b/backend/src/main/resources/db/migration/V56__add_transcription_block_mentioned_persons.sql @@ -8,12 +8,13 @@ -- all blocks mentioning person X" query on the person detail page joins on -- the indexed person_id column — equally fast as JSONB GIN containment, no -- new dependency. document_comments.comment_mentions stays as a many-to-many --- to AppUser; the divergence is intentional: Person mentions need lazy --- degradation when a person is deleted (no FK), while user mentions don't. +-- to AppUser; the divergence is intentional. -- --- No FK on person_id: when a Person is deleted we want @Auguste Raddatz to --- remain visible as plain unlinked text inside the transcription rather than --- vanishing or cascade-deleting the block. +-- person_id FK: added in V71 with ON DELETE CASCADE (this column was originally +-- created without a FK — that decision was reversed). The literal "@Name" in +-- transcription_blocks.text keeps the name visible after a person is deleted; +-- the sidecar row carries only the link and is removed with the person, so the +-- read renderer degrades to plain unlinked text. See ADR-032. CREATE TABLE transcription_block_mentioned_persons ( block_id UUID NOT NULL REFERENCES transcription_blocks(id) ON DELETE CASCADE, diff --git a/backend/src/main/resources/db/migration/V71__person_delete_on_delete_fk.sql b/backend/src/main/resources/db/migration/V71__person_delete_on_delete_fk.sql new file mode 100644 index 00000000..252c215b --- /dev/null +++ b/backend/src/main/resources/db/migration/V71__person_delete_on_delete_fk.sql @@ -0,0 +1,50 @@ +-- Move person-delete referential integrity from application code into the database (#684). +-- +-- Before this migration, PersonService.deletePerson nulled documents.sender_id and removed +-- document_receivers rows in Java before deleting the person, because the two V1 FKs into +-- persons had no ON DELETE behaviour. Any other delete path (a future endpoint, a manual +-- psql, a batch job) could still orphan rows or 500. This migration makes the database the +-- single source of truth so a person delete is safe from every path. +-- +-- Cascade boundary: the cascade stays STRICTLY at the join/reference layer and NEVER reaches +-- documents rows — a cascade into documents would destroy historical letters. sender_id is +-- SET NULL (documents.senderText preserves the raw textual attribution); the receiver join +-- row and the @-mention sidecar row are dropped. +-- +-- No NOT VALID + VALIDATE two-step: these tables are small (thousands of rows → sub-second +-- ACCESS EXCLUSIVE lock). Do NOT copy this drop-and-recreate pattern onto a large table. +-- +-- Not audit-logged: a DB ON DELETE cascade runs below AuditService — a known, accepted trade. +-- The person-delete action itself is still logged at the service layer. + +-- documents.sender_id → ON DELETE SET NULL (deleted sender clears the link; the document survives). +ALTER TABLE public.documents + DROP CONSTRAINT fkl5xhww7es3b4um01vmly4y18m, + ADD CONSTRAINT fkl5xhww7es3b4um01vmly4y18m + FOREIGN KEY (sender_id) REFERENCES public.persons(id) ON DELETE SET NULL; + +-- document_receivers.person_id → ON DELETE CASCADE (drop the join row), the symmetric +-- completion of V14, which added the same to the document_id side of this table. +ALTER TABLE public.document_receivers + DROP CONSTRAINT fkcg7r68qvosqricx1betgrlt7s, + ADD CONSTRAINT fkcg7r68qvosqricx1betgrlt7s + FOREIGN KEY (person_id) REFERENCES public.persons(id) ON DELETE CASCADE; + +-- Soft reference fix: transcription_block_mentioned_persons.person_id was a UUID with no FK +-- (V56), so deleting a person left dangling mention rows. Give it a real FK with CASCADE. +-- Clean up pre-existing orphans first — production likely holds dangling rows because the old +-- deletePerson never cleaned mention rows, and the ADD CONSTRAINT validation scan fails on them. +-- A DO block with RAISE NOTICE surfaces the purge count: Flyway runs each statement via JDBC +-- and discards a trailing SELECT's result set, so a "SELECT count(*)" would log nothing. +DO $$ +DECLARE removed int; +BEGIN + DELETE FROM transcription_block_mentioned_persons m + WHERE NOT EXISTS (SELECT 1 FROM persons p WHERE p.id = m.person_id); + GET DIAGNOSTICS removed = ROW_COUNT; + RAISE NOTICE 'V71 orphaned_mention_rows_removed=%', removed; +END $$; + +ALTER TABLE public.transcription_block_mentioned_persons + ADD CONSTRAINT fk_tbmp_person + FOREIGN KEY (person_id) REFERENCES public.persons(id) ON DELETE CASCADE; diff --git a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java index add05ed7..dbccb285 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java @@ -21,6 +21,7 @@ import jakarta.persistence.PersistenceContext; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.UUID; import static org.assertj.core.api.Assertions.assertThat; @@ -707,4 +708,133 @@ class PersonRepositoryTest { assertThat(found).isPresent(); assertThat(found.get().getGeneration()).isNull(); } + + // ─── #684: ON DELETE integrity enforced at the database layer ────────────── + // A raw deleteById (bypassing PersonService) must keep referential integrity: + // documents.sender_id → SET NULL, document_receivers.person_id → CASCADE, and the + // transcription_block_mentioned_persons soft reference → CASCADE. These run against + // real Postgres because the FK ON DELETE behaviour never fires on H2. + + @Test + void deleteById_personSenderOfAReceiverOfB_nullsSender_dropsReceiverRow_bothDocumentsSurvive() { + Person target = personRepository.save(Person.builder().firstName("Weg").lastName("Person").build()); + Person bystander = personRepository.save(Person.builder().firstName("Bleibt").lastName("Hier").build()); + + Document sent = documentRepository.save(Document.builder() + .title("Gesendet").originalFilename("sent.pdf") + .status(DocumentStatus.UPLOADED).sender(target).build()); + Document received = documentRepository.save(Document.builder() + .title("Empfangen").originalFilename("received.pdf") + .status(DocumentStatus.UPLOADED).sender(bystander) + .receivers(Set.of(target)).build()); + entityManager.flush(); + entityManager.clear(); + + personRepository.deleteById(target.getId()); + entityManager.flush(); + entityManager.clear(); + + assertThat(personRepository.findById(target.getId())).isEmpty(); + + Document reloadedSent = documentRepository.findById(sent.getId()).orElseThrow(); + assertThat(reloadedSent.getSender()).isNull(); // AC-1: SET NULL + + Document reloadedReceived = documentRepository.findById(received.getId()).orElseThrow(); + assertThat(reloadedReceived.getReceivers()) + .noneMatch(p -> p.getId().equals(target.getId())); // AC-2: CASCADE drops the join row + + // Cascade-boundary guard (Nora, non-negotiable): the cascade stops at the join/reference + // layer — both documents themselves survive. Guards against a future migration turning + // documents.sender_id SET NULL into CASCADE and destroying historical letters. + assertThat(documentRepository.findById(sent.getId())).isPresent(); + assertThat(documentRepository.findById(received.getId())).isPresent(); + } + + @Test + void deleteById_receiverWithCoReceiver_dropsOnlyDeletedPersonsJoinRow() { + Person target = personRepository.save(Person.builder().firstName("Weg").lastName("Person").build()); + Person coReceiver = personRepository.save(Person.builder().firstName("Mit").lastName("Empfänger").build()); + Person sender = personRepository.save(Person.builder().firstName("Send").lastName("Er").build()); + + Document doc = documentRepository.save(Document.builder() + .title("Brief").originalFilename("brief.pdf") + .status(DocumentStatus.UPLOADED).sender(sender) + .receivers(Set.of(target, coReceiver)).build()); + entityManager.flush(); + entityManager.clear(); + + personRepository.deleteById(target.getId()); + entityManager.flush(); + entityManager.clear(); + + Document reloaded = documentRepository.findById(doc.getId()).orElseThrow(); + assertThat(reloaded.getReceivers()).extracting(Person::getId) + .containsExactly(coReceiver.getId()); // co-receiver untouched + } + + @Test + void deleteById_personIsSenderAndReceiverOfSameDocument_documentSurvives_senderNull_receiverDropped() { + // AC-8: the trickier same-document interaction the cross-document cases don't exercise. + Person target = personRepository.save(Person.builder().firstName("Beides").lastName("Person").build()); + Person coReceiver = personRepository.save(Person.builder().firstName("Mit").lastName("Empfänger").build()); + + Document doc = documentRepository.save(Document.builder() + .title("Selbstbrief").originalFilename("self.pdf") + .status(DocumentStatus.UPLOADED).sender(target) + .receivers(Set.of(target, coReceiver)).build()); + entityManager.flush(); + entityManager.clear(); + + personRepository.deleteById(target.getId()); + entityManager.flush(); + entityManager.clear(); + + Document reloaded = documentRepository.findById(doc.getId()).orElseThrow(); + assertThat(reloaded.getSender()).isNull(); + assertThat(reloaded.getReceivers()).extracting(Person::getId) + .containsExactly(coReceiver.getId()); + } + + @Test + void deleteById_mentionedPerson_dropsMentionRow_blockTextSurvives() { + // AC-3: the @-mention sidecar is a CASCADE soft reference, but the literal "@Name" lives + // in transcription_blocks.text and must stay visible as plain text after the person goes. + Person mentioned = personRepository.save(Person.builder().firstName("Auguste").lastName("Raddatz").build()); + Document doc = documentRepository.save(Document.builder() + .title("Brief").originalFilename("brief.pdf") + .status(DocumentStatus.UPLOADED).build()); + entityManager.flush(); + + UUID annotationId = UUID.randomUUID(); + UUID blockId = UUID.randomUUID(); + entityManager.createNativeQuery( + "INSERT INTO document_annotations (id, document_id, page_number, x, y, width, height, color) " + + "VALUES (?1, ?2, 1, 0.1, 0.2, 0.3, 0.1, '#fff')") + .setParameter(1, annotationId).setParameter(2, doc.getId()).executeUpdate(); + entityManager.createNativeQuery( + "INSERT INTO transcription_blocks (id, annotation_id, document_id, text) VALUES (?1, ?2, ?3, ?4)") + .setParameter(1, blockId).setParameter(2, annotationId).setParameter(3, doc.getId()) + .setParameter(4, "Brief an @Auguste Raddatz").executeUpdate(); + entityManager.createNativeQuery( + "INSERT INTO transcription_block_mentioned_persons (block_id, person_id, display_name) " + + "VALUES (?1, ?2, ?3)") + .setParameter(1, blockId).setParameter(2, mentioned.getId()) + .setParameter(3, "Auguste Raddatz").executeUpdate(); + entityManager.flush(); + entityManager.clear(); + + personRepository.deleteById(mentioned.getId()); + entityManager.flush(); + entityManager.clear(); + + Number mentionRows = (Number) entityManager.createNativeQuery( + "SELECT count(*) FROM transcription_block_mentioned_persons WHERE person_id = ?1") + .setParameter(1, mentioned.getId()).getSingleResult(); + assertThat(mentionRows.longValue()).isZero(); + + String text = (String) entityManager.createNativeQuery( + "SELECT text FROM transcription_blocks WHERE id = ?1") + .setParameter(1, blockId).getSingleResult(); + assertThat(text).isEqualTo("Brief an @Auguste Raddatz"); + } }