Refactor registration command CQRS flow#282
Open
dmytrocraft wants to merge 61 commits into
Open
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors register-user into CQRS: commands become immutable input-only; command response DTO removed; EmailNormalizer and FindUserByEmail query handler added; RegisterUserOrchestrator coordinates pre-check/dispatch/post-check; processor/resolver delegate to orchestrator; repositories, cache, validator, and tests updated. ChangesRegister User CQRS Refactor
Estimated code review effort Possibly related PRs
Suggested labels Suggested reviewers
Sequence Diagram(s) sequenceDiagram
participant API as API (Processor/Resolver)
participant Orch as RegisterUserOrchestrator
participant QH as FindUserByEmailQueryHandler
participant Repo as UserRepository / MongoDB
participant Bus as CommandBus
participant CH as RegisterUserCommandHandler
API->>Orch: register(email, initials, password)
Orch->>QH: find(email)
QH->>Repo: findByEmail(normalized)
Repo-->>QH: ?User
alt user exists
QH-->>Orch: User -> Orch throws DuplicateEmailException
Orch-->>API: Duplicate error
else not found
Orch->>Bus: dispatch(RegisterUserCommand)
Bus->>CH: invoke(command)
CH->>Repo: save(user)
CH->>Bus: publish(UserRegisteredEvent)
CH-->>Bus: void
Bus-->>Orch: dispatch complete
Orch->>QH: find(email) -- post-dispatch reload
QH->>Repo: findByEmail(normalized)
Repo-->>QH: User
QH-->>Orch: User
Orch-->>API: User
end
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
3 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 24 files
Confidence score: 3/5
- There is a concrete validation-order risk in
src/User/Application/Resolver/RegisterUserMutationResolver.php: readingemailfrom$argsbefore validation can trigger failures on malformed input before the validator runs. - Because this issue is severity 6/10 with high confidence (9/10) and affects request handling behavior, it introduces moderate user-facing regression risk until fixed.
- This should be straightforward to address by validating the input payload first and only then accessing required keys like
email. - Pay close attention to
src/User/Application/Resolver/RegisterUserMutationResolver.php- ensure validation happens before any required argument access to avoid premature runtime errors.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/User/Application/Resolver/RegisterUserMutationResolver.php">
<violation number="1" location="src/User/Application/Resolver/RegisterUserMutationResolver.php:36">
P2: `email` is read from `$args` before input validation, so malformed input can fail before the validator handles it. Validate first, then access required keys.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as REST/GraphQL Client
participant API as API Platform
participant Processor as RegisterUserProcessor
participant Resolver as RegisterUserMutationResolver
participant QueryHandler as FindUserByEmailQueryHandler
participant CmdBus as CommandBus
participant CmdHandler as RegisterUserCommandHandler
participant Repo as UserRepository
participant EventBus as EventBus
Note over Client,EventBus: User Registration Flow (CQRS Refactored)
alt Existing User (email already registered)
Client->>API: POST /users (REST) or mutation (GraphQL)
alt REST path
API->>Processor: process(data)
Processor->>QueryHandler: find(email)
QueryHandler->>Repo: findByEmail(email)
Repo-->>QueryHandler: Existing User
QueryHandler-->>Processor: Existing User
Processor-->>API: Return existing User
else GraphQL path
API->>Resolver: __invoke(null, context)
Resolver->>QueryHandler: find(email)
QueryHandler->>Repo: findByEmail(email)
Repo-->>QueryHandler: Existing User
QueryHandler-->>Resolvers: Existing User
Resolver-->>API: Return existing User
end
API-->>Client: 200 OK + existing User
else New User (email not found)
Client->>API: POST /users (REST) or mutation (GraphQL)
alt REST path
API->>Processor: process(data)
Processor->>QueryHandler: find(email)
QueryHandler->>Repo: findByEmail(email)
Repo-->>QueryHandler: null
QueryHandler-->>Processor: null
Processor->>CmdBus: dispatch(RegisterUserCommand)
CmdBus->>CmdHandler: __invoke(command)
CmdHandler->>Repo: save(user)
CmdHandler->>EventBus: publish(UserRegisteredEvent)
Processor->>QueryHandler: find(email)
QueryHandler->>Repo: findByEmail(email)
Repo-->>QueryHandler: New User
alt User found
QueryHandler-->>Processor: New User
Processor-->>API: Return new User
else User not found
QueryHandler-->>Processor: null
Processor-->>API: Throw UserNotFoundException
end
else GraphQL path
API->>Resolver: __invoke(null, context)
Resolver->>QueryHandler: find(email)
QueryHandler->>Repo: findByEmail(email)
Repo-->>QueryHandler: null
QueryHandler-->>Resolver: null
Resolver->>CmdBus: dispatch(RegisterUserCommand)
CmdBus->>CmdHandler: __invoke(command)
CmdHandler->>Repo: save(user)
CmdHandler->>EventBus: publish(UserRegisteredEvent)
Resolver->>QueryHandler: find(email)
QueryHandler->>Repo: findByEmail(email)
Repo-->>QueryHandler: New User
alt User found
QueryHandler-->>Resolver: New User
Resolver-->>API: Return new User
else User not found
QueryHandler-->>Resolver: null
Resolver-->>API: Throw UserNotFoundException
end
end
API-->>Client: 200 OK + new User
end
Note over CmdHandler: Write-side only - no response mutation, no existing-user check
Note over QueryHandler,Repo: Read-side query for pre-check and post-dispatch resolution
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
=============================================
Coverage 100.00% 100.00%
- Complexity 0 2924 +2924
=============================================
Files 551 562 +11
Lines 9541 9987 +446
=============================================
+ Hits 9541 9987 +446
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
specs/issue-217-register-user-cqrs/architecture.md (1)
47-57: 💤 Low valueConsider documenting post-dispatch query failure handling.
The orchestration flow correctly describes the pre-query, dispatch, and post-query pattern. However, step 4 (line 54) states "Query by email again and return the persisted user" without explicitly addressing what happens if the post-dispatch query returns null, which could occur if persistence failed or in race conditions.
While the AI summary indicates the implementation throws
UserNotFoundExceptionin this case, explicitly documenting this error path would make the architecture more complete and help future implementers understand the expected behavior.📝 Suggested addition
1. Query by email. 2. Return existing user when found. 3. Dispatch the command when missing. -4. Query by email again and return the persisted user. +4. Query by email again and return the persisted user (throw UserNotFoundException if not found).🤖 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 `@specs/issue-217-register-user-cqrs/architecture.md` around lines 47 - 57, Add explicit documentation for the post-dispatch-query failure case: in the orchestration description for RegisterUserProcessor and RegisterUserMutationResolver, state that after dispatching the register command a second query-by-email is performed and if it returns null the implementation should throw UserNotFoundException (or a clearly named domain error) to signal persistence/runtime failure; include this failure path and the expected exception in the architecture text so implementers know to handle/log and surface this error from the resolver.specs/issue-217-register-user-cqrs/research.md (1)
27-27: 💤 Low valueRemove unrelated repository observation.
The note about
.github/copilot-instructions.mdnot being present appears unrelated to the CQRS refactor research and adds no value to understanding the current registration flow or the planned changes.🧹 Suggested removal
- The repository already exposes `findByEmail(string $email): ?UserInterface`. -- `.github/copilot-instructions.md` is not present in this branch.🤖 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 `@specs/issue-217-register-user-cqrs/research.md` at line 27, Remove the unrelated repository observation mentioning ".github/copilot-instructions.md" from the research note: delete the line "- `.github/copilot-instructions.md` is not present in this branch." in specs/issue-217-register-user-cqrs/research.md so the document only contains content relevant to the CQRS refactor and registration flow.
🤖 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 `@specs/issue-217-register-user-cqrs/research.md`:
- Line 48: There is a naming mismatch: the test file
SignUpCommandHandlerTest.php uses "SignUp" while the codebase/PR uses
RegisterUserCommand and RegisterUserCommandHandler; decide whether these are
distinct commands or the same and then rename or update accordingly—if they are
the same command, rename SignUpCommandHandlerTest.php to
RegisterUserCommandHandlerTest (and update any class/test names inside from
SignUp* to RegisterUser*), or if they are distinct, update the PR/docs to
reference SignUpCommand/SignUpCommandHandler where appropriate and ensure the
test targets the correct class (SignUpCommandHandlerTest -> tests the
SignUpCommandHandler class). Ensure all references to SignUpCommand,
SignUpCommandHandler, and SignUpCommandHandlerTest are consistently changed to
RegisterUserCommand, RegisterUserCommandHandler, and
RegisterUserCommandHandlerTest (or vice versa) so imports, assertions, and test
bootstrapping match the chosen terminology.
---
Nitpick comments:
In `@specs/issue-217-register-user-cqrs/architecture.md`:
- Around line 47-57: Add explicit documentation for the post-dispatch-query
failure case: in the orchestration description for RegisterUserProcessor and
RegisterUserMutationResolver, state that after dispatching the register command
a second query-by-email is performed and if it returns null the implementation
should throw UserNotFoundException (or a clearly named domain error) to signal
persistence/runtime failure; include this failure path and the expected
exception in the architecture text so implementers know to handle/log and
surface this error from the resolver.
In `@specs/issue-217-register-user-cqrs/research.md`:
- Line 27: Remove the unrelated repository observation mentioning
".github/copilot-instructions.md" from the research note: delete the line "-
`.github/copilot-instructions.md` is not present in this branch." in
specs/issue-217-register-user-cqrs/research.md so the document only contains
content relevant to the CQRS refactor and registration flow.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8c0b6ed-0b8a-49d5-9c63-42065991a3be
📒 Files selected for processing (24)
config/packages/test/cache.yamldocs/design-and-architecture.mdspecs/issue-217-register-user-cqrs/architecture.mdspecs/issue-217-register-user-cqrs/epics.mdspecs/issue-217-register-user-cqrs/implementation-readiness.mdspecs/issue-217-register-user-cqrs/prd.mdspecs/issue-217-register-user-cqrs/product-brief-distillate.mdspecs/issue-217-register-user-cqrs/product-brief.mdspecs/issue-217-register-user-cqrs/research.mdspecs/issue-217-register-user-cqrs/run-summary.mdsrc/User/Application/Command/RegisterUserCommand.phpsrc/User/Application/CommandHandler/RegisterUserCommandHandler.phpsrc/User/Application/DTO/RegisterUserCommandResponse.phpsrc/User/Application/Processor/RegisterUserProcessor.phpsrc/User/Application/Query/FindUserByEmailQueryHandler.phpsrc/User/Application/Query/FindUserByEmailQueryHandlerInterface.phpsrc/User/Application/Resolver/RegisterUserMutationResolver.phptests/Unit/User/Application/Command/RegisterUserBatchCommandResponseTest.phptests/Unit/User/Application/Command/SignUpCommandResponseTest.phptests/Unit/User/Application/Command/SignUpCommandTest.phptests/Unit/User/Application/CommandHandler/SignUpCommandHandlerTest.phptests/Unit/User/Application/Processor/RegisterUserProcessorTest.phptests/Unit/User/Application/Query/FindUserByEmailQueryHandlerTest.phptests/Unit/User/Application/Resolver/RegisterUserMutationResolverTest.php
💤 Files with no reviewable changes (4)
- src/User/Application/CommandHandler/RegisterUserCommandHandler.php
- tests/Unit/User/Application/Command/SignUpCommandResponseTest.php
- tests/Unit/User/Application/Command/SignUpCommandTest.php
- src/User/Application/DTO/RegisterUserCommandResponse.php
dmytrocraft
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
4229d17
Contributor
Author
|
Ready for human review. Current state: all CI checks are green, the branch is mergeable, and GitHub review threads show 0 unresolved. Human approval is the remaining requested step. |
…ter-user-cqrs # Conflicts: # config/packages/test/cache.yaml
dmytrocraft
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
042502f
Contributor
Code Review: PASS
Review Output |
Contributor
Code Review: PASS
Review Output |
Contributor
Code Review: PASS
Review Output |
Contributor
Code Review: PASS
Review Output |
Contributor
|
STATUS: PASS Code Review passed for commit Verification evidence:
|
Contributor
BMAD FR/NFR Review Gate: FAIL
Review Output |
Contributor
BMAD FR/NFR Review Gate: FAIL
Review OutputVerification Output |
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
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
3 participants
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.
Description
Refactors user registration so
RegisterUserCommandis a write-side request only. Existing-user lookups and created-user response resolution now live in the REST processor and GraphQL resolver through a dedicatedFindUserByEmailQueryHandler, whileRegisterUserCommandHandleronly creates users and publishes registration events.Also removes
RegisterUserCommandResponse, updates affected unit tests, documents the CQRS command/response boundary, and adds issue #217 planning artifacts underspecs/issue-217-register-user-cqrs/.Related Issue
Closes #217
Motivation and Context
The registration command previously behaved as both a command and response container, which blurred CQRS responsibilities. This keeps registration API behavior stable while separating write-side command handling from read-side response lookup.
How Has This Been Tested?
Local full CI passed with custom ports to avoid workspace conflicts:
make ci./vendor/bin/phpunit tests/Unit/User/Application/Resolver/RegisterUserMutationResolverTest.php tests/Unit/User/Application/Processor/RegisterUserProcessorTest.php tests/Unit/User/Application/Query/FindUserByEmailQueryHandlerTest.php tests/Unit/User/Application/Command/SignUpCommandTest.php tests/Unit/User/Application/CommandHandler/SignUpCommandHandlerTest.phpFull local CI included unit, integration, Behat, Psalm, Psalm taint/security, Deptrac, PHP Insights, OpenAPI diff/validation, Schemathesis, and Infection. Infection result: 4559/4559 mutants killed, MSI 100%.
Performance changes: none expected. This change adds one explicit post-dispatch read on the new-user registration path to resolve the created user outside the command handler; existing-user registration short-circuits before dispatch as before.
Screenshots (if appropriate):
N/A
Types of changes
Checklist:
Summary by cubic
Refactors registration CQRS so the handler creates users and returns
RegisterUserCommandResponse.userwhile keeping REST/GraphQL behavior unchanged. Enforces case-insensitive email uniqueness via storednormalizedEmailwith a partial unique index; adds an operational backfill runbook and BMAD manual evidence; documents 409 conflicts for ambiguous OAuth linking; addresses #217.Refactors
RegisterUserCommandimmutable; addedRegisterUserCommandDispatcher,FindUserByEmailQueryHandler,EmailNormalizer, andUserPasswordHashFactory.normalizedEmail; repositories/cache normalize emails; added backfill command with dry-run/report and report writer; included rollout runbook and BMAD manual evidence.SocialIdentityLinker; OpenAPI documents the 409 problem response.Bug Fixes
MONGODB_IMAGEoverride.Written for commit 951333c. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Documentation
Tests
BMAD Planning Evidence
specs/issue-217-register-user-cqrs/