From 6d2b6f3d2b38465a5e31295afe76b79b2e148df9 Mon Sep 17 00:00:00 2001 From: Marcel Date: Sun, 14 Jun 2026 18:36:43 +0200 Subject: [PATCH] feat(relationship): add PUT update endpoint, align DELETE mismatch to 404 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PUT /api/persons/{id}/relationships/{relId} (@RequirePermission WRITE_ALL) updates a relationship's type, related person, dates and notes in place, preserving the original createdAt. The update re-runs every create invariant — self-relation (VALIDATION_ERROR), date coherence/order, reverse PARENT_OF (CIRCULAR_RELATIONSHIP) and the (person, relatedPerson, type) unique constraint via saveAndFlush (DUPLICATE_RELATIONSHIP) — and re-flags both endpoints as family members when edited into a family type. The directed orientation is preserved per viewpoint, so a PARENT_OF edge stays parent->child whether edited from either person's page. Ownership mismatch now returns 404 RELATIONSHIP_NOT_FOUND (shared loadOwned helper) for both PUT and DELETE — anti-enumeration, replacing DELETE's former 403. No @Version: last-write-wins, matching person edit (single-writer archive). REQ-004, REQ-005, REQ-006, REQ-007, REQ-008, REQ-009, REQ-012, REQ-013, REQ-018 Refs #837 Co-Authored-By: Claude Opus 4.8 --- .../relationship/RelationshipController.java | 9 ++ .../relationship/RelationshipService.java | 68 ++++++++- .../RelationshipControllerTest.java | 49 ++++++ .../RelationshipServiceIntegrationTest.java | 36 ++++- .../relationship/RelationshipServiceTest.java | 141 +++++++++++++++++- 5 files changed, 291 insertions(+), 12 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipController.java b/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipController.java index 2b2de737..7d83966c 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipController.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipController.java @@ -68,6 +68,15 @@ public class RelationshipController { .body(relationshipService.addRelationship(id, dto)); } + @PutMapping("/api/persons/{id}/relationships/{relId}") + @RequirePermission(Permission.WRITE_ALL) + public RelationshipDTO updateRelationship( + @PathVariable UUID id, + @PathVariable UUID relId, + @Valid @RequestBody RelationshipUpsertRequest dto) { + return relationshipService.updateRelationship(id, relId, dto); + } + @DeleteMapping("/api/persons/{id}/relationships/{relId}") @ResponseStatus(HttpStatus.NO_CONTENT) @RequirePermission(Permission.WRITE_ALL) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipService.java b/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipService.java index 0265bfe1..c466bb92 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/person/relationship/RelationshipService.java @@ -145,19 +145,81 @@ public class RelationshipService { return toDTO(saved); } + @Transactional + public RelationshipDTO updateRelationship(UUID personId, UUID relId, RelationshipUpsertRequest dto) { + PersonRelationship rel = loadOwnedRelationship(personId, relId); + + // The other party from {personId}'s viewpoint cannot be {personId} itself. + if (personId.equals(dto.relatedPersonId())) { + throw DomainException.badRequest( + ErrorCode.VALIDATION_ERROR, "Cannot relate a person to themselves"); + } + validateRelationshipDates(dto.fromDate(), dto.fromDatePrecision(), dto.toDate(), dto.toDatePrecision()); + + // Preserve the directed orientation: {personId} keeps whichever role (subject or + // object) it already holds on the row, and the edited "related person" takes the + // other role. So a PARENT_OF edge stays parent→child whether the curator edits it + // from the parent's page or the child's. + Person viewpoint = personId.equals(rel.getPerson().getId()) ? rel.getPerson() : rel.getRelatedPerson(); + Person other = personService.getById(dto.relatedPersonId()); + boolean viewpointIsSubject = personId.equals(rel.getPerson().getId()); + Person newSubject = viewpointIsSubject ? viewpoint : other; + Person newObject = viewpointIsSubject ? other : viewpoint; + + if (dto.relationType() == RelationType.PARENT_OF + && relationshipRepository.existsByPersonIdAndRelatedPersonIdAndRelationType( + newObject.getId(), newSubject.getId(), RelationType.PARENT_OF)) { + throw DomainException.conflict( + ErrorCode.CIRCULAR_RELATIONSHIP, + "Reverse PARENT_OF already exists between " + newSubject.getId() + " and " + newObject.getId()); + } + + rel.setPerson(newSubject); + rel.setRelatedPerson(newObject); + rel.setRelationType(dto.relationType()); + rel.setFromDate(dto.fromDate()); + rel.setFromDatePrecision(normalizePrecision(dto.fromDatePrecision())); + rel.setToDate(dto.toDate()); + rel.setToDatePrecision(normalizePrecision(dto.toDatePrecision())); + rel.setNotes(blankToNull(dto.notes())); + + PersonRelationship saved; + try { + saved = relationshipRepository.saveAndFlush(rel); + } catch (DataIntegrityViolationException e) { + throw DomainException.conflict( + ErrorCode.DUPLICATE_RELATIONSHIP, + "Relationship already exists for (" + newSubject.getId() + ", " + newObject.getId() + ", " + dto.relationType() + ")"); + } + // An edit into a family type flags both endpoints; never auto-unflags (additive). + if (FAMILY_RELATION_TYPES.contains(dto.relationType())) { + personService.setFamilyMember(newSubject.getId(), true); + personService.setFamilyMember(newObject.getId(), true); + } + return toDTO(saved); + } + @Transactional public void deleteRelationship(UUID personId, UUID relId) { + PersonRelationship rel = loadOwnedRelationship(personId, relId); + relationshipRepository.delete(rel); + } + + // Loads the row and verifies {personId} is one of its endpoints. A mismatch is 404 + // (not 403): an anti-enumeration choice so a curator cannot probe relationship ids + // belonging to people they cannot see. Shared by update + delete for consistency. + private PersonRelationship loadOwnedRelationship(UUID personId, UUID relId) { PersonRelationship rel = relationshipRepository.findById(relId) .orElseThrow(() -> DomainException.notFound( ErrorCode.RELATIONSHIP_NOT_FOUND, "Relationship not found: " + relId)); - UUID storageSubject = rel.getPerson().getId(); UUID storageObject = rel.getRelatedPerson().getId(); if (!personId.equals(storageSubject) && !personId.equals(storageObject)) { - throw DomainException.forbidden( + throw DomainException.notFound( + ErrorCode.RELATIONSHIP_NOT_FOUND, "Relationship " + relId + " does not belong to person " + personId); } - relationshipRepository.delete(rel); + return rel; } @Transactional diff --git a/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipControllerTest.java b/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipControllerTest.java index 8bf9b92f..bec1b1c7 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipControllerTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipControllerTest.java @@ -26,6 +26,8 @@ import java.util.UUID; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; @@ -159,4 +161,51 @@ class RelationshipControllerTest { mockMvc.perform(delete("/api/persons/{id}/relationships/{relId}", PERSON_ID, relId).with(csrf())) .andExpect(status().isNoContent()); } + + // ─── PUT /api/persons/{id}/relationships/{relId} ────────────────────────── + + @Test + @WithMockUser(username = "testuser", authorities = {"WRITE_ALL"}) + void updateRelationship_returns200_with_RelationshipDTO_for_WRITE_ALL_user() throws Exception { + UUID relId = UUID.randomUUID(); + RelationshipDTO updated = new RelationshipDTO( + relId, PERSON_ID, OTHER_ID, + "Alice Müller", null, null, + "Bob Müller", null, null, + RelationType.SPOUSE_OF, null, DatePrecision.UNKNOWN, null, DatePrecision.UNKNOWN, null); + when(relationshipService.updateRelationship(any(), any(), any())).thenReturn(updated); + + mockMvc.perform(put("/api/persons/{id}/relationships/{relId}", PERSON_ID, relId).with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"relatedPersonId\":\"" + OTHER_ID + "\",\"relationType\":\"SPOUSE_OF\"}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.relationType").value("SPOUSE_OF")); + } + + @Test + void updateRelationship_returns401_whenUnauthenticated_and_does_not_touch_service() throws Exception { + mockMvc.perform(put("/api/persons/{id}/relationships/{relId}", PERSON_ID, UUID.randomUUID()).with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"relatedPersonId\":\"" + OTHER_ID + "\",\"relationType\":\"SPOUSE_OF\"}")) + .andExpect(status().isUnauthorized()); + verify(relationshipService, never()).updateRelationship(any(), any(), any()); + } + + @Test + @WithMockUser(username = "testuser", authorities = {"READ_ALL"}) + void updateRelationship_returns403_for_READ_ALL_only_user() throws Exception { + mockMvc.perform(put("/api/persons/{id}/relationships/{relId}", PERSON_ID, UUID.randomUUID()).with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"relatedPersonId\":\"" + OTHER_ID + "\",\"relationType\":\"SPOUSE_OF\"}")) + .andExpect(status().isForbidden()); + } + + @Test + @WithMockUser(username = "testuser", authorities = {"WRITE_ALL"}) + void updateRelationship_returns400_when_relationType_is_unknown_value() throws Exception { + mockMvc.perform(put("/api/persons/{id}/relationships/{relId}", PERSON_ID, UUID.randomUUID()).with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"relatedPersonId\":\"" + OTHER_ID + "\",\"relationType\":\"NOT_A_REAL_TYPE\"}")) + .andExpect(status().isBadRequest()); + } } diff --git a/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceIntegrationTest.java b/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceIntegrationTest.java index 5f8391b2..859180b5 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceIntegrationTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceIntegrationTest.java @@ -109,20 +109,50 @@ class RelationshipServiceIntegrationTest { } @Test - void deleteRelationship_throws_403_when_rel_belongs_to_different_person() { + void deleteRelationship_throws_404_when_rel_belongs_to_different_person() { RelationshipDTO created = relationshipService.addRelationship(alice.getId(), new RelationshipUpsertRequest(bob.getId(), RelationType.PARENT_OF, null, null, null, null, null)); - // Charlie is unrelated to this row. + // Charlie is unrelated to this row. Ownership mismatch is 404, not 403, so a + // curator cannot enumerate relationship ids belonging to people they can't see + // (anti-enumeration; aligned with the PUT endpoint — ADR-044). assertThatThrownBy(() -> relationshipService.deleteRelationship(charlie.getId(), created.id())) .isInstanceOf(DomainException.class) .extracting("code") - .isEqualTo(ErrorCode.FORBIDDEN); + .isEqualTo(ErrorCode.RELATIONSHIP_NOT_FOUND); // The row is still there. assertThat(relationshipRepository.findById(created.id())).isPresent(); } + @Test + void updateRelationship_persists_new_type_dates_and_notes() { + RelationshipDTO created = relationshipService.addRelationship(alice.getId(), + new RelationshipUpsertRequest(bob.getId(), RelationType.FRIEND, null, null, null, null, null)); + + RelationshipDTO updated = relationshipService.updateRelationship(alice.getId(), created.id(), + new RelationshipUpsertRequest(bob.getId(), RelationType.SPOUSE_OF, + LocalDate.of(1923, 5, 12), DatePrecision.DAY, null, null, "wedding day")); + + assertThat(updated.id()).isEqualTo(created.id()); + assertThat(updated.relationType()).isEqualTo(RelationType.SPOUSE_OF); + assertThat(updated.fromDate()).isEqualTo(LocalDate.of(1923, 5, 12)); + assertThat(updated.fromDatePrecision()).isEqualTo(DatePrecision.DAY); + assertThat(updated.notes()).isEqualTo("wedding day"); + } + + @Test + void updateRelationship_throws_404_when_rel_belongs_to_different_person() { + RelationshipDTO created = relationshipService.addRelationship(alice.getId(), + new RelationshipUpsertRequest(bob.getId(), RelationType.PARENT_OF, null, null, null, null, null)); + + assertThatThrownBy(() -> relationshipService.updateRelationship(charlie.getId(), created.id(), + new RelationshipUpsertRequest(bob.getId(), RelationType.FRIEND, null, null, null, null, null))) + .isInstanceOf(DomainException.class) + .extracting("code") + .isEqualTo(ErrorCode.RELATIONSHIP_NOT_FOUND); + } + @Test void addRelationship_throws_409_when_reverse_SPOUSE_OF_pair_already_exists() { // V55 enforces symmetric uniqueness for SPOUSE_OF. Inserting (alice, bob, SPOUSE_OF) diff --git a/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceTest.java index 6123ee30..30449e6d 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/person/relationship/RelationshipServiceTest.java @@ -61,9 +61,9 @@ class RelationshipServiceTest { charlie = person("Charlie"); } - // --- Nora blocker 1 --- + // --- Nora blocker 1 (anti-enumeration: ownership mismatch is 404, aligned with PUT) --- @Test - void deleteRelationship_throws_FORBIDDEN_when_rel_belongs_to_different_person() { + void deleteRelationship_throws_NOT_FOUND_when_rel_belongs_to_different_person() { UUID relId = UUID.randomUUID(); PersonRelationship rel = parentOf(alice, bob, relId); when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); @@ -71,7 +71,7 @@ class RelationshipServiceTest { assertThatThrownBy(() -> service.deleteRelationship(charlie.getId(), relId)) .isInstanceOf(DomainException.class) .extracting("code") - .isEqualTo(ErrorCode.FORBIDDEN); + .isEqualTo(ErrorCode.RELATIONSHIP_NOT_FOUND); verify(relationshipRepository, never()).delete(any()); } @@ -249,6 +249,131 @@ class RelationshipServiceTest { .isEqualTo(ErrorCode.RELATIONSHIP_NOT_FOUND); } + // --- updateRelationship (REQ-004/006/007/008/009/010/013) --- + + @Test + void updateRelationship_throws_NOT_FOUND_when_relId_unknown() { + UUID relId = UUID.randomUUID(); + when(relationshipRepository.findById(relId)).thenReturn(Optional.empty()); + + var dto = new RelationshipUpsertRequest(bob.getId(), RelationType.FRIEND, null, null, null, null, null); + assertThatThrownBy(() -> service.updateRelationship(alice.getId(), relId, dto)) + .isInstanceOf(DomainException.class) + .extracting("code") + .isEqualTo(ErrorCode.RELATIONSHIP_NOT_FOUND); + verify(relationshipRepository, never()).saveAndFlush(any()); + } + + @Test + void updateRelationship_throws_NOT_FOUND_when_rel_belongs_to_different_person() { + UUID relId = UUID.randomUUID(); + PersonRelationship rel = parentOf(alice, bob, relId); + when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); + + var dto = new RelationshipUpsertRequest(bob.getId(), RelationType.FRIEND, null, null, null, null, null); + assertThatThrownBy(() -> service.updateRelationship(charlie.getId(), relId, dto)) + .isInstanceOf(DomainException.class) + .extracting("code") + .isEqualTo(ErrorCode.RELATIONSHIP_NOT_FOUND); + verify(relationshipRepository, never()).saveAndFlush(any()); + } + + @Test + void updateRelationship_throws_VALIDATION_ERROR_on_self_relation() { + UUID relId = UUID.randomUUID(); + PersonRelationship rel = relOf(alice, bob, RelationType.FRIEND, relId); + when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); + + var dto = new RelationshipUpsertRequest(alice.getId(), RelationType.FRIEND, null, null, null, null, null); + assertThatThrownBy(() -> service.updateRelationship(alice.getId(), relId, dto)) + .isInstanceOf(DomainException.class) + .extracting("code") + .isEqualTo(ErrorCode.VALIDATION_ERROR); + verify(relationshipRepository, never()).saveAndFlush(any()); + } + + @Test + void updateRelationship_throws_INVALID_RELATIONSHIP_DATES_when_toDate_before_fromDate() { + UUID relId = UUID.randomUUID(); + PersonRelationship rel = relOf(alice, bob, RelationType.FRIEND, relId); + when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); + + var dto = new RelationshipUpsertRequest(bob.getId(), RelationType.FRIEND, + LocalDate.of(1950, 1, 1), DatePrecision.YEAR, + LocalDate.of(1940, 1, 1), DatePrecision.YEAR, null); + assertThatThrownBy(() -> service.updateRelationship(alice.getId(), relId, dto)) + .isInstanceOf(DomainException.class) + .extracting("code") + .isEqualTo(ErrorCode.INVALID_RELATIONSHIP_DATES); + verify(relationshipRepository, never()).saveAndFlush(any()); + } + + @Test + void updateRelationship_throws_CIRCULAR_when_reverse_PARENT_OF_exists() { + UUID relId = UUID.randomUUID(); + PersonRelationship rel = relOf(alice, bob, RelationType.FRIEND, relId); + when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); + when(personService.getById(bob.getId())).thenReturn(bob); + when(relationshipRepository.existsByPersonIdAndRelatedPersonIdAndRelationType( + bob.getId(), alice.getId(), RelationType.PARENT_OF)).thenReturn(true); + + var dto = new RelationshipUpsertRequest(bob.getId(), RelationType.PARENT_OF, null, null, null, null, null); + assertThatThrownBy(() -> service.updateRelationship(alice.getId(), relId, dto)) + .isInstanceOf(DomainException.class) + .extracting("code") + .isEqualTo(ErrorCode.CIRCULAR_RELATIONSHIP); + verify(relationshipRepository, never()).saveAndFlush(any()); + } + + @Test + void updateRelationship_throws_DUPLICATE_when_db_constraint_violated() { + UUID relId = UUID.randomUUID(); + PersonRelationship rel = relOf(alice, bob, RelationType.FRIEND, relId); + when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); + when(personService.getById(bob.getId())).thenReturn(bob); + when(relationshipRepository.saveAndFlush(any())).thenThrow(new DataIntegrityViolationException("unique_rel")); + + var dto = new RelationshipUpsertRequest(bob.getId(), RelationType.SPOUSE_OF, null, null, null, null, null); + assertThatThrownBy(() -> service.updateRelationship(alice.getId(), relId, dto)) + .isInstanceOf(DomainException.class) + .extracting("code") + .isEqualTo(ErrorCode.DUPLICATE_RELATIONSHIP); + } + + @Test + void updateRelationship_updates_fields_and_returns_dto() { + UUID relId = UUID.randomUUID(); + PersonRelationship rel = relOf(alice, bob, RelationType.FRIEND, relId); + when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); + when(personService.getById(bob.getId())).thenReturn(bob); + when(relationshipRepository.saveAndFlush(any())).thenAnswer(inv -> inv.getArgument(0)); + + var dto = new RelationshipUpsertRequest(bob.getId(), RelationType.SPOUSE_OF, + LocalDate.of(1923, 5, 12), DatePrecision.DAY, null, null, "wedding day"); + var result = service.updateRelationship(alice.getId(), relId, dto); + + assertThat(result.relationType()).isEqualTo(RelationType.SPOUSE_OF); + assertThat(result.fromDate()).isEqualTo(LocalDate.of(1923, 5, 12)); + assertThat(result.fromDatePrecision()).isEqualTo(DatePrecision.DAY); + assertThat(result.toDatePrecision()).isEqualTo(DatePrecision.UNKNOWN); + assertThat(result.notes()).isEqualTo("wedding day"); + } + + @Test + void updateRelationship_marks_both_endpoints_family_when_updated_to_family_type() { + UUID relId = UUID.randomUUID(); + PersonRelationship rel = relOf(alice, bob, RelationType.FRIEND, relId); + when(relationshipRepository.findById(relId)).thenReturn(Optional.of(rel)); + when(personService.getById(bob.getId())).thenReturn(bob); + when(relationshipRepository.saveAndFlush(any())).thenAnswer(inv -> inv.getArgument(0)); + + var dto = new RelationshipUpsertRequest(bob.getId(), RelationType.SIBLING_OF, null, null, null, null, null); + service.updateRelationship(alice.getId(), relId, dto); + + verify(personService).setFamilyMember(alice.getId(), true); + verify(personService).setFamilyMember(bob.getId(), true); + } + @Test void getFamilyNetwork_excludes_edges_where_one_endpoint_is_not_a_family_member() { // alice and bob are family members; charlie is NOT (not in findAllFamilyMembers result). @@ -293,11 +418,15 @@ class RelationshipServiceTest { } private static PersonRelationship parentOf(Person parent, Person child, UUID id) { + return relOf(parent, child, RelationType.PARENT_OF, id); + } + + private static PersonRelationship relOf(Person subject, Person object, RelationType type, UUID id) { return PersonRelationship.builder() .id(id) - .person(parent) - .relatedPerson(child) - .relationType(RelationType.PARENT_OF) + .person(subject) + .relatedPerson(object) + .relationType(type) .createdAt(Instant.now()) .build(); }