refactor(relationship): collapse add/update onto shared invariant helpers
addRelationship and updateRelationship each inlined the same self-check, reverse-PARENT_OF cycle check, saveAndFlush→DUPLICATE conflict mapping, and family-membership flagging. Extract them into requireNotSelf, requireNoReverseParent, persistOrConflict, and flagFamilyMembership so the shared invariants live once and each public method reads as its own clear create-vs-mutate sequence. Behaviour-preserving: RelationshipServiceTest (22) and RelationshipServiceIntegrationTest (10) stay green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -99,22 +99,12 @@ public class RelationshipService {
|
|||||||
|
|
||||||
@Transactional
|
@Transactional
|
||||||
public RelationshipDTO addRelationship(UUID personId, RelationshipUpsertRequest dto) {
|
public RelationshipDTO addRelationship(UUID personId, RelationshipUpsertRequest dto) {
|
||||||
if (personId.equals(dto.relatedPersonId())) {
|
requireNotSelf(personId, dto.relatedPersonId());
|
||||||
throw DomainException.badRequest(
|
|
||||||
ErrorCode.VALIDATION_ERROR, "Cannot relate a person to themselves");
|
|
||||||
}
|
|
||||||
Person person = personService.getById(personId);
|
Person person = personService.getById(personId);
|
||||||
Person relatedPerson = personService.getById(dto.relatedPersonId());
|
Person relatedPerson = personService.getById(dto.relatedPersonId());
|
||||||
|
|
||||||
validateRelationshipDates(dto.fromDate(), dto.fromDatePrecision(), dto.toDate(), dto.toDatePrecision());
|
validateRelationshipDates(dto.fromDate(), dto.fromDatePrecision(), dto.toDate(), dto.toDatePrecision());
|
||||||
|
requireNoReverseParent(person.getId(), relatedPerson.getId(), dto.relationType());
|
||||||
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());
|
|
||||||
}
|
|
||||||
|
|
||||||
PersonRelationship rel = PersonRelationship.builder()
|
PersonRelationship rel = PersonRelationship.builder()
|
||||||
.person(person)
|
.person(person)
|
||||||
@@ -127,22 +117,8 @@ public class RelationshipService {
|
|||||||
.notes(blankToNull(dto.notes()))
|
.notes(blankToNull(dto.notes()))
|
||||||
.build();
|
.build();
|
||||||
|
|
||||||
PersonRelationship saved;
|
PersonRelationship saved = persistOrConflict(rel, person.getId(), relatedPerson.getId(), dto.relationType());
|
||||||
try {
|
flagFamilyMembership(dto.relationType(), person.getId(), relatedPerson.getId());
|
||||||
// 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);
|
|
||||||
}
|
|
||||||
return toDTO(saved);
|
return toDTO(saved);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -151,29 +127,20 @@ public class RelationshipService {
|
|||||||
PersonRelationship rel = loadOwnedRelationship(personId, relId);
|
PersonRelationship rel = loadOwnedRelationship(personId, relId);
|
||||||
|
|
||||||
// The other party from {personId}'s viewpoint cannot be {personId} itself.
|
// The other party from {personId}'s viewpoint cannot be {personId} itself.
|
||||||
if (personId.equals(dto.relatedPersonId())) {
|
requireNotSelf(personId, dto.relatedPersonId());
|
||||||
throw DomainException.badRequest(
|
|
||||||
ErrorCode.VALIDATION_ERROR, "Cannot relate a person to themselves");
|
|
||||||
}
|
|
||||||
validateRelationshipDates(dto.fromDate(), dto.fromDatePrecision(), dto.toDate(), dto.toDatePrecision());
|
validateRelationshipDates(dto.fromDate(), dto.fromDatePrecision(), dto.toDate(), dto.toDatePrecision());
|
||||||
|
|
||||||
// Preserve the directed orientation: {personId} keeps whichever role (subject or
|
// Preserve the directed orientation: {personId} keeps whichever role (subject or
|
||||||
// object) it already holds on the row, and the edited "related person" takes the
|
// 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
|
// other role. So a PARENT_OF edge stays parent→child whether the curator edits it
|
||||||
// from the parent's page or the child's.
|
// 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());
|
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 newSubject = viewpointIsSubject ? viewpoint : other;
|
||||||
Person newObject = viewpointIsSubject ? other : viewpoint;
|
Person newObject = viewpointIsSubject ? other : viewpoint;
|
||||||
|
|
||||||
if (dto.relationType() == RelationType.PARENT_OF
|
requireNoReverseParent(newSubject.getId(), newObject.getId(), dto.relationType());
|
||||||
&& 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.setPerson(newSubject);
|
||||||
rel.setRelatedPerson(newObject);
|
rel.setRelatedPerson(newObject);
|
||||||
@@ -184,20 +151,53 @@ public class RelationshipService {
|
|||||||
rel.setToDatePrecision(DatePrecisionValidation.normalize(dto.toDatePrecision()));
|
rel.setToDatePrecision(DatePrecisionValidation.normalize(dto.toDatePrecision()));
|
||||||
rel.setNotes(blankToNull(dto.notes()));
|
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 {
|
try {
|
||||||
saved = relationshipRepository.saveAndFlush(rel);
|
return relationshipRepository.saveAndFlush(rel);
|
||||||
} catch (DataIntegrityViolationException e) {
|
} catch (DataIntegrityViolationException e) {
|
||||||
throw DomainException.conflict(
|
throw DomainException.conflict(
|
||||||
ErrorCode.DUPLICATE_RELATIONSHIP,
|
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);
|
|
||||||
}
|
}
|
||||||
return toDTO(saved);
|
|
||||||
|
// 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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@Transactional
|
@Transactional
|
||||||
|
|||||||
Reference in New Issue
Block a user