fix(timeline): resolve a multi-event letter link deterministically
A letter whose document is in more than one curated event's `documents` set was linked by `putIfAbsent` over `eventRepository.findAll()`, whose iteration order JPA does not guarantee — so the same letter could cluster under a different event across re-seeds/VACUUM with no data change. resolveLetterEventLinks now runs over a stably-ordered copy (earliest event date, undated last, then lowest id), so the link is a deterministic property of the data. Fixes review finding #9 (Architect-3). Refs #850 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -202,7 +202,7 @@
|
|||||||
| REQ-006 | letter linked to no curated event → loose chronological letter (alternating; density strip past 12) | #850 | inline-event-clustering | `frontend/src/lib/timeline/YearBand.svelte`, `frontend/src/lib/timeline/eventClustering.ts` | `eventClustering.spec.ts#keeps a letter with no linkedEventId loose`; `YearBand.svelte.spec.ts#renders loose (unlinked) letters in a sparse year as alternating .letter-row, no card` | Done |
|
| REQ-006 | letter linked to no curated event → loose chronological letter (alternating; density strip past 12) | #850 | inline-event-clustering | `frontend/src/lib/timeline/YearBand.svelte`, `frontend/src/lib/timeline/eventClustering.ts` | `eventClustering.spec.ts#keeps a letter with no linkedEventId loose`; `YearBand.svelte.spec.ts#renders loose (unlinked) letters in a sparse year as alternating .letter-row, no card` | Done |
|
||||||
| REQ-007 | loose-letter layout + density strip count ONLY non-event-linked letters; clustered letter never also loose | #850 | inline-event-clustering | `frontend/src/lib/timeline/YearBand.svelte`, `frontend/src/lib/timeline/eventClustering.ts` | `eventClustering.spec.ts#places each letter in exactly one place`; `YearBand.svelte.spec.ts#counts only loose letters in the density strip; event letters stay in the card` | Done |
|
| REQ-007 | loose-letter layout + density strip count ONLY non-event-linked letters; clustered letter never also loose | #850 | inline-event-clustering | `frontend/src/lib/timeline/YearBand.svelte`, `frontend/src/lib/timeline/eventClustering.ts` | `eventClustering.spec.ts#places each letter in exactly one place`; `YearBand.svelte.spec.ts#counts only loose letters in the density strip; event letters stay in the card` | Done |
|
||||||
| REQ-008 | letter whose only linking event is filtered out (absent from lookup) → loose, never re-introduces the event (filter-then-cluster) | #850 | inline-event-clustering | `frontend/src/lib/timeline/eventClustering.ts` (`splitYearLetters` filters via `eventLookup`) | `eventClustering.spec.ts#keeps a letter whose linkedEventId is absent from the lookup loose (filter-then-cluster, REQ-008)` | Done |
|
| REQ-008 | letter whose only linking event is filtered out (absent from lookup) → loose, never re-introduces the event (filter-then-cluster) | #850 | inline-event-clustering | `frontend/src/lib/timeline/eventClustering.ts` (`splitYearLetters` filters via `eventLookup`) | `eventClustering.spec.ts#keeps a letter whose linkedEventId is absent from the lookup loose (filter-then-cluster, REQ-008)` | Done |
|
||||||
| REQ-009 | `TimelineEntryDTO` carries nullable `linkedEventId` for LETTER entries, one batched pass, no new column/migration, not @Schema(REQUIRED) | #850 | inline-event-clustering | `backend/.../timeline/TimelineEntryDTO.java`, `backend/.../timeline/TimelineService.java#resolveLetterEventLinks`, `frontend/src/lib/generated/api.ts` | `TimelineServiceTest#letter_in_a_curated_events_documents_carries_that_events_id`, `#letter_in_no_curated_event_has_null_linkedEventId` | Done |
|
| REQ-009 | `TimelineEntryDTO` carries nullable `linkedEventId` for LETTER entries, one batched pass, no new column/migration, not @Schema(REQUIRED); a multi-event letter links deterministically (earliest date, then id) | #850 | inline-event-clustering | `backend/.../timeline/TimelineEntryDTO.java`, `backend/.../timeline/TimelineService.java#resolveLetterEventLinks`, `frontend/src/lib/generated/api.ts` | `TimelineServiceTest#letter_in_a_curated_events_documents_carries_that_events_id`, `#letter_in_no_curated_event_has_null_linkedEventId`, `#multi_event_letter_links_deterministically_to_the_earliest_event` | Done |
|
||||||
| REQ-010 | event/letter/sender/receiver text via Svelte `{...}` escaping; no `{@html}` anywhere in `lib/timeline/` (grep gate, CWE-79) | #850 | inline-event-clustering | `frontend/src/lib/timeline/*.svelte` | `timeline-no-raw-html.spec.ts#no timeline component contains the raw-HTML directive`; `EventCluster.svelte.spec.ts#renders an HTML-bearing event title verbatim as text, never as markup` | Done |
|
| REQ-010 | event/letter/sender/receiver text via Svelte `{...}` escaping; no `{@html}` anywhere in `lib/timeline/` (grep gate, CWE-79) | #850 | inline-event-clustering | `frontend/src/lib/timeline/*.svelte` | `timeline-no-raw-html.spec.ts#no timeline component contains the raw-HTML directive`; `EventCluster.svelte.spec.ts#renders an HTML-bearing event title verbatim as text, never as markup` | Done |
|
||||||
| REQ-011 | wrapping header keeps #842 add-event CTA + #780 filter trigger; meta-line drops the grouping segment | #850 | inline-event-clustering | `frontend/src/routes/zeitstrahl/+page.svelte` | `page.svelte.spec.ts#renders the meta sub-line with range and counts, no grouping segment`, `#drops the letters segment instead of showing "0 Briefe"` (no "Gruppierung") | Done |
|
| REQ-011 | wrapping header keeps #842 add-event CTA + #780 filter trigger; meta-line drops the grouping segment | #850 | inline-event-clustering | `frontend/src/routes/zeitstrahl/+page.svelte` | `page.svelte.spec.ts#renders the meta sub-line with range and counts, no grouping segment`, `#drops the letters segment instead of showing "0 Briefe"` (no "Gruppierung") | Done |
|
||||||
| REQ-012 | show-more/less labels are new Paraglide keys in de/en/es; unused #827 grouping/Thema keys removed | #850 | inline-event-clustering | `frontend/messages/{de,en,es}.json`, `frontend/src/lib/timeline/EventCluster.svelte` | `messages.spec.ts#zeitstrahl visual-fidelity keys are present in all locales` (now asserts `timeline_bucket_show_more`/`_less`; `timeline_grouping_date` removed) | Done |
|
| REQ-012 | show-more/less labels are new Paraglide keys in de/en/es; unused #827 grouping/Thema keys removed | #850 | inline-event-clustering | `frontend/messages/{de,en,es}.json`, `frontend/src/lib/timeline/EventCluster.svelte` | `messages.spec.ts#zeitstrahl visual-fidelity keys are present in all locales` (now asserts `timeline_bucket_show_more`/`_less`; `timeline_grouping_date` removed) | Done |
|
||||||
|
|||||||
@@ -269,17 +269,28 @@ public class TimelineService {
|
|||||||
* event whose {@code documents} set contains the letter (REQ-009). A single doc→event map is
|
* event whose {@code documents} set contains the letter (REQ-009). A single doc→event map is
|
||||||
* built over the already-loaded events — no per-letter query ({@code TimelineEvent.documents}
|
* built over the already-loaded events — no per-letter query ({@code TimelineEvent.documents}
|
||||||
* carries {@code @BatchSize(50)}). When a document is referenced by more than one curated
|
* carries {@code @BatchSize(50)}). When a document is referenced by more than one curated
|
||||||
* event, the first by repository iteration order wins ({@code putIfAbsent}). The map is built
|
* event, the earliest-dated event wins (then lowest {@code eventId}); the pass runs over a
|
||||||
* from <em>all</em> events (not just the year/type-filtered ones) so the link is a stable
|
* stably-ordered copy so the link is a deterministic property of the data, not a coin-flip on
|
||||||
* property of the data; the frontend's filter-then-cluster decides whether the linked event is
|
* the undefined repository iteration order ({@code putIfAbsent} keeps the first = winner). The
|
||||||
* actually on screen (#850). Logs nothing — the assembly path keeps UUIDs out of logs (§2.7).
|
* map is built from <em>all</em> events (not just the year/type-filtered ones) so the link is a
|
||||||
|
* stable property of the data; the frontend's filter-then-cluster decides whether the linked
|
||||||
|
* event is actually on screen (#850). Logs nothing — the assembly path keeps UUIDs out of logs (§2.7).
|
||||||
*/
|
*/
|
||||||
private Map<UUID, UUID> resolveLetterEventLinks(List<Document> letters, List<TimelineEvent> events) {
|
private Map<UUID, UUID> resolveLetterEventLinks(List<Document> letters, List<TimelineEvent> events) {
|
||||||
Set<UUID> letterDocIds = letters.stream().map(Document::getId).collect(Collectors.toSet());
|
Set<UUID> letterDocIds = letters.stream().map(Document::getId).collect(Collectors.toSet());
|
||||||
if (letterDocIds.isEmpty()) return Map.of();
|
if (letterDocIds.isEmpty()) return Map.of();
|
||||||
|
|
||||||
|
// Stable order so a multi-event letter links deterministically: earliest event date
|
||||||
|
// (undated last), then lowest id. putIfAbsent below then keeps the winner (REQ-009).
|
||||||
|
List<TimelineEvent> ordered = events.stream()
|
||||||
|
.sorted(Comparator
|
||||||
|
.comparing(TimelineEvent::getEventDate,
|
||||||
|
Comparator.nullsLast(Comparator.naturalOrder()))
|
||||||
|
.thenComparing(TimelineEvent::getId))
|
||||||
|
.toList();
|
||||||
|
|
||||||
Map<UUID, UUID> eventByDocId = new HashMap<>();
|
Map<UUID, UUID> eventByDocId = new HashMap<>();
|
||||||
for (TimelineEvent ev : events) {
|
for (TimelineEvent ev : ordered) {
|
||||||
Set<Document> linkedDocs = ev.getDocuments();
|
Set<Document> linkedDocs = ev.getDocuments();
|
||||||
if (linkedDocs == null) continue;
|
if (linkedDocs == null) continue;
|
||||||
for (Document linked : linkedDocs) {
|
for (Document linked : linkedDocs) {
|
||||||
|
|||||||
@@ -549,6 +549,46 @@ class TimelineServiceTest {
|
|||||||
assertThat(entry.linkedEventId()).isNull();
|
assertThat(entry.linkedEventId()).isNull();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void multi_event_letter_links_deterministically_to_the_earliest_event() {
|
||||||
|
// REQ-009: a document referenced by >1 curated event links to the earliest-dated event
|
||||||
|
// (then lowest id), independent of repository iteration order — not a coin-flip on
|
||||||
|
// findAll()'s undefined order.
|
||||||
|
Document shared = docWithDate(LocalDate.of(1916, 5, 1), DatePrecision.MONTH);
|
||||||
|
TimelineEvent earlier = TimelineEvent.builder()
|
||||||
|
.id(UUID.fromString("00000000-0000-0000-0000-000000000001"))
|
||||||
|
.title("Frühes Ereignis").type(EventType.PERSONAL)
|
||||||
|
.eventDate(LocalDate.of(1913, 3, 1)).precision(DatePrecision.MONTH)
|
||||||
|
.documents(new HashSet<>(Set.of(shared)))
|
||||||
|
.build();
|
||||||
|
TimelineEvent later = TimelineEvent.builder()
|
||||||
|
.id(UUID.fromString("00000000-0000-0000-0000-000000000002"))
|
||||||
|
.title("Spätes Ereignis").type(EventType.PERSONAL)
|
||||||
|
.eventDate(LocalDate.of(1914, 3, 1)).precision(DatePrecision.MONTH)
|
||||||
|
.documents(new HashSet<>(Set.of(shared)))
|
||||||
|
.build();
|
||||||
|
when(timelineEventService.assembleDerivedEvents()).thenReturn(List.of());
|
||||||
|
when(documentService.getAllForTimeline()).thenReturn(List.of(shared));
|
||||||
|
|
||||||
|
// `later` first in iteration: a naive putIfAbsent would wrongly pick it.
|
||||||
|
when(eventRepository.findAll()).thenReturn(List.of(later, earlier));
|
||||||
|
assertThat(theLetter(timelineService.assemble(noFilters())).linkedEventId())
|
||||||
|
.isEqualTo(earlier.getId());
|
||||||
|
|
||||||
|
// Reversed order yields the same winner — the link is order-independent.
|
||||||
|
when(eventRepository.findAll()).thenReturn(List.of(earlier, later));
|
||||||
|
assertThat(theLetter(timelineService.assemble(noFilters())).linkedEventId())
|
||||||
|
.isEqualTo(earlier.getId());
|
||||||
|
}
|
||||||
|
|
||||||
|
private static TimelineEntryDTO theLetter(TimelineDTO result) {
|
||||||
|
return java.util.stream.Stream.concat(
|
||||||
|
result.years().stream().flatMap(y -> y.entries().stream()),
|
||||||
|
result.undated().stream())
|
||||||
|
.filter(e -> e.kind() == Kind.LETTER)
|
||||||
|
.findFirst().orElseThrow();
|
||||||
|
}
|
||||||
|
|
||||||
private static TimelineEntryDTO onlyLetterEntry(TimelineDTO result) {
|
private static TimelineEntryDTO onlyLetterEntry(TimelineDTO result) {
|
||||||
assertThat(result.years()).hasSize(1);
|
assertThat(result.years()).hasSize(1);
|
||||||
return result.years().get(0).entries().get(0);
|
return result.years().get(0).entries().get(0);
|
||||||
|
|||||||
Reference in New Issue
Block a user