refactor: ROP sweep across Domain + Application (10 sites)#32
Merged
Conversation
…idator<Inputs>
Apply the input-validator-then-Map pattern established by MenuReview to the
remaining two aggregates that still used imperative pre-checks + post-construction
aggregate validators. Validation now runs over an immutable Inputs record BEFORE
the aggregate is constructed - no half-built aggregate ever exists on the failure
path, multiple violations aggregate into one Error.InvalidInput, and the rules
that fire are all visible in one place.
Dinner.TryCreate
- Folds three "if (... == default)" + "if (end <= start)" pre-checks plus the
six s_validator rules into a single InlineValidator<CreateInputs>.
- The DateTime range rule is gated by .When(both non-default) so a single
invalid input never produces two errors (matches existing test expectations
of ContainSingle on each scenario).
- .WithErrorCode preserves "dinner.invalid.start-required" /
"dinner.invalid.end-required" / "dinner.invalid.schedule" so the
DinnerTests.ReasonCode assertions still pass.
- Drops the never-failing Id.NotEmpty (always NewUniqueV7) and
Status.NotEmpty (always set to Upcoming in ctor).
Reservation.TryCreate
- Removes the bug-shaped duplication where "if (guestCount <= 0)" pre-check
fired the same effect as the validator's GreaterThan(0) rule through two
different error paths. Now a single rule fires once.
- Consolidates into InlineValidator<CreateInputs> matching Dinner.
- .WithErrorCode preserves "reservation.invalid.guest-count" so
ReservationTests.ReasonCode assertions still pass.
All 45 Domain tests pass.
…().Ensure() chains
Sweep of all remaining handlers/queries/loaders that used the imperative
"if (x is null) return Result.Fail(...); if (other check) return Result.Fail(...)"
pattern. Each converted to the .ToResult(notFoundError).Ensure(predicate, error)
chain pioneered by UpdateMenuReviewCommandHandler.LoadReviewOwnedByAsync.
Refactored sites:
- HostResourceLoader.GetByIdAsync : trivial .ToResult()
- GetMenuQueryHandler.Handle : .ToResult().Ensure(host-match)
- GetDinnerQueryHandler.Handle : .ToResult().Ensure(host-match)
- UpdateMenuCommandHandler.LoadMenuAsync : .ToResult().Ensure(host-match)
- ScheduleDinnerCommandHandler.Handle : LoadMenuOwnedByHostAsync helper
+ .BindAsync(Dinner.TryCreate)
+ .TapAsync(_dinnerRepository.Add)
- DinnerTransitionPipeline.ApplyAsync : LoadOwnedDinnerAsync helper
+ .BindAsync(transition)
+ .TapAsync(repo.Update)
- ListReservationsForDinnerQueryHandler.Handle : LoadOwnedDinnerAsync
+ .BindAsync(BuildPageAsync);
cursor decode extracted into
a static helper returning
Result<Guid?>
- SubmitMenuReviewCommandHandler.EnsureCallerReservedAsync :
.ToResult(notFound).Ensure(status).Map(_=>dinner)
replaces the "if null OR wrong-status
-> return Fail" guard.
Behavior is preserved end-to-end:
- Domain 45/45, Application 1/1, Api 74/74
- Full .http replay across all 30 files: 37/37 requests behave as expected
(Recipe 22 fail-loud 404s, leak-shielded NotFound + Detail, state-machine
422s, RFC-7807 problem details, ETag preconditions, idempotency middleware
all still fire correctly)
No public-API changes, no error-code changes, no test changes - this is a
pure stylistic refactor that brings the codebase to 100% ROP coverage at
load-check-act seams. Controllers retain their imperative ResolveCallerGuestId
+ Unauthorized pattern - that's the ROP-to-ActionResult boundary and is
intentionally kept imperative.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors domain creation and application handlers to consistently use a Railway-Oriented Programming (ROP) style at load/validate/act seams, replacing imperative early-return checks with validator + Result chaining.
Changes:
- Refactored
Dinner.TryCreateandReservation.TryCreateto validate viaInlineValidator<CreateInputs>and construct aggregates via.Map(...). - Updated multiple application query/command handlers to use
.ToResult(...).Ensure(...)chains and helper loaders for ownership checks. - Extracted cursor decoding into a helper in
ListReservationsForDinnerQueryand reorganized handler flow aroundBindAsync(...).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Domain/src/Reservation/Entities/Reservation.cs | Moves creation-time validation into an input validator and maps to aggregate construction/events |
| Domain/src/Dinner/Entities/Dinner.cs | Replaces imperative schedule checks + aggregate validator with input validator + map construction |
| Application/src/Reservations/Queries/ListReservationsForDinnerQuery.cs | Adds ownership loader helper and extracts cursor decoding into a helper result |
| Application/src/Menus/Queries/GetMenuQueryHandler.cs | Converts to .ToResult(...).Ensure(...) ownership check chain |
| Application/src/Menus/Commands/UpdateMenuCommandHandler.cs | Converts menu load + ownership check to .ToResult(...).Ensure(...) |
| Application/src/MenuReviews/Commands/SubmitMenuReviewCommandHandler.cs | Converts reservation existence/status checks to .ToResult(...).Ensure(...) chain |
| Application/src/Hosts/Authorization/HostResourceLoader.cs | Simplifies GetByIdAsync via .ToResult(...) |
| Application/src/Dinners/Queries/GetDinnerQuery.cs | Converts to .ToResult(...).Ensure(...) ownership check chain |
| Application/src/Dinners/Commands/ScheduleDinnerCommandHandler.cs | Introduces menu ownership loader helper; binds into Dinner.TryCreate then persists |
| Application/src/Dinners/Commands/DinnerTransitionCommandHandlers.cs | Refactors transition pipeline into load/ensure/bind/tap update chain |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+28
to
+39
| TimeProvider clock) => | ||
| s_inputValidator.ValidateToResult(new CreateInputs(dinnerId, guestUserId, guestCount)) | ||
| .Map(inputs => | ||
| { | ||
| var reservation = new Reservation( | ||
| ReservationId.NewUniqueV7(), inputs.DinnerId, inputs.GuestUserId, | ||
| inputs.GuestCount, clock.GetUtcNow()); | ||
| reservation.DomainEvents.Add(new ReservationCreated( | ||
| reservation.Id, reservation.DinnerId, reservation.GuestUserId, | ||
| reservation.GuestCount, reservation.ReservedAt)); | ||
| return reservation; | ||
| }); |
Comment on lines
+43
to
+50
| static readonly InlineValidator<CreateInputs> s_inputValidator = new() | ||
| { | ||
| v => v.RuleFor(x => x.DinnerId).NotEmpty(), | ||
| v => v.RuleFor(x => x.GuestUserId).NotEmpty(), | ||
| v => v.RuleFor(x => x.GuestCount).GreaterThan(0) | ||
| .WithErrorCode("reservation.invalid.guest-count") | ||
| .WithMessage("GuestCount must be positive."), | ||
| }; |
Comment on lines
+59
to
+72
| TimeProvider clock) => | ||
| s_inputValidator.ValidateToResult(new CreateInputs( | ||
| name, description, hostId, menuId, startDateTime, endDateTime)) | ||
| .Map(inputs => | ||
| { | ||
| var dinner = new Dinner( | ||
| DinnerId.NewUniqueV7(), | ||
| inputs.Name, inputs.Description, inputs.HostId, inputs.MenuId, | ||
| inputs.StartDateTime, inputs.EndDateTime); | ||
| dinner.DomainEvents.Add(new DinnerScheduled( | ||
| dinner.Id, dinner.HostId, dinner.MenuId, | ||
| dinner.StartDateTime, dinner.EndDateTime, clock.GetUtcNow())); | ||
| return dinner; | ||
| }); |
Comment on lines
+174
to
191
| static readonly InlineValidator<CreateInputs> s_inputValidator = new() | ||
| { | ||
| v => v.RuleFor(x => x.Id).NotEmpty(), | ||
| v => v.RuleFor(x => x.Name).NotEmpty(), | ||
| v => v.RuleFor(x => x.Description).NotEmpty(), | ||
| v => v.RuleFor(x => x.HostId).NotEmpty(), | ||
| v => v.RuleFor(x => x.MenuId).NotEmpty(), | ||
| v => v.RuleFor(x => x.Status).NotEmpty(), | ||
| v => v.RuleFor(x => x.StartDateTime).NotEqual(default(DateTimeOffset)) | ||
| .WithErrorCode("dinner.invalid.start-required") | ||
| .WithMessage("StartDateTime is required."), | ||
| v => v.RuleFor(x => x.EndDateTime).NotEqual(default(DateTimeOffset)) | ||
| .WithErrorCode("dinner.invalid.end-required") | ||
| .WithMessage("EndDateTime is required."), | ||
| v => v.RuleFor(x => x.EndDateTime) | ||
| .Must((inputs, end) => end > inputs.StartDateTime) | ||
| .WithErrorCode("dinner.invalid.schedule") | ||
| .WithMessage("EndDateTime must be strictly after StartDateTime.") | ||
| .When(x => x.StartDateTime != default && x.EndDateTime != default), | ||
| }; |
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
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.
ROP sweep across the entire codebase
Per audit request, this PR closes every remaining ROP gap at the load-check-act seams of the application and the pre-validation seams of the domain. Two commits, separated for cleaner review:
Commit 1 — Domain TryCreate refactors (
8dcdf02)Apply the
InlineValidator<Inputs>+.Map(...)pattern (established byMenuReview) to the remaining two aggregates that still used imperative pre-checks + post-construction aggregate validators.Dinner.TryCreate— folds 3if (... == default)pre-checks + 6 aggregate-shape rules into oneInlineValidator<CreateInputs>. The schedule rule is.When(both non-default)-gated so single bad inputs never double up..WithErrorCodepreserves all 3 reason codes the tests assert against.Reservation.TryCreate— removes a bug-shaped duplication whereif (guestCount <= 0)and the validator'sGreaterThan(0)rule both fired the same effect through two different error paths. Now one rule fires once.Commit 2 — Application ROP-ify (
8a883ee)Sweep of 8 sites that still used
if (x is null) return Result.Fail(...); if (other check) return Result.Fail(...). All converted to.ToResult(notFoundError).Ensure(predicate, error)chains followingUpdateMenuReviewCommandHandler.LoadReviewOwnedByAsync.HostResourceLoader.GetByIdAsync.ToResult()GetMenuQueryHandler.Handle.ToResult().Ensure(host)GetDinnerQueryHandler.Handle.ToResult().Ensure(host)UpdateMenuCommandHandler.LoadMenuAsync.ToResult().Ensure(host)ScheduleDinnerCommandHandler.Handle.BindAsync(TryCreate)+.TapAsync(Add)DinnerTransitionPipeline.ApplyAsync.BindAsync(transition)+.TapAsync(Update)ListReservationsForDinnerQueryHandler.Handle.BindAsync(BuildPage); cursor decode extracted into staticResult<Guid?>helperSubmitMenuReviewCommandHandler.EnsureCallerReservedAsync.ToResult().Ensure(status).Map(_ => dinner)Intentionally left imperative
if (ResolveCallerGuestId.IsFailure) return Unauthorized()— controllers are the ROP→ActionResult boundary; the current pattern is correct.Reservation.Cancel's reason guard — single non-duplicated rule; folding it into a validator would be over-engineering.Verification
.httpreplay across all 30 files: 37/37 requests behave as expectedNet delta:
-18lines across 10 files refactored.