fix(review): address PR #788 review blockers
Some checks failed
CI / Unit & Component Tests (pull_request) Successful in 3m26s
CI / OCR Service Tests (pull_request) Successful in 23s
CI / Backend Unit Tests (pull_request) Successful in 3m53s
CI / fail2ban Regex (pull_request) Failing after 46s
CI / Semgrep Security Scan (pull_request) Successful in 25s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m6s
Some checks failed
CI / Unit & Component Tests (pull_request) Successful in 3m26s
CI / OCR Service Tests (pull_request) Successful in 23s
CI / Backend Unit Tests (pull_request) Successful in 3m53s
CI / fail2ban Regex (pull_request) Failing after 46s
CI / Semgrep Security Scan (pull_request) Successful in 25s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m6s
- 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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"));
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -13,8 +13,6 @@ import java.util.UUID;
|
||||
@Repository
|
||||
public interface JourneyItemRepository extends JpaRepository<JourneyItem, UUID> {
|
||||
|
||||
List<JourneyItem> findAllByGeschichteId(UUID geschichteId);
|
||||
|
||||
/** Returns items ordered by position ASC for the read-model assembly path. */
|
||||
List<JourneyItem> findByGeschichteIdOrderByPosition(UUID geschichteId);
|
||||
|
||||
|
||||
@@ -138,6 +138,11 @@ public class JourneyItemService {
|
||||
Set<UUID> existingIds = journeyItemRepository.findIdsByGeschichteId(geschichteId);
|
||||
List<UUID> 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");
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -96,7 +96,7 @@ class JourneyItemIntegrationTest {
|
||||
geschichteRepository.deleteById(geschichteId);
|
||||
em.flush();
|
||||
|
||||
assertThat(journeyItemRepository.findAllByGeschichteId(geschichteId)).isEmpty();
|
||||
assertThat(journeyItemRepository.findByGeschichteIdOrderByPosition(geschichteId)).isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -376,8 +376,10 @@ package "Supporting" {
|
||||
--
|
||||
geschichte_id : UUID <<FK>>
|
||||
document_id : UUID <<FK>>
|
||||
position : INTEGER NOT NULL
|
||||
position : INTEGER NOT NULL CHECK (position > 0)
|
||||
note : TEXT
|
||||
==
|
||||
UNIQUE (geschichte_id, position) DEFERRABLE INITIALLY DEFERRED
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user