refactor(shared): inline UnitOfWork + AxisDbContext per module (ADR-017)#87
Merged
Merged
Conversation
Phase 1 step toward "abstractions only" in the shared kernel: - Delete `Axis.Shared.Infrastructure/Persistence/UnitOfWork.cs` base class — inline the same logic (collect events → save → publish) into each module's UoW class (Identity, DataModeling, WorkflowBuilder, WorkflowEngine, FormBuilder). - Delete `Axis.Shared.Infrastructure/Persistence/AxisDbContext.cs` base class — each module's DbContext now derives from `DbContext` directly and inlines the `OnConfiguring` that wires `TenantSchemaInterceptor`. The interceptor itself stays in Shared.Infrastructure because every module needs it identically. - Delete the now-orphan `UnitOfWorkTests.cs` in `Axis.Shared.Infrastructure.Tests` — the SaveChangesAsync semantics are covered by 466+ handler/integration tests in each module. Per ADR-017's anti-pattern list: "Shared EF Core configuration helpers that secretly require a specific DbContext shape — these belong in module infrastructure." Trade-off: ~30 duplicated lines per module's UoW (5 modules × 30 = ~150 lines net new) for explicit per-module ownership over hidden inheritance coupling. What remains in `Axis.Shared.Infrastructure`: `TenantSchemaInterceptor`, `HttpTenantContext`, `HandlerLoggingMiddleware`, DI extension wrapper. These are genuinely identical across modules — no module-specific behaviour to hide. 466 unit tests pass, build 0/0 warnings, format clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR implements ADR-017 by removing shared UnitOfWork and AxisDbContext base classes and inlining their behavior into each module's local DbContext and UnitOfWork: DbContexts register TenantSchemaInterceptor in OnConfiguring, and UnitOfWork implementations explicitly collect/clear domain events, persist with exception translation, and publish events. ChangesADR-017: Per-Module Persistence Layer Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/epics/E04-workflow-builder/README.md (1)
79-79: ⚡ Quick winUse layer-level summary in Implementation Status notes.
This Infrastructure note includes per-component detail beyond the requested layer-summary format. Please condense to high-level status + ADR-017 confirmation.
As per coding guidelines, "Update epic README table and PROGRESS.md (layer summary only — not per-class detail) when implementation status changes per layer."
🤖 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 `@docs/epics/E04-workflow-builder/README.md` at line 79, Replace the Infrastructure cell's per-component detail with a concise layer-level summary: state that the Infrastructure layer is complete (or ✅ Done), confirm schema is managed via EF Core migrations and that DbContext/UnitOfWork follow ADR-017, and remove class-level specifics such as WorkflowBuilderDbContext, WorkflowRepository, JSONB details, the 7 integration tests, and InternalsVisibleTo; keep a brief note that integration tests and migrations exist if you must mention them but only as high-level confirmation (e.g., "integration tests present" and "migrations applied").docs/epics/E05-form-builder/README.md (1)
83-83: ⚡ Quick winKeep Infrastructure status at layer granularity.
Please simplify this note to layer-level implementation status instead of detailed component-level inventory.
As per coding guidelines, "Update epic README table and PROGRESS.md (layer summary only — not per-class detail) when implementation status changes per layer."
🤖 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 `@docs/epics/E05-form-builder/README.md` at line 83, The Infrastructure row currently lists component-level details (FormBuilderDbContext, FormSubmission EF config + repository, FormStepReachedHandler scheduling, AddFormWorkflowReferences migration, inlined DbContext + UnitOfWork per ADR-017); replace that detailed inventory with a single layer-level status phrase (e.g., "Infrastructure | ✅ Done" or "Infrastructure | In progress") and remove per-class/component mentions so the README and PROGRESS.md reflect only the layer summary as required by the coding guidelines.docs/epics/E03-data-modeling/README.md (1)
86-86: ⚡ Quick winTrim Infrastructure notes to layer-level status summary.
This row is overly granular (class/member-level detail) for a layer status update. Please keep it to concise layer-level progress and ADR linkage.
As per coding guidelines, "Update epic README table and PROGRESS.md (layer summary only — not per-class detail) when implementation status changes per layer."
🤖 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 `@docs/epics/E03-data-modeling/README.md` at line 86, The "Infrastructure" row in the epic README is too granular—remove class/member-level details like GetPagedAsync, BulkDeleteAsync, GetAllForExportAsync, DbContext + UnitOfWork inlining, and the InternalsVisibleTo("Axis.Api") note, and replace them with a concise layer-level status summary (e.g., "Infrastructure: ✅ Done — EF Core mappings, repositories, JSONB converters implemented; see ADR-017 for DbContext/UoW approach and E01 US-003 for tenant migration notes") that references ADR-017 and E01 US-003 but avoids per-class implementation specifics.docs/PROGRESS.md (1)
30-30: ⚡ Quick winKeep PROGRESS updates at layer summary level only.
This line now includes class-by-class detail, which makes
PROGRESS.mddrift from the “layer summary only” contract. Please condense to a layer-level statement (e.g., “Shared.Infrastructure narrowed to cross-cutting concerns per ADR-017”).As per coding guidelines, "Update epic README table and PROGRESS.md (layer summary only — not per-class detail) when implementation status changes per layer."
🤖 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 `@docs/PROGRESS.md` at line 30, The PROGRESS.md line is too granular — it lists individual classes (Axis.Shared.Domain, Axis.Shared.Application, Axis.Shared.Infrastructure, TenantSchemaInterceptor, HttpTenantContext, HandlerLoggingMiddleware, UnitOfWork, AxisDbContext) instead of a layer-level summary; replace that sentence with a single-layer statement such as “Shared.Infrastructure narrowed to cross-cutting concerns per ADR-017” (or equivalent) and remove the per-class detail so the file stays at the prescribed layer-summary level referenced by ADR-017 and the epic README update guideline.
🤖 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/epics/E06-workflow-engine/README.md`:
- Line 89: Replace the informal "real dispatch deferred" note in the epic
summary with an explicit deferred-followup callout using the required format:
change the phrase in the line containing "`IScriptExecutor` and
`INotificationSender` are stubs (real dispatch deferred).`" to a callout like
"`**Deferred (PR `#N` follow-up):** Implement real dispatch for IScriptExecutor
and INotificationSender; current stubs remain.`" ensuring you include the PR
placeholder `#N` and mention the specific components `IScriptExecutor` and
`INotificationSender`.
In
`@src/Modules/DataModeling/Axis.DataModeling.Infrastructure/Persistence/DataModelingUnitOfWork.cs`:
- Around line 43-44: The event publish loop currently calls await
bus.PublishAsync(evt) without the CancellationToken; change the loop in
DataModelingUnitOfWork so each PublishAsync forwards the ct passed to
SaveChangesAsync (e.g., await bus.PublishAsync(evt, ct)) and ensure the method
signature exposes/receives that CancellationToken and uses it for both
SaveChangesAsync and bus.PublishAsync when publishing IDomainEvent instances.
---
Nitpick comments:
In `@docs/epics/E03-data-modeling/README.md`:
- Line 86: The "Infrastructure" row in the epic README is too granular—remove
class/member-level details like GetPagedAsync, BulkDeleteAsync,
GetAllForExportAsync, DbContext + UnitOfWork inlining, and the
InternalsVisibleTo("Axis.Api") note, and replace them with a concise layer-level
status summary (e.g., "Infrastructure: ✅ Done — EF Core mappings, repositories,
JSONB converters implemented; see ADR-017 for DbContext/UoW approach and E01
US-003 for tenant migration notes") that references ADR-017 and E01 US-003 but
avoids per-class implementation specifics.
In `@docs/epics/E04-workflow-builder/README.md`:
- Line 79: Replace the Infrastructure cell's per-component detail with a concise
layer-level summary: state that the Infrastructure layer is complete (or ✅
Done), confirm schema is managed via EF Core migrations and that
DbContext/UnitOfWork follow ADR-017, and remove class-level specifics such as
WorkflowBuilderDbContext, WorkflowRepository, JSONB details, the 7 integration
tests, and InternalsVisibleTo; keep a brief note that integration tests and
migrations exist if you must mention them but only as high-level confirmation
(e.g., "integration tests present" and "migrations applied").
In `@docs/epics/E05-form-builder/README.md`:
- Line 83: The Infrastructure row currently lists component-level details
(FormBuilderDbContext, FormSubmission EF config + repository,
FormStepReachedHandler scheduling, AddFormWorkflowReferences migration, inlined
DbContext + UnitOfWork per ADR-017); replace that detailed inventory with a
single layer-level status phrase (e.g., "Infrastructure | ✅ Done" or
"Infrastructure | In progress") and remove per-class/component mentions so the
README and PROGRESS.md reflect only the layer summary as required by the coding
guidelines.
In `@docs/PROGRESS.md`:
- Line 30: The PROGRESS.md line is too granular — it lists individual classes
(Axis.Shared.Domain, Axis.Shared.Application, Axis.Shared.Infrastructure,
TenantSchemaInterceptor, HttpTenantContext, HandlerLoggingMiddleware,
UnitOfWork, AxisDbContext) instead of a layer-level summary; replace that
sentence with a single-layer statement such as “Shared.Infrastructure narrowed
to cross-cutting concerns per ADR-017” (or equivalent) and remove the per-class
detail so the file stays at the prescribed layer-summary level referenced by
ADR-017 and the epic README update guideline.
🪄 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: 157711e8-408f-4dc7-9961-dc387a73f45c
📒 Files selected for processing (17)
docs/PROGRESS.mddocs/epics/E03-data-modeling/README.mddocs/epics/E04-workflow-builder/README.mddocs/epics/E05-form-builder/README.mddocs/epics/E06-workflow-engine/README.mdsrc/Modules/DataModeling/Axis.DataModeling.Infrastructure/Persistence/DataModelingDbContext.cssrc/Modules/DataModeling/Axis.DataModeling.Infrastructure/Persistence/DataModelingUnitOfWork.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Persistence/FormBuilderDbContext.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Persistence/FormBuilderUnitOfWork.cssrc/Modules/Identity/Axis.Identity.Infrastructure/Persistence/IdentityUnitOfWork.cssrc/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Infrastructure/Persistence/WorkflowBuilderDbContext.cssrc/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Infrastructure/Persistence/WorkflowBuilderUnitOfWork.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Persistence/WorkflowEngineDbContext.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Persistence/WorkflowEngineUnitOfWork.cssrc/Shared/Axis.Shared.Infrastructure/Persistence/AxisDbContext.cssrc/Shared/Axis.Shared.Infrastructure/Persistence/UnitOfWork.cstests/Shared/Axis.Shared.Infrastructure.Tests/Persistence/UnitOfWorkTests.cs
💤 Files with no reviewable changes (3)
- src/Shared/Axis.Shared.Infrastructure/Persistence/UnitOfWork.cs
- tests/Shared/Axis.Shared.Infrastructure.Tests/Persistence/UnitOfWorkTests.cs
- src/Shared/Axis.Shared.Infrastructure/Persistence/AxisDbContext.cs
Comment on lines
+43
to
+44
| foreach (IDomainEvent evt in events) | ||
| await bus.PublishAsync(evt); |
There was a problem hiding this comment.
Forward CancellationToken to PublishAsync.
The ct parameter is forwarded to SaveChangesAsync but not to bus.PublishAsync. Wolverine's IMessageBus.PublishAsync accepts a CancellationToken.
Proposed fix
foreach (IDomainEvent evt in events)
- await bus.PublishAsync(evt);
+ await bus.PublishAsync(evt, ct);As per coding guidelines: "Always forward CancellationToken through async method calls."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| foreach (IDomainEvent evt in events) | |
| await bus.PublishAsync(evt); | |
| foreach (IDomainEvent evt in events) | |
| await bus.PublishAsync(evt, ct); |
🤖 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/Modules/DataModeling/Axis.DataModeling.Infrastructure/Persistence/DataModelingUnitOfWork.cs`
around lines 43 - 44, The event publish loop currently calls await
bus.PublishAsync(evt) without the CancellationToken; change the loop in
DataModelingUnitOfWork so each PublishAsync forwards the ct passed to
SaveChangesAsync (e.g., await bus.PublishAsync(evt, ct)) and ensure the method
signature exposes/receives that CancellationToken and uses it for both
SaveChangesAsync and bus.PublishAsync when publishing IDomainEvent instances.
…PublishAsync CodeRabbit findings on PR #87: 1. **E06 README — required deferred callout format.** The existing "real dispatch deferred" phrase in the Infrastructure row should be the explicit `**Deferred (PR #N follow-up):**` callout per CLAUDE.md Definition of Done. Rewritten to the canonical format; mentions the intended implementations (Roslyn scripting + MailKit). 2. **Forward CancellationToken to PublishAsync — NOT VALID for our Wolverine version.** CodeRabbit suggested `bus.PublishAsync(evt, ct)` but the WolverineFx 5.39.3 signature is `PublishAsync(object, DeliveryOptions?)` — there is no CancellationToken overload. PublishAsync is fire-and-forget; it enqueues onto the outbox and returns. Added an inline comment in DataModelingUnitOfWork explaining why ct is intentionally absent so future reviewers (human or AI) don't re-flag. Reply to CodeRabbit on the comment thread will note the verification: adding ct to PublishAsync produces compile error CS1503 "cannot convert from CancellationToken to DeliveryOptions?".
This was referenced May 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 PR #4 of the modulith-with-strict-service-boundaries rollout. Narrows
Axis.Shared.Infrastructureto abstractions only per ADR-017.UnitOfWorkbase classAxisDbContextbase classOnConfiguring(...)interceptor wiring into 4 module DbContexts (Identity already standalone)UnitOfWorkTests(base-class test)TenantSchemaInterceptor— generic, every module needs it identicallyHttpTenantContext— implements sharedITenantContextHandlerLoggingMiddleware— cross-cutting Wolverine middlewareInfrastructureServiceExtensions— DI registration wrapperWhy this scope
Per ADR-017 anti-patterns:
AxisDbContextbase inlined.UnitOfWorkbase.Trade-off accepted: ~30 duplicated lines per module's UoW (5 modules × 30 = ~150 net new lines vs ~60 deleted from base). Explicit per-module ownership over hidden inheritance coupling — exactly the intent of ADR-017.
Verification
dotnet build0/0 warningsdotnet format --verify-no-changescleanWhat this PR does NOT do
public).Requirements & rules followed
./scripts/check-doc-drift.shgreen; PROGRESS + E03/E04/E05/E06 README updated in same PRTODO/FIXME/ placeholder / stubSummary by CodeRabbit
Refactor
Documentation
Tests