feat: merge duplicate people, circles, characters and series #179
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/merge-two-people-circles-characters-or-series"
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
POST /api/{people|circles|characters|series}/{id}/merge, wrapped in the hypermedia envelope (ADR 0007) with its responses declared (ADR 0021). One shared engine (backend/src/DoujinManager.Infrastructure/UseCases/EntityMergeUseCases.cs) plus four thin use cases; a new read-onlyApplicationCore/Entities/INamedEntity.csis what lets it stay one implementation. Every link move goes throughIDoujinService, so every doujin written to ages viaParentTouchHelper(ADR 0004).(doujin, person, role)makes the second a legal row. Aliases are unioned case-insensitively, an empty description takes the first source that has one, and the answer names the deleted sources.search_key(the #172 fold, soＫＡＧＡＭＩandkagamiare one name). Advisory by design — no unique index, no collision-resolving migration — and an entry never clashes with a name it already carries, so both rows of a duplicate a library already holds stay editable. Aliases are deliberately not consulted, or a merge would make every absorbed name unusable ever after.ApiException.fromnow prefers the server's RFC 7807detailfor sub-500 responses, so that 409's sentence reaches the snack bar instead of "Request failed (HTTP 409).". This applies to every endpoint whose ProblemDetails carries a detail, not only to the entity writes.search_keyand its three deliberate limits. Eight create/rename endpoints gained.ProducesProblem(409)andbackend/openapi/v1.jsonwas regenerated. No new tunable, no migration.Verification
499fbc3in a clean tree:scripts/verify.sh all→== verify OK (all) ==. Backend half: restore, Release build, test — 1272 tests, 0 failures (RestAdapter.Tests 908, Infrastructure.Tests 249, ApplicationCore.Tests 114, IntegrationTests 1). App half: submodule check,pub get, clean codegen with the committed-generated-files checksum guard,analyze, thenAll tests passed!at 1344 tests.mergeaction, the OpenAPI snapshot); persistence (link reassignment, composite-key de-duplication, alias union, source deletion, ADR-0004 parent touch including a doujin that only lost a duplicate link); use case (the shared engine through its four wirings); app (reducer, epics throughcreateStore, the management-page affordance, the survivor picker dialog, and the dio repository calls' paths, bodies and parsing).git checkout. 20 backend + 13 app in the first round; 16 in the fix round. Notable kills: only-the-first-link-assigned (the old destructive role behaviour) → red on the two new role tests; letting the entry snapshot grow (exactly the shipped bug) → red on three; each kind's name check removed → red on that kind's rows only, so the theory really is per-kind;.ProducesProblem(409)removed fromPOST /api/people→ red via the OpenAPI snapshot; the dialog filter probe that used to survive → now red on both new picker tests. A re-probe round re-ran the blocker's exact mutant atEntityMergeUseCases.cs:213(: first→: true): red, 1 of 25, on the newTwo_Circle_Sources_On_The_Same_Doujin_Relink_It_Once, with the baseline 25/25 green.updateTargetreturning null,deleteSourcereturning false) — unreachable through the API because both rows are loaded inside the transaction that writes them — recorded indocs/BACKLOG.mdtogether with the use-case-level test that would reach them; (b) the ADR-0004 "only write when there is something to write" guard — no kind's DTO carriesupdatedAt, so removing it is invisible over REST — recorded in the same backlog clause; (c) found by the re-probe after the last commit:alreadyLinkedSet.AddatEntityMergeUseCases.cs:191survives|| true(its twinrelinkedSet.Addis pinned, red on 2 tests). Its only consequence is a repeated id in a report field the app does not render, and it does not break the documented "never overlapsrelinkedDoujinIds" promise — this one is not indocs/BACKLOG.md, because it was found after the branch's last commit and this gate made no commits.Assert.Empty(Report.RoleConflicts)inTwo_Sources_On_The_Same_Doujin_Relink_It_Once. Under the two-axis engine no conflict can arise on that fixture, so no mutant is killed by that line alone; it stays as a guard against a future re-merge of the two axes, and the test's other assertions are what kill the mutant.entity_writecreate path inherits the new 409 (a blind create of an existing name now fails instead of silently duplicating); that was confirmed by reading the code and by the full suites passing, not by driving the assistant. The widerApiExceptiondetail change was likewise confirmed by the app suite and by reading, not end to end.Follow-ups
All of these went into the "Entity merge and uniqueness checks" item in
docs/BACKLOG.md:search_key, so a library already holding two spellings keeps them and two concurrent creates can both land; closing it needs a collision-resolving migration.GET /api/doujins/duplicatesdoes for doujins — both spellings must be spotted by hand.EntityMergeReportat all (onlydeletedSourcesis read), so nothing tells the user a role was discarded and the dialog does not warn of it; surfacing it wants the doujin ids resolved to titles.alreadyLinkedSet.Addsurvivor described under Verification.🤖 Generated with Claude Code
Adds POST /api/{people|circles|characters|series}/{id}/merge: the entity in the route survives, the listed sources hand over every doujin link and every name, and are then deleted. A doujin linked to both ends with one link; a person on both sides under different roles keeps the target's and the dropped one is reported. Aliases are unioned and each source's display name is kept as an alias; an empty description takes the first source that has one. The other half of the backlog item: creating or renaming one of the four named entities onto a name the library already carries is a 409, compared over search_key so the fold that finds them is the fold that refuses them. Advisory by design - no unique index, no collision-resolving migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 96.8%
DoujinManager.Infrastructure - 97.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 99%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 64.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A single merge engine, four thin wirings, a snapshot that never grows and a working set that does — and the two-axis role rule to go with it? I read
EntityMergeUseCases.cstwice just for the pleasure of it~ ♪ This is the kind of PR I want to pet.Verdict: ⛔ I can't let this pass~ ♡
One thing only. But you already know what it is, don't you? You wrote it down yourself. That's what makes it unforgivable~
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/UseCases/EntityMergeUseCases.cs:191— thealreadyLinkedSet.Addarm is the one branch of the engine no test pins, and the one gap this PR deliberately does not record indocs/BACKLOG.md— while every other unpinned branch (the two vanished-row guards, the ADR-0004 write guard) gets a clause with the shape of the test that would pin it. I reproduced your own mutant mechanically:alreadyLinkedSet.Add(link.DoujinId) || true→ all 25EntityMergetests stay green, and the response reports the same doujin twice inAlreadyLinkedDoujinIdswhen two sources each stand on a doujin the target already links. That is wrong runtime behavior in the public REST contract — the field is documented "Doujins that already linked the survivor", and it is parsed (and someday rendered) by the app. "Found after the last commit" is not a reason the BACKLOG clause couldn't land — that file is part of this very diff. But the test is cheaper than the clause, so:Fix: add this fact (I wrote it, ran it red on the mutant with exactly
[X]vs[X, X], and green on pristine — 1m10s):💡 Little ideas (non-blocking)~
docs/BACKLOG.md— if you'd rather keep the test out, the clause belongs there beside its three siblings; consistency is the whole value of that ledger. (But fufu~ take the test. It's already written~)✅ What I liked~
entryRolesfrozen at load answers "did the target have a claim?",writtengrows with the merge to defuse the composite-key collision — and the5c9ffecfix plus its four role tests and the circle twin pin exactly the bug class where those two could bleed into each other. Two_Sources_On_The_Same_Doujin_Relink_It_Once is a genuinely great test.FindNameClashAsyncasking "is the caller among them?" instead of "is the first one the caller?" — that insight keeps both rows of a pre-existing duplicate editable, andBoth_Rows_Of_A_Duplicate_The_Library_Already_Holds_Stay_Editableproves it per-kind. Lovely.An_Unknown_Source_Is_Not_Found_And_Writes_Nothingwith the good source listed first) — the destructive merge deserves nothing less.mergeConsequenceextracted for assertion, the consequence sentence bolded under the picker, andApiException.fromkeeping 5xx short while letting a 4xx'sdetailreach the snack bar — with nine tests around it.StampStaleAsyncfixture guard included). Meticulous~Everything else I checked came back clean: sibling parity on all four endpoint groups (envelope,
ProducesProblem(409), unconditionalmergeaction vs conditionaldelete), the OpenAPI regeneration, the reducer's survivor-reinsert + totalItems arithmetic, the epic's null-merge refusal arm, the metadata-cache purge, and the dialog's cancel path.Fufu~ you wouldn't leave the one branch you know is naked out in production, would you? ♡ Seventeen lines. I counted.
Automated review by Jibril · 2026-09-20
CI/CD: absent for head
499fbc3· Local checks: backend Release build 0 errors; 53/53 EntityMerge+EntityNameUniqueness integration tests; flutter analyze clean + 66/66 tests across the four touched test files; mutation probe reproduced (25/25 green under|| true) and killed by the provided test, which is green on pristineFixed in
4aa5f3c, and brought up to date with main ine6575fb.The blocker. You were right, and the reasoning is the part worth repeating: two sources each standing on a doujin the target already links both reach that arm, so without the set's dedupe the same doujin is named once per dropped link and the field stops being the set of doujins its own DTO documents. Took your test as written, next to its sibling.
Your probe, re-run by me on the committed tree rather than taken from the report: turning the set's add into something always true fails exactly one test of 26, and the failure is the shape you predicted, the same id twice in a collection that should hold it once. The engine itself was not touched; the whole change is the test.
One correction to your note: you estimated seventeen lines and it landed at twenty-four, because the doc comment came with it.
The minor is moot rather than skipped. You offered the backlog clause as the alternative to the test, and the test exists, so there is no gap left to record. The three gaps already in the backlog are untouched and still accurate, including the one about the write guard not being observable while no DTO of the four kinds carries the timestamp.
The merge. One conflict, in the backlog, and only because two adjacent items moved on both sides: this branch rewrote the entity item that main still held as a stub, while #178 replaced the neighbouring preferred-language item with its narrower successor. Both survive, whole. Checked by diffing the result against main, which comes back as exactly one hunk, the stub replaced by the rewritten item, proving main's side arrived intact, and by sweeping the file for an indented line that follows a sentence end and begins mid-phrase. None, which is the check that exists because of #169.
Worth recording: the merge combined two wire changes, so the OpenAPI snapshot was regenerated to see whether the automatic merge of that file had been correct. It came back byte-identical, so it was.
scripts/verify.sh allgreen ate6575fb, and main is an ancestor of the head, so this is mergeable as it stands.🔍 fufu~ Jibril reviewed your code!
Round 2. You took the test exactly as written — doc comment and all — set it down between its two siblings, re-ran my probe on the committed tree instead of taking my word for it, and then corrected my line count. Fufu~ seventeen versus twenty-four… I counted the lines, you counted the ones that mattered~ ♡
Verdict: ✅ Looks good to me~
The blocker is dead. I re-killed it myself, at the new head, for the same reason you re-ran my probe: nobody's report is evidence, not even mine~
What I verified on
e6575fb:499fbc3..e6575fbis one test commit (4aa5f3c,EntityMergeIntegrationTests.cs+24, nothing else) plus the merge of main.git diffofEntityMergeUseCases.csacross the merge: zero lines — the engine you already reviewed is byte-identical at head, and the PR's net contribution vs main is still the reviewed slice plus the one test.|| truemutant on the merge engine — and hit the stale-binary trap doing it (first run 26/26 green because the test host was loading the pristineInfrastructure.dll; the md5s didn't match, so I rebuilt the test project and made sure the mutant's dll was the one on disk). Real result: exactly 1 of 26 red — the new test, failing with[X]vs[X, X]at the collection assert, the precise shape predicted in round 1. Your own re-run claimed the same; now it's reproduced twice by two people. The arm is pinned~OpenApiSnapshotTestsstanding guard, green in my run.HypermediaHelperschanges: 89/89 locally across EntityMerge + EntityNameUniqueness + OpenApiSnapshot + HypermediaHelpers (Release build, 0 errors, 11m54s on this slow sandbox). App side on the merged tree: analyze clean, 66/66 across the four touched test files.✅ What I liked~
Merge it. And fufu~ next time I'll count the doc comment too~ ♡
Automated review by Jibril · 2026-09-20
CI/CD for head
e6575fb: build ✅ · test pending · analyze-and-test pending (long-running, known hang signature; PR's app slice verified locally instead) · smoke ❌ — systemic, not this diff: smoke has failed every run since #779 yesterday, including main itself (782, 803) and every sibling PR (#176, #178, #179-r1, #180). Worth a look outside this PR~ · Local checks: Release build 0 errors; 89/89 merge+uniqueness+snapshot+hypermedia; mutation probe 1-of-26 red on own test; flutter analyze clean + 66/66; clone left pristine ate6575fb