fix(tag): resolve case-colliding tag names without throwing (#730) #733

Merged

marcel merged 5 commits from fix/issue-730-tag-case-collision into main 2026-06-06 11:38:47 +02:00

Conversation 8 Commits 5 Files Changed 5 +312 -15

marcel commented 2026-06-06 10:59:39 +02:00

Owner

Copy Link

Fixes #730.

Problem

Saving any edit to a document failed with HTTP 500 when the document carried a tag whose name collides case-insensitively with another tag in the canonical tree (e.g. parent Weihnachten + child weihnachten). Every save re-resolves the document's tags by name via TagService.findOrCreate, which did tagRepository.findByNameIgnoreCase(name) → Optional<Tag>. Two case-insensitive matches → NonUniqueResultException → generic 500. ~180 document-tag attachments across 10 canonical tags were un-editable on staging, with no end-user workaround.

The colliding names are legitimate distinct tree nodes (own source_ref, own document attachments) — a lookup bug, not a data bug.

Fix (code-only, zero migration, fully reversible)

TagService.findOrCreate now resolves unambiguously and never throws:

1. findByName(name) — exact-case wins (the edit round-trip replays the stored name),
2. else findAllByNameIgnoreCase(name) → lowest-id match (deterministic tie-break, never throws),
3. else create.

Optional<Tag> findByNameIgnoreCase is deleted so the throwing call can't be reintroduced (its only production caller was findOrCreate). Load-bearing comments at findOrCreate and the repo methods warn against adding a unique(lower(name)) constraint — it would be wrong (collisions are valid) and would fail to apply against the existing rows.

Single source of truth preserved: single-doc edit, bulk-edit, and upload-batch all funnel through resolveTags → findOrCreate, so this one change fixes all three.

Tests

• TagServiceTest (mocked repo) — four resolution branches: exact-case wins; single CI; multiple CI → lowest id asserted across two calls (proves determinism, not luck); create-when-absent; whitespace+collision. The 3 pre-existing findByNameIgnoreCase mocks were updated.
• TagCaseCollisionIntegrationTest (Testcontainers postgres:16-alpine) — saving a doc tagged with child weihnachten keeps exactly the child (id-asserted, not the parent); the Glückwünsche/glückwünsche umlaut pair resolves deterministically without throwing (the regression a plain-ASCII test would miss — only the real DB proves LOWER() folds ü); bulk-edit path guarded.

TagServiceTest 45 ✅ · TagCaseCollisionIntegrationTest 3 ✅ · DocumentServiceTest 187 ✅ · clean package ✅.

Notes

• No DB migration; no tag data touched.
• GlobalExceptionHandler keeps the wire response opaque (no Hibernate/SQL leak) — verified, no new handler needed.
• Sibling sweep → follow-ups filed: #731 (same risk in PersonService.findOrCreateByAlias, latent), #732 (round-trip tag IDs instead of names — the cleaner long-term shape, out of scope here).

🤖 Generated with Claude Code

Fixes #730. ## Problem Saving **any** edit to a document failed with HTTP 500 when the document carried a tag whose name collides **case-insensitively** with another tag in the canonical tree (e.g. parent `Weihnachten` + child `weihnachten`). Every save re-resolves the document's tags by **name** via `TagService.findOrCreate`, which did `tagRepository.findByNameIgnoreCase(name)` → `Optional<Tag>`. Two case-insensitive matches → `NonUniqueResultException` → generic 500. ~180 document-tag attachments across 10 canonical tags were un-editable on staging, with no end-user workaround. The colliding names are **legitimate** distinct tree nodes (own `source_ref`, own document attachments) — a *lookup* bug, not a data bug. ## Fix (code-only, zero migration, fully reversible) `TagService.findOrCreate` now resolves unambiguously and never throws: 1. `findByName(name)` — exact-case wins (the edit round-trip replays the stored name), 2. else `findAllByNameIgnoreCase(name)` → **lowest-id** match (deterministic tie-break, never throws), 3. else create. `Optional<Tag> findByNameIgnoreCase` is **deleted** so the throwing call can't be reintroduced (its only production caller was `findOrCreate`). Load-bearing comments at `findOrCreate` and the repo methods warn against adding a `unique(lower(name))` constraint — it would be wrong (collisions are valid) and would fail to apply against the existing rows. Single source of truth preserved: single-doc edit, bulk-edit, and upload-batch all funnel through `resolveTags → findOrCreate`, so this one change fixes all three. ## Tests - `TagServiceTest` (mocked repo) — four resolution branches: exact-case wins; single CI; **multiple CI → lowest id asserted across two calls** (proves determinism, not luck); create-when-absent; whitespace+collision. The 3 pre-existing `findByNameIgnoreCase` mocks were updated. - `TagCaseCollisionIntegrationTest` (Testcontainers `postgres:16-alpine`) — saving a doc tagged with child `weihnachten` keeps **exactly** the child (id-asserted, not the parent); the **`Glückwünsche`/`glückwünsche` umlaut pair** resolves deterministically without throwing (the regression a plain-ASCII test would miss — only the real DB proves `LOWER()` folds `ü`); bulk-edit path guarded. `TagServiceTest` 45 ✅ · `TagCaseCollisionIntegrationTest` 3 ✅ · `DocumentServiceTest` 187 ✅ · `clean package` ✅. ## Notes - No DB migration; no tag data touched. - `GlobalExceptionHandler` keeps the wire response opaque (no Hibernate/SQL leak) — verified, no new handler needed. - Sibling sweep → follow-ups filed: **#731** (same risk in `PersonService.findOrCreateByAlias`, latent), **#732** (round-trip tag IDs instead of names — the cleaner long-term shape, out of scope here). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 2 commits 2026-06-06 10:59:40 +02:00

fix(tag): resolve case-colliding tag names without throwing (#730) ... d000170f52

findOrCreate used tagRepository.findByNameIgnoreCase, which returns
Optional<Tag> and threw NonUniqueResultException whenever two tags
collided case-insensitively (a canonical parent and its same-named
lowercase child). Every document carrying such a tag became un-editable:
any save re-resolves the whole tag set by name and blew up with a 500.

Replace the throwing lookup with exact-case-first resolution: findByName
(exact) → findAllByNameIgnoreCase (lowest-id, deterministic, never
throws) → create. Delete findByNameIgnoreCase so the throwing call can't
be reintroduced. Case collisions are valid tree nodes — no migration, no
unique(lower(name)) constraint.

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

test(tag): pin case-colliding tag resolution on real Postgres (#730) ... a58378e8f0

Mocked TagServiceTest can't prove the two things that actually broke:
that findAllByNameIgnoreCase folds umlauts the way Postgres LOWER() does,
and that saving a document tagged with a case-colliding tag no longer
throws NonUniqueResultException. Testcontainers postgres:16-alpine:

- updateDocument on a doc tagged with the child "weihnachten" succeeds
  and keeps exactly the child tag (not the parent).
- findOrCreate("GLÜCKWÜNSCHE") resolves the Glückwünsche/glückwünsche
  umlaut pair deterministically (lowest id) without throwing — the
  regression catcher a plain-ASCII pair would miss.
- bulk-edit funnels through resolveTags → findOrCreate, guarding a
  future refactor that bypasses it.

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

marcel commented 2026-06-06 11:01:18 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

I reviewed the four changed files against the TDD discipline, clean-code, and reliability lenses. The fix is small, correct, and the single-source-of-truth funnel (resolveTags → findOrCreate) is preserved exactly as the issue demanded. The deleted findByNameIgnoreCase has no remaining callers anywhere in backend/src (main or test) — I grepped; the only production caller was findOrCreate, so removing it to prevent reintroduction is clean and correct.

Blockers

None that block merge. The behaviour is right and tested.

Suggestions

1. TDD red-phase evidence is asserted in prose, not provable from the diff. TagService.java:67-73 and the rewritten TagServiceTest tests look correct, but the PR rewrites the three existing mocks in the same commit as the production change. Because findByNameIgnoreCase is deleted, the old tests would not have compiled — so there is no moment where the new tests ran red against the old implementation. That is acceptable for a method-signature swap, but the determinism test (findOrCreate_returnsLowestIdDeterministically...) is the one behaviour that genuinely needed a red phase, and I'd want to trust it failed against a naive .get(0) implementation. Consider noting in the PR that the determinism assertion was verified to fail against findFirst()-style code.

2. Optional.get() on line 71 (.min(...).get()) violates our own guard-clause rule (developer.md: "never Optional.get() without guard"). It is safe here because the list is non-empty (guarded by !caseInsensitive.isEmpty() on line 70), but .orElseThrow() would read with intent and survive a future refactor that moves the emptiness check. Minor.

3. Comment density. Lines 68/71/73 carry trailing // comments explaining what the code does. Our style says rename/extract rather than comment the "what". The // never throw and // exact-case wins notes are arguably why comments (they encode the #730 contract), so I'd keep them — but they sit right at the edge of the rule. Fine as-is given the load-bearing nature.

4. Guard-clause style at line 68 (if (exact.isPresent()) return exact.get();) is clean and reads well — good. No nesting. This is the shape I want.

Solid, disciplined fix. Ship after a nod on point 1.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** I reviewed the four changed files against the TDD discipline, clean-code, and reliability lenses. The fix is small, correct, and the single-source-of-truth funnel (`resolveTags → findOrCreate`) is preserved exactly as the issue demanded. The deleted `findByNameIgnoreCase` has **no remaining callers** anywhere in `backend/src` (main or test) — I grepped; the only production caller was `findOrCreate`, so removing it to prevent reintroduction is clean and correct. ### Blockers None that block merge. The behaviour is right and tested. ### Suggestions 1. **TDD red-phase evidence is asserted in prose, not provable from the diff.** `TagService.java:67-73` and the rewritten `TagServiceTest` tests look correct, but the PR rewrites the three existing mocks *in the same commit* as the production change. Because `findByNameIgnoreCase` is deleted, the old tests would not have compiled — so there is no moment where the new tests ran red against the *old* implementation. That is acceptable for a method-signature swap, but the determinism test (`findOrCreate_returnsLowestIdDeterministically...`) is the one behaviour that genuinely needed a red phase, and I'd want to trust it failed against a naive `.get(0)` implementation. Consider noting in the PR that the determinism assertion was verified to fail against `findFirst()`-style code. 2. **`Optional.get()` on line 71** (`.min(...).get()`) violates our own guard-clause rule (developer.md: "never `Optional.get()` without guard"). It is *safe here* because the list is non-empty (guarded by `!caseInsensitive.isEmpty()` on line 70), but `.orElseThrow()` would read with intent and survive a future refactor that moves the emptiness check. Minor. 3. **Comment density.** Lines 68/71/73 carry trailing `//` comments explaining *what* the code does. Our style says rename/extract rather than comment the "what". The `// never throw` and `// exact-case wins` notes are arguably *why* comments (they encode the #730 contract), so I'd keep them — but they sit right at the edge of the rule. Fine as-is given the load-bearing nature. 4. **Guard-clause style** at line 68 (`if (exact.isPresent()) return exact.get();`) is clean and reads well — good. No nesting. This is the shape I want. Solid, disciplined fix. Ship after a nod on point 1.

marcel commented 2026-06-06 11:01:35 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Senior Application Architect

Verdict: ⚠️ Approved with concerns

This is the right architectural call, and I want to say so plainly: the issue correctly rejects the instinct to add a unique(lower(name)) constraint. Normally I push integrity down to PostgreSQL — but here case-collision is a valid domain state (a parent container and its same-named lowercase child are distinct canonical nodes, each with its own source_ref and document attachments). A DB uniqueness constraint would both reject legitimate rows and fail to apply against the existing 10 staging rows, turning a clean code deploy into a failed Flyway migration that blocks startup. The load-bearing comment at TagRepository.java:23-27 documenting this is exactly the kind of "memory of why" I want in the codebase. Good.

The identity model is sound: source_ref (tag_path) is the stable identity per ADR-025; name is a human-editable label. findByNameIgnoreCase was always a leaky identity that only held while names were globally case-insensitively unique — an invariant the canonical tree explicitly violates. Resolving by name is a smell, and the PR honestly names the cleaner long-term shape (round-trip IDs) as out-of-scope follow-up #732. Correct scoping.

Blockers

1. Missing ADR. This is an architectural decision with lasting consequences: "tag-name resolution tolerates case-collision and must never be constrained at the DB layer; tie-break is lowest-id." That is precisely the class of decision my doc-currency table says gets an ADR in docs/adr/. Right now the rationale lives only in a repo comment and the issue body — both of which are easy to lose. A short ADR (Context: canonical tree has valid case-colliding nodes; Decision: exact-case → lowest-id CI → create, no unique(lower(name)); Alternatives: DB constraint [rejected], ID round-trip [deferred to #732]; Consequences: free-text weihnachten binds to the deep child) would lock this in. I treat a missing ADR for a decision of this weight as a blocker, not a concern.

Suggestions

1. Glossary. If "case-colliding tag" / "orphan tag" (the null-sourceRef/parentId tag created on line 73) are not already in docs/GLOSSARY.md, add them — both are now load-bearing domain terms.

2. Layering is respected: DocumentService.resolveTags calls TagService.findOrCreate, never TagRepository. No boundary leak. Good.

No DB diagram update needed — no schema change. Approve once the ADR lands.

## 🏛️ Markus Keller — Senior Application Architect **Verdict: ⚠️ Approved with concerns** This is the right architectural call, and I want to say so plainly: the issue correctly *rejects* the instinct to add a `unique(lower(name))` constraint. Normally I push integrity down to PostgreSQL — but here case-collision is a **valid domain state** (a parent container and its same-named lowercase child are distinct canonical nodes, each with its own `source_ref` and document attachments). A DB uniqueness constraint would both reject legitimate rows *and* fail to apply against the existing 10 staging rows, turning a clean code deploy into a failed Flyway migration that blocks startup. The load-bearing comment at `TagRepository.java:23-27` documenting this is exactly the kind of "memory of why" I want in the codebase. Good. The identity model is sound: `source_ref` (tag_path) is the stable identity per ADR-025; `name` is a human-editable label. `findByNameIgnoreCase` was always a leaky identity that only held while names were globally case-insensitively unique — an invariant the canonical tree explicitly violates. Resolving by name is a smell, and the PR honestly names the cleaner long-term shape (round-trip IDs) as out-of-scope follow-up #732. Correct scoping. ### Blockers 1. **Missing ADR.** This is an architectural decision with lasting consequences: "tag-name resolution tolerates case-collision and must never be constrained at the DB layer; tie-break is lowest-id." That is precisely the class of decision my doc-currency table says gets an ADR in `docs/adr/`. Right now the rationale lives only in a repo comment and the issue body — both of which are easy to lose. A short ADR (Context: canonical tree has valid case-colliding nodes; Decision: exact-case → lowest-id CI → create, no `unique(lower(name))`; Alternatives: DB constraint [rejected], ID round-trip [deferred to #732]; Consequences: free-text `weihnachten` binds to the deep child) would lock this in. **I treat a missing ADR for a decision of this weight as a blocker, not a concern.** ### Suggestions 1. **Glossary.** If "case-colliding tag" / "orphan tag" (the null-`sourceRef`/`parentId` tag created on line 73) are not already in `docs/GLOSSARY.md`, add them — both are now load-bearing domain terms. 2. **Layering** is respected: `DocumentService.resolveTags` calls `TagService.findOrCreate`, never `TagRepository`. No boundary leak. Good. No DB diagram update needed — no schema change. Approve once the ADR lands.

marcel commented 2026-06-06 11:01:57 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

The test strategy is genuinely good: the four resolution branches are unit-tested with a mocked repo (fast layer), and the things a mock cannot prove — Postgres LOWER() folding and the real save round-trip — are pinned with Testcontainers postgres:16-alpine (correct: never H2 for this). The umlaut pair is the right instinct; a plain-ASCII test would stay green while ü/ä folding regressed. The determinism test calling findOrCreate twice and asserting the same id is exactly how you distinguish "deterministic" from "happened to pick this one". I want to highlight that as a model.

Blockers

None.

Suggestions

1. The integration "umlaut" test does NOT actually exercise the multi-row case-insensitive resolution branch with full confidence — verify the seeding. In findOrCreate_resolvesUmlautCollisionDeterministically_withoutThrow, the query is "GLÜCKWÜNSCHE" (all-caps) and the test asserts no exact-case row exists so it falls through to the CI branch. Good. But it leans on assertThat(tagRepository.findAllByNameIgnoreCase("glückwünsche")).hasSize(2) to prove folding. That assertion proves the repository folds correctly, but the subsequent findOrCreate("GLÜCKWÜNSCHE") is a different input string — if Postgres folded Ü→ü for the lowercase query but not for the uppercase one (it won't, but the test doesn't prove symmetry), the two could diverge. Tiny gap. Consider asserting findAllByNameIgnoreCase("GLÜCKWÜNSCHE") (the actual resolution input) also returns 2, so the proof matches the path under test.

2. No @Transactional rollback concern, but watch cross-test tag bleed. The class is @Transactional so each test rolls back — good, deterministic. But all three tests seed Weihnachten/weihnachten or Glückwünsche with the same names; if the rollback ever weakened (e.g. a @Commit slips in, or a nested new-transaction service call), you'd get >2 CI matches and silent flake. The id-asserted assertions would catch it, so acceptable.

3. Missing negative/edge cases at the unit layer: no test for findOrCreate with leading/trailing case-collision where exact-case and a CI sibling both exist but the exact row has the higher id — proving exact-case wins over the lowest-id rule. The exact-case test exists, but not the adversarial "exact-case row is not the lowest id" variant. One more unit test closes it.

4. Bulk-edit test is a good regression guard against a future refactor bypassing findOrCreate — exactly the "thin bulk-edit assertion" the issue asked for. The upload-batch path is only covered by the shared-code comment, which the issue explicitly accepted. Fine.

Strong coverage. Address point 1's symmetry gap and I'm fully satisfied.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** The test strategy is genuinely good: the four resolution branches are unit-tested with a mocked repo (fast layer), and the things a mock *cannot* prove — Postgres `LOWER()` folding and the real save round-trip — are pinned with Testcontainers `postgres:16-alpine` (correct: never H2 for this). The umlaut pair is the right instinct; a plain-ASCII test would stay green while `ü`/`ä` folding regressed. The determinism test calling `findOrCreate` twice and asserting the same id is exactly how you distinguish "deterministic" from "happened to pick this one". I want to highlight that as a model. ### Blockers None. ### Suggestions 1. **The integration "umlaut" test does NOT actually exercise the multi-row case-insensitive *resolution* branch with full confidence — verify the seeding.** In `findOrCreate_resolvesUmlautCollisionDeterministically_withoutThrow`, the query is `"GLÜCKWÜNSCHE"` (all-caps) and the test asserts no exact-case row exists so it falls through to the CI branch. Good. But it leans on `assertThat(tagRepository.findAllByNameIgnoreCase("glückwünsche")).hasSize(2)` to prove folding. That assertion proves the *repository* folds correctly, but the subsequent `findOrCreate("GLÜCKWÜNSCHE")` is a **different** input string — if Postgres folded `Ü`→`ü` for the lowercase query but not for the uppercase one (it won't, but the test doesn't prove symmetry), the two could diverge. Tiny gap. Consider asserting `findAllByNameIgnoreCase("GLÜCKWÜNSCHE")` (the actual resolution input) also returns 2, so the proof matches the path under test. 2. **No `@Transactional` rollback concern, but watch cross-test tag bleed.** The class is `@Transactional` so each test rolls back — good, deterministic. But all three tests seed `Weihnachten`/`weihnachten` or `Glückwünsche` with the *same* names; if the rollback ever weakened (e.g. a `@Commit` slips in, or a nested new-transaction service call), you'd get >2 CI matches and silent flake. The id-asserted assertions would catch it, so acceptable. 3. **Missing negative/edge cases at the unit layer:** no test for `findOrCreate` with leading/trailing case-collision where exact-case *and* a CI sibling both exist but the exact row has the *higher* id — proving exact-case wins over the lowest-id rule. The exact-case test exists, but not the adversarial "exact-case row is not the lowest id" variant. One more unit test closes it. 4. **Bulk-edit test is a good regression guard** against a future refactor bypassing `findOrCreate` — exactly the "thin bulk-edit assertion" the issue asked for. The upload-batch path is only covered by the shared-code comment, which the issue explicitly accepted. Fine. Strong coverage. Address point 1's symmetry gap and I'm fully satisfied.

marcel commented 2026-06-06 11:02:15 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

I reviewed this for injection, exception opacity, authorization, and information disclosure. No security regressions; the change actually closes a small availability/info-leak surface.

What I checked and why it's safe:

1. Exception opacity (the issue's verification item). I traced GlobalExceptionHandler.java. There is no dedicated handler for IncorrectResultSizeDataAccessException / NonUniqueResultException; they fall through to @ExceptionHandler(Exception.class) (line ~100), which returns a fixed ErrorResponse(ErrorCode.INTERNAL_ERROR, "An unexpected error occurred") and routes the stack trace to Sentry via Sentry.captureException(ex). So the Hibernate/SQL detail never reached the wire — the response body was already opaque. The PR's claim is accurate; no new handler is needed. More importantly, this fix removes the throw path entirely, so the DoS-flavoured failure (every save on ~180 doc-tag attachments returning 500) can no longer be triggered by ordinary user input. Net availability improvement.

2. No injection introduced. findByName and findAllByNameIgnoreCase are Spring Data derived queries — parameterized by construction, no JPQL/SQL string concatenation. User-supplied tag names flow through bound parameters only. Clean (CWE-89 not applicable).

3. No mass-assignment / IDOR. Resolution is by name→entity within the document's own tag set; no client-controlled ID is trusted to cross an authorization boundary. The write entry points (updateDocument, applyBulkEditToDocument) retain their existing @RequirePermission(WRITE_ALL) enforcement at the controller — unchanged by this diff.

4. Determinism is also a mild anti-abuse property. Lowest-id tie-break means a malicious actor cannot influence which of two colliding tags they bind to by racing inserts — the oldest row always wins. Fine.

Blockers

None.

Suggestions

1. Consider a one-line regression assertion (Sara's domain, but security-relevant) that the save path returns a mapped error rather than leaking on the generic handler — codifying "this input class never produces a stack-trace body". Optional; the existing opacity is structural.

No CWE findings. Approved.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** I reviewed this for injection, exception opacity, authorization, and information disclosure. No security regressions; the change actually *closes* a small availability/info-leak surface. What I checked and why it's safe: 1. **Exception opacity (the issue's verification item).** I traced `GlobalExceptionHandler.java`. There is **no** dedicated handler for `IncorrectResultSizeDataAccessException` / `NonUniqueResultException`; they fall through to `@ExceptionHandler(Exception.class)` (line ~100), which returns a fixed `ErrorResponse(ErrorCode.INTERNAL_ERROR, "An unexpected error occurred")` and routes the stack trace to Sentry via `Sentry.captureException(ex)`. So the Hibernate/SQL detail never reached the wire — the response body was already opaque. The PR's claim is accurate; no new handler is needed. More importantly, this fix removes the *throw path entirely*, so the DoS-flavoured failure (every save on ~180 doc-tag attachments returning 500) can no longer be triggered by ordinary user input. Net availability improvement. 2. **No injection introduced.** `findByName` and `findAllByNameIgnoreCase` are Spring Data *derived* queries — parameterized by construction, no JPQL/SQL string concatenation. User-supplied tag names flow through bound parameters only. Clean (CWE-89 not applicable). 3. **No mass-assignment / IDOR.** Resolution is by name→entity within the document's own tag set; no client-controlled ID is trusted to cross an authorization boundary. The write entry points (`updateDocument`, `applyBulkEditToDocument`) retain their existing `@RequirePermission(WRITE_ALL)` enforcement at the controller — unchanged by this diff. 4. **Determinism is also a mild anti-abuse property.** Lowest-id tie-break means a malicious actor cannot influence *which* of two colliding tags they bind to by racing inserts — the oldest row always wins. Fine. ### Blockers None. ### Suggestions 1. Consider a one-line regression assertion (Sara's domain, but security-relevant) that the save path returns a mapped error rather than leaking on the *generic* handler — codifying "this input class never produces a stack-trace body". Optional; the existing opacity is structural. No CWE findings. Approved.

marcel commented 2026-06-06 11:02:31 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

This is the kind of fix I like: code-only, zero-migration, fully reversible by rolling back the JAR. No Compose change, no new service, no env var, no schema touch. From an operability standpoint there is nothing that can fail at deploy time that wasn't already there.

What I checked:

1. No Flyway migration in the PR's 4-file scope. Confirmed — and that is the correct decision. A unique(lower(name)) index would have been an operational landmine: it would fail to apply against the existing 10 colliding rows on staging, and a failed migration blocks Spring Boot startup (Flyway runs on boot). That converts a clean rollout into a wedged container. The PR explicitly resisting this is the right call for a single-VPS deployment where a failed migration = downtime.

2. Testcontainers, not H2. TagCaseCollisionIntegrationTest pins postgres:16-alpine via @Import(PostgresContainerConfig.class) — image tag is pinned (good, reproducible) and matches the production engine. This runs in CI integration layer; no shared staging DB dependency, ephemeral container per run. Deterministic. This is exactly how migrations/queries should be verified before they hit prod.

3. Rollback story. Reverting is git revert + redeploy JAR. The deleted findByNameIgnoreCase would come back, but no data has changed, so there is no asymmetric rollback hazard. Clean.

4. Post-deploy verification. The issue lays out a sensible staging smoke check (grab the NonUniqueResultException GlitchTip group, save the 4423782d… repro doc → expect 200, spot-check one doc per colliding tag). I'd add: make that a scripted smoke assertion rather than a manual log-read, consistent with our "automate post-deploy verification" rule. Not a blocker — it's a runbook nicety.

Blockers

None.

Suggestions

1. After deploy, confirm the GlitchTip/Sentry group for IncorrectResultSizeDataAccessException stops receiving new events (the Sentry.captureException in the generic handler will have been feeding it). A silent drop-to-zero is your proof the fix took.

No infra concerns. Approved.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** This is the kind of fix I like: **code-only, zero-migration, fully reversible by rolling back the JAR.** No Compose change, no new service, no env var, no schema touch. From an operability standpoint there is nothing that can fail at deploy time that wasn't already there. What I checked: 1. **No Flyway migration in the PR's 4-file scope.** Confirmed — and that is the *correct* decision. A `unique(lower(name))` index would have been an operational landmine: it would fail to apply against the existing 10 colliding rows on staging, and a failed migration blocks Spring Boot startup (Flyway runs on boot). That converts a clean rollout into a wedged container. The PR explicitly resisting this is the right call for a single-VPS deployment where a failed migration = downtime. 2. **Testcontainers, not H2.** `TagCaseCollisionIntegrationTest` pins `postgres:16-alpine` via `@Import(PostgresContainerConfig.class)` — image tag is pinned (good, reproducible) and matches the production engine. This runs in CI integration layer; no shared staging DB dependency, ephemeral container per run. Deterministic. This is exactly how migrations/queries should be verified before they hit prod. 3. **Rollback story.** Reverting is `git revert` + redeploy JAR. The deleted `findByNameIgnoreCase` would come back, but no data has changed, so there is no asymmetric rollback hazard. Clean. 4. **Post-deploy verification.** The issue lays out a sensible staging smoke check (grab the `NonUniqueResultException` GlitchTip group, save the `4423782d…` repro doc → expect 200, spot-check one doc per colliding tag). I'd add: make that a scripted smoke assertion rather than a manual log-read, consistent with our "automate post-deploy verification" rule. Not a blocker — it's a runbook nicety. ### Blockers None. ### Suggestions 1. After deploy, confirm the GlitchTip/Sentry group for `IncorrectResultSizeDataAccessException` stops receiving new events (the `Sentry.captureException` in the generic handler will have been feeding it). A silent drop-to-zero is your proof the fix took. No infra concerns. Approved.

marcel commented 2026-06-06 11:02:51 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer & Business Analyst

Verdict: ⚠️ Approved with concerns

I assessed this against the issue's acceptance criteria (the three Gherkin scenarios) and the requirements-completeness lens — does the implementation satisfy what was specified, and are the unspecified behaviours surfaced rather than silently resolved?

Acceptance-criteria traceability — all three map to tests:

• "Saving a document tagged with a case-colliding tag succeeds" → updateDocument_succeedsAndKeepsExactChildTag_whenTaggedWithCaseCollidingChild asserts both no-throw and the surviving tag is the child's id, not the parent's. The criterion said "keeps exactly the weihnachten tag (not Weihnachten)" — the test asserts hasSize(1) + id equality. ✅ Fully covered, including the "No 500 alone is too weak" caveat.
• "Exact-case match wins over a case-insensitive sibling" → findOrCreate_exactCaseWins_overCaseInsensitiveSibling. ✅
• "findOrCreate never throws on case-insensitive duplicates → same one deterministically (lowest id)" → findOrCreate_returnsLowestIdDeterministically... (twice-called, same-id). ✅

Every Must in the issue is traceable to a named test. That is the standard I want.

Blockers

None — no acceptance criterion is unmet.

Concerns / Open Questions (surfacing, not resolving)

1. Free-text authoring semantics are an accepted-but-user-invisible behaviour. The issue's "Decision (option A)" explicitly accepts that typing bare weihnachten in new-doc/bulk-edit binds to the deep child (Weihnachten/weihnachten), while Weihnachten binds to the parent container. That is a latent usability cliff: an author cannot tell, from the typeahead, that they are tagging a leaf vs. a container. The issue correctly defers the typeahead/chip disambiguation to a follow-up — but I want to confirm a follow-up issue actually exists for "show tree path in tag typeahead so a container is distinguishable from its same-named child." The PR notes #731 (PersonService sibling) and #732 (ID round-trip) but I do not see the typeahead-disambiguation follow-up referenced. If it's unfiled, it will be lost. Please file it or point me at it.

2. Observability acceptance item is unverified in-PR. The issue lists a verification step: confirm no new NonUniqueResultException occurrences post-deploy on staging via the GlitchTip group + the 4423782d… repro. That is a real acceptance gate that cannot be ticked from the diff alone — it requires the deploy + proofshot. Flagging it as an open item against "done," not against the code.

3. No ambiguity in the Must requirements — the three scenarios are testable and unambiguous. Good. The single residual ambiguity (free-text binding direction) was consciously resolved by the product owner as Option A, documented in the issue. I accept it as a deliberate decision, not a gap.

Approve the code; please confirm the typeahead follow-up is filed so concern #1 doesn't evaporate.

## 📋 Elicit — Requirements Engineer & Business Analyst **Verdict: ⚠️ Approved with concerns** I assessed this against the issue's acceptance criteria (the three Gherkin scenarios) and the requirements-completeness lens — does the implementation satisfy what was specified, and are the unspecified behaviours surfaced rather than silently resolved? **Acceptance-criteria traceability — all three map to tests:** - *"Saving a document tagged with a case-colliding tag succeeds"* → `updateDocument_succeedsAndKeepsExactChildTag_whenTaggedWithCaseCollidingChild` asserts both no-throw **and** the surviving tag is the child's id, not the parent's. The criterion said "keeps exactly the `weihnachten` tag (not `Weihnachten`)" — the test asserts `hasSize(1)` + id equality. ✅ Fully covered, including the "No 500 alone is too weak" caveat. - *"Exact-case match wins over a case-insensitive sibling"* → `findOrCreate_exactCaseWins_overCaseInsensitiveSibling`. ✅ - *"findOrCreate never throws on case-insensitive duplicates → same one deterministically (lowest id)"* → `findOrCreate_returnsLowestIdDeterministically...` (twice-called, same-id). ✅ Every Must in the issue is traceable to a named test. That is the standard I want. ### Blockers None — no acceptance criterion is unmet. ### Concerns / Open Questions (surfacing, not resolving) 1. **Free-text authoring semantics are an accepted-but-user-invisible behaviour.** The issue's "Decision (option A)" explicitly accepts that typing bare `weihnachten` in new-doc/bulk-edit binds to the *deep child* (`Weihnachten/weihnachten`), while `Weihnachten` binds to the parent container. That is a **latent usability cliff**: an author cannot tell, from the typeahead, that they are tagging a leaf vs. a container. The issue correctly defers the typeahead/chip disambiguation to a follow-up — but I want to confirm **a follow-up issue actually exists** for "show tree path in tag typeahead so a container is distinguishable from its same-named child." The PR notes #731 (PersonService sibling) and #732 (ID round-trip) but I do **not** see the typeahead-disambiguation follow-up referenced. If it's unfiled, it will be lost. Please file it or point me at it. 2. **Observability acceptance item is unverified in-PR.** The issue lists a verification step: confirm no new `NonUniqueResultException` occurrences post-deploy on staging via the GlitchTip group + the `4423782d…` repro. That is a real acceptance gate that cannot be ticked from the diff alone — it requires the deploy + proofshot. Flagging it as an open item against "done," not against the code. 3. **No ambiguity in the Must requirements** — the three scenarios are testable and unambiguous. Good. The single residual ambiguity (free-text binding direction) was consciously resolved by the product owner as Option A, documented in the issue. I accept it as a deliberate decision, not a gap. Approve the code; please confirm the typeahead follow-up is filed so concern #1 doesn't evaporate.

marcel commented 2026-06-06 11:03:05 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved (LGTM — out of my domain, with one downstream flag)

This is a backend-only change: four files touching TagService, TagRepository, and their tests. There is no Svelte component, no markup, no CSS token, no message-catalog (messages/{de,en,es}.json) change in this PR's scope. So there is nothing here for me to assess on contrast, touch targets, focus indicators, semantic HTML, or 320px layout. I checked specifically for any frontend or i18n surface in the diff and found none.

What I can speak to is the user-facing experience this fix restores, and it's a meaningful win for our audience:

• Before: a 67-year-old transcriber editing a weihnachten-tagged letter changed the date, hit save, and got the opaque generic error ("Ein unerwarteter Fehler ist aufgetreten") with no hint that a tag was the cause and no workaround. That is a Critical-severity failure by my rubric — it blocks the core task and the error message fails Nielsen heuristic #9 (help users diagnose and recover). This PR removes the failure entirely. Good.

Blockers

None in my domain.

Suggestion (downstream, not this PR)

1. The deferred typeahead-disambiguation follow-up is a UX issue, and I want eyes on it when it lands. With Option A, an author typing weihnachten silently binds to the deep child and Weihnachten to the parent container — but the tag chip and typeahead give zero visual cue which one they got. For our senior authors especially, "looks the same, behaves differently" is a recognition-over-recall failure (heuristic #6). When that follow-up is built, the fix is concrete: show the tree path in the typeahead row and on the chip (e.g. Weihnachten / weihnachten with the parent segment in text-ink-3), so a container is visually distinct from its same-named leaf. Tag that issue for my review.

Nothing to change here. Approved.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved (LGTM — out of my domain, with one downstream flag)** This is a backend-only change: four files touching `TagService`, `TagRepository`, and their tests. There is no Svelte component, no markup, no CSS token, no message-catalog (`messages/{de,en,es}.json`) change in this PR's scope. So there is nothing here for me to assess on contrast, touch targets, focus indicators, semantic HTML, or 320px layout. I checked specifically for any frontend or i18n surface in the diff and found none. What I *can* speak to is the **user-facing experience this fix restores**, and it's a meaningful win for our audience: - Before: a 67-year-old transcriber editing a `weihnachten`-tagged letter changed the date, hit save, and got the opaque generic error ("Ein unerwarteter Fehler ist aufgetreten") with **no hint** that a tag was the cause and **no workaround**. That is a Critical-severity failure by my rubric — it blocks the core task and the error message fails Nielsen heuristic #9 (help users diagnose and recover). This PR removes the failure entirely. Good. ### Blockers None in my domain. ### Suggestion (downstream, not this PR) 1. **The deferred typeahead-disambiguation follow-up is a UX issue, and I want eyes on it when it lands.** With Option A, an author typing `weihnachten` silently binds to the deep child and `Weihnachten` to the parent container — but the tag chip and typeahead give **zero visual cue** which one they got. For our senior authors especially, "looks the same, behaves differently" is a recognition-over-recall failure (heuristic #6). When that follow-up is built, the fix is concrete: show the tree path in the typeahead row and on the chip (e.g. `Weihnachten / weihnachten` with the parent segment in `text-ink-3`), so a container is visually distinct from its same-named leaf. Tag that issue for my review. Nothing to change here. Approved.

marcel referenced this pull request 2026-06-06 11:09:59 +02:00

UX: Tag-Typeahead/Chips zeigen den Baumpfad, um gleichnamige Eltern/Kind-Tags zu unterscheiden (Follow-up zu #730) #734

marcel referenced this issue from a commit 2026-06-06 11:10:42 +02:00

test(tag): close review-flagged gaps in case-collision coverage (#730)

marcel added 3 commits 2026-06-06 11:10:42 +02:00

refactor(tag): use orElseThrow over Optional.get in findOrCreate (#730) ... 80f6468d52

The lowest-id tie-break stream is guarded non-empty, so .get() never
throws — but the project bans Optional.get(). Switch to .orElseThrow()
for the project idiom. No behaviour change.

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

test(tag): close review-flagged gaps in case-collision coverage (#730) ... 2710f2e233

Two adversarial gaps from PR #733 review:

- Unit: exact-case must win even when its id is NOT the lowest, proving
  exact-case short-circuits before the lowest-id tie-break (a naive
  "lowest id across all CI matches" would pick the wrong row).
- Integration: assert findAllByNameIgnoreCase folds the UPPERCASE
  "GLÜCKWÜNSCHE" — the exact string findOrCreate passes — so the umlaut
  proof matches the resolution path under test, not a lowercase probe.

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

docs(adr): ADR-032 tag-name resolution tolerates case-collisions (#730) ... bafbf609eb

Records the lasting decision behind the #730 fix: exact-case-first
resolution, deterministic lowest-id case-insensitive fallback, and the
explicit refusal of a unique(lower(name)) constraint (collisions are
valid canonical nodes). Previously the rationale lived only in code
comments and the issue body. Raised as a blocker in the PR #733 review.

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

marcel referenced this issue from a commit 2026-06-06 11:10:42 +02:00

docs(adr): ADR-032 tag-name resolution tolerates case-collisions (#730)

marcel commented 2026-06-06 11:10:55 +02:00

Author

Owner

Copy Link

Review feedback addressed ✅

Thanks all. Pushed 80f6468d..bafbf609.

Markus (Architect) — 🚫→ blocker resolved

• ADR added — bafbf609 docs/adr/032-tag-name-resolution-tolerates-case-collisions.md. Records exact-case-first resolution, the deterministic lowest-id fallback, and the explicit refusal of a unique(lower(name)) constraint, with consequences/alternatives. The decision no longer lives only in code comments + the issue.

Sara (QA)

• Umlaut proof now matches the path under test — 2710f2e2. The precondition asserts findAllByNameIgnoreCase("GLÜCKWÜNSCHE") (the uppercase string findOrCreate actually passes), so it exercises LOWER('GLÜCKWÜNSCHE') folding rather than a lowercase probe.
• Exact-case-not-lowest-id test added — 2710f2e2, findOrCreate_exactCaseWins_evenWhenItsIdIsNotTheLowest: the exact row carries the higher id and findAllByNameIgnoreCase is verified never called, proving exact-case short-circuits before the lowest-id rule (a naive "lowest id across all CI matches" would fail this).

Felix (Dev)

• .get() → .orElseThrow() — 80f6468d. The stream is guarded non-empty, but this honours the project's no-Optional.get() rule.
• On the red-phase note: the documented real red was findOrCreate_exactCaseWins… (asserted expected geburt but was null before the rewrite). The determinism/multi-CI branch was written in the green step, not isolated-red — being transparent: its red wasn't separately captured. The new evenWhenItsIdIsNotTheLowest test plus the verified never() interaction now pin the ordering adversarially.

Elicit + Leonie (Reqs/UX)

• Typeahead/chips tree-path disambiguation filed — #734 (ui, P3). Joins #731 (sibling Person bug) and #732 (round-trip tag IDs).

Verification: TagServiceTest 46 ✅ · TagCaseCollisionIntegrationTest 3 ✅ · clean package ✅.

## Review feedback addressed ✅ Thanks all. Pushed `80f6468d..bafbf609`. ### Markus (Architect) — 🚫→ blocker resolved - **ADR added** — `bafbf609` `docs/adr/032-tag-name-resolution-tolerates-case-collisions.md`. Records exact-case-first resolution, the deterministic lowest-id fallback, and the explicit refusal of a `unique(lower(name))` constraint, with consequences/alternatives. The decision no longer lives only in code comments + the issue. ### Sara (QA) - **Umlaut proof now matches the path under test** — `2710f2e2`. The precondition asserts `findAllByNameIgnoreCase("GLÜCKWÜNSCHE")` (the uppercase string `findOrCreate` actually passes), so it exercises `LOWER('GLÜCKWÜNSCHE')` folding rather than a lowercase probe. - **Exact-case-not-lowest-id test added** — `2710f2e2`, `findOrCreate_exactCaseWins_evenWhenItsIdIsNotTheLowest`: the exact row carries the *higher* id and `findAllByNameIgnoreCase` is verified never called, proving exact-case short-circuits before the lowest-id rule (a naive "lowest id across all CI matches" would fail this). ### Felix (Dev) - **`.get()` → `.orElseThrow()`** — `80f6468d`. The stream is guarded non-empty, but this honours the project's no-`Optional.get()` rule. - **On the red-phase note:** the documented real red was `findOrCreate_exactCaseWins…` (asserted `expected geburt but was null` before the rewrite). The determinism/multi-CI branch was written in the green step, not isolated-red — being transparent: its red wasn't separately captured. The new `evenWhenItsIdIsNotTheLowest` test plus the verified `never()` interaction now pin the ordering adversarially. ### Elicit + Leonie (Reqs/UX) - **Typeahead/chips tree-path disambiguation filed** — **#734** (ui, P3). Joins #731 (sibling Person bug) and #732 (round-trip tag IDs). Verification: `TagServiceTest` 46 ✅ · `TagCaseCollisionIntegrationTest` 3 ✅ · `clean package` ✅.

marcel merged commit bafbf609eb into main 2026-06-06 11:38:47 +02:00

marcel deleted branch fix/issue-730-tag-case-collision 2026-06-06 11:38:47 +02:00

marcel referenced this pull request 2026-06-06 12:19:22 +02:00

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

marcel referenced this pull request 2026-06-06 12:20:02 +02:00

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

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#733