fix: edge case to ignore parsing very large numbers#37104
fix: edge case to ignore parsing very large numbers#37104rahulbarwal merged 4 commits intoreleasefrom
Conversation
WalkthroughThe changes in this pull request focus on enhancing type safety within the codebase by updating function parameter types from Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.test.ts (2)
78-102: Simplify JSON test inputThe multiline template literal could be simplified for better readability.
- const input = ` - { - "label": "green", - "value": "green" - } - `; + const input = JSON.stringify({ + label: "green", + value: "green" + });
104-120: Document the number precision limitationThis test addresses the core issue from #27881, but would benefit from a comment explaining the JavaScript number precision limitation (2^53 - 1).
+ // JavaScript's Number.MAX_SAFE_INTEGER is 2^53 - 1 (9007199254740991) + // Numbers larger than this lose precision when parsed, so we keep them as strings it("Edge Case: For very long numbers passed as string, don't parse it as number", () => {app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.ts (2)
Line range hint
18-29: TODO comment needs addressingThere's a TODO comment about fixing ESLint disable. Consider using proper TypeScript types instead of
any.Suggested fix:
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - obj: any, + obj: unknown,
Line range hint
13-73: Consider a comprehensive solution for large number handlingWhile the current changes improve type safety, a more comprehensive solution might be needed for handling large numbers:
- Consider adding a new field type specifically for large numbers
- Document the maximum supported number size in the API
- Add validation to prevent precision loss
app/client/src/widgets/JSONFormWidget/schemaParser.ts (2)
Line range hint
328-328: Track TODO comments in issue tracker.Multiple TODO comments indicate future fixes needed for eslint rules. These should be tracked to ensure they don't get forgotten.
Would you like me to create GitHub issues to track these TODOs? I can help create structured issues for:
- Fixing eslint disable rules
- Updating type definitions
Also applies to: 366-366
Line range hint
229-266: Add validation for large numbers in fieldTypeFor.Given the PR objectives to fix large number parsing issues, consider adding explicit validation for large numbers in
fieldTypeFor.export const fieldTypeFor = (value: unknown): FieldType => { const dataType = dataTypeFor(value); + if (dataType === DataType.NUMBER && typeof value === 'number') { + if (!Number.isSafeInteger(value)) { + console.warn(`Large number detected: ${value}. Precision may be lost.`); + } + } const potentialFieldType = DATA_TYPE_POTENTIAL_FIELD[dataType];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/widgets/JSONFormWidget/schemaParser.ts (1 hunks)
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.test.ts (1 hunks)
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.ts (3 hunks)
🔇 Additional comments (4)
app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.test.ts (1)
7-7: Well-structured test organization!The separation into "falsey" and "truthy" value handling provides clear test categorization.
Also applies to: 60-60
app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.ts (2)
10-10: Type safety improvements look good!The change from
anytoLoDashStaticand parameter renaming improves type safety and readability.Also applies to: 13-15
Line range hint
51-73: Verify validation behavior with large numbersThe validation logic looks good, but let's verify it handles the edge cases correctly.
✅ Verification successful
The previous search was too specific. Let's broaden our search to find relevant test cases and implementations that can help us verify the behavior with large numbers.
Let's examine the test cases specifically related to number handling and the implementation of isFinite validation to verify the behavior with large numbers.
Let's examine the complete test file and the actual implementation of number validation to verify how large numbers are handled.
Large number validation is properly handled
The code correctly handles large numbers through:
- Explicit test case for very long numbers (60+ digits) ensuring they remain as strings
- Proper use of
_.isFinite()for number validation- Fallback to string handling for large numeric values that exceed JavaScript's number precision
The validation logic maintains data integrity by not attempting to parse very large numbers into JavaScript numbers, which could lead to precision loss.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases involving large numbers rg -l "defaultOptionValueValidation.*[0-9]{16,}" --type ts # Search for related bug reports or issues rg "large.*number.*validation|precision.*loss" --type tsLength of output: 119
Script:
#!/bin/bash # Search for test cases related to select validation rg -A 5 "defaultOptionValueValidation" --type ts # Search for test cases with numeric values in select validation rg -A 5 "isFinite.*value" --type ts # Look for test files related to select validation fd "select.*test" -e ts -e jsLength of output: 49957
Script:
#!/bin/bash # Let's check the test cases in select.test.ts for numeric validation rg -A 10 "test.*number|test.*large|test.*finite" app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.test.ts # Check implementation of isFinite validation ast-grep --pattern 'if ($$$isFinite$$$) { $$$ }'Length of output: 189
Script:
#!/bin/bash # Let's check the test file content for numeric validation cases cat app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.test.ts # Also check the implementation of _.isFinite usage rg -A 5 "_.isFinite\(value\)" --type tsLength of output: 11796
app/client/src/widgets/JSONFormWidget/schemaParser.ts (1)
Line range hint
642-644: Consider adding recursion depth limit.The recursive schema parsing in
convertArrayToSchemaandconvertObjectToSchemacould potentially cause stack overflow with deeply nested structures.Consider adding a depth limit parameter to prevent stack overflow:
static convertObjectToSchema = ({ baseSchemaPath, currSourceData, + depth = 0, + maxDepth = 100, ...rest }: Omit<ParserOptions, "identifier">): Schema => { + if (depth >= maxDepth) { + console.warn(`Maximum recursion depth ${maxDepth} reached`); + return {}; + }Also applies to: 699-701
| it("return undefined when input is undefined", () => { | ||
| const input = undefined; | ||
| const expectedOutput = { | ||
| isValid: true, | ||
| parsed: undefined, | ||
| messages: [{ name: "", message: "" }], | ||
| }; | ||
|
|
||
| it("return null when input is null", () => { | ||
| const input = null; | ||
| const expectedOutput = { | ||
| isValid: true, | ||
| parsed: null, | ||
| messages: [{ name: "", message: "" }], | ||
| }; | ||
|
|
||
| const response = defaultOptionValueValidation( | ||
| input, | ||
| {} as JSONFormWidgetProps, | ||
| _, | ||
| ); | ||
|
|
||
| expect(response).toEqual(expectedOutput); | ||
| }); | ||
| const response = defaultOptionValueValidation( | ||
| input, | ||
| {} as JSONFormWidgetProps, | ||
| _, | ||
| ); | ||
|
|
||
| it("return empty string with empty string", () => { | ||
| const input = ""; | ||
| const expectedOutput = { | ||
| isValid: true, | ||
| parsed: "", | ||
| messages: [{ name: "", message: "" }], | ||
| }; | ||
|
|
||
| const response = defaultOptionValueValidation( | ||
| input, | ||
| {} as JSONFormWidgetProps, | ||
| _, | ||
| ); | ||
|
|
||
| expect(response).toEqual(expectedOutput); | ||
| }); | ||
| expect(response).toEqual(expectedOutput); | ||
| }); | ||
|
|
||
| it("return null when input is null", () => { | ||
| const input = null; | ||
| const expectedOutput = { | ||
| isValid: true, | ||
| parsed: null, | ||
| messages: [{ name: "", message: "" }], | ||
| }; | ||
|
|
||
| const response = defaultOptionValueValidation( | ||
| input, | ||
| {} as JSONFormWidgetProps, | ||
| _, | ||
| ); | ||
|
|
||
| expect(response).toEqual(expectedOutput); | ||
| }); | ||
|
|
||
| it("return empty string with empty string", () => { | ||
| const input = ""; | ||
| const expectedOutput = { | ||
| isValid: true, | ||
| parsed: "", | ||
| messages: [{ name: "", message: "" }], | ||
| }; | ||
|
|
||
| const response = defaultOptionValueValidation( | ||
| input, | ||
| {} as JSONFormWidgetProps, | ||
| _, | ||
| ); | ||
|
|
||
| it("return value with string", () => { | ||
| const input = "green"; | ||
| const expectedOutput = { | ||
| isValid: true, | ||
| parsed: "green", | ||
| messages: [{ name: "", message: "" }], | ||
| }; | ||
|
|
||
| const response = defaultOptionValueValidation( | ||
| input, | ||
| {} as JSONFormWidgetProps, | ||
| _, | ||
| ); | ||
|
|
||
| expect(response).toEqual(expectedOutput); | ||
| expect(response).toEqual(expectedOutput); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using test.each for falsey value tests
The falsey value tests follow the same pattern and could be more maintainable using test.each.
- describe("handling falsey values", () => {
- it("return undefined when input is undefined", () => {
- const input = undefined;
- const expectedOutput = {
- isValid: true,
- parsed: undefined,
- messages: [{ name: "", message: "" }],
- };
- // ... similar pattern repeats
+ describe("handling falsey values", () => {
+ test.each([
+ ["undefined", undefined],
+ ["null", null],
+ ["empty string", ""]
+ ])("returns correct output for %s", (_, input) => {
+ const expectedOutput = {
+ isValid: true,
+ parsed: input,
+ messages: [{ name: "", message: "" }],
+ };
+
+ const response = defaultOptionValueValidation(
+ input,
+ {} as JSONFormWidgetProps,
+ _,
+ );
+
+ expect(response).toEqual(expectedOutput);
+ });📝 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("return undefined when input is undefined", () => { | |
| const input = undefined; | |
| const expectedOutput = { | |
| isValid: true, | |
| parsed: undefined, | |
| messages: [{ name: "", message: "" }], | |
| }; | |
| it("return null when input is null", () => { | |
| const input = null; | |
| const expectedOutput = { | |
| isValid: true, | |
| parsed: null, | |
| messages: [{ name: "", message: "" }], | |
| }; | |
| const response = defaultOptionValueValidation( | |
| input, | |
| {} as JSONFormWidgetProps, | |
| _, | |
| ); | |
| expect(response).toEqual(expectedOutput); | |
| }); | |
| const response = defaultOptionValueValidation( | |
| input, | |
| {} as JSONFormWidgetProps, | |
| _, | |
| ); | |
| it("return empty string with empty string", () => { | |
| const input = ""; | |
| const expectedOutput = { | |
| isValid: true, | |
| parsed: "", | |
| messages: [{ name: "", message: "" }], | |
| }; | |
| const response = defaultOptionValueValidation( | |
| input, | |
| {} as JSONFormWidgetProps, | |
| _, | |
| ); | |
| expect(response).toEqual(expectedOutput); | |
| }); | |
| expect(response).toEqual(expectedOutput); | |
| }); | |
| it("return null when input is null", () => { | |
| const input = null; | |
| const expectedOutput = { | |
| isValid: true, | |
| parsed: null, | |
| messages: [{ name: "", message: "" }], | |
| }; | |
| const response = defaultOptionValueValidation( | |
| input, | |
| {} as JSONFormWidgetProps, | |
| _, | |
| ); | |
| expect(response).toEqual(expectedOutput); | |
| }); | |
| it("return empty string with empty string", () => { | |
| const input = ""; | |
| const expectedOutput = { | |
| isValid: true, | |
| parsed: "", | |
| messages: [{ name: "", message: "" }], | |
| }; | |
| const response = defaultOptionValueValidation( | |
| input, | |
| {} as JSONFormWidgetProps, | |
| _, | |
| ); | |
| it("return value with string", () => { | |
| const input = "green"; | |
| const expectedOutput = { | |
| isValid: true, | |
| parsed: "green", | |
| messages: [{ name: "", message: "" }], | |
| }; | |
| const response = defaultOptionValueValidation( | |
| input, | |
| {} as JSONFormWidgetProps, | |
| _, | |
| ); | |
| expect(response).toEqual(expectedOutput); | |
| expect(response).toEqual(expectedOutput); | |
| }); | |
| describe("handling falsey values", () => { | |
| test.each([ | |
| ["undefined", undefined], | |
| ["null", null], | |
| ["empty string", ""] | |
| ])("returns correct output for %s", (_, input) => { | |
| const expectedOutput = { | |
| isValid: true, | |
| parsed: input, | |
| messages: [{ name: "", message: "" }], | |
| }; | |
| const response = defaultOptionValueValidation( | |
| input, | |
| {} as JSONFormWidgetProps, | |
| _, | |
| ); | |
| expect(response).toEqual(expectedOutput); | |
| }); | |
| }); |
app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/select.ts
Show resolved
Hide resolved
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11517646992. |
|
Deploy-Preview-URL: https://ce-37104.dp.appsmith.com |
## Description <ins>Problem</ins> The defaultOptionValueValidation function in select.ts is not properly handling different input values, resulting in inconsistent behavior. <ins>Root cause</ins> This function does not account for edge cases where the input value is a very large number and is resulting in a rounded off number, leading to incorrect validation results. <ins>Solution</ins> This PR changes the behavior to parsing only when required. Fixes appsmithorg#27881 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.JSONForm" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11517642696> > Commit: d31373a > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11517642696&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JSONForm` > Spec: > <hr>Fri, 25 Oct 2024 12:16:10 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced type safety for various functions, improving overall reliability. - Streamlined validation logic for option values, allowing for more robust input handling. - **Bug Fixes** - Improved test coverage for `defaultOptionValueValidation`, ensuring accurate handling of both truthy and falsy values. - **Documentation** - Added comments indicating areas for future improvements in the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Problem
The defaultOptionValueValidation function in select.ts is not properly handling different input values, resulting in inconsistent behavior.
Root cause
This function does not account for edge cases where the input value is a very large number and is resulting in a rounded off number, leading to incorrect validation results.
Solution
This PR changes the behavior to parsing only when required.
Fixes #27881
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.JSONForm"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11517642696
Commit: d31373a
Cypress dashboard.
Tags:
@tag.JSONFormSpec:
Fri, 25 Oct 2024 12:16:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
defaultOptionValueValidation, ensuring accurate handling of both truthy and falsy values.Documentation