From 20cfe41f21f17e62ff3920a3b545926d8e71b0d5 Mon Sep 17 00:00:00 2001 From: Marcel Date: Sat, 6 Jun 2026 12:50:21 +0200 Subject: [PATCH] fix(person): resolve case-colliding aliases without throwing (#731) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findOrCreateByAlias resolved via Optional findByAliasIgnoreCase, which throws NonUniqueResultException once two aliases collide only by case (müller / Müller) — a generic 500 on the importer path. Mirror the #730 tag fix: resolve exact-case first, then the lowest-id case-insensitive sibling, then create-when-absent (institution/group and maiden-name alias preserved). The throwing Optional<…>IgnoreCase variant is deleted so it can't be reused. Co-Authored-By: Claude Opus 4.8 --- .../person/PersonRepository.java | 12 +++- .../familienarchiv/person/PersonService.java | 57 +++++++++------ .../person/PersonRepositoryTest.java | 28 +++++--- .../person/PersonServiceIntegrationTest.java | 29 ++++++++ .../person/PersonServiceTest.java | 72 +++++++++++++++---- 5 files changed, 151 insertions(+), 47 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/person/PersonRepository.java b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonRepository.java index 0ca82751..841d8f38 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/person/PersonRepository.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonRepository.java @@ -29,8 +29,16 @@ public interface PersonRepository extends JpaRepository { // Stammbaum-Knoten: alle Personen mit family_member = true. List findByFamilyMemberTrueOrderByLastNameAscFirstNameAsc(); - // Lookup by full alias string, used during ODS mass import - Optional findByAliasIgnoreCase(String alias); + // Exact-case alias lookup — the first resolution step in findOrCreateByAlias. + // Case-colliding aliases across persons (müller / Müller) are valid human labels, NOT + // duplicates: source_ref is the stable identity (ADR-025/032), alias is editable. Do NOT + // add a unique(lower(alias)) constraint — see ADR-032. + Optional findByAlias(String alias); + + // Plural case-insensitive alias lookup — the fallback step. Returns ALL case-folding + // siblings so the service can pick a deterministic one (lowest id) instead of letting a + // derived Optional<…>IgnoreCase throw NonUniqueResultException. See ADR-032. + List findAllByAliasIgnoreCase(String alias); // Lookup by the normalizer person_id, used for idempotent canonical re-import (Phase 3). Optional findBySourceRef(String sourceRef); diff --git a/backend/src/main/java/org/raddatz/familienarchiv/person/PersonService.java b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonService.java index 1646c236..f0a682db 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/person/PersonService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/person/PersonService.java @@ -1,5 +1,6 @@ package org.raddatz.familienarchiv.person; +import java.util.Comparator; import java.util.List; import java.util.Optional; import java.util.UUID; @@ -125,32 +126,42 @@ public class PersonService { PersonType type = PersonTypeClassifier.classify(alias); if (type == PersonType.SKIP) return null; - return personRepository.findByAliasIgnoreCase(alias).orElseGet(() -> { - if (type == PersonType.INSTITUTION || type == PersonType.GROUP) { - return personRepository.save(Person.builder() - .alias(alias) - .lastName(alias) - .personType(type) - .build()); - } + // Aliases differing only by case (müller / Müller) are valid distinct persons, not + // duplicates, so resolution must never throw: exact-case first, then the lowest-id + // case-insensitive sibling, then create. Mirrors the tag path — see ADR-032. + Optional exact = personRepository.findByAlias(alias); + if (exact.isPresent()) return exact.get(); // exact-case wins + List caseInsensitive = personRepository.findAllByAliasIgnoreCase(alias); + if (!caseInsensitive.isEmpty()) { + return caseInsensitive.stream().min(Comparator.comparing(Person::getId)).orElseThrow(); // deterministic tie-break — list is non-empty, never throws + } - PersonNameParser.SplitName split = PersonNameParser.split(alias); - Person person = personRepository.save(Person.builder() + // Create-when-absent: institution/group keep the full label in lastName; a person name + // is split and a maiden name (geb. …) becomes a MAIDEN_NAME alias. + if (type == PersonType.INSTITUTION || type == PersonType.GROUP) { + return personRepository.save(Person.builder() .alias(alias) - .firstName(split.firstName()) - .lastName(split.lastName()) + .lastName(alias) + .personType(type) .build()); - if (split.maidenName() != null) { - int nextSortOrder = aliasRepository.findMaxSortOrder(person.getId()) + 1; - aliasRepository.save(PersonNameAlias.builder() - .person(person) - .lastName(split.maidenName()) - .type(PersonNameAliasType.MAIDEN_NAME) - .sortOrder(nextSortOrder) - .build()); - } - return person; - }); + } + + PersonNameParser.SplitName split = PersonNameParser.split(alias); + Person person = personRepository.save(Person.builder() + .alias(alias) + .firstName(split.firstName()) + .lastName(split.lastName()) + .build()); + if (split.maidenName() != null) { + int nextSortOrder = aliasRepository.findMaxSortOrder(person.getId()) + 1; + aliasRepository.save(PersonNameAlias.builder() + .person(person) + .lastName(split.maidenName()) + .type(PersonNameAliasType.MAIDEN_NAME) + .sortOrder(nextSortOrder) + .build()); + } + return person; } /** diff --git a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java index 21c24847..48f518d2 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonRepositoryTest.java @@ -121,26 +121,34 @@ class PersonRepositoryTest { .containsExactly("Anna", "Clara"); } - // ─── findByAliasIgnoreCase ──────────────────────────────────────────────── + // ─── findByAlias (exact) / findAllByAliasIgnoreCase (case-folding siblings) ─── @Test - void findByAliasIgnoreCase_returnsMatchingPerson() { + void findByAlias_returnsExactCaseMatchOnly() { personRepository.save(Person.builder() .firstName("Karl").lastName("Brandt").alias("Opa Karl").build()); - Optional found = personRepository.findByAliasIgnoreCase("opa karl"); - - assertThat(found).isPresent(); - assertThat(found.get().getFirstName()).isEqualTo("Karl"); + assertThat(personRepository.findByAlias("Opa Karl")).isPresent(); + assertThat(personRepository.findByAlias("opa karl")).isEmpty(); // exact-case: a folded form does NOT match } @Test - void findByAliasIgnoreCase_returnsEmpty_whenAliasDoesNotMatch() { - Optional found = personRepository.findByAliasIgnoreCase("nobody"); - - assertThat(found).isEmpty(); + void findAllByAliasIgnoreCase_returnsEmpty_whenAliasDoesNotMatch() { + assertThat(personRepository.findAllByAliasIgnoreCase("nobody")).isEmpty(); } + @Test + void findAllByAliasIgnoreCase_foldsUmlautCase_inRealPostgres() { + // Proves Postgres LOWER() folds ü the same way for both rows — a plain-ASCII probe would + // stay green even if umlaut folding regressed. Both case-colliding aliases must match. + personRepository.save(Person.builder().lastName("Müller").alias("Müller").build()); + personRepository.save(Person.builder().lastName("müller").alias("müller").build()); + + assertThat(personRepository.findAllByAliasIgnoreCase("MÜLLER")).hasSize(2); + } + + // ─── findByFirstNameIgnoreCaseAndLastNameIgnoreCase ─────────────────────── + // ─── findByFirstNameIgnoreCaseAndLastNameIgnoreCase ─────────────────────── @Test diff --git a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceIntegrationTest.java b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceIntegrationTest.java index 66e0a93b..285d927a 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceIntegrationTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceIntegrationTest.java @@ -20,6 +20,7 @@ import jakarta.persistence.EntityManager; import jakarta.persistence.PersistenceContext; import java.util.Set; +import java.util.UUID; import static org.assertj.core.api.Assertions.assertThat; @@ -75,6 +76,34 @@ class PersonServiceIntegrationTest { assertThat(result.getLastName()).isEqualTo("Cram"); } + // ─── #731: case-colliding alias resolution against real Postgres ─────────── + // The umlaut pair is mandatory — only the real DB proves Postgres LOWER() folds ü; a + // plain-ASCII test would stay green while umlaut aliases regressed. + + @Test + void findOrCreateByAlias_resolvesUmlautAliasCollision_toLowestId_withoutThrow() { + Person muller = personRepository.save(Person.builder().lastName("Müller").alias("Müller").build()); + Person mullerLower = personRepository.save(Person.builder().lastName("müller").alias("müller").build()); + UUID expected = muller.getId().compareTo(mullerLower.getId()) <= 0 ? muller.getId() : mullerLower.getId(); + + // No exact-case "MÜLLER" row → falls through to the case-insensitive branch with two + // candidates and must pick the lowest id, never throwing NonUniqueResultException. + Person resolved = personService.findOrCreateByAlias("MÜLLER"); + + assertThat(resolved.getId()).isEqualTo(expected); + } + + @Test + void findOrCreateByAlias_umlautAliasCollision_isDeterministicAcrossCalls() { + personRepository.save(Person.builder().lastName("Müller").alias("Müller").build()); + personRepository.save(Person.builder().lastName("müller").alias("müller").build()); + + Person first = personService.findOrCreateByAlias("MÜLLER"); + Person second = personService.findOrCreateByAlias("MÜLLER"); + + assertThat(second.getId()).isEqualTo(first.getId()); + } + // ─── #667: confirm round-trip + reader-default semantics ────────────────── @Test diff --git a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceTest.java index e6a9035a..300681dd 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/person/PersonServiceTest.java @@ -375,14 +375,57 @@ class PersonServiceTest { // ─── findOrCreateByAlias ───────────────────────────────────────────────── @Test - void findOrCreateByAlias_returnsExisting_whenAliasFound() { - String alias = "Walter de Gruyter"; - Person existing = Person.builder().id(UUID.randomUUID()).alias(alias).build(); - when(personRepository.findByAliasIgnoreCase(alias)).thenReturn(Optional.of(existing)); + void findOrCreateByAlias_returnsExactCaseMatch_overCaseInsensitiveSibling() { + String alias = "müller"; + Person exact = Person.builder().id(UUID.randomUUID()).alias("müller").build(); + when(personRepository.findByAlias(alias)).thenReturn(Optional.of(exact)); Person result = personService.findOrCreateByAlias(alias); - assertThat(result).isEqualTo(existing); + assertThat(result).isEqualTo(exact); + verify(personRepository, never()).findAllByAliasIgnoreCase(any()); + verify(personRepository, never()).save(any()); + } + + @Test + void findOrCreateByAlias_returnsExactCaseMatch_evenWhenMultipleSiblingsCollide() { + String alias = "Müller"; + Person exact = Person.builder().id(UUID.randomUUID()).alias("Müller").build(); + when(personRepository.findByAlias(alias)).thenReturn(Optional.of(exact)); + + Person result = personService.findOrCreateByAlias(alias); + + assertThat(result).isEqualTo(exact); + // exact-case short-circuits — the case-insensitive siblings are never consulted. + verify(personRepository, never()).findAllByAliasIgnoreCase(any()); + } + + @Test + void findOrCreateByAlias_usesSingleCaseInsensitiveMatch_whenNoExactCase() { + String alias = "müller"; + Person only = Person.builder().id(UUID.randomUUID()).alias("Müller").build(); + when(personRepository.findByAlias(alias)).thenReturn(Optional.empty()); + when(personRepository.findAllByAliasIgnoreCase(alias)).thenReturn(List.of(only)); + + Person result = personService.findOrCreateByAlias(alias); + + assertThat(result).isEqualTo(only); + verify(personRepository, never()).save(any()); + } + + @Test + void findOrCreateByAlias_returnsLowestIdDeterministically_whenMultipleCaseInsensitiveMatches() { + String alias = "müller"; + Person lower = Person.builder().id(UUID.fromString("00000000-0000-0000-0000-000000000001")).alias("Müller").build(); + Person higher = Person.builder().id(UUID.fromString("00000000-0000-0000-0000-000000000002")).alias("müller").build(); + when(personRepository.findByAlias(alias)).thenReturn(Optional.empty()); + when(personRepository.findAllByAliasIgnoreCase(alias)).thenReturn(List.of(higher, lower)); // unordered + + Person first = personService.findOrCreateByAlias(alias); + Person second = personService.findOrCreateByAlias(alias); + + assertThat(first.getId()).isEqualTo(lower.getId()); // lowest id wins + assertThat(second.getId()).isEqualTo(first.getId()); // same result every call — never throws verify(personRepository, never()).save(any()); } @@ -390,7 +433,8 @@ class PersonServiceTest { void findOrCreateByAlias_createsNew_whenAliasNotFound() { String alias = "Clara Cram"; Person saved = Person.builder().id(UUID.randomUUID()).alias(alias).firstName("Clara").lastName("Cram").build(); - when(personRepository.findByAliasIgnoreCase(alias)).thenReturn(Optional.empty()); + when(personRepository.findByAlias(alias)).thenReturn(Optional.empty()); + when(personRepository.findAllByAliasIgnoreCase(alias)).thenReturn(List.of()); when(personRepository.save(any())).thenReturn(saved); Person result = personService.findOrCreateByAlias(alias); @@ -403,7 +447,8 @@ class PersonServiceTest { void findOrCreateByAlias_createsMaidenNameAlias_whenGebPresent() { String alias = "Clara Cram geb. de Gruyter"; Person saved = Person.builder().id(UUID.randomUUID()).alias(alias).firstName("Clara").lastName("Cram").build(); - when(personRepository.findByAliasIgnoreCase(alias)).thenReturn(Optional.empty()); + when(personRepository.findByAlias(alias)).thenReturn(Optional.empty()); + when(personRepository.findAllByAliasIgnoreCase(alias)).thenReturn(List.of()); when(personRepository.save(any())).thenReturn(saved); when(aliasRepository.findMaxSortOrder(saved.getId())).thenReturn(0); when(aliasRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); @@ -425,7 +470,8 @@ class PersonServiceTest { @Test void findOrCreateByAlias_setsInstitutionType_withFullNameInLastName() { String alias = "Arthur Collignon GmbH"; - when(personRepository.findByAliasIgnoreCase(alias)).thenReturn(Optional.empty()); + when(personRepository.findByAlias(alias)).thenReturn(Optional.empty()); + when(personRepository.findAllByAliasIgnoreCase(alias)).thenReturn(List.of()); when(personRepository.save(any())).thenAnswer(inv -> { Person p = inv.getArgument(0); p.setId(UUID.randomUUID()); @@ -442,7 +488,8 @@ class PersonServiceTest { @Test void findOrCreateByAlias_setsGroupType_withFullNameInLastName() { String alias = "Geschwister de Gruyter"; - when(personRepository.findByAliasIgnoreCase(alias)).thenReturn(Optional.empty()); + when(personRepository.findByAlias(alias)).thenReturn(Optional.empty()); + when(personRepository.findAllByAliasIgnoreCase(alias)).thenReturn(List.of()); when(personRepository.save(any())).thenAnswer(inv -> { Person p = inv.getArgument(0); p.setId(UUID.randomUUID()); @@ -460,7 +507,8 @@ class PersonServiceTest { void findOrCreateByAlias_noAlias_whenNoGeb() { String alias = "Clara Cram"; Person saved = Person.builder().id(UUID.randomUUID()).alias(alias).firstName("Clara").lastName("Cram").build(); - when(personRepository.findByAliasIgnoreCase(alias)).thenReturn(Optional.empty()); + when(personRepository.findByAlias(alias)).thenReturn(Optional.empty()); + when(personRepository.findAllByAliasIgnoreCase(alias)).thenReturn(List.of()); when(personRepository.save(any())).thenReturn(saved); personService.findOrCreateByAlias(alias); @@ -472,11 +520,11 @@ class PersonServiceTest { void findOrCreateByAlias_trimsInput() { String alias = " Clara Cram "; Person saved = Person.builder().id(UUID.randomUUID()).alias("Clara Cram").build(); - when(personRepository.findByAliasIgnoreCase("Clara Cram")).thenReturn(Optional.of(saved)); + when(personRepository.findByAlias("Clara Cram")).thenReturn(Optional.of(saved)); personService.findOrCreateByAlias(alias); - verify(personRepository).findByAliasIgnoreCase("Clara Cram"); + verify(personRepository).findByAlias("Clara Cram"); } // ─── updatePerson (notes) ────────────────────────────────────────────────