fix(Utils): coerce non-string values before split in validateRule - #8198
fix(Utils): coerce non-string values before split in validateRule#8198bardock-2393 wants to merge 1 commit into
Conversation
The semver and MODULO branches of validateRule guarded against a missing rule value but still called .split() directly on rule.value, so a truthy non-string value (e.g. a boolean) reached .split and threw. Coerce to a string first so validation degrades to false instead of crashing CreateSegment/EditSegment's isValid check.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-actions
Bot
added
the
front-end
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f878f4cf-e85a-4366-88a4-0078f2dc7264
📒 Files selected for processing (2)
frontend/common/utils/__tests__/utils.test.tsfrontend/common/utils/utils.tsx
| it('does not throw when value is a non-string type for MODULO', () => { | ||
| const rule = buildRule({ operator: 'MODULO', value: true }) | ||
| expect(() => Utils.validateRule(rule)).not.toThrow() | ||
| }) | ||
|
|
||
| it('does not throw when value is a non-string type for a semver operator', () => { | ||
| const rule = buildRule({ operator: 'GREATER_THAN:semver', value: true }) | ||
| expect(() => Utils.validateRule(rule)).not.toThrow() | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the validation result for non-string values.
These tests only verify that Utils.validateRule does not throw. They do not verify that the supplied true value is rejected. Add toBe(false) assertions for both the MODULO and semver cases. The SegmentCondition.value contract permits booleans, so the validation result is part of this regression contract.
Suggested assertions
expect(() => Utils.validateRule(rule)).not.toThrow()
+ expect(Utils.validateRule(rule)).toBe(false)Apply the same assertion to both tests.
📝 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.
| it('does not throw when value is a non-string type for MODULO', () => { | |
| const rule = buildRule({ operator: 'MODULO', value: true }) | |
| expect(() => Utils.validateRule(rule)).not.toThrow() | |
| }) | |
| it('does not throw when value is a non-string type for a semver operator', () => { | |
| const rule = buildRule({ operator: 'GREATER_THAN:semver', value: true }) | |
| expect(() => Utils.validateRule(rule)).not.toThrow() | |
| }) | |
| it('does not throw when value is a non-string type for MODULO', () => { | |
| const rule = buildRule({ operator: 'MODULO', value: true }) | |
| expect(() => Utils.validateRule(rule)).not.toThrow() | |
| expect(Utils.validateRule(rule)).toBe(false) | |
| }) | |
| it('does not throw when value is a non-string type for a semver operator', () => { | |
| const rule = buildRule({ operator: 'GREATER_THAN:semver', value: true }) | |
| expect(() => Utils.validateRule(rule)).not.toThrow() | |
| expect(Utils.validateRule(rule)).toBe(false) | |
| }) |
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7536
Utils.validateRule(used by both the create-segment and edit-segment forms to decide whether a condition row is valid) crashes withTypeError: Cannot read properties of undefined (reading 'split')for some segment conditions.#7779 previously added a guard so a missing (
null/undefined) condition value returnsfalseinstead of crashing for the semver and Modulo operators. That guard only covers a missing value, though — it still calls.split()directly onrule.valueonce the value is present. If a condition reachesvalidateRulewith a truthy value that isn't a string (for example a boolean, which can happen while a condition row's value is mid-update),.splitdoesn't exist on it and the same class of crash occurs, which is why this issue stayed open after #7779 merged.This PR coerces the value to a string (
`${rule.value}`) before calling.split()in both the semver and Modulo branches, sovalidateRulenever throws regardless of what type the value happens to be, while keeping the existing pass/fail validation behaviour for well-formed input unchanged.How did you test this code?
Added unit tests in
frontend/common/utils/__tests__/utils.test.tscoveringvalidateRulefor the Modulo and semver operators with missing (undefined/null/empty string) and non-string (boolean) values, plus theIS_SET/IS_NOT_SET"hideValue" operator, and existing valid/invalid value cases. The two non-string-value tests fail on the code prior to this fix (confirmed by reverting the fix locally and re-running) and pass afterwards.Ran the full frontend unit test suite (
npm run test:unit): 368 tests passed across 28 suites. Also rannpx eslintandnpm run typecheckagainst the changed files — no new lint or type errors introduced.