Skip to content

Commit

Permalink
fix: when value of array is empty, no need for data type recalculation (
Browse files Browse the repository at this point in the history
appsmithorg#38794)

## Description
<ins>Problem</ins>
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. 

<ins>Solution</ins>
- 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 appsmithorg#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"

### 🔍 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/12905055896>
> Commit: cad7015
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12905055896&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.JSONForm`
> Spec:
> <hr>Wed, 22 Jan 2025 09:50:23 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

- **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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
rahulbarwal authored Jan 23, 2025
1 parent a9afed6 commit d93132a
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 52 deletions.
159 changes: 114 additions & 45 deletions src/widgets/JSONFormWidget/schemaParser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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],
["[email protected]", FieldType.EMAIL_INPUT],
["[email protected]", 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: "[email protected]", expected: FieldType.EMAIL_INPUT },
{ input: "[email protected]", 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);
});
});
});
Expand Down
17 changes: 10 additions & 7 deletions src/widgets/JSONFormWidget/schemaParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
};

Expand Down

0 comments on commit d93132a

Please sign in to comment.