Move person-delete FK detach to database-level ON DELETE (#684) #736
Reference in New Issue
Block a user
Delete Branch "feat/issue-684-person-delete-db-fk"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #684.
Moves person-delete referential integrity from application code into the database, removes the now-redundant app-layer detach, simplifies
mergePersons, and closes the@-mention soft-reference gap. This fixes the pre-existing dead-link-on-deleted-person case — it is not a purely invisible refactor.What changed
V71__person_delete_on_delete_fk.sql(one atomic migration):documents.sender_id→ON DELETE SET NULL(document survives;senderTextpreserves the raw attribution).document_receivers.person_id→ON DELETE CASCADE(symmetric completion of V14).transcription_block_mentioned_persons.person_id→ real FK withON DELETE CASCADE, reversing V56's "no FK". Pre-existing orphan rows are purged first in aDO/RAISE NOTICEblock (Flyway discards a trailingSELECT).PersonService.deletePersonthinned togetById+deleteById;mergePersonsdrops the explicitdeleteReceiverReferencesand leans on the cascade.PersonRepository:reassignSenderToNull+deleteReceiverReferencesdeleted;reassignSender/insertMissingReceiverReferencegetclearAutomatically/flushAutomatically. NoEntityManagerinjected.Acceptance criteria
SET NULLPersonRepositoryTestcascade testsCASCADE+ co-receivers intactPersonRepositoryTestCASCADE+ block text survivesPersonRepositoryTestPersonServiceIntegrationTestEntityManager)PersonService/PersonRepository+ rewritten unit testsmention.spec.tsPersonServiceIntegrationTestPersonRepositoryTestTesting
PersonServiceTest(57) +PersonRepositoryTest(49) +PersonServiceIntegrationTest(12); migrations V1→V71 run against real Postgres via Testcontainers. Full backend compiles.mention.spec.ts47 green; zero newsvelte-checkerrors in touched files.Notes
mainadvanced and 028–031 are now taken.SELECT count(*) FROM transcription_block_mentioned_persons m WHERE NOT EXISTS (SELECT 1 FROM persons p WHERE p.id = m.person_id);— V71 logs the purge asV71 orphaned_mention_rows_removed=N.🤖 Generated with Claude Code
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Clean-room review of PR #736 against issue #684 (AC-1..AC-8, D1-D5). Every acceptance criterion is satisfied and traceable to a test; every locked decision is honored. No scope gaps, no scope creep. The one stated deviation (ADR-032 vs the issue's ADR-028) is correct and properly justified.
AC-by-AC coverage trace (AC → test)
SET NULL, document survivesPersonRepositoryTest.deleteById_personSenderOfAReceiverOfB_…— assertsreloadedSent.getSender()is null and doc present. Migration setsON DELETE SET NULL.CASCADE, doc + co-receivers survivedeleteById_receiverWithCoReceiver_dropsOnlyDeletedPersonsJoinRow— co-receiver kept viacontainsExactly(coReceiver).CASCADEand block text unchangeddeleteById_mentionedPerson_dropsMentionRow_blockTextSurvives— asserts 0 mention rows andtext == "Brief an @Auguste Raddatz". Both halves of the AC tested.PersonServiceIntegrationTest.deletePerson_…_noOrphankept green; D3/Leonie confirmed no/persons/reviewcopy change — correctly absent.EntityManager, unit tests rewritten)reassignSenderToNull+deleteReceiverReferencesdeleted fromPersonRepository;deletePersonthinned togetById+deleteById; noEntityManagerinjected; both Mockito tests rewritten withverifyNoMoreInteractions.mention.spec.tsstrengthened — asserts plain textAuguste Raddatzpresent and none of<a,person-mention,data-person-id,href. Exactly the contract D3 specified.PersonServiceIntegrationTest.mergePersons_targetInheritsReferences_sourceJoinRowCascadeDrops_noFkError— target becomes sender of A, receiverscontainsExactlyInAnyOrder(target, coReceiver).deleteById_personIsSenderAndReceiverOfSameDocument_…— the trickiest interaction, with a co-receiver guard. This was the AC I (Elicit) and Sara pushed for; it is present and precise.Bonus guard honored: Nora's non-negotiable cascade-boundary assertion ("both documents survive") is present and explicitly commented in the AC-1/2 test — this protects against a future migration silently turning
SET NULLintoCASCADEondocuments.sender_id. Good.Decisions D1-D5
fk_tbmp_personadded withON DELETE CASCADE, orphans purged first.mergePersons) — ✅ honored;deleteReceiverReferencescall removed, cascade relied upon,reassignSender/insertMissingReceiverReferencekept.EntityManager;@Modifying(clear/flush)on native queries; test-side staleness) — ✅ honored exactly.On the ADR-028 → ADR-032 deviation — acceptable
I independently checked
docs/adr/on the branch:028-pdfjs-wasm-decoders-and-csp-constraint.mdthrough031-document-title-…are all occupied. The issue's "028" was written beforemainadvanced; 032 is the genuine next free number. The PR notes call this out explicitly. From a requirements standpoint the intent of D5 — "a durable, discoverable record exists" — is fully met; the number is an addressing detail, not a requirement. No concern.Blockers
None.
Suggestions (non-blocking, optional)
Requirements verdict: this PR is a model of spec-to-test traceability. Every AC has a named, asserting test; every decision is visible in the diff; and the work correctly reframes itself as a fix for the dead-link-on-deleted-person case (AC-6) rather than an invisible refactor — exactly as the issue's framing note asked.
👨💻 Felix Brandt — Senior Fullstack Developer
✅ Approved
Clean-room review of the diff, no prior context. This is exactly how I'd want a "move integrity to the DB" refactor to land: the tests prove the new behaviour against the substrate that actually exercises it, the service shrinks honestly, and the dead code is deleted rather than commented out.
TDD & test soundness — verified, not assumed
The whole premise of this PR is "Postgres
ON DELETEdoes the work now," so I checked that the cascade tests run on real Postgres and not H2 (where FKON DELETEsilently no-ops):PersonRepositoryTest—@DataJpaTest+@AutoConfigureTestDatabase(replace = NONE)+@Import({PostgresContainerConfig.class, FlywayConfig.class}).PostgresContainerConfigspins uppostgres:16-alpinevia Testcontainers with@ServiceConnection, andFlywayConfigruns V1→V71. The cascades genuinely fire. ✅PersonServiceIntegrationTest—@SpringBootTest+@Import(PostgresContainerConfig.class), same container. ✅The four repo cascade tests each
flush()/clear()before and after thedeleteById, so the assertions read post-cascade DB state through a cold L1 cache rather than stale managed entities. The same-document sender+receiver edge (AC-8) and the co-receiver-survival case are the two I'd have asked for explicitly — both present. The Nora cascade-boundary guard (both documents survive) is a real assertion, not a comment, and it's the right thing to pin against a future migration flippingSET NULLintoCASCADE.verifyNoMoreInteractionsrewrites — soundCross-checked both unit verifications against the service bodies:
deletePerson→getById→findById+deleteById, andverifyNoMoreInteractionsnow catches any reintroduced detach call. ✅mergePersons→findById(source),findById(target),reassignSender,insertMissingReceiverReference,deleteById— all five verified, exhaustively closed. This is stricter than the loose verifies it replaced; the old tests would have passed even ifdeleteReceiverReferenceswere still called. Good upgrade.Frontend degradation test — contract is accurate
Decoded
mention.ts: withmentionedPersons: [],renderTranscriptionBodyshort-circuits the substitution loop and returnsescapeHtml(text)unchanged. So the test's four negative assertions (<a,person-mention,data-person-id,hrefall absent) plus the verbatim-text positive assertion genuinely lock the "deleted person degrades to plain text" contract. The renamed test (renders nothing…→renders a deleted-person @mention as plain text…) now describes a real AC-6 behaviour rather than an empty-input no-op — net coverage gain, not a swap. No Svelte 5 concerns; this is a pure-function unit test.Clean code
deletePerson/mergePersonsare down to guard-then-act, happy path at the lowest indentation. The new Javadoc explains why (the cascade) rather than what — correct use of comments.DO/RAISE NOTICEvs trailing-SELECTFlyway detail, the orphan pre-clean rationale) — keep them.reassignSenderToNullanddeleteReceiverReferencesfully removed from both repo and tests; no orphaned references. NoEntityManagerinjected into the repo. ✅Suggestions (non-blocking)
PersonRepository.javahas no trailing newline (\ No newline at end of file). Trivial, but worth fixing while the file is open — POSIX-clean and avoids a noisy diff next time the interface changes.mergePersonsstill throwsResponseStatusExceptionfor the same-source-target guard while the not-found paths useDomainException. Pre-existing, untouched by this PR, so not a finding here — but it's the kind of inconsistency I'd fold into a follow-up: per our error-handling rule, domain guards should carry anErrorCode.No blockers. The refactor closes a real bug (the dead-link-on-deleted-person case), the behaviour is proven on the right substrate, and the cleanup is complete. Nice work.
🛠️ Tobias Wendt — DevOps
Verdict: ✅ Approved — migration is operationally sound for this data scale. One pre-deploy guidance correction and a couple of suggestions below, none blocking.
What I checked
V1__initial_schema.sqlandV14. V71 drops exactly the right pre-existing constraints:documents.fkl5xhww7es3b4um01vmly4y18m(sender_id → persons, no ON DELETE in V1) ✓document_receivers.fkcg7r68qvosqricx1betgrlt7s(person_id → persons, no ON DELETE in V1) ✓document_idside (fks7t60twjgfmpeqcuc3g0fvjpm), so the "symmetric completion of V14" claim is accurate —person_idgenuinely had no ON DELETE behaviour until now.A wrong constraint name here would fail the
DROP CONSTRAINTand abort the migration in CI before prod — but it's correct, so no surprise on deploy.db/migration(V70 is the prior max). No gaps introduced, no edits to applied migrations (V56's change is comment-only — Flyway doesn't checksum comments differently, but be aware: editing an already-applied migration file changes its checksum). See blocker-adjacent note below.PersonRepositoryTestexercises the actual ON DELETE behaviour (correctly noted as never firing on H2). The orphan-purgeDOblock executes on every CI run from a clean DB (zero orphans there), so the happy path is covered; the non-empty purge path is not (clean CI DB has no orphans to delete) — acceptable, the DELETE is trivial SQL.Migration safety / locking
DROP CONSTRAINT ... ADD CONSTRAINTtakesACCESS EXCLUSIVEondocumentsanddocument_receivers, and the new FK ontranscription_block_mentioned_personsrequires a validating scan underACCESS EXCLUSIVE. At the stated scale (thousands of rows) this is sub-second. The migration's own header comment already documents this and warns against copying the drop-and-recreate pattern onto a large table — good operational hygiene, exactly the kind of comment I want to see.ACCESS EXCLUSIVEduring the full-table validation scan on large tables. At thousands of rows the scan is sub-second, so the two-step would add complexity for no benefit. Agree with the decision and the inline rationale.ADD CONSTRAINT, so the validation scan won't trip over dangling rows. If the purge missed any row the ADD would fail loudly and roll back the whole migration (Flyway runs the script in one transaction on Postgres) — fail-safe, not fail-open.Observability of the purge
RAISE NOTICE 'V71 orphaned_mention_rows_removed=%'is the right call.GET DIAGNOSTICS ... ROW_COUNTis the correct way to capture the count inside theDOblock, and the comment explaining why a trailingSELECTwouldn't surface anything (Flyway discards the trailing result set) is genuinely useful for the next person.NOTICElines. By default the JDBC driver buffersNOTICEs into connection warnings; whether they reach Flyway's log / your deploy console depends on log level. If you want a guaranteed audit trail of the purge count in prod, the safest belt-and-suspenders is to run the sizingSELECT count(*)from the PR notes manually right before deploy and record the number — don't rely solely on the NOTICE showing up in the deploy log.Pre-deploy orphan-count guidance — one correction
The PR body sizing query is:
This is correct and matches the DELETE predicate exactly — good. Two operational notes:
ACCESS EXCLUSIVEwindow. A count from last week's backup tells you nothing about today's orphans.Deploy ordering / downtime
Note worth flagging (not a blocker)
validateMigrationNaming/repair is handled — but since the change is only comments, confirm your Flyway config isn't set to fail on checksum drift, or runflyway repairpost-deploy. Comment-only edits to applied migrations are a common source of "works in CI (clean DB), fails on prod (V56 already applied)" surprises. Please double-check Flyway'svalidateOnMigratebehaviour against prod before merging — this is the one thing most likely to bite on the real deploy.Nice work overall — the migration is well-commented, the cascade boundary is pinned by a test, and the constraint names are correct.
🎨 Leonie — UI/UX Expert
✅ Approved
This is a backend-heavy PR (DB migration + service thinning) with a single user-facing surface: the graceful degradation of
@-mentions when a mentioned person is deleted. I reviewed that surface end to end and it is correct, honest, and well-protected by the contract test. Nothing of UX concern beyond the degradation contract — and that contract is solid.What I checked
1. The user-observable behaviour — does the reader still see the name? ✅
renderTranscriptionBody(frontend/src/lib/shared/discussion/mention.ts) escapes the full block text first, then only injects an<a class="person-mention" …>for each entry inmentionedPersons. When V71'sON DELETE CASCADEdrops the sidecar row, the renderer receives an empty array, the substitution loop never runs, and the escaped text — including the literal@Auguste Raddatz— falls through verbatim. The reader sees:Auguste Raddatz), in the surrounding sentence,<a>, no orphaned anchor, no console/render error.This is exactly the right degradation: a deleted link is invisible to the reader; the name survives because it lives in
transcription_blocks.text. The backend testdeleteById_mentionedPerson_dropsMentionRow_blockTextSurvives(PersonRepositoryTest) correctly pins that the block text is untouched, so the two halves of the contract (DB keeps the text, renderer shows it) are both locked.2. Does the contract test protect the right property? ✅
The new
mention.spec.tstest (renders a deleted-person @mention as plain text with no dead link) is the strongest version of this assertion I'd want:toContain('Auguste Raddatz')+toBe('Brief an @Auguste Raddatz vom Mai')— the reader-facing property ("reader still sees the name") is asserted directly, not inferred.<a,person-mention,data-person-id,hrefall absent — every anchor artifact that could silently reintroduce a dead link is named and excluded.It also strengthens coverage rather than weakening it: the replaced "renders nothing when mentionedPersons is empty" test had no
@-trigger in its input, so it never exercised the degradation path. Its plain-text property is still independently covered byreturns escaped plain text when no mentions. Net coverage is up.Suggestions (non-blocking)
@prefix asymmetry. In the degraded path the trigger@is preserved (@Auguste Raddatz), whereas a live mention strips it (anchor text is the bareAuguste Raddatz, per thestrips the @ prefixtest). So a reader on a page mixing live and deleted mentions seesAuguste Raddatznext to@Auguste Raddatz. This is cosmetic — the name is fully readable either way and the@reads naturally as historical-text shorthand — and the issue explicitly scoped out copy/affordance changes. Worth a one-line note in ADR-032 so a future reader knows the asymmetry is known and accepted, not an oversight. Not a blocker.No affordance is needed here, and adding one would be wrong. I considered whether the reader deserves a "this person was removed" cue. They don't: the mention was an editor convenience, the historical name is intact, and a tooltip/badge on every former-mention would add noise to the read path for the 60+ audience we optimise for. Silent degradation to plain text is the correct, calm choice. Agreeing with the issue's decision to leave the
/persons/reviewdialog copy unchanged for the same reason.Blockers: none.
Verdict: Approved — the degradation is correct and graceful, the contract test guards the reader-facing property explicitly, and there is no missing UX affordance on this surface.
🧪 Sara — QA Engineer
Verdict: ✅ Approved with one doc-accuracy nit
I came in cold and went straight for the question that decides whether this PR is real coverage or theatre: do these tests actually fire the database FK cascade, or are they passing against an in-memory fake that silently ignores
ON DELETE? The answer is yes — this is honest, real-Postgres coverage. Details below.What I verified
The cascade tests run against real Postgres, not H2. This was my top concern and it checks out:
PostgresContainerConfig→new PostgreSQLContainer<>("postgres:16-alpine")with@ServiceConnection. The canonical setup.PersonRepositoryTest:@DataJpaTest+@AutoConfigureTestDatabase(replace = NONE)+@Import({PostgresContainerConfig.class, FlywayConfig.class}).replace=NONEis the load-bearing annotation — without it@DataJpaTestwould swap in an embedded DB and the FKON DELETEclauses would never fire, so the assertions would pass against nothing. It's present. Flyway runs V1→V71, so the V71 constraints actually exist in the schema under test.PersonServiceIntegrationTest:@SpringBootTest(NONE)+@Import(PostgresContainerConfig)+@Transactional. Real Postgres, service path.Flush/clear discipline is correct where a DB cascade is asserted. Every cascade test does
flush()/clear()after the fixture build and afterdeleteById/mergePersons, then re-reads viafindById. The native cascade runs beneath the Hibernate session, so without the post-deleteclear()the L1 cache would serve a stale managedDocumentwith the oldreceivers/senderand the assertion would be a false green. This is the correct guard and it's applied consistently — no flakiness risk from stale L1 state.Edge cases are genuinely tested, not stubbed:
deleteById_personIsSenderAndReceiverOfSameDocument_...) — sender nulled, own receiver row dropped, co-receiver survives. This is the interaction the cross-document cases can't reach; good that it's separate.assertThat(documentRepository.findById(sent.getId())).isPresent()for both documents. This is the real Nora invariant: it would fail loudly if a future migration flippedsender_idfromSET NULLtoCASCADEand started eating historical letters. Non-trivial, non-negotiable, present.0AND the block text is byte-exact"Brief an @Auguste Raddatz". Both halves prove something: the sidecar link dies, the visible name stays. This is the soft-reference contract, properly pinned.Unit-test rewrites are meaningful.
verifyNoMoreInteractions(personRepository)on bothdeletePersonandmergePersonsis the right tool here — it's what actually proves the AC-5 cleanup. Without it, the removedreassignSenderToNull/deleteReceiverReferencescalls could silently creep back and theverify(...)allowlist wouldn't catch it.verifyNoMoreInteractionsturns "we deleted these calls" into an enforced invariant. Good rigor.Frontend
mention.spec.ts(AC-6). The rewritten degradation case is stronger than what it replaced: it asserts the verbatim sentence survives and negatively asserts no<a,person-mention,data-person-id, orhrefleaks through. That locks the no-dead-link contract against a future renderer refactor. Exactly the kind of negative assertion I want on a degradation path.Suggestion (non-blocking)
Inaccurate justification comment in
PersonServiceIntegrationTest.deletePerson_...noOrphan. The comment reads:Document.receiversis a@ManyToMany(fetch = FetchType.LAZY)join (document_receivers), not an@ElementCollection, and it'sLAZY, notEAGER. Theflush()/clear()is still correct and genuinely necessary (the native cascade runs below the session → L1 must be cleared so the re-read sees DB state), so this is purely a misleading rationale, not a test defect. Tighten the comment to "the L1 cache holds a stale managed Document after the below-session cascade" so the next maintainer isn't misled about the mapping. Pure comment fix — does not block merge.Coverage gaps I considered and am OK with
DOblock itself (the pre-existing-dangling-rows cleanup). Hard to exercise cleanly since Flyway runs once at context start and you can't easily seed orphans before the constraint exists. Acceptable to leave to the deploy-timeSELECT count(*)sizing step the PR body documents.Solid test pyramid: meaningful Mockito unit layer, real-Postgres integration for the behaviour that only exists at the DB layer, and a tightened frontend degradation spec. The one nit is cosmetic. Nice work.
— Sara
🏛️ Markus Keller — Architect
✅ Approved
This is the textbook version of a change I keep asking for: integrity enforced at the lowest trustworthy layer, the service thinned to a
deleteById, and the why captured in an ADR. I formed my view from the diff and verified the load-bearing facts against the schema history. Nothing in my domain blocks it.What I checked (and it holds up)
ON DELETEbehaviour. This is the right direction —PersonService.deletePerson/mergePersonswere the only safe delete path; any future endpoint, batch job, or manualpsqlwould have orphaned rows or 500'd. The DB is now the single source of truth. Exactly the principle in my review lens (constraints over application checks).V1__initial_schema.sql:documents.sender_id→fkl5xhww7es3b4um01vmly4y18manddocument_receivers.person_id→fkcg7r68qvosqricx1betgrlt7sboth exist as named FKs. V71's drop-and-recreate by name is safe for any DB that ran V1.document_idside ofdocument_receivers(fks7t60twjgfmpeqcuc3g0fvjpm, → CASCADE). Theperson_idside (fkcg7r68...) was left bare. V71 closing it is genuinely the missing half, not a re-do.sender_id→SET NULL(document +senderTextsurvive), joins/sidecar →CASCADE, and the cascade never reachesdocuments. The non-negotiable "both documents survive" assertion inPersonRepositoryTestguards against a future migration silently upgradingSET NULLtoCASCADEand destroying historical letters. Good defensive instinct — that's the kind of invariant that deserves a test, and it has one.ACCESS EXCLUSIVElock; the inline comment correctly scopes that to small tables and warns against copying the pattern onto large ones. Orphantranscription_block_mentioned_personsrows are purged beforeADD CONSTRAINTvalidation — without that, the validation scan would fail on existing dangling rows. TheDO/RAISE NOTICEpurge-count log (vs. a trailingSELECTFlyway would discard) shows real understanding of the JDBC execution path.ON DELETEactually fires. V1→V71 runs in CI. Schema drift is caught before production.documents, honour V56's no-FK) are each argued, not hand-waved. Number 032 is unique and correctly sequenced — I verifieddocs/adr/. The "audit cascade runs below AuditService — accepted trade" note is exactly the kind of consequence an ADR exists to record.transcription_block_mentioned_persons.person_id→ bothdb-orm.pumlanddb-relationships.pumlupdated (person_id marked<<FK>>, all three relationship lines annotated with theirON DELETEmode). No new package/service/route, so the C4 diagrams are correctly untouched. The stale V56 "no FK" comment was rewritten to point at V71 + ADR-032. Per my doc-currency table, this PR's obligations are all met.EntityManagerinjected into the repository; cross-domain access untouched; two now-dead repository methods removed rather than left to rot. TheclearAutomatically/flushAutomaticallyon the surviving bulk updates is correctly justified in the comment (L1 cache desync below Hibernate).Blockers
None.
Suggestions (non-blocking, do not hold the merge)
docs/adr/). Not introduced here and not this PR's job to fix — flagging only so it lands on someone's radar before the next ADR collides.Clean change. Merge it.
— @mkeller
🔒 Nora — Security Expert
Verdict: ✅ Approved
This is a clean-room security pass. I treated the migration as hostile DDL and the renderer change as a potential stored-XSS regression. Both hold up. This is a defensive change — it moves a fragile application-layer invariant into the database, which is exactly where referential-integrity guarantees belong (fail-closed by construction, enforced on every delete path, not just
PersonService).What I checked (and why it's safe)
1. Data-loss / data-retention — the cascade never destroys a historical letter ✅
documents.sender_id→ON DELETE SET NULL, not CASCADE. The document row survives;documents.senderTextpreserves the raw textual attribution, so nulling the FK loses no historical record. This is the correct non-destructive choice and the ADR-032 "Alternatives considered" explicitly rejects cascadingdocuments.sender_id. Good — that rejected alternative is the one that would have deleted family letters.document_receivers.person_id,transcription_block_mentioned_persons.person_id) reach only join/sidecar rows, never adocumentsrow.2. Cascade-boundary invariant is test-pinned ✅ (this was my non-negotiable)
PersonRepositoryTest.deleteById_personSenderOfAReceiverOfB_...assertsdocumentRepository.findById(sent)andfindById(received)are bothpresentafter the delete, with an explicit comment naming the threat (a future migration flipping SET NULL → CASCADE). That is the regression lock I wanted — a renderer/schema refactor cannot silently turn this into a letter-shredder without a red test. AC-8 (...senderAndReceiverOfSameDocument...) covers the trickier same-row interaction. Tests run against real Postgres via Testcontainers, which is mandatory here — H2 would never fire the FKON DELETEbehaviour, so a unit-only test would be security theater.3. Constraint names verified against source — migration is correct and complete ✅
I cross-checked the
DROP CONSTRAINTnames in V71 againstV1__initial_schema.sql:documents.sender_id=fkl5xhww7es3b4um01vmly4y18m✓document_receivers.person_id=fkcg7r68qvosqricx1betgrlt7s✓document_idsides of the join tables, so theperson_idFK ondocument_receiverswas still the V1-default (noON DELETE) — V71 dropping/recreating it is correct, not a double-apply.4. Orphan-purge DELETE is bounded — no over-delete ✅
Correlated
NOT EXISTSonperson_id— deletes only rows already orphaned (a person that no longer exists). It cannot touch a row whoseperson_idstill resolves, and it physically cannot reachdocumentsorpersons. The(block_id, person_id)PK (V65) means these are whole tuples; no duplicate/partial-row hazard. The purge is gated behind theADD CONSTRAINTvalidation scan, so it is also necessary (the scan would 500 the migration on pre-existing dangling rows otherwise). Count is surfaced viaRAISE NOTICE— good operability, no sensitive data in the log line.5. @-mention XSS / escaping degradation contract ✅
Read
mention.tsin full.renderTranscriptionBodyescapes the entire block text first (escapeHtml, all five special chars incl. apostrophe), then only emits an<a>for entries present inmentionedPersons, and refuses to render an anchor for a non-UUIDpersonId(CWE-601 open-redirect defense — good forward-thinking guard). With the sidecar row gone (post-cascade),mentionedPersonsis empty → the@DisplayNamefalls through as plain escaped text. The newmention.spec.tstest locks this precisely: asserts the name survives verbatim and that none of<a,person-mention,data-person-id,hrefleak through. That is the right contract — degradation must not silently reintroduce a dead/injectable link. TheSafeHtmlbrand keeps{@html}consumers honest.6. Permission posture unchanged ✅
PersonControllerstill carries@RequirePermission(Permission.WRITE_ALL)onDELETE /{id}andPOST /{id}/merge. ThinningdeletePerson/mergePersonsdid not touch the authorization boundary. The DELETE endpoint is@DeleteMapping(explicit verb), not a wildcard@RequestMapping.Auditability (Suggestion, not a blocker)
DB
ON DELETEcascades run belowAuditService, so the row-level cleanup (which receiver/mention links were dropped) is not audit-logged. This is acceptable: the person-delete action is still logged at the service layer, ADR-032 documents the trade explicitly, and the dropped rows are pure derived link-state (the human-readable@Namesurvives intranscription_blocks.text,senderTextsurvives on the document). For an 8-person family archive this is the right cost/benefit. I'd only revisit if a future compliance requirement demands per-link delete provenance — note it in the ADR's Consequences if that ever lands.Blockers
None.
Suggestions
DOblock — it's implied byNOT EXISTSbut an explicit "innocent row survives" test would pin the over-delete boundary the same way the document-survival test pins the cascade boundary. Not blocking; the constraint logic is unambiguous.Nice work — security-positive refactor, fail-closed, and the one assertion I'd have insisted on is already in the suite.