marcel/familienarchiv
1
0
Fork 0
Code Issues 108 Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(timeline): engage optimistic lock via explicit version compare

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-06-13 11:08:29 +02:00
committed by marcel
parent 34146d7309
commit 209f223b9f
2 changed files with 54 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
backend/src/main/java/org/raddatz/familienarchiv/timeline/TimelineEventService.java
View File

@@ -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.
*
* <p>This explicit compare is the control — NOT {@code event.setVersion(clientVersion)} before
* flush. Setting {@code @Version} on a <em>managed</em> 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());
}
}

59
backend/src/test/java/org/raddatz/familienarchiv/timeline/TimelineEventServiceTest.java
View File

@@ -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)