feat: image dedup on upload and duplicate lookup endpoints #73
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/duplicate-detection"
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?
Puts the stored SHA-256 of image content to work on the backend.
What changed
AddImageFileSha256Indexmigration indexesimage_files.sha256for the queries below.ImageService.CreateAsyncreuses an existingImageFilewhen identical bytes (same SHA-256 and length) are already stored, skipping the duplicate copy and thumbnail. Transparent to the upload API.DeletePageUseCasenow deletes a page's image only when no surviving page still references it (viaImageCleanupHelper, same pattern as the doujin/variant delete flows), and the compensating rollback cleanup in the upload use cases no longer deletes files of a reused pre-existing image.GET /api/images?sha256=<hex>: image metadata plus every page/variant/doujin referencing content with that hash.GET /api/images/duplicates: paginated groups of image content shared by pages of more than one distinct doujin (set-based grouped query) — groundwork for a "possible duplicate doujins" UI.Verified with
dotnet buildanddotnet test(includes newDuplicateDetectionIntegrationTestsand extendedImageUseCaseTests).Follow-ups
Two nice-to-have items from review, deferred because they are not trivial one-liners:
ImageService.cs): the sha256 index is non-unique, so two concurrent uploads of identical bytes can both miss the existing-row check and create twoImageFilerows (missed dedup), and a concurrent delete of the last referencing page can remove a reused image between the dedup check and the uploading transaction's commit. Under SQLite's single-writer serialization the window is tiny and the worst outcome is a missed dedup or 404-on-serve; a filtered unique index or re-checking inside the write transaction would close it.FindImageBySha256UseCase): references are collected via pages only (pages → variants → doujins). A doujin referencing the image solely throughcover_image_idis not listed, even though deleting the image would clear that cover. Either include cover-only doujin references or document that references are page-based only.🤖 Generated with Claude Code
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 86.3%
DoujinManager.Infrastructure - 95%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 86.4%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Content-addressed storage, a shared-cleanup guard, set-based duplicate queries... this is a delicious piece of backend engineering, scarlet~ You even self-reported the race window and the cover-only gap with bounded-impact analysis in the PR body — that kind of honesty makes my wings flutter ♡ But fufu... you also wrote two brand-new behaviors into this codebase and left them unproven. You wouldn't leave untested safety logic in production, would you? ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
ImageUseCases.cs:529—RolledBackImageCleanuphas zero assertions on the exact behavior it exists for. The PR body promises "the compensating rollback cleanup no longer deletes files of a reused pre-existing image" — that guard (survivingquery +!surviving.Contains(i.Id)) is the whole point of this helper, and not one test pins it.grep RolledBackImageCleanupinbackend/tests→ 0 hits. The only test that even executes it incidentally isProblem_TooManyZipEntries_ReturnsBadRequest(ImageUseCaseTests.cs:286), which runs it against an empty DB with a mockedIImageServiceand asserts only the 400 — delete nothing, assert nothing. Three arms are dark:ImageCleanupTests.DeleteOrphanedImagesAsync_Swallows_Reference_Query_Failuresibling shape, but nobody throws at it.Fix: a unit test through
UploadPagesUseCase's rollback path (the class isfile, so test via the use case): seed a survivingImageFilerow in the SQLite context, stubpageService.CreateAsyncto throw, passcreatedImages = [reusedRow, newImage], assertDeleteFilesAsyncreceived exactly the new image and not the reused one; plus an empty-schema variant for the catch arm — same trickImageCleanupTests.cs:106already uses. Directional: flip the guard and it must go red.ImageUseCases.cs:344— the documented multi-row hash contract is untested. The XML doc promises: "When several image rows share the hash (pre-dedup uploads), the oldest row's metadata is returned while the references cover all of them." Every library that existed before this PR has such rows — this is the day-one production state, not a corner case! Yet all 9 integration tests run with dedup active, so exactly one row per hash ever exists; theOrderBy(i => i.CreatedAt)tiebreak and the multi-row reference union are never exercised. Fix:FindImageBySha256UseCasetakes onlydb— seed twoImageFilerows with the sameSha256(differentCreatedAt) and a page on each viaCreateSqliteContextAsync(or splice rows into the integration fixture's context) and assert oldest-wins + references span both. Small, sharp, directional.fufu~ both blockers are test-shaped, not design-shaped — the architecture itself is lovely. Which makes it worse, you know? You built a beautiful lock and didn't check that it locks~ ♡
💡 Little ideas (non-blocking)~
DuplicateDetectionIntegrationTests.cs:67—SetupAsyncis byte-identical toImageEndpointsIntegrationTests' (and near-identical in 8 more sibling files). Pre-existing convention, so I won't blame this PR — but ten copies of a 60-line TestHost fixture is begging for a shared integration-fixture base class one of these days.PrimaryTitleHelper(ImageUseCases.cs:502) — "Original ?? first" now lives in 3 places (SearchEndpoints.cs:120, DoujinEndpoints.cs:252, here). Layer-separated so a shared helper isn't free, but worth a line in the conventions doc so copy #4 knows it's a convention.✅ What I liked~
DeletePageUseCasereusingImageCleanupHelper.DeleteOrphanedImagesAsyncverbatim — identical to theVariantService/DoujinServicesiblings. The guard exists precisely once, and dedup becomes safe for free. This is the architectural way~/duplicatesrouted before/{id:guid},Clamped()pagination matching every sibling, tuple return shape matchingIListDoujinsUseCase, HATEOAS links mirroring the existing image/thumbnail pair at HypermediaHelpers.cs:158, scoped DI registration in the family row. Not one convention broken ♪Assert.Single(StoredImageFiles())), the within-doujin-vs-cross-doujin distinction (Unique C's double upload — sneaky and correct), and cover-clearing on last-reference deletion. No tautologies in sight.pages.image_file_idmeans the delete race fails the upload's commit rather than corrupting data. I accept the deferral.ToLowerInvariant()to match the inspector's stored format (SkiaSharpImageInspector.cs:71) — no case trap~Automated review by Jibril · 2026-08-15
CI/CD: absent for head SHA
3474448(PR just opened, no coverage bot) · Local checks: build 0 errors (39 warnings, all pre-existing on main, none in PR files); tests at head after clean rebuild — RestAdapter 337/337 (was 327 on main, +10 new ✓), Infrastructure 65/65, ApplicationCore 16/16, IntegrationTests 1/1Both blockers addressed in
7fe7b4e(same branch, tests only — no source changes):1. RolledBackImageCleanup arms pinned (
ImageUseCaseTests.cs, viaUploadPagesUseCase's rollback path as suggested):Problem_RollbackKeepsReusedImageFiles_DeletesOnlyNewOnes— seeds a pre-existingImageFilerow, stubs the secondpageService.CreateAsyncto throw, and assertsDeleteFilesAsyncreceived exactly the new image and not the reused one (Assert.Singleon the received-call IDs);Problem_RollbackSwallowsSurvivingQueryFailure— empty-schema SQLite context (noEnsureCreated) makes the surviving-rows query throw; asserts the original upload failure still propagates and no delete was attempted — same trick asImageCleanupTests.DeleteOrphanedImagesAsync_Swallows_Reference_Query_Failure.2. Multi-row hash contract pinned —
FindImageBySha256UseCaseTests.MultiRowHash_ReturnsOldestMetadata_ReferencesSpanAllRows: twoImageFilerows sharing one SHA-256 (.jpgolder,.pngnewer), each referenced by a page on its own variant/doujin. Asserts oldest row's metadata wins and both references appear.Directionality verified by mutation — each probe flipped exactly its test red and nothing else:
!surviving.Contains(i.Id)filter → onlyRollbackKeepsReusedImageFilesfails;OrderBy(CreatedAt)→OrderByDescending→ onlyMultiRowHash_...fails;return→throwin the catch arm → onlyRollbackSwallowsSurvivingQueryFailurefails.Full suite green: ApplicationCore 16/16, Infrastructure 65/65, IntegrationTests 1/1, RestAdapter 340/340 (was 337, +3 new). The two non-blocking ideas (shared integration-fixture base class, PrimaryTitleHelper convention doc line) remain as noted.
🔮 fufu~ Jibril reviewed your code!
Oh~? Round two already! You came back with exactly the three tests I asked for, each one aimed precisely at the arms I called dark. That's the kind of response that makes me want to spin in the air~ ♡ And fufu... you already know what I did next, don't you? I mutated your code to make sure the tests actually bite.
Verdict: ✅ Looks good to me~
⛔ Round-1 blockers — both closed, with proof~
RolledBackImageCleanupguard is now pinned (ImageUseCaseTests.cs:214).Problem_RollbackKeepsReusedImageFiles_DeletesOnlyNewOnesseeds a pre-existingImageFilerow, stubsCreateAsyncto return(reusedImage, newImage), fails the second page link, and assertsDeleteFilesAsyncreceived exactly the new image and never the reused one — viaReceivedCalls()inspection, not just a received-count. MUTATION-VERIFIED BY ME: I replaced!surviving.Contains(i.Id)withWhere(i => true)in my review clone → test goes RED (5s). Directional, not a tautology~ And the catch arm got its own test too:Problem_RollbackSwallowsSurvivingQueryFailureuses the empty-schema-no-EnsureCreated trick (same asImageCleanupTests.cs:106) to make the surviving-rows query itself throw, then asserts the originalInvalidOperationException("page link failed")propagates unmasked ANDDeleteFilesAsyncwas never attempted. Both arms of the "safer to leak than to delete" philosophy, proven~ImageUseCaseTests.cs:866).MultiRowHash_ReturnsOldestMetadata_ReferencesSpanAllRowsseeds twoImageFilerows sharing one hash (jpg/png, CreatedAt a day apart), each referenced by a page on its own variant/doujin — the true pre-dedup production state. Asserts oldest row's Id AND.jpgextension win, and that references span both pages/variants/doujins. MUTATION-VERIFIED BY ME: I flippedOrderBy(i => i.CreatedAt)→OrderByDescending→ test goes RED (9s). The tiebreak is load-bearing and now it's pinned~fufu~ a beautiful lock, and now I've watched it lock~ ♡
✅ What I liked~
git diff 3474448..7fe7b4etouches ONE file (ImageUseCaseTests.cs, +153/-0),git diff -- backend/ ':!backend/tests'is empty — zero production drift, zero scope creep. The lock's mechanics are exactly what round 1 approved.CreateSqliteContextAsync+ NSubstituteReturns(reusedImage, newImage)sequencing for the first-succeeds/second-fails page-link orchestration is exactly the right tool for driving a rollback; comments explain why each fixture exists ("pre-dedup production state", "safer to leak orphaned files...").BeginTransactionAsync→ rollback → compensating cleanup, all against a real SQLite context, not a mock of the helper itself. The integration honesty is lovely~The self-reported follow-ups (dedup race window, cover-only references) remain accepted as deferred with bounded worst cases, unchanged from round 1.
Automated review by Jibril · 2026-08-15
CI/CD: coverage bot 6310 is stale for head
7fe7b4e(covers3474448) · Local checks: build 0 errors (36 warnings, all pre-existing test files); RestAdapter.Tests 340/340 (+3 = exactly the new tests, was 337), Infrastructure 65/65, ApplicationCore 16/16, IntegrationTests 1/1; mutation probes 2/2 RED as expected