From 164de6fb7e30a26f7ca0196bf91d7f11e059b57b Mon Sep 17 00:00:00 2001 From: Piyush Mishra Date: Fri, 29 Jan 2021 11:34:28 +0530 Subject: [PATCH] Avoid double validation (#2687) --- ...il.test.ts => DynamicBindingUtils.test.ts} | 3 + app/client/src/utils/DynamicBindingUtils.ts | 11 +- app/client/src/workers/evaluation.test.ts | 10 +- app/client/src/workers/evaluation.worker.ts | 133 ++++++++---------- app/client/src/workers/evaluationUtils.ts | 55 +++++--- 5 files changed, 107 insertions(+), 105 deletions(-) rename app/client/src/utils/{DynamicBindingsUtil.test.ts => DynamicBindingUtils.test.ts} (89%) diff --git a/app/client/src/utils/DynamicBindingsUtil.test.ts b/app/client/src/utils/DynamicBindingUtils.test.ts similarity index 89% rename from app/client/src/utils/DynamicBindingsUtil.test.ts rename to app/client/src/utils/DynamicBindingUtils.test.ts index b319780940..9b2c93b0b0 100644 --- a/app/client/src/utils/DynamicBindingsUtil.test.ts +++ b/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]); diff --git a/app/client/src/utils/DynamicBindingUtils.ts b/app/client/src/utils/DynamicBindingUtils.ts index 705fbbb9a7..c769bac4f4 100644 --- a/app/client/src/utils/DynamicBindingUtils.ts +++ b/app/client/src/utils/DynamicBindingUtils.ts @@ -249,12 +249,11 @@ export const unsafeFunctionForEval = [ "setInterval", "Promise", ]; + export const isChildPropertyPath = ( parentPropertyPath: string, childPropertyPath: string, -): boolean => { - const regexTest = new RegExp( - `^${parentPropertyPath.replace(".", "\\.")}(\\.\\S+)?$`, - ); - return regexTest.test(childPropertyPath); -}; +): boolean => + parentPropertyPath === childPropertyPath || + childPropertyPath.startsWith(`${parentPropertyPath}.`) || + childPropertyPath.startsWith(`${parentPropertyPath}[`); diff --git a/app/client/src/workers/evaluation.test.ts b/app/client/src/workers/evaluation.test.ts index 06718f2ac7..8442674bd1 100644 --- a/app/client/src/workers/evaluation.test.ts +++ b/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": [], }); }); }); diff --git a/app/client/src/workers/evaluation.worker.ts b/app/client/src/workers/evaluation.worker.ts index 43ffa95e42..0f4f4b28f8 100644 --- a/app/client/src/workers/evaluation.worker.ts +++ b/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 = {}; + validationPaths: Record> = {}; 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, - ): Array { - // Use a set to ensure that we dont have duplicates - const parents: Set = 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, inverseMap: DependencyMap, @@ -455,9 +421,34 @@ export class DataTreeEvaluator { return sortOrder; } + getValidationPaths(unevalDataTree: DataTree): Record> { + const result: Record> = {}; + 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, ) { const changePaths: Set = new Set(dependenciesOfRemovedPaths); - differences.forEach((d) => { - if (d.path) { - // Apply the changes into the evalTree so that it gets the latest changes - applyChange(this.evalTree, undefined, d); + for (const d of differences) { + 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 + applyChange(this.evalTree, undefined, d); - // If this is a property path change, simply add for evaluation - if (d.path.length > 1) { - const propertyPath = convertPathToString(d.path); - changePaths.add(propertyPath); - - // If this is an array update, trim the array index and add it to the change paths for evaluation - // This is because sometimes inside an object of array time, if only a particular entry changes, the - // difference comes as propertyPath[0].fieldChanged. Another entity could depend on propertyPath and not - // propertyPath[0]. The said entity must be evaluated. - // To do this, we are trimming the array index - if (propertyPath.lastIndexOf("[") > 0) { - changePaths.add( - propertyPath.substr(0, propertyPath.lastIndexOf("[")), - ); + // If this is a property path change, simply add for evaluation and move on + if (d.path.length > 1) { + changePaths.add(convertPathToString(d.path)); + continue; + } + // A top level entity (widget/action) has been added or deleted + if (d.path.length === 1) { + const entityName = d.path[0]; + /** + * We want to add all pre-existing dynamic and static bindings in dynamic paths of this entity to get evaluated and validated. + * Example: + * - Table1.tableData = {{Api1.data}} + * - 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: diff --git a/app/client/src/workers/evaluationUtils.ts b/app/client/src/workers/evaluationUtils.ts index 6f05837929..e726ab886a 100644 --- a/app/client/src/workers/evaluationUtils.ts +++ b/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 | 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, +): Array => { + // Use a set to ensure that we dont have duplicates + const parents: Set = 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, ) { 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;