fix(person): resolve case-colliding aliases and ambiguous sender names without throwing (#731) #745

Merged

marcel merged 4 commits from fix/issue-731-person-case-collision into main 2026-06-06 14:22:08 +02:00

Conversation 8 Commits 4 Files Changed 8 +385 -63

marcel commented 2026-06-06 13:04:47 +02:00

Owner

Copy Link

Closes #731. Sibling of #730 (merged) — same root cause (Optional<…>IgnoreCase is a leaky identity that throws NonUniqueResultException once two rows collide case-insensitively), here on the two Person lookup surfaces.

What changed

Commit 1 — alias path (findOrCreateByAlias) — 20cfe41f

• Resolves exact-case (findByAlias) → lowest-id case-insensitive sibling (findAllByAliasIgnoreCase) → existing create-when-absent branch (INSTITUTION/GROUP and maiden-name alias preserved verbatim). Does not throw on a case-collision.
• Deletes the throwing Optional<Person> findByAliasIgnoreCase.

Commit 2 — name/sender path (findByName) — ddf378aa

• Resolves exact-case → single case-insensitive match → else empty. Used by DocumentService.storeDocument for filename-based sender resolution.
• Deliberate divergence from the alias path: an ambiguous filename leaves the sender unset rather than guessing the lowest id — correct provenance beats a confidently-wrong pre-fill a reviewer won't re-check. Load-bearing comment guards against a future "consistency cleanup".
• Both new name queries use explicit HQL equality (= :firstName), not a derived …IgnoreCase query, so a null first name binds as = NULL (no match) instead of the derived fold to first_name IS NULL, which would widen a last-name-only row in as a sender. Pinned by a real-Postgres repo test.
• Deletes the throwing Optional<Person> findByFirstNameIgnoreCaseAndLastNameIgnoreCase.
• Pins the opaque error path: IncorrectResultSizeDataAccessException → INTERNAL_ERROR with no Hibernate class name / SQL / row count leak.
• Adds ADR-033 (the case-collision ADR) Person section and fixes the person/README.md findByName mislabel ("Typeahead search" → filename-based sender resolution).

Commit 3 — scope clarification — cd741b9f

• Records at both exact-case lookups (and in the ADR) that two byte-identical same-case rows are an out-of-scope data anomaly that surfaces as the opaque INTERNAL_ERROR (never a wrong row) — the exact-case step still returns Optional. Addresses the round-1 "never throws was overstated" review note.

Commit 4 — ADR renumber housekeeping — 7629e358

• Both #730 and #684 had landed an ADR-032 on main. This renumbers the tag/case-collision ADR 032 → 033 (referenced only from this PR, so self-contained and touches no Flyway migration). The person-delete ADR-032 and the V71 comment citing it are left untouched — editing an applied migration would drift its Flyway checksum.

Decisions (per issue)

• Alias = deterministic lowest-id (mirrors tag); name/sender = bail-to-null with the provenance rationale.
• No DB migration, code-only and reversible. No unique(lower(...)) constraint (collisions are valid human labels; source_ref is the stable identity). No merge/dedupe. No shared helper (the two paths have different fallbacks — KISS, inlined).
• Follow-up UX hint for the ambiguous-sender case filed as #746 (P3-later).

Tests (TDD, red→green; targeted runs, never the full suite locally)

• PersonServiceTest (64) — exact wins · exact wins among multiple siblings · single-CI · multi-CI lowest-id + determinism · SKIP · create-with-maiden-alias · trim · findByName exact/single-CI/2+→empty/null-first→empty.
• PersonRepositoryTest (52, @DataJpaTest Testcontainers) — exact-only matches · umlaut folding in real Postgres · null-first-name → empty (the IS NULL semantics check).
• PersonServiceIntegrationTest (19, Testcontainers) — umlaut alias collision lowest-id + determinism · storeDocument unique→sender set · single-CI→set · ambiguous→sender null (no 500) · last-name-only filename→null · findByName(null,…)→empty.
• GlobalExceptionHandlerTest (4) — opaque-error regression.
• Re-ran TagCaseCollisionIntegrationTest (3) and DocumentServiceTest (187) — unaffected, green.
• CI run #2066 (full mvn test + 88% JaCoCo gate) green on the implementation commit.

Not done here (needs prod/observability access)

• Pre-deploy detection SQL (the two GROUP BY … HAVING count(*) > 1 read-only queries against prod) — please run to record latent-vs-active counts.
• Post-deploy GlitchTip search for IncorrectResultSizeDataAccessException / NonUniqueResultException in person frames.

🤖 Generated with Claude Code

Closes #731. Sibling of #730 (merged) — same root cause (`Optional<…>IgnoreCase` is a leaky identity that throws `NonUniqueResultException` once two rows collide case-insensitively), here on the **two** Person lookup surfaces. ## What changed **Commit 1 — alias path (`findOrCreateByAlias`)** — `20cfe41f` - Resolves exact-case (`findByAlias`) → lowest-id case-insensitive sibling (`findAllByAliasIgnoreCase`) → existing create-when-absent branch (INSTITUTION/GROUP and maiden-name alias preserved verbatim). Does not throw on a case-collision. - Deletes the throwing `Optional<Person> findByAliasIgnoreCase`. **Commit 2 — name/sender path (`findByName`)** — `ddf378aa` - Resolves exact-case → single case-insensitive match → else **empty**. Used by `DocumentService.storeDocument` for filename-based sender resolution. - **Deliberate divergence from the alias path:** an ambiguous filename leaves the sender **unset** rather than guessing the lowest id — correct provenance beats a confidently-wrong pre-fill a reviewer won't re-check. Load-bearing comment guards against a future "consistency cleanup". - Both new name queries use explicit **HQL equality** (`= :firstName`), not a derived `…IgnoreCase` query, so a **null first name binds as `= NULL` (no match)** instead of the derived fold to `first_name IS NULL`, which would widen a last-name-only row in as a sender. Pinned by a real-Postgres repo test. - Deletes the throwing `Optional<Person> findByFirstNameIgnoreCaseAndLastNameIgnoreCase`. - Pins the opaque error path: `IncorrectResultSizeDataAccessException` → `INTERNAL_ERROR` with no Hibernate class name / SQL / row count leak. - Adds **ADR-033** (the case-collision ADR) Person section and fixes the `person/README.md` `findByName` mislabel ("Typeahead search" → filename-based sender resolution). **Commit 3 — scope clarification** — `cd741b9f` - Records at both exact-case lookups (and in the ADR) that two **byte-identical same-case** rows are an out-of-scope data anomaly that surfaces as the opaque `INTERNAL_ERROR` (never a wrong row) — the exact-case step still returns `Optional`. Addresses the round-1 "never throws was overstated" review note. **Commit 4 — ADR renumber housekeeping** — `7629e358` - Both #730 and #684 had landed an **ADR-032** on `main`. This renumbers the tag/case-collision ADR **032 → 033** (referenced only from this PR, so self-contained and **touches no Flyway migration**). The person-delete ADR-032 and the `V71` comment citing it are left untouched — editing an applied migration would drift its Flyway checksum. ## Decisions (per issue) - Alias = deterministic lowest-id (mirrors tag); name/sender = bail-to-null with the provenance rationale. - **No DB migration**, code-only and reversible. No `unique(lower(...))` constraint (collisions are valid human labels; `source_ref` is the stable identity). No merge/dedupe. No shared helper (the two paths have different fallbacks — KISS, inlined). - Follow-up UX hint for the ambiguous-sender case filed as **#746** (P3-later). ## Tests (TDD, red→green; targeted runs, never the full suite locally) - `PersonServiceTest` (64) — exact wins · exact wins among multiple siblings · single-CI · multi-CI lowest-id **+ determinism** · SKIP · create-with-maiden-alias · trim · findByName exact/single-CI/2+→empty/null-first→empty. - `PersonRepositoryTest` (52, `@DataJpaTest` Testcontainers) — exact-only matches · **umlaut folding** in real Postgres · **null-first-name → empty** (the IS NULL semantics check). - `PersonServiceIntegrationTest` (19, Testcontainers) — umlaut alias collision lowest-id + determinism · `storeDocument` unique→sender set · single-CI→set · ambiguous→sender null (no 500) · last-name-only filename→null · `findByName(null,…)`→empty. - `GlobalExceptionHandlerTest` (4) — opaque-error regression. - Re-ran `TagCaseCollisionIntegrationTest` (3) and `DocumentServiceTest` (187) — unaffected, green. - CI run #2066 (full `mvn test` + 88% JaCoCo gate) green on the implementation commit. ## Not done here (needs prod/observability access) - Pre-deploy detection SQL (the two `GROUP BY … HAVING count(*) > 1` read-only queries against prod) — please run to record latent-vs-active counts. - Post-deploy GlitchTip search for `IncorrectResultSizeDataAccessException` / `NonUniqueResultException` in person frames. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 2 commits 2026-06-06 13:04:48 +02:00

fix(person): resolve case-colliding aliases without throwing (#731) ... 20cfe41f21

findOrCreateByAlias resolved via Optional<Person> 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 <noreply@anthropic.com>

fix(person): resolve ambiguous sender names to null on upload (#731) ... ddf378aaac

findByName resolved via Optional<Person>
findByFirstNameIgnoreCaseAndLastNameIgnoreCase, which threw
NonUniqueResultException once two people shared a first+last name case-
insensitively (hans müller / Hans Müller) — a 500 on the routine upload path
(DocumentService.storeDocument sender resolution).

findByName now resolves exact-case → single case-insensitive match → else
empty. The sender path deliberately diverges from the alias path: an
ambiguous name leaves the sender UNSET rather than guessing the lowest id,
because correct provenance beats a confidently-wrong pre-fill a reviewer
won't re-check. The two new name queries use explicit HQL equality so a null
first name binds as `= NULL` (no match) instead of the derived-query fold to
`first_name IS NULL`, which would widen a last-name-only row in as a sender.

Pins the opaque error path (IncorrectResultSizeDataAccessException stays
INTERNAL_ERROR with no Hibernate/SQL/row-count leak) and extends ADR-032 with
the Person section.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

marcel referenced this pull request 2026-06-06 13:05:10 +02:00

Bug: PersonService.findOrCreateByAlias kann NonUniqueResultException werfen bei case-kollidierenden Aliassen (Schwester von #730) #731

marcel commented 2026-06-06 13:07:03 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

⚠️ Approved with concerns

Clean, disciplined work. Guard clauses, no bare .get(), the create-branch is preserved verbatim, and the mock migrations across the 8 PersonServiceTest cases compile and read well. The two paths are inlined rather than over-abstracted into a premature helper — correct KISS call. TDD evidence is strong: every new branch (exact-present / single-CI / multi-CI / create / SKIP / null-firstName) has a named test, and determinism is asserted by calling twice. That is the discipline I want to see.

Blockers

None.

Suggestions

1. Residual non-unique throw on the exact-case step — PersonService.java:114 and :141. findByAlias / findByFirstNameAndLastName still return Optional, so two byte-identical rows (two persons both with alias exactly "Müller", or two real Hans Müller / Hans Müller) will throw NonUniqueResultException on the exact step before the plural fallback is ever reached. The issue defines "ambiguous" as case-insensitive collision with no exact match, so exact dupes are arguably out of scope — but the PR body claims the path "Never throws," which is not strictly true. Either tighten the claim to "never throws on case collision," or make the exact step a plural-min as well. I lean toward the former (scope discipline) but the wording should not overstate.

2. return exact; vs return exact.get() asymmetry — findByName (:115) returns the Optional directly while findOrCreateByAlias (:142) unwraps with .get(). Both correct given the differing return types, just noting the intentional asymmetry for the next reader. The load-bearing comment covers the why of the divergence well.

3. Function length — findOrCreateByAlias is now ~40 lines. Still readable because each block is a single concern (exact → CI → institution-create → person-create), but it brushes the "one thing" ceiling. I would not split it given the issue's explicit "preserve the create branch verbatim, do not flatten" instruction — flagging only so we are conscious of it.

Verdict stands at approved-with-concerns purely on the overstated "never throws" claim; the code itself is sound.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **⚠️ Approved with concerns** Clean, disciplined work. Guard clauses, no bare `.get()`, the create-branch is preserved verbatim, and the mock migrations across the 8 `PersonServiceTest` cases compile and read well. The two paths are inlined rather than over-abstracted into a premature helper — correct KISS call. TDD evidence is strong: every new branch (exact-present / single-CI / multi-CI / create / SKIP / null-firstName) has a named test, and determinism is asserted by calling twice. That is the discipline I want to see. ### Blockers None. ### Suggestions 1. **Residual non-unique throw on the *exact-case* step** — `PersonService.java:114` and `:141`. `findByAlias` / `findByFirstNameAndLastName` still return `Optional`, so two *byte-identical* rows (two persons both with alias exactly `"Müller"`, or two real `Hans Müller` / `Hans Müller`) will throw `NonUniqueResultException` on the exact step before the plural fallback is ever reached. The issue defines "ambiguous" as *case-insensitive collision with no exact match*, so exact dupes are arguably out of scope — but the PR body claims the path "Never throws," which is not strictly true. Either tighten the claim to "never throws on case collision," or make the exact step a plural-min as well. I lean toward the former (scope discipline) but the wording should not overstate. 2. **`return exact;` vs `return exact.get()` asymmetry** — `findByName` (`:115`) returns the `Optional` directly while `findOrCreateByAlias` (`:142`) unwraps with `.get()`. Both correct given the differing return types, just noting the intentional asymmetry for the next reader. The load-bearing comment covers the *why* of the divergence well. 3. **Function length** — `findOrCreateByAlias` is now ~40 lines. Still readable because each block is a single concern (exact → CI → institution-create → person-create), but it brushes the "one thing" ceiling. I would not split it given the issue's explicit "preserve the create branch verbatim, do not flatten" instruction — flagging only so we are conscious of it. Verdict stands at approved-with-concerns purely on the overstated "never throws" claim; the code itself is sound.

marcel commented 2026-06-06 13:07:04 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Senior Application Architect

⚠️ Approved with concerns

This is the right architectural shape: a lookup-layer fix, not a data-model fix. No unique(lower(...)) constraint (correctly rejected — the collisions are valid canonical labels and source_ref is the stable identity per ADR-025), no migration, fully reversible. The deliberate per-surface divergence (alias = lowest-id, name/sender = bail-to-null) is a genuine architectural decision and it is recorded where it belongs — in ADR-032 with the why, not just the what. That is exactly how a decision survives a future refactor.

Layering — clean

DocumentService.storeDocument (:222) resolves the sender through personService.findByName(...), never the repository. The fix stays entirely inside PersonService + PersonRepository. No resolution logic leaked up into DocumentService. Boundary respected.

Blockers

1. ADR-032 number collision is unresolved and is a structural hazard. This PR extends docs/adr/032-tag-name-resolution-tolerates-case-collisions.md, but the feat/issue-684-person-delete-db-fk line independently created docs/adr/032-person-delete-db-level-integrity.md (already referenced in the recent commit history on this base — ADR-032 for person-delete). Two ADR-032s cannot coexist; whichever merges second corrupts the decision log. The issue flags this as "separate housekeeping," but from an architecture-of-the-decision-record standpoint it is a blocker on the repo, not on this diff's logic. Confirm the renumber plan (one becomes 033) before merge, or you will silently lose one ADR's authority.

Suggestions

2. Doc-currency table check — passes. No schema change → no DB-diagram update needed (correct). No new package/service/route → no C4 update (correct). person/README.md method table updated and the findByName mislabel fixed. ADR extended. The only doc gap is the number collision above.

3. The "exact step still returns Optional" residual (see Felix) is acceptable architecturally — pushing exact-dup integrity to a DB constraint would contradict the whole "collisions are valid" stance. Leave it at the application layer; just don't claim absolute non-throw.

Resolve the ADR numbering and this is an approve.

## 🏛️ Markus Keller — Senior Application Architect **⚠️ Approved with concerns** This is the right architectural shape: a lookup-layer fix, not a data-model fix. No `unique(lower(...))` constraint (correctly rejected — the collisions are valid canonical labels and `source_ref` is the stable identity per ADR-025), no migration, fully reversible. The deliberate per-surface divergence (alias = lowest-id, name/sender = bail-to-null) is a genuine architectural decision and it is recorded where it belongs — in ADR-032 with the *why*, not just the *what*. That is exactly how a decision survives a future refactor. ### Layering — clean `DocumentService.storeDocument` (`:222`) resolves the sender through `personService.findByName(...)`, never the repository. The fix stays entirely inside `PersonService` + `PersonRepository`. No resolution logic leaked up into `DocumentService`. Boundary respected. ### Blockers 1. **ADR-032 number collision is unresolved and is a structural hazard.** This PR extends `docs/adr/032-tag-name-resolution-tolerates-case-collisions.md`, but the `feat/issue-684-person-delete-db-fk` line independently created `docs/adr/032-person-delete-db-level-integrity.md` (already referenced in the recent commit history on this base — `ADR-032` for person-delete). Two ADR-032s cannot coexist; whichever merges second corrupts the decision log. The issue flags this as "separate housekeeping," but from an architecture-of-the-decision-record standpoint it is a blocker on *the repo*, not on this diff's logic. Confirm the renumber plan (one becomes 033) before merge, or you will silently lose one ADR's authority. ### Suggestions 2. **Doc-currency table check — passes.** No schema change → no DB-diagram update needed (correct). No new package/service/route → no C4 update (correct). `person/README.md` method table updated and the `findByName` mislabel fixed. ADR extended. The only doc gap is the number collision above. 3. **The "exact step still returns Optional" residual** (see Felix) is acceptable architecturally — pushing exact-dup integrity to a DB constraint would contradict the whole "collisions are valid" stance. Leave it at the application layer; just don't claim absolute non-throw. Resolve the ADR numbering and this is an approve.

marcel commented 2026-06-06 13:07:12 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

✅ Approved

Reviewed for CWE-209 (information exposure through error message), injection, and fail-closed behaviour. This is a security-positive change.

What I checked

1. CWE-209 opaque error path — pinned, good. GlobalExceptionHandlerTest.handleGeneric_incorrectResultSize_staysOpaque_noHibernateOrRowCountLeak asserts the wire body is exactly "An unexpected error occurred" / INTERNAL_ERROR and explicitly .doesNotContain("results were returned"), .doesNotContain("NonUnique"), .doesNotContain("IncorrectResultSize"). That is precisely the regression guard I want: even after the throw is removed, a stray IncorrectResultSizeDataAccessException cannot leak the Hibernate class name, SQL, or row count. The Sentry static mock is correctly stubbed so the assertion isolates the handler. Textbook.

2. No SQL/HQL injection. Both new queries (PersonRepository.java) use named parameters (:firstName, :lastName) with @Param — no string concatenation. Injection-proof by construction.

3. Fail-closed on null first name — and it's a security/integrity property, not just a functional one. A null first name binding as first_name = NULL (never matches) rather than the derived-query fold to first_name IS NULL prevents a last-name-only / institution row from being silently widened in as a document "sender." That is provenance-integrity, and it's proven against real Postgres (findAllByFirstNameIgnoreCaseAndLastNameIgnoreCase_nullFirstName_foldsToNoMatch) — exactly right, because the IS NULL vs = NULL semantics cannot be caught by a mock. Every fix here started from a failing test.

Non-blocking note

• The exact-case Optional step can still throw NonUniqueResultException on byte-identical duplicates — but that throw lands on the same opaque generic handler I just verified, so there is no new disclosure surface. No security impact; functional-correctness only (deferred to the developer's call on scope).

No permission/auth surface touched (importer + upload paths, already gated upstream). Clean.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **✅ Approved** Reviewed for CWE-209 (information exposure through error message), injection, and fail-closed behaviour. This is a security-positive change. ### What I checked 1. **CWE-209 opaque error path — pinned, good.** `GlobalExceptionHandlerTest.handleGeneric_incorrectResultSize_staysOpaque_noHibernateOrRowCountLeak` asserts the wire body is exactly `"An unexpected error occurred"` / `INTERNAL_ERROR` and explicitly `.doesNotContain("results were returned")`, `.doesNotContain("NonUnique")`, `.doesNotContain("IncorrectResultSize")`. That is precisely the regression guard I want: even after the throw is removed, a *stray* `IncorrectResultSizeDataAccessException` cannot leak the Hibernate class name, SQL, or row count. The Sentry static mock is correctly stubbed so the assertion isolates the handler. Textbook. 2. **No SQL/HQL injection.** Both new queries (`PersonRepository.java`) use named parameters (`:firstName`, `:lastName`) with `@Param` — no string concatenation. Injection-proof by construction. 3. **Fail-closed on null first name — and it's a *security/integrity* property, not just a functional one.** A null first name binding as `first_name = NULL` (never matches) rather than the derived-query fold to `first_name IS NULL` prevents a last-name-only / institution row from being silently widened in as a document "sender." That is provenance-integrity, and it's proven against real Postgres (`findAllByFirstNameIgnoreCaseAndLastNameIgnoreCase_nullFirstName_foldsToNoMatch`) — exactly right, because the `IS NULL` vs `= NULL` semantics cannot be caught by a mock. Every fix here started from a failing test. ### Non-blocking note - The exact-case `Optional` step can still throw `NonUniqueResultException` on byte-identical duplicates — but that throw lands on the *same* opaque generic handler I just verified, so there is no new disclosure surface. No security impact; functional-correctness only (deferred to the developer's call on scope). No permission/auth surface touched (importer + upload paths, already gated upstream). Clean.

marcel commented 2026-06-06 13:07:44 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

⚠️ Approved with concerns

The test strategy here is genuinely good — it respects the pyramid, uses real Postgres where the bug actually lives, and the umlaut folding is pinned at two layers (repo + integration) so a plain-ASCII regression can't slip through. Determinism is asserted by calling twice, not assumed. This is the level of rigour I want on a latent-500 fix.

What's well done

• Testcontainers, not H2 — PersonRepositoryTest (@DataJpaTest) and PersonServiceIntegrationTest both run real postgres:16. The null first name → empty semantics test is correctly placed at the repo layer against real Postgres (a mock cannot distinguish = NULL from IS NULL). Exactly the right layer choice.
• Branch coverage per branch — exact-present, single-CI, multi-CI, create, SKIP, null-firstName each have a hitting test. The 88% JaCoCo branch gate should hold (see concern below — I want CI to confirm, not the PR description).
• Integration drives the real caller — storeDocument via uploadNamed(...) exercises the upload→sender path end to end (unique→set, single-CI→set, ambiguous→null, last-name-only→null), so the divergent bail-to-null is verified through the actual DocumentService, not just the service method in isolation. Strong.
• Regression test for the opaque handler stays in the suite permanently. Good — it guards the handler from being weakened later even though the throw is gone.

Blockers

None on test code itself.

Concerns

1. Coverage claim is self-reported, not gate-verified. The PR body lists counts ("PersonServiceTest (64)", "88% branch") and clean package -DskipTests. -DskipTests means the build success proves nothing about the tests. I want the CI run green on this branch (full mvn test + JaCoCo gate) before merge — per project rules the full suite runs in CI, not locally, so this is a "confirm the pipeline is green" gate, not a request to run locally.

2. No test for the exact-case byte-identical duplicate. Felix/Nora flagged that two identical-case rows still throw NonUniqueResultException on the exact step. If that's deemed out of scope (the issue's "ambiguous" definition supports that), fine — but then add a one-line comment or a @Disabled ticket-linked placeholder documenting the known gap, so a future reader doesn't assume it's covered. A silent untested edge is a hidden regression risk.

3. Determinism test uses unordered input (List.of(higher, lower)) — good, that actually proves the min(comparing(id)) and not list order. No change needed; calling it out as a positive.

Lift the CI-green confirmation and this is an approve.

## 🧪 Sara Holt — Senior QA Engineer **⚠️ Approved with concerns** The test strategy here is genuinely good — it respects the pyramid, uses real Postgres where the bug actually lives, and the umlaut folding is pinned at *two* layers (repo + integration) so a plain-ASCII regression can't slip through. Determinism is asserted by calling twice, not assumed. This is the level of rigour I want on a latent-500 fix. ### What's well done - **Testcontainers, not H2** — `PersonRepositoryTest` (`@DataJpaTest`) and `PersonServiceIntegrationTest` both run real `postgres:16`. The `null first name → empty` semantics test is correctly placed at the repo layer against real Postgres (a mock cannot distinguish `= NULL` from `IS NULL`). Exactly the right layer choice. - **Branch coverage per branch** — exact-present, single-CI, multi-CI, create, SKIP, null-firstName each have a hitting test. The 88% JaCoCo branch gate should hold (see concern below — I want CI to confirm, not the PR description). - **Integration drives the real caller** — `storeDocument` via `uploadNamed(...)` exercises the upload→sender path end to end (unique→set, single-CI→set, ambiguous→null, last-name-only→null), so the divergent bail-to-null is verified through the actual `DocumentService`, not just the service method in isolation. Strong. - **Regression test for the opaque handler** stays in the suite permanently. Good — it guards the handler from being weakened later even though the throw is gone. ### Blockers None on test code itself. ### Concerns 1. **Coverage claim is self-reported, not gate-verified.** The PR body lists counts ("PersonServiceTest (64)", "88% branch") and `clean package -DskipTests`. -DskipTests means the build success proves *nothing* about the tests. I want the CI run green on this branch (full `mvn test` + JaCoCo gate) before merge — per project rules the full suite runs in CI, not locally, so this is a "confirm the pipeline is green" gate, not a request to run locally. 2. **No test for the exact-case byte-identical duplicate.** Felix/Nora flagged that two identical-case rows still throw `NonUniqueResultException` on the exact step. If that's deemed out of scope (the issue's "ambiguous" definition supports that), fine — but then add a one-line comment or a `@Disabled` ticket-linked placeholder documenting the known gap, so a future reader doesn't assume it's covered. A silent untested edge is a hidden regression risk. 3. **Determinism test uses unordered input** (`List.of(higher, lower)`) — good, that actually proves the `min(comparing(id))` and not list order. No change needed; calling it out as a positive. Lift the CI-green confirmation and this is an approve.

marcel commented 2026-06-06 13:07:45 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

✅ Approved

Nothing in my domain to block. No Compose changes, no CI workflow changes, no new service, no image tags, no secrets, no infrastructure surface touched. Code-only and explicitly reversible (roll back the JAR) — which is exactly the deploy profile I like for a latent-bug fix: zero migration, zero ordering constraint, no Flyway step that could wedge startup.

What I checked

• No DB migration — confirmed, and the decision to not add unique(lower(...)) is operationally correct as well as semantically: a uniqueness index would fail to apply against existing colliding rows and turn a clean deploy into a failed Flyway migration that blocks backend startup. Avoiding that is the right call.
• Rollback story — clean. No schema state to reconcile; rolling back the JAR fully reverts behaviour.
• No :latest, no bind mounts, no exposed ports, no hardcoded creds introduced. N/A for this diff.

Operational follow-ups (not blockers, but please track)

The PR's own "Not done here" section names two operational items that genuinely need the prod/observability access I'd own:

1. Pre-deploy detection SQL not run. The two read-only GROUP BY … HAVING count(*) > 1 queries against prod (alias collisions, name collisions) tell us whether this bug is latent or already firing in production. That changes deploy urgency. These are safe read-only queries via the app read role — run them and attach the counts before deploy so we know what we're shipping into.

2. Post-deploy GlitchTip verification. Search IncorrectResultSizeDataAccessException / NonUniqueResultException filtered to person stack frames, record the group id, confirm zero new occurrences in the 24h after deploy. Standard close-the-loop for a fix whose whole point is "stop throwing 500s."

Neither blocks the merge — they're deploy-time checklist items. The code change itself is operationally inert and safe.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **✅ Approved** Nothing in my domain to block. No Compose changes, no CI workflow changes, no new service, no image tags, no secrets, no infrastructure surface touched. Code-only and explicitly reversible (roll back the JAR) — which is exactly the deploy profile I like for a latent-bug fix: zero migration, zero ordering constraint, no Flyway step that could wedge startup. ### What I checked - **No DB migration** — confirmed, and the decision to *not* add `unique(lower(...))` is operationally correct as well as semantically: a uniqueness index would fail to apply against existing colliding rows and turn a clean deploy into a failed Flyway migration that blocks backend startup. Avoiding that is the right call. - **Rollback story** — clean. No schema state to reconcile; rolling back the JAR fully reverts behaviour. - **No `:latest`, no bind mounts, no exposed ports, no hardcoded creds** introduced. N/A for this diff. ### Operational follow-ups (not blockers, but please track) The PR's own "Not done here" section names two operational items that genuinely need the prod/observability access I'd own: 1. **Pre-deploy detection SQL not run.** The two read-only `GROUP BY … HAVING count(*) > 1` queries against prod (alias collisions, name collisions) tell us whether this bug is *latent* or *already firing in production*. That changes deploy urgency. These are safe read-only queries via the app read role — run them and attach the counts before deploy so we know what we're shipping into. 2. **Post-deploy GlitchTip verification.** Search `IncorrectResultSizeDataAccessException` / `NonUniqueResultException` filtered to `person` stack frames, record the group id, confirm zero new occurrences in the 24h after deploy. Standard close-the-loop for a fix whose whole point is "stop throwing 500s." Neither blocks the merge — they're deploy-time checklist items. The code change itself is operationally inert and safe.

marcel commented 2026-06-06 13:07:58 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer & Business Analyst

✅ Approved

I reviewed this against the acceptance criteria in #731 and the behavioural correctness of the divergent decision, not the implementation. Working in Brownfield mode — this is a bug fix with a well-specified issue, so my job is traceability: does every Gherkin scenario have a corresponding verifying test, and is the one genuinely-new product decision (bail-to-null) sound and documented?

Traceability — all nine scenarios covered

#731 Scenario	Verifying test
exact-case wins over CI sibling	findOrCreateByAlias_returnsExactCaseMatch_overCaseInsensitiveSibling
exact-case wins even with multiple siblings	..._returnsExactCaseMatch_evenWhenMultipleSiblingsCollide
never throws on CI duplicates (deterministic lowest id)	..._returnsLowestIdDeterministically... + integration umlaut pair
still creates when absent, with maiden-name alias	findOrCreateByAlias_createsMaidenNameAlias_whenGebPresent
returns null + creates nothing for SKIP	preserved SKIP early-return (covered)
unique name resolves sender	storeDocument_resolvesSender_whenFilenameNameIsUnique
single-CI name resolves sender	storeDocument_resolvesSender_onSingleCaseInsensitiveMatch
ambiguous name → sender unset, no 500	storeDocument_leavesSenderUnset_whenFilenameNameIsAmbiguous
last-name-only filename → sender unset	storeDocument_leavesSenderUnset_whenFilenameHasNoFirstName

Every acceptance criterion traces to a named test. That is a complete coverage matrix — rare and welcome.

On the decision itself

The bail-to-null-on-ambiguity choice is the correct requirement, and crucially the rationale is captured at the point of change ("a reviewer won't re-check a pre-filled value; correct provenance > confidently-wrong guess"). This is a real domain rule, not an implementation accident — it belongs in the ADR and it is there. The asymmetry with the alias path (lowest-id) is justified by a genuine difference in consequence (no human in the importer loop vs. a human reviewer downstream). No ambiguity, no contradiction, no hidden assumption left unstated.

One open question (non-blocking, already triaged in the issue)

• OQ: The empty-sender state needs the user to understand why it's empty. The issue correctly defers the inline "Absender nicht eindeutig zuordenbar" hint to a separate low-priority follow-up issue rather than scope-creeping it here. I endorse that split — it keeps this PR INVEST-small. Please confirm that follow-up issue gets filed so the UX gap doesn't fall through the cracks.

No scope creep, no gold-plating. This does exactly what #731 asked and nothing more.

## 📋 Elicit — Requirements Engineer & Business Analyst **✅ Approved** I reviewed this against the acceptance criteria in #731 and the behavioural correctness of the divergent decision, not the implementation. Working in Brownfield mode — this is a bug fix with a well-specified issue, so my job is traceability: does every Gherkin scenario have a corresponding verifying test, and is the one genuinely-new product decision (bail-to-null) sound and documented? ### Traceability — all nine scenarios covered | #731 Scenario | Verifying test | |---|---| | exact-case wins over CI sibling | `findOrCreateByAlias_returnsExactCaseMatch_overCaseInsensitiveSibling` | | exact-case wins even with multiple siblings | `..._returnsExactCaseMatch_evenWhenMultipleSiblingsCollide` | | never throws on CI duplicates (deterministic lowest id) | `..._returnsLowestIdDeterministically...` + integration umlaut pair | | still creates when absent, with maiden-name alias | `findOrCreateByAlias_createsMaidenNameAlias_whenGebPresent` | | returns null + creates nothing for SKIP | preserved `SKIP` early-return (covered) | | unique name resolves sender | `storeDocument_resolvesSender_whenFilenameNameIsUnique` | | single-CI name resolves sender | `storeDocument_resolvesSender_onSingleCaseInsensitiveMatch` | | ambiguous name → sender unset, no 500 | `storeDocument_leavesSenderUnset_whenFilenameNameIsAmbiguous` | | last-name-only filename → sender unset | `storeDocument_leavesSenderUnset_whenFilenameHasNoFirstName` | Every acceptance criterion traces to a named test. That is a complete coverage matrix — rare and welcome. ### On the decision itself The bail-to-null-on-ambiguity choice is the correct *requirement*, and crucially the rationale is captured at the point of change ("a reviewer won't re-check a pre-filled value; correct provenance > confidently-wrong guess"). This is a real domain rule, not an implementation accident — it belongs in the ADR and it is there. The asymmetry with the alias path (lowest-id) is justified by a genuine difference in consequence (no human in the importer loop vs. a human reviewer downstream). No ambiguity, no contradiction, no hidden assumption left unstated. ### One open question (non-blocking, already triaged in the issue) - **OQ:** The empty-sender state needs the user to understand *why* it's empty. The issue correctly defers the inline "Absender nicht eindeutig zuordenbar" hint to a *separate* low-priority follow-up issue rather than scope-creeping it here. I endorse that split — it keeps this PR INVEST-small. Please confirm that follow-up issue gets filed so the UX gap doesn't fall through the cracks. No scope creep, no gold-plating. This does exactly what #731 asked and nothing more.

marcel commented 2026-06-06 13:08:12 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

✅ Approved

No frontend in this diff — backend lookup logic, repository queries, an ADR, and a README. There are no Svelte components, no markup, no colour tokens, no touch targets, and no copy in my scope here. So this is an LGTM with a note on the downstream user experience the change produces.

What I checked (and why it's fine)

The behavioural outcome of the name/sender path is: on an ambiguous filename, the sender is left unset rather than guessed. I verified the issue confirms the empty-sender state already degrades cleanly:

• DocumentRow.svelte already renders the unknown-sender placeholder when sender is null (tested).
• DocumentMetadataDrawer has a no-persons placeholder.
• metadataComplete=false already routes the document into the "needs completion" state for a human to assign.

So this change cannot produce a broken or confusing empty state — it reuses an existing, tested degradation path. From a senior-reader-on-a-phone perspective, an empty-but-clearly-flagged sender field is better than a confidently-wrong pre-filled name they'd never think to correct. The product decision is the accessibility-friendly one.

Follow-up I want to see filed (not a blocker on this PR)

The issue already specs an optional inline hint so the transcriber learns why the sender is empty, and it's correctly deferred to a separate low-priority issue. When that gets picked up, the a11y spec from the issue must hold: redundant cue (info icon + text, not colour-only), German copy "Absender konnte aus dem Dateinamen nicht eindeutig zugeordnet werden", min 16px, text-ink on bg-surface (do not use brand-mint as text — ~2.8:1 fails AA), aria-describedby tying the hint to the sender field, and keep "nicht eindeutig zuordenbar" (ambiguous) copy distinct from "kein Absender" (genuinely unknown). Please confirm that issue exists so it doesn't get lost.

Nothing to change here.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **✅ Approved** No frontend in this diff — backend lookup logic, repository queries, an ADR, and a README. There are no Svelte components, no markup, no colour tokens, no touch targets, and no copy in my scope here. So this is an LGTM with a note on the *downstream user experience* the change produces. ### What I checked (and why it's fine) The behavioural outcome of the name/sender path is: on an ambiguous filename, **the sender is left unset** rather than guessed. I verified the issue confirms the empty-sender state already degrades cleanly: - `DocumentRow.svelte` already renders the unknown-sender placeholder when sender is null (tested). - `DocumentMetadataDrawer` has a no-persons placeholder. - `metadataComplete=false` already routes the document into the "needs completion" state for a human to assign. So this change cannot produce a broken or confusing empty state — it reuses an existing, tested degradation path. From a senior-reader-on-a-phone perspective, an empty-but-clearly-flagged sender field is *better* than a confidently-wrong pre-filled name they'd never think to correct. The product decision is the accessibility-friendly one. ### Follow-up I want to see filed (not a blocker on this PR) The issue already specs an optional inline hint so the transcriber learns *why* the sender is empty, and it's correctly deferred to a separate low-priority issue. When that gets picked up, the a11y spec from the issue must hold: redundant cue (info icon + text, **not colour-only**), German copy "Absender konnte aus dem Dateinamen nicht eindeutig zugeordnet werden", min 16px, `text-ink` on `bg-surface` (do **not** use `brand-mint` as text — ~2.8:1 fails AA), `aria-describedby` tying the hint to the sender field, and keep "nicht eindeutig zuordenbar" (ambiguous) copy distinct from "kein Absender" (genuinely unknown). Please confirm that issue exists so it doesn't get lost. Nothing to change here.

marcel added 1 commit 2026-06-06 13:36:53 +02:00

docs(person): clarify case-collision scope at the exact-case lookups (#731) ... cd741b9f57

Review noted the "never throws" claim was overstated: the exact-case Optional
lookups still surface a NonUniqueResultException on two byte-identical
same-case rows. That is a true data anomaly out of #731's scope (ambiguous =
case-insensitive) and resolves to the opaque INTERNAL_ERROR, never a wrong
row. Record that boundary at both resolution points and in ADR-032 so the gap
is not silently assumed covered.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

marcel commented 2026-06-06 13:39:27 +02:00

Author

Owner

Copy Link

🔁 Round-2 re-review (clean agent)

Re-reviewed at head cd741b9f against the seven round-1 persona reviews. Verified the source at PersonService.java, PersonRepository.java, docs/adr/032-tag-name-resolution-tolerates-case-collisions.md, the four test files, CI run state, and the ADR file inventory on the base. I did not write this code.

Concern 1 — "Never throws" overstated (Felix / Nora / Sara) — ✅ RESOLVED

A load-bearing scope comment now sits at both exact-case lookups: findByName (PersonService.java:114-116) and findOrCreateByAlias (:144-146), each stating that two byte-identical same-case rows are an out-of-scope data anomaly that the exact Optional surfaces as the opaque INTERNAL_ERROR — never a wrong row. ADR-032 gains a matching "Scope — ambiguous is case-insensitive only" bullet making the same admission. The QA ask (a comment documenting the known gap) is satisfied. Minor residual: commit-1's description line in the PR body still reads "Never throws" unqualified — harmless, since every code/ADR/README surface now scopes the claim to case-collision. Not a blocker.

Concern 2 — ADR-032 number collision (Markus) — ✅ RESOLVED (out of scope, correctly)

This PR touches exactly one 032 file (032-tag-name-resolution-tolerates-case-collisions.md) and only extends it with the Person section — matching issue #731's explicit decision. Confirmed via git diff --name-only origin/main...HEAD. Note for the record: the second file, 032-person-delete-db-level-integrity.md, is already on origin/main (merged via #684, commit 73dd6c80), so the number collision is live on main now — but it predates this branch and is not this PR's to fix. The author handled this exactly right: do not renumber here; the cleanup belongs to a separate housekeeping pass.

Concern 3 — CI green (Sara) — ✅ RESOLVED

Actions run #2066 on ddf378aa (the full implementation commit) completed conclusion=success — full mvn test + 88% JaCoCo gate. Run #2067 on cd741b9f is still in_progress, but that commit is docs/comments-only (ADR + README + in-code comments); the compiled test set and coverage-bearing code are byte-identical to ddf378aa, so the gate result carries.

Concern 4 — Deferred ops: pre-deploy detection SQL + post-deploy GlitchTip (Tobias) — ✅ RESOLVED (out of scope, correctly)

Both are documented in the PR body's "Not done here" section and require prod/observability access. Explicitly deploy-time checklist items, not merge blockers — correctly deferred.

Concern 5 — UX follow-up issue (Leonie / Elicit) — ✅ RESOLVED

Filed as issue #746 (labels feature / ui / P3-later), carrying the a11y spec (redundant cue, German copy, 16px, text-ink on bg-surface, aria-describedby). The UX gap will not fall through.

Fresh sanity pass

• Layering intact: DocumentService.storeDocument resolves the sender via personService.findByName(...), no resolution logic leaked into the document domain.
• Both new repo queries use @Param-bound named parameters (no concatenation); the fail-closed null-first-name semantics (= :firstName, not derived IS NULL) are pinned by a real-Postgres repo test and an integration test.
• The min(Comparator.comparing(Person::getId)).orElseThrow() is provably non-throwing (guarded by !isEmpty()), satisfying the no-bare-.get() rule.
• The create-when-absent branch (INSTITUTION/GROUP + maiden-name alias) is preserved verbatim.
• No new defects found.

Overall verdict: ✅ Approved

All five round-1 concerns are resolved (three directly, two correctly out-of-scope). No blockers remain. Only follow-up that is genuinely actionable in this repo — independent of this PR — is renumbering one of the two live ADR-032 files on main; that is housekeeping for a separate change, not a gate on #745.

## 🔁 Round-2 re-review (clean agent) Re-reviewed at head `cd741b9f` against the seven round-1 persona reviews. Verified the source at `PersonService.java`, `PersonRepository.java`, `docs/adr/032-tag-name-resolution-tolerates-case-collisions.md`, the four test files, CI run state, and the ADR file inventory on the base. I did not write this code. ### Concern 1 — "Never throws" overstated (Felix / Nora / Sara) — ✅ RESOLVED A load-bearing scope comment now sits at **both** exact-case lookups: `findByName` (`PersonService.java:114-116`) and `findOrCreateByAlias` (`:144-146`), each stating that two byte-identical same-case rows are an out-of-scope data anomaly that the exact `Optional` surfaces as the opaque `INTERNAL_ERROR` — never a wrong row. ADR-032 gains a matching **"Scope — ambiguous is case-insensitive only"** bullet making the same admission. The QA ask (a comment documenting the known gap) is satisfied. Minor residual: commit-1's description line in the PR body still reads "Never throws" unqualified — harmless, since every code/ADR/README surface now scopes the claim to case-collision. Not a blocker. ### Concern 2 — ADR-032 number collision (Markus) — ✅ RESOLVED (out of scope, correctly) This PR touches exactly one 032 file (`032-tag-name-resolution-tolerates-case-collisions.md`) and only extends it with the Person section — matching issue #731's explicit decision. Confirmed via `git diff --name-only origin/main...HEAD`. Note for the record: the second file, `032-person-delete-db-level-integrity.md`, is **already on `origin/main`** (merged via #684, commit `73dd6c80`), so the number collision is live on main *now* — but it predates this branch and is not this PR's to fix. The author handled this exactly right: do not renumber here; the cleanup belongs to a separate housekeeping pass. ### Concern 3 — CI green (Sara) — ✅ RESOLVED Actions run **#2066** on `ddf378aa` (the full implementation commit) completed `conclusion=success` — full `mvn test` + 88% JaCoCo gate. Run **#2067** on `cd741b9f` is still `in_progress`, but that commit is docs/comments-only (ADR + README + in-code comments); the compiled test set and coverage-bearing code are byte-identical to `ddf378aa`, so the gate result carries. ### Concern 4 — Deferred ops: pre-deploy detection SQL + post-deploy GlitchTip (Tobias) — ✅ RESOLVED (out of scope, correctly) Both are documented in the PR body's "Not done here" section and require prod/observability access. Explicitly deploy-time checklist items, not merge blockers — correctly deferred. ### Concern 5 — UX follow-up issue (Leonie / Elicit) — ✅ RESOLVED Filed as issue **#746** (labels feature / ui / P3-later), carrying the a11y spec (redundant cue, German copy, 16px, `text-ink` on `bg-surface`, `aria-describedby`). The UX gap will not fall through. ### Fresh sanity pass - Layering intact: `DocumentService.storeDocument` resolves the sender via `personService.findByName(...)`, no resolution logic leaked into the document domain. - Both new repo queries use `@Param`-bound named parameters (no concatenation); the fail-closed null-first-name semantics (`= :firstName`, not derived `IS NULL`) are pinned by a real-Postgres repo test and an integration test. - The `min(Comparator.comparing(Person::getId)).orElseThrow()` is provably non-throwing (guarded by `!isEmpty()`), satisfying the no-bare-`.get()` rule. - The create-when-absent branch (INSTITUTION/GROUP + maiden-name alias) is preserved verbatim. - No new defects found. ### Overall verdict: ✅ Approved All five round-1 concerns are resolved (three directly, two correctly out-of-scope). No blockers remain. Only follow-up that is genuinely actionable in this repo — independent of this PR — is renumbering one of the two live ADR-032 files on `main`; that is housekeeping for a separate change, not a gate on #745.

marcel added 1 commit 2026-06-06 13:52:53 +02:00

docs(adr): renumber tag case-collision ADR 032 → 033 to resolve number clash (#731) ... 7629e35897

Both #730 (tag case-collision) and #684 (person-delete DB integrity) landed
an ADR-032 on main. Renumber the tag/case-collision one to 033 — it is
referenced only from this PR's person-domain comments and its own file, so the
move is self-contained and touches no Flyway migration. The person-delete
ADR-032 and the V71 migration comment that cites it are deliberately left
untouched (editing an applied migration would drift its Flyway checksum).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

marcel merged commit 7629e35897 into main 2026-06-06 14:22:08 +02:00

marcel deleted branch fix/issue-731-person-case-collision 2026-06-06 14:22:08 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

needs-discussion

Has an open decision or design question that must be resolved before implementation can start.

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#745