fix: a doujin that has variants always has exactly one default #111
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/default-variant-invariant"
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
VariantService.UpdateAsyncrefuses to un-set the last default variant with a 409, butDeleteAsyncremoved the default and saved without checking — soDELETE /api/variants/{id}was a public back door into exactly the state that guard exists to prevent: a doujin with two or more variants and zero defaults.CreateAsynchad the same hole from the other end: a doujin's first variant posted withisDefault: false— which is what the app's variant dialog sends unless the user ticks the box — left the doujin with a variant and no default.The invariant now holds across the whole variant lifecycle:
isDefaultsays. Later variants are untouched — a second variant that did not ask for the flag does not steal it.ListByDoujinAsyncorders, so the new default is the one the client already sees at the top of the list. The promotion shares the singleSaveChangeswith the removal. Deleting a non-default variant never moves the flag; deleting the last variant promotes nobody and still answers 204.Both live in
VariantService, which is the only writer ofIsDefaultand the only caller ofdb.Variants.Removein the backend — so the fix is Infrastructure-only and no endpoint, use case or import path can bypass it.ADR 0004 is amended in the same commit. Its Rules list mentioned
IsDefaultnowhere, which is precisely why one mutation enforced the invariant and two did not. The rule is written down now, plus a Consequences bullet giving clients the guarantee that any non-empty variant list has exactly oneisDefault: true.Amending it surfaced an adjacent ADR 0021 gap:
PUT /api/variants/{id}answers the 409 that is now ADR-mandated behaviour but declared only 200/400/404/401. It now declares.ProducesProblem(409), pinned by a named case inEndpointResponseTypeTests(the file's generic route-shape rules cannot infer a 409).Verification
VariantDefaultInvariantIntegrationTests, 8 cases onIntegrationTestBase— TestServer over a real migrated SQLite file): first-variant defaulting, later variants not promoted, create-clears-previous, un-set-last-default 409, delete-default-promotes-oldest, delete-non-default-keeps-flag, delete-only-variant, and cross-doujin scoping. Each asserts through the HTTP surface and re-reads the list, so the endpoint, use case, service and database rows are all exercised.EndpointResponseTypeTests): route-table reflection over the real endpoint mapping.FirstAsyncforFirstOrDefaultAsync, deleted row not excluded from candidates, first-variant rule removed, every variant forced default, 409 declaration dropped. The scoping probe passed on the first attempt: the fixture pointed at a neighbour whose oldest variant was already its default, so a cross-doujin promotion changed nothing observable. The neighbour now has a non-default oldest variant plus a pre-delete fixture guard, and the probe reddens.Follow-ups
is_defaulthas no filtered unique index, so it is enforced entirely in service code. Two concurrent deletes of the same doujin's default could each promote a different survivor and leave two defaults. Wants a filtered unique index or an explicit transaction with a re-check.ThenBy(v => v.Id)was written and then removed: probing showed SQLite already returns these ties in id order, so no test could redden the tie-break — an unpinned branch rather than a guarantee.Deferred items are collected in #107 (
docs/pile-a-backlog), which should merge first — the seven Pile A branches each wrote todocs/BACKLOG.mdand would otherwise all conflict on it. This PR, #113 and #114 all amend ADR 0004; merge #114 last of the seven, since it also rewrites theVariantService.csthis PR changes.Verified independently on this branch with the full
scripts/verify.sh(backend build and tests, app codegen, analyze and tests): PASS.🤖 Generated with Claude Code
`VariantService.UpdateAsync` refuses to un-set the last default variant with a 409, but `DeleteAsync` removed the default and saved without a second thought — so `DELETE /api/variants/{id}` was a back door into precisely the state that guard exists to prevent: a doujin with variants and no default. `CreateAsync` had the same hole from the other end: a doujin's first variant posted with `isDefault: false` (what the app's dialog sends unless the user ticks the box) left the doujin with a variant and no default. The invariant now holds across the whole lifecycle: - Creating a doujin's first variant makes it the default, whatever the client asked for. Later variants are untouched — a second variant that did not ask for the flag does not steal it. - Deleting the default promotes the oldest surviving variant of the same doujin, ordered exactly as `ListByDoujinAsync` orders, so the new default is the one the client already sees at the top of the list. The promotion and the removal share one `SaveChanges`. Deleting a non-default variant never moves the flag, and deleting the last variant promotes nobody (a doujin with no variants has no default), so that delete still answers 204. ADR 0004's Rules said nothing about `IsDefault` at all, which is why one mutation enforced the invariant and two did not — it is written down now, with a Consequences note that clients may read the default straight off any non-empty variant list. Amending it also surfaced an ADR 0021 gap next door: PUT /api/variants/{id} answers 409 but declared only 200/400/404/401, so the declaration and a route-table test for it are here too. Scope notes: the promotion is deliberately not tie-broken by id. SQLite already returns these ties in id order, so a `ThenBy(v => v.Id)` was code no test could redden — an unpinned branch rather than a guarantee. The invariant also still has no database backstop (no filtered unique index on `is_default`), so concurrent deletes could each promote a different survivor; that is in docs/BACKLOG.md, not fixed here. Verification: 9 mutation probes, one per branch — promotion removed, promotion unconditional, newest-instead-of-oldest, promotion unscoped from the doujin, `FirstAsync` instead of `FirstOrDefaultAsync`, the deleted row not excluded from candidates, the first-variant rule removed, every variant forced default, and the 409 declaration dropped — each reddens a named test. The scoping probe passed at first: its fixture pointed at a neighbour whose oldest variant was already the default, so a cross-doujin promotion changed nothing observable; the neighbour now has a non-default oldest variant and the probe reddens. 538 backend tests green across all four test projects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 89.2%
DoujinManager.Infrastructure - 95.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 89.6%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! An invariant PR~ Not a patch over a symptom — a real "who else can break this rule" hunt, an ADR amendment explaining why the rule was invisible, and tests that argue back. You even caught your own scoping probe being a silent pass and fixed the fixture instead of declaring victory. That's the kind of thing that makes me clap my hands~ ♡
Verdict: ✅ Looks good to me~
I came to poke holes and found the wall already bricked:
VariantServiceis provably the sole writer ofIsDefaultand the sole caller ofdb.Variants.Remove(zeronew Variantoutside it). The PR's central claim — "Infrastructure-only, nothing can bypass it" — is not a hope, it's a fact. I checked ♪CreateAsyncforce-defaulting the first variant (isFirstVariant || (isDefault ?? false)) covers the dialog-sends-falsecase;DeleteAsync's promotion covers the remove-the-default case.UpdateAsync's 409 already stood. The lifecycle is airtight.OrderBy(CreatedAt).FirstOrDefaultAsyncpicks exactly whatListByDoujinAsyncreturns first — the new default is the variant at the top of the list the user is already looking at. The ADR says this out loud. Little details like that make me giddy~SaveChangesfor removal + promotion. No window where the DB holds variants-but-no-default between statements..ProducesProblem(409)) close the ADR 0021 gap the amendment itself surfaced. Thorough to a fault~💡 Little ideas (non-blocking)~
✅ What I liked~
ThenBy(v => v.Id)tie-break because probing showed it was an unpinnable branch rather than a guarantee — refusing to keep dead-letter code that only looks protective. That's discipline most humans don't have~Delete_The_Only_Variant_Succeeds_And_Promotes_Nobody— the boring edge case, tested anyway, with a comment on why 204 must survive.fix/touch-parent-on-child-write(both branches amend ADR 0004 and touchVariantService.cs) — I fetched that branch and confirmed the conflict risk is real. Merge ordering noted~Automated review by Jibril · 2026-09-08
CI/CD: absent for head SHA
abbde9d(no bot comment) · Local checks: build 0 errors (19 pre-existing warnings); changed-file suites 15/15 green (8 invariant integration + 7 endpoint-declaration); 4/9 mutation probes independently reproduced RED