Conversation
|
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:
📝 WalkthroughWalkthroughAdded per-event validation of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Konrad-Simso
changed the title
![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
🤖 Fix all issues with AI agents
In `@src/Storage/Controllers/ProcessController.cs`:
- Around line 380-398: Rename the parameter SystemUserOwnerOrgNo in the
ValidateInstanceEventUserObject method to use camelCase (e.g.,
systemUserOwnerOrgNo) and update all internal references inside the method (the
null check) to the new name; then update all call sites that pass or reference
this parameter to use the new parameter name so signatures match (search for
ValidateInstanceEventUserObject usages and adjust callers and any related
delegates/overloads accordingly).
🧹 Nitpick comments (2)
src/Storage/Controllers/ProcessController.cs (2)
162-173: Error message is misleading for theSystemUserId/SystemUserOwnerOrgNocase.
ValidateInstanceEventUserObjectreturnsfalsefor two distinct reasons:
- All user identifiers are null (truly missing user object).
SystemUserIdis provided butSystemUserOwnerOrgNois missing.The error message on line 172 always says
"Missing user object", which is inaccurate for case 2. Consider either returning distinct messages or having the validation method provide a reason string.Suggested approach
- bool validUserObject = ValidateInstanceEventUserObject( - user?.UserId, - user?.OrgId, - user?.SystemUserId, - user?.SystemUserOwnerOrgNo, - user?.EndUserSystemId - ); - if (!validUserObject) - { - return BadRequest($"Missing user object in {nameof(instanceEvent.User)}"); - } + string? userValidationError = ValidateInstanceEventUserObject( + user?.UserId, + user?.OrgId, + user?.SystemUserId, + user?.SystemUserOwnerOrgNo, + user?.EndUserSystemId + ); + if (userValidationError is not null) + { + return BadRequest(userValidationError); + }And update the helper accordingly:
private static string? ValidateInstanceEventUserObject( int? userId, string? orgId, Guid? systemUserId, string? systemUserOwnerOrgNo, int? endUserSystemId ) { if (userId is null && orgId is null && systemUserId is null && endUserSystemId is null) { return "Missing user object in User"; } if (systemUserId is not null && systemUserOwnerOrgNo is null) { return "SystemUserOwnerOrgNo is required when SystemUserId is provided"; } return null; }
388-391: Consider whether empty/whitespace strings should also be rejected.
orgIdandSystemUserOwnerOrgNoare only checked fornull, but an empty or whitespace-only string would pass validation. Elsewhere in this method (line 175),IsNullOrWhiteSpaceis used forInstanceId. If blank strings are invalid for these fields, consider usingstring.IsNullOrWhiteSpacehere as well.Example
- if (userId is null && orgId is null && systemUserId is null && endUserSystemId is null) + if (userId is null && string.IsNullOrWhiteSpace(orgId) && systemUserId is null && endUserSystemId is null) { return false; } - if (systemUserId is not null && SystemUserOwnerOrgNo is null) + if (systemUserId is not null && string.IsNullOrWhiteSpace(systemUserOwnerOrgNo))
HauklandJ
left a comment
HauklandJ
left a comment
There was a problem hiding this comment.
At least a test for ValidateInstanceEventUserObject
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/UnitTest/TestingControllers/ProcessControllerTest.cs`:
- Around line 677-700: Add an InlineData row to exercise the branch where
systemUserId is non-null but systemUserOwnerOrgNo is null so the rule
(systemUserId is not null && systemUserOwnerOrgNo is null) is tested; update the
Theory data in ProcessControllerTest.ValidateInstanceEventUserObject to include
an entry like a GUID string for the systemUserId parameter (e.g.
"00000000-0000-0000-0000-000000000001") with systemUserOwnerOrgNo null and
expectedResult false so
ProcessController.ValidateInstanceEventUserObject(userId, orgId, systemUserId,
systemUserOwnerOrgNo, endUserSystemId) covers that branch.
🧹 Nitpick comments (1)
src/Storage/Controllers/ProcessController.cs (1)
380-403: Consider adding a reverse check:systemUserOwnerOrgNowithoutsystemUserId.The method enforces that
systemUserIdrequiressystemUserOwnerOrgNo, but the opposite is not checked. When another identifier is present (e.g.,userId), a request with an orphanedsystemUserOwnerOrgNo(nosystemUserId) passes validation silently. This likely indicates a malformed payload.Suggested addition
if (systemUserId is not null && string.IsNullOrWhiteSpace(systemUserOwnerOrgNo)) { return false; } + if (systemUserId is null && !string.IsNullOrWhiteSpace(systemUserOwnerOrgNo)) + { + return false; + } return true;
|
|
Have tried rerunning Code test and analysis pipeline, it meets a problem with the following |
2 participants
Description
Add validation for user object on instance events.
Verification
Summary by CodeRabbit
Bug Fixes
Tests