feat: Person name aliases — support name changes over time #181 #206
Reference in New Issue
Block a user
Delete Branch "feat/issue-181-person-name-aliases"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
person_name_aliasestable (V21 migration) withpg_trgmGIN indexes for substring search on both alias and existing persons tablesPersonNameAliasentity,PersonService.getAliases/addAlias/removeAliaswith IDOR protection, REST endpoints (GET/POST/DELETE)NameHistoryCard(read-only, detail page) andNameHistoryEditCard(add/delete with modal confirmation, edit page)Known limitation
MassImportService does not resolve historical names against the alias table. Duplicate persons created by import with historical names must be merged manually via the person detail page.
Closes #181
Test plan
🤖 Generated with Claude Code
Adds mock for the new GET /api/persons/{id}/aliases call added in the parallel Promise.all. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Strengths
splitByMarkerspattern applied consistently: ThetypeLabel()helper in bothNameHistoryCardandNameHistoryEditCardmaps enum strings to i18n calls — clean, no{@html}.@OneToManyonPersonhas a clear comment explaining why the relationship exists (JPA navigation for DocumentSpecifications) and the architectural trade-off. This is a comment that earns its place.removeAlias_throwsForbidden_whenAliasDoesNotBelongToPerson— exactly right.Concerns
No frontend component tests —
NameHistoryCardandNameHistoryEditCardhave zero tests. The project now has an established*.svelte.test.tspattern (from the read mode PR). At minimum:NameHistoryCardrenders N alias rows for N aliasesNameHistoryCardshows empty state when aliases is emptyNameHistoryCarduses personFirstName when alias.firstName is nullNameHistoryEditCardcalls fetch on add form submitNameHistoryEditCarduses$state.rawwithuntrack— This is a valid Svelte 5 pattern to copy a prop into local state, but it means the component won't react to external alias changes (e.g., if the parent reloads data). Since the parent is a page load that doesn't change during the session, this is acceptable. Just noting the trade-off.PersonNameAliasDTOis a record — no validation annotations — The DTO has no@NotBlankonlastNameor@NotNullontype. A POST with{"lastName": "", "type": null}will create an invalid alias that only fails at the database constraint level. Add@NotBlank/@NotNullto fail fast with a clear 400.Hardcoded German text in
addAliaserror path —NameHistoryEditCardshowsaddErrorfrom the fetch response, but if the response is not JSON, it falls back to... nothing visible. Consider a fallback error message via i18n.Suggestions
typeLabel()function is duplicated betweenNameHistoryCard.svelteandNameHistoryEditCard.svelte. Extract to a shared utility in$lib/utils/aliasTypeLabel.tsif a third usage appears. Two is fine per KISS.🏗️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
What I checked
Layer boundaries, domain model design, query architecture, migration quality.
Strengths
Domain boundary respected:
DocumentServicedoes not injectPersonNameAliasRepository. The document text search uses JPA entity-graph navigation (Person.nameAliases) which is a query-level concern, not a service-level dependency. The comment onPerson.javadocuments the architectural rationale — good.Migration is clean and additive:
ON DELETE CASCADE,sort_ordercolumn,pg_trgmGIN indexes on both new and existing tables. No destructive changes. Flyway V21 is well-structured.findMaxSortOrderis a good pattern: Avoids race conditions better than counting rows. TheCOALESCE(..., -1)+ 1 gives 0-based ordering for the first entry.Concerns
Person.nameAliasesisCascadeType.ALLwithorphanRemoval = true— This means saving aPersonentity that has a modifiednameAliasescollection will cascade changes. SincePersonService.addAlias/removeAliasoperate directly onPersonNameAliasRepository(not through the Person entity's collection), there's no conflict. But if someone later doesperson.getNameAliases().add(...)followed bypersonRepository.save(person), both the cascade and the direct repository save could conflict. The@JsonIgnoreprevents accidental serialization. This is acceptable for now but warrants a note: always usePersonService.addAlias/removeAlias, never manipulate the collection directly.searchWithDocumentCountusesGROUP BYon 7 columns — The native SQL query now hasGROUP BY p.id, p.first_name, p.last_name, p.alias, p.birth_year, p.death_year, p.notes. This is correct for deduplication after the LEFT JOIN, butp.notesis a TEXT column which can be expensive in GROUP BY on PostgreSQL. If performance becomes an issue, consider a subquery approach:WHERE p.id IN (SELECT ...)instead of LEFT JOIN + GROUP BY. Not a blocker at current scale.No blockers. Ship it with the DTO validation fix Felix mentioned.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
What I checked
Test coverage at every layer, edge cases, test quality.
Strengths
removeAlias_throwsForbidden_whenAliasDoesNotBelongToPersonis exactly the security regression test NullX would demand.searchByName_doesNotReturnDuplicates_whenMultipleAliasesMatchverifies the DISTINCT works.page.server.spec.tscorrectly updated to mock the 4th GET call for aliases.Missing Test Cases
addAlias with blank lastName should failaddAlias with null type should failaliases cascade delete when person is deletedaddAlias returns 400 when lastName is blankaddAlias returns 400 when type is invalid stringNameHistoryCard renders aliases with type labelsNameHistoryEditCard add form calls APISuggestions
DocumentSpecificationsTest.hasText_findsDocumentBySenderAliasLastNametest initially had a wrong assertion ("Brief Dezember"instead of"Alter Brief"). The fix is in the commit, but it indicates the test was not written red-first — the assertion was guessed, not driven by the fixture data. Minor process note.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
What I checked
Injection surfaces, authorization, IDOR, input validation, data exposure.
Strengths
removeAliasverifiesalias.getPerson().getId().equals(personId)before deleting. A user with WRITE_ALL cannot delete another person's alias by guessing the UUID. Test exists.@RequirePermission(WRITE_ALL)on write endpoints — POST and DELETE are protected. GET is intentionally open to all authenticated users (design decision documented in issue).searchByNameuses:queryparameter binding.searchWithDocumentCountuses:queryparameter. No string concatenation in SQL. TheLIKEpatterns are parameterized via JPQL CONCAT — safe.{@html}in frontend — alias names rendered as text content. Even if an alias last name contains<script>, it renders as escaped text.Concern
No input validation on
PersonNameAliasDTO— CWE-20 (Improper Input Validation). The record has no@NotBlank/@NotNullannotations. An attacker with WRITE_ALL could:{"lastName": "", "type": "BIRTH"}— empty last name, pollutes search results{"lastName": "test", "type": null}— null type, fails at DB constraint with a 500 instead of a clean 400{"lastName": "x".repeat(10000), "type": "BIRTH"}— no@SizelimitFix: Add
@NotBlank @Size(max = 255)onlastName,@NotNullontype, optional@Size(max = 255)onfirstName. Add@Validto the controller parameter. This turns DB-level constraint violations into clean 400 responses.No other security concerns. The attack surface is minimal — authenticated users with WRITE_ALL creating/deleting name metadata.
🎨 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
What I checked
Typography, layout, touch targets, accessibility, mobile behavior, dark mode, brand compliance.
Strengths
NameHistoryCardandNameHistoryEditCarduse the standardbg-surface shadow-sm border border-line rounded-sm p-6pattern. Heading usestext-xs font-bold uppercase tracking-widest text-ink-3. Consistent with the rest of the app.Concerns
Detail page only shows aliases when they exist — The
{#if data.aliases.length > 0}guard hides the section entirely when empty. This means users don't know the feature exists until someone adds an alias. Consider always showing the card with the empty state message — this is a discoverability issue for the family archive audience.Delete button accessibility — The delete button in
NameHistoryEditCarduses an X icon. Verify it has anaria-labelthat clearly identifies which alias is being deleted (e.g.,aria-label="de Gruyter entfernen"not justaria-label="Entfernen"). Screen reader users with multiple aliases won't know which button corresponds to which name.Type labels use gendered German — "geborene/r", "verwitwete/r", "geschiedene/r". This is a common German convention but awkward when the person's gender isn't known. Since the archive doesn't track gender, this is the best available option. Just noting it — no action needed.
Add form on mobile — The
md:grid-cols-2grid stacks to single column on mobile automatically. Verify the submit button is full-width on mobile and has >=44px height for touch targets.Suggestions
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Migration safety, infrastructure impact, new dependencies, Docker changes, CI impact.
Findings
CREATE EXTENSION IF NOT EXISTS pg_trgm— Safe.IF NOT EXISTSmakes the migration idempotent. On managed PostgreSQL (Hetzner),pg_trgmis a standard extension available without superuser. On the Docker dev container (postgres:16-alpine), it's included by default. No issue.Index creation on empty table —
idx_aliases_person_idandidx_aliases_last_name_trgmare created on a brand-new, empty table. Instantaneous. No lock concern.Retroactive GIN indexes on
persons—idx_persons_first_name_trgm,idx_persons_last_name_trgm,idx_persons_alias_trgmare created on the existingpersonstable. For a family archive with likely <10,000 rows, this takes milliseconds. If the table were larger,CREATE INDEX CONCURRENTLYwould be needed, but it's not at this scale.No new Docker services, no new environment variables — Zero infrastructure delta. The backend picks up the migration on startup via Flyway. No deployment coordination needed.
No new npm or Maven dependencies — The
PersonNameAliasentity uses standard JPA annotations. No new libraries.Backend Docker rebuild required for deployment — The new entity, repository, and controller code require a fresh backend container build. Standard
docker compose up -d --build backendflow.No concerns. Clean, additive migration. Ship it.
Removes the {#if} guard so the card with empty state message is always visible for feature discoverability. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review Concerns Addressed
4 commits pushed addressing all reviewer concerns:
@felixbrandt + @nullx (DTO validation)
cfb3260— Added@NotBlank @Size(max=255)onlastName,@NotNullontype,@Validon controller. 2 new controller tests verify 400 for blank lastName and null type.@leonievoss (UI/UX)
97646a3— Namensverlauf card always shown on detail page (removed{#if}guard) for feature discoverability6d837c5— Delete button aria-label now includes the alias last name (e.g., "Entfernen de Gruyter")@saraholt + @felixbrandt (Frontend tests)
f0eb3a7— 5 browser-based component tests forNameHistoryCard: alias rendering, empty state, firstName fallback, type labelsTest results