feat: image upload, serving, and page management (Phase 3b Part B) #7
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/image-upload-and-serving"
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?
Phase 3b Part B: Image Upload, Serving, and Page Management
Adds image upload (multipart + ZIP), image/thumbnail serving, page management, and page reordering.
Architecture
Follows the established pattern:
HTTP Endpoint → UseCase (orchestrator) → Service (BL) → EF CoreNew services:
IImageService/ImageService— inspects images viaIImageInspector, stores viaIImageStorage, generates thumbnails viaIThumbnailGenerator, managesImageFileentitiesIPageService/PageService— creates/list/reorder/deletePageentities, keepsVariant.PageCountin syncNew use cases (thin orchestrators):
UploadPagesUseCase— multipart upload → check variant exists → for each file: create image → create page → transaction-wrappedUploadZipPagesUseCase— ZIP upload → extract → for each image: create image → create page → transaction-wrappedGetImageUseCase— serve image file stream with media typeGetThumbnailUseCase— serve WebP thumbnail streamReorderPagesUseCase— check variant exists → reorder pagesDeletePageUseCase— delete page, decrement PageCountListPagesUseCase— list pages for variantREST endpoints
/api/variants/{id}/pages/api/variants/{id}/pages/zip/api/images/{id}/api/thumbnails/{id}/api/variants/{id}/pages/api/variants/{id}/pages/reorder/api/pages/{id}Infrastructure
ImageInfrastructureRegistrationhelper — shared DI registration for image ports, used by bothProgram.csand testsBeginTransactionAsync/CommitAsyncfor atomicityUploadImageFile(string FileName, Stream Content)decouples use cases from ASP.NET'sIFormFileTests (28 new, 149 total)
Integration tests: multipart upload (creates pages + images), ZIP upload, get image (file stream), get thumbnail, list pages, reorder, delete, not-found cases, empty files
Unit tests with NSubstitute mocks: upload (variant not found, happy), ZIP upload (variant not found, happy), reorder (variant not found, happy), delete (happy, not found)
Test results
All 149 tests pass. 0 errors, 0 warnings.
New services: - IImageService/ImageService: inspect, store, thumbnail, CRUD for ImageFile - IPageService/PageService: create, list, reorder, delete, update chapter New use cases (thin orchestrators): - UploadPagesUseCase: multipart upload → inspect → store → thumbnail → pages - UploadZipPagesUseCase: ZIP extract → inspect → store → thumbnail → pages - GetImageUseCase: serve image file stream with media type - GetThumbnailUseCase: serve WebP thumbnail stream - ReorderPagesUseCase: reorder page sort order within variant - DeletePageUseCase: delete page, decrement PageCount - ListPagesUseCase: list pages for variant ordered by SortOrder REST endpoints: - POST /api/variants/{id}/pages (multipart upload) - POST /api/variants/{id}/pages/zip (ZIP upload) - GET /api/images/{id} (serve image) - GET /api/thumbnails/{id} (serve thumbnail) - PUT /api/variants/{id}/pages/reorder - DELETE /api/pages/{id} - GET /api/variants/{id}/pages Infrastructure: - ImageInfrastructureRegistration helper for shared DI registration - Upload use cases wrapped in transactions for atomicity Tests (28 new, 149 total): - Integration: multipart upload, ZIP upload, get image/thumbnail, list pages, reorder, delete, not-found cases - Unit with mocks: upload (variant not found, happy), ZIP (variant not found, happy), reorder (variant not found, happy), delete (happy, not found) All 149 tests pass, 0 errors.Summary
Summary
Coverage
DoujinManager.ApplicationCore - 81.9%
DoujinManager.Infrastructure - 91.3%
pshot
DoujinManager.RestAdapter - 80.2%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 18%
🤖 Hermes automated review: changes requested
Reviewed base
fe993d4→ headefa2205(15 files, +1610/−7). Ran the test suites locally: RestAdapter.Tests 92/92 passed, Infrastructure.Tests 43/43 passed. Architecture is clean (endpoint → use case → service → EF Core), and the security-sensitive pieces are handled well —FilesystemImageStoragevalidates extensions with a regex and derives paths solely from server-generated UUIDs (no path traversal),ZipExtractorhas decompression-bomb protection and never writes extracted entries to disk by name (content goes toMemoryStream, so zip-slip is impossible), and upload endpoints correctly dispose streams.One finding is worth addressing before merge, plus a few minor notes.
Major
backend/src/DoujinManager.Infrastructure/UseCases/ImageUseCases.cs(UploadPagesUseCase.ExecuteAsync~line 30,UploadZipPagesUseCase.ExecuteAsync~line 62)Both upload use cases wrap the per-file loop in
db.Database.BeginTransactionAsync(), butImageService.CreateAsync(ImageService.cs~line 19) writes the image and thumbnail to disk viaimageStorage.SaveAsync/thumbnailStorage.SaveAsyncand then callsSaveChangesAsync(ct)inside the loop. If a later file in the loop fails (e.g. a corrupt image throws ininspector.InspectAsync, or a DB constraint fires), the DB transaction rolls back the rows for earlier files — but their physical files on disk remain, now with no referencing DB row. This leaks storage on every partial-failure upload and leaves unreferenced files that no code path cleans up.Suggested fix: wrap the loop in
try { ... } catch { /* delete already-created images */ ; await tx.RollbackAsync(ct); throw; }. On failure, iterate thepages/created images so far and callimageService.DeleteAsync(image.Id, ct)(which removes both files and the DB row), then rethrow. Alternatively, defer filesystem writes until after the transaction commits — but the compensating-delete approach is the smaller change.Minor
ImageEndpoints.csPOST/api/variants/{variantId}/pages(~line 44) iteratesform.Fileswithout a cap on file count or total bytes. ASP.NET's defaultRequestFormLimitsapply, but an explicit cap (e.g. reject > N files or > M bytes total) would make the limit intentional and protect the thumbnail generator (Skia) from doing unbounded work. The ZIP path is protected bymaxEntrySizeBytesper entry but has no cap on entry count.SaveChangesAsyncper image inside the transaction —ImageService.CreateAsyncdoes one DB round-trip per file. For large uploads this is N round-trips. Not a correctness issue; consider batching if upload sizes grow.DeletePageUseCasedoes not delete the associatedImageFile—PageService.DeleteAsync(PageService.cs~line 110) removes the page row and decrementsPageCountbut leaves theImageFileentity and its filesystem files behind. This is safe if images may be shared across pages, but if the page→image relation is 1:1 (as the upload path implies), it leaks files over time. Worth a confirm-and-document or a cascade delete.ReorderAsyncsilently ignores unknown/duplicate page IDs —PageService.cs~line 78: apageIdin the request that doesn't belong to the variant is dropped without error (theorderMaplookup simply never matches), and duplicate IDs in the input collapse via dict overwrite. If strict validation is desired, reject unknown IDs; otherwise document the lenient behavior.ImageEndpoints.cs:44multipartOpenReadStream()— streams are correctly disposed after the use case returns, but on the early-return400path (form.Files.Count == 0) no streams are opened, so that's fine. Just noting the disposal pattern is correct.The pre-existing
CS8602/CS8604nullable warnings inDoujinEndpoints.csandDoujinCrudIntegrationTests.csare not introduced by this PR.Automated daily review. I never merge PRs. Note: this is a PR conversation comment with file:line references, not a formal Forgejo review approval — the MCP integration cannot create inline review comments or approval states.
Addressed all review findings in commit
e72b654. 149 tests pass.Major — Orphaned files on transaction rollback ✅
Both
UploadPagesUseCaseandUploadZipPagesUseCasenow wrap the file loop intry/catch. On failure:This prevents orphaned image/thumbnail files on disk when a later file in the upload fails.
Minor fixes
Upload count caps — Multipart upload limited to 100 files per request. ZIP upload limited to 500 entries. Both reject with clear error messages.
DeletePageUseCase cleans up images — Now injects
IImageServiceand deletes the associatedImageFile(both filesystem files and DB row) after removing the page. Prevents storage leaks over time.ReorderAsync validates page IDs — Now rejects:
ArgumentExceptionwith the unknown IDs listed)ArgumentException)Previously these were silently ignored.
Not addressed (deferred)
🤖 Hermes automated review: changes requested
Re-reviewed the fix commit
e72b654(basefe993d4, delta from previous headefa2205). CI/CD: CI / build ✅ (19s) and CI / test ✅ (26s) both passed for heade72b654— local build/test skipped per CI policy.The minor items from the previous review are all properly addressed:
DeletePageUseCasenow cleans up the associatedImageFileReorderAsyncnow validates unknown/duplicate page IDsHowever, the major finding — orphaned files on rollback — is not actually fixed by the compensating-delete approach. Here is why:
Major: Compensating delete is ineffective after rollback
backend/src/DoujinManager.Infrastructure/Services/ImageService.cs:79-82:The compensating-delete path in
ImageUseCases.cscallsimageService.DeleteAsync(imageId)afterawait tx.RollbackAsync(ct). The problem:ImageService.CreateAsyncwrote eachImageFilerow viaSaveChangesAsyncinside the transaction.tx.RollbackAsync()removes those rows from the database.DeleteAsyncthen runsFirstOrDefaultAsync(i => i.Id == id)— this is a LINQ query that always hits the database (unlikeFind(), which checks the change tracker first).null→DeleteAsyncreturnsfalse→ the physical image/thumbnail files on disk are never deleted.The
createdImageslist stores onlyImageFileIdvalues — it does not retain theExtensionneeded to construct the storage path. So even if the delete method were restructured, the extension information is lost once the DB row is gone.This means the original orphaned-files bug is still present. The physical files written by
imageStorage.SaveAsync/thumbnailStorage.SaveAsyncremain on disk after a partial-failure upload, with no referencing DB row.Suggested fix (pick one):
Track full entities — change
createdImagesfromList<ImageFileId>toList<ImageFile>, and add anIImageServicemethod likeDeleteFilesAsync(ImageFile image, ct)that deletes physical files directly usingimage.Extensionwithout querying the DB:Then in the catch block:
foreach (var img in createdImages) { try { await imageService.DeleteFilesAsync(img, ct); } catch { } }Defer filesystem writes — buffer image bytes in memory, commit the DB transaction first, then write files to disk only after
CommitAsyncsucceeds. This is cleaner but uses more memory for large uploads.Verify with a test — if you believe the change tracker returns the entity after rollback, add an integration test that forces a mid-loop failure (e.g., inject a corrupt image as the 2nd file in a 2-file upload) and assert the first file's image/thumbnail are physically deleted from disk. The current test suite uses NSubstitute mocks for
IImageService, so the real DB-after-rollback behavior is untested.The cleanest approach is option 1 — it's a small change and makes the compensation independent of DB state.
Automated daily review. I never merge PRs. Note: this is a PR conversation comment with file:line references, not a formal Forgejo review approval — the MCP integration cannot create inline review comments or approval states.
Fixed the compensating delete in commit
cd85bde. The reviewer was exactly right —DeleteAsyncqueries the DB, which is useless after rollback.What changed:
IImageService.DeleteFilesAsync(ImageFile image, ct)— new method that deletes physical files (image + thumbnail) directly usingimage.Idandimage.Extension, without any DB query. Independent of transaction state.createdImageschanged fromList<ImageFileId>toList<ImageFile>— retains the full entity includingExtension, so the catch block has everything it needs to delete files.Catch blocks now call
DeleteFilesAsyncinstead ofDeleteAsync— works correctly afterRollbackAsyncbecause it never touches the DB.The compensating delete is now truly independent of DB state. 149 tests pass.
Added integration test in commit
623bc41. 150 tests pass.Upload_WithCorruptSecondFile_RollsBackAndCleansUpFiles— proves the compensating delete actually works:image/jpegcontent type)500(corrupt image throws in SkiaSharp during inspection)Directory.GetFiles(_imageDir, "*", SearchOption.AllDirectories)is empty_thumbnailDirThis test would have failed with the previous
DeleteAsyncapproach (files would remain on disk) and passes with theDeleteFilesAsyncfix.🤖 Hermes automated review: no blocking issues found
Re-reviewed the new commits since the last review (
e72b654→623bc41, delta +64/−10 across 4 files). CI/CD: CI / build ✅ (21s) and CI / test ✅ (27s) both passed for head623bc41— local build/test skipped per CI policy.The major finding from the previous review — compensating delete was ineffective after rollback — is now correctly fixed:
✅ Major: Compensating delete now works after rollback
Commit
cd85bdeimplements exactly the approach suggested in the previous review:createdImageschanged fromList<ImageFileId>→List<ImageFile>— retains the full entity includingExtension, which is needed to construct the storage path.ImageUseCases.cs:39, 97IImageService.DeleteFilesAsync(ImageFile image, ct)— deletes physical files (image + thumbnail) directly usingimage.Idandimage.Extension, with no DB query. Confirmed independent of transaction state:FilesystemImageStorage.DeleteAsync(id, extension)andFilesystemThumbnailStorage.DeleteAsync(id)are pure-filesystem operations.ImageService.cs:95-101FileNotFoundExceptionpaths swallowed — makes the cleanup idempotent and safe even if a file was already gone. Correct.UploadPagesUseCaseandUploadZipPagesUseCase— the catch blocks now callDeleteFilesAsync(image, ct)instead of the DB-dependentDeleteAsync(imageId, ct).ImageUseCases.cs:59-65, 123-129This resolves the root cause: the previous
DeleteAsyncranFirstOrDefaultAsyncagainst a DB whose rows had just been rolled back, so it returnednulland never deleted the physical files.DeleteFilesAsyncbypasses the DB entirely.✅ Integration test proves the fix
Commit
623bc41addsUpload_WithCorruptSecondFile_RollsBackAndCleansUpFiles:image/jpegcontent-type) → corrupt image throws in SkiaSharp during inspection → triggers the rollback path_imageDirand_thumbnailDir(usesSearchOption.AllDirectories)PageCount == 0(confirms DB rollback)The test fixture (
ImageEndpointsIntegrationTests.cs:25-64) wires real temp directories into aWebApplicationFactoryviaImageInfrastructureRegistration.Register, so this exercises the real storage layer — not a mock. This test would have failed against the previousDeleteAsyncimplementation (files would have remained on disk). Good regression coverage.Minor / non-blocking
catch { /* best-effort cleanup */ }aroundDeleteFilesAsyncswallows all exceptions, not just file-related ones. IfimageStorageorthumbnailStorageever throws something unexpected (e.g. a permissions error worth surfacing), it will be silently dropped. Acceptable for a compensating path, but a debug-level log there would aid future diagnosis.SaveChangesAsync-per-image observation from the first review remains a non-correctness performance note (deferred as intended).Verdict: The fix is precise, correctly addresses the rollback-stale-files bug, and is backed by a real integration test. No blocking issues.
Automated daily review. I never merge PRs. Note: this is a PR conversation comment with file:line references, not a formal Forgejo review approval — the MCP integration cannot create inline review comments or approval states.