From 4eb6abd9201b2912b59a8223ca7de037c65ba76f Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 8 Jun 2026 17:16:32 +0200 Subject: [PATCH] fix(review): address PR #788 review blockers - GlobalExceptionHandler maps uq_journey_items_geschichte_position constraint violation to HTTP 409 JOURNEY_ITEM_POSITION_CONFLICT - JourneyItemService.reorder() rejects duplicate IDs before set-equality check to prevent silent position overwrite - JourneyItemRepository removes orphaned findAllByGeschichteId method - GeschichteView removes stale com.fasterxml.jackson import - Tests: add appendItem_returns409_on_position_conflict (controller), reorder_returns400_when_itemIds_contain_duplicates (service) - Fix JourneyItemIntegrationTest compilation after repository cleanup - db-orm.puml annotates journey_items position with CHECK + UNIQUE DEFERRABLE Co-Authored-By: Claude Sonnet 4.6 --- .../exception/GlobalExceptionHandler.java | 9 ++++++++- .../geschichte/GeschichteView.java | 1 - .../journeyitem/JourneyItemRepository.java | 2 -- .../journeyitem/JourneyItemService.java | 5 +++++ .../geschichte/GeschichteControllerTest.java | 16 ++++++++++++++++ .../journeyitem/JourneyItemIntegrationTest.java | 2 +- .../journeyitem/JourneyItemServiceTest.java | 14 ++++++++++++++ docs/architecture/db/db-orm.puml | 4 +++- 8 files changed, 47 insertions(+), 6 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java b/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java index 686ef457..c56cc576 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java @@ -78,7 +78,14 @@ public class GlobalExceptionHandler { // Log the constraint NAME only — schema metadata, safe for Loki, and enough to tell which // constraint fired at 2am. Never pass `ex` / `ex.getMessage()`: those embed the SQL + the // offending values (CWE-209). No Sentry: an integrity violation is a 400, not a system fault. - log.warn("Rejected a request that violated a database integrity constraint: {}", constraintNameOf(ex)); + String constraint = constraintNameOf(ex); + log.warn("Rejected a request that violated a database integrity constraint: {}", constraint); + if ("uq_journey_items_geschichte_position".equals(constraint)) { + // DEFERRABLE INITIALLY DEFERRED — fires at commit when concurrent appends/reorders collide + return ResponseEntity.status(409) + .body(new ErrorResponse(ErrorCode.JOURNEY_ITEM_POSITION_CONFLICT, + "A position conflict was detected — another request modified this journey simultaneously")); + } return ResponseEntity.badRequest() .body(new ErrorResponse(ErrorCode.VALIDATION_ERROR, "The submitted data violated a database constraint")); } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/geschichte/GeschichteView.java b/backend/src/main/java/org/raddatz/familienarchiv/geschichte/GeschichteView.java index 2fa5a920..e7d0e3f6 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/geschichte/GeschichteView.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/geschichte/GeschichteView.java @@ -1,6 +1,5 @@ package org.raddatz.familienarchiv.geschichte; -import com.fasterxml.jackson.annotation.JsonInclude; import io.swagger.v3.oas.annotations.media.Schema; import org.raddatz.familienarchiv.geschichte.journeyitem.JourneyItemView; import org.raddatz.familienarchiv.person.Person; diff --git a/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemRepository.java b/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemRepository.java index 4a56ee07..666681e6 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemRepository.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemRepository.java @@ -13,8 +13,6 @@ import java.util.UUID; @Repository public interface JourneyItemRepository extends JpaRepository { - List findAllByGeschichteId(UUID geschichteId); - /** Returns items ordered by position ASC for the read-model assembly path. */ List findByGeschichteIdOrderByPosition(UUID geschichteId); diff --git a/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemService.java b/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemService.java index d0835c26..e898b026 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemService.java @@ -138,6 +138,11 @@ public class JourneyItemService { Set existingIds = journeyItemRepository.findIdsByGeschichteId(geschichteId); List requestedIds = dto.getItemIds() != null ? dto.getItemIds() : List.of(); + if (requestedIds.size() != new HashSet<>(requestedIds).size()) { + throw DomainException.badRequest(ErrorCode.VALIDATION_ERROR, + "Duplicate item IDs in reorder request"); + } + if (!existingIds.equals(new HashSet<>(requestedIds))) { throw DomainException.badRequest(ErrorCode.VALIDATION_ERROR, "Requested item IDs do not match the journey's existing items"); diff --git a/backend/src/test/java/org/raddatz/familienarchiv/geschichte/GeschichteControllerTest.java b/backend/src/test/java/org/raddatz/familienarchiv/geschichte/GeschichteControllerTest.java index 88320dc2..0305358f 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/geschichte/GeschichteControllerTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/geschichte/GeschichteControllerTest.java @@ -359,6 +359,22 @@ class GeschichteControllerTest { .andExpect(jsonPath("$[0].id").value(itemId.toString())); } + // ─── error mapping ─────────────────────────────────────────────────────── + + @Test + @WithMockUser(authorities = "BLOG_WRITE") + void appendItem_returns409_on_position_conflict() throws Exception { + UUID id = UUID.randomUUID(); + when(journeyItemService.append(eq(id), any())) + .thenThrow(DomainException.conflict(ErrorCode.JOURNEY_ITEM_POSITION_CONFLICT, "conflict")); + + mockMvc.perform(post("/api/geschichten/{id}/items", id).with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"note\":\"x\"}")) + .andExpect(status().isConflict()) + .andExpect(jsonPath("$.code").value("JOURNEY_ITEM_POSITION_CONFLICT")); + } + // ─── helpers ───────────────────────────────────────────────────────────── private JourneyItemView itemViewStub(UUID id, int position, String note) { diff --git a/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemIntegrationTest.java b/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemIntegrationTest.java index 8de9c41e..00c2591c 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemIntegrationTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemIntegrationTest.java @@ -96,7 +96,7 @@ class JourneyItemIntegrationTest { geschichteRepository.deleteById(geschichteId); em.flush(); - assertThat(journeyItemRepository.findAllByGeschichteId(geschichteId)).isEmpty(); + assertThat(journeyItemRepository.findByGeschichteIdOrderByPosition(geschichteId)).isEmpty(); } @Test diff --git a/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemServiceTest.java index a6afe3b6..2f3161e2 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/geschichte/journeyitem/JourneyItemServiceTest.java @@ -448,6 +448,20 @@ class JourneyItemServiceTest { // ─── reorder ───────────────────────────────────────────────────────────── + @Test + void reorder_returns400_when_itemIds_contain_duplicates() { + UUID id1 = UUID.randomUUID(); + when(journeyItemRepository.findIdsByGeschichteId(geschichteId)).thenReturn(Set.of(id1)); + + JourneyReorderDTO dto = new JourneyReorderDTO(); + dto.setItemIds(List.of(id1, id1)); // duplicate + + assertThatThrownBy(() -> journeyItemService.reorder(geschichteId, dto)) + .isInstanceOf(DomainException.class) + .satisfies(e -> assertThat(((DomainException) e).getCode()) + .isEqualTo(ErrorCode.VALIDATION_ERROR)); + } + @Test void reorder_returns400_when_itemId_belongs_to_different_journey() { UUID foreignId = UUID.randomUUID(); diff --git a/docs/architecture/db/db-orm.puml b/docs/architecture/db/db-orm.puml index 4f71c08a..a1ebd7d2 100644 --- a/docs/architecture/db/db-orm.puml +++ b/docs/architecture/db/db-orm.puml @@ -376,8 +376,10 @@ package "Supporting" { -- geschichte_id : UUID <> document_id : UUID <> - position : INTEGER NOT NULL + position : INTEGER NOT NULL CHECK (position > 0) note : TEXT + == + UNIQUE (geschichte_id, position) DEFERRABLE INITIALLY DEFERRED } }