Avoid double validation (#2687)

app/client/src/utils/DynamicBindingUtils.test.ts

@@ -43,6 +43,9 @@ describe("isChildPropertyPath function", () => {
       ["Table1.selectedRow", "1Table1.selectedRow", false],
       ["Table1.selectedRow", "Table11selectedRow", false],
       ["Table1.selectedRow", "Table1.selectedRow", true],
+      ["Dropdown1.options", "Dropdown1.options[1]", true],
+      ["Dropdown1.options[1]", "Dropdown1.options[1].value", true],
+      ["Dropdown1", "Dropdown1.options[1].value", true],
     ];
     cases.forEach((testCase) => {
       const result = isChildPropertyPath(testCase[0], testCase[1]);

app/client/src/utils/DynamicBindingUtils.ts

@@ -249,12 +249,11 @@ export const unsafeFunctionForEval = [
   "setInterval",
   "Promise",
 ];
+
 export const isChildPropertyPath = (
   parentPropertyPath: string,
   childPropertyPath: string,
-): boolean => {
+): boolean =>
-  const regexTest = new RegExp(
+  parentPropertyPath === childPropertyPath ||
-    `^${parentPropertyPath.replace(".", "\\.")}(\\.\\S+)?$`,
+  childPropertyPath.startsWith(`${parentPropertyPath}.`) ||
-  );
+  childPropertyPath.startsWith(`${parentPropertyPath}[`);
-  return regexTest.test(childPropertyPath);
-};

app/client/src/workers/evaluation.test.ts

@@ -436,7 +436,7 @@ const WIDGET_CONFIG_MAP: WidgetTypeConfigMap = {
 
 const BASE_WIDGET: DataTreeWidget = {
   widgetId: "randomID",
-  widgetName: "randomName",
+  widgetName: "randomWidgetName",
   bottomRow: 0,
   isLoading: false,
   leftColumn: 0,
@@ -452,7 +452,7 @@ const BASE_WIDGET: DataTreeWidget = {
 
 const BASE_ACTION: DataTreeAction = {
   actionId: "randomId",
-  name: "randomName",
+  name: "randomActionName",
   config: {
     timeoutInMillisecond: 10,
   },
@@ -645,6 +645,7 @@ describe("DataTreeEvaluator", () => {
       ...unEvalTree,
       Api1: {
         ...BASE_ACTION,
+        name: "Api1",
         data: [
           {
             test: "Hey",
@@ -669,7 +670,6 @@ describe("DataTreeEvaluator", () => {
     ]);
     expect(updatedDependencyMap).toStrictEqual({
       Api1: ["Api1.data"],
-      Input1: ["Input1.text"],
       Text1: ["Text1.text"],
       Text2: ["Text2.text"],
       Text3: ["Text3.text"],
@@ -693,7 +693,6 @@ describe("DataTreeEvaluator", () => {
       "Table1.selectedRowIndex": [],
       "Table1.selectedRowIndices": [],
       "Text4.text": [],
-      "Input1.text": [],
     });
   });
 
@@ -710,6 +709,7 @@ describe("DataTreeEvaluator", () => {
       },
       Api1: {
         ...BASE_ACTION,
+        name: "Api1",
         data: [
           {
             test: "Hey",
@@ -751,7 +751,6 @@ describe("DataTreeEvaluator", () => {
         "Dropdown1.selectedOptionValue",
         "Dropdown1.selectedOptionValueArr",
       ],
-      Input1: ["Input1.text"],
       "Text2.text": ["Text1.text"],
       "Text3.text": ["Text1.text"],
       "Dropdown1.selectedOptionValue": [],
@@ -761,7 +760,6 @@ describe("DataTreeEvaluator", () => {
       "Table1.selectedRowIndex": [],
       "Table1.selectedRowIndices": [],
       "Text4.text": ["Table1.selectedRow.test"],
-      "Input1.text": [],
     });
   });
 });

app/client/src/workers/evaluation.worker.ts

@@ -39,6 +39,7 @@ import {
   removeFunctionsFromDataTree,
   translateDiffEventToDataTreeDiffEvent,
   validateWidgetProperty,
+  getImmediateParentsOfPropertyPaths,
 } from "./evaluationUtils";
 import {
   EXECUTION_PARAM_KEY,
@@ -205,6 +206,7 @@ export class DataTreeEvaluator {
   widgetConfigMap: WidgetTypeConfigMap = {};
   evalTree: DataTree = {};
   allKeys: Record<string, true> = {};
+  validationPaths: Record<string, Set<string>> = {};
   oldUnEvalTree: DataTree = {};
   errors: EvalError[] = [];
   parsedValueCache: Map<
@@ -327,25 +329,11 @@ export class DataTreeEvaluator {
     removedPaths.forEach((removedPath) => {
       _.unset(this.evalTree, removedPath);
     });
-
     const evaluatedTree = this.evaluateTree(this.evalTree, subTreeSortOrder);
     const evalStop = performance.now();
 
-    const validateStart = performance.now();
-    // Validate and parse updated widgets
-    const updatedWidgets = new Set(
-      subTreeSortOrder.map((path) => path.split(".")[0]),
-    );
-
-    const validatedTree = getValidatedTree(
-      this.widgetConfigMap,
-      evaluatedTree,
-      updatedWidgets,
-    );
-    const validateEnd = performance.now();
-
     // Remove functions
-    this.evalTree = removeFunctionsFromDataTree(validatedTree);
+    this.evalTree = removeFunctionsFromDataTree(evaluatedTree);
     const totalEnd = performance.now();
     // TODO: For some reason we are passing some reference which are getting mutated.
     // Need to check why big api responses are getting split between two eval runs
@@ -360,7 +348,6 @@ export class DataTreeEvaluator {
         calculateSortOrderStop - calculateSortOrderStart
       ).toFixed(2),
       evaluate: (evalStop - evalStart).toFixed(2),
-      validate: (validateEnd - validateStart).toFixed(2),
     };
     LOGS.push({ timeTakenForSubTreeEval });
     return this.evalTree;
@@ -382,7 +369,7 @@ export class DataTreeEvaluator {
       // Add all the sorted nodes in the final list
       finalSortOrder = [...finalSortOrder, ...subSortOrderArray];
 
-      parents = this.getImmediateParentsOfPropertyPaths(subSortOrderArray);
+      parents = getImmediateParentsOfPropertyPaths(subSortOrderArray);
       // If we find parents of the property paths in the sorted array, we should continue finding all the nodes dependent
       // on the parents
       computeSortOrder = parents.length > 0;
@@ -407,27 +394,6 @@ export class DataTreeEvaluator {
     return finalSortOrderArray;
   }
 
-  // The idea is to find the immediate parents of the property paths
-  // e.g. For Table1.selectedRow.email, the parent is Table1.selectedRow
-  getImmediateParentsOfPropertyPaths(
-    propertyPaths: Array<string>,
-  ): Array<string> {
-    // Use a set to ensure that we dont have duplicates
-    const parents: Set<string> = new Set();
-
-    propertyPaths.forEach((path) => {
-      const parentProperty = path.substr(0, path.lastIndexOf("."));
-
-      if (parentProperty.length != 0) {
-        parents.add(parentProperty);
-      } else {
-        // We have reached the top of the path. No parent exists
-      }
-    });
-
-    return Array.from(parents);
-  }
-
   getEvaluationSortOrder(
     changes: Array<string>,
     inverseMap: DependencyMap,
@@ -455,9 +421,34 @@ export class DataTreeEvaluator {
     return sortOrder;
   }
 
+  getValidationPaths(unevalDataTree: DataTree): Record<string, Set<string>> {
+    const result: Record<string, Set<string>> = {};
+    for (const key in unevalDataTree) {
+      const entity = unevalDataTree[key];
+      if (isAction(entity)) {
+        // TODO: add the properties to a global map somewhere
+        result[entity.name] = new Set(
+          ["config", "isLoading", "data"].map((e) => `${entity.name}.${e}`),
+        );
+      } else if (isWidget(entity)) {
+        if (!this.widgetConfigMap[entity.type])
+          throw new CrashingError(
+            `${entity.widgetName} has unrecognised entity type: ${entity.type}`,
+          );
+        const { validations } = this.widgetConfigMap[entity.type];
+
+        result[entity.widgetName] = new Set(
+          Object.keys(validations).map((e) => `${entity.widgetName}.${e}`),
+        );
+      }
+    }
+    return result;
+  }
+
   createDependencyMap(unEvalTree: DataTree): DependencyMap {
     let dependencyMap: DependencyMap = {};
     this.allKeys = getAllPaths(unEvalTree);
+    this.validationPaths = this.getValidationPaths(unEvalTree);
     Object.keys(unEvalTree).forEach((entityName) => {
       const entity = unEvalTree[entityName];
       if (isAction(entity) || isWidget(entity)) {
@@ -908,12 +899,16 @@ export class DataTreeEvaluator {
     // In worst case, it tends to take ~12.5% of entire diffCalc (8 ms out of 67ms for 132 array of NEW)
     // TODO: Optimise by only getting paths of changed node
     this.allKeys = getAllPaths(unEvalDataTree);
+    this.validationPaths = this.getValidationPaths(unEvalDataTree);
     // Transform the diff library events to Appsmith evaluator events
     differences
       .map(translateDiffEventToDataTreeDiffEvent)
       .forEach((dataTreeDiff) => {
         const entityName = dataTreeDiff.payload.propertyPath.split(".")[0];
-        const entity = unEvalDataTree[entityName];
+        let entity = unEvalDataTree[entityName];
+        if (dataTreeDiff.event === DataTreeDiffEvent.DELETE) {
+          entity = this.oldUnEvalTree[entityName];
+        }
         const entityType = isValidEntity(entity) ? entity.ENTITY_TYPE : "noop";
 
         if (entityType !== "noop") {
@@ -956,13 +951,9 @@ export class DataTreeEvaluator {
               removedPaths.push(dataTreeDiff.payload.propertyPath);
               // If an existing widget was deleted, remove all the bindings from the global dependency map
               if (
-                entityType === ENTITY_TYPE.WIDGET &&
+                isWidget(entity) &&
                 dataTreeDiff.payload.propertyPath === entityName
               ) {
-                const entity: DataTreeWidget = unEvalDataTree[
-                  entityName
-                ] as DataTreeWidget;
-
                 const widgetBindings = this.listEntityDependencies(
                   entity,
                   entityName,
@@ -1098,40 +1089,34 @@ export class DataTreeEvaluator {
     removedPaths: Array<string>,
   ) {
     const changePaths: Set<string> = new Set(dependenciesOfRemovedPaths);
-    differences.forEach((d) => {
+    for (const d of differences) {
-      if (d.path) {
+      if (!Array.isArray(d.path) || d.path.length === 0) continue; // Null check for typescript
-        // Apply the changes into the evalTree so that it gets the latest changes
+      // Apply the changes into the evalTree so that it gets the latest changes
-        applyChange(this.evalTree, undefined, d);
+      applyChange(this.evalTree, undefined, d);
 
-        // If this is a property path change, simply add for evaluation
+      // If this is a property path change, simply add for evaluation and move on
-        if (d.path.length > 1) {
+      if (d.path.length > 1) {
-          const propertyPath = convertPathToString(d.path);
+        changePaths.add(convertPathToString(d.path));
-          changePaths.add(propertyPath);
+        continue;
-
+      }
-          // If this is an array update, trim the array index and add it to the change paths for evaluation
+      // A top level entity (widget/action) has been added or deleted
-          // This is because sometimes inside an object of array time, if only a particular entry changes, the
+      if (d.path.length === 1) {
-          // difference comes as propertyPath[0].fieldChanged. Another entity could depend on propertyPath and not
+        const entityName = d.path[0];
-          // propertyPath[0]. The said entity must be evaluated.
+        /**
-          // To do this, we are trimming the array index
+         * We want to add all pre-existing dynamic and static bindings in dynamic paths of this entity to get evaluated and validated.
-          if (propertyPath.lastIndexOf("[") > 0) {
+         * Example:
-            changePaths.add(
+         * - Table1.tableData = {{Api1.data}}
-              propertyPath.substr(0, propertyPath.lastIndexOf("[")),
+         * - Api1 gets created.
-            );
+         * - This function gets called with a diff {path:["Api1"]}
+         * We want to add `Api.data` to changedPaths so that `Table1.tableData` can be discovered below.
+         */
+        if (entityName in this.validationPaths) {
+          for (const dependency of this.validationPaths[entityName]) {
+            changePaths.add(dependency);
           }
-        } else if (d.path.length === 1) {
-          /*
-              When we see a new widget has been added or or delete an old widget ( d.path.length === 1)
-              We want to add all the dependencies in the sorted order to make
-              sure all the bindings are evaluated.
-            */
-          this.sortedDependencies.forEach((dependency) => {
-            if (d.path && dependency.split(".")[0] === d.path[0]) {
-              changePaths.add(dependency);
-            }
-          });
         }
       }
-    });
+    }
 
     // If a nested property path has changed and someone (say x) is dependent on the parent of the said property,
     // x must also be evaluated. For example, the following relationship exists in dependency map:

app/client/src/workers/evaluationUtils.ts

@@ -1,4 +1,8 @@
-import { DependencyMap, isDynamicValue } from "../utils/DynamicBindingUtils";
+import {
+  DependencyMap,
+  isDynamicValue,
+  isChildPropertyPath,
+} from "../utils/DynamicBindingUtils";
 import { WidgetType } from "../constants/WidgetConstants";
 import { WidgetProps } from "../widgets/BaseWidget";
 import { WidgetTypeConfigMap } from "../utils/WidgetFactory";
@@ -6,12 +10,18 @@ import { VALIDATORS } from "./validations";
 import { Diff } from "deep-diff";
 import {
   DataTree,
+  DataTreeAction,
   DataTreeEntity,
   DataTreeWidget,
   ENTITY_TYPE,
 } from "../entities/DataTree/dataTreeFactory";
 import _ from "lodash";
 
+// Dropdown1.options[1].value -> Dropdown1.options[1]
+// Dropdown1.options[1] -> Dropdown1.options
+// Dropdown1.options -> Dropdown1
+export const IMMEDIATE_PARENT_REGEX = /^(.*)(\..*|\[.*\])$/;
+
 export enum DataTreeDiffEvent {
   NEW = "NEW",
   DELETE = "DELETE",
@@ -115,13 +125,6 @@ export const translateDiffEventToDataTreeDiffEvent = (
   return result;
 };
 
-export const isPropertyPathOrNestedPath = (
-  path: string,
-  comparePath: string,
-): boolean => {
-  return path === comparePath || comparePath.startsWith(`${path}.`);
-};
-
 /*
   Table1.selectedRow
   Table1.selectedRow.email: ["Input1.defaultText"]
@@ -137,7 +140,7 @@ export const addDependantsOfNestedPropertyPaths = (
     withNestedPaths.add(propertyPath);
     dependantNodes
       .filter((dependantNodePath) =>
-        isPropertyPathOrNestedPath(propertyPath, dependantNodePath),
+        isChildPropertyPath(propertyPath, dependantNodePath),
       )
       .forEach((dependantNodePath) => {
         inverseMap[dependantNodePath].forEach((path) => {
@@ -148,7 +151,7 @@ export const addDependantsOfNestedPropertyPaths = (
   return [...withNestedPaths.values()];
 };
 
-export function isWidget(entity: DataTreeEntity): boolean {
+export function isWidget(entity: DataTreeEntity): entity is DataTreeWidget {
   return (
     typeof entity === "object" &&
     "ENTITY_TYPE" in entity &&
@@ -156,7 +159,7 @@ export function isWidget(entity: DataTreeEntity): boolean {
   );
 }
 
-export function isAction(entity: DataTreeEntity): boolean {
+export function isAction(entity: DataTreeEntity): entity is DataTreeAction {
   return (
     typeof entity === "object" &&
     "ENTITY_TYPE" in entity &&
@@ -197,17 +200,18 @@ export const makeParentsDependOnChildren = (
   });
   return depMap;
 };
+
 export const makeParentsDependOnChild = (
   depMap: DependencyMap,
   child: string,
 ): DependencyMap => {
   const result: DependencyMap = depMap;
   let curKey = child;
-  const rgx = /^(.*)\..*$/;
+
   let matches: Array<string> | null;
   // Note: The `=` is intentional
   // Stops looping when match is null
-  while ((matches = curKey.match(rgx)) !== null) {
+  while ((matches = curKey.match(IMMEDIATE_PARENT_REGEX)) !== null) {
     const parentKey = matches[1];
     // Todo: switch everything to set.
     const existing = new Set(result[parentKey] || []);
@@ -218,6 +222,25 @@ export const makeParentsDependOnChild = (
   return result;
 };
 
+// The idea is to find the immediate parents of the property paths
+// e.g. For Table1.selectedRow.email, the parent is Table1.selectedRow
+export const getImmediateParentsOfPropertyPaths = (
+  propertyPaths: Array<string>,
+): Array<string> => {
+  // Use a set to ensure that we dont have duplicates
+  const parents: Set<string> = new Set();
+
+  propertyPaths.forEach((path) => {
+    const matches = path.match(IMMEDIATE_PARENT_REGEX);
+
+    if (matches !== null) {
+      parents.add(matches[1]);
+    }
+  });
+
+  return Array.from(parents);
+};
+
 export function validateWidgetProperty(
   widgetConfigMap: WidgetTypeConfigMap,
   widgetType: WidgetType,
@@ -245,14 +268,8 @@ export function validateWidgetProperty(
 export function getValidatedTree(
   widgetConfigMap: WidgetTypeConfigMap,
   tree: DataTree,
-  only?: Set<string>,
 ) {
   return Object.keys(tree).reduce((tree, entityKey: string) => {
-    if (only && only.size) {
-      if (!only.has(entityKey)) {
-        return tree;
-      }
-    }
     const entity = tree[entityKey] as DataTreeWidget;
     if (!isWidget(entity)) {
       return tree;