From 7342c60952b28fe1afe19406d287ffbc78311000 Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 19 May 2026 07:42:59 +0200 Subject: [PATCH] fix(document): fix test assertion structure + add entity graph decision comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Refactor DocumentLazyLoadingTest: pull value assertions (assertThat) out of assertThatCode lambdas so failures surface as AssertionError rather than "unexpected exception: AssertionError" (review item 1) - Add @EntityGraph("Document.full") to findBySenderId, findByReceiversId, findConversation, and findSinglePersonCorrespondence — all return full Documents to the controller for JSON serialization (review item 2) - Add "// Callers access only ..." comments to un-graphed methods where no lazy associations are touched: findByTags_Id, findByStatus, findByMetadataCompleteFalse(Sort), findByMetadataCompleteFalse(Pageable) - Remove "what" inline comments from @Transactional(readOnly=true) on getRecentActivity and getDocumentById — the why is in ADR-022 (item 4) - Add named-graph coupling consequence to ADR-022: Document.java and DocumentRepository.java graph name strings must stay in sync (item 5) Co-Authored-By: Claude Sonnet 4.6 --- .../document/DocumentRepository.java | 10 ++++++++-- .../document/DocumentService.java | 4 ---- .../document/DocumentLazyLoadingTest.java | 19 +++++++++++++------ docs/adr/022-eager-to-lazy-fetch-strategy.md | 6 ++++++ 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentRepository.java b/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentRepository.java index c8a0c255..e887eeb8 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentRepository.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentRepository.java @@ -44,17 +44,19 @@ public interface DocumentRepository extends JpaRepository, JpaSp // Wie oben, gibt aber nur das erste Ergebnis zurück — sicher wenn doppelte Dateinamen existieren Optional findFirstByOriginalFilename(String originalFilename); - // Findet alle Dokumente mit einem bestimmten Status - // z.B. um alle offenen "PLACEHOLDER" zu finden + // Callers access only status/id scalar fields — no graph needed. List findByStatus(DocumentStatus status); // Prüft effizient, ob ein Dateiname schon existiert (gibt true/false zurück) boolean existsByOriginalFilename(String originalFilename); + @EntityGraph("Document.full") List findBySenderId(UUID senderId); + @EntityGraph("Document.full") List findByReceiversId(UUID receiverId); + // Callers access only doc.getTags() to mutate the set — receivers/sender not touched; no graph needed. List findByTags_Id(UUID tagId); @Query("SELECT d FROM Document d WHERE d.id NOT IN (SELECT DISTINCT dv.documentId FROM DocumentVersion dv)") @@ -69,12 +71,15 @@ public interface DocumentRepository extends JpaRepository, JpaSp long countByMetadataCompleteFalse(); + // No production callers — only used if a future export path iterates the full list; no graph needed. List findByMetadataCompleteFalse(Sort sort); + // Callers map to IncompleteDocumentDTO using only scalar fields (id, title, createdAt) — no graph needed. Page findByMetadataCompleteFalse(Pageable pageable); Optional findFirstByMetadataCompleteFalseAndIdNot(UUID id, Sort sort); + @EntityGraph("Document.full") @Query("SELECT DISTINCT d FROM Document d " + "JOIN d.receivers r " + "WHERE " + @@ -89,6 +94,7 @@ public interface DocumentRepository extends JpaRepository, JpaSp @Param("to") LocalDate to, Sort sort); + @EntityGraph("Document.full") @Query("SELECT DISTINCT d FROM Document d " + "LEFT JOIN d.receivers r " + "WHERE (d.sender.id = :personId OR r.id = :personId) " + diff --git a/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java b/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java index e4bd434d..a739764e 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java @@ -636,8 +636,6 @@ public class DocumentService { return saved; } - // @Transactional(readOnly=true) keeps the Hibernate session open so the - // lazy-loaded sender and tags on returned documents remain accessible to callers. @Transactional(readOnly = true) public List getRecentActivity(int size) { return documentRepository.findAll( @@ -846,8 +844,6 @@ public class DocumentService { documentRepository.save(doc); } - // @Transactional(readOnly=true) keeps the Hibernate session open so the - // lazy-loaded tags and receivers on the returned document remain accessible to callers. @Transactional(readOnly = true) public Document getDocumentById(UUID id) { Document doc = documentRepository.findById(id) diff --git a/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentLazyLoadingTest.java b/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentLazyLoadingTest.java index 09391662..3db5caaa 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentLazyLoadingTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentLazyLoadingTest.java @@ -70,12 +70,16 @@ class DocumentLazyLoadingTest { Document result = documentService.getDocumentById(doc.getId()); + // Only the collection access itself is in assertThatCode — guards against LazyInitializationException. + // Value assertions live outside so failures surface as AssertionError, not as unexpected exception. assertThatCode(() -> { - assertThat(result.getTags()).isNotEmpty(); - result.getTags().forEach(t -> assertThat(t.getName()).isNotNull()); - assertThat(result.getReceivers()).isNotEmpty(); - result.getReceivers().forEach(r -> assertThat(r.getLastName()).isNotNull()); + result.getTags().size(); + result.getReceivers().size(); }).doesNotThrowAnyException(); + assertThat(result.getTags()).isNotEmpty(); + result.getTags().forEach(t -> assertThat(t.getName()).isNotNull()); + assertThat(result.getReceivers()).isNotEmpty(); + result.getReceivers().forEach(r -> assertThat(r.getLastName()).isNotNull()); } @Test @@ -88,11 +92,14 @@ class DocumentLazyLoadingTest { List results = documentService.getRecentActivity(3); + // Access lazy fields inside assertThatCode — guards against LazyInitializationException. + // Value assertions live outside so failures surface as AssertionError, not as unexpected exception. assertThatCode(() -> { - results.forEach(d -> assertThat(d.getSender()).isNotNull()); - results.forEach(d -> assertThat(d.getSender().getLastName()).isNotNull()); + results.forEach(d -> d.getSender().getLastName()); results.forEach(d -> d.getTags().size()); }).doesNotThrowAnyException(); + results.forEach(d -> assertThat(d.getSender()).isNotNull()); + results.forEach(d -> assertThat(d.getSender().getLastName()).isNotNull()); } @Test diff --git a/docs/adr/022-eager-to-lazy-fetch-strategy.md b/docs/adr/022-eager-to-lazy-fetch-strategy.md index 88e6bc7c..d876c1b4 100644 --- a/docs/adr/022-eager-to-lazy-fetch-strategy.md +++ b/docs/adr/022-eager-to-lazy-fetch-strategy.md @@ -102,3 +102,9 @@ open long enough for the caller to use the result." `Person` and `Tag` carry this annotation to prevent Jackson from attempting to serialize Hibernate proxy internals when the association is not initialized. Any new entity that is used as a lazy association and serialized directly (without a DTO) needs the same annotation. +- **Named graph strings in `Document.java` and `DocumentRepository.java` must stay in sync.** + The `@NamedEntityGraph(name = "Document.full")` / `@NamedEntityGraph(name = "Document.list")` + definitions on `Document` are referenced by string in every `@EntityGraph(value = "...")` on + `DocumentRepository`. If the names diverge (e.g. a graph is renamed in one place but not the + other), Spring Data throws at application startup. Always update both files together when + renaming or restructuring a named graph.