fix(validator): treat falsy values as valid template params (#2008)#2072
fix(validator): treat falsy values as valid template params (#2008)#2072armorbreak001 wants to merge 1 commit intoasyncapi:masterfrom
Conversation
…#2008) Problem: treats , , as missing required parameters, causing false-positive validation errors. Fix: Use check instead of truthiness check. Before: required param with value → marked as missing ❌ After: required param with value → accepted ✅
armorbreak001
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
📝 WalkthroughWalkthroughModified the required parameter validation logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/generator/lib/templates/config/validator.js`:
- Line 78: Add Jest test cases in
apps/generator/test/templateConfigValidator.test.js that call
validateTemplateConfig (the function under test) with required parameters set to
falsy values to ensure they are accepted: add assertions that
validateTemplateConfig does not throw when a required param is provided as
false, 0, '', and null (in addition to the existing “missing” case), and keep
the existing configParams/templateParams setup so the missing-parameter
detection still only treats undefined as missing.
🪄 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: fb32b841-26e9-49dc-b962-70bb07ded726
📒 Files selected for processing (1)
apps/generator/lib/templates/config/validator.js
| function isRequiredParamProvided(configParams, templateParams) { | ||
| const missingParams = Object.keys(configParams || {}) | ||
| .filter(key => configParams[key].required && !templateParams[key]); | ||
| .filter(key => configParams[key].required && templateParams[key] === undefined); |
There was a problem hiding this comment.
Add test coverage for falsy required-parameter values.
The fix correctly narrows the missing-parameter check to === undefined, which aligns with the PR objective and permits false, 0, '', and null as valid inputs. However, per the existing test at apps/generator/test/templateConfigValidator.test.js:68-80, only the "completely missing" case is covered. Without new tests, a future regression that reintroduces a truthiness check (or switches to == null, which would re-reject null) could silently slip through.
Consider adding cases that assert validateTemplateConfig does not throw when a required param is provided as false, 0, '', and null.
Also worth confirming explicitly: accepting null as "provided" is intentional here (the PR description says "arguably null"); the AI summary lists null among accepted values, so the current === undefined semantics match that intent.
Want me to draft the additional Jest cases for templateConfigValidator.test.js?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/generator/lib/templates/config/validator.js` at line 78, Add Jest test
cases in apps/generator/test/templateConfigValidator.test.js that call
validateTemplateConfig (the function under test) with required parameters set to
falsy values to ensure they are accepted: add assertions that
validateTemplateConfig does not throw when a required param is provided as
false, 0, '', and null (in addition to the existing “missing” case), and keep
the existing configParams/templateParams setup so the missing-parameter
detection still only treats undefined as missing.
2 participants
Problem
Fixes #2008
In
validator.js, required template parameters are validated with:The
!templateParams[key]check uses truthiness, which incorrectly treats these valid values as missing:!valueundefinedtruenulltruefalsetrue0true''(empty string)trueFix
1 line change:
!templateParams[key]→templateParams[key] === undefinedOnly truly undefined values are treated as missing required params.
Impact
false,0, or''as template param valuesSummary by CodeRabbit
false,0, empty strings, andnull) as valid for required parameters, preventing unnecessary validation errors.