From d7a46de1cc175fdd4b4befb300cc641a678a321e Mon Sep 17 00:00:00 2001 From: Marcel Date: Fri, 17 Apr 2026 00:24:04 +0200 Subject: [PATCH] =?UTF-8?q?refactor(#248):=20address=20PR=20review=20conce?= =?UTF-8?q?rns=20=E2=80=94=20TagOperator=20enum,=20typed=20projection,=20b?= =?UTF-8?q?ean=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace stringly-typed "AND"/"OR" tagOperator with TagOperator enum (DocumentService, DocumentController) - Replace Object[] with TagCount projection interface in TagRepository.findDocumentCountsPerTag() - Use @NotNull + @Valid on MergeTagDTO.targetId; remove manual null check from TagController - Correct ALLOWED_TAG_COLORS to match actual frontend CSS tokens (sage/sienna/amber/slate/violet/rose/cobalt/moss/sand/coral) - Add TOCTOU comment to validateNoAncestorCycle() with mitigation explanation - Add test: deleteWithDescendants_skipsDocTagDeletion_whenDescendantIdsIsEmpty - Update TagServiceTest to use mock TagRepository.TagCount projection Co-Authored-By: Claude Sonnet 4.6 --- .../controller/DocumentController.java | 4 +++- .../controller/TagController.java | 6 ++---- .../familienarchiv/dto/MergeTagDTO.java | 3 ++- .../familienarchiv/dto/TagOperator.java | 9 +++++++++ .../repository/TagRepository.java | 13 ++++++++++--- .../service/DocumentService.java | 5 +++-- .../familienarchiv/service/TagService.java | 17 +++++++++++------ .../service/TagServiceTest.java | 19 ++++++++++++++++++- 8 files changed, 58 insertions(+), 18 deletions(-) create mode 100644 backend/src/main/java/org/raddatz/familienarchiv/dto/TagOperator.java diff --git a/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java b/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java index 6f47ea01..22671407 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java @@ -15,6 +15,7 @@ import io.swagger.v3.oas.annotations.Parameter; import io.swagger.v3.oas.annotations.responses.ApiResponse; import org.raddatz.familienarchiv.dto.DocumentSearchResult; import org.raddatz.familienarchiv.dto.DocumentUpdateDTO; +import org.raddatz.familienarchiv.dto.TagOperator; import org.raddatz.familienarchiv.dto.DocumentVersionSummary; import org.raddatz.familienarchiv.dto.IncompleteDocumentDTO; import org.raddatz.familienarchiv.exception.DomainException; @@ -209,7 +210,8 @@ public class DocumentController { if (!"ASC".equalsIgnoreCase(dir) && !"DESC".equalsIgnoreCase(dir)) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "dir must be ASC or DESC"); } - return ResponseEntity.ok(documentService.searchDocuments(q, from, to, senderId, receiverId, tags, tagQ, status, sort, dir, tagOp)); + TagOperator operator = "OR".equalsIgnoreCase(tagOp) ? TagOperator.OR : TagOperator.AND; + return ResponseEntity.ok(documentService.searchDocuments(q, from, to, senderId, receiverId, tags, tagQ, status, sort, dir, operator)); } // --- TRAINING LABELS --- diff --git a/backend/src/main/java/org/raddatz/familienarchiv/controller/TagController.java b/backend/src/main/java/org/raddatz/familienarchiv/controller/TagController.java index 46ab77f8..39f66981 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/controller/TagController.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/controller/TagController.java @@ -23,7 +23,7 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestController; -import org.springframework.web.server.ResponseStatusException; +import jakarta.validation.Valid; import lombok.RequiredArgsConstructor; @@ -60,9 +60,7 @@ public class TagController { @PostMapping("/{id}/merge") @RequirePermission(Permission.ADMIN_TAG) - public ResponseEntity mergeTag(@PathVariable UUID id, @RequestBody MergeTagDTO dto) { - if (dto.targetId() == null) - throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "targetId required"); + public ResponseEntity mergeTag(@PathVariable UUID id, @Valid @RequestBody MergeTagDTO dto) { return ResponseEntity.ok(tagService.mergeTags(id, dto.targetId())); } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/dto/MergeTagDTO.java b/backend/src/main/java/org/raddatz/familienarchiv/dto/MergeTagDTO.java index 6ccac157..be57f404 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/dto/MergeTagDTO.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/dto/MergeTagDTO.java @@ -1,5 +1,6 @@ package org.raddatz.familienarchiv.dto; +import jakarta.validation.constraints.NotNull; import java.util.UUID; -public record MergeTagDTO(UUID targetId) {} +public record MergeTagDTO(@NotNull UUID targetId) {} diff --git a/backend/src/main/java/org/raddatz/familienarchiv/dto/TagOperator.java b/backend/src/main/java/org/raddatz/familienarchiv/dto/TagOperator.java new file mode 100644 index 00000000..c3e0f290 --- /dev/null +++ b/backend/src/main/java/org/raddatz/familienarchiv/dto/TagOperator.java @@ -0,0 +1,9 @@ +package org.raddatz.familienarchiv.dto; + +/** Determines how multiple selected tag filters are combined in a document search. */ +public enum TagOperator { + /** Every tag set must match (default). */ + AND, + /** At least one tag set must match. */ + OR +} diff --git a/backend/src/main/java/org/raddatz/familienarchiv/repository/TagRepository.java b/backend/src/main/java/org/raddatz/familienarchiv/repository/TagRepository.java index c3e7bbef..fcc2dffd 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/repository/TagRepository.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/repository/TagRepository.java @@ -13,6 +13,13 @@ import org.springframework.data.repository.query.Param; public interface TagRepository extends JpaRepository { + /** Typed projection for document-count aggregation results. */ + interface TagCount { + UUID getTagId(); + Long getCount(); + } + + Optional findByNameIgnoreCase(String name); List findByNameContainingIgnoreCase(String name); @@ -111,9 +118,9 @@ public interface TagRepository extends JpaRepository { void reparentChildren(@Param("sourceId") UUID sourceId, @Param("targetId") UUID targetId); /** - * Returns (tag_id, count) pairs for all tags that appear in document_tags. + * Returns (tagId, count) pairs for all tags that appear in document_tags. * Used to populate documentCount in the tag tree without N+1 queries. */ - @Query(value = "SELECT tag_id, COUNT(*) FROM document_tags GROUP BY tag_id", nativeQuery = true) - List findDocumentCountsPerTag(); + @Query(value = "SELECT tag_id AS tagId, COUNT(*) AS count FROM document_tags GROUP BY tag_id", nativeQuery = true) + List findDocumentCountsPerTag(); } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java b/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java index 7826153d..2f074e48 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java @@ -9,6 +9,7 @@ import org.raddatz.familienarchiv.dto.DocumentUpdateDTO; import org.raddatz.familienarchiv.dto.IncompleteDocumentDTO; import org.raddatz.familienarchiv.dto.MatchOffset; import org.raddatz.familienarchiv.dto.SearchMatchData; +import org.raddatz.familienarchiv.dto.TagOperator; import org.raddatz.familienarchiv.model.Document; import org.raddatz.familienarchiv.model.DocumentStatus; import org.raddatz.familienarchiv.model.ScriptType; @@ -293,7 +294,7 @@ public class DocumentService { } // 1. Allgemeine Suche (für das Suchfeld im Frontend) - public DocumentSearchResult searchDocuments(String text, LocalDate from, LocalDate to, UUID sender, UUID receiver, List tags, String tagQ, DocumentStatus status, DocumentSort sort, String dir, String tagOperator) { + public DocumentSearchResult searchDocuments(String text, LocalDate from, LocalDate to, UUID sender, UUID receiver, List tags, String tagQ, DocumentStatus status, DocumentSort sort, String dir, TagOperator tagOperator) { boolean hasText = StringUtils.hasText(text); List rankedIds = null; @@ -302,7 +303,7 @@ public class DocumentService { if (rankedIds.isEmpty()) return DocumentSearchResult.withMatchData(List.of(), Map.of()); } - boolean useOrLogic = "OR".equalsIgnoreCase(tagOperator); + boolean useOrLogic = tagOperator == TagOperator.OR; List> expandedTagSets = tagService.expandTagNamesToDescendantIdSets(tags); Specification textSpec = hasText ? hasIds(rankedIds) : (root, query, cb) -> null; diff --git a/backend/src/main/java/org/raddatz/familienarchiv/service/TagService.java b/backend/src/main/java/org/raddatz/familienarchiv/service/TagService.java index 4fe3112c..ff29b72c 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/service/TagService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/service/TagService.java @@ -28,9 +28,11 @@ import lombok.extern.slf4j.Slf4j; @Slf4j public class TagService { + // These 10 color tokens are the fixed palette. + // Keep in sync with the --c-tag-* tokens defined in frontend/src/routes/layout.css. static final Set ALLOWED_TAG_COLORS = Set.of( - "navy", "teal", "ocean", "forest", "sage", - "sienna", "terracotta", "ochre", "rose", "violet" + "sage", "sienna", "amber", "slate", "violet", + "rose", "cobalt", "moss", "sand", "coral" ); private final TagRepository tagRepository; @@ -148,8 +150,8 @@ public class TagService { List all = tagRepository.findAll(); Map counts = tagRepository.findDocumentCountsPerTag().stream() .collect(Collectors.toMap( - row -> (UUID) row[0], - row -> (Long) row[1] + TagRepository.TagCount::getTagId, + TagRepository.TagCount::getCount )); return buildTree(all, counts); } @@ -184,8 +186,11 @@ public class TagService { } private void validateNoAncestorCycle(UUID tagId, UUID proposedParentId) { - // TOCTOU: concurrent writes could both pass this check; the self-reference - // CHECK constraint on the DB column is the hard backstop for the self-loop case. + // TOCTOU note: concurrent admin writes could both pass this check and create a + // multi-node cycle. This is intentionally not locked because: (a) the endpoint + // requires ADMIN_TAG permission so concurrency is rare, (b) the DB-level + // CHECK (parent_id != id) prevents infinite self-loops as a hard backstop, + // and (c) the window is microseconds. Do NOT add a pessimistic lock here. List ancestors = tagRepository.findAncestorIds(proposedParentId); if (ancestors.contains(tagId)) { throw DomainException.badRequest(ErrorCode.TAG_CYCLE_DETECTED, diff --git a/backend/src/test/java/org/raddatz/familienarchiv/service/TagServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/service/TagServiceTest.java index 14540abc..f4c564c8 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/service/TagServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/service/TagServiceTest.java @@ -18,6 +18,7 @@ import java.util.UUID; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; @@ -256,8 +257,11 @@ class TagServiceTest { void getTagTree_populatesDocumentCount_fromAggregateQuery() { UUID tagId = UUID.randomUUID(); Tag tag = Tag.builder().id(tagId).name("Tag").build(); + TagRepository.TagCount countEntry = mock(TagRepository.TagCount.class); + when(countEntry.getTagId()).thenReturn(tagId); + when(countEntry.getCount()).thenReturn(5L); when(tagRepository.findAll()).thenReturn(List.of(tag)); - when(tagRepository.findDocumentCountsPerTag()).thenReturn(List.of(new Object[]{tagId, 5L})); + when(tagRepository.findDocumentCountsPerTag()).thenReturn(List.of(countEntry)); var tree = tagService.getTagTree(); @@ -456,6 +460,19 @@ class TagServiceTest { verify(tagRepository).deleteAllById(List.of(id)); } + @Test + void deleteWithDescendants_skipsDocTagDeletion_whenDescendantIdsIsEmpty() { + UUID id = UUID.randomUUID(); + Tag tag = Tag.builder().id(id).name("Tag").build(); + when(tagRepository.findById(id)).thenReturn(Optional.of(tag)); + when(tagRepository.findDescendantIds(id)).thenReturn(List.of()); + + tagService.deleteWithDescendants(id); + + verify(tagRepository, never()).deleteDocumentTagsByTagIds(any()); + verify(tagRepository).deleteAllById(List.of()); + } + // ─── delete ─────────────────────────────────────────────────────────────── @Test