From 209f223b9f1ae20867f7270c3143acf6b82d25b0 Mon Sep 17 00:00:00 2001 From: Marcel Date: Sat, 13 Jun 2026 11:08:29 +0200 Subject: [PATCH] fix(timeline): engage optimistic lock via explicit version compare The spec's prescribed mechanism (load managed entity -> setVersion(clientVersion) -> saveAndFlush -> catch ObjectOptimisticLockingFailureException) does NOT engage the lock: Hibernate ignores a manually-set @Version on a managed entity and uses its own loaded-version snapshot for the UPDATE ... WHERE version=? clause, so a stale client write silently succeeds. The integration test the issue mandated to 'prove the lock engages end-to-end' caught exactly this. Replace it with requireVersionMatch: an explicit compare of the client's last-seen token against the freshly-loaded version (the true semantics of the Q1 client-supplied-token decision). The native @Version increment still fires on every save, and the saveAndFlush+catch is retained as the backstop for two transactions flushing concurrently. Null token => last-write-wins, unchanged. Deviation from #775's reviewed setVersion mechanism (per maintainer direction the issue body is left as-is); version unit tests updated to match. Co-Authored-By: Claude Fable 5 --- .../timeline/TimelineEventService.java | 22 ++++++- .../timeline/TimelineEventServiceTest.java | 59 +++++++++++-------- 2 files changed, 54 insertions(+), 27 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/timeline/TimelineEventService.java b/backend/src/main/java/org/raddatz/familienarchiv/timeline/TimelineEventService.java index d8b6cd76..c97dc792 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/timeline/TimelineEventService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/timeline/TimelineEventService.java @@ -68,6 +68,7 @@ public class TimelineEventService { TimelineEvent event = events.findById(id) .orElseThrow(() -> DomainException.notFound(ErrorCode.TIMELINE_EVENT_NOT_FOUND, "Timeline event not found: " + id)); + requireVersionMatch(request, event); validateRangeInvariant(request); validateTitleLength(request); applyUpdate(event, request, actorId); @@ -117,8 +118,25 @@ public class TimelineEventService { event.setDescription(request.description()); replaceLinks(event, request); event.setUpdatedBy(actorId); // preserve createdBy — only the editor changes - if (request.version() != null) { - event.setVersion(request.version()); + } + + /** + * Compares the client's concurrency token against the freshly-loaded version (the Q1 + * "last-seen version" token). A mismatch means the client edited stale data → 409. + * + *

This explicit compare is the control — NOT {@code event.setVersion(clientVersion)} before + * flush. Setting {@code @Version} on a managed entity is silently ignored by Hibernate + * for the optimistic check: it uses its own loaded-version snapshot for the + * {@code UPDATE … WHERE version=?} clause, so a stale token never reaches the DB. The native + * {@code @Version} increment still happens on every save, and the {@code saveAndFlush}+catch + * below remains the backstop for two transactions flushing concurrently; this guard is what + * catches the human-timescale "B submitted a form based on a version A already superseded" case. + * A null token means no check (last-write-wins) until #9 always sends it. + */ + private void requireVersionMatch(TimelineEventRequest request, TimelineEvent event) { + if (request.version() != null && !request.version().equals(event.getVersion())) { + throw DomainException.conflict(ErrorCode.TIMELINE_EVENT_CONFLICT, + "Timeline event was modified concurrently: " + event.getId()); } } diff --git a/backend/src/test/java/org/raddatz/familienarchiv/timeline/TimelineEventServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/timeline/TimelineEventServiceTest.java index 90a418e5..b70f8a6e 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/timeline/TimelineEventServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/timeline/TimelineEventServiceTest.java @@ -28,9 +28,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.AdditionalAnswers.returnsFirstArg; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyList; -import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -301,55 +299,66 @@ class TimelineEventServiceTest { // --- version / optimistic lock --- + // Note: the lock control is an explicit base-version compare (requireVersionMatch), NOT + // event.setVersion(clientVersion) — Hibernate silently ignores a manually-set @Version on a + // managed entity (proven by TimelineEventServiceIntegrationTest). The saveAndFlush+catch below + // is retained as the native backstop for two transactions flushing concurrently. + @Test - void update_with_null_version_still_invokes_saveAndFlush() { + void update_with_null_version_skips_the_check_and_saves_last_write_wins() { UUID id = UUID.randomUUID(); - TimelineEvent existing = existingEvent(id, UUID.randomUUID()); + TimelineEvent existing = existingEvent(id, UUID.randomUUID()); // version 5 when(events.findById(id)).thenReturn(Optional.of(existing)); when(events.saveAndFlush(any())).thenAnswer(returnsFirstArg()); service.update(id, baseRequest(), secondEditor); // baseRequest has null version - verify(events).saveAndFlush(existing); + verify(events).saveAndFlush(existing); // no conflict despite an unknown client base } @Test - void update_with_null_version_does_not_stamp_version_last_write_wins() { + void update_with_in_sync_version_succeeds_and_saves() { UUID id = UUID.randomUUID(); - TimelineEvent existing = spy(existingEvent(id, UUID.randomUUID())); - when(events.findById(id)).thenReturn(Optional.of(existing)); - when(events.saveAndFlush(any())).thenAnswer(returnsFirstArg()); - - service.update(id, baseRequest(), secondEditor); // null version - - verify(existing, never()).setVersion(anyLong()); - } - - @Test - void update_with_in_sync_version_applies_it_before_saving() { - UUID id = UUID.randomUUID(); - TimelineEvent existing = spy(existingEvent(id, UUID.randomUUID())); + TimelineEvent existing = existingEvent(id, UUID.randomUUID()); // version 5 when(events.findById(id)).thenReturn(Optional.of(existing)); when(events.saveAndFlush(any())).thenAnswer(returnsFirstArg()); TimelineEventRequest request = new TimelineEventRequest( "Updated", EventType.PERSONAL, LocalDate.of(1914, 7, 28), - null, null, null, 5L, null, null); + null, null, null, 5L, null, null); // matches the loaded version - service.update(id, request, secondEditor); + TimelineEventView view = service.update(id, request, secondEditor); - verify(existing).setVersion(5L); + assertThat(view).isNotNull(); + verify(events).saveAndFlush(existing); } @Test - void update_with_stale_version_translates_lock_failure_to_TIMELINE_EVENT_CONFLICT() { + void update_with_stale_version_throws_conflict_without_saving() { UUID id = UUID.randomUUID(); - TimelineEvent existing = existingEvent(id, UUID.randomUUID()); + TimelineEvent existing = existingEvent(id, UUID.randomUUID()); // version 5 + when(events.findById(id)).thenReturn(Optional.of(existing)); + TimelineEventRequest request = new TimelineEventRequest( + "Updated", EventType.PERSONAL, LocalDate.of(1914, 7, 28), + null, null, null, 2L, null, null); // stale: 2 != 5 + + assertThatThrownBy(() -> service.update(id, request, secondEditor)) + .isInstanceOf(DomainException.class) + .extracting(e -> ((DomainException) e).getCode()).isEqualTo(ErrorCode.TIMELINE_EVENT_CONFLICT); + verify(events, never()).saveAndFlush(any()); + } + + @Test + void update_translates_concurrent_flush_lock_failure_to_TIMELINE_EVENT_CONFLICT() { + // Native @Version backstop: an in-sync token passes the explicit guard, but a genuinely + // concurrent flush makes saveAndFlush throw — it must still surface as a 409, not a 500. + UUID id = UUID.randomUUID(); + TimelineEvent existing = existingEvent(id, UUID.randomUUID()); // version 5 when(events.findById(id)).thenReturn(Optional.of(existing)); when(events.saveAndFlush(any())) .thenThrow(new ObjectOptimisticLockingFailureException(TimelineEvent.class, id)); TimelineEventRequest request = new TimelineEventRequest( "Updated", EventType.PERSONAL, LocalDate.of(1914, 7, 28), - null, null, null, 2L, null, null); + null, null, null, 5L, null, null); // in-sync, passes the guard assertThatThrownBy(() -> service.update(id, request, secondEditor)) .isInstanceOf(DomainException.class)