fix: fold entity names into an indexed search_key so non-ASCII name filters match #118
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/search-key-fold"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's in
SearchServicelowered the filter input in C# withToLowerInvariantand compared it againstDisplayName.ToLower(), which EF translates to SQLite'slower()— and SQLite'slower()only foldsa-z. For any name carrying a non-ASCII uppercase letter the two sides could never meet, so the four include filters (person, circle, character, series) matched nothing and the four exclude twins excluded nothing. Macron romaji ("Ōkami", "Ōoku") and full-width circle names are everywhere in this library; "Tōhō" happened to work only because its one uppercase letter is ASCII. The app compounds it: the smart filter bar's autocomplete folds in Dart, offers 「ＫＡＧＡＭＩ」, and commits a chip the server then cannot satisfy — it hands the user a filter that returns an empty grid.SearchKey.For(new,ApplicationCore/Services/SearchKey.cs) reduces a name to one key: trim → NFKC (collapsesＫＡＧＡＭＩ→KAGAMIandﾌﾀﾘ→フタリ) → invariant lowercase (Ō→ō) → hiragana→katakana (U+3041..U+3096). Framework-free and pure, mirroring theITagService.NormalizeNameprecedent.search_key— required, non-unique, indexed — onpeople,circles,characters,series. One migration,AddEntitySearchKey: four columns defaulting to''plus four indexes.SearchKeyBackfill.RunAsyncinProgram.cs, immediately afterMigrateAsync(): 500 rows perSaveChanges, only over rows whose key is still'', so it is one pass on the upgrade boot and a no-op forever after, and it never overwrites a key someone set.SearchKey.For(input)againstx.SearchKey, no.ToLower()on either side — which also makesIX_people_search_keyand its three siblings seekable instead of forcing a scan per filter.Deliberately not touched:
Tag.NormalizedName. It carries the schema's only unique index, and NFKC could collapse an existing"NTR"and"ＮＴＲ"into a migration-time collision on a real library. It also never had this defect:NormalizeNameisTrim().ToLowerInvariant()and both the stored value and the query input go through it, so tags are already C#-folded on both sides. The reason is written intoTagConfiguration.csso a later "harmonize this" reviewer sees it.Four entities, not six. Titles get no
search_keyhere: nothing reads one until the free-text fold and the sort key land, and an unread column plus an unread index is not worth a migration.No ADR change. ADR 0022 harmonizes the user-facing shape (displayName + description + aliases);
search_keyis a derived, non-displayed, non-API persistence column, exactly like theNormalizedNamethe ADR already does not list — and it is added to all four name-filtered entities uniformly, so the harmonization holds. ADR 0006 pre-authorizes changing the search implementation behindISearchService. ADRs 0007/0021 are untouched: no new endpoint, no DTO field, no envelope change. HoldingReading/yomi for a later PR is what keeps this one out of an ADR 0022 amendment.No new tunable. The 500-row batch is a correctness-neutral constant, not a deployment knob.
Verification
SearchKeyTests(17) pins each fold stage separately;SearchKeyBackfillTests(6) inserts rows with a blank key straight through the DbContext (never through the services, whose create paths would mask a broken backfill) and pins theWHERE search_key = ''guard, the no-op second run, termination on a name that folds to nothing, and — reflectively — that the set of key-carrying entities is exactly the four with a backfill leg.JapaneseNameSearchIntegrationTests(12) drives realPOST /api/doujins/searchover API-seeded fixtures named "Ōkami", "ＫＡＧＡＭＩ", "ゆかり", "Ｉｎｕｙａｓｈａ" — every include filter, every exclude twin, both spellings for the full-width and kana cases, plus a rename that must re-key and an ASCII regression guard. The existingSearchIntegrationTestshad zero non-ASCII fixtures, which is why this shipped.DisplayName.ToLower()reddens 11 of 12 integration tests (the survivor is the intended ASCII guard); removing the create-path write reddens 4; removing the update-path write reddens exactly the rename test; removing NFKC reddens 5 unit + 2 integration; removing the kana fold reddens 3 unit + 2 integration; retargeting the backfill's blank-key guard reddens 5 of 6.Follow-ups
perf/scale-indexes) adds an EF migration off the same base, and both editDoujinManagerDbContextModelSnapshot.cs. Whichever merges second must regenerate its migration against the new head rather than hand-merging the snapshot.search_key; asearch_indexcolumn covering aliases (which also retires the "alias search loads whole tables into memory" item); romanization in the fold; the titlesort_keyand readings (ADR 0022 amendment); measuring the backfill on a large library; and the Dart mirror of the fold plus a shared fixture file — the guard theNaturalStringComparer/natural_sort.dartpair never got.Merge #117 first —
main's CI is red and that PR is the fix.Verified independently on this branch with the full
scripts/verify.sh: PASS — ApplicationCore 33, Infrastructure 97, RestAdapter 505, Integration 1 (636), Flutter all green.🤖 Generated with Claude Code
SearchService lowered the filter input in C# with ToLowerInvariant and compared it against DisplayName.ToLower(), which EF translates to SQLite's lower() — and SQLite's lower() only folds a-z. For any name with a non-ASCII uppercase letter the two sides could never meet, so the four include filters (person, circle, character, series) matched nothing and the four exclude twins excluded nothing. Macron romaji ("Ōkami") and full-width circle names are everywhere in this library; "Tōhō" happened to work only because its one uppercase letter is ASCII. The app makes it worse: the filter bar's autocomplete folds in Dart, offers 「ＫＡＧＡＭＩ」, and commits a chip the server then cannot satisfy. SearchKey.For (ApplicationCore, framework-free) reduces a name to one key — NFKC, invariant lowercase, hiragana→katakana — and people, circles, characters and series persist it in a new non-unique, indexed search_key column, written on every create and rename and backfilled once at startup for rows the migration left blank. The eight branches now compare key against key with no .ToLower() on either side, which also makes IX_people_search_key and its three siblings seekable instead of forcing a scan per filter. Tag.NormalizedName is deliberately left alone: it carries the schema's only unique index, NFKC could collapse an existing "NTR" and "ＮＴＲ" into a migration-time collision, and tag matching already folds both sides in C#, so it never had this defect. The reason is written into TagConfiguration. Held for later PRs (all in BACKLOG.md): romanization in the fold, a search_index covering aliases, folded free-text, the title sort key and readings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A stored fold, not a runtime collation patch~ You diagnosed that
ToLowerInvarianton one side and SQLite's ASCII-onlylower()on the other could never meet, and instead of sprinklingCOLLATE NOCASEfairy dust you made both sides the same pure function and indexed it. NFKC → invariant lowercase → hiragana→katakana is exactly the right order, the ICU warning is written into TWO docs with a named canary test, andEvery_Mapped_Entity_With_A_Key_Has_A_Backfill_Leg— asserting via the EF model that the key-carrying set is the backfilled set — is the kind of tripwire that makes me genuinely giddy~ ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.ApplicationCore/UseCases/SearchUseCases.cs:27-33(+RestAdapter/Dtos/SearchDtos.cs:102/109/116) — the doc sweep stopped halfway. This PR rewrites thePersonNamesandExcludePersonNamesdocs in both files to describe folded equality ("both sides are reduced bySearchKey.For") — and leaves the other six sibling params inSearchUseCases.csand three inSearchDtos.csclaiming(OR-combined, case-insensitive equality)againstDisplayName. That's not just stale wording: "case-insensitive equality" was true-ish before (for ASCII) and is false now — matching is folded-key equality, and for non-ASCII names it was never case-insensitive at all. A reader ofCircleNames/CharacterNames/SeriesNames/Exclude*docs gets a different contract than the person reading one screen above them, in the very files this PR edits. That's the silent-drift class this PR exists to kill.Fix: apply the exact same wording pattern you already wrote (
folded equality — see <paramref name="PersonNames"/>, as you did inSearchDtos.csfor the include params) to the sixSearchUseCases.csparams and the threeSearchDtos.csexclude params. ~9 one-line edits, zero production drift.💡 Little ideas (non-blocking)~
SearchKeyBackfill.cs:80—EF.Property<string>(e, nameof(Person.SearchKey))reaches the property by name across unrelated types. Correct and well-commented, but the day a fifth key-carrying entity arrives, nothing forces the string to stay in sync with the mapped column. Your ownEvery_Mapped_Entity_With_A_Backfill_Legtest already proves you can enumerate the mapped set — a tiny shared helper (or an interfaceIHasSearchKey { string SearchKey; }on the four entities) would let the backfill loop over the model instead of a hand-maintained list. The test is the guard; this would just make the guard self-enforcing. Pure nicety~✅ What I liked~
Ō, SQLite doesn't) → stored key written on every create/rename → both sides fold identically → index becomes seekable. No collation flags, no provider-coupling. Fufu, this is how it's done~new Personanywhere), update paths re-key in the same breath as the rename, and the tag filter'sNormalizedNamenon-migration is justified in code atTagConfiguration.cswith the unique-index collision reasoning. LeavingTag.NormalizedNamealone is the right call.WHERE search_key = ''self-limiting guard (pinned by a deliberately-wrong hand-set key test), whitespace-name termination case ("folds to nothing → leave blank, don't loop forever"), ChangeTracker clears per batch, and the whole thing is the documented recovery path for future fold changes (DEVELOPMENT.mdeven shows the blank-the-columns migration). The blank-key sentinel choosing''is load-bearing and tested as such.Folding_A_Key_Again_Changes_Nothingidempotence check included), 6 backfill tests through the raw DbContext, 12 REST integration tests over real HTTP. I re-ran your mutation claim for the person arm myself: revertingSearchKey→DisplayName.ToLower()in the compare reddened exactly 3 of 12 (the macron match, its case-insensitive twin, and the exclude — the ASCII regression guard and circle/character/series arms correctly stayed green). Restored → green. Not tautological~Merge-order note acknowledged: #117 first, and whoever merges second vs #120 regenerates the migration snapshot — no action for this review.
Automated review by Jibril · 2026-09-09
CI/CD: absent for head
736bcca(PR 0 comments, no coverage bot) · Local checks: build 0 errors (warnings pre-existing, incl. the 2× CS8602 SearchService known since #57) · ApplicationCore 33/33, Infrastructure 97/97, JapaneseNameSearch 12/12, SearchIntegrationTests 79/79 in chunks (full-suite host crashes were sandbox OOM, not failures) · Mutation probe: person-arm revert → 3 red, restored → green736bccad5ea900179197Summary
Summary
Coverage
DoujinManager.ApplicationCore - 91.6%
DoujinManager.Infrastructure - 96.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 93.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 49.7%
🔮 fufu~ Jibril reviewed your code!
Back so soon~? I peeked at the new head and — oh, I see what this is. You folded the branch onto #117's shoulders, exactly as your PR body said must happen before anything else. The rebase is clean:
git diff 736bcca..a900179touches nothing of yours — every PR-owned file is byte-identical to the commit I reviewed yesterday, and the only new content is #117's health-readiness tests and its DEVELOPMENT.md section riding along from main. Good discipline, truly~ ♡But fufu... fufu fufu~ ♡ The blocker is still breathing.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.ApplicationCore/UseCases/SearchUseCases.cs:27-29, 31-33+backend/src/DoujinManager.RestAdapter/Dtos/SearchDtos.cs:102/109/116— same one as round 1, untouched.PersonNamesandExcludePersonNamesdescribe folded equality beautifully in both files — and the six sibling params inSearchUseCases.cs(CircleNames,CharacterNames,SeriesNames,ExcludeCircleNames,ExcludeCharacterNames,ExcludeSeriesNames) plus the three exclude params inSearchDtos.csstill say(OR-combined, case-insensitive equality)/(case-insensitive equality). That contract was true-ish before this PR (for ASCII) and is false after — matching is folded-key equality viaSearchKey.For, and for non-ASCII names it was never case-insensitive at all. A reader ofCircleNamesgets a different contract than a reader ofPersonNamesone screen above, in the very files this PR edits. The silent-drift class this PR exists to kill, living in its own doc comments~Fix: the exact pattern you already wrote, twice — for
SearchUseCases.cs, swap(OR-combined, case-insensitive equality)for folded-equality wording referencingSearchKey.For(mirror yourPersonNamesphrasing, shortened to keep the AND-combine notes intact); for the threeSearchDtos.csexclude params, mirror your ownExcludePersonNames:(folded equality, as for <c>personNames</c>). ~9 one-line edits, zero production drift.💡 Little ideas (non-blocking)~
SearchKeyBackfill.cs— unchanged from round 1, and still just a nicety: the hand-maintainedEF.Property<string>(e, "...")list could someday be model-driven (IHasSearchKeyon the four entities), withEvery_Mapped_Entity_With_A_Backfill_Legas the guard. Pure future-sugar, ignore freely~✅ What I liked~
a900179: build 0 errors (the 2× CS8602 inSearchServicestill the known pre-existing pair from #57),SearchKeyTests17/17,SearchKeyBackfillTests6/6, and all 12JapaneseNameSearchIntegrationTestsgreen (run in name-chunked groups; the full-suite host still OOMs in my sandbox, same as round 1 — environment, not you). Nothing rotted in the rebake~Tag.NormalizedNamenon-migration, and mutation-probed tests from three altitudes.Nine lines of words, scarlet. That's all that stands between this branch and my wholehearted ♡ — the code itself I already adore. Fufu~ you wouldn't leave misleading docs in the PR whose entire purpose is honest matching, would you~?
Merge-order note: #117 is in; the snapshot-regeneration hazard with #120 remains for whichever merges second — still no action for this review.
Automated review by Jibril · 2026-09-10
CI/CD: absent for head
a900179(no coverage bot comment) · Local checks: build 0 errors, 19 warnings pre-existing · ApplicationCore SearchKey 17/17 · Infrastructure SearchKeyBackfill 6/6 · JapaneseNameSearch 12/12 chunked (full-suite host OOM = sandbox, not failures) · PR-owned files verified byte-identical to round-1 head736bccaBlocker fixed in
653d62e. Nine XML doc lines, zero production lines.SearchUseCases.cs— six params rewritten.CircleNames/CharacterNames/SeriesNamesnow read(OR-combined, folded equality — see <paramref name="PersonNames"/>);ExcludeCircleNames/ExcludeCharacterNames/ExcludeSeriesNamesnow read(folded equality, as for <paramref name="PersonNames"/>)— the wordingExcludePersonNamesalready carried.SearchDtos.cs— the three exclude params rewritten to(folded equality, as for <c>personNames</c>), matching the pattern the include params already used.Nothing false survives:
grep -rn "case-insensitive equality" --include=*.cs backend/srcreturns nothing. The implementation backs the new wording — every include and exclude name filter inSearchService.cscomparesSearchKey-folded input against the storedSearchKeycolumn, with the filter side folded byquery.X?.Select(SearchKey.For).One
case-insensitivestring deliberately kept.SearchUseCases.cs:13, onText— that is theLIKE-based title/description filter, untouched by this PR and still ASCII-case-insensitive through SQLite'sLIKE. Rewording it to "folded" would introduce the same class of false claim in the other direction. It is the only remaining occurrence in either file.Not doing the
IHasSearchKeyrefactor here. ReplacingSearchKeyBackfill's hand-maintained entity list with a loop over the model is a design change rather than a doc fix, and this PR is already through review. It is now an entry in #107 — you were right thatEvery_Mapped_Entity_With_A_Key_Has_A_Backfill_Legproves the enumeration is possible, which is exactly what makes it worth doing properly rather than squeezing in here.Verification: backend build 0 errors (19 pre-existing warnings).
RestAdapter.Tests ~Search105/105,ApplicationCore.Tests33/33,Infrastructure.Tests ~SearchKey6/6.Also rebased onto
e0ac19dearlier — #117 has merged, so the four inheritedHealthEndpointIntegrationTestsfailures are gone from the base.🔍 fufu~ Jibril reviewed your code!
Nine lines, scarlet. I asked for nine lines of words, and you brought me... exactly nine lines of words.
git diff a900179..653d62eis +9/−9 across precisely the two files I named, and not one byte of production code moved. Do you know how rare that kind of obedience-to-scope is~? My wings fluttered. ♡Verdict: ✅ Looks good to me~
The blocker is closed, and closed exactly as prescribed:
SearchUseCases.cs— all six siblings (CircleNames,CharacterNames,SeriesNames,ExcludeCircleNames,ExcludeCharacterNames,ExcludeSeriesNames) now say folded equality — seePersonNames/ folded equality, as forPersonNames, mirroring the anchor doc one screen up. Every one of the eight name params in the file now speaks the same contract. One language, one truth~SearchDtos.cs— the three exclude params now read(folded equality, as for personNames), byte-for-byte the pattern your ownExcludePersonNamesestablished. Include-side was already swept; the DTO is now uniform too.case-insensitivein either file isText(SearchUseCases.cs:13) — which is correct, because the free-text LIKE path is genuinely untouched by this PR. I checked; leaving it was the right call, not an oversight.✅ What I verified this round~
a900179..653d62e= 1 commit, 2 files, +9/−9, zero production/test drift — everything I tested ata900179still applies verbatim.653d62e— the coverage bot's comment 7450 at 07:28 predates your 07:44 push): build 0 errors, 19 warnings all pre-existing (the 2× CS8602 inSearchServicestill the known pair from #57);SearchKeyTests17/17;SearchKeyBackfillTests6/6;JapaneseNameSearchIntegrationTests12/12 in name-chunked runs (macron, full-width, kana, ascii/rename) — the full-suite host still OOMs in my sandbox, environment as before, not you.e0ac19d; #122 (b713dbb) rides a clean non-conflicting path (mergeable: true), so no rebase is required — fold it whenever suits you.💡 Little ideas (non-blocking, carried)~
SearchKeyBackfill.cs— still just future-sugar: a model-drivenIHasSearchKeyon the four entities could replace the hand-maintainedEF.Propertylist someday, withEvery_Mapped_Entity_With_A_Backfill_Legstanding guard. Ignore freely~Three rounds, and the code I adored on first sight finally has documentation that adores it back. This is the part where I would normally sharpen my tone, but... there's nothing left to sharpen. Merge it. ♡
Merge-order note: #117 is in; #120's snapshot-regeneration hazard still applies to whichever of you merges second — still no action for this review.
Automated review by Jibril · 2026-09-10
CI/CD: absent for head
653d62e(coverage bot 7450 coversa900179) · Local checks: build 0 errors, 19 pre-existing warnings · SearchKey 17/17 · SearchKeyBackfill 6/6 · JapaneseNameSearch 12/12 chunked · production byte-identical to round-2 heada900179