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 545a1e78..94cff9b1 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 @@ -99,22 +99,12 @@ public class RelationshipService { @Transactional public RelationshipDTO addRelationship(UUID personId, RelationshipUpsertRequest dto) { - if (personId.equals(dto.relatedPersonId())) { - throw DomainException.badRequest( - ErrorCode.VALIDATION_ERROR, "Cannot relate a person to themselves"); - } + requireNotSelf(personId, dto.relatedPersonId()); Person person = personService.getById(personId); Person relatedPerson = personService.getById(dto.relatedPersonId()); validateRelationshipDates(dto.fromDate(), dto.fromDatePrecision(), dto.toDate(), dto.toDatePrecision()); - - if (dto.relationType() == RelationType.PARENT_OF - && relationshipRepository.existsByPersonIdAndRelatedPersonIdAndRelationType( - relatedPerson.getId(), personId, RelationType.PARENT_OF)) { - throw DomainException.conflict( - ErrorCode.CIRCULAR_RELATIONSHIP, - "Reverse PARENT_OF already exists between " + personId + " and " + relatedPerson.getId()); - } + requireNoReverseParent(person.getId(), relatedPerson.getId(), dto.relationType()); PersonRelationship rel = PersonRelationship.builder() .person(person) @@ -127,22 +117,8 @@ public class RelationshipService { .notes(blankToNull(dto.notes())) .build(); - PersonRelationship saved; - try { - // saveAndFlush so the unique_rel constraint violates synchronously and is - // caught here, not at commit time outside the @Transactional boundary. - saved = relationshipRepository.saveAndFlush(rel); - } catch (DataIntegrityViolationException e) { - throw DomainException.conflict( - ErrorCode.DUPLICATE_RELATIONSHIP, - "Relationship already exists for (" + personId + ", " + relatedPerson.getId() + ", " + dto.relationType() + ")"); - } - // Family-graph edges imply both endpoints are family members. Idempotent: the - // setter is a no-op when the person is already flagged, so re-imports stay clean. - if (FAMILY_RELATION_TYPES.contains(dto.relationType())) { - personService.setFamilyMember(person.getId(), true); - personService.setFamilyMember(relatedPerson.getId(), true); - } + PersonRelationship saved = persistOrConflict(rel, person.getId(), relatedPerson.getId(), dto.relationType()); + flagFamilyMembership(dto.relationType(), person.getId(), relatedPerson.getId()); return toDTO(saved); } @@ -151,29 +127,20 @@ public class RelationshipService { 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"); - } + requireNotSelf(personId, dto.relatedPersonId()); 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 viewpoint = viewpointIsSubject ? rel.getPerson() : rel.getRelatedPerson(); + Person other = personService.getById(dto.relatedPersonId()); 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()); - } + requireNoReverseParent(newSubject.getId(), newObject.getId(), dto.relationType()); rel.setPerson(newSubject); rel.setRelatedPerson(newObject); @@ -184,20 +151,53 @@ public class RelationshipService { rel.setToDatePrecision(DatePrecisionValidation.normalize(dto.toDatePrecision())); rel.setNotes(blankToNull(dto.notes())); - PersonRelationship saved; + PersonRelationship saved = persistOrConflict(rel, newSubject.getId(), newObject.getId(), dto.relationType()); + flagFamilyMembership(dto.relationType(), newSubject.getId(), newObject.getId()); + return toDTO(saved); + } + + // --- shared create/update invariants --------------------------------------------- + + // A person cannot be related to themselves, from either viewpoint. + private static void requireNotSelf(UUID viewpointId, UUID relatedPersonId) { + if (viewpointId.equals(relatedPersonId)) { + throw DomainException.badRequest( + ErrorCode.VALIDATION_ERROR, "Cannot relate a person to themselves"); + } + } + + // A PARENT_OF edge must not already have its mirror (child PARENT_OF parent) stored — + // that would be a cycle. No-op for every other relation type. + private void requireNoReverseParent(UUID subjectId, UUID objectId, RelationType type) { + if (type == RelationType.PARENT_OF + && relationshipRepository.existsByPersonIdAndRelatedPersonIdAndRelationType( + objectId, subjectId, RelationType.PARENT_OF)) { + throw DomainException.conflict( + ErrorCode.CIRCULAR_RELATIONSHIP, + "Reverse PARENT_OF already exists between " + subjectId + " and " + objectId); + } + } + + // saveAndFlush so the unique_rel constraint violates synchronously and is caught here, + // inside the @Transactional boundary, not at commit time as a raw 500. + private PersonRelationship persistOrConflict(PersonRelationship rel, UUID subjectId, UUID objectId, RelationType type) { try { - saved = relationshipRepository.saveAndFlush(rel); + return relationshipRepository.saveAndFlush(rel); } catch (DataIntegrityViolationException e) { throw DomainException.conflict( ErrorCode.DUPLICATE_RELATIONSHIP, - "Relationship already exists for (" + newSubject.getId() + ", " + newObject.getId() + ", " + dto.relationType() + ")"); + "Relationship already exists for (" + subjectId + ", " + objectId + ", " + type + ")"); } - // 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); + } + + // Family-graph edges imply both endpoints are family members. Idempotent (the setter is + // a no-op when already flagged, so re-imports stay clean) and additive — an edit never + // auto-unflags. + private void flagFamilyMembership(RelationType type, UUID subjectId, UUID objectId) { + if (FAMILY_RELATION_TYPES.contains(type)) { + personService.setFamilyMember(subjectId, true); + personService.setFamilyMember(objectId, true); } - return toDTO(saved); } @Transactional