fix: Updating the function definition for checking reactive cyclic dependencies (#41049)
## Description Updating the function definition for checking reactive cyclic dependencies to throw an error in all such scenarios and block multiple execute API calls. Fixes [#41048](https://github.com/appsmithorg/appsmith/issues/41048) ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/15919295485> > Commit: 05c9f6481e464dc802b2b556a0eba48c66cc3030 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=15919295485&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Fri, 27 Jun 2025 07:06:10 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved the logic for detecting reactive dependency misuse, resulting in clearer and more efficient checks. * Updated type handling to support partial entities in action detection. * **New Features** * Enhanced detection of conflicting trigger and data paths within dependencies, providing immediate feedback when misuse is found. * Introduced a feature flag to control reactive dependency misuse detection. * **Tests** * Added comprehensive tests to validate detection of reactive dependency misuse in various direct and transitive dependency scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Ankita Kinger
GitHub
parent
bb96d0f616
commit
791476290f
|
|
@ -413,7 +413,9 @@ export function isAppsmithEntity(
|
|||
);
|
||||
}
|
||||
|
||||
export function isJSAction(entity: DataTreeEntity): entity is JSActionEntity {
|
||||
export function isJSAction(
|
||||
entity: Partial<DataTreeEntity>,
|
||||
): entity is JSActionEntity {
|
||||
return (
|
||||
typeof entity === "object" &&
|
||||
"ENTITY_TYPE" in entity &&
|
||||
|
|
|
|||
|
|
@ -4,9 +4,13 @@ import {
|
|||
entityTypeCheckForPathDynamicTrigger,
|
||||
getEntityNameAndPropertyPath,
|
||||
IMMEDIATE_PARENT_REGEX,
|
||||
isAction,
|
||||
isJSAction,
|
||||
} from "ee/workers/Evaluation/evaluationUtils";
|
||||
import type { ConfigTree } from "entities/DataTree/dataTreeTypes";
|
||||
import { isPathDynamicTrigger } from "utils/DynamicBindingUtils";
|
||||
import { WorkerEnv } from "workers/Evaluation/handlers/workerEnv";
|
||||
import { ActionRunBehaviour } from "PluginActionEditor/types/PluginActionTypes";
|
||||
|
||||
type SortDependencies =
|
||||
| {
|
||||
|
|
@ -23,6 +27,9 @@ export class DependencyMapUtils {
|
|||
): SortDependencies {
|
||||
const dependencyTree: Array<[string, string | undefined]> = [];
|
||||
const dependencies = dependencyMap.rawDependencies;
|
||||
const featureFlags = WorkerEnv.getFeatureFlags();
|
||||
const isReactiveActionsEnabled =
|
||||
featureFlags.release_reactive_actions_enabled;
|
||||
|
||||
for (const [node, deps] of dependencies.entries()) {
|
||||
if (deps.size) {
|
||||
|
|
@ -38,7 +45,7 @@ export class DependencyMapUtils {
|
|||
.reverse()
|
||||
.filter((edge) => !!edge);
|
||||
|
||||
if (configTree) {
|
||||
if (configTree && isReactiveActionsEnabled) {
|
||||
this.detectReactiveDependencyMisuse(dependencyMap, configTree);
|
||||
}
|
||||
|
||||
|
|
@ -163,81 +170,91 @@ export class DependencyMapUtils {
|
|||
) {
|
||||
const dependencies = dependencyMap.rawDependencies;
|
||||
|
||||
// Helper function to get all transitive dependencies
|
||||
const getAllTransitiveDependencies = (node: string): Set<string> => {
|
||||
const allDeps = new Set<string>();
|
||||
const queue = [node];
|
||||
for (const node of dependencies.keys()) {
|
||||
const { entityName: nodeName } = getEntityNameAndPropertyPath(node);
|
||||
const nodeConfig = configTree[nodeName];
|
||||
|
||||
while (queue.length > 0) {
|
||||
const current = queue.shift()!;
|
||||
const deps = dependencyMap.getDirectDependencies(current) || [];
|
||||
const isJSActionEntity = isJSAction(nodeConfig);
|
||||
const isActionEntity = isAction(nodeConfig);
|
||||
|
||||
for (const dep of deps) {
|
||||
if (!allDeps.has(dep)) {
|
||||
allDeps.add(dep);
|
||||
queue.push(dep);
|
||||
}
|
||||
}
|
||||
if (isJSActionEntity) {
|
||||
// Only continue if at least one function is automatic
|
||||
const hasAutomaticFunc = Object.values(nodeConfig.meta).some(
|
||||
(jsFunction) =>
|
||||
jsFunction.runBehaviour === ActionRunBehaviour.AUTOMATIC,
|
||||
);
|
||||
|
||||
if (!hasAutomaticFunc) continue;
|
||||
} else if (isActionEntity) {
|
||||
// Only continue if runBehaviour is AUTOMATIC
|
||||
if (nodeConfig.runBehaviour !== ActionRunBehaviour.AUTOMATIC) continue;
|
||||
} else {
|
||||
// If not a JSAction, or Action, skip
|
||||
continue;
|
||||
}
|
||||
|
||||
return allDeps;
|
||||
};
|
||||
// For each entity, check if both .run and a .data path are present
|
||||
let hasRun = false;
|
||||
let hasData = false;
|
||||
let dataPath = "";
|
||||
let runPath = "";
|
||||
|
||||
for (const [node, deps] of dependencies.entries()) {
|
||||
// Get all dependencies including transitive ones
|
||||
const allDeps = new Set<string>();
|
||||
const queue = Array.from(deps);
|
||||
|
||||
while (queue.length > 0) {
|
||||
const dep = queue.shift()!;
|
||||
|
||||
if (!allDeps.has(dep)) {
|
||||
allDeps.add(dep);
|
||||
const depDeps = dependencyMap.getDirectDependencies(dep) || [];
|
||||
|
||||
queue.push(...depDeps);
|
||||
}
|
||||
}
|
||||
|
||||
// Separate dependencies into trigger paths and data paths
|
||||
const triggerPaths = Array.from(deps).filter((dep) =>
|
||||
this.isTriggerPath(dep, configTree),
|
||||
const transitiveDeps = this.getAllTransitiveDependencies(
|
||||
dependencyMap,
|
||||
node,
|
||||
);
|
||||
const dataPaths = Array.from(deps).filter((dep) => this.isDataPath(dep));
|
||||
|
||||
// For each trigger path, check if there's a data path from the same entity
|
||||
for (const triggerPath of triggerPaths) {
|
||||
const triggerEntity = triggerPath.split(".")[0];
|
||||
for (const dep of transitiveDeps) {
|
||||
const { entityName } = getEntityNameAndPropertyPath(dep);
|
||||
const entityConfig = configTree[entityName];
|
||||
|
||||
// Find data paths from the same entity
|
||||
const sameEntityDataPaths = dataPaths.filter((dataPath) => {
|
||||
const dataEntity = dataPath.split(".")[0];
|
||||
if (entityConfig && entityConfig.ENTITY_TYPE === "ACTION") {
|
||||
if (this.isTriggerPath(dep, configTree)) {
|
||||
hasRun = true;
|
||||
runPath = dep;
|
||||
}
|
||||
|
||||
return dataEntity === triggerEntity;
|
||||
});
|
||||
if (DependencyMapUtils.isDataPath(dep)) {
|
||||
hasData = true;
|
||||
dataPath = dep;
|
||||
}
|
||||
|
||||
if (sameEntityDataPaths.length > 0) {
|
||||
// Check if any of these data paths depend on the trigger path (directly or indirectly)
|
||||
for (const dataPath of sameEntityDataPaths) {
|
||||
const dataPathTransitiveDeps =
|
||||
getAllTransitiveDependencies(dataPath);
|
||||
|
||||
if (dataPathTransitiveDeps.has(triggerPath)) {
|
||||
const error = new Error(
|
||||
`Reactive dependency misuse: '${node}' depends on both trigger path '${triggerPath}' and data path '${dataPath}' from the same entity, and '${dataPath}' depends on '${triggerPath}' (directly or indirectly). This can cause unexpected reactivity.`,
|
||||
);
|
||||
|
||||
// Add custom properties
|
||||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
|
||||
// @ts-ignore
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
(error as any).node = node;
|
||||
|
||||
throw error;
|
||||
}
|
||||
if (hasRun && hasData) {
|
||||
throw Object.assign(
|
||||
new Error(
|
||||
`Reactive dependency misuse: '${node}' depends on both trigger path '${runPath}' and data path '${dataPath}' from the same entity. This can cause unexpected reactivity.`,
|
||||
),
|
||||
{ node, triggerPath: runPath, dataPath },
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns all transitive dependencies (direct and indirect, no duplicates) for a given node.
|
||||
*/
|
||||
static getAllTransitiveDependencies(
|
||||
dependencyMap: DependencyMap,
|
||||
node: string,
|
||||
): string[] {
|
||||
const dependencies = dependencyMap.rawDependencies;
|
||||
const visited = new Set<string>();
|
||||
|
||||
function traverse(current: string) {
|
||||
const directDeps = dependencies.get(current) || new Set<string>();
|
||||
|
||||
for (const dep of directDeps) {
|
||||
if (!visited.has(dep)) {
|
||||
visited.add(dep);
|
||||
traverse(dep);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
traverse(node);
|
||||
|
||||
return Array.from(visited);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,337 @@
|
|||
import DependencyMap from "../index";
|
||||
import { DependencyMapUtils } from "../DependencyMapUtils";
|
||||
import { PluginType } from "entities/Plugin";
|
||||
import {
|
||||
ENTITY_TYPE,
|
||||
type DataTreeEntityConfig,
|
||||
type MetaArgs,
|
||||
} from "ee/entities/DataTree/types";
|
||||
import type { ActionRunBehaviourType } from "PluginActionEditor/types/PluginActionTypes";
|
||||
|
||||
describe("detectReactiveDependencyMisuse", () => {
|
||||
function makeConfigTreeWithAction(
|
||||
entityName: string,
|
||||
runBehaviour: ActionRunBehaviourType = "AUTOMATIC",
|
||||
): DataTreeEntityConfig {
|
||||
return {
|
||||
ENTITY_TYPE: ENTITY_TYPE.ACTION,
|
||||
dynamicTriggerPathList: [{ key: "run" }],
|
||||
dynamicBindingPathList: [],
|
||||
bindingPaths: {},
|
||||
reactivePaths: {},
|
||||
dependencyMap: {},
|
||||
logBlackList: {},
|
||||
pluginType: PluginType.API,
|
||||
pluginId: "mockPluginId",
|
||||
actionId: "mockActionId",
|
||||
name: entityName,
|
||||
runBehaviour,
|
||||
};
|
||||
}
|
||||
|
||||
function makeConfigTreeWithJSAction(
|
||||
entityName: string,
|
||||
meta: Record<string, MetaArgs> = {},
|
||||
): DataTreeEntityConfig {
|
||||
return {
|
||||
ENTITY_TYPE: ENTITY_TYPE.JSACTION,
|
||||
meta,
|
||||
dynamicBindingPathList: [],
|
||||
actionNames: new Set(["myFun1", "myFun2"]),
|
||||
bindingPaths: {},
|
||||
reactivePaths: {},
|
||||
dependencyMap: {},
|
||||
pluginType: PluginType.JS,
|
||||
name: entityName,
|
||||
actionId: "mockJSActionId",
|
||||
dynamicTriggerPathList: [],
|
||||
variables: [],
|
||||
};
|
||||
}
|
||||
|
||||
function makeConfigTreeWithWidget(entityName: string): DataTreeEntityConfig {
|
||||
return {
|
||||
ENTITY_TYPE: ENTITY_TYPE.WIDGET,
|
||||
bindingPaths: {},
|
||||
reactivePaths: {},
|
||||
triggerPaths: {},
|
||||
validationPaths: {},
|
||||
logBlackList: {},
|
||||
propertyOverrideDependency: {},
|
||||
overridingPropertyPaths: {},
|
||||
privateWidgets: {},
|
||||
widgetId: "mockWidgetId",
|
||||
defaultMetaProps: [],
|
||||
type: "MOCK_WIDGET_TYPE",
|
||||
dynamicBindingPathList: [],
|
||||
name: entityName,
|
||||
};
|
||||
}
|
||||
|
||||
it("does not throw for Widget entity", () => {
|
||||
const dependencyMap = new DependencyMap();
|
||||
|
||||
dependencyMap.addNodes({
|
||||
Widget1: true,
|
||||
"Widget1.run": true,
|
||||
"Widget1.data": true,
|
||||
});
|
||||
dependencyMap.addDependency("Widget1", ["Widget1.run", "Widget1.data"]);
|
||||
const configTree = {
|
||||
Widget1: makeConfigTreeWithWidget("Widget1"),
|
||||
};
|
||||
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(
|
||||
dependencyMap,
|
||||
configTree,
|
||||
);
|
||||
}).not.toThrow();
|
||||
});
|
||||
|
||||
it("does not throw for MANUAL ACTION entity", () => {
|
||||
const dependencyMap = new DependencyMap();
|
||||
|
||||
dependencyMap.addNodes({
|
||||
"JSObject1.myFun1": true,
|
||||
"Query3.run": true,
|
||||
"Query3.data": true,
|
||||
});
|
||||
dependencyMap.addDependency("JSObject1.myFun1", [
|
||||
"Query3.run",
|
||||
"Query3.data",
|
||||
]);
|
||||
const configTree = {
|
||||
Query3: makeConfigTreeWithAction("Query3", "MANUAL"),
|
||||
JSObject1: makeConfigTreeWithJSAction("JSObject1", {
|
||||
myFun1: {
|
||||
runBehaviour: "MANUAL",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
myFun2: {
|
||||
runBehaviour: "MANUAL",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
}),
|
||||
};
|
||||
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(
|
||||
dependencyMap,
|
||||
configTree,
|
||||
);
|
||||
}).not.toThrow();
|
||||
});
|
||||
|
||||
it("does not throw for JSAction entity with no AUTOMATIC function", () => {
|
||||
const dependencyMap = new DependencyMap();
|
||||
|
||||
dependencyMap.addNodes({
|
||||
"JSObject1.myFun1": true,
|
||||
"JSObject1.myFun2": true,
|
||||
"Query2.run": true,
|
||||
"Query2.data": true,
|
||||
});
|
||||
// JSObject1.myFun2 depends on Query2.run
|
||||
dependencyMap.addDependency("JSObject1.myFun2", ["Query2.run"]);
|
||||
// JSObject1.myFun1 depends on both JSObject1.myFun2 and and Query2.data (transitive)
|
||||
dependencyMap.addDependency("JSObject1.myFun1", [
|
||||
"JSObject1.myFun2",
|
||||
"Query2.data",
|
||||
]);
|
||||
|
||||
// meta has no AUTOMATIC runBehaviour
|
||||
const configTree = {
|
||||
JSObject1: makeConfigTreeWithJSAction("JSObject1", {
|
||||
myFun1: {
|
||||
runBehaviour: "MANUAL",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
myFun2: {
|
||||
runBehaviour: "MANUAL",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
}),
|
||||
Query2: makeConfigTreeWithAction("Query2", "AUTOMATIC"),
|
||||
};
|
||||
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(
|
||||
dependencyMap,
|
||||
configTree,
|
||||
);
|
||||
}).not.toThrow();
|
||||
});
|
||||
|
||||
it("does not throw if a node depends only on .run or only on .data for AUTOMATIC ACTION", () => {
|
||||
const configTree = {
|
||||
Api1: makeConfigTreeWithAction("Api1", "AUTOMATIC"),
|
||||
JSObject1: makeConfigTreeWithJSAction("JSObject1", {
|
||||
myFun1: {
|
||||
runBehaviour: "AUTOMATIC",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
myFun2: {
|
||||
runBehaviour: "AUTOMATIC",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
}),
|
||||
JSObject2: makeConfigTreeWithJSAction("JSObject2", {
|
||||
myFun1: {
|
||||
runBehaviour: "AUTOMATIC",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
myFun2: {
|
||||
runBehaviour: "AUTOMATIC",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
}),
|
||||
};
|
||||
|
||||
// Only .run
|
||||
const depMapRun = new DependencyMap();
|
||||
|
||||
depMapRun.addNodes({ "JSObject1.myFun1": true, "Api1.run": true });
|
||||
depMapRun.addDependency("JSObject1.myFun1", ["Api1.run"]);
|
||||
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(depMapRun, configTree);
|
||||
}).not.toThrow();
|
||||
|
||||
// Only .data
|
||||
const depMapData = new DependencyMap();
|
||||
|
||||
depMapData.addNodes({ "JSObject2.myFun1": true, "Api1.data": true });
|
||||
depMapData.addDependency("JSObject2.myFun1", ["Api1.data"]);
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(depMapData, configTree);
|
||||
}).not.toThrow();
|
||||
});
|
||||
|
||||
it("throws if a node depends on both .run and .data of the same AUTOMATIC ACTION entity", () => {
|
||||
const dependencyMap = new DependencyMap();
|
||||
|
||||
// Add nodes
|
||||
dependencyMap.addNodes({
|
||||
"JSObject1.myFun1": true,
|
||||
"Query1.run": true,
|
||||
"Query1.data": true,
|
||||
});
|
||||
// JSObject1.myFun1 depends on both Query1.run and Query1.data
|
||||
dependencyMap.addDependency("JSObject1.myFun1", [
|
||||
"Query1.run",
|
||||
"Query1.data",
|
||||
]);
|
||||
const configTree = {
|
||||
JSObject1: makeConfigTreeWithJSAction("JSObject1", {
|
||||
myFun1: {
|
||||
runBehaviour: "AUTOMATIC",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
myFun2: {
|
||||
runBehaviour: "MANUAL",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
}),
|
||||
Query1: makeConfigTreeWithAction("Query1", "AUTOMATIC"),
|
||||
};
|
||||
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(
|
||||
dependencyMap,
|
||||
configTree,
|
||||
);
|
||||
}).toThrow(/Reactive dependency misuse/);
|
||||
});
|
||||
|
||||
it("throws if a node depends on both .run and .data of the same AUTOMATIC ACTION entity via transitive dependency", () => {
|
||||
const dependencyMap = new DependencyMap();
|
||||
|
||||
dependencyMap.addNodes({
|
||||
"JSObject1.myFun1": true,
|
||||
"JSObject1.myFun2": true,
|
||||
"Query2.run": true,
|
||||
"Query2.data": true,
|
||||
});
|
||||
// JSObject1.myFun2 depends on Query2.run
|
||||
dependencyMap.addDependency("JSObject1.myFun2", ["Query2.run"]);
|
||||
// JSObject1.myFun1 depends on both JSObject1.myFun2 and and Query2.data (transitive)
|
||||
dependencyMap.addDependency("JSObject1.myFun1", [
|
||||
"JSObject1.myFun2",
|
||||
"Query2.data",
|
||||
]);
|
||||
const configTree = {
|
||||
Query2: makeConfigTreeWithAction("Query2", "AUTOMATIC"),
|
||||
JSObject1: makeConfigTreeWithJSAction("JSObject1", {
|
||||
myFun1: {
|
||||
runBehaviour: "AUTOMATIC",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
myFun2: {
|
||||
runBehaviour: "MANUAL",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
}),
|
||||
};
|
||||
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(
|
||||
dependencyMap,
|
||||
configTree,
|
||||
);
|
||||
}).toThrow(/Reactive dependency misuse/);
|
||||
});
|
||||
|
||||
it("throws for JSAction entity with at least one AUTOMATIC function", () => {
|
||||
// meta has one AUTOMATIC runBehaviour
|
||||
const dependencyMap = new DependencyMap();
|
||||
|
||||
dependencyMap.addNodes({
|
||||
"JSObject1.myFun1": true,
|
||||
"JSObject1.myFun2": true,
|
||||
"Query2.run": true,
|
||||
"Query2.data": true,
|
||||
});
|
||||
// JSObject1.myFun2 depends on Query2.run
|
||||
dependencyMap.addDependency("JSObject1.myFun2", ["Query2.run"]);
|
||||
// JSObject1.myFun1 depends on both JSObject1.myFun2 and and Query2.data (transitive)
|
||||
dependencyMap.addDependency("JSObject1.myFun1", [
|
||||
"JSObject1.myFun2",
|
||||
"Query2.data",
|
||||
]);
|
||||
const configTree = {
|
||||
JSObject1: makeConfigTreeWithJSAction("JSObject1", {
|
||||
myFun1: {
|
||||
runBehaviour: "AUTOMATIC",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
myFun2: {
|
||||
runBehaviour: "MANUAL",
|
||||
arguments: [],
|
||||
confirmBeforeExecute: false,
|
||||
},
|
||||
}),
|
||||
Query2: makeConfigTreeWithAction("Query2", "MANUAL"),
|
||||
};
|
||||
|
||||
expect(() => {
|
||||
DependencyMapUtils.detectReactiveDependencyMisuse(
|
||||
dependencyMap,
|
||||
configTree,
|
||||
);
|
||||
}).toThrow(/Reactive dependency misuse/);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user