setup integration test and final fixes

src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/Person.java

@@ -19,7 +19,7 @@ import java.util.Set;
 @Builder(toBuilder = true)
 @NoArgsConstructor
 @AllArgsConstructor
-public class Person {
+public class Person implements Comparable<Person> {
 
     @Id
     @NonNull
@@ -45,10 +45,6 @@ public class Person {
     @ElementCollection(fetch = FetchType.EAGER)
     private Set<Integer> parentIds = new HashSet<>();
 
-    public Person(int id) {
-        this.id = id;
-    }
-
     public Set<Integer> relatedIds() {
         val ids = new HashSet<Integer>();
         if (partnerId() != null) {
@@ -58,4 +54,9 @@ public class Person {
         ids.addAll(childIds);
         return Collections.unmodifiableSet(ids);
     }
+
+    @Override
+    public int compareTo(Person that) {
+        return this.id - that.id;
+    }
 }

src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonController.java

@@ -7,7 +7,6 @@ import lombok.val;
 import org.springframework.http.ResponseEntity;
 import org.springframework.web.bind.annotation.*;
 
-import java.util.ArrayList;
 import java.util.List;
 
 @RestController
@@ -22,17 +21,17 @@ public class PersonController {
             @RequestBody PersonUpsertRequest request
     ) {
         val person = request.toDomain();
-        personService.upsertPerson(person);
+        val parentsInValidFamilies = personService.upsertPersonAndGetValidParents(person);
 
-        val parentsInValidFamilies = personService.getParentsInValidFamilies();
         if (parentsInValidFamilies.isEmpty()) {
-            // Specs state HTTP 444, but since this is a custom nginx status + not a client error, I cannot do this
+            // Specs state HTTP 444, but since this is a custom nginx status + a client error, I cannot do this in good faith
+            // return ResponseEntity.status(444).build(); bwegh
             return ResponseEntity.noContent().build(); // 204 instead
         }
-        val result = new ArrayList<PersonUpsertResponse>();
+
-        for (val parent : parentsInValidFamilies) {
+        val result = parentsInValidFamilies.stream()
-            result.add(PersonUpsertResponse.fromDomain(parent));
+                .map(PersonUpsertResponse::fromDomain)
-        }
+                .toList();
         return ResponseEntity.ok(result);
     }
 

src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonService.java

@@ -22,8 +22,9 @@ public class PersonService {
     private final PersonRepository personRepository;
 
     // Cache all the valid parents, so we do not have to recalculate all on each upsert.
-    private final SortedSet<Person> parentsInValidFamiliesCache = new TreeSet<>(Comparator.comparingInt(Person::id));
+    private final Set<Person> parentsInValidFamiliesCache = new HashSet<>();
 
+    @Locked
     @PostConstruct
     private void populateCache() {
         // Possible overflow risk, consider streaming/paging solution here
@@ -41,19 +42,19 @@ public class PersonService {
      *
      * @param newPerson The Person data to be persisted
      */
-    @Locked.Write
+    @Locked
     @Transactional
-    public void upsertPerson(Person newPerson) {
+    public Collection<Person> upsertPersonAndGetValidParents(Person newPerson) {
         int id = newPerson.id();
         val relatedPersons = retrieveRelatedOf(newPerson);
         val oldPerson = relatedPersons.getLeft();
         val related = relatedPersons.getRight();
 
-        val modifiedPersons = oldPerson == null ? modifiedPersonSetOf(
+        val modifiedPersons = oldPerson == null ? union(
                 setPartner(id, related, newPerson.partnerId(), null),
                 setParents(id, related, newPerson.parentIds(), Set.of()),
                 setChildren(id, related, newPerson.childIds(), Set.of())
-        ) : modifiedPersonSetOf(
+        ) : union(
                 setPartner(id, related, newPerson.partnerId(), oldPerson.partnerId()),
                 setParents(id, related, newPerson.parentIds(), oldPerson.parentIds()),
                 setChildren(id, related, newPerson.childIds(), oldPerson.childIds())
@@ -72,24 +73,27 @@ public class PersonService {
             val invalidModifieds = Sets.difference(modifiedPersons, validModifieds);
             parentsInValidFamiliesCache.removeAll(invalidModifieds);
         }
+
+        // Return sorted, defensive copy
+        return parentsInValidFamiliesCache.stream()
+                .map(p -> p.toBuilder().build())
+                .collect(Collectors.toCollection(TreeSet::new));
     }
 
     private Pair<Person, Map<Integer, Person>> retrieveRelatedOf(Person newPerson) {
         val oldPerson = personRepository.findById(newPerson.id()).orElse(null);
-        return oldPerson == null || newPerson.equals(oldPerson) ?
+        val relevantPeople = oldPerson == null || newPerson.equals(oldPerson)
-                Pair.of(
+                ? Set.of(newPerson)
-                        oldPerson,
+                : Set.of(newPerson, oldPerson);
-                        retrieveRelatedOf(Set.of(newPerson))
+        return Pair.of(
-                ) :
+                oldPerson,
-                Pair.of(
+                retrieveRelatedOf(relevantPeople)
-                        oldPerson,
+        );
-                        retrieveRelatedOf(Set.of(newPerson, oldPerson))
-                );
     }
 
-    private Map<Integer, Person> retrieveRelatedOf(Set<Person> persons) {
+    private Map<Integer, Person> retrieveRelatedOf(Set<Person> people) {
         val ids = new HashSet<Integer>();
-        for (val person : persons) {
+        for (val person : people) {
             ids.addAll(person.relatedIds());
             if (ids.size() > MAX_RELATED_ASSUMPTION) {
                 throw new AssertionError("Assumed that the amount of related IDs will never exceed %d. Need to rethink this program.".formatted(MAX_RELATED_ASSUMPTION));
@@ -108,20 +112,19 @@ public class PersonService {
         // Need to copy, because .difference returns an unmodifiable view
         val missingIds = Set.copyOf(Sets.difference(ids, related.keySet()));
         for (val missingId : missingIds) {
-            related.put(missingId, new Person(missingId));
+            related.put(missingId, Person.builder().id(missingId).build());
         }
         return Collections.unmodifiableMap(related);
     }
 
-    private Set<Person> modifiedPersonSetOf(
+    @SafeVarargs
-            Set<Person> modifiedPartners,
+    private static Set<Person> union(
-            Set<Person> modifiedParents,
+            Set<Person>... sets
-            Set<Person> modifiedChildren
     ) {
         val result = new HashSet<Person>();
-        result.addAll(modifiedPartners);
+        for (val set : sets) {
-        result.addAll(modifiedParents);
+            result.addAll(set);
-        result.addAll(modifiedChildren);
+        }
         return result;
     }
 
@@ -131,12 +134,16 @@ public class PersonService {
             Integer newPartnerId,
             Integer oldPartnerId
     ) {
-        val modifiedPersons = new HashSet<Person>();
         if (Objects.equals(newPartnerId, oldPartnerId)) {
             return Set.of();
         }
+        val modifiedPersons = new HashSet<Person>();
         if (newPartnerId != null) {
             val newPartner = related.get(newPartnerId);
+            partnerOf(newPartner, related).ifPresent(p -> {
+                p.partnerId(null);
+                modifiedPersons.add(p);
+            });
             newPartner.partnerId(selfId);
             modifiedPersons.add(newPartner);
         }
@@ -148,12 +155,27 @@ public class PersonService {
         return modifiedPersons;
     }
 
+    // Possible N+1 here, if the partner is not known by the related.
+    // Similar performance as looking this up within getRelated..., as traversal is always necessary
+    private Optional<Person> partnerOf(Person person, Map<Integer, Person> related) {
+        if (person.partnerId() == null) {
+            return Optional.empty();
+        }
+        val relatedPartner = related.get(person.partnerId());
+        return relatedPartner == null
+                ? personRepository.findById(person.partnerId()) // N+1 :(
+                : Optional.of(relatedPartner);
+    }
+
     private Set<Person> setParents(
             int childId,
             Map<Integer, Person> related,
             Set<Integer> newParentIds,
             Set<Integer> oldParentIds
     ) {
+        if (Objects.equals(newParentIds, oldParentIds)) {
+            return Set.of();
+        }
         val modifiedPersons = new HashSet<Person>();
         for (val removeFromId : Sets.difference(oldParentIds, newParentIds)) {
             val removeFrom = related.get(removeFromId);
@@ -165,7 +187,7 @@ public class PersonService {
             addTo.childIds().add(childId);
             modifiedPersons.add(addTo);
         }
-        return modifiedPersons;
+        return Collections.unmodifiableSet(modifiedPersons);
     }
 
     private Set<Person> setChildren(
@@ -174,6 +196,9 @@ public class PersonService {
             Set<Integer> newChildIds,
             Set<Integer> oldChildIds
     ) {
+        if (Objects.equals(newChildIds, oldChildIds)) {
+            return Set.of();
+        }
         val modifiedPersons = new HashSet<Person>();
         for (val removeFromId : Sets.difference(oldChildIds, newChildIds)) {
             val removeFrom = related.get(removeFromId);
@@ -185,12 +210,7 @@ public class PersonService {
             addTo.parentIds().add(parentId);
             modifiedPersons.add(addTo);
         }
-        return modifiedPersons;
+        return Collections.unmodifiableSet(modifiedPersons);
-    }
-
-    @Locked.Read
-    public SortedSet<Person> getParentsInValidFamilies() {
-        return Collections.unmodifiableSortedSet(parentsInValidFamiliesCache);
     }
 
     private Set<Person> filterValid(Set<Person> candidates) {
@@ -230,9 +250,8 @@ public class PersonService {
      *
      * @param id The ID of the person to be deleted and blacklisted.
      */
-    @Locked.Write
+    @Locked
     public void deletePersonById(int id) {
         throw new UnsupportedOperationException();
     }
-
 }

src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/dto/PersonUpsertRequest.java

@@ -2,38 +2,35 @@ package com.spijkerman.ivo.threekidfamily.domain.person.dto;
 
 import com.spijkerman.ivo.threekidfamily.domain.person.Person;
 import jakarta.annotation.Nullable;
+import lombok.Builder;
 import lombok.val;
 
 import java.time.LocalDate;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 
+@Builder
 public record PersonUpsertRequest(
-       int id,
+        int id,
-       @Nullable String name,
+        @Nullable String name,
-       @Nullable LocalDate birthDate,
+        @Nullable LocalDate birthDate,
-       @Nullable PersonReference parent1,
+        @Nullable PersonReference parent1,
-       @Nullable PersonReference parent2,
+        @Nullable PersonReference parent2,
-       @Nullable PersonReference partner,
+        @Nullable PersonReference partner,
-       @Nullable List<PersonReference> children
+        @Nullable List<PersonReference> children
 ) {
-    public record PersonReference(int id){}
+    public record PersonReference(int id) {
+    }
 
     public Person toDomain() {
-        var builder = Person.builder()
-                .id(id)
-                .name(name)
-                .partnerId(idOf(partner));
-
         val childIds = new HashSet<Integer>();
         if (children != null) {
             for (val child : children) {
                 childIds.add(idOf(child));
             }
         }
-        builder.childIds(childIds);
 
+        // Strip parent ordinal, as behavior is not specified for when two people have the same parents in a different order
         val parentIds = new HashSet<Integer>();
         if (parent1 != null) {
             parentIds.add(idOf(parent1));
@@ -41,9 +38,15 @@ public record PersonUpsertRequest(
         if (parent2 != null) {
             parentIds.add(idOf(parent2));
         }
-        builder.parentIds(parentIds);
 
-        return builder.build();
+        return Person.builder()
+                .id(id)
+                .name(name)
+                .birthDate(birthDate)
+                .partnerId(idOf(partner))
+                .parentIds(parentIds)
+                .childIds(childIds)
+                .build();
     }
 
     private static @Nullable Integer idOf(@Nullable PersonReference personReference) {

src/test/java/com/spijkerman/ivo/threekidfamily/ThreeKidFamilyIT.java

@@ -6,13 +6,14 @@ import lombok.val;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
 import org.springframework.boot.test.web.client.TestRestTemplate;
 import org.springframework.http.HttpMethod;
 import org.springframework.http.HttpStatus;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
+@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT)
 class ThreeKidFamilyIT {
 
     @Autowired

src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonControllerIT.java

@@ -0,0 +1,158 @@
+package com.spijkerman.ivo.threekidfamily.domain.person;
+
+import com.google.common.util.concurrent.Futures;
+import com.spijkerman.ivo.threekidfamily.domain.person.dto.PersonUpsertRequest;
+import lombok.val;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.autoconfigure.web.client.AutoConfigureWebClient;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
+import org.springframework.boot.test.web.client.TestRestTemplate;
+import org.springframework.http.HttpEntity;
+import org.springframework.http.HttpMethod;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.ResponseEntity;
+
+import java.time.Duration;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.atomic.LongAdder;
+import java.util.function.Consumer;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+@AutoConfigureWebClient
+@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT)
+class PersonControllerIT {
+
+    @Autowired
+    private TestRestTemplate restTemplate;
+
+    @Test
+    void upsertFamily() {
+        val expectedBody = "[" +
+                "{\"id\":0,\"name\":\"Mom\",\"birthDate\":\"1992-03-14\"}," +
+                "{\"id\":1,\"name\":null,\"birthDate\":null}" +
+                "]";
+        upsertFamily(
+                0,
+                this::assert204,
+                res -> assert200(res, expectedBody)
+        );
+    }
+
+    @Test
+    @Disabled
+    void naiveLoadTest() throws Exception {
+        val startMs = System.currentTimeMillis();
+        val deadlineMs = startMs + 10_000;
+        val idPage = new AtomicInteger(0);
+        val cpus = Runtime.getRuntime().availableProcessors();
+        val upserts = new LongAdder();
+        val firstException = new AtomicReference<Exception>();
+        try (val executor = Executors.newFixedThreadPool(cpus)) {
+            for (int i = 0; i < cpus; i++) {
+                executor.submit(() -> {
+                    try {
+                        while (System.currentTimeMillis() < deadlineMs) {
+                            upsertFamily(
+                                    idPage.getAndIncrement(),
+                                    this::assert2xx,
+                                    this::assert2xx
+                            );
+                            upserts.add(4); // upsertFamily does 4 upserts internally
+                        }
+                    } catch (Exception e) {
+                        firstException.compareAndSet(null, e);
+                        e.printStackTrace();
+                    }
+                });
+            }
+        }
+
+        if (firstException.get() != null) {
+            throw firstException.get();
+        }
+
+        val endMs = System.currentTimeMillis();
+        val runTime = Duration.ofMillis(endMs - startMs);
+        val rate = upserts.sum() / (runTime.getSeconds() + runTime.getNano() / 1_000_000_000.0);
+
+        System.err.printf("Performed %d upsert requests in %s. Avg rate: %f req/s%n", upserts.sum(), runTime, rate);
+    }
+
+    private void assert2xx(ResponseEntity<?> response) {
+        assertThat(response.getStatusCode().is2xxSuccessful()).isTrue();
+    }
+
+    private void assert200(ResponseEntity<?> response, String expectedBody) {
+        assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
+        assertThat(response.getBody()).isEqualTo(expectedBody);
+    }
+
+    private void assert204(ResponseEntity<?> response) {
+        assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT);
+        assertThat(response.hasBody()).isFalse();
+    }
+
+    private void upsertFamily(
+            int idPage,
+            Consumer<ResponseEntity<String>> firstAssertions,
+            Consumer<ResponseEntity<String>> finalAssertion
+    ) {
+        val momId = idPage * 10;
+        val dadId = momId + 1;
+        val aliceId = momId + 2;
+        val bobId = momId + 3;
+        val charlieId = momId + 4;
+
+        val mom = PersonUpsertRequest.builder()
+                .id(momId)
+                .name("Mom")
+                .birthDate(LocalDate.of(1992, 3, 14))
+                .partner(ref(dadId))
+                .build();
+        // Dad is implicit
+        val alice = PersonUpsertRequest.builder()
+                .id(aliceId)
+                .birthDate(LocalDate.now().minusYears(14))
+                .parent1(ref(momId))
+                .parent2(ref(dadId))
+                .build();
+        val bob = PersonUpsertRequest.builder()
+                .id(bobId)
+                .parent1(ref(momId))
+                .parent2(ref(dadId))
+                .build();
+        val charlie = PersonUpsertRequest.builder()
+                .id(charlieId)
+                // Note: switched around parent ids
+                .parent1(ref(dadId))
+                .parent2(ref(momId))
+                .build();
+
+        for (val req : List.of(mom, alice, bob)) {
+            val response = upsertRequest(req);
+            firstAssertions.accept(response);
+        }
+
+        val response = upsertRequest(charlie);
+        finalAssertion.accept(response);
+    }
+
+    private ResponseEntity<String> upsertRequest(PersonUpsertRequest requestBody) {
+        return restTemplate.exchange("/api/v1/people", HttpMethod.POST, new HttpEntity<>(requestBody), String.class);
+    }
+
+    private static PersonUpsertRequest.PersonReference ref(int id) {
+        return new PersonUpsertRequest.PersonReference(id);
+    }
+}

src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonServiceTest.java

@@ -5,7 +5,6 @@ import com.google.common.collect.Maps;
 import lombok.val;
 import org.assertj.core.util.Sets;
 import org.junit.jupiter.api.Test;
-import org.mockito.Mockito;
 
 import java.time.LocalDate;
 import java.util.*;
@@ -16,8 +15,8 @@ import static org.mockito.Mockito.*;
 class PersonServiceTest {
 
     @Test
-    void testUpsertPerson_forFullPerson_verifyFamilySaved() {
+    void testUpsertPerson_forFullPerson_AndGetValidParents_verifyFamilySaved() {
-        val repository = repository();
+        val repository = mockRepository();
         val sut = new PersonService(repository);
 
         val given = Person.builder()
@@ -29,7 +28,7 @@ class PersonServiceTest {
                 .partnerId(20)
                 .build();
 
-        sut.upsertPerson(given);
+        sut.upsertPersonAndGetValidParents(given);
 
         verify(repository).findById(1);
         verify(repository).findAllById(Set.of(2, 10, 20));
@@ -44,8 +43,8 @@ class PersonServiceTest {
     }
 
     @Test
-    void testUpsertPerson_forFullThenSlimPerson_expectFamilyStripped() {
+    void testUpsertPerson_forFullThenSlimPerson_AndGetValidParents_expectFamilyStripped() {
-        val repository = repository();
+        val repository = mockRepository();
         val sut = new PersonService(repository);
 
         val fullGiven = Person.builder()
@@ -61,9 +60,9 @@ class PersonServiceTest {
                 .id(1)
                 .build();
 
-        sut.upsertPerson(fullGiven);
+        sut.upsertPersonAndGetValidParents(fullGiven);
         clearInvocations(repository); // So we only have to verify the invocations of inserting slim alice
-        sut.upsertPerson(slimGiven);
+        sut.upsertPersonAndGetValidParents(slimGiven);
 
         verify(repository).findById(1);
         verify(repository).findAllById(Set.of(2, 10, 20));
@@ -77,8 +76,8 @@ class PersonServiceTest {
     }
 
     @Test
-    void testUpsertPerson_forTwoPartners_expectOneSave() {
+    void testUpsertPerson_AndGetValidParents_forTwoPartners_expectOneSave() {
-        val repository = repository();
+        val repository = mockRepository();
         val sut = new PersonService(repository);
 
         val alice = Person.builder()
@@ -90,19 +89,19 @@ class PersonServiceTest {
                 .partnerId(1)
                 .build();
 
-        sut.upsertPerson(alice);
+        sut.upsertPersonAndGetValidParents(alice);
-        sut.upsertPerson(bob);
+        sut.upsertPersonAndGetValidParents(bob);
 
         verify(repository).findById(1);
         verify(repository).findById(2);
         verify(repository).findAllById(Set.of(1));
         verify(repository).findAllById(Set.of(2));
-        verify(repository).saveAll(Set.of(alice, bob));
         verify(repository).findAllById(Set.of(1, 2)); // For cache update
+        verify(repository).saveAll(Set.of(alice, bob));
         verifyNoMoreInteractions(repository);
     }
 
-    private static PersonRepository repository() {
+    private static PersonRepository mockRepository() {
         val mockDb = new HashMap<Integer, Person>();
         val repository = mock(PersonRepository.class);
 
@@ -135,7 +134,7 @@ class PersonServiceTest {
 
     @Test
     void getParentsInValidFamiliesCache() {
-        val repository = repository();
+        val repository = mockRepository();
         val sut = new PersonService(repository);
 
         // Two parents
@@ -162,16 +161,12 @@ class PersonServiceTest {
                 .parentIds(parentIds)
                 .build();
 
-        sut.upsertPerson(mom);
+        // No valid parents yet
-        sut.upsertPerson(dad);
+        assertThat(sut.upsertPersonAndGetValidParents(mom)).isEmpty();
-        sut.upsertPerson(alice);
+        assertThat(sut.upsertPersonAndGetValidParents(dad)).isEmpty();
-        sut.upsertPerson(bob);
+        assertThat(sut.upsertPersonAndGetValidParents(alice)).isEmpty();
-
+        assertThat(sut.upsertPersonAndGetValidParents(bob)).isEmpty();
-        // No valid persons yet
-        assertThat(sut.getParentsInValidFamilies()).isEmpty();
-        sut.upsertPerson(charlie);
-
         // Both parents are valid now
-        assertThat(sut.getParentsInValidFamilies()).map(Person::id).containsExactly(1, 2);
+        assertThat(sut.upsertPersonAndGetValidParents(charlie)).map(Person::id).containsExactly(1, 2);
     }
 }