From eae2e46b9de4beae598e18d9539b6c24630576a3 Mon Sep 17 00:00:00 2001 From: Hetu Nandu Date: Wed, 23 Jun 2021 12:59:36 +0530 Subject: [PATCH] Fix cyclical dependency issue when Api returns an error (#5333) Co-authored-by: Trisha Anand --- app/client/src/workers/DataTreeEvaluator.ts | 326 ++++++++++---------- app/client/src/workers/evaluationUtils.ts | 58 +++- 2 files changed, 206 insertions(+), 178 deletions(-) diff --git a/app/client/src/workers/DataTreeEvaluator.ts b/app/client/src/workers/DataTreeEvaluator.ts index fbaec066a9..dea0c247e0 100644 --- a/app/client/src/workers/DataTreeEvaluator.ts +++ b/app/client/src/workers/DataTreeEvaluator.ts @@ -187,7 +187,6 @@ export default class DataTreeEvaluator { return false; }); this.logs.push({ - differences, evaluationOrder, sortedDependencies: this.sortedDependencies, inverse: this.inverseDependencyMap, @@ -750,195 +749,190 @@ export default 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); + const translatedDiffs = differences.map( + translateDiffEventToDataTreeDiffEvent, + ); + this.logs.push({ differences, translatedDiffs }); // Transform the diff library events to Appsmith evaluator events - differences - .map(translateDiffEventToDataTreeDiffEvent) - .forEach((dataTreeDiff) => { - const entityName = dataTreeDiff.payload.propertyPath.split(".")[0]; - let entity = unEvalDataTree[entityName]; - if (dataTreeDiff.event === DataTreeDiffEvent.DELETE) { - entity = this.oldUnEvalTree[entityName]; - } - const entityType = isValidEntity(entity) ? entity.ENTITY_TYPE : "noop"; + _.flatten(translatedDiffs).forEach((dataTreeDiff) => { + const entityName = dataTreeDiff.payload.propertyPath.split(".")[0]; + let entity = unEvalDataTree[entityName]; + if (dataTreeDiff.event === DataTreeDiffEvent.DELETE) { + entity = this.oldUnEvalTree[entityName]; + } + const entityType = isValidEntity(entity) ? entity.ENTITY_TYPE : "noop"; - if (entityType !== "noop") { - switch (dataTreeDiff.event) { - case DataTreeDiffEvent.NEW: { - // If a new entity/property was added, add all the internal bindings for this entity to the global dependency map - if ( - (isWidget(entity) || isAction(entity)) && - !this.isDynamicLeaf( - unEvalDataTree, - dataTreeDiff.payload.propertyPath, - ) - ) { - const entityDependencyMap: DependencyMap = this.listEntityDependencies( - entity, - entityName, - ); - if (Object.keys(entityDependencyMap).length) { - didUpdateDependencyMap = true; - // The entity might already have some dependencies, - // so we just want to update those - Object.entries(entityDependencyMap).forEach( - ([entityDependent, entityDependencies]) => { - if (this.dependencyMap[entityDependent]) { - this.dependencyMap[ - entityDependent - ] = this.dependencyMap[entityDependent].concat( - entityDependencies, - ); - } else { - this.dependencyMap[ - entityDependent - ] = entityDependencies; - } - }, - ); - } - } - // Either a new entity or a new property path has been added. Go through existing dynamic bindings and - // find out if a new dependency has to be created because the property path used in the binding just became - // eligible - const possibleReferencesInOldBindings: DependencyMap = this.getPropertyPathReferencesInExistingBindings( + if (entityType !== "noop") { + switch (dataTreeDiff.event) { + case DataTreeDiffEvent.NEW: { + // If a new entity/property was added, add all the internal bindings for this entity to the global dependency map + if ( + (isWidget(entity) || isAction(entity)) && + !this.isDynamicLeaf( unEvalDataTree, dataTreeDiff.payload.propertyPath, + ) + ) { + const entityDependencyMap: DependencyMap = this.listEntityDependencies( + entity, + entityName, ); - // We have found some bindings which are related to the new property path and hence should be added to the - // global dependency map - if (Object.keys(possibleReferencesInOldBindings).length) { + if (Object.keys(entityDependencyMap).length) { didUpdateDependencyMap = true; - Object.assign( - this.dependencyMap, - possibleReferencesInOldBindings, + // The entity might already have some dependencies, + // so we just want to update those + Object.entries(entityDependencyMap).forEach( + ([entityDependent, entityDependencies]) => { + if (this.dependencyMap[entityDependent]) { + this.dependencyMap[entityDependent] = this.dependencyMap[ + entityDependent + ].concat(entityDependencies); + } else { + this.dependencyMap[entityDependent] = entityDependencies; + } + }, ); } - break; } - case DataTreeDiffEvent.DELETE: { - // Add to removedPaths as they have been deleted from the evalTree - removedPaths.push(dataTreeDiff.payload.propertyPath); - // If an existing widget was deleted, remove all the bindings from the global dependency map - if ( - (isWidget(entity) || isAction(entity)) && - dataTreeDiff.payload.propertyPath === entityName - ) { - const entityDependencies = this.listEntityDependencies( - entity, - entityName, - ); - Object.keys(entityDependencies).forEach((widgetDep) => { - didUpdateDependencyMap = true; - delete this.dependencyMap[widgetDep]; - }); - } - // Either an existing entity or an existing property path has been deleted. Update the global dependency map - // by removing the bindings from the same. - Object.keys(this.dependencyMap).forEach((dependencyPath) => { + // Either a new entity or a new property path has been added. Go through existing dynamic bindings and + // find out if a new dependency has to be created because the property path used in the binding just became + // eligible + const possibleReferencesInOldBindings: DependencyMap = this.getPropertyPathReferencesInExistingBindings( + unEvalDataTree, + dataTreeDiff.payload.propertyPath, + ); + // We have found some bindings which are related to the new property path and hence should be added to the + // global dependency map + if (Object.keys(possibleReferencesInOldBindings).length) { + didUpdateDependencyMap = true; + Object.assign( + this.dependencyMap, + possibleReferencesInOldBindings, + ); + } + break; + } + case DataTreeDiffEvent.DELETE: { + debugger; + // Add to removedPaths as they have been deleted from the evalTree + removedPaths.push(dataTreeDiff.payload.propertyPath); + // If an existing widget was deleted, remove all the bindings from the global dependency map + if ( + (isWidget(entity) || isAction(entity)) && + dataTreeDiff.payload.propertyPath === entityName + ) { + const entityDependencies = this.listEntityDependencies( + entity, + entityName, + ); + Object.keys(entityDependencies).forEach((widgetDep) => { didUpdateDependencyMap = true; - if ( - isChildPropertyPath( - dataTreeDiff.payload.propertyPath, - dependencyPath, - ) - ) { - delete this.dependencyMap[dependencyPath]; - } else { - const toRemove: Array = []; - this.dependencyMap[dependencyPath].forEach( - (dependantPath) => { - if ( - isChildPropertyPath( - dataTreeDiff.payload.propertyPath, - dependantPath, - ) - ) { - dependenciesOfRemovedPaths.push(dependencyPath); - toRemove.push(dependantPath); - } - }, - ); - this.dependencyMap[dependencyPath] = _.difference( - this.dependencyMap[dependencyPath], - toRemove, - ); - } + delete this.dependencyMap[widgetDep]; }); - break; } - - case DataTreeDiffEvent.EDIT: { - // We only care if the difference is in dynamic bindings since static values do not need - // an evaluation. + // Either an existing entity or an existing property path has been deleted. Update the global dependency map + // by removing the bindings from the same. + Object.keys(this.dependencyMap).forEach((dependencyPath) => { + didUpdateDependencyMap = true; if ( - (isWidget(entity) || isAction(entity)) && - typeof dataTreeDiff.payload.value === "string" + isChildPropertyPath( + dataTreeDiff.payload.propertyPath, + dependencyPath, + ) ) { - const entity: DataTreeAction | DataTreeWidget = unEvalDataTree[ - entityName - ] as DataTreeAction | DataTreeWidget; - const fullPropertyPath = dataTreeDiff.payload.propertyPath; - const entityPropertyPath = fullPropertyPath.substring( - fullPropertyPath.indexOf(".") + 1, - ); - const isABindingPath = isPathADynamicBinding( - entity, - entityPropertyPath, - ); - if (isABindingPath) { - didUpdateDependencyMap = true; - - const { jsSnippets } = getDynamicBindings( - dataTreeDiff.payload.value, - ); - const correctSnippets = jsSnippets.filter( - (jsSnippet) => !!jsSnippet, - ); - // We found a new dynamic binding for this property path. We update the dependency map by overwriting the - // dependencies for this property path with the newly found dependencies - if (correctSnippets.length) { - this.dependencyMap[fullPropertyPath] = correctSnippets; - } else { - // The dependency on this property path has been removed. Delete this property path from the global - // dependency map - delete this.dependencyMap[fullPropertyPath]; + delete this.dependencyMap[dependencyPath]; + } else { + const toRemove: Array = []; + this.dependencyMap[dependencyPath].forEach((dependantPath) => { + if ( + isChildPropertyPath( + dataTreeDiff.payload.propertyPath, + dependantPath, + ) + ) { + dependenciesOfRemovedPaths.push(dependencyPath); + toRemove.push(dependantPath); } - if (isAction(entity)) { - // Actions have a defined dependency map that should always be maintained - if (entityPropertyPath in entity.dependencyMap) { - const entityDependenciesName = entity.dependencyMap[ - entityPropertyPath - ].map((dep) => `${entityName}.${dep}`); + }); + this.dependencyMap[dependencyPath] = _.difference( + this.dependencyMap[dependencyPath], + toRemove, + ); + } + }); + break; + } - // Filter only the paths which exist in the appsmith world to avoid cyclical dependencies - const filteredEntityDependencies = entityDependenciesName.filter( - (path) => this.allKeys.hasOwnProperty(path), - ); + case DataTreeDiffEvent.EDIT: { + // We only care if the difference is in dynamic bindings since static values do not need + // an evaluation. + if ( + (isWidget(entity) || isAction(entity)) && + typeof dataTreeDiff.payload.value === "string" + ) { + const entity: DataTreeAction | DataTreeWidget = unEvalDataTree[ + entityName + ] as DataTreeAction | DataTreeWidget; + const fullPropertyPath = dataTreeDiff.payload.propertyPath; + const entityPropertyPath = fullPropertyPath.substring( + fullPropertyPath.indexOf(".") + 1, + ); + const isABindingPath = isPathADynamicBinding( + entity, + entityPropertyPath, + ); + if (isABindingPath) { + didUpdateDependencyMap = true; - // Now assign these existing dependent paths to the property path in dependencyMap - if (fullPropertyPath in this.dependencyMap) { - this.dependencyMap[ - fullPropertyPath - ] = this.dependencyMap[fullPropertyPath].concat( - filteredEntityDependencies, - ); - } else { - this.dependencyMap[ - fullPropertyPath - ] = filteredEntityDependencies; - } + const { jsSnippets } = getDynamicBindings( + dataTreeDiff.payload.value, + ); + const correctSnippets = jsSnippets.filter( + (jsSnippet) => !!jsSnippet, + ); + // We found a new dynamic binding for this property path. We update the dependency map by overwriting the + // dependencies for this property path with the newly found dependencies + if (correctSnippets.length) { + this.dependencyMap[fullPropertyPath] = correctSnippets; + } else { + // The dependency on this property path has been removed. Delete this property path from the global + // dependency map + delete this.dependencyMap[fullPropertyPath]; + } + if (isAction(entity)) { + // Actions have a defined dependency map that should always be maintained + if (entityPropertyPath in entity.dependencyMap) { + const entityDependenciesName = entity.dependencyMap[ + entityPropertyPath + ].map((dep) => `${entityName}.${dep}`); + + // Filter only the paths which exist in the appsmith world to avoid cyclical dependencies + const filteredEntityDependencies = entityDependenciesName.filter( + (path) => this.allKeys.hasOwnProperty(path), + ); + + // Now assign these existing dependent paths to the property path in dependencyMap + if (fullPropertyPath in this.dependencyMap) { + this.dependencyMap[fullPropertyPath] = this.dependencyMap[ + fullPropertyPath + ].concat(filteredEntityDependencies); + } else { + this.dependencyMap[ + fullPropertyPath + ] = filteredEntityDependencies; } } } } - break; - } - default: { - break; } + break; + } + default: { + break; } } - }); + } + }); const diffCalcEnd = performance.now(); const subDepCalcStart = performance.now(); if (didUpdateDependencyMap) { diff --git a/app/client/src/workers/evaluationUtils.ts b/app/client/src/workers/evaluationUtils.ts index f813250c95..c6961956d2 100644 --- a/app/client/src/workers/evaluationUtils.ts +++ b/app/client/src/workers/evaluationUtils.ts @@ -88,8 +88,8 @@ export function getEntityNameAndPropertyPath( export const translateDiffEventToDataTreeDiffEvent = ( difference: Diff, -): DataTreeDiff => { - const result: DataTreeDiff = { +): DataTreeDiff | DataTreeDiff[] => { + let result: DataTreeDiff | DataTreeDiff[] = { payload: { propertyPath: "", value: "", @@ -126,25 +126,53 @@ export const translateDiffEventToDataTreeDiffEvent = ( propertyPath, value: difference.rhs, }; - } else { + } else if (difference.lhs === undefined || difference.rhs === undefined) { // Handle static value changes that change structure that can lead to // old bindings being eligible - if ( - difference.lhs === undefined && - typeof difference.rhs === "object" - ) { + if (difference.lhs === undefined && isTrueObject(difference.rhs)) { result.event = DataTreeDiffEvent.NEW; result.payload = { propertyPath }; } - if ( - difference.rhs === undefined && - typeof difference.lhs === "object" - ) { + if (difference.rhs === undefined && isTrueObject(difference.lhs)) { result.event = DataTreeDiffEvent.DELETE; result.payload = { propertyPath }; } - } + } else if ( + isTrueObject(difference.lhs) && + !isTrueObject(difference.rhs) + ) { + // This will happen for static value changes where a property went + // from being an object to any other type like string or number + // in such a case we want to delete all nested paths of the + // original lhs object + result = Object.keys(difference.lhs).map((diffKey) => { + const path = `${propertyPath}.${diffKey}`; + return { + event: DataTreeDiffEvent.DELETE, + payload: { + propertyPath: path, + }, + }; + }); + } else if ( + !isTrueObject(difference.lhs) && + isTrueObject(difference.rhs) + ) { + // This will happen for static value changes where a property went + // from being any other type like string or number to an object + // in such a case we want to add all nested paths of the + // new rhs object + result = Object.keys(difference.rhs).map((diffKey) => { + const path = `${propertyPath}.${diffKey}`; + return { + event: DataTreeDiffEvent.NEW, + payload: { + propertyPath: path, + }, + }; + }); + } break; } case "A": { @@ -546,3 +574,9 @@ export const addErrorToEntityProperty = ( } return dataTree; }; + +// For the times when you need to know if something truly an object like { a: 1, b: 2} +// typeof, lodash.isObject and others will return false positives for things like array, null, etc +export const isTrueObject = (item: unknown): boolean => { + return Object.prototype.toString.call(item) === "[object Object]"; +};