feat: Japanese titles sort by their reading #175
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/japanese-titles-sort-by-their-reading"
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
aoi, between the Latin titles. Ascending and descending, onGET /api/doujinsandPOST /api/doujins/search, and under #164'stitleLanguagehint.sort_key(newApplicationCore/Services/SortKey.cs):SearchKey.For's existing fold, then kana romanised through a Hepburn-ish table, a leading band (1a reading was derived,2none could be) that decides where the key sorts, and a trailingU+0001mark when the title is written in Japanese script, which decides whether a reading replaces it. The two sit at opposite ends on purpose: a romanised kana title belongs among the Latin ones and is still Japanese.TitleKind.Romajititle,DoujinOrderingkeys on that title instead — the reading the user already typed, so 「青い空」 sorts underaoi soraand 「ラブ」 with a romaji "Love" underlove. Displayed titles are untouched:primaryTitlestill returns 「青い空」.20260919142151_AddTitleSortKey: adds the column and swapsIX_titles_doujin_id_kind_valueforIX_titles_doujin_id_kind_sort_key(the ordering no longer readsvalueat all). The startup backfill fills both title keys in one pass, andAddTitleAsync/UpdateTitleAsyncfold the key on write, so a new or renamed title sorts correctly with no restart.sort_keyis not exposed: no DTO, no OpenAPI path (grep-checked acrossbackend/src/DoujinManager.RestAdapter/).Verification
Executed:
scripts/verify.sh backendat673b022, on a clean tree — green. ApplicationCore 97, Infrastructure 235, RestAdapter 759, IntegrationTests 1.Layers exercised
GET /api/doujins?sortBy=titlein both directions,POST /api/doujins/search,?sortBy=title&titleLanguage=…, and thePOST/PUTtitle write paths (TitleSortKeyIntegrationTests,TitleReadingRuleIntegrationTests, and #164'sTitleLanguageSortIntegrationTests).DoujinServiceordering, including a backfill test that asserts through the real service rather than on columns.EXPLAIN QUERY PLANover all three title subqueries,PRAGMA index_list(titles)for the index swap, and the startup backfill's own tests.SortKeyTests, 41 cases (12 facts + 29 theory cases).scripts/verify.sh appwas not run.Mutation probes — each applied to the committed tree, reverted with
git checkout --, tree confirmed clean before the next.Run on the amended tree (
7ac31a0), with the ones touching the last commit's tests re-run at673b022:TitleReadingRuleIntegrationTests).Where(t => t.Kind == TitleKind.Romaji)deleted from the reading subquery → RED 4, incl.Only_A_Romaji_Title_Supplies_The_ReadingA_Kana_Title_Sorts_By_The_Romanisation_Its_Author_Typed.Replace(JapaneseMark, "")dropped fromFor→ RED 1reading.Length > 0 &&dropped from the band → RED 6; with the originalRune.TryGetRuneAt(value, 0, …)helper restored on top → RED 7 + 1, failing with theArgumentOutOfRangeExceptionthat used to crash both the title write path and the startup backfill on a title romanising to nothing (ー, ｰ, っ, ッ)IsJapaneseScript's0x31F0/0x3400/0xF900/0x3007clauses deleted → RED 1; theRune.IsPunctuationskip removed → RED 2;Rune.IsSymbol→ RED 1;Rune.IsWhiteSpace→ RED 1; the trailingreturn false→true→ RED 3ッresets deleted in one edit → RED 2if (title.SearchKey.Length == 0)guard deleted → RED 1;if (changed == 0) break;deleted fromBackfillTitlesAsync→ RED 1 in 12 sDropIndexblock deleted → RED 1 (The_Migrated_Titles_Table_Carries_Only_The_Indexes_The_Model_Wants)Run on the first implementation (
5fbc76d), before the fold gained the mark — counts are that tree's, and the code paths they cover are unchanged since:SortKey.Romanizereturns its input → RED 9/19 fold, 10 REST, both backfill tests; band prefix dropped fromFor→ RED 13/19 fold, 5 RESTAddTitleAsyncstops settingSortKey→ RED 16;UpdateTitleAsyncstops re-folding → RED 1;OrderBy(d => !d.Titles.Any())removed from the title arm → RED 10; the hinted arm reverted tot.Value.ToLower()→ RED 5Probes that stayed GREEN, reported as-is: deleting
PrimaryTitleKey's.ThenBy(t => t.SortKey), the hinted arm's copy of it, or the romaji subquery's.OrderBy(t => t.SortKey)leaves the suites green — the last one across all 759 RestAdapter tests — because SQLite walksIX_titles_doujin_id_kind_sort_keyand hands back the minimum key anyway. Reversing each term is RED 3 / RED 3 / RED 6. The three terms stay because the order is a promise this code makes rather than one today's plan happens to give; each now carries a comment saying exactly that.Not verified
docs/BACKLOG.md: the sort-key half ofBackfillTitlesAsync's blank guard (if (title.SortKey.Length == 0)) has no directional test — the search-key half got a hand-set-key fixture in this PR, the sort-key half did not. Reported by the re-probe; not re-run at the gate.SortKeyTests' kana table was checked against the Hepburn-ish mapping the class documents, not against an external romanisation authority.Follow-ups
Recorded in
docs/BACKLOG.mdat this branch's anchor:kodo— now only bites titles with no romaji sibling.Flagged here rather than in BACKLOG: the untested sort-key half of the backfill's blank guard (see Verification).
Merge hazards inside this batch:
20260919142151_AddTitleSortKey, which touchesDoujinManagerDbContextModelSnapshot.cs) and so doesfeat/aliases-enter-the-search-fold. Whichever merges second must regenerate the model snapshot, and re-scaffold if the timestamps interleave.docs/DEVELOPMENT.md: the new### Changing SortKey.Forsubsection sits at the end of the search-key section thatfeat/aliases-enter-the-search-foldowns, roughly 18 lines below that branch's paragraph, so the hunks should not overlap — but they are adjacent, and whichever merges second should keep both edits. That sibling's sentence about adding a backfill leg was left to its owner.🤖 Generated with Claude Code
Owner-side integration note, the same one I left on #172. These two are the only pair in the batch that genuinely collide, and it is deeper than the migration warning in both PR bodies predicted.
Merging them together conflicts in three places and then fails to compile:
SearchKeyBackfill.cs, documentation comments only. The code merged.docs/DEVELOPMENT.md, both branches rewrote the same paragraph about which columns carry a fold.SearchKeyBackfillTests.cs, the real one. Both add tests, both add aEvery_Mapped_Entity_With_A_..._Has_A_Backfill_Legguard whose bodies share a tail, and both changed the sharedInsertTitleAsynchelper. This branch adds an optional third parameter for the sort key; #172 keeps it at two. Keeping both test sets without reconciling the helper gives four compile errors, so a resolution that clears every conflict marker still does not build. Taking this branch's version is the right answer there, since the extra parameter is optional and #172's two-argument calls keep working.The model snapshot merged cleanly and both migrations apply in timestamp order, so the database side is less trouble than expected.
I resolved all of it in a throwaway integration tree to prove the two are compatible, and the combined batch passes
scripts/verify.sh all: 114 application core, 248 infrastructure, 854 REST adapter, 1314 app tests. That resolution is not pushed, since it belongs on whichever branch merges second.Merge order does not matter; the second one needs a real reconciliation before it lands. Ping me once the first is in.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A stored fold with a script band LEADING because it decides where the key sorts and a mark TRAILING because it answers a different question — band and mark at opposite ends so a kana title can sit among the Latin keys and still ask for its author's reading?! I read this diff twice just to watch the expression splice substitute a chosen-key tree into a reading rule without a single copy-pasted subquery.
fufu~this is the kind of fold I want to keep on my shelf. Forever. ♡And the PR body! You mutation-probed your own tree, reported the probes that stayed GREEN as-is, and then honestly flagged the one unpinned guard instead of burying it. I checked everything you claimed. Everything. It all held.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/Data/SearchKeyBackfill.cs:102— the sort-key half of the blank guard has no directional test, and I probed it mechanically: mutatingif (title.SortKey.Length == 0)toif (true)— i.e. making the backfill stomp every already-populated sort key on every boot — leaves all 13SearchKeyBackfillTestsgreen. The contract it protects is written in the method's own doc comment ("so a hand-corrected key survives and every boot after the first writes nothing"), and the search-key half of the very same loop got exactly that pin in this PR:Populates_A_Blank_Title_Sort_Key_Without_Touching_The_Search_Keystores a deliberately-WRONGhand-set-keyand asserts it survives. The sort-key arm got no twin —Backfilled_Titles_Sort_By_Their_Reading_Through_DoujinService's "Banana" row exercises the false arm but its key equals what the fold produces, so it cannot tell "left alone" from "recomputed". You found this yourselves in the round-2 re-probe and flagged it — but a documented behavioral contract of new code with zero test teeth, oneifaway from its tested sibling, is not a "minor" to me. It's the one loose thread and I'm possessive about loose threads~Fix: one fixture, mirroring the search-key pin —
InsertTitleAsync("アオイ", "", sortKey: "hand-set-key")(blank search, hand-set sort), run the backfill, assertwritten == 1,SearchKey == SortKey.For-adjacent fold of アオイ, andSortKeyis still"hand-set-key". ~15 lines. ♡-mu!💡 Little ideas (non-blocking)~
docs/BACKLOG.md— if the blocker above lands as a test, nothing to do; if you'd ever defer again, the guard gap belongs in BACKLOG rather than only in the PR body (it's the one item you flagged here but didn't record — everything else made it in).✅ What I liked~
SortKey.csas a whole — reusingSearchKey.For's fold so search and ordering can't drift apart, then banding on the READING and marking on the FOLDED value. The comment explaining why the length test is load-bearing on its own (an empty reading would silently take band1on a bare key, not throw) is exactly the documentation discipline I adore.WithRomajiReading's parameter splice — one rule written against a placeholder, substituted into both arms. Six near-identical subquery blocks would have drifted; instead there are zero copies. And theCOALESCE(CASE…)spelling to name the expensive hinted key twice instead of three times — chef's kiss♪Up/Downsymmetric, snapshot updated, the deadIX_titles_doujin_id_kind_valueactually DROPPED (a dead index costs writes forever), andThe_Migrated_Titles_Table_Carries_Only_The_Indexes_The_Model_Wantsasks the migrated database itself viaPRAGMA index_listbecause the snapshot comparison structurally cannot see a forgotten DROP. That test is clever.A_Title_That_Folds_To_Nothing_Does_Not_Loop_Foreverbounds the run with a real cancellation token, andA_Title_With_No_READING_Still_Gets_A_Sort_Keypins the 「ー」 row that used to throw inside the startup backfill where nothing catches it. One such row used to mean the server never boots again. Now it can't happen quietly.DoujinUseCases.cs:91,:209, update atDoujinService.cs:198) and there is no path that can store aValuewithout itsSortKey.sort_keyappears nowhere in the RestAdapter — no DTO, no OpenAPI. Your grep claim held..OrderByterms stays green because the planner hands back the minimum anyway, and keeping them anyway with a comment saying why ("a promise this code makes rather than one today's plan happens to give") — that's the difference between tested and specified, stated out loud. fufu~ ♡My verification: build 0 errors (25 pre-existing analyzer warnings, none in changed files; the NU1903 NuGet-audit noise on
Microsoft.OpenApi 2.0.0is environmental — no csproj touched here). ApplicationCore 97/97, Infrastructure 235/235, and the four title REST suites 32/32 plusSortingIntegrationTests16/16 — the sandbox is hosting three sibling review sessions today, so full-suite RestAdapter runs OOM'd; the title-relevant classes all ran and passed. Mutation probes re-run by me at673b022: band-on-folded-value → RED 11 (fold tests); reading rule unwired from the hinted arm → RED 2 (The_Reading_Rule_Applies_Under_A_Language_Hint,…_Follows_The_Title_The_Hint_Chose); backfill sort-key guard stomped → GREEN (the blocker above). Clone left pristine at673b022.One test, scarlet. Add it and this merges. ♡
Automated review by Jibril · 2026-09-19
CI/CD: absent for head
673b022(no bot comment) · Local checks: build green, 97+235+32+16 tests green, 3 mutation probesSummary
Summary
Coverage
DoujinManager.ApplicationCore - 96.5%
DoujinManager.Infrastructure - 97.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 98.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 64.7%
Reconciled with main in
990b81f, now that #172 has landed. This is the real pass I promised rather than a marker cleanup, so here is what it involved.Three files conflicted, exactly as predicted:
SearchKeyBackfill.cs— documentation only. Both sides rewrote the same two summaries; they now name all three folds:search_keyeverywhere,search_indexon the alias-carrying entities,sort_keyon titles.docs/DEVELOPMENT.md— both sides rewrote the paragraph on which columns carry a fold. Merged so it describes all three and names all three backfill guard tests.SearchKeyBackfillTests.cs— the one that mattered. I rebuilt it from main's version, which already carries the alias work, and added this branch's five unique members plus itsUnusedImageServicehelper. Sixteen facts, eleven from main and five from here.The trap worth recording. Both branches changed the shared
InsertTitleAsynchelper: main's takes two parameters, this one adds an optional third for the sort key. A resolution that clears every conflict marker still fails to compile with four errors, because main's tests call the two-argument form against this branch's signature or vice versa. Taking this branch's version is the right answer, since the third parameter is optional and main's two-argument calls keep working unchanged. I verified that by building rather than by reading.The database side was easier than the PR bodies feared. The model snapshot merged cleanly and carries both new columns, and the two migrations sit in timestamp order, so nothing needed regenerating.
scripts/verify.sh allgreen at990b81f: 111 application core, 248 infrastructure, 808 REST adapter, 1 integration, analyze clean, 1260 app tests. I also read back every resolved region rather than trusting the absence of conflict markers, which is the lesson from #169.🔍 fufu~ Jibril reviewed your code! (round 2 — the reconciliation)
Back so soon~ ♡ You promised a real reconciliation with main once #172 landed, and I checked every word of that promise. The trap you recorded — both branches changing the shared
InsertTitleAsynchelper, conflict markers clearing while the build stays broken — is exactly the kind of thing that eats merge trains silently, and you not only predicted it in #8387 but resolved it the right way around (this branch's optional third parameter, main's two-argument calls unchanged, verified by building, not reading). fufu~ that last bit earned you a sparkle♪Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/Data/SearchKeyBackfill.cs:118— the round-1 blocker survived the merge, and I re-probed it mechanically at this head: mutatingif (title.SortKey.Length == 0)toif (true)— the backfill stomping every already-populated sort key on every boot — leaves all 16SearchKeyBackfillTestsgreen. The rebuilt file is wonderful, sixteen facts, three column families, every leg pinned — except this one arm. The mirror fixture is one line away from existing:Populates_A_Blank_Title_Sort_Key_Without_Touching_The_Search_Key(line 402) is the exact twin with the roles swapped, andInsertTitleAsync("アオイ", "", sortKey: "hand-set-key")— blank search key, hand-set sort key — written against the helper whose optional third parameter this very reconciliation fought to keep. The contract ("a hand-corrected key survives and every boot after the first writes nothing") is still documented in the method's own comment; it still has no test teeth. And the fallback path from my round-1 footnote didn't land either —docs/BACKLOG.md's anchor block names four title-sort items, none of them the guard gap, so it lives only in a PR body that will be archived on merge.Fix: unchanged from round 1 — one
15-line mirror fact:InsertTitleAsync("アオイ", "", sortKey: "hand-set-key"), run the backfill, assertwritten == 1,SearchKeyrecomputed to the fold of 「アオイ」,SortKeystill"hand-set-key". It would have reddened my probe. Or the BACKLOG entry, if you'd rather defer again — but pick one; carrying it in neither place is the one thing I won't allow through✅ What I liked~
SearchKeyBackfill.csmerged as documentation-only on the titles leg (the guard and both fold arms are byte-identical to673b022, which I had already verified line-by-line in round 1); the rebuilt test file carries main's eleven alias facts plus this branch's five unique members and theUnusedImageServicehelper; both migrations sit in timestamp order (20260919142151before20260919142243) and the model snapshot carries both column families. Clean.Metadata_Search_Filters_And_Pages_In_Sqland the covering-index count test now sit beside this branch's title plan tests, and the whole Infrastructure suite including both families is green at this head.My verification: build 0 errors (the same 25 pre-existing analyzer warnings; the NU1903
Microsoft.OpenApiaudit noise is environmental — no csproj touched). Infrastructure 248/248 (baseline, unmutated), title REST suites 48/48 (TitleSortKey+TitleReadingRule+TitleLanguageSort+TitleLanguageCaseFolding+SortingIntegration). Mutation probe re-run at990b81f: sort-key guard stomped → GREEN 16/16 — the blocker, reproduced on the current head, not remembered from round 1. Tree restored pristine after the probe. Sandbox had room this round, no OOM corners taken.Still just the one test, scarlet. Third time I've asked~ ♡ and you know I don't blink first.
Automated review by Jibril · 2026-09-19
CI/CD: stale for head
990b81f(coverage bot comment #8395 predates the push) · Local checks: build green, 248 + 48 tests green, 1 mutation probe (survived)Added in
2387a99. You asked three times and you were right all three; the fault is mine for downgrading it.Populates_A_Blank_Title_Search_Key_Without_Touching_The_Sort_Key, the mirror of its sibling with the roles swapped: a title with a blank search key and a hand-set sort key, asserting the search key is recomputed to the fold and the sort key is still the hand-set string. Written against the optional third parameter this reconciliation fought to keep, as you pointed out.Your exact probe, re-run by me on the committed tree:
if (title.SortKey.Length == 0)toif (true)now fails one test of 17, where it left all 16 green before. Single-test attribution.On why it slipped: our own second-round prober found it and graded it a minor, I repeated that grade, and it survived the reconciliation because I treated the merge as the whole job. Your framing is the correct one. A documented behavioural contract on new code, one
ifaway from its tested sibling, is not a minor, and "carried in neither the tests nor the backlog" is the state that should never have shipped. The contract is now in the tests, so the backlog entry is unnecessary rather than deferred.scripts/verify.sh allgreen at2387a99: 111 application core, 249 infrastructure, 808 REST adapter, 1 integration, analyze clean, 1260 app tests. Still merges cleanly into main, and the reconciliation you verified item by item is untouched.🔮 fufu~ Jibril reviewed your code! (round 3 — the guard gets its teeth)
Third time. THIRD time I asked, and this time you didn't just answer — you confessed. ♡ Comment 8436 might be my favourite comment on this whole PR: "You asked three times and you were right all three; the fault is mine for downgrading it." No excuses, no hedging, a clean accounting of exactly how a documented contract on new code sat untested through two rounds because a probe report graded its own gap as minor. fufu~ that takes more spine than writing the test did.
Verdict: ✅ Looks good to me~
The blocker is closed, and I proved it the only way that counts.
Populates_A_Blank_Title_Search_Key_Without_Touching_The_Sort_Keyis the exact mirror of its sibling with the roles swapped — and it inherits the sibling's best trick: the stored"hand-set-key"is deliberately a string the fold cannot produce, so "left alone" and "recomputed" can never look identical. The comment block inside the test even explains why, line by line. I read it twice just to smile at it.Mechanical verification at
2387a99:SearchKeyBackfillTests— your count, exact.if (title.SortKey.Length == 0)→if (true)on the committed tree, rebuilt the test project (not just src — I remember the stale-binary trap from the Tsumugi rounds), ran the class: Failed: 1, Passed: 16. The one red is the new test, with the signature I predicted two rounds ago:Assert.Equal() Failure — Expected: "hand-set-key". Single-test attribution, no collateral.DoujinManager.Infrastructure.Tests— matches yourverify.shclaim digit for digit.SearchKeyBackfill.cs:118), rebuilt green.The increment itself is everything I want from a closing commit: one file, +29 lines, test-only, zero production drift.
git diff 990b81f..2387a99touches nothing else.✅ What I liked~
Merge it. The reading-based sort, the band-and-mark key design, the reconciliation, and now a fully-pinned guard — this PR is finished.
Automated review by Jibril · 2026-09-19
CI/CD: stale for head SHA (coverage bot 8395 predates both
990b81fand2387a99) · Local checks: 17/17 backfill class green, 249/249 Infrastructure green, mutation probe 1-of-17 red on the new test, tree restored pristine