From ea1d5ea7eb87ac9636ac532f298321ab51746815 Mon Sep 17 00:00:00 2001 From: Ivo Spijkerman Date: Thu, 23 Oct 2025 21:43:23 +0200 Subject: [PATCH] setup integration test and final fixes --- .../threekidfamily/domain/person/Person.java | 11 +- .../domain/person/PersonController.java | 15 +- .../domain/person/PersonService.java | 87 ++++++---- .../person/dto/PersonUpsertRequest.java | 37 ++-- .../ivo/threekidfamily/ThreeKidFamilyIT.java | 3 +- .../domain/person/PersonControllerIT.java | 158 ++++++++++++++++++ .../domain/person/PersonServiceTest.java | 45 +++-- 7 files changed, 266 insertions(+), 90 deletions(-) create mode 100644 src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonControllerIT.java diff --git a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/Person.java b/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/Person.java index 6510e6f..a40fd29 100644 --- a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/Person.java +++ b/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 { @Id @NonNull @@ -45,10 +45,6 @@ public class Person { @ElementCollection(fetch = FetchType.EAGER) private Set parentIds = new HashSet<>(); - public Person(int id) { - this.id = id; - } - public Set relatedIds() { val ids = new HashSet(); 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; + } } diff --git a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonController.java b/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonController.java index 2b46ac3..97a2b67 100644 --- a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonController.java +++ b/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(); - for (val parent : parentsInValidFamilies) { - result.add(PersonUpsertResponse.fromDomain(parent)); - } + + val result = parentsInValidFamilies.stream() + .map(PersonUpsertResponse::fromDomain) + .toList(); return ResponseEntity.ok(result); } diff --git a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonService.java b/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonService.java index 19cd74c..f9629e9 100644 --- a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonService.java +++ b/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 parentsInValidFamiliesCache = new TreeSet<>(Comparator.comparingInt(Person::id)); + private final Set 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 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> retrieveRelatedOf(Person newPerson) { val oldPerson = personRepository.findById(newPerson.id()).orElse(null); - return oldPerson == null || newPerson.equals(oldPerson) ? - Pair.of( - oldPerson, - retrieveRelatedOf(Set.of(newPerson)) - ) : - Pair.of( - oldPerson, - retrieveRelatedOf(Set.of(newPerson, oldPerson)) - ); + val relevantPeople = oldPerson == null || newPerson.equals(oldPerson) + ? Set.of(newPerson) + : Set.of(newPerson, oldPerson); + return Pair.of( + oldPerson, + retrieveRelatedOf(relevantPeople) + ); } - private Map retrieveRelatedOf(Set persons) { + private Map retrieveRelatedOf(Set people) { val ids = new HashSet(); - 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 modifiedPersonSetOf( - Set modifiedPartners, - Set modifiedParents, - Set modifiedChildren + @SafeVarargs + private static Set union( + Set... sets ) { val result = new HashSet(); - result.addAll(modifiedPartners); - result.addAll(modifiedParents); - result.addAll(modifiedChildren); + for (val set : sets) { + result.addAll(set); + } return result; } @@ -131,12 +134,16 @@ public class PersonService { Integer newPartnerId, Integer oldPartnerId ) { - val modifiedPersons = new HashSet(); if (Objects.equals(newPartnerId, oldPartnerId)) { return Set.of(); } + val modifiedPersons = new HashSet(); 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 partnerOf(Person person, Map 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 setParents( int childId, Map related, Set newParentIds, Set oldParentIds ) { + if (Objects.equals(newParentIds, oldParentIds)) { + return Set.of(); + } val modifiedPersons = new HashSet(); 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 setChildren( @@ -174,6 +196,9 @@ public class PersonService { Set newChildIds, Set oldChildIds ) { + if (Objects.equals(newChildIds, oldChildIds)) { + return Set.of(); + } val modifiedPersons = new HashSet(); 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; - } - - @Locked.Read - public SortedSet getParentsInValidFamilies() { - return Collections.unmodifiableSortedSet(parentsInValidFamiliesCache); + return Collections.unmodifiableSet(modifiedPersons); } private Set filterValid(Set 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(); } - } diff --git a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/dto/PersonUpsertRequest.java b/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/dto/PersonUpsertRequest.java index fd3378e..9a1c8fe 100644 --- a/src/main/java/com/spijkerman/ivo/threekidfamily/domain/person/dto/PersonUpsertRequest.java +++ b/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, - @Nullable String name, - @Nullable LocalDate birthDate, - @Nullable PersonReference parent1, - @Nullable PersonReference parent2, - @Nullable PersonReference partner, - @Nullable List children + int id, + @Nullable String name, + @Nullable LocalDate birthDate, + @Nullable PersonReference parent1, + @Nullable PersonReference parent2, + @Nullable PersonReference partner, + @Nullable List 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(); 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(); 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) { diff --git a/src/test/java/com/spijkerman/ivo/threekidfamily/ThreeKidFamilyIT.java b/src/test/java/com/spijkerman/ivo/threekidfamily/ThreeKidFamilyIT.java index 47d854b..7689070 100644 --- a/src/test/java/com/spijkerman/ivo/threekidfamily/ThreeKidFamilyIT.java +++ b/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 diff --git a/src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonControllerIT.java b/src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonControllerIT.java new file mode 100644 index 0000000..c8da1c4 --- /dev/null +++ b/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(); + 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> firstAssertions, + Consumer> 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 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); + } +} \ No newline at end of file diff --git a/src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonServiceTest.java b/src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonServiceTest.java index 4bd59f9..bbe25c2 100644 --- a/src/test/java/com/spijkerman/ivo/threekidfamily/domain/person/PersonServiceTest.java +++ b/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() { - val repository = repository(); + void testUpsertPerson_forFullPerson_AndGetValidParents_verifyFamilySaved() { + 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() { - val repository = repository(); + void testUpsertPerson_forFullThenSlimPerson_AndGetValidParents_expectFamilyStripped() { + 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() { - val repository = repository(); + void testUpsertPerson_AndGetValidParents_forTwoPartners_expectOneSave() { + 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.upsertPerson(bob); + sut.upsertPersonAndGetValidParents(alice); + 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(); 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); - sut.upsertPerson(dad); - sut.upsertPerson(alice); - sut.upsertPerson(bob); - - // No valid persons yet - assertThat(sut.getParentsInValidFamilies()).isEmpty(); - sut.upsertPerson(charlie); - + // No valid parents yet + assertThat(sut.upsertPersonAndGetValidParents(mom)).isEmpty(); + assertThat(sut.upsertPersonAndGetValidParents(dad)).isEmpty(); + assertThat(sut.upsertPersonAndGetValidParents(alice)).isEmpty(); + assertThat(sut.upsertPersonAndGetValidParents(bob)).isEmpty(); // Both parents are valid now - assertThat(sut.getParentsInValidFamilies()).map(Person::id).containsExactly(1, 2); + assertThat(sut.upsertPersonAndGetValidParents(charlie)).map(Person::id).containsExactly(1, 2); } } \ No newline at end of file