fix[validation]: improve validation messages for user role apis (BUG-3835) #826
Conversation
WalkthroughReplaced generic validators in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Route (v1 user-role)
participant V as Validator (per-field rules)
participant S as Service
note over V: Explicit validators: patterns, isIn, numeric, common constants
C->>R: POST /user-role (body)
R->>V: validateCreate(fields)
alt valid
V-->>R: next()
R->>S: createUserRole()
S-->>R: result
R-->>C: 200 OK
else invalid
V-->>R: field-specific errors
R-->>C: 400 Bad Request
end
C->>R: PUT /user-role/:id (body)
R->>V: validateUpdate(id, fields)
alt valid
V-->>R: next()
R->>S: updateUserRole()
S-->>R: result
R-->>C: 200 OK
else invalid
V-->>R: field-specific errors
R-->>C: 400 Bad Request
end
C->>R: GET /user-role (query)
R->>V: validateList(query)
alt valid
V-->>R: next()
R->>S: listUserRoles()
S-->>R: results
R-->>C: 200 OK
else invalid
V-->>R: query-specific errors
R-->>C: 400 Bad Request
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![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 (7)
src/validators/v1/user-role.js (7)
17-23: Title: confirm whether digits are allowed; if yes, expand regexCurrent regex forbids digits. If titles like "role_1" are valid, allow digits.
Apply if digits should be allowed:
- .matches(/^[a-z_]+$/) - .withMessage('title must contain only lowercase letters (a–z) and underscores') + .matches(/^[a-z0-9_]+$/) + .withMessage('title must contain only lowercase letters (a–z), digits (0–9), and underscores')
24-30: user_type: consider normalizing to int and validating numericallyIf
user_typeis stored/handled as a number, normalize and validate as int; otherwise, keep strings as-is.- .isIn(['0', '1']) - .withMessage('user_type must be 0 (non-admin) or 1 (admin)') + .toInt() + .isIn([0, 1]) + .withMessage('user_type must be 0 (non-admin) or 1 (admin)')
38-44: Label: i18n and length guard (optional)Current regex excludes non‑ASCII letters and hyphens. If i18n labels are required, switch to Unicode properties. Also consider a max length to bound payload size.
Apply if i18n needed:
- .matches(/^[A-Z][a-zA-Z\s]*$/) - .withMessage('label must start with an uppercase letter and contain only letters and spaces') + .matches(/^[\p{Lu}][\p{L}\s]*$/u) + .withMessage('label must start with an uppercase letter and contain only letters and spaces (i18n supported)')Optionally add a max length:
+ .isLength({ max: 100 }) + .withMessage('label must be at most 100 characters')
45-50: status: trim before optional to treat whitespace‑only as “missing”Reorder to avoid rejecting values like " " when you intend to ignore them.
- req.checkBody('status') - .optional({ checkFalsy: true }) - .trim() + req.checkBody('status') + .trim() + .optional({ checkFalsy: true }) .isIn([common.ACTIVE_STATUS, common.INACTIVE_STATUS]) .withMessage(`status must be either ${common.ACTIVE_STATUS} or ${common.INACTIVE_STATUS} when provided`)
55-55: Strengthen id param validationValidate format, not just presence. Pick the appropriate constraint for your ID type.
Option A (UUID v4):
- req.checkParams('id').notEmpty().withMessage('id param is required') + req.checkParams('id').isUUID('4').withMessage('id param must be a valid UUID')Option B (numeric):
- req.checkParams('id').notEmpty().withMessage('id param is required') + req.checkParams('id').isInt().withMessage('id param must be a valid integer')
89-118: List validators: trim before optional + stronger numeric check for organization_id
- Place
trim()beforeoptional()so whitespace-only inputs are ignored.- Prefer
.toInt().isInt()fororganization_id.- Consider normalizing
user_typeas int for consistency with create/update.- req.checkQuery('title') - .optional() - .trim() + req.checkQuery('title') + .trim() + .optional() .matches(/^[a-z_]+$/) .withMessage('title is invalid. Use only lowercase letters a–z and underscores') - req.checkQuery('user_type') - .optional() - .trim() - .isIn(['0', '1']) - .withMessage('user_type is invalid. Allowed values: 0 (non-admin) or 1 (admin)') + req.checkQuery('user_type') + .trim() + .optional() + .toInt() + .isIn([0, 1]) + .withMessage('user_type is invalid. Allowed values: 0 (non-admin) or 1 (admin)') - req.checkQuery('visibility') - .optional() - .trim() + req.checkQuery('visibility') + .trim() + .optional() .isIn(['PUBLIC']) .withMessage('visibility is invalid. Allowed value: PUBLIC') - req.checkQuery('status') - .optional() - .trim() + req.checkQuery('status') + .trim() + .optional({ checkFalsy: true }) .isIn([common.ACTIVE_STATUS, common.INACTIVE_STATUS]) .withMessage(`status must be either ${common.ACTIVE_STATUS} or ${common.INACTIVE_STATUS} when provided`) - req.checkQuery('organization_id') - .optional() - .trim() - .matches(/^[0-9]+$/) - .withMessage('organization_id is invalid. Must be numeric') + req.checkQuery('organization_id') + .trim() + .optional() + .toInt() + .isInt() + .withMessage('organization_id is invalid. Must be an integer')
4-11: Remove unused validateList from src/validators/v1/user-role.jsDefined but not exported or referenced anywhere in the repository — remove it or export/use it if intended.
Location: src/validators/v1/user-role.js (lines 4–11)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validators/v1/user-role.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/user-role.js
🧬 Code graph analysis (1)
src/validators/v1/user-role.js (1)
src/constants/blacklistConfig.js (1)
userRole(332-335)
🔇 Additional comments (5)
src/validators/v1/user-role.js (5)
3-3: Confirm status constants exist and match API/domain valuesVerify
common.ACTIVE_STATUSandcommon.INACTIVE_STATUSare defined and exactly match the values persisted/used downstream.
31-37: Visibility restricted to PUBLIC only — confirm product intentIs
PUBLICthe only valid value for create/update? If other visibilities (e.g., PRIVATE/RESTRICTED) are supported now or in the near term, widenisIn.
57-63: Same as create/titleSee comment on Lines 17–23 regarding digits and length.
64-70: Same as create/user_typeSee comment on Lines 24–30 about normalizing to int vs strings.
71-77: Same as create/visibilitySee comment on Lines 31–37 (PUBLIC-only).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/validators/v1/user-role.js (2)
117-124: Align list() optional validators with sanitized valuesIn
list(), the query validators call.optional()before trimming and don’t usecheckFalsy, so a parameter likestatus=%20%20is treated as provided and failsisIn(). Trim first and enablecheckFalsyso blank query params are skipped; please apply the same pattern to the other optional query fields here.req.checkQuery('status') - .optional() - .trim() + .trim() + .optional({ checkFalsy: true }) .isIn([common.ACTIVE_STATUS, common.INACTIVE_STATUS])
4-11: Remove unused validateList helper
validateListis no longer invoked after inlining the per-field checks; keeping unused helpers invites drift. Please drop this function (and its call sites, if any remain elsewhere).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validators/v1/user-role.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/user-role.js
🧬 Code graph analysis (1)
src/validators/v1/user-role.js (1)
src/constants/blacklistConfig.js (1)
userRole(332-335)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validators/v1/user-role.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/user-role.js
🧬 Code graph analysis (1)
src/validators/v1/user-role.js (1)
src/constants/blacklistConfig.js (1)
userRole(332-335)
| req.checkQuery('title') | ||
| .optional() | ||
| .trim() | ||
| .matches(/^[a-z_]+$/) | ||
| .withMessage('title is invalid. Use only lowercase letters a–z and underscores') | ||
|
|
||
| req.checkQuery('user_type') | ||
| .optional() | ||
| .trim() | ||
| .isIn(['0', '1']) | ||
| .withMessage('user_type is invalid. Allowed values: 0 (non-admin) or 1 (admin)') | ||
|
|
||
| req.checkQuery('visibility') | ||
| .optional() | ||
| .trim() | ||
| .isIn(['PUBLIC']) | ||
| .withMessage('visibility is invalid. Allowed value: PUBLIC') | ||
|
|
||
| req.checkQuery('status') | ||
| .optional() | ||
| .trim() | ||
| .isIn([common.ACTIVE_STATUS, common.INACTIVE_STATUS]) | ||
| .withMessage(`status must be either ${common.ACTIVE_STATUS} or ${common.INACTIVE_STATUS} when provided`) | ||
|
|
||
| default: (req) => { | ||
| const allowedVariables = ['title', 'user_type', 'visibility', 'organization_id', 'status'] | ||
| validateList(req, allowedVariables) | ||
| req.checkQuery('organization_id') | ||
| .optional() | ||
| .trim() | ||
| .matches(/^[0-9]+$/) | ||
| .withMessage('organization_id is invalid. Must be numeric') |
There was a problem hiding this comment.
Trim before marking list query params optional
In list(), every query-chain calls optional() before trim(). When a client sends "status= " (or other fields with only whitespace), the value is still truthy when .optional() runs, so the chain keeps going and .isIn()/.matches() reject it. We just fixed this exact bug for status in create/update; the GET flow should behave the same way. Reorder the calls so you trim first and only then mark the field optional (using checkFalsy to drop empty strings). (github.com)
- req.checkQuery('title')
- .optional()
- .trim()
+ req.checkQuery('title')
+ .trim()
+ .optional({ checkFalsy: true })
.matches(/^[a-z_]+$/)
.withMessage('title is invalid. Use only lowercase letters a–z and underscores')
- req.checkQuery('user_type')
- .optional()
- .trim()
+ req.checkQuery('user_type')
+ .trim()
+ .optional({ checkFalsy: true })
.isIn(['0', '1'])
.withMessage('user_type is invalid. Allowed values: 0 (non-admin) or 1 (admin)')
- req.checkQuery('visibility')
- .optional()
- .trim()
+ req.checkQuery('visibility')
+ .trim()
+ .optional({ checkFalsy: true })
.isIn(['PUBLIC'])
.withMessage('visibility is invalid. Allowed value: PUBLIC')
- req.checkQuery('status')
- .optional()
- .trim()
+ req.checkQuery('status')
+ .trim()
+ .optional({ checkFalsy: true })
.isIn([common.ACTIVE_STATUS, common.INACTIVE_STATUS])
.withMessage(`status must be either ${common.ACTIVE_STATUS} or ${common.INACTIVE_STATUS} when provided`)
- req.checkQuery('organization_id')
- .optional()
- .trim()
+ req.checkQuery('organization_id')
+ .trim()
+ .optional({ checkFalsy: true })
.matches(/^[0-9]+$/)
.withMessage('organization_id is invalid. Must be numeric')📝 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.
| req.checkQuery('title') | |
| .optional() | |
| .trim() | |
| .matches(/^[a-z_]+$/) | |
| .withMessage('title is invalid. Use only lowercase letters a–z and underscores') | |
| req.checkQuery('user_type') | |
| .optional() | |
| .trim() | |
| .isIn(['0', '1']) | |
| .withMessage('user_type is invalid. Allowed values: 0 (non-admin) or 1 (admin)') | |
| req.checkQuery('visibility') | |
| .optional() | |
| .trim() | |
| .isIn(['PUBLIC']) | |
| .withMessage('visibility is invalid. Allowed value: PUBLIC') | |
| req.checkQuery('status') | |
| .optional() | |
| .trim() | |
| .isIn([common.ACTIVE_STATUS, common.INACTIVE_STATUS]) | |
| .withMessage(`status must be either ${common.ACTIVE_STATUS} or ${common.INACTIVE_STATUS} when provided`) | |
| default: (req) => { | |
| const allowedVariables = ['title', 'user_type', 'visibility', 'organization_id', 'status'] | |
| validateList(req, allowedVariables) | |
| req.checkQuery('organization_id') | |
| .optional() | |
| .trim() | |
| .matches(/^[0-9]+$/) | |
| .withMessage('organization_id is invalid. Must be numeric') | |
| req.checkQuery('title') | |
| .trim() | |
| .optional({ checkFalsy: true }) | |
| .matches(/^[a-z_]+$/) | |
| .withMessage('title is invalid. Use only lowercase letters a–z and underscores') | |
| req.checkQuery('user_type') | |
| .trim() | |
| .optional({ checkFalsy: true }) | |
| .isIn(['0', '1']) | |
| .withMessage('user_type is invalid. Allowed values: 0 (non-admin) or 1 (admin)') | |
| req.checkQuery('visibility') | |
| .trim() | |
| .optional({ checkFalsy: true }) | |
| .isIn(['PUBLIC']) | |
| .withMessage('visibility is invalid. Allowed value: PUBLIC') | |
| req.checkQuery('status') | |
| .trim() | |
| .optional({ checkFalsy: true }) | |
| .isIn([common.ACTIVE_STATUS, common.INACTIVE_STATUS]) | |
| .withMessage(`status must be either ${common.ACTIVE_STATUS} or ${common.INACTIVE_STATUS} when provided`) | |
| req.checkQuery('organization_id') | |
| .trim() | |
| .optional({ checkFalsy: true }) | |
| .matches(/^[0-9]+$/) | |
| .withMessage('organization_id is invalid. Must be numeric') |
🤖 Prompt for AI Agents
In src/validators/v1/user-role.js around lines 99 to 127, each query validation
chain currently calls .optional() before .trim(), causing whitespace-only values
to be treated as present and then rejected; reorder the calls to trim first and
then make the field optional using the falsy-check so empty strings are dropped
(e.g., call .trim() then .optional({ checkFalsy: true }) before the following
validators like .matches()/.isIn()), and apply this change to title, user_type,
visibility, status and organization_id chains.
Summary by CodeRabbit