Add the Entity Framework Core persistence package by DutchyD · Pull Request #2 · CodeByDylan/Loom

DutchyD · 2026-07-30T04:32:25Z

Adds Loom.Persistence.EntityFrameworkCore, the single seam where Loom touches an object-relational mapper, and closes the three gaps that its absence left open.

Before this, a specification declaring eager loading could not be applied at all — it threw, naming the missing package. Paging could not count asynchronously. Domain events were collected and never delivered.

What it does

Concern
Identity	Registers a conversion for every `Id<TEntity>` in an assembly, so identities are stored as their underlying value.
Specifications	`ApplySpecification` applies eager loading, criteria and ordering together.
Paging	`ToPageAsync` counts and fetches, skipping the fetch when the page starts past the end.
Domain events	A save interceptor drains changed aggregates and dispatches before the commit.
Deferred events	An optional outbox delivers after the commit, at least once.

Provider-neutral: it references the mapper and its relational companion, and no database provider. A consumer picks its own.

Decisions worth reviewing

Events dispatch before the commit. Anything a handler changes joins the same save, so the operation is atomic, and a handler failure abandons the save. The alternative leaves a change committed with the reaction silently lost, which is the worst outcome available. The costs are real and covered by tests: a handler cannot see its own transaction committed, and must not reach outside the process.

Deferred delivery is opted into per event, not globally. An immediate handler may change data atomically but must not send an email; a deferred handler may send an email but must be idempotent. Those contracts conflict, so a global switch would silently invert the meaning of every handler in a codebase. The choice is declared on the event, where a handler author will see it.

Domain event handlers are a separate abstraction from request handlers. Fan-out rather than request/response: many per event, invoked by infrastructure, no decorator chain. Keeping them separate is what preserves the rule that request dispatch has no notifications.

A handler returns a failure rather than throwing, but abandoning a save is only expressible as an exception, so the failure is wrapped in one carrying the error and the event type for a caller to translate back.

An abandoned outbox message is left in place, not deleted. It is evidence of a bug.

Two things found by building it, not by reasoning about it

Identity conversion has to be registered as a convention, not during model creation. Property discovery skips properties whose type it does not recognise, so an identity that is not a primary key never enters the model and there is nothing left to configure later. The first implementation walked the finished model and silently did nothing for foreign identities. A test querying on one now covers it.

Timestamps could not be stored as offset-bearing instants. SQLite refuses to order by them, so the outbox delivery query failed outright. They are stored as UTC ticks, which sorts identically on every provider.

A correction to existing guidance

The rules previously said an entity must be materialised through its identity constructor, because a parameterless one would mint a fresh identity and detach the object from its row. That is not what happens — the mapper writes the identity from the column and discards the minted one. Following the old rule also makes the model fail to build whenever a domain constructor takes one parameter, since the mapper cannot choose between them. Corrected in both rule sets.

Testing

Against SQLite in memory rather than a container. Everything here is provider-neutral mapper behaviour, so a real relational provider is sufficient fidelity while keeping the suite fast and CI free of a container runtime — and as above, it caught a genuine provider incompatibility that a single-provider suite would have hidden.

Tests wire the container the way a consumer would, so an event handler receives the same scoped context the save is running on. That is the property most worth covering, and constructing the pieces directly cannot express it.

153 tests, no failures, no warnings under warnings-as-errors, stable across repeated runs. A vulnerable transitive dependency of the test-only provider was pinned forward rather than suppressed.

Known gaps

An abandoned message has nowhere to be seen or retried from. Recorded on the roadmap; worth building once something has actually been abandoned in anger.
Nothing here has been exercised by a real application. Still the main thing this does not prove.

Summary by CodeRabbit

New Features
- Added Entity Framework Core persistence support, including identity mapping, specifications, eager loading, and asynchronous paging.
- Added domain event handling during saves, with support for cascading events and transactional failure handling.
- Added deferred domain events with configurable outbox storage, retries, delivery tracking, and background processing.
- Added configuration APIs for registering persistence, event dispatching, and outbox behavior.
Documentation
- Updated persistence guidance, roadmap status, API conventions, and dependency policies.
Tests
- Added comprehensive coverage for paging, queries, identity mapping, domain events, and outbox delivery.

Add the foundational Loom packages

Adds the contract for reacting to a domain event, and a marker distinguishing events delivered after their transaction commits from those delivered inside it. A handler returns a result rather than throwing. For an ordinary event that lets a failure abort the transaction the operation belonged to, so nothing is committed; for a deferred event it marks the delivery as failed and leaves it to be retried. This is what makes the result reference load-bearing here rather than speculative. The handler contract is fan-out and is deliberately not the request handler abstraction. Any number may exist for one event, infrastructure invokes them all, and nobody awaits a value. Keeping the two as separate types is what preserves the rule that request dispatch has no notifications; collapsing them is the change that would break it. The deferred marker names the guarantee rather than the mechanism, so the contract is visible where an event is declared instead of being determined by a configuration call elsewhere. The two contracts genuinely conflict: an ordinary handler may change data atomically but must not reach outside the process, while a deferred handler may reach outside the process but must be idempotent. A global switch would silently invert both.

…port Introduces the single Entity Framework Core seam. Provider-neutral: it references the ORM and nothing else, so a consumer chooses its own database provider. Identity mapping is registered from ConfigureConventions rather than OnModelCreating, and the distinction is not cosmetic. Property discovery skips properties whose type it does not recognise, so an identity that is not a primary key never enters the model at all and there is nothing left to configure by the time OnModelCreating runs. Registering the conversion as a convention happens first and makes the type recognisable. An earlier attempt that walked the finished model silently failed for exactly this reason, and a test querying on a foreign identity now covers it. Identities are found by looking for entity types rather than by scanning for closed identity types, since a foreign identity appears nowhere but as a property type. One call covers a whole assembly instead of naming each entity and failing at run time when a new one is forgotten. Specification application is named ApplySpecification rather than Apply because the specification package's own overload is unconstrained while this one requires a reference type, so both would be applicable to the same call and importing both namespaces would fail to compile with an ambiguity that is tedious to diagnose. Eager loading is applied first so that filtering and ordering compose over the loaded graph. Paging counts and fetches in two round trips, skipping the fetch when the requested page starts past the end. Tests run against SQLite in memory rather than a container. Everything here is provider-neutral ORM behaviour, so a real relational provider is enough fidelity while keeping the suite fast and free of a container runtime. A consumer testing its own queries against its own provider is a separate concern.

Drains events from changed aggregates and dispatches them before the save executes, so anything a handler changes joins the same save and the operation is atomic. A handler failure abandons the save, so a rule meant to react to a change cannot silently fail while the change itself is committed. A handler returns a failure rather than throwing, but abandoning a save is only expressible as an exception, so the failure is wrapped in one that carries the error and the event type. A caller reporting outcomes as results translates it back at its boundary. Draining repeats, because a handler may raise further events, and stops after a fixed number of passes so that two aggregates raising events at each other produce a diagnosable error rather than a hang. Handlers are resolved as a sequence, so an event with no handlers is not an error and every handler for an event runs. Dispatch stops at the first failure, since the transaction is about to be abandoned and further work would be discarded. The limitations are deliberate and covered by tests rather than left implicit: a handler cannot see its own transaction committed, and a deferred event raised with no outbox configured is refused rather than quietly downgraded to immediate delivery. Tests wire the container the way a consumer would, so a handler receives the same scoped context the save is running on. An earlier version constructed the pieces directly and could not express that, which is the property most worth covering.

Delivers events marked as deferred after their transaction commits, at least once. The record of what is owed is written through the same context as the change that raised it, so it commits in the same transaction — that atomicity is the entire point, and is what an after-the-fact dispatch cannot offer. Opting in is per event rather than global. An immediate handler may change data atomically but must not reach outside the process; a deferred handler may reach outside the process but must be idempotent. Those contracts conflict, so a global switch would silently invert the meaning of every handler in a codebase. The choice is therefore visible where an event is declared. Timestamps are stored as UTC ticks rather than as offset-bearing instants. Providers disagree on whether such a value can be ordered at all — SQLite refuses outright — so a package that must order by time cannot store the native type and stay provider-neutral. This was found by the tests rather than reasoned about in advance. Delivery is deliberately resilient in three ways, each covered by a test: a message that cannot be parsed or whose handler throws is recorded as failed rather than stopping the pass, so one poisoned message cannot block everything behind it; a message that exhausts its attempts is abandoned rather than retried forever; and an abandoned message is left in place rather than deleted, because it is evidence of a bug. Event types are resolved by full name without assembly version, so that a version bump does not orphan messages already recorded, and only from assemblies the consumer names, which keeps resolution unambiguous. The delivery loop is separated from the background service that drives it, so a pass can be run and asserted on without waiting for a timer. The table is mapped explicitly rather than added implicitly: a package that silently adds tables to someone else's database produces migrations nobody expected.

Corrects a rule that was wrong. The guidance said an entity must be materialised through its identity constructor, on the reasoning that a parameterless one would mint a fresh identity and detach the object from its row. That is not what happens: the mapper writes the identity from the column, discarding the minted one. Worse, following the old rule makes the model fail to build whenever a domain constructor also takes one parameter, because the mapper cannot then choose between them. Entities should expose a private parameterless constructor instead. Also records where identity conversion is registered and why the distinction matters, since registering it from OnModelCreating silently fails for any identity that is not a primary key. Adds three rules learned while building the package: a request handler and a domain event handler are separate abstractions on purpose; an instant that must be ordered is stored as ticks because providers disagree about ordering offset-bearing values; and one third-party dependency means one product rather than one package identifier, which is what allows the relational companion alongside the mapper itself while still excluding a database provider.

coderabbitai · 2026-07-30T04:32:33Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 32c3589f-daa7-488c-928b-9bdac53be823

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.39% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding a new Entity Framework Core persistence package.

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 5

🧹 Nitpick comments (2)

src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxDeliveryService.cs (1)
22-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Pass-level failures are swallowed with no logging.

A failed pass (e.g., SaveChangesAsync failure) is silently ignored — there's no way to observe recurring failures in production. This aligns with the PR's own disclosed gap ("no operational visibility... for abandoned outbox messages"); consider injecting an ILogger here to at least surface the exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxDeliveryService.cs`
around lines 22 - 39, The generic exception handler in OutboxDeliveryService’s
delivery loop currently swallows pass-level failures; inject or reuse an ILogger
and log the caught exception with clear delivery-pass context before continuing
service execution.
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxEventSerializer.cs (1)
12-12: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Static type-resolution cache is shared across all OutboxOptions configurations.

ResolvedTypes is static, so it's shared by every OutboxEventSerializer instance regardless of the EventAssemblies it was configured with. Combined with the shared-singleton issue in LoomPersistenceOptions.UseOutbox (multiple contexts), this could return the wrong Type or an incorrect ambiguity error for a context whose assemblies differ from whichever configuration first resolved that type name. Making the cache an instance field would scope it correctly per serializer/configuration.
♻️ Proposed fix
-internal sealed class OutboxEventSerializer(OutboxOptions options)
+internal sealed class OutboxEventSerializer(OutboxOptions options)
 {
-    private static readonly ConcurrentDictionary<string, Type> ResolvedTypes = new();
+    private readonly ConcurrentDictionary<string, Type> _resolvedTypes = new();
(and update ResolveType to use _resolvedTypes instead of ResolvedTypes.)
Also applies to: 33-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxEventSerializer.cs` at
line 12, Make the type-resolution cache in OutboxEventSerializer instance-scoped
rather than static, declaring it as _resolvedTypes so each serializer uses its
own EventAssemblies configuration. Update ResolveType and all related cache
accesses to use _resolvedTypes, preserving the existing resolution and ambiguity
behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/agents/00-core.md`:
- Line 1: Remove the explicit “EF Core 10” version from the Data row in the
shared core template, leaving only the dependency and provider names. Keep
version ownership in Directory.Packages.props as required by the template’s
dependency guidance.

In `@src/Loom.Persistence.EntityFrameworkCore/DomainEventInterceptor.cs`:
- Around line 46-59: Handle the synchronous save path in DomainEventInterceptor
by overriding SavingChanges alongside SavingChangesAsync. Ensure it either
drains domain events and outbox entries synchronously or explicitly throws to
require SaveChangesAsync, so synchronous SaveChanges cannot bypass processing.

In `@src/Loom.Persistence.EntityFrameworkCore/LoomPersistenceOptions.cs`:
- Around line 28-45: Update UseOutbox<TContext> so OutboxOptions and
OutboxEventSerializer are registered with a TContext-specific service key or
wrapper, ensuring each context retains its own batch/retry settings and
EventAssemblies. Preserve the existing per-context registrations for
OutboxProcessor<TContext>, OutboxDeliveryService<TContext>, and OutboxSink.

In `@tests/Loom.Entities.Tests/DomainEventTests.cs`:
- Line 5: Update the DomainEventTests test fixture declaration to be internal
sealed instead of public, keeping its test behavior unchanged and removing it
from the public surface.

In
`@tests/Loom.Persistence.EntityFrameworkCore.Tests/DomainEventDispatchTests.cs`:
- Around line 65-105: Update the save/transaction orchestration used by
TestDbContext.SaveChangesAsync so expected handler failures are returned as a
Result containing the DomainEventDispatchException details rather than thrown
through SaveChangesAsync. Preserve rollback of both the Order and AuditEntries,
while allowing only programming faults to propagate as exceptions. Ensure the
FailingHandler result, error metadata, and OrderCancelled event type remain
available to callers.

---

Nitpick comments:
In `@src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxDeliveryService.cs`:
- Around line 22-39: The generic exception handler in OutboxDeliveryService’s
delivery loop currently swallows pass-level failures; inject or reuse an ILogger
and log the caught exception with clear delivery-pass context before continuing
service execution.

In `@src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxEventSerializer.cs`:
- Line 12: Make the type-resolution cache in OutboxEventSerializer
instance-scoped rather than static, declaring it as _resolvedTypes so each
serializer uses its own EventAssemblies configuration. Update ResolveType and
all related cache accesses to use _resolvedTypes, preserving the existing
resolution and ambiguity behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 99617820-f981-4c9b-902d-b1331f27429e

📥 Commits

Reviewing files that changed from the base of the PR and between 1d52ad6 and b2f35be.

📒 Files selected for processing (40)

AGENTS.md
Directory.Packages.props
Loom.slnx
docs/ROADMAP.md
docs/agents/00-core.md
src/Loom.Entities/IDomainEvent.cs
src/Loom.Entities/IDomainEventHandler.cs
src/Loom.Entities/Loom.Entities.csproj
src/Loom.Persistence.EntityFrameworkCore/DomainEventDispatchException.cs
src/Loom.Persistence.EntityFrameworkCore/DomainEventDispatcher.cs
src/Loom.Persistence.EntityFrameworkCore/DomainEventInterceptor.cs
src/Loom.Persistence.EntityFrameworkCore/IDeferredDomainEventSink.cs
src/Loom.Persistence.EntityFrameworkCore/IDomainEventDispatcher.cs
src/Loom.Persistence.EntityFrameworkCore/IdValueConverter.cs
src/Loom.Persistence.EntityFrameworkCore/Loom.Persistence.EntityFrameworkCore.csproj
src/Loom.Persistence.EntityFrameworkCore/LoomPersistenceOptions.cs
src/Loom.Persistence.EntityFrameworkCore/ModelConfigurationBuilderExtensions.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxDeliveryService.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxEventSerializer.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxMessage.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxModelBuilderExtensions.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxOptions.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxProcessor.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/OutboxSink.cs
src/Loom.Persistence.EntityFrameworkCore/Outbox/UtcTicksConverter.cs
src/Loom.Persistence.EntityFrameworkCore/PagingQueryableExtensions.cs
src/Loom.Persistence.EntityFrameworkCore/PersistenceServiceCollectionExtensions.cs
src/Loom.Persistence.EntityFrameworkCore/SpecificationQueryableExtensions.cs
tests/Loom.Entities.Tests/DomainEventTests.cs
tests/Loom.Entities.Tests/TestEntities.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/AsyncPagingTests.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/DomainEventDispatchTests.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/IdentityMappingTests.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/Loom.Persistence.EntityFrameworkCore.Tests.csproj
tests/Loom.Persistence.EntityFrameworkCore.Tests/OutboxHost.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/OutboxTests.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/SpecificationQueryTests.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/SqliteFixture.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/TestHost.cs
tests/Loom.Persistence.EntityFrameworkCore.Tests/TestModel.cs

… settings Addresses review feedback on three real defects. Saving synchronously bypassed domain event dispatch entirely, because only the asynchronous interception point was overridden. The synchronous one now refuses the save when events are pending. It refuses rather than draining because handlers return a task, so honouring them here would mean blocking on one, which risks deadlock depending on the caller's context; and it refuses rather than proceeding because letting the save through would discard the events in silence. A save with nothing pending is still allowed, so only saves that would actually drop something are rejected. Outbox configuration was registered without reference to the context it belonged to, so configuring a second outbox meant the last call won and the first context ran with settings nobody chose for it. Settings, and the serializer that depends on them, are now tied to their context. The sink stays shared, since it writes to whichever context is saving, but is registered once so a second outbox does not produce a duplicate the interceptor would have to choose between. The type-resolution cache was static while the assemblies it resolves against are per-outbox, so one context's configuration could answer another's lookups. It is now per instance. The write side needs no configuration at all — an event knows its own type — so serializing moved to a non-generic helper, which also keeps the sink free of a context type it has no business naming. The delivery loop swallowed pass-level failures without a trace. Individual message failures are recorded against the message, but a pass failing as a whole left no record anywhere, so it is now logged with the context it failed for. Also corrects a corrupted line in the shared template: a table row had been merged into the marker comment on line one, which stopped the assembly script recognising the marker, so every assembled file began with a stray row and an internal comment. Versions are removed from the stack table in the same place, since that table sits directly above the rule saying versions live in the central manifest.

DutchyD and others added 6 commits July 30, 2026 06:00

Merge branch 'development'

f4fa54b

Add the foundational Loom packages

coderabbitai Bot reviewed Jul 30, 2026

View reviewed changes

DutchyD force-pushed the feature/persistence-efcore branch from cd7c897 to b2f35be Compare July 30, 2026 05:22

DutchyD merged commit ab97211 into development Jul 30, 2026
2 checks passed

DutchyD deleted the feature/persistence-efcore branch July 30, 2026 05:48

Navigation Menu

Uh oh!

Add the Entity Framework Core persistence package - #2

Add the Entity Framework Core persistence package#2
DutchyD merged 7 commits into
developmentfrom
feature/persistence-efcore

DutchyD commented Jul 30, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading

Review skipped

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

DutchyD commented Jul 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it does

Decisions worth reviewing

Two things found by building it, not by reasoning about it

A correction to existing guidance

Testing

Known gaps

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

DutchyD commented Jul 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading