fix: Race condition in JS object mutation (#28083)

## Description
Fixes race condition in JS Object mutation where evaluation overrides
the variables state.

Root Cause:
Execution context is shared between every evaluation request and trigger
execution request. A trigger execution request could be paused when an
asynchronous task is awaited. In the mean time, worker might pick up the
task to perform and evaluation. Evaluation would end up replacing the
entities in the execution context, there by resetting the actions
performed by the trigger execution before it was paused.

What the fix does?
We are now caching the JS object for reuse which means that every
execution/evaluation request reuses the same JS Object as long the JS
Object isn't modified by a user action.

#### PR fixes following issue(s)
Fixes #27978 
>
>
#### Media
> A video or a GIF is preferred. when using Loom, don’t embed because it
looks like it’s a GIF. instead, just link to the video
>
>
#### Type of change
- Bug fix (non-breaking change which fixes an issue)
>
>
## Testing
>
#### How Has This Been Tested?
- [x] Manual
- [x] Jest
- [x] Cypress
>
>
#### Test Plan
> Add Testsmith test cases links that relate to this PR
>
>
#### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)
>
>
>
## Checklist:
#### Dev activity
- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag


#### QA activity:
- [x] [Speedbreak
features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-)
have been covered
- [x] Test plan covers all impacted features and [areas of
interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-)
- [ ] Test plan has been peer reviewed by project stakeholders and other
QA members
- [x] Manually tested functionality on DP
- [ ] We had an implementation alignment call with stakeholders post QA
Round 2
- [ ] Cypress test cases have been added and approved by SDET/manual QA
- [x] Added `Test Plan Approved` label after Cypress tests were reviewed
- [ ] Added `Test Plan Approved` label after JUnit tests were reviewed

app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObjectMutation_spec.ts

@@ -90,4 +90,52 @@ describe("JSObject testing", () => {
         expect($label).contains("[]");
       });
   });
+
+  it("6. Bug 27978 Check assignment should not get overridden by evaluation", () => {
+    _.entityExplorer.DragNDropWidget(_.draggableWidgets.TEXT, 400, 400);
+    _.propPane.TypeTextIntoField(
+      "Text",
+      `{{JSObject1.data.length ? 'id-' + JSObject1.data[0].id : 'Not Set' }}`,
+      true,
+      false,
+    );
+    _.entityExplorer.NavigateToSwitcher("Explorer");
+    _.apiPage.CreateAndFillApi(
+      _.dataManager.dsValues[_.dataManager.defaultEnviorment].mockApiUrl,
+    );
+    const JS_OBJECT_BODY = `export default {
+      data: [],
+      async getData() {
+        await Api1.run()
+        return Api1.data
+      },
+      async myFun1() {
+        this.data = await this.getData();
+        console.log(this.data);
+      },
+      async myFun2() {
+        const data = await this.getData();
+        data.push({ name: "test123" })
+        this.data = data;
+        console.log(this.data);
+      },
+    }`;
+    _.jsEditor.CreateJSObject(JS_OBJECT_BODY, {
+      paste: true,
+      completeReplace: true,
+      toRun: false,
+      shouldCreateNewJSObj: true,
+    });
+    _.jsEditor.SelectFunctionDropdown("myFun1");
+    _.jsEditor.RunJSObj();
+    _.entityExplorer.SelectEntityByName("Text2", "Widgets");
+    _.agHelper.AssertContains("id-1");
+    cy.reload();
+    _.agHelper.AssertContains("Not Set");
+    _.entityExplorer.SelectEntityByName("JSObject1", "Queries/JS");
+    _.jsEditor.SelectFunctionDropdown("myFun2");
+    _.jsEditor.RunJSObj();
+    _.entityExplorer.SelectEntityByName("Text2", "Widgets");
+    _.agHelper.AssertContains("id-1");
+  });
 });

app/client/src/ce/actions/evaluationActions.ts

@@ -66,8 +66,6 @@ export const EVALUATE_REDUX_ACTIONS = [
   ReduxActionTypes.MOVE_ACTION_SUCCESS,
   ReduxActionTypes.RUN_ACTION_SUCCESS,
   ReduxActionErrorTypes.RUN_ACTION_ERROR,
-  ReduxActionTypes.EXECUTE_PLUGIN_ACTION_SUCCESS,
-  ReduxActionErrorTypes.EXECUTE_PLUGIN_ACTION_ERROR,
   ReduxActionTypes.CLEAR_ACTION_RESPONSE,
   // JS Actions
   ReduxActionTypes.CREATE_JS_ACTION_SUCCESS,

app/client/src/workers/Evaluation/JSObject/Collection.ts

@@ -1,6 +1,20 @@
 import { getEntityNameAndPropertyPath } from "@appsmith/workers/Evaluation/evaluationUtils";
 import { klona } from "klona/full";
 import { get, set } from "lodash";
+import TriggerEmitter, { BatchKey } from "../fns/utils/TriggerEmitter";
+import ExecutionMetaData from "../fns/utils/ExecutionMetaData";
+import type { JSActionEntity } from "@appsmith/entities/DataTree/types";
+
+export enum PatchType {
+  "SET" = "SET",
+  "GET" = "GET",
+}
+
+export interface Patch {
+  path: string;
+  method: PatchType;
+  value?: unknown;
+}
 
 export type VariableState = Record<string, Record<string, any>>;
 
@@ -61,6 +75,7 @@ export default class JSObjectCollection {
     const newVarState = { ...this.variableState[entityName] };
     newVarState[propertyPath] = variableValue;
     this.variableState[entityName] = newVarState;
+    JSObjectCollection.clearCachedVariablesForEvaluationContext(entityName);
   }
 
   static getVariableState(
@@ -77,6 +92,53 @@ export default class JSObjectCollection {
       delete jsObject[propertyPath];
   }
 
+  /**Map<JSObjectName, Map<variableName, variableValue> */
+  static cachedJSVariablesByEntityName: Record<string, JSActionEntity> = {};
+
+  /**Computes  Map<JSObjectName, Map<variableName, variableValue> with getters & setters to track JS mutations
+   * We cache and reuse this map. We recreate only when the JSObject's content changes or when any of the variables
+   * gets evaluated
+   */
+  static getVariablesForEvaluationContext(entityName: string) {
+    if (JSObjectCollection.cachedJSVariablesByEntityName[entityName])
+      return JSObjectCollection.cachedJSVariablesByEntityName[entityName];
+    const varState = JSObjectCollection.getVariableState(entityName);
+    const variables = Object.entries(varState);
+    const newJSObject = {} as JSActionEntity;
+
+    for (const [varName, varValue] of variables) {
+      let variable = varValue;
+      Object.defineProperty(newJSObject, varName, {
+        enumerable: true,
+        configurable: true,
+        get() {
+          TriggerEmitter.emit(BatchKey.process_js_variable_updates, {
+            path: `${entityName}.${varName}`,
+            method: PatchType.GET,
+          });
+          return variable;
+        },
+        set(value) {
+          TriggerEmitter.emit(BatchKey.process_js_variable_updates, {
+            path: `${entityName}.${varName}`,
+            method: PatchType.SET,
+            value,
+          });
+          variable = value;
+        },
+      });
+    }
+    ExecutionMetaData.setExecutionMetaData({
+      enableJSVarUpdateTracking: true,
+    });
+    JSObjectCollection.cachedJSVariablesByEntityName[entityName] = newJSObject;
+    return JSObjectCollection.cachedJSVariablesByEntityName[entityName];
+  }
+
+  static clearCachedVariablesForEvaluationContext(entityName: string) {
+    delete JSObjectCollection.cachedJSVariablesByEntityName[entityName];
+  }
+
   static clear() {
     this.variableState = {};
     this.unEvalState = {};

app/client/src/workers/Evaluation/JSObject/JSVariableFactory.ts

@@ -1,52 +0,0 @@
-import { PatchType } from "./JSVariableUpdates";
-import ExecutionMetaData from "../fns/utils/ExecutionMetaData";
-import type { JSActionEntity } from "@appsmith/entities/DataTree/types";
-import TriggerEmitter, { BatchKey } from "../fns/utils/TriggerEmitter";
-
-class JSFactory {
-  static create(
-    jsObjectName: string,
-    varState: Record<string, unknown> = {},
-  ): JSActionEntity {
-    const newJSObject: any = {};
-
-    const variables = Object.entries(varState);
-
-    for (const [varName, varValue] of variables) {
-      let variable = varValue;
-      Object.defineProperty(newJSObject, varName, {
-        enumerable: true,
-        configurable: true,
-        get() {
-          TriggerEmitter.emit(BatchKey.process_js_variable_updates, {
-            path: `${jsObjectName}.${varName}`,
-            method: PatchType.GET,
-          });
-          return variable;
-        },
-        set(value) {
-          TriggerEmitter.emit(BatchKey.process_js_variable_updates, {
-            path: `${jsObjectName}.${varName}`,
-            method: PatchType.SET,
-            value,
-          });
-          variable = value;
-        },
-      });
-
-      ExecutionMetaData.setExecutionMetaData({
-        enableJSVarUpdateTracking: false,
-      });
-
-      newJSObject[varName] = varValue;
-
-      ExecutionMetaData.setExecutionMetaData({
-        enableJSVarUpdateTracking: true,
-      });
-    }
-
-    return newJSObject;
-  }
-}
-
-export default JSFactory;

app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts

@@ -5,17 +5,8 @@ import { evalTreeWithChanges } from "../evalTreeWithChanges";
 import { get } from "lodash";
 import { isJSObjectVariable } from "./utils";
 import isDeepEqualES6 from "fast-deep-equal/es6";
-
-export enum PatchType {
-  "SET" = "SET",
-  "GET" = "GET",
-}
-
-export interface Patch {
-  path: string;
-  method: PatchType;
-  value?: unknown;
-}
+import type { Patch } from "./Collection";
+import { PatchType } from "./Collection";
 
 export type UpdatedPathsMap = Record<string, Patch>;
 

app/client/src/workers/Evaluation/JSObject/__test__/JSVariableFactory.test.ts

@@ -1,9 +1,9 @@
-import JSFactory from "../JSVariableFactory";
 import ExecutionMetaData from "workers/Evaluation/fns/utils/ExecutionMetaData";
 import type { JSActionEntity } from "@appsmith/entities/DataTree/types";
 import TriggerEmitter, {
   jsVariableUpdatesHandlerWrapper,
 } from "workers/Evaluation/fns/utils/TriggerEmitter";
+import JSObjectCollection from "../Collection";
 
 const applyJSVariableUpdatesToEvalTreeMock = jest.fn();
 jest.mock("../JSVariableUpdates.ts", () => ({
@@ -36,7 +36,12 @@ describe("JSVariableFactory", () => {
       weakSet: new WeakSet(),
     } as unknown as JSActionEntity;
 
-    const proxiedJSObject = JSFactory.create("JSObject1", jsObject);
+    Object.entries(jsObject).forEach(([k, v]) =>
+      JSObjectCollection.setVariableValue(v, `JSObject1.${k}`),
+    );
+
+    const proxiedJSObject =
+      JSObjectCollection.getVariablesForEvaluationContext("JSObject1");
 
     ExecutionMetaData.setExecutionMetaData({
       enableJSVarUpdateTracking: true,
@@ -96,7 +101,12 @@ describe("JSVariableFactory", () => {
       weakSet: new WeakSet(),
     } as unknown as JSActionEntity;
 
-    const proxiedJSObject = JSFactory.create("JSObject1", jsObject);
+    Object.entries(jsObject).forEach(([k, v]) =>
+      JSObjectCollection.setVariableValue(v, `JSObject1.${k}`),
+    );
+
+    const proxiedJSObject =
+      JSObjectCollection.getVariablesForEvaluationContext("JSObject1");
 
     ExecutionMetaData.setExecutionMetaData({
       enableJSVarUpdateTracking: false,
@@ -125,7 +135,12 @@ describe("JSVariableFactory", () => {
       weakSet: new WeakSet(),
     } as unknown as JSActionEntity;
 
-    const proxiedJSObject = JSFactory.create("JSObject1", jsObject);
+    Object.entries(jsObject).forEach(([k, v]) =>
+      JSObjectCollection.setVariableValue(v, `JSObject1.${k}`),
+    );
+
+    const proxiedJSObject =
+      JSObjectCollection.getVariablesForEvaluationContext("JSObject1");
 
     ExecutionMetaData.setExecutionMetaData({
       enableJSVarUpdateTracking: true,

app/client/src/workers/Evaluation/JSObject/index.ts

@@ -92,6 +92,7 @@ export function saveResolvedFunctionsAndJSUpdates(
     try {
       JSObjectCollection.deleteResolvedFunction(entityName);
       JSObjectCollection.deleteUnEvalState(entityName);
+      JSObjectCollection.clearCachedVariablesForEvaluationContext(entityName);
 
       const parseStartTime = performance.now();
       const { parsedObject, success } = parseJSObject(entity.body);

app/client/src/workers/Evaluation/fns/utils/TriggerEmitter.ts

@@ -1,10 +1,7 @@
 import { EventEmitter } from "events";
 import { MAIN_THREAD_ACTION } from "@appsmith/workers/Evaluation/evalWorkerActions";
 import { WorkerMessenger } from "workers/Evaluation/fns/utils/Messenger";
-import type {
-  Patch,
-  UpdatedPathsMap,
-} from "workers/Evaluation/JSObject/JSVariableUpdates";
+import type { UpdatedPathsMap } from "workers/Evaluation/JSObject/JSVariableUpdates";
 import { applyJSVariableUpdatesToEvalTree } from "workers/Evaluation/JSObject/JSVariableUpdates";
 import ExecutionMetaData from "./ExecutionMetaData";
 import { get } from "lodash";
@@ -18,6 +15,7 @@ import type {
 import type { UpdateActionProps } from "workers/Evaluation/handlers/updateActionData";
 import { handleActionsDataUpdate } from "workers/Evaluation/handlers/updateActionData";
 import { getEntityNameAndPropertyPath } from "@appsmith/workers/Evaluation/evaluationUtils";
+import type { Patch } from "workers/Evaluation/JSObject/Collection";
 
 const _internalSetTimeout = self.setTimeout;
 const _internalClearTimeout = self.clearTimeout;

app/client/src/workers/Evaluation/getEntityForContext.ts

@@ -2,7 +2,6 @@ import type { JSActionEntity } from "@appsmith/entities/DataTree/types";
 import type { DataTreeEntity } from "entities/DataTree/dataTreeTypes";
 import { ENTITY_TYPE_VALUE } from "entities/DataTree/dataTreeFactory";
 import JSObjectCollection from "./JSObject/Collection";
-import JSFactory from "./JSObject/JSVariableFactory";
 import { jsObjectFunctionFactory } from "./fns/utils/jsObjectFnFactory";
 import { isObject } from "lodash";
 
@@ -53,7 +52,8 @@ export function getEntityForEvalContext(
           return Object.assign({}, jsObject, fns);
         }
 
-        jsObjectForEval = JSFactory.create(entityName, jsObjectForEval);
+        jsObjectForEval =
+          JSObjectCollection.getVariablesForEvaluationContext(entityName);
         return Object.assign(jsObjectForEval, fns);
       }
     }