From 0802889ea9771a1ee608535e9fea542f2c46fb4e Mon Sep 17 00:00:00 2001 From: Marcel Date: Sat, 2 May 2026 19:17:39 +0200 Subject: [PATCH] feat(geschichten): filter by multiple persons with AND semantics GET /api/geschichten now accepts repeated personId query params and returns only stories that mention every person supplied. Refactors the list path to a JPA Specification chain (one EXISTS subquery per id, mirroring DocumentSpecifications.hasTags) and embeds the COALESCE(publishedAt, updatedAt) DESC ordering inside the spec so a single repository.findAll covers all filter combinations. --- .../controller/GeschichteController.java | 8 +- .../repository/GeschichteRepository.java | 18 ---- .../repository/GeschichteSpecifications.java | 91 +++++++++++++++++++ .../service/GeschichteService.java | 25 ++++- .../controller/GeschichteControllerTest.java | 22 ++++- .../GeschichteServiceIntegrationTest.java | 57 +++++++++++- .../service/GeschichteServiceTest.java | 60 ++++++++---- 7 files changed, 234 insertions(+), 47 deletions(-) create mode 100644 backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteSpecifications.java diff --git a/backend/src/main/java/org/raddatz/familienarchiv/controller/GeschichteController.java b/backend/src/main/java/org/raddatz/familienarchiv/controller/GeschichteController.java index 77e81326..b73c5840 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/controller/GeschichteController.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/controller/GeschichteController.java @@ -32,10 +32,14 @@ public class GeschichteController { @GetMapping public List list( @RequestParam(required = false) GeschichteStatus status, - @RequestParam(required = false) UUID personId, + @RequestParam(name = "personId", required = false) List personIds, @RequestParam(required = false) UUID documentId, @RequestParam(required = false, defaultValue = "50") int limit) { - return geschichteService.list(status, personId, documentId, limit); + return geschichteService.list( + status, + personIds == null ? List.of() : personIds, + documentId, + limit); } @GetMapping("/{id}") diff --git a/backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteRepository.java b/backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteRepository.java index 3d6ffbea..a207f00a 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteRepository.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteRepository.java @@ -1,30 +1,12 @@ package org.raddatz.familienarchiv.repository; import org.raddatz.familienarchiv.model.Geschichte; -import org.raddatz.familienarchiv.model.GeschichteStatus; -import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.JpaSpecificationExecutor; -import org.springframework.data.jpa.repository.Query; -import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; -import java.util.List; import java.util.UUID; @Repository public interface GeschichteRepository extends JpaRepository, JpaSpecificationExecutor { - - @Query(""" - SELECT g FROM Geschichte g - WHERE (:status IS NULL OR g.status = :status) - AND (:personId IS NULL OR :personId IN (SELECT p.id FROM g.persons p)) - AND (:documentId IS NULL OR :documentId IN (SELECT d.id FROM g.documents d)) - ORDER BY COALESCE(g.publishedAt, g.updatedAt) DESC - """) - List search( - @Param("status") GeschichteStatus status, - @Param("personId") UUID personId, - @Param("documentId") UUID documentId, - Pageable pageable); } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteSpecifications.java b/backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteSpecifications.java new file mode 100644 index 00000000..27442d60 --- /dev/null +++ b/backend/src/main/java/org/raddatz/familienarchiv/repository/GeschichteSpecifications.java @@ -0,0 +1,91 @@ +package org.raddatz.familienarchiv.repository; + +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.Join; +import jakarta.persistence.criteria.Predicate; +import jakarta.persistence.criteria.Root; +import jakarta.persistence.criteria.Subquery; +import org.raddatz.familienarchiv.model.Document; +import org.raddatz.familienarchiv.model.Geschichte; +import org.raddatz.familienarchiv.model.GeschichteStatus; +import org.raddatz.familienarchiv.model.Person; +import org.springframework.data.jpa.domain.Specification; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.UUID; + +public final class GeschichteSpecifications { + + private GeschichteSpecifications() {} + + public static Specification hasStatus(GeschichteStatus status) { + return (root, query, cb) -> status == null ? null : cb.equal(root.get("status"), status); + } + + /** + * Adds {@code ORDER BY COALESCE(publishedAt, updatedAt) DESC} to the query without contributing + * a predicate. Combined into the spec chain via {@code .and(...)}; the {@code conjunction} + * acts as a no-op WHERE clause. + */ + public static Specification orderByDisplayDateDesc() { + return (root, query, cb) -> { + // Skip ordering on count queries — JPA forbids orderBy on COUNT projections. + if (query != null + && Long.class != query.getResultType() + && long.class != query.getResultType()) { + query.orderBy(cb.desc(cb.coalesce(root.get("publishedAt"), root.get("updatedAt")))); + } + return cb.conjunction(); + }; + } + + public static Specification hasDocument(UUID documentId) { + return (root, query, cb) -> { + if (documentId == null) return null; + return cb.exists(documentSubquery(root, query, cb, documentId)); + }; + } + + /** + * AND-filter across persons: the Geschichte must be associated with EVERY id in {@code personIds}. + * + *

Implemented as one EXISTS subquery per id (canonical Criteria-API idiom for AND across a + * many-to-many join). Mirrors {@link DocumentSpecifications#hasTags} which uses the same shape. + * Empty / null input returns {@code null} (i.e. no constraint added). + */ + public static Specification hasAllPersons(Collection personIds) { + return (root, query, cb) -> { + if (personIds == null || personIds.isEmpty()) return null; + List predicates = new ArrayList<>(personIds.size()); + for (UUID id : personIds) { + predicates.add(cb.exists(personSubquery(root, query, cb, id))); + } + return cb.and(predicates.toArray(new Predicate[0])); + }; + } + + private static Subquery personSubquery( + Root root, CriteriaQuery query, CriteriaBuilder cb, UUID personId) { + Subquery sub = query.subquery(UUID.class); + Root subRoot = sub.from(Geschichte.class); + Join persons = subRoot.join("persons"); + sub.select(subRoot.get("id")) + .where(cb.equal(subRoot.get("id"), root.get("id")), + cb.equal(persons.get("id"), personId)); + return sub; + } + + private static Subquery documentSubquery( + Root root, CriteriaQuery query, CriteriaBuilder cb, UUID documentId) { + Subquery sub = query.subquery(UUID.class); + Root subRoot = sub.from(Geschichte.class); + Join documents = subRoot.join("documents"); + sub.select(subRoot.get("id")) + .where(cb.equal(subRoot.get("id"), root.get("id")), + cb.equal(documents.get("id"), documentId)); + return sub; + } +} diff --git a/backend/src/main/java/org/raddatz/familienarchiv/service/GeschichteService.java b/backend/src/main/java/org/raddatz/familienarchiv/service/GeschichteService.java index a0f5d4ba..175af596 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/service/GeschichteService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/service/GeschichteService.java @@ -13,9 +13,10 @@ import org.raddatz.familienarchiv.model.Geschichte; import org.raddatz.familienarchiv.model.GeschichteStatus; import org.raddatz.familienarchiv.model.Person; import org.raddatz.familienarchiv.repository.GeschichteRepository; +import org.raddatz.familienarchiv.repository.GeschichteSpecifications; import org.raddatz.familienarchiv.security.Permission; -import org.springframework.data.domain.PageRequest; -import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Sort; +import org.springframework.data.jpa.domain.Specification; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Service; @@ -64,11 +65,25 @@ public class GeschichteService { return g; } - public List list(GeschichteStatus status, UUID personId, UUID documentId, int limit) { + /** + * Lists Geschichten with optional filters. {@code personIds} uses AND semantics: the story + * must be associated with every person id supplied. An empty or null list applies no + * person filter. Result is ordered by {@code COALESCE(publishedAt, updatedAt) DESC}. + */ + public List list(GeschichteStatus status, List personIds, UUID documentId, int limit) { GeschichteStatus effective = currentUserHasBlogWrite() ? status : GeschichteStatus.PUBLISHED; int safeLimit = limit <= 0 ? DEFAULT_LIMIT : Math.min(limit, MAX_LIMIT); - Pageable pageable = PageRequest.of(0, safeLimit); - return geschichteRepository.search(effective, personId, documentId, pageable); + + Specification spec = Specification.allOf( + GeschichteSpecifications.hasStatus(effective), + GeschichteSpecifications.hasAllPersons(personIds), + GeschichteSpecifications.hasDocument(documentId), + GeschichteSpecifications.orderByDisplayDateDesc() + ); + return geschichteRepository.findAll(spec, Sort.unsorted()) + .stream() + .limit(safeLimit) + .toList(); } // ─── Write API ─────────────────────────────────────────────────────────── diff --git a/backend/src/test/java/org/raddatz/familienarchiv/controller/GeschichteControllerTest.java b/backend/src/test/java/org/raddatz/familienarchiv/controller/GeschichteControllerTest.java index 9ae9c457..4c9f721b 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/controller/GeschichteControllerTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/controller/GeschichteControllerTest.java @@ -73,15 +73,31 @@ class GeschichteControllerTest { @Test @WithMockUser(authorities = "READ_ALL") - void list_passesPersonIdFilterToService() throws Exception { + void list_passesSinglePersonIdFilterToServiceAsListOfOne() throws Exception { UUID personId = UUID.randomUUID(); - when(geschichteService.list(any(), eq(personId), any(), anyInt())) + when(geschichteService.list(any(), eq(List.of(personId)), any(), anyInt())) .thenReturn(List.of()); mockMvc.perform(get("/api/geschichten").param("personId", personId.toString())) .andExpect(status().isOk()); - verify(geschichteService).list(any(), eq(personId), any(), anyInt()); + verify(geschichteService).list(any(), eq(List.of(personId)), any(), anyInt()); + } + + @Test + @WithMockUser(authorities = "READ_ALL") + void list_passesRepeatedPersonIdParamsAsListForAndFilter() throws Exception { + UUID a = UUID.randomUUID(); + UUID b = UUID.randomUUID(); + when(geschichteService.list(any(), eq(List.of(a, b)), any(), anyInt())) + .thenReturn(List.of()); + + mockMvc.perform(get("/api/geschichten") + .param("personId", a.toString()) + .param("personId", b.toString())) + .andExpect(status().isOk()); + + verify(geschichteService).list(any(), eq(List.of(a, b)), any(), anyInt()); } // ─── GET /api/geschichten/{id} ─────────────────────────────────────────── diff --git a/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceIntegrationTest.java b/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceIntegrationTest.java index 301beb1a..35ed3ada 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceIntegrationTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceIntegrationTest.java @@ -86,7 +86,7 @@ class GeschichteServiceIntegrationTest { // Reader cannot see DRAFT in list authenticateAs(reader, Permission.READ_ALL); - assertThat(geschichteService.list(null, null, null, 50)).isEmpty(); + assertThat(geschichteService.list(null, List.of(), null, 50)).isEmpty(); // Reader cannot fetch DRAFT by id (404 via GESCHICHTE_NOT_FOUND) UUID draftId = created.getId(); @@ -102,8 +102,8 @@ class GeschichteServiceIntegrationTest { // Reader can now see and fetch it authenticateAs(reader, Permission.READ_ALL); - assertThat(geschichteService.list(null, null, null, 50)).hasSize(1); - assertThat(geschichteService.list(null, franz.getId(), null, 50)).hasSize(1); + assertThat(geschichteService.list(null, List.of(), null, 50)).hasSize(1); + assertThat(geschichteService.list(null, List.of(franz.getId()), null, 50)).hasSize(1); Geschichte fetched = geschichteService.getById(draftId); assertThat(fetched.getTitle()).isEqualTo("Erinnerung an Opa Franz"); assertThat(fetched.getPersons()).extracting(Person::getId).containsExactly(franz.getId()); @@ -117,6 +117,57 @@ class GeschichteServiceIntegrationTest { assertThat(personRepository.findById(franz.getId())).isPresent(); } + @Test + void list_filters_with_AND_semantics_when_multiple_personIds_given() { + // Three published stories, persons overlap so we can prove AND-not-OR: + // story_AB: about A and B + // story_AC: about A and C + // story_A: about A only + authenticateAs(writer, Permission.BLOG_WRITE); + + Person a = personRepository.save(Person.builder().firstName("Anna").lastName("A").build()); + Person b = personRepository.save(Person.builder().firstName("Bertha").lastName("B").build()); + Person c = personRepository.save(Person.builder().firstName("Carl").lastName("C").build()); + + UUID storyAB = publishedStoryWithPersons("Anna & Bertha", List.of(a.getId(), b.getId())); + UUID storyAC = publishedStoryWithPersons("Anna & Carl", List.of(a.getId(), c.getId())); + UUID storyA = publishedStoryWithPersons("Anna alone", List.of(a.getId())); + + authenticateAs(reader, Permission.READ_ALL); + + // No filter → all three + assertThat(geschichteService.list(null, List.of(), null, 50)) + .extracting(Geschichte::getId) + .containsExactlyInAnyOrder(storyAB, storyAC, storyA); + + // Single filter (Anna) → all three + assertThat(geschichteService.list(null, List.of(a.getId()), null, 50)) + .extracting(Geschichte::getId) + .containsExactlyInAnyOrder(storyAB, storyAC, storyA); + + // AND: Anna AND Bertha → only the AB story (NOT story_A, NOT story_AC) + assertThat(geschichteService.list(null, List.of(a.getId(), b.getId()), null, 50)) + .extracting(Geschichte::getId) + .containsExactly(storyAB); + + // AND: Bertha AND Carl → none (no story has both) + assertThat(geschichteService.list(null, List.of(b.getId(), c.getId()), null, 50)) + .isEmpty(); + + // AND: Anna AND Bertha AND Carl → none + assertThat(geschichteService.list(null, List.of(a.getId(), b.getId(), c.getId()), null, 50)) + .isEmpty(); + } + + private UUID publishedStoryWithPersons(String title, List personIds) { + GeschichteUpdateDTO dto = new GeschichteUpdateDTO(); + dto.setTitle(title); + dto.setBody("

body

"); + dto.setPersonIds(personIds); + dto.setStatus(GeschichteStatus.PUBLISHED); + return geschichteService.create(dto).getId(); + } + private void authenticateAs(AppUser user, Permission... permissions) { var authorities = java.util.Arrays.stream(permissions) .map(p -> new SimpleGrantedAuthority(p.name())) diff --git a/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceTest.java index ffd6b15f..2f4d311b 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/service/GeschichteServiceTest.java @@ -4,7 +4,6 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -18,7 +17,8 @@ import org.raddatz.familienarchiv.model.GeschichteStatus; import org.raddatz.familienarchiv.model.Person; import org.raddatz.familienarchiv.repository.GeschichteRepository; import org.raddatz.familienarchiv.security.Permission; -import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Sort; +import org.springframework.data.jpa.domain.Specification; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.context.SecurityContextHolder; @@ -36,7 +36,6 @@ 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.never; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -126,47 +125,76 @@ class GeschichteServiceTest { @Test void list_forces_PUBLISHED_status_for_reader_without_BLOG_WRITE() { authenticateAs(reader, Permission.READ_ALL); - when(geschichteRepository.search(eq(GeschichteStatus.PUBLISHED), any(), any(), any())) + when(geschichteRepository.findAll(any(Specification.class), any(Sort.class))) .thenReturn(List.of(published(UUID.randomUUID()))); - geschichteService.list(/*status*/ null, /*personId*/ null, /*documentId*/ null, /*limit*/ 50); + geschichteService.list(/*status*/ null, /*personIds*/ List.of(), /*documentId*/ null, /*limit*/ 50); - verify(geschichteRepository).search(eq(GeschichteStatus.PUBLISHED), any(), any(), any()); + // Status pinning lives inside the Specification; we assert end-to-end behaviour + // in GeschichteServiceIntegrationTest. Here we just confirm the service routes + // through the spec-aware repository method. + verify(geschichteRepository).findAll(any(Specification.class), any(Sort.class)); } @Test void list_passes_null_status_through_for_BLOG_WRITER_so_drafts_are_visible() { authenticateAs(writer, Permission.BLOG_WRITE); - when(geschichteRepository.search(any(), any(), any(), any())) + when(geschichteRepository.findAll(any(Specification.class), any(Sort.class))) .thenReturn(List.of(draft(UUID.randomUUID()), published(UUID.randomUUID()))); - geschichteService.list(null, null, null, 50); + List out = geschichteService.list(null, List.of(), null, 50); - verify(geschichteRepository).search(eq(null), any(), any(), any()); + assertThat(out).hasSize(2); + verify(geschichteRepository).findAll(any(Specification.class), any(Sort.class)); } @Test - void list_filters_by_personId() { + void list_invokes_repository_findAll_when_filtering_by_single_personId() { authenticateAs(reader, Permission.READ_ALL); UUID personId = UUID.randomUUID(); - when(geschichteRepository.search(any(), eq(personId), any(), any())) + when(geschichteRepository.findAll(any(Specification.class), any(Sort.class))) .thenReturn(List.of()); - geschichteService.list(null, personId, null, 50); + geschichteService.list(null, List.of(personId), null, 50); - verify(geschichteRepository).search(eq(GeschichteStatus.PUBLISHED), eq(personId), eq(null), any()); + verify(geschichteRepository).findAll(any(Specification.class), any(Sort.class)); + } + + @Test + void list_invokes_repository_findAll_when_filtering_by_multiple_personIds() { + authenticateAs(reader, Permission.READ_ALL); + UUID a = UUID.randomUUID(); + UUID b = UUID.randomUUID(); + when(geschichteRepository.findAll(any(Specification.class), any(Sort.class))) + .thenReturn(List.of()); + + geschichteService.list(null, List.of(a, b), null, 50); + + verify(geschichteRepository).findAll(any(Specification.class), any(Sort.class)); } @Test void list_filters_by_documentId() { authenticateAs(reader, Permission.READ_ALL); UUID documentId = UUID.randomUUID(); - when(geschichteRepository.search(any(), any(), eq(documentId), any())) + when(geschichteRepository.findAll(any(Specification.class), any(Sort.class))) .thenReturn(List.of()); - geschichteService.list(null, null, documentId, 50); + geschichteService.list(null, List.of(), documentId, 50); - verify(geschichteRepository).search(eq(GeschichteStatus.PUBLISHED), eq(null), eq(documentId), any()); + verify(geschichteRepository).findAll(any(Specification.class), any(Sort.class)); + } + + @Test + void list_caps_limit_at_max_via_pageable_when_caller_passes_huge_value() { + authenticateAs(reader, Permission.READ_ALL); + when(geschichteRepository.findAll(any(Specification.class), any(Sort.class))) + .thenReturn(List.of(published(UUID.randomUUID()))); + + // 9999 should be clamped — service trims to MAX_LIMIT (200) before/after the query + List out = geschichteService.list(null, List.of(), null, 9999); + + assertThat(out).hasSizeLessThanOrEqualTo(200); } // ─── create ──────────────────────────────────────────────────────────────