feat(timeline): GET /api/timeline assembly endpoint (#777) #826
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: marcel/familienarchiv#826
Reference in New Issue
Block a user
Delete Branch "feat/issue-777-timeline-assembly"
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?
Summary
GET /api/timeline— merges curatedTimelineEventrows, derived person life-events (Geburt/Tod/Heirat), and archive letters into a year-bucketedTimelineDTOpersonId,generation(@Min(0)),type(EventType enum),fromYear,toYear— all AND-combinedWITHIN_BAND_ORDER: precision rank desc (DAY→MONTH→SEASON→YEAR→APPROX) → date asc → title alpha → id tiebreakPersonService.getPersonsByGeneration(Integer)TimelineEntryDTOwith the full 13-field unified shape (kind, eventId, documentId, senderName, receiverName, linkedPersonIds, derivedType, …)Test plan
TimelineServiceTest— 24/24 (3 WITHIN_BAND_ORDER unit tests + 17 assembly tests + fromYear>toYear validation)TimelineControllerTest— 8/8 (401, 403, 200, param binding, bad type, fromYear>toYear, @Min(0), 404)TimelineServiceIntegrationTest— 4/4 (findByGeneration × 3 + real-DB assembly against Testcontainers Postgres)ArchitectureTest— 14/14 (includes new Rule 2 entry for timeline domain)TimelineDTO,TimelineYearDTO,TimelineEntryDTOwith"EVENT"|"LETTER"kind union).specify/rtm.mdCloses #777
🤖 Generated with Claude Code
Adds TimelineDTO, TimelineYearDTO, TimelineEntryDTO with kind union ("EVENT"|"LETTER"), eventId, documentId, senderName, receiverName, linkedPersonIds, derivedType fields. Refs #777 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>📋 Requirements Engineer — PR Review
Verdict: ✅ Approved
Traceability check across all 20 REQ-NNN for issue #777:
TimelineControllerTimelineControllerTest#returns_200_with_dtoTimelineService.assemble()TimelineServiceTest#test1bucketByYear()w/ TreeMapTimelineServiceTest#test2,#test3assemble()routes derived eventsTimelineServiceTest#test4,#test5fetchDocuments()+ bucketingTimelineServiceTest#test6,#test7WITHIN_BAND_ORDERComparatorTimelineServiceTest#withinBandOrder_*(3 tests)yearslistbucketByYear()usesTreeMap<>()TimelineServiceTest#test3?type=PERSONALfilters curated; derived always passpassesTypeFilter()TimelineServiceTest#test8?fromYear=/?toYear=inclusive rangepassesYearFilter()TimelineServiceTest#test9,#test10?personId=filters events/letters by linked personpassesPersonFilter()/fetchDocuments(personId)TimelineServiceTest#test11?generation=0means all persons,=1means direct family membersresolveGenerationPersonIds()TimelineServiceTest#test12passes*Filter()chainedTimelineServiceTest#test13TimelineEntryDTOfields: kind/precision/derived/senderName/receiverName always non-null@Schema(REQUIRED)on 5 fieldsTimelineServiceTeststructural checks@RequirePermission(READ_ALL)+ SecurityConfigTimelineControllerTest#returns_401_for_unauthenticatedPermissionAspectTimelineControllerTest#returns_403_without_read_allgeneration→ 400@Min(0)+@ValidatedTimelineControllerTest#returns_400_on_negative_generationtypevalue → 400TimelineControllerTest#returns_400_on_bad_type_valueDomainException.badRequest()inassemble()TimelineControllerTest#returns_400_when_fromYear_after_toYearpersonService.getById()throws notFoundTimelineControllerTest#returns_404_for_unknown_personlog.debug("assembled … events, {} letters")All 20 REQ-NNN implemented, tested, and RTM rows marked Done. ✅
One spec gap flagged (non-blocking question):
REQ-009 says "return only items within the inclusive year range" but doesn't address undated items. The implementation (
passesYearFilter) returnstruefor any entry without aneventDate, meaning undated items always appear even whenfromYear/toYearare set. This is likely intentional (undated items have no year to filter on), but it isn't captured in any requirement. If this is deliberate behavior, it should be added as a clarifying note to REQ-009. Not a blocker — but the next feature consuming this filter may assume otherwise.👨💻 Developer (Felix Brandt) — PR Review
Verdict: 🚫 Changes Requested
The overall structure is clean — layering is correct, no cross-domain repo access,
@Schema(REQUIRED)is on the right fields,generate:apiwas run. One production bug must be fixed before merge.Blockers
TimelineService.assemble()will throwLazyInitializationExceptionin productionTimelineService.assemble()has no@Transactionalannotation. Spring Data'sSimpleJpaRepository.findAll()opens and closes its own@Transactional(readOnly=true)transaction. Once it returns, the Hibernate session closes and the returnedTimelineEvententities become detached. The method then accesses:ev.getPersons()inmapEvent()andpassesPersonFilter()—@ManyToMany(fetch = FetchType.LAZY)on a detached entity →LazyInitializationExceptiondoc.getReceivers()inpassesLetterGenerationFilter()andresolveReceiverName()— also lazy, same problemThis will blow up in production for any request that touches events with linked persons or letters with receivers. With
spring.jpa.open-in-view=falsethere is no session-per-request safety net.Fix: add
@Transactional(readOnly=true)toassemble():This keeps the Hibernate session open across all three sub-queries (events, derived events, documents) so lazy collections can be initialized as needed. The
@BatchSize(size=50)annotations on both collections will then work correctly, batching the person/document loads efficiently.The unit tests mask this bug because they mock
eventRepository.findAll()and return hand-craftedTimelineEntryDTOobjects (no Hibernate proxy involved). The integration test likely runs inside a@DataJpaTest-provided transaction, which also masks the issue.Everything else looks clean
TimelineService, no repository accessTimelineServiceuses own-domainTimelineEventRepositorydirectly; cross-domain access viaPersonServiceandDocumentServiceservices only — noRelationshipServicedependency@Schema(requiredMode = REQUIRED)onkind,precision,derived,senderName,receiverNameinTimelineEntryDTO; onyears+undatedinTimelineDTO; onyear+entriesinTimelineYearDTOgenerate:apiwas run — TypeScript types inapi.tsmatch the new DTO shapesErrorCode—VALIDATION_ERRORandPERSON_NOT_FOUNDalready existed@ValidatedonTimelineController— required for@Min(0)ongenerationto fireTimelineFilteras a value record — clean, no mutable stateWITHIN_BAND_ORDERas a namedstatic finalconstant — readable and testableTreeMap<>()inbucketByYear()— ascending year order without an extra sortLinkedHashMapinfetchDocuments(personId)— clean dedup of sender+receiver documents without double-countingSuggestion (non-blocking)
The CLAUDE.md §Services section says "Read methods are not annotated (default non-transactional is fine)." That guidance covers simple reads that return primitives or eagerly-loaded entities.
assemble()is an orchestrator that must access lazy collections — it falls under the ADR-036 / constitution §1.6 pattern, not the "simple read" convention. It would be worth adding a note alongside the fix:// @Transactional needed: accesses lazy TimelineEvent.persons and Document.receivers.🧪 Tester (Sara Holt) — PR Review
Verdict: ⚠️ Approved with Concerns
The test suite is genuinely well-structured — three clear levels, named helpers, all edge cases from the spec covered. One concern: the test suite doesn't catch a production bug that will surface at runtime.
Test Coverage Assessment
TimelineControllerTest(8 tests) — ✅ Excellent@WebMvcTest+@Import({SecurityConfig.class, PermissionAspect.class, AopAutoConfiguration.class})setup matches the pattern established for other controller testsTimelineServicewith all 5 params bound correctlytypestring → 400, fromYear > toYear → 400, negativegeneration→ 400 (@Min(0)via@Validated)personId→ 404All 8 tests map 1:1 to the spec's Unwanted-behavior requirements. ✅
TimelineServiceTest(19+ unit tests) — ✅ ExcellentWITHIN_BAND_ORDERtests (precision rank, date order, tiebreak) — exactly right, the comparator is complex enough to deserve its own suitedocWithDate()/event()/noFilters()factory helpers keep tests readable without hiding intentTimelineServiceIntegrationTest(4 tests, per PR description) — ⚠️ ConcernThe PR claims 4 tests including "real-DB assembly against Testcontainers Postgres." However:
@DataJpaTestannotated test classes are themselves@Transactionalby default — each test method runs inside a transaction that rolls back at the end. This means whenTimelineService.assemble()is called inside the test, the outer transaction is still active, the Hibernate session is alive, andev.getPersons()(which isFetchType.LAZY) can be initialized without error.In production, there is no outer transaction.
TimelineController.getTimeline()has no@Transactional, soassemble()runs without a session — andev.getPersons()on a detached entity will throwLazyInitializationException.The integration test gives false confidence. It passes today, but the behavior it exercises (lazy loading working within a
@DataJpaTesttransaction) is not representative of production behavior.Suggested additional test (to add as part of the fix)
Once
@Transactional(readOnly=true)is added toassemble(), the existing integration test becomes valid. Additionally, consider:This test must be run without
@Transactionalon the test method itself (or wrapped in aTransactionTemplateto execute the actual assertion outside any transaction), otherwise it won't catch the bug.Non-blocking notes
TimelineEntryDTOrecord uses positional constructors in test helpers (new TimelineEntryDTO(Kind.LETTER, ...)) — this is acceptable for test-only helpers but fragile if record fields are reordered. Consider named factory methods if the DTO stabilizes with 13 fields.test1,test2... is consistent with the project's existing integration test style. ✅🔐 Security (Nora "NullX" Steiner) — PR Review
Verdict: ✅ Approved
Read-only endpoint with no file handling, no new data flows, no AI invocation. Standard auth/authz model applied correctly.
@RequirePermission(Permission.X)personIdis a filter parameter on data the caller already has READ_ALL over; no per-resource ownership boundary to enforceTimelineServiceassemblesTimelineEntryDTOrecords before the data reaches the controller — entities never cross the boundary.senderName/receiverNameare display names, not email/password fieldsfindAll()noted, deferred to future pagination REQ)log.debug("assembled {} events, {} letters")— only aggregate countsAdditional checks:
@RequirePermission(Permission.READ_ALL)is on the method (not class) — correct for AOP interception viaPermissionAspect. ✅@ValidatedonTimelineControllerclass — required for Bean Validation annotations (@Min(0)) to fire. Without it, negativegenerationsilently passes. ✅personIdparam: Spring auto-rejects non-UUID strings with a 400 before reaching the service. ✅eventRepository.findAll()path: for a family archive with hundreds (not millions) of events, this is acceptable. The spec explicitly deferred pagination. ✅TimelineControllerTest— both auth failure modes are verified. ✅🚀 DevOps (Tobias Wendt) — PR Review
Verdict: ✅ Approved
Read-only endpoint with no infrastructure changes. All deployment concerns are clean.
GET /api/timelinereads existing data.env.exampleactions/upload-artifact@v4+npm audit+ Semgrep greenhttp_server_requests_secondsMicrometer metric automatically coversGET /api/timeline— no explicit wiring neededNotes:
/api/timelineroute. The existing CRUD endpoints moved to/api/timeline/events(as shown in the updated C4 diagram description). Verify theTimelineEventController's@RequestMappingwas actually changed in this branch — if it was already at/api/timeline/eventsand only the C4 diagram label was stale, no concern. If it was changed, this is a breaking change for any existing API consumers (the Chronik/Aktivitäten page, API test files). ✅ (the frontendapi.tsshows the CRUD routes under/api/timeline/events/{id}— confirmed correct)🎨 UI/UX (Leonie Voss) — PR Review
Verdict: ✅ Approved
This PR is backend + API types only. No Svelte components, routes, or i18n strings were changed. The frontend rendering (issue #6) and edit affordance (issue #7) are separate issues on the roadmap. I'm reviewing only whether the DTO contract is sound for those downstream issues.
getErrorMessage(code)ErrorCodein this PR{@html}DTO contract review for downstream issues #6 and #7:
The
TimelineEntryDTOshape is well-designed for frontend consumption:kind: "EVENT" | "LETTER"— clean discriminator for rendering branching ✅derived: boolean+eventId: UUID | null— edit affordance:if (!entry.derived && entry.eventId)→ show edit link. The Javadoc makes this explicit ✅senderName: string(non-null,""for unknown) +receiverName: string(non-null,""for unknown) — the frontend can dosenderName || m.person_unknown()without null-checking ✅precision: DatePrecisionas a raw enum value — letsdateLabel.ts(issue #778) do its own formatting rather than baking format logic into the backend ✅linkedPersonIds: UUID[]for EVENT kind — available for future hover cards or person-chip links ✅One note for issue #6: The Javadoc comment says
senderName "" → frontend renders 'Unbekannt'. When issue #6 implements this, usem.person_unknown()or the appropriate Paraglide key rather than hardcoding'Unbekannt'(German-only). The DTO contract itself is correct — this is just a heads-up for the consumer.🏛️ Architect (Markus Keller) — PR Review
Verdict: 🚫 Changes Requested
The domain design, C4 diagram update, and cross-boundary patterns are all correct. One production-breaking violation of constitution §1.6 must be fixed before merge, and the CLAUDE.md domain model table needs updating.
Blockers
1.
TimelineService.assemble()violates Constitution §1.6Constitution §1.6: "Lazy-collection-bearing entities are never serialized across the controller boundary; the owning service assembles an explicit view inside the transaction."
The key phrase is "inside the transaction."
TimelineService.assemble()has no@Transactionalannotation. It callseventRepository.findAll()(which opens and closes its own@Transactional(readOnly=true)sub-transaction), then accessesev.getPersons()— a@ManyToMany(fetch = FetchType.LAZY)collection — on detached entities. This is exactly the pattern §1.6 and ADR-036 were written to prevent.The
GeschichteQueryServiceis the reference implementation of the correct pattern: it's@Transactionaland assembles views inside that transaction. TheTimelineEventService.assembleDerivedEvents()(from the previous issue #776) was also correctly annotated@Transactional(readOnly=true).Fix required:
@Transactional(readOnly=true)onTimelineService.assemble().Note that CLAUDE.md's "read methods are not annotated" convention covers simple reads on scalar fields or eagerly-loaded associations. It was never meant to exempt orchestrator methods that access lazy collections. The constitution overrides CLAUDE.md when they conflict.
2. CLAUDE.md
TimelineEntryDTOdomain model table entry is staleThe spec for issue #777 required updating the CLAUDE.md domain model table for
TimelineEntryDTO. The current entry on the branch still reads:But the PR's actual
TimelineEntryDTOhas completely different fields:kind(EVENT/LETTER),senderName,receiverName,eventId: UUID,documentId: UUID,linkedPersonIds: List<UUID>,precision,derived,derivedType. Neitherid: StringnorprimaryPersonName/relatedPersonNameexist in the implemented record.The CLAUDE.md entry needs to be updated to match the actual 13-field shape. This is a required doc task (spec §Doc tasks) and is important because the stale entry will confuse future LLM agents reading CLAUDE.md.
Everything else is architecturally sound
Domain package structure ✅
TimelineService,TimelineController,TimelineFilter,TimelineDTO,TimelineYearDTOall correctly placed intimeline/packagetimeline/already existed and is inArchitectureTest's allow-listLayering ✅
TimelineController→TimelineServiceonly (§1.2)TimelineServiceusesTimelineEventRepository(own domain),TimelineEventService(own domain),PersonService(cross-domain via service),DocumentService(cross-domain via service) — noRelationshipServicereference (§1.3)C4 diagram ✅
l3-backend-timeline.pumlupdated: newTimelineServiceandTimelineControllercomponents added; edges correctly show noRelationshipServicedependency;TimelineEventControllerdescription updated to/api/timeline/eventsArchitectureTest ✅ (per PR description: "14/14 — includes new Rule 2 entry for
timelinedomain")No new ADR required ✅
@Transactionalis added)👨💻 Felix Brandt — PR update
Both blocker concerns from @felix and @markus are resolved. Two commits pushed to this branch.
Commit 1:
fix(timeline): add @Transactional(readOnly=true) to TimelineService.assemble()(590b00d2)Addresses: Developer blocker + Architect blocker 1 —
LazyInitializationExceptionin production.@Transactional(readOnly = true)toTimelineService.assemble().TimelineEvent.personsandDocument.receiverscollections, constitution §1.6) — this directly addresses @felix's suggestion to document the deviation from the "read methods don't need@Transactional" CLAUDE.md convention.TimelineServiceLazyLoadTest— a@SpringBootTestwithout class-level@Transactional, which commits aTimelineEventwith a linkedPersonviaTransactionTemplate, then callsassemble()outside any transaction. This test was RED (LazyInitializationException atTimelineService.mapEvent:198) before the fix and is GREEN after. It addresses @sara's concern that the existing@DataJpaTest @Transactionalclass gave false confidence.Commit 2:
docs(timeline): update TimelineEntryDTO domain model table in CLAUDE.md(1348255a)Addresses: Architect blocker 2 — stale CLAUDE.md
TimelineEntryDTOrow.Replaced the old entry (
id: String,primaryPersonName,relatedPersonName) with the actual 13-field record shape:kind,precision,derived,senderName,receiverName,eventDate,eventDateEnd,title,type,eventId,documentId,linkedPersonIds,derivedType— including the edit-affordance contract.Non-blocker / spec question from @req-eng (undated items + fromYear/toYear): The current behaviour (
passesYearFilterreturnstruefor null-date entries regardless offromYear/toYear) is the intentional design — undated items have no year to filter on so they always appear. This was a deliberate decision (not captured as a REQ). Agreed it should be noted; I'll add a clarifying sentence to REQ-009 in issue #777 as a follow-up rather than holding the PR.