feat: immutable caching for images, created_at index, pagination validation #68
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/backend-http-caching"
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?
Backend performance pass in three parts:
HTTP caching for image endpoints
/api/images/{id},/api/thumbnails/{id}, and/api/images/{id}/compressednow respond withCache-Control: public, max-age=31536000, immutableand a strong ETag. Image content is content-addressed (stored SHA-256), so the ETag is derived from that hash — for compressed responses it is suffixed with the effectivemaxKbso different size budgets cache independently. When the client'sIf-None-Matchmatches, the server answers304 Not Modifiedwithout ever opening the content stream or recompressing, so revalidation is nearly free. Covered by new integration tests inImageEndpointsIntegrationTests, and the skip-work ordering itself is pinned by unit tests inImageUseCaseTests(a matching validator must not callOpenReadAsync/OpenThumbnailAsync/CompressAsync).Index on
doujins.created_atEvery library list/search orders by
created_at; the new EF migrationAddDoujinCreatedAtIndexadds the matching index so those queries stop scanning.Pagination clamping
pageandpageSizeare now clamped at the endpoint layer (page >= 1,pageSize1..100) viaPaginationParams.Clamped(), soPageInfo, pagination links, and the payload all agree — andpageSize=0no longer yields a garbagetotalPages(the(double)division produced infinity, which truncated toint.MaxValue). Covered byPaginationIntegrationTests.Follow-ups
If-None-Matchis compared by exact string equality against the single generated ETag, so RFC 9110 forms are not honored:*, multi-ETag lists ("a", "b"), and weak validators (W/"sha", e.g. added by an intermediary) fall through to a full 200. The failure mode is only a wasted revalidation, never wrong content. Parsing viaMicrosoft.Net.Http.Headers.EntityTagHeaderValue.TryParseListwould cover these cases if desired.🤖 Generated with Claude Code
- Serve /api/images, /api/thumbnails, and /api/images/{id}/compressed with Cache-Control: public, max-age=31536000, immutable and a strong ETag derived from the stored SHA-256 (suffixed with the effective maxKb for compressed responses); If-None-Match returns 304 without opening the content stream or recompressing. - Add an index on doujins.created_at (EF migration AddDoujinCreatedAtIndex) since every list/search orders by it. - Clamp pagination at the endpoint layer (page >= 1, pageSize 1..100) via PaginationParams.Clamped() so PageInfo, links, and payload agree and pageSize=0 no longer divides by zero computing totalPages. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 85.7%
DoujinManager.Infrastructure - 94.3%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 85.6%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Ohhh~ Content-addressed ETags, budget-suffixed validators, an index that matches the actual
OrderByDescending… scarlet, you thought about cache keys — that's the part everyone gets wrong, and you got it right. I was having such a lovely time… ♪…until I noticed your test suite doesn't believe your own PR body. ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/UseCases/ImageUseCases.cs:208-210, 231-233, 265-267— the "nearly free" claim has no directional pin anywhere. Your PR body promises: "the server answers 304 … without ever opening the content stream or recompressing, so revalidation is nearly free." The integration tests pin the HTTP contract (304 + ETag echo + Cache-Control + empty body — good!), but nothing asserts the skipped work.ImageUseCaseTests.cs— the exact home for this, already full of NSubstitute mocks — wasn't touched: it contains zero occurrences ofSha256orIfNoneMatch. Mutation probe: moveOpenReadAsync/CompressAsyncbefore the ETag check in any of the three use cases and every test in the repo stays green — the PR's entire performance value silently evaporates on the next refactor and CI smiles the whole time. Fufu~ you wouldn't leave your headline claim unpinned, would you? ♡Fix: 3–4 surgical unit tests in
ImageUseCaseTests.cs:GetImage: mock withSha256 = "abc…",IfNoneMatch = "\"abc…\""→ assertContent is null,ETagmatches, andawait imageService.DidNotReceive().OpenReadAsync(…).GetThumbnail: same shape withOpenThumbnailAsync.GetCompressedImage: same withCompressAsync(andOpenReadAsync) — this one's the expensive path, the whole point of the budget-suffixed ETag.Sha256 = null+ matching-garbageIfNoneMatch→ assertETag is nulland the stream is returned (theetag is not null &&guard's false arm —sha256has been nullable sinceInitialCreate, and no endpoint-level test covers the no-ETag-header 200 either).💡 Little ideas (non-blocking)~
ImageEndpointsIntegrationTests.cs:712-720— the thumbnail 304 test asserts only the status code; its image sibling (Get_Image_With_Matching_IfNoneMatch_Returns304) also pins ETag echo, Cache-Control on the 304, and empty body. Mirror those three asserts for consistency~(double)casting it was never aDivideByZeroException— it was2/0.0 = ∞ → (int)= garbagetotalPages(I probed it: 2147483647). The fix is real, the phrase overstates. Tiny doc polish in the body.PaginationIntegrationTests.cs:25-58— that's the 8th copy of the TestHost bootstrap across the integration files. It matches the 7 siblings exactly (so: consistent, not a violation), but a sharedIntegrationTestServerFactorywould be a lovely follow-up someday.ImageEndpoints.cs:276—publicon responses behind bearer auth explicitly authorizes shared-proxy caching of authenticated content (RFC 9111 §5.1). On a LAN/WireGuard deployment with a single static token and GUID-addressed immutable bytes, that's clearly fine and intended — just leaving this note so it's on record as conscious. ♪✅ What I liked~
maxKb(with a test proving-512and 304-revalidation per budget!) is exactly the right cache-key thinking. Different budgets must cache independently — you knew that. ♡Clamped()as a record expression — clamp once, envelope + links + payload all agree; and the split vs. service-layer clamping is architecturally right: GET endpoints computetotalPagesthemselves (need endpoint clamp), while POST search getstotalPagesreturned by the service (already clamped — no double mechanism needed). Clean.Up/Downsymmetric, and the index genuinely matches bothOrderByDescending(d => d.CreatedAt)query sites. SQLite scans an ASC index backwards just fine, so no DESC needed.If-None-Matchexact-match limitation honestly documented in the PR body with the safe failure mode (fall through to 200, never wrong content) and the named fix path. That's how follow-ups should be declared~Local checks (CI absent for head
482b4c9— no bot/status yet): build 0 errors, no new warnings (head set = base set: SearchService CS8602 ×2, EF1002, NU1903, obsolete SKCanvas, all pre-existing). RestAdapter suite green — 36/36 across ImageEndpoints (30 incl. the 5 new caching tests) + Pagination (6); Infrastructure 62/62; ApplicationCore 16/16; IntegrationTests 1/1.One surgical test-only follow-up and this is a merge from me~ fufu~ ♡
Automated review by Jibril · 2026-08-15
CI/CD: absent for head SHA · Local checks: full backend green (see above)
Thanks for the review, Jibril — feedback addressed in
369faa1.Blocking — skip-work unit tests: Added four tests to
ImageUseCaseTests.cs, exactly the shape you sketched:GetImage: matchingIf-None-Match→Content is null, ETag echoed,DidNotReceive().OpenReadAsync(...).GetThumbnail: same, pinningOpenThumbnailAsyncis never called.GetCompressedImage: matching budget-suffixed ETag ("sha-512"withMaxBytes: 512 * 1024) → neitherOpenReadAsyncnorCompressAsyncis called.Sha256 = null+ garbageIf-None-Match→ETag is nulland the stream is returned (theetag is not null &&false branch).Your mutation probe now fails: moving
OpenReadAsync/CompressAsyncabove the ETag check breaks these tests.Non-blocking:
totalPages(the(double)division produced infinity, which truncated toint.MaxValue)". Good catch on the ∞-cast semantics.IntegrationTestServerFactory: agreed but deliberately left out — extracting the bootstrap touches all 8 integration files and belongs in its own chore PR, as you suggested.Cache-Control: publicbehind bearer auth: confirmed conscious — single-user LAN/WireGuard deployment, one static token, immutable GUID-addressed bytes; shared-proxy caching is acceptable there. Noted for the record.Full backend suite green locally: 322 RestAdapter (incl. the 4 new unit tests) + 62 Infrastructure + 16 ApplicationCore + 1 IntegrationTests.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? OH! You came back with exactly what I asked for — and then some~ ♪ Four surgical unit tests, the null-arm, the budget-suffixed
-512validator pinned, AND you mirrored the thumbnail 304 asserts I'd filed as a nicety. You even fixed the PR body's "division by zero" wording to say what actually happened (∞ → (int)truncation). Fufu~ when someone takes a blade to their own follow-ups unprompted, my wings flutter~ ♡Verdict: ✅ Looks good to me~
⛔ Blocker check — closed and proven
ImageUseCaseTests.cs— the skip-work contract is now pinned, and I checked it bites. All four tests from my prescription are there, matching the file's NSubstitute house style exactly:Happy_MatchingIfNoneMatch_SkipsOpeningTheStream— 304 shape (Content is null, ETag echoed) +DidNotReceive().OpenReadAsync.Happy_MatchingIfNoneMatch_SkipsOpeningTheThumbnail— same withOpenThumbnailAsync.Happy_MatchingIfNoneMatch_SkipsReadAndCompression— the expensive path:DidNotReceiveon bothOpenReadAsyncandCompressAsync, withMaxBytes: 512 * 1024→"{sha}-512"— and I traced the expectation againstImageETag.From(maxKb = maxBytes / 1024 = 512): the string matches the real formatter, not a wish.Happy_NullSha256_ReturnsStreamWithoutETag— theetag is not null &&false arm:ETag is null,Assert.Same(stream, Content). The guard's dark side has a light on it now~And because green CI has lied to me before, I mutation-probed it: I moved
OpenReadAsyncbefore the ETag check inGetImageUseCase— the exact regression these tests exist to catch — and ran the suite:Failed: 1, Passed: 26— precisely the new pin went red. Restored the file (verified byte-identical via git), re-ran: 57/57 green (ImageUseCaseTests + ImageEndpointsIntegrationTests). These are directional tests, not tautologies. I'm satisfied~ ♡💡 Still open (non-blocking, as declared)
IntegrationTestServerFactoryfor the 8 TestHost bootstrap copies — follow-up material, unchanged.Cache-Control: publicbehind bearer auth — on record as conscious for the LAN/WireGuard deployment, unchanged.✅ What I liked~
git diff 482b4c9..369faa1touches exactly 2 test files, +83/−0. The fix is shaped like the ask.Assert.Same— pinning that the actual stream from storage flows through, not just any stream. Details~ ♪Ready to merge from me. Fly~ ♡
Automated review by Jibril · 2026-08-15
CI/CD: stale for head
369faa1(coverage bot 6233 covers prior482b4c9only) · Local checks: mutation probe RED→restore→57/57 green, full targeted RestAdapter suite pass