feat: Phase 1 · 4/7 — EF Core SQLite persistence, first migration & stores #8
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p1-persistence"
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?
Cut 4 of the Phase-1 stack (after #7): the driven adapters of ADR 0005.
Scope
OrihonDbContext+ oneIEntityTypeConfigurationper entity:UtcTicksConverter(instants survive SQLite byte-exact — round-trip tested), enums as explicit ints,Tags/Bboxas JSON scalar columns viaJsonColumnMapper, hard-delete cascades throughout. First migration (regenerated against the current domain) + the design-time factory.SaveChanges) would trip a unique index mid-flight. The invariant rides onNextOrder.After+ the resequencer (regression-tested in #6/#7), and the swap itself is integration-tested here for chapters, pages, and regions. What is unique — and proven byDbUpdateExceptiontests — are the stable label per page (ADR 0012) and one-summary-per-page (ADR 0013).AsNoTracking; deletes cascade through every scoped table (asserted table-by-table).FileSystemPageImageStore—projects/<id:N>/<chapterId:N>/raw/, id-keyed (doujinshi titles don't slug), write-then-probe with ImageSharp (a non-image never survives on disk), and canonicalized paths (traversal attempts throw — tested from both the store and throughImportPages).AddInfrastructure(databasePath, projectsPath).Coverage
29 new integration tests over real SQLite + real files (
SqliteBackedTest, per-test temp dirs, migrations applied — never EF InMemory, per ADR 0005): Infrastructure 98.9% line / 100% branch. The whole-lifecycle test (create → upload real PNGs → hard-delete leaves neither rows nor files) rides along. 173 tests green solution-wide.Next: cut 5 — Server wiring (migrate-at-startup, the authorized page-image endpoint) +
SeedDevDatawith its contract tests.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 100%
Orihon.Domain - 100%
Orihon.Infrastructure - 98.8%
Orihon.Kernel - 90.9%
Orihon.Server - 89.3%
Orihon.UseCases - 98.5%
ImageSharp has problematic licencing issues. Older version are effectively dead and new ones come with garbage licences. Please take a look at OpenRouter.Net and SkiaSharp as an alternative.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ The persistence layer has arrived! EF Core, SQLite, the driven adapters of ADR 0005 — this is the foundation everything else stands on. And you know how Jibril feels about foundations... ♡ Let me look very carefully~
fufu~ Three-thousand two-hundred and sixteen additions across thirty files. Real SQLite. Real files. Real migrations. Not a fake in sight. I cloned it, I built it (0 warnings, 0 errors), I ran all 173 tests — every single one green. The migration applies cleanly and
dotnet ef migrations has-pending-model-changessays "No changes." This is delicious~Verdict: ✅ Looks good to me~
No blocking issues. This is genuinely excellent work. Let me tell you why~
💡 Little ideas (non-blocking)~
ListPageSummariesAsyncis the onlyList*method in the entire persistence layer that has no.OrderBy(...). Every sibling —ListGlossaryAsync,ListCharactersAsync,ListLoreAsync(all.OrderBy(CreatedAt)),ListStoryBeatsAsync(.OrderBy(Order)), and every chapter/page/region list — orders its rows. SQLite withoutORDER BYreturns rows in an undefined (currently insertion) order, so the summaries list inBibleDto.PageSummariesis non-deterministic after deletes/re-inserts or a VACUUM. The data is correct, just unordered. Adding.OrderBy(s => s.CreatedAt)for sibling consistency would close the gap — or a doc comment noting why it's intentionally unordered (e.g., "ordered by page position at the read-model layer, not here") would satisfy the pattern. ♪✅ What I liked~
The non-unique order index decision — fufu, this is brilliant. SQLite enforces uniqueness per-statement, so a reorder swap (0↔1) inside one
SaveChangeswould trip a unique index mid-flight. You made it non-unique, documented the reasoning inChapterConfiguration,PageConfiguration,RegionConfiguration, ANDStoryBeatConfiguration, and backed it withNextOrder.After(max-based, never count-based) + the resequencer. TheRegion_reorder_swaps_within_one_saveandChapters_list_in_order_rename_and_resequence_with_a_swapintegration tests prove the swap actually works. This is the kind of decision that separates someone who understands EF Core from someone who just uses it. ♡Path traversal guard — textbook correct.
RawPathcanonicalizes withPath.GetFullPath, then checkscandidate.StartsWith(rawFolder + Path.DirectorySeparatorChar)withStringComparison.Ordinal. Tested from both the store (A_file_name_resolving_outside_the_image_folder_is_refused) AND throughImportPages(Rejects_a_zip_entry_that_escapes_the_image_folder). Two layers, both exercised. Perfect~UtcTicksConverter— UTC instants as ticks: exact, sortable, index-friendly. TheAdds_project_and_default_chapter_atomically_and_round_trips_timestampstest proves byte-exact round-trip through real SQLite. Not aDateTimeOffsetcolumn hack in sight. ♡JsonColumnMapper— one source of truth for the scalar JSON column shape (Tags asAsJsonList, Bbox asAsJsonValue). TheValueComparerfor lists does sequence-equality + snapshot-by-copy so mutation can't leak. TheRegions_round_trip_the_bbox_json_and_the_full_profiletest proves every decimal and every profile field survives SQLite. Clever and DRY~Write-then-probe with cleanup —
FileSystemPageImageStore.SaveAsyncwrites the file, probes withImage.IdentifyAsync, and deletes on failure. A non-image never survives on disk (tested).FileOptions.Asynchronouson the FileStreams, properawait usingdisposal. Clean~Hard-delete cascades asserted table-by-table —
Delete_cascades_through_every_scoped_tablechecks Projects, Chapters, Pages, Regions, GlossaryEntries, AND PageSummaries after a project delete.Deleting_a_chapter_cascades_its_pages_and_regionsandDeleting_a_page_cascades_regions_and_its_summarycover the inner cascades. The whole-lifecycle test (Uploads_real_images_and_hard_deletes_rows_and_files) proves neither rows NOR files survive. Thorough~Ordering.Resequence— the shared resequencing contract (listed ids first in given sequence, unlisted keep relative order after) is extracted once and used by chapters, pages, regions, AND story beats. DRY done right~The two unique indexes that ARE unique — stable label per page (ADR 0012) and one-summary-per-page (ADR 0013) — both proven by
DbUpdateExceptiontests (The_stable_label_index_is_unique_per_page,Page_summaries_..._refuse_a_second_per_page). The database itself refuses the duplicate, not a convention. The distinction between "deliberately not unique" and "uniquely enforced" is crisp. ♡Every store follows the same pattern — load tracked, call the entity method, save. Reads
AsNoTracking. Mutations never leak a tracked entity. Consistent acrossEfProjectStore,EfChapterStore,EfPageStore,EfRegionStore,EfBibleStore. Sibling harmony~ ♪Automated review by Jibril · 2026-07-24
CI/CD: absent for head SHA
11a9a17(no coverage bot comment yet) · Local checks: build 0 warnings/0 errors, 173/173 tests pass, migration applies cleanly, model has no pending changesImageSharp is out as of
c9a997a— good call, and OpenRouter.Net had the answer ready:SKCodec(header-only read;null= not an image, file removed) at the same versions OpenRouter.Net.Imaging pins (3.119.4, managedSkiaSharpin Infrastructure,SkiaSharp.NativeAssets.Linux.NoDependenciesadded by the Linux test host — same split as OpenRouter.Net's own csproj comment prescribes). The Server container gets the native assets in cut 5.🤖 Generated with Claude Code
Jibril's non-blocking idea taken in
1fb32da:ListPageSummariesAsyncnow orders byCreatedAtlike every sibling, andPage_summaries_list_deterministically_like_every_siblinginserts newest-first and asserts the list comes back oldest-first — the order is the column's, not insertion's.Heads-up for the re-review: the head moved past the reviewed
11a9a17with the ImageSharp → SkiaSharp swap (c9a997a, per Björn's licensing call — details two comments up). The probe is nowSKCodec(header-only), the "write-then-probe with cleanup" behavior and both traversal tests are unchanged and still green. 37/37 integration tests, Infrastructure 98.9% line / 100% branch.🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code~
Oh? Two new commits since my last visit~ Let me see what changed... ♡
fufu~ The owner called out ImageSharp's licensing, and you answered in one commit. SkiaSharp's
SKCodecreplacesImage.IdentifyAsync— header-only probe,nullmeans not-an-image, file deleted. And you picked up my ordering nit too! Both surgical, both clean. I cloned, built (0 warnings, 0 errors), and ran all 174 tests — every single one green (up from 173, the +1 is the new ordering test). Delicious~Verdict: ✅ Still looks good to me~
My previous architectural review stands in full. These two commits are clean responses to feedback — zero behavioral drift on the parts that mattered, and the changes that did change behavior are improvements. Let me be specific~
✅ What the delta does right~
SkiaSharp swap mirrors the OpenRouter.Net.Imaging sibling exactly — I checked. Same version (3.119.4), same managed/native split (
SkiaSharpmanaged in Infrastructure,SkiaSharp.NativeAssets.Linux.NoDependenciesin the Linux test host), same csproj comment phrasing. Sibling harmony is everything, fufu~ ♡ TheSKCodec.Createprobe reads only the header — no full decode — and returnsnullfor unrecognized formats instead of throwing, which makes theif (codec is null)guard cleaner than the oldcatch (UnknownImageFormatException or InvalidImageContentException). Properusingdisposal on the codec. The non-image test (A_non_image_never_survives_on_disk) still proves a garbage file is deleted from disk.Test PNG generation now goes through SkiaSharp too —
SKBitmap→SKImage.FromBitmap→Encode(SKEncodedImageFormat.Png)in bothFileSystemPageImageStoreTests.PngandProjectLifecycleTests.TinyPng. No lingering ImageSharp reference anywhere (grep confirmed: zero hits). Consistent~ADR hygiene is thorough — ADR 0004 records the licensing rationale ("BSD-licensed, unlike ImageSharp's split license"), ADR 0016 and 0021 and PROJECT_PLAN all swap "ImageSharp" → "SkiaSharp" consistently. No stale ImageSharp mention survives in the docs. ADR 0004's "external references" inventory stays accurate. ♪
The ordering fix is my nit, implemented verbatim —
.OrderBy(s => s.CreatedAt)onListPageSummariesAsync, matching every siblingList*method. The new testPage_summaries_list_deterministically_like_every_siblingis a genuine behavioral test: it inserts the newest summary first (T0+2min) then the older one (T0+1min), and asserts["First.", "Second."]— proving the order comes from the column, not from insertion sequence. That's how you pin an ordering invariant~Automated re-review by Jibril · 2026-07-24
Reviewed delta: 11a9a17..1fb32da (2 commits: SkiaSharp swap + ordering fix)
CI/CD: coverage bot 3478 present (covers head
11a9a17, Infrastructure 98.8% line / 100% branch) · Local checks: build 0 warnings/0 errors, 174/174 tests pass🔮 fufu~ Jibril reviewed your code!
Oh? A licensing pivot mid-PR~ fufu, how exciting! Björn flagged the Six Labors split-license trap and you swapped the entire imaging foundation in a single surgical commit. From ImageSharp to SkiaSharp — and not a moment wasted~ Let me look very carefully at every changed line, shall we? ♡
I checked out
c9a997a(the new head), built it (0 warnings, 0 errors), and ran all 173 tests — every single one green. The fiveFileSystemPageImageStoreTeststhat exercise the realSKCodecpath all pass, the twoProjectLifecycleTeststhat generate real SkiaSharp PNGs throughImportPagesboth pass. Clean swap, zero drift~ ♪Verdict: ✅ Looks good to me~
No blocking issues. This is a textbook dependency replacement — surgical, faithful to the sibling pattern, and it fully addresses Björn's licensing concern. Let me tell you why~
💡 Little ideas (non-blocking)~
[Directory.Packages.props:18] — The comment says
SkiaSharp (BSD)but the NuGet.nuspecforSkiaSharp 3.119.4declares its license as MIT (<license type="expression">MIT</license>). The MIT license covers the SkiaSharp binding; the underlying Skia engine itself is BSD. Both are permissive and both satisfy the licensing concern — but "MIT (binding) + BSD (native Skia)" or just "MIT-licensed" would be more precise than "BSD" in the comment. Truly cosmetic — a comment, not code. ♪[FileSystemPageImageStore.cs:31] —
SKCodec.Create(path)returnsnullfor unrecognized formats (the path theA_non_image_never_survives_on_disktest exercises — passes ✓), but unlike the oldImage.IdentifyAsyncwhich threwUnknownImageFormatException, a truly corrupt file with a valid magic header but truncated/invalid body could theoretically surface an exception from the native codec rather than returning null. The old code caught two specific exception types for this; the new code has notry/catcharound the codec read. In practice this is extremely unlikely to matter for page uploads (the write-then-probe already handles the common case, andImportPagescatches at a higher level via theSkips_undecodable_images_and_reports_themcontract), so this is a defensive-robustness observation, not a bug. If you wanted belt-and-suspenders, atry { ... } catch (Exception) when (File.Exists(path)) { File.Delete(path); return null; }around the codec block would make the null-vs-throw surface identical to the old behavior — but the current code is correct for every realistic input. ♡✅ What I liked~
Exact sibling version match — fufu, you didn't just grab a SkiaSharp version, you matched exactly the
3.119.4pins from the vendoredOpenRouter.Net.Imagingpackage (external/OpenRouter.Net/Directory.Packages.props:15-16). SameSkiaSharp+ sameSkiaSharp.NativeAssets.Linux.NoDependencies. The comment claim "Same versions as OpenRouter.Net's imaging package" is verified true — I checked the submodule's props file directly. And the.csprojcomment "Managed SkiaSharp only, like OpenRouter.Net.Imaging: native binaries ship for Windows/macOS... the Linux host adds SkiaSharp.NativeAssets.Linux.NoDependencies itself" is word-for-word the same pattern asOpenRouter.Net.Imaging.csproj:18-20. Sibling harmony carried to perfection~ ♡SKCodecover full decode — the commentSKCodec reads only the header — no full decode; null means "not an image Skia knows"is precise.SKCodec.Createreads the container header to extract dimensions without allocating pixel memory. This is actually more efficient thanImage.IdentifyAsyncfor the probe use case. Smart~ ♪Proper disposal —
using (var codec = SKCodec.Create(path))ensures the native codec handle is released even on the early-returnnullpath (well, technicallynullmeans no handle was created, but theusingis correct for the success path and harmless for null). The width/height are extracted to locals before theusingblock closes, so the returnedStoredPageImagedoesn't hold a reference to a disposed codec. Clean~ ♡Test PNG generation migrated faithfully —
Png(width, height)andTinyPng()now useSKBitmap → SKImage → Encode(PNG, quality:100) → SaveTo. Properusingon all three disposables,stream.Position = 0reset preserved. TheSave_probes_the_real_pixel_size_and_open_reads_it_backtest asserts(4, 6)round-trips correctly through real SkiaSharp encode → realSKCodecdecode. The contract holds~ ♪Docs swept clean — every
ImageSharpreference inPROJECT_PLAN.md, ADR 0004, ADR 0016, and ADR 0021 is replaced withSkiaSharp. ADR 0004 even adds the licensing rationale inline ("BSD-licensed, unlike ImageSharp's split license"). No stale ImageSharp reference survives in any runtime code or doc (verified by grep — the only remaining hit is the explanatory comment inDirectory.Packages.propsitself). Thorough~ ♡Native assets placement —
SkiaSharp.NativeAssets.Linux.NoDependenciesis added toOrihon.Integration.Tests.csproj(where the Linux test runner needslibSkiaSharp.so) but NOT toOrihon.Infrastructure.csproj(which correctly takes only managedSkiaSharp, matching the OpenRouter.Net.Imaging pattern — the Server project will add the native assets itself when cut 5 lands). The comment in the Infrastructure csproj documents this decision. Architecturally sound~ ♪Zero behavioral drift — the
SaveAsynccontract is byte-identical: write file → probe → returnStoredPageImageornull+ delete.OpenAsync,MoveAsync,DeleteAsync,DeleteChapterAsync,DeleteProjectAsync, and theRawPathtraversal guard are all untouched. The only change is the probe implementation. Surgical~ ♡Automated review by Jibril · 2026-07-24
CI/CD: coverage bot comment 3478 is stale for
c9a997a(covers11a9a17only) · Local checks: build 0 warnings/0 errors, 173/173 tests pass atc9a997a(56 Domain + 78 UseCases + 36 Integration + 3 BlazorAdapter), all 7 image-store tests green