fix(geschichte): delete note-less journey items before document delete (#805) #806
@@ -52,6 +52,11 @@ public enum AuditKind {
|
||||
/** Payload: {@code {"ip": "1.2.3.4", "email": "addr"}} — password NEVER included */
|
||||
LOGIN_RATE_LIMITED,
|
||||
|
||||
// --- Documents ---
|
||||
|
||||
/** Payload: none — the deleted document's id is carried in the documentId column */
|
||||
DOCUMENT_DELETED,
|
||||
|
||||
// --- Reading Journeys (Lesereisen) ---
|
||||
|
||||
/** Payload: {@code {"geschichteId": "uuid", "itemId": "uuid"}} — documentId is null (journey-scoped, not document-scoped) */
|
||||
|
||||
@@ -168,8 +168,8 @@ public class DocumentController {
|
||||
|
||||
@DeleteMapping("/{id}")
|
||||
@RequirePermission(Permission.WRITE_ALL)
|
||||
public ResponseEntity<Void> deleteDocument(@PathVariable UUID id) {
|
||||
documentService.deleteDocument(id);
|
||||
public ResponseEntity<Void> deleteDocument(@PathVariable UUID id, Authentication authentication) {
|
||||
documentService.deleteDocument(id, requireUserId(authentication));
|
||||
return ResponseEntity.noContent().build();
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,11 @@
|
||||
package org.raddatz.familienarchiv.document;
|
||||
|
||||
import java.util.UUID;
|
||||
|
||||
/**
|
||||
* Published by DocumentService.deleteDocument inside its @Transactional boundary,
|
||||
* before documentRepository.deleteById fires. Listeners run synchronously in the
|
||||
* publisher's thread and transaction via plain @EventListener — this is load-bearing:
|
||||
* see ADR-038.
|
||||
*/
|
||||
public record DocumentDeletingEvent(UUID documentId) {}
|
||||
@@ -28,6 +28,7 @@ import org.raddatz.familienarchiv.ocr.TrainingLabel;
|
||||
import org.raddatz.familienarchiv.person.Person;
|
||||
import org.raddatz.familienarchiv.tag.Tag;
|
||||
import org.raddatz.familienarchiv.document.DocumentRepository;
|
||||
import org.springframework.context.ApplicationEventPublisher;
|
||||
import org.springframework.data.domain.Page;
|
||||
import org.springframework.data.domain.PageRequest;
|
||||
import org.springframework.data.domain.Pageable;
|
||||
@@ -80,6 +81,7 @@ public class DocumentService {
|
||||
private final TranscriptionBlockQueryService transcriptionBlockQueryService;
|
||||
private final AuditLogQueryService auditLogQueryService;
|
||||
private final ThumbnailAsyncRunner thumbnailAsyncRunner;
|
||||
private final ApplicationEventPublisher eventPublisher;
|
||||
|
||||
public record StoreResult(Document document, boolean isNew) {}
|
||||
|
||||
@@ -1097,11 +1099,13 @@ public class DocumentService {
|
||||
}
|
||||
|
||||
@Transactional
|
||||
public void deleteDocument(UUID id) {
|
||||
public void deleteDocument(UUID id, UUID actorId) {
|
||||
if (!documentRepository.existsById(id)) {
|
||||
throw DomainException.notFound(ErrorCode.DOCUMENT_NOT_FOUND, "Document not found: " + id);
|
||||
}
|
||||
eventPublisher.publishEvent(new DocumentDeletingEvent(id));
|
||||
documentRepository.deleteById(id);
|
||||
auditService.logAfterCommit(AuditKind.DOCUMENT_DELETED, actorId, id, null);
|
||||
}
|
||||
|
||||
@Transactional
|
||||
|
||||
@@ -0,0 +1,30 @@
|
||||
package org.raddatz.familienarchiv.geschichte.journeyitem;
|
||||
|
||||
import lombok.RequiredArgsConstructor;
|
||||
import lombok.extern.slf4j.Slf4j;
|
||||
import org.raddatz.familienarchiv.document.DocumentDeletingEvent;
|
||||
import org.springframework.context.event.EventListener;
|
||||
import org.springframework.stereotype.Component;
|
||||
|
||||
@Component
|
||||
@RequiredArgsConstructor
|
||||
@Slf4j
|
||||
class JourneyItemDocumentDeleteListener {
|
||||
|
||||
private final JourneyItemRepository journeyItemRepository;
|
||||
|
||||
/**
|
||||
* Plain @EventListener — runs synchronously in the publisher's thread and transaction.
|
||||
* Load-bearing choice: AFTER_COMMIT would fire after the FK ON DELETE SET NULL has
|
||||
* already 500'd; @Async would run outside the delete transaction (breaks AC-5 rollback).
|
||||
* See ADR-038. DocumentService cannot call JourneyItemService directly because
|
||||
* Spring Framework 7 prohibits the resulting constructor-injection cycle.
|
||||
*/
|
||||
@EventListener
|
||||
void onDocumentDeleting(DocumentDeletingEvent event) {
|
||||
int deleted = journeyItemRepository.deleteNoteLessByDocumentId(event.documentId());
|
||||
if (deleted > 0) {
|
||||
log.warn("Cascade-deleted {} note-less journey item(s) for document {}", deleted, event.documentId());
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,6 +1,7 @@
|
||||
package org.raddatz.familienarchiv.geschichte.journeyitem;
|
||||
|
||||
import org.springframework.data.jpa.repository.JpaRepository;
|
||||
import org.springframework.data.jpa.repository.Modifying;
|
||||
import org.springframework.data.jpa.repository.Query;
|
||||
import org.springframework.data.repository.query.Param;
|
||||
import org.springframework.stereotype.Repository;
|
||||
@@ -43,6 +44,20 @@ public interface JourneyItemRepository extends JpaRepository<JourneyItem, UUID>
|
||||
boolean existsByGeschichteIdAndDocumentId(
|
||||
@Param("geschichteId") UUID geschichteId, @Param("documentId") UUID documentId);
|
||||
|
||||
/**
|
||||
* Deletes note-less items (note IS NULL or note = '') linked to the given document.
|
||||
* Used by JourneyItemDocumentDeleteListener before the document row is removed, so
|
||||
* the FK ON DELETE SET NULL never fires on rows that would violate chk_journey_item_not_empty.
|
||||
* Explicit JPQL — same trap as existsByGeschichteIdAndDocumentId: the transient
|
||||
* getDocumentId() getter makes Spring Data unable to resolve a derived query path.
|
||||
* clearAutomatically = true invalidates the L1 cache so AC-2's "note-carrying survives"
|
||||
* assertion never reads a stale entity. flushAutomatically = true makes the
|
||||
* flush-before-delete contract explicit rather than relying on Hibernate AUTO flush mode.
|
||||
*/
|
||||
@Modifying(clearAutomatically = true, flushAutomatically = true)
|
||||
@Query("DELETE FROM JourneyItem i WHERE i.document.id = :documentId AND (i.note IS NULL OR i.note = '')")
|
||||
int deleteNoteLessByDocumentId(@Param("documentId") UUID documentId);
|
||||
|
||||
/**
|
||||
* Loads journey items with their linked Document in a single JOIN FETCH query,
|
||||
* eliminating the N+1 SELECT that would occur when accessing item.getDocument()
|
||||
|
||||
@@ -402,6 +402,7 @@ class DocumentControllerTest {
|
||||
@WithMockUser(authorities = "WRITE_ALL")
|
||||
void deleteDocument_returns204_whenHasWritePermission() throws Exception {
|
||||
UUID id = UUID.randomUUID();
|
||||
when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build());
|
||||
mockMvc.perform(org.springframework.test.web.servlet.request.MockMvcRequestBuilders
|
||||
.delete("/api/documents/" + id).with(csrf()))
|
||||
.andExpect(status().isNoContent());
|
||||
|
||||
@@ -30,6 +30,7 @@ import org.raddatz.familienarchiv.document.DocumentRepository;
|
||||
import org.raddatz.familienarchiv.filestorage.FileService;
|
||||
import org.raddatz.familienarchiv.tag.TagService;
|
||||
import org.raddatz.familienarchiv.person.PersonService;
|
||||
import org.springframework.context.ApplicationEventPublisher;
|
||||
import org.springframework.data.domain.Page;
|
||||
import org.springframework.data.domain.PageImpl;
|
||||
import org.springframework.data.domain.PageRequest;
|
||||
@@ -75,6 +76,7 @@ class DocumentServiceTest {
|
||||
@Mock AuditLogQueryService auditLogQueryService;
|
||||
@Mock TranscriptionBlockQueryService transcriptionBlockQueryService;
|
||||
@Mock ThumbnailAsyncRunner thumbnailAsyncRunner;
|
||||
@Mock ApplicationEventPublisher eventPublisher;
|
||||
// Real factory (pure, dependency-free) so save-time title-regeneration tests exercise the
|
||||
// shared composition rather than a stub — the #726 single source of truth.
|
||||
@Spy DocumentTitleFactory documentTitleFactory = new DocumentTitleFactory();
|
||||
@@ -87,7 +89,7 @@ class DocumentServiceTest {
|
||||
UUID id = UUID.randomUUID();
|
||||
when(documentRepository.existsById(id)).thenReturn(true);
|
||||
|
||||
documentService.deleteDocument(id);
|
||||
documentService.deleteDocument(id, UUID.randomUUID());
|
||||
|
||||
verify(documentRepository).deleteById(id);
|
||||
}
|
||||
@@ -97,7 +99,7 @@ class DocumentServiceTest {
|
||||
UUID id = UUID.randomUUID();
|
||||
when(documentRepository.existsById(id)).thenReturn(false);
|
||||
|
||||
assertThatThrownBy(() -> documentService.deleteDocument(id))
|
||||
assertThatThrownBy(() -> documentService.deleteDocument(id, UUID.randomUUID()))
|
||||
.isInstanceOf(DomainException.class)
|
||||
.hasMessageContaining(id.toString());
|
||||
verify(documentRepository, never()).deleteById(any());
|
||||
|
||||
@@ -0,0 +1,261 @@
|
||||
package org.raddatz.familienarchiv.geschichte.journeyitem;
|
||||
|
||||
import jakarta.persistence.EntityManager;
|
||||
import jakarta.persistence.PersistenceContext;
|
||||
import org.junit.jupiter.api.AfterEach;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.raddatz.familienarchiv.PostgresContainerConfig;
|
||||
import org.raddatz.familienarchiv.audit.AuditKind;
|
||||
import org.raddatz.familienarchiv.audit.AuditService;
|
||||
import org.raddatz.familienarchiv.document.Document;
|
||||
import org.raddatz.familienarchiv.document.DocumentRepository;
|
||||
import org.raddatz.familienarchiv.document.DocumentService;
|
||||
import org.raddatz.familienarchiv.document.DocumentStatus;
|
||||
import org.raddatz.familienarchiv.geschichte.Geschichte;
|
||||
import org.raddatz.familienarchiv.geschichte.GeschichteRepository;
|
||||
import org.raddatz.familienarchiv.geschichte.GeschichteStatus;
|
||||
import org.raddatz.familienarchiv.geschichte.GeschichteType;
|
||||
import org.raddatz.familienarchiv.user.AppUser;
|
||||
import org.raddatz.familienarchiv.user.AppUserRepository;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.boot.test.context.SpringBootTest;
|
||||
import org.springframework.context.annotation.Import;
|
||||
import org.springframework.jdbc.core.JdbcTemplate;
|
||||
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
|
||||
import org.springframework.security.core.authority.SimpleGrantedAuthority;
|
||||
import org.springframework.security.core.context.SecurityContextHolder;
|
||||
import org.springframework.test.context.ActiveProfiles;
|
||||
import org.springframework.test.context.bean.override.mockito.MockitoBean;
|
||||
import org.springframework.test.context.bean.override.mockito.MockitoSpyBean;
|
||||
import software.amazon.awssdk.services.s3.S3Client;
|
||||
|
||||
import java.util.List;
|
||||
import java.util.UUID;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.ArgumentMatchers.eq;
|
||||
import static org.mockito.Mockito.doThrow;
|
||||
import static org.mockito.Mockito.never;
|
||||
import static org.mockito.Mockito.reset;
|
||||
import static org.mockito.Mockito.verify;
|
||||
|
||||
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE)
|
||||
@ActiveProfiles("test")
|
||||
@Import(PostgresContainerConfig.class)
|
||||
class JourneyItemDocumentDeleteTest {
|
||||
|
||||
@MockitoBean
|
||||
S3Client s3Client;
|
||||
|
||||
@MockitoBean
|
||||
AuditService auditService;
|
||||
|
||||
@MockitoSpyBean
|
||||
DocumentRepository documentRepository;
|
||||
|
||||
@PersistenceContext
|
||||
EntityManager em;
|
||||
|
||||
@Autowired DocumentService documentService;
|
||||
@Autowired JourneyItemRepository journeyItemRepository;
|
||||
@Autowired GeschichteRepository geschichteRepository;
|
||||
@Autowired DocumentRepository docRepo;
|
||||
@Autowired AppUserRepository appUserRepository;
|
||||
@Autowired JdbcTemplate jdbcTemplate;
|
||||
|
||||
Geschichte journey;
|
||||
Document doc;
|
||||
AppUser writer;
|
||||
|
||||
@BeforeEach
|
||||
void seed() {
|
||||
writer = appUserRepository.save(AppUser.builder()
|
||||
.email("delete-test-writer@test")
|
||||
.password("hash")
|
||||
.build());
|
||||
doc = docRepo.save(Document.builder()
|
||||
.title("Testbrief")
|
||||
.originalFilename("testbrief.pdf")
|
||||
.status(DocumentStatus.UPLOADED)
|
||||
.build());
|
||||
journey = geschichteRepository.save(Geschichte.builder()
|
||||
.title("Eine Lesereise")
|
||||
.status(GeschichteStatus.DRAFT)
|
||||
.type(GeschichteType.JOURNEY)
|
||||
.build());
|
||||
SecurityContextHolder.getContext().setAuthentication(
|
||||
new UsernamePasswordAuthenticationToken(writer.getEmail(), null,
|
||||
List.of(new SimpleGrantedAuthority("BLOG_WRITE"))));
|
||||
}
|
||||
|
||||
@AfterEach
|
||||
void cleanup() {
|
||||
SecurityContextHolder.clearContext();
|
||||
reset(documentRepository);
|
||||
// Deletion order is FK-load-bearing: journey_items reference both documents
|
||||
// and geschichten, so children must be removed before their parents.
|
||||
journeyItemRepository.deleteAll();
|
||||
docRepo.deleteAll();
|
||||
geschichteRepository.deleteAll();
|
||||
appUserRepository.deleteAll();
|
||||
}
|
||||
|
||||
// ─── AC-1: headline ───────────────────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void deleting_document_linked_via_note_less_item_deletes_item_not_500() {
|
||||
JourneyItem item = journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey).position(10).document(doc).build());
|
||||
em.clear();
|
||||
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
|
||||
assertThat(journeyItemRepository.findById(item.getId())).isEmpty();
|
||||
assertThat(docRepo.findById(doc.getId())).isEmpty();
|
||||
}
|
||||
|
||||
// ─── AC-2: note-carrying item survives as placeholder ─────────────────────
|
||||
|
||||
@Test
|
||||
void deleting_document_preserves_note_carrying_item_as_placeholder() {
|
||||
JourneyItem item = journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey).position(10).document(doc).note("curator context").build());
|
||||
em.clear();
|
||||
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
em.clear();
|
||||
|
||||
JourneyItem surviving = journeyItemRepository.findById(item.getId()).orElseThrow();
|
||||
assertThat(surviving.getDocumentId()).isNull();
|
||||
assertThat(surviving.getNote()).isEqualTo("curator context");
|
||||
}
|
||||
|
||||
// ─── AC-3: note-only item untouched ───────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void deleting_document_does_not_affect_note_only_item() {
|
||||
JourneyItem noteOnly = journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey).position(10).note("Einleitung").build());
|
||||
em.clear();
|
||||
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
em.clear();
|
||||
|
||||
JourneyItem reloaded = journeyItemRepository.findById(noteOnly.getId()).orElseThrow();
|
||||
assertThat(reloaded.getDocumentId()).isNull();
|
||||
assertThat(reloaded.getNote()).isEqualTo("Einleitung");
|
||||
}
|
||||
|
||||
// ─── AC-4: asymmetric multi-journey ───────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void deleting_document_applies_independently_per_referencing_item() {
|
||||
Geschichte journey2 = geschichteRepository.save(Geschichte.builder()
|
||||
.title("Zweite Reise")
|
||||
.status(GeschichteStatus.DRAFT)
|
||||
.type(GeschichteType.JOURNEY)
|
||||
.build());
|
||||
|
||||
JourneyItem noteLess = journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey).position(10).document(doc).build());
|
||||
JourneyItem noteCarrying = journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey2).position(10).document(doc).note("Begleittext").build());
|
||||
em.clear();
|
||||
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
em.clear();
|
||||
|
||||
assertThat(journeyItemRepository.findById(noteLess.getId())).isEmpty();
|
||||
JourneyItem surviving = journeyItemRepository.findById(noteCarrying.getId()).orElseThrow();
|
||||
assertThat(surviving.getDocumentId()).isNull();
|
||||
assertThat(surviving.getNote()).isEqualTo("Begleittext");
|
||||
}
|
||||
|
||||
// ─── AC-5: rollback guard ─────────────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void listener_deletes_roll_back_when_document_delete_fails() {
|
||||
JourneyItem item = journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey).position(10).document(doc).build());
|
||||
em.clear();
|
||||
|
||||
doThrow(new RuntimeException("simulated failure"))
|
||||
.when(documentRepository).deleteById(any());
|
||||
|
||||
assertThatThrownBy(() -> documentService.deleteDocument(doc.getId(), writer.getId()))
|
||||
.isInstanceOf(RuntimeException.class);
|
||||
|
||||
em.clear();
|
||||
assertThat(journeyItemRepository.findById(item.getId())).isPresent();
|
||||
}
|
||||
|
||||
// ─── AC-6: empty-string note boundary ────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void empty_string_note_item_is_cascaded_whitespace_only_note_is_preserved() {
|
||||
// uq_journey_items_geschichte_document prevents two items with the same
|
||||
// (geschichte_id, document_id) in one journey — use two separate journeys.
|
||||
Geschichte journey2 = geschichteRepository.save(Geschichte.builder()
|
||||
.title("Zweite Reise für AC-6")
|
||||
.status(GeschichteStatus.DRAFT)
|
||||
.type(GeschichteType.JOURNEY)
|
||||
.build());
|
||||
|
||||
UUID emptyNoteItemId = UUID.randomUUID();
|
||||
UUID whitespaceNoteItemId = UUID.randomUUID();
|
||||
|
||||
jdbcTemplate.update(
|
||||
"INSERT INTO journey_items (id, geschichte_id, position, document_id, note) VALUES (?,?,?,?,?)",
|
||||
emptyNoteItemId, journey.getId(), 10, doc.getId(), "");
|
||||
jdbcTemplate.update(
|
||||
"INSERT INTO journey_items (id, geschichte_id, position, document_id, note) VALUES (?,?,?,?,?)",
|
||||
whitespaceNoteItemId, journey2.getId(), 20, doc.getId(), " ");
|
||||
em.clear();
|
||||
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
em.clear();
|
||||
|
||||
assertThat(journeyItemRepository.findById(emptyNoteItemId)).isEmpty();
|
||||
JourneyItem whitespaceItem = journeyItemRepository.findById(whitespaceNoteItemId).orElseThrow();
|
||||
assertThat(whitespaceItem.getDocumentId()).isNull();
|
||||
assertThat(whitespaceItem.getNote()).isEqualTo(" ");
|
||||
}
|
||||
|
||||
// ─── Idempotency / no-collateral ──────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void deleting_document_in_zero_journeys_returns_no_collateral() {
|
||||
Document unlinked = docRepo.save(Document.builder()
|
||||
.title("Unverknüpfter Brief")
|
||||
.originalFilename("other.pdf")
|
||||
.status(DocumentStatus.UPLOADED)
|
||||
.build());
|
||||
JourneyItem unrelated = journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey).position(10).note("unrelated note").build());
|
||||
em.clear();
|
||||
|
||||
documentService.deleteDocument(unlinked.getId(), writer.getId());
|
||||
em.clear();
|
||||
|
||||
assertThat(docRepo.findById(unlinked.getId())).isEmpty();
|
||||
assertThat(journeyItemRepository.findById(unrelated.getId())).isPresent();
|
||||
assertThat(journeyItemRepository.count()).isEqualTo(1);
|
||||
}
|
||||
|
||||
// ─── AC-7: audit — DOCUMENT_DELETED emitted, JOURNEY_ITEM_REMOVED absent ─
|
||||
|
||||
@Test
|
||||
void deleting_document_emits_document_audit_but_no_journey_item_audit() {
|
||||
journeyItemRepository.save(
|
||||
JourneyItem.builder().geschichte(journey).position(10).document(doc).build());
|
||||
em.clear();
|
||||
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
|
||||
verify(auditService).logAfterCommit(eq(AuditKind.DOCUMENT_DELETED), eq(writer.getId()), eq(doc.getId()), any());
|
||||
verify(auditService, never()).logAfterCommit(eq(AuditKind.JOURNEY_ITEM_REMOVED), any(), any(), any());
|
||||
}
|
||||
}
|
||||
@@ -6,8 +6,10 @@ import org.junit.jupiter.api.AfterEach;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.raddatz.familienarchiv.PostgresContainerConfig;
|
||||
import org.raddatz.familienarchiv.audit.AuditService;
|
||||
import org.raddatz.familienarchiv.document.Document;
|
||||
import org.raddatz.familienarchiv.document.DocumentRepository;
|
||||
import org.raddatz.familienarchiv.document.DocumentService;
|
||||
import org.raddatz.familienarchiv.document.DocumentStatus;
|
||||
import org.raddatz.familienarchiv.geschichte.Geschichte;
|
||||
import org.raddatz.familienarchiv.geschichte.GeschichteRepository;
|
||||
@@ -42,12 +44,16 @@ class JourneyItemIntegrationTest {
|
||||
@MockitoBean
|
||||
S3Client s3Client;
|
||||
|
||||
@MockitoBean
|
||||
AuditService auditService;
|
||||
|
||||
@PersistenceContext
|
||||
EntityManager em;
|
||||
|
||||
@Autowired GeschichteRepository geschichteRepository;
|
||||
@Autowired JourneyItemRepository journeyItemRepository;
|
||||
@Autowired JourneyItemService journeyItemService;
|
||||
@Autowired DocumentService documentService;
|
||||
@Autowired DocumentRepository documentRepository;
|
||||
@Autowired AppUserRepository appUserRepository;
|
||||
|
||||
@@ -212,8 +218,9 @@ class JourneyItemIntegrationTest {
|
||||
UUID itemId = saved.getItems().get(0).getId(); // extract before clear
|
||||
em.clear();
|
||||
|
||||
// Delete document — ON DELETE SET NULL fires at DB level
|
||||
documentRepository.deleteById(doc.getId());
|
||||
// Route through service so the DocumentDeletingEvent fires and the listener
|
||||
// removes note-less items before ON DELETE SET NULL acts on note-carrying rows.
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
em.flush();
|
||||
em.clear();
|
||||
|
||||
@@ -351,8 +358,8 @@ class JourneyItemIntegrationTest {
|
||||
em.flush();
|
||||
em.clear();
|
||||
|
||||
// ON DELETE SET NULL fires at DB level (V72)
|
||||
documentRepository.deleteById(doc.getId());
|
||||
// Route through service so the DocumentDeletingEvent fires (V72 cascade fix).
|
||||
documentService.deleteDocument(doc.getId(), writer.getId());
|
||||
em.flush();
|
||||
em.clear();
|
||||
|
||||
|
||||
118
docs/adr/038-domain-event-driven-journey-item-cleanup.md
Normal file
118
docs/adr/038-domain-event-driven-journey-item-cleanup.md
Normal file
@@ -0,0 +1,118 @@
|
||||
# ADR-038 — Domain event drives note-less journey-item cleanup on document delete
|
||||
|
||||
**Status:** Accepted
|
||||
**Date:** 2026-06-11
|
||||
**Issue:** #805 (P1 — deleting a document linked via a note-less journey_item 500s at DB constraint)
|
||||
|
||||
## Context
|
||||
|
||||
Two constraints in V72 encode contradictory rules for a journey item that has a
|
||||
`document_id` but no `note`:
|
||||
|
||||
- **`fk_journey_items_document` → `ON DELETE SET NULL`** — when a document is deleted,
|
||||
Postgres nulls out `document_id`.
|
||||
- **`chk_journey_item_not_empty`** — requires at least one of `document_id` or `note`
|
||||
to be non-null.
|
||||
|
||||
A note-less item (`document_id` set, `note IS NULL`) satisfies the CHECK while the
|
||||
document exists. Deleting the document causes Postgres to attempt `SET NULL`, which
|
||||
would leave both columns null — a direct CHECK violation. Postgres aborts the
|
||||
transaction with a 500 that bypasses `GlobalExceptionHandler`.
|
||||
|
||||
The natural fix — delete note-less items inside `DocumentService.deleteDocument` before
|
||||
`deleteById` runs — cannot call `JourneyItemService` directly: `JourneyItemService`
|
||||
already injects `DocumentService`, and Spring Framework 7 (used by Spring Boot 4)
|
||||
**fully prohibits constructor-injection cycles**. The application will not start if such
|
||||
a cycle is introduced.
|
||||
|
||||
## Decision
|
||||
|
||||
`DocumentService.deleteDocument` publishes a **`DocumentDeletingEvent`** (plain record,
|
||||
payload: `documentId` UUID only) via `ApplicationEventPublisher` **before**
|
||||
`documentRepository.deleteById`. A dedicated `@Component`
|
||||
`JourneyItemDocumentDeleteListener` in the `geschichte.journeyitem` package consumes
|
||||
this event and calls `journeyItemRepository.deleteNoteLessByDocumentId(documentId)`
|
||||
directly — bypassing `JourneyItemService` to avoid re-introducing the cycle and to
|
||||
suppress the per-item `JOURNEY_ITEM_REMOVED` audit emission (see audit decision below).
|
||||
|
||||
### Load-bearing listener-phase choice: plain `@EventListener`
|
||||
|
||||
The listener is annotated with `@EventListener` (not
|
||||
`@TransactionalEventListener(AFTER_COMMIT)`, not `@Async`). **This choice is
|
||||
load-bearing:**
|
||||
|
||||
- **`AFTER_COMMIT` would break the fix entirely.** `AFTER_COMMIT` fires *after* the
|
||||
surrounding transaction has committed. By that point, `documentRepository.deleteById`
|
||||
has already executed and Postgres has already tried `ON DELETE SET NULL` — the
|
||||
constraint violation fires before the listener ever runs.
|
||||
- **`@Async` would break rollback atomicity (AC-5).** An async listener runs on a
|
||||
separate thread in its own transaction. If `deleteDocument` subsequently rolls back
|
||||
(e.g. due to an unrelated failure), the listener's deletes are in a committed async
|
||||
transaction and cannot be undone.
|
||||
- **Plain `@EventListener` runs synchronously in the publisher's thread and
|
||||
transaction.** The listener's JPQL delete and the `deleteById` are a single atomic
|
||||
unit: if either fails, both roll back together.
|
||||
|
||||
### Repository method
|
||||
|
||||
```java
|
||||
@Modifying(clearAutomatically = true)
|
||||
@Query("DELETE FROM JourneyItem i WHERE i.document.id = :documentId AND (i.note IS NULL OR i.note = '')")
|
||||
int deleteNoteLessByDocumentId(@Param("documentId") UUID documentId);
|
||||
```
|
||||
|
||||
`i.document.id` (the real association path) is used instead of `i.documentId`: the
|
||||
transient `getDocumentId()` getter on `JourneyItem` makes Spring Data unable to resolve
|
||||
a derived query path (same trap documented at `JourneyItemRepository:33-44`).
|
||||
|
||||
`clearAutomatically = true` invalidates the L1 cache so subsequent reads in the same
|
||||
session do not return stale entities.
|
||||
|
||||
The predicate `(note IS NULL OR note = '')` covers the `note = ''` edge case that the
|
||||
service layer can never produce (normalizeNote converts blank strings to null), but that
|
||||
may exist via raw SQL inserts or legacy data. Whitespace-only notes (`note = ' '`)
|
||||
do not match and are preserved as note-carrying placeholders.
|
||||
|
||||
### Audit decision
|
||||
|
||||
The listener calls the repository directly rather than routing through
|
||||
`JourneyItemService.delete`. This deliberately bypasses the `JOURNEY_ITEM_REMOVED`
|
||||
audit emission: a document used in multiple journeys would otherwise produce N audit
|
||||
rows for a single user action. The `DOCUMENT_DELETED` entry written by `deleteDocument`
|
||||
is the sole audit record for the operation.
|
||||
|
||||
### Boundary: documents must not depend on journey
|
||||
|
||||
The event direction is `document → journey`, never the reverse. `DocumentService`
|
||||
publishes events it knows nothing about the consumers of; `JourneyItemService`'s
|
||||
dependency on `DocumentService` is unchanged and remains the only cross-domain
|
||||
reference. This direction is the prerequisite for the cycle constraint to hold.
|
||||
|
||||
## Alternatives rejected
|
||||
|
||||
- **DB trigger on `journey_items`** — trigger logic is opaque to Java developers,
|
||||
invisible to code review, and not covered by the JPA test harness.
|
||||
- **RESTRICT instead of SET NULL** — breaks the existing note-carrying placeholder
|
||||
UX: deleting a document with a note-carrying journey item would 409 instead of
|
||||
preserving the item as a placeholder.
|
||||
- **Relax `chk_journey_item_not_empty`** — the constraint enforces a real invariant
|
||||
(every item must have at least document or note). Removing it would allow empty rows.
|
||||
- **`@Lazy` on the `JourneyItemService → DocumentService` injection** — Spring Boot 4 /
|
||||
Spring Framework 7 prohibits constructor-injection cycles regardless of `@Lazy`.
|
||||
- **Make `DocumentService` call `JourneyItemService`** — introduces the prohibited
|
||||
cycle. Rejected at design time.
|
||||
|
||||
## Consequences
|
||||
|
||||
- **No schema change** — no new Flyway migration, no `db-orm.puml` /
|
||||
`db-relationships.puml` update.
|
||||
- This is the **first custom domain event** in the codebase. No prior
|
||||
`ApplicationEventPublisher` usage existed in `main/`. New cross-domain cleanup that
|
||||
cannot use direct service calls should follow this pattern.
|
||||
- All tests that delete documents and then assert journey-item state **must route
|
||||
through `DocumentService.deleteDocument`**, not `documentRepository.deleteById`.
|
||||
The existing `JourneyItemIntegrationTest` tests that covered the note-carrying
|
||||
placeholder UX have been updated accordingly.
|
||||
- The `DOCUMENT_DELETED` `AuditKind` was added as part of this fix to give AC-7's
|
||||
audit assertion a positive check (absence-only assertions pass vacuously if all
|
||||
auditing regresses).
|
||||
@@ -20,6 +20,7 @@ System_Boundary(backend, "API Backend (Spring Boot)") {
|
||||
Component(geschSvc, "GeschichteService", "Spring Service", "Manages story lifecycle (DRAFT → PUBLISHED with timestamp). Supports two subtypes: STORY (prose) and JOURNEY (ordered JourneyItem sequence). Sanitizes HTML body with an allowlist policy.")
|
||||
Component(geschQuerySvc, "GeschichteQueryService", "Spring Service", "Read-only facade over GeschichteRepository. Exposes existsById() and findById() to prevent JourneyItemService from crossing domain boundaries.")
|
||||
Component(journeyItemSvc, "JourneyItemService", "Spring Service", "Manages journey item lifecycle: append (100-item cap), updateNote (three-way PATCH), delete, and reorder (DEFERRABLE position swap). Serves both STORY and JOURNEY subtypes.")
|
||||
Component(journeyListener, "JourneyItemDocumentDeleteListener", "Spring @EventListener", "Consumes DocumentDeletingEvent synchronously inside the delete transaction and removes note-less journey items before ON DELETE SET NULL fires, preventing a chk_journey_item_not_empty violation. See ADR-038.")
|
||||
Component(exHandler, "GlobalExceptionHandler", "Spring @RestControllerAdvice", "Converts DomainException, validation errors, and generic exceptions to ErrorResponse JSON with machine-readable ErrorCode and HTTP status.")
|
||||
}
|
||||
|
||||
@@ -44,6 +45,8 @@ Rel(geschCtrl, journeyItemSvc, "Delegates journey item CRUD")
|
||||
Rel(journeyItemSvc, geschQuerySvc, "Checks Geschichte existence")
|
||||
Rel(geschQuerySvc, db, "Reads geschichten", "JDBC")
|
||||
Rel(journeyItemSvc, db, "Reads / writes journey_items", "JDBC")
|
||||
Rel(documentSvc, journeyListener, "DocumentDeletingEvent", "in-process event")
|
||||
Rel(journeyListener, db, "Deletes note-less journey_items", "JDBC")
|
||||
Rel(auditSvc, db, "Writes audit_log", "JDBC")
|
||||
Rel(auditQuery, db, "Reads audit_log", "JDBC")
|
||||
Rel(notifSvc, db, "Reads / writes notifications", "JDBC")
|
||||
|
||||
Reference in New Issue
Block a user