Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 3m16s
CI / OCR Service Tests (pull_request) Successful in 1m34s
CI / Backend Unit Tests (pull_request) Failing after 4m14s
CI / Unit & Component Tests (push) Failing after 3m29s
CI / OCR Service Tests (push) Successful in 50s
CI / Backend Unit Tests (push) Failing after 3m43s
Markus #4 (PR #366 review). PersonDisplayNameChangedEvent is the first custom application event in this codebase — the prior @EventListener (OcrTrainingService.recoverOrphanedRuns) consumed Spring's built-in ApplicationReadyEvent. The pattern is load-bearing for future cross-domain decoupling and warrants a documented decision rather than a comment buried in the listener. Captures: synchronous-by-default rationale, package layout (event in publisher's model/, listener in consumer's service/), saveAllAndFlush vs saveAll for exception surfacing, the migration path to @TransactionalEvent Listener + @Async if archive growth forces it, and the rejected alternatives (direct call, DB trigger, Hibernate entity listener). Refs #362 #366 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
56 lines
6.6 KiB
Markdown
56 lines
6.6 KiB
Markdown
# ADR-006: Synchronous domain events inside the publisher's transaction
|
|
|
|
## Status
|
|
|
|
Accepted
|
|
|
|
## Context
|
|
|
|
Issue #362 introduced the first cross-domain side-effect in this codebase: when a Person's display name changes, every transcription block that mentions the person must be rewritten — both `block.text` (the literal `@OldName` substring) and the `mentionedPersons` sidecar (the `displayName` field on the matching `PersonMention`). The rewrite is bidirectionally referential — Person depends on Transcription to make the rename atomic, and Transcription depends on Person to know what the new display name is.
|
|
|
|
A direct method call from `PersonService` into `TranscriptionBlockService` would invert the existing dependency arrow (Document → Person, not Person → Transcription) and introduce a runtime-circular reference at the package level. Avoiding the cycle while keeping the rename atomic is the constraint this ADR addresses.
|
|
|
|
Two prior pieces of infrastructure constrain the solution:
|
|
|
|
- `transcription_blocks.version` (JPA `@Version`) — concurrent autosave on a referenced block must roll back the rename instead of silently overwriting the autosave.
|
|
- `OcrTrainingService.recoverOrphanedRuns` is the only existing `@EventListener` and it consumes Spring's built-in `ApplicationReadyEvent` — no precedent for a custom domain event in this codebase before now.
|
|
|
|
## Decision
|
|
|
|
`PersonService.updatePerson` publishes `PersonDisplayNameChangedEvent(personId, oldDisplayName, newDisplayName)` via `ApplicationEventPublisher` whenever `Person.getDisplayName()` flips between the pre-save snapshot and the post-save value. `PersonMentionPropagationListener` (in the transcription package's `service/` layer) handles the event with `@EventListener @Transactional`, finds blocks via `findByMentionedPersons_PersonId`, rewrites text + sidecar, and calls `saveAllAndFlush`.
|
|
|
|
**Synchronous on purpose.** Spring's default event dispatcher invokes listeners on the publishing thread, inside the publisher's transaction. The propagation runs as part of the same `@Transactional` boundary as the rename — `OptimisticLockingFailureException` from a referenced block bubbles back up, the surrounding transaction rolls back, and `PersonService.updatePerson` translates it to `DomainException(PERSON_RENAME_CONFLICT, 409)`.
|
|
|
|
**Pattern for future cross-domain decoupling:**
|
|
1. Event record in `model/` of the publishing domain (e.g. `PersonDisplayNameChangedEvent`).
|
|
2. Listener in `service/` of the consuming domain (e.g. `PersonMentionPropagationListener`).
|
|
3. `@EventListener @Transactional` on the listener method — no `@TransactionalEventListener` unless the work genuinely doesn't need to commit with the publisher.
|
|
4. `saveAllAndFlush` (not `saveAll`) on any write where exceptions must surface inside the listener call so the publisher can catch and translate them — `saveAll` defers exceptions to commit time, after the publisher's `try` block has exited.
|
|
5. Audit log line at `INFO` level on the listener method — historical-text mutation needs an audit trail.
|
|
|
|
## Alternatives Considered
|
|
|
|
| Alternative | Why rejected |
|
|
|---|---|
|
|
| `PersonService` calls `TranscriptionBlockService.propagateDisplayNameChange(...)` directly | Inverts the dependency arrow. Person becomes runtime-coupled to Transcription; future domains that also care about renames (Comments, Notifications) compound the coupling. Events keep Person agnostic of who consumes them. |
|
|
| `@TransactionalEventListener(AFTER_COMMIT) + @Async` | The propagation would run after the rename commits, on a separate transaction. A failed propagation could leave block text out of sync with the renamed person until manual repair. Atomic transactional coupling is the safer default for historical-text mutation; switch to async only when the block count makes sync latency unacceptable (rough threshold: tens of thousands of blocks per renamed person). |
|
|
| Database trigger on `persons.last_name` | PL/pgSQL trigger would have to reach into `transcription_block_mentioned_persons` and `transcription_blocks.text`, smearing domain logic across SQL and Java. JPA's `@Version` would also be invisible to the trigger, so concurrent block autosaves would race silently. |
|
|
| Hibernate entity listener (`@PostUpdate` on Person) | Couples to Hibernate internals; harder to test in isolation; mixes lifecycle hooks with cross-domain side effects. Spring's `ApplicationEventPublisher` keeps the integration declarative and unit-testable. |
|
|
|
|
## Consequences
|
|
|
|
**Easier:**
|
|
- Person domain stays free of any compile-time dependency on Transcription. Future consumers (Comments, Notifications) subscribe to the same event without `PersonService` knowing they exist.
|
|
- Rename + propagation share one transaction → no half-applied state visible to readers, no orphaned rewrites if the rename fails after propagation, no "eventually-consistent" window for an archive that prizes historical fidelity.
|
|
- Concurrent autosaves on referenced blocks raise a structured 409 the frontend can render meaningfully (`error_person_rename_conflict`) instead of a generic 500.
|
|
- The pattern itself (record event in `model/`, listener in consumer's `service/`, sync `@EventListener @Transactional`, `saveAllAndFlush`) is reusable for the next cross-domain side effect.
|
|
|
|
**Harder:**
|
|
- Listener latency adds to the rename request's response time. The 200-block latency floor (< 2 s) is a merge-blocking regression test; if archive growth pushes it up, the migration path is one-annotation: switch to `@TransactionalEventListener(AFTER_COMMIT) + @Async` and add a manual-repair tool for propagation failures.
|
|
- Tests for the listener path require routing the publisher mock through a real listener (see `PersonServiceTest#updatePerson_throwsConflict_whenBlockSaveAllAndFlushHitsOptimisticLock`). Slightly more setup than a pure-Mockito test, but exercises the production call chain.
|
|
- `saveAllAndFlush` is mandatory in any synchronous listener that must surface JPA exceptions to the publisher's `try`-block. `saveAll` alone defers the flush to transaction commit, which happens after the publisher returns.
|
|
|
|
## Future Direction
|
|
|
|
If a single rename starts touching tens of thousands of blocks, switch the listener to `@TransactionalEventListener(phase = AFTER_COMMIT)` paired with `@Async` and add (a) an idempotency key to the event so a retry doesn't double-rewrite, (b) an admin tool that scans for sidecar entries whose `displayName` doesn't match the current `Person.getDisplayName()` and repairs them. At that point the orphan-guard path (existsById check before the rewrite) re-enters the listener as a deliberate piece of the async machinery rather than dead code.
|