From d93132a877b1b37182dcda48bcb0f1f050ece515 Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Thu, 23 Jan 2025 15:14:49 +0530 Subject: [PATCH] fix: when value of array is empty, no need for data type recalculation (#38794) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Problem When we have a `select/multi-select` field type and the source data gives empty array to the `JSONFormWidget`, the widget tries to gauge the sub type for value inside the array, and since it is empty it got `undefined` This led to re-evaluation of property config for the field. Solution - Added a check to prevent unnecessary recalculation of sub data types when arrays are empty in `checkIfArrayAndSubDataTypeChanged`. - Changed parameter type from `any` to `unknown` in `dataTypeFor` and `subDataTypeFor` functions for improved type safety. - Added and refactored unit tests. This refactor enhances type safety and optimizes performance in the schema parsing logic. Fixes #37246 _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" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: cad7015881c16a9af273b84dcf44cc33e32fb7d9 > Cypress dashboard. > Tags: `@tag.JSONForm` > Spec: >
Wed, 22 Jan 2025 09:50:23 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Tests** - Enhanced test suite for determining field types. - Added comprehensive test cases covering primitive values, email formats, date formats, array types, object types, and edge cases. - **Refactor** - Updated type annotations in schema parser functions to improve type safety. - Changed parameter types from `any` to `unknown`. - Added clarifying comment for handling empty arrays in type checking. --- .../JSONFormWidget/schemaParser.test.ts | 159 +++++++++++++----- src/widgets/JSONFormWidget/schemaParser.ts | 17 +- 2 files changed, 124 insertions(+), 52 deletions(-) diff --git a/src/widgets/JSONFormWidget/schemaParser.test.ts b/src/widgets/JSONFormWidget/schemaParser.test.ts index 947a1c48ac08..20e7ed8a6b99 100644 --- a/src/widgets/JSONFormWidget/schemaParser.test.ts +++ b/src/widgets/JSONFormWidget/schemaParser.test.ts @@ -1873,54 +1873,123 @@ describe(".normalizeArrayValue", () => { }); describe(".fieldTypeFor", () => { - it("return default field type of data passed", () => { - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const inputAndExpectedOutputs: [any, FieldType][] = [ - ["string", FieldType.TEXT_INPUT], - ["2021-12-30T10:36:12.1212+05:30", FieldType.DATEPICKER], - ["December 30, 2021 10:36 AM", FieldType.DATEPICKER], - ["December 30, 2021", FieldType.DATEPICKER], - ["2021-12-30 10:36", FieldType.DATEPICKER], - ["2021-12-30T10:36:12", FieldType.DATEPICKER], - ["2021-12-30 10:36:12 AM", FieldType.DATEPICKER], - ["30/12/2021 10:36", FieldType.DATEPICKER], - ["30 December, 2021", FieldType.DATEPICKER], - ["10:36 AM 30 December, 2021", FieldType.DATEPICKER], - ["2021-12-30", FieldType.DATEPICKER], - ["12-30-2021", FieldType.DATEPICKER], - ["30-12-2021", FieldType.DATEPICKER], - ["12/30/2021", FieldType.DATEPICKER], - ["30/12/2021", FieldType.DATEPICKER], - ["30/12/21", FieldType.DATEPICKER], - ["12/30/21", FieldType.DATEPICKER], - ["40/10/40", FieldType.TEXT_INPUT], - ["2000/10", FieldType.TEXT_INPUT], - ["1", FieldType.TEXT_INPUT], - ["#111", FieldType.TEXT_INPUT], - ["999", FieldType.TEXT_INPUT], - ["test@demo.com", FieldType.EMAIL_INPUT], - ["test@.com", FieldType.TEXT_INPUT], - [10, FieldType.NUMBER_INPUT], - [[{}], FieldType.ARRAY], - [[""], FieldType.MULTISELECT], - [[1], FieldType.MULTISELECT], - [[null], FieldType.MULTISELECT], - [null, FieldType.TEXT_INPUT], - [undefined, FieldType.TEXT_INPUT], - [{ foo: "" }, FieldType.OBJECT], - [ - () => { - 10; - }, - FieldType.TEXT_INPUT, - ], + it("returns appropriate field types for primitive values", () => { + const testCases = [ + { input: "simple text", expected: FieldType.TEXT_INPUT }, + { input: 42, expected: FieldType.NUMBER_INPUT }, + { input: true, expected: FieldType.SWITCH }, + { input: null, expected: FieldType.TEXT_INPUT }, + { input: undefined, expected: FieldType.TEXT_INPUT }, ]; - inputAndExpectedOutputs.forEach(([input, expectedOutput]) => { - const result = fieldTypeFor(input); + testCases.forEach(({ expected, input }) => { + expect(fieldTypeFor(input)).toEqual(expected); + }); + }); - expect(result).toEqual(expectedOutput); + it("detects email field type correctly", () => { + const testCases = [ + { input: "valid@email.com", expected: FieldType.EMAIL_INPUT }, + { input: "user.name+tag@example.co.uk", expected: FieldType.EMAIL_INPUT }, + { input: "invalid.email@", expected: FieldType.TEXT_INPUT }, + { input: "@invalid.com", expected: FieldType.TEXT_INPUT }, + { input: "not-an-email", expected: FieldType.TEXT_INPUT }, + ]; + + testCases.forEach(({ expected, input }) => { + expect(fieldTypeFor(input)).toEqual(expected); + }); + }); + + it("detects date field type correctly", () => { + const testCases = [ + { input: "2024-03-14", expected: FieldType.DATEPICKER }, + { input: "03/14/2024", expected: FieldType.DATEPICKER }, + { input: "14/03/2024", expected: FieldType.DATEPICKER }, + { input: "2024-03-14T15:30:00", expected: FieldType.DATEPICKER }, + { input: "March 14, 2024", expected: FieldType.DATEPICKER }, + { input: "not-a-date", expected: FieldType.TEXT_INPUT }, + { input: "99/99/9999", expected: FieldType.TEXT_INPUT }, + { + input: "2021-12-30T10:36:12.1212+05:30", + expected: FieldType.DATEPICKER, + }, + { input: "December 30, 2021 10:36 AM", expected: FieldType.DATEPICKER }, + { input: "2021-12-30 10:36", expected: FieldType.DATEPICKER }, + { input: "2021-12-30 10:36:12 AM", expected: FieldType.DATEPICKER }, + { input: "30/12/2021 10:36", expected: FieldType.DATEPICKER }, + { input: "30 December, 2021", expected: FieldType.DATEPICKER }, + { input: "10:36 AM 30 December, 2021", expected: FieldType.DATEPICKER }, + ]; + + testCases.forEach(({ expected, input }) => { + expect(fieldTypeFor(input)).toEqual(expected); + }); + }); + + it("determines array field types correctly", () => { + const testCases = [ + { + input: [ + { id: 1, name: "test1" }, + { id: 2, name: "test2" }, + ], + expected: FieldType.ARRAY, + }, + { + input: ["option1", "option2"], + expected: FieldType.MULTISELECT, + }, + { + input: [1, 2, 3], + expected: FieldType.MULTISELECT, + }, + { + input: [], + expected: FieldType.MULTISELECT, + }, + { + input: [["nested"], ["arrays"]], + expected: FieldType.ARRAY, + }, + ]; + + testCases.forEach(({ expected, input }) => { + expect(fieldTypeFor(input)).toEqual(expected); + }); + }); + + it("handles object field types correctly", () => { + const testCases = [ + { + input: { key: "value" }, + expected: FieldType.OBJECT, + }, + { + input: { nested: { object: true } }, + expected: FieldType.OBJECT, + }, + { + input: {}, + expected: FieldType.OBJECT, + }, + ]; + + testCases.forEach(({ expected, input }) => { + expect(fieldTypeFor(input)).toEqual(expected); + }); + }); + + it("handles special cases and edge values", () => { + const testCases = [ + { input: () => {}, expected: FieldType.TEXT_INPUT }, + { input: Symbol("test"), expected: FieldType.TEXT_INPUT }, + { input: BigInt(9007199254740991), expected: FieldType.NUMBER_INPUT }, + { input: NaN, expected: FieldType.NUMBER_INPUT }, + ]; + + testCases.forEach(({ expected, input }) => { + expect(fieldTypeFor(input)).toEqual(expected); }); }); }); diff --git a/src/widgets/JSONFormWidget/schemaParser.ts b/src/widgets/JSONFormWidget/schemaParser.ts index 42c4724f9ec1..da63757acc9c 100644 --- a/src/widgets/JSONFormWidget/schemaParser.ts +++ b/src/widgets/JSONFormWidget/schemaParser.ts @@ -183,9 +183,7 @@ export const getSourceDataPathFromSchemaItemPath = ( return sourceDataPath; }; -// TODO: Fix this the next time the file is edited -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export const dataTypeFor = (value: any) => { +export const dataTypeFor = (value: unknown) => { const typeOfValue = typeof value; if (Array.isArray(value)) return DataType.ARRAY; @@ -195,13 +193,11 @@ export const dataTypeFor = (value: any) => { return typeOfValue as DataType; }; -// TODO: Fix this the next time the file is edited -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export const subDataTypeFor = (value: any) => { +export const subDataTypeFor = (value: unknown) => { const dataType = dataTypeFor(value); if (dataType === DataType.ARRAY) { - return dataTypeFor(value[0]); + return dataTypeFor((value as unknown[])[0]); } return undefined; @@ -352,6 +348,13 @@ export const checkIfArrayAndSubDataTypeChanged = ( const currSubDataType = subDataTypeFor(currentData); const prevSubDataType = subDataTypeFor(prevData); + /** + * If the array is empty, then we don't need to check for sub data type + * as it would be an empty array, which will always be `undefined`. + * which leads to unnecessary re calculation of a type(https://github.com/appsmithorg/appsmith/issues/37246) + */ + if (currentData.length === 0 || prevData.length === 0) return false; + return currSubDataType !== prevSubDataType; };