fix: Added lint error for appsmith store mutations (#33484)

Co-authored-by: “sneha122” <“sneha@appsmith.com”>

app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts

@@ -1,54 +0,0 @@
-import {
-  PROPERTY_SELECTOR,
-  WIDGET,
-  getWidgetSelector,
-} from "../../../../locators/WidgetLocators";
-
-import {
-  entityExplorer,
-  jsEditor,
-  agHelper,
-  locators,
-  propPane,
-} from "../../../../support/Objects/ObjectsCore";
-
-describe(
-  "Linting warning for imperative store update",
-  { tags: ["@tag.JS"] },
-  function () {
-    it("Shows lint error for imperative store update", function () {
-      entityExplorer.DragDropWidgetNVerify(WIDGET.BUTTON, 200, 200);
-      entityExplorer.DragDropWidgetNVerify(WIDGET.TEXT, 400, 400);
-
-      agHelper.GetNClick(getWidgetSelector(WIDGET.BUTTON));
-      propPane.TypeTextIntoField("Label", "{{appsmith.store.name = 6}}");
-
-      //Mouse hover to exact warning message
-      agHelper.AssertElementVisibility(locators._lintErrorElement);
-      agHelper.HoverElement(locators._lintErrorElement);
-      agHelper.AssertContains("Use storeValue() method to modify the store");
-      agHelper.Sleep();
-
-      //Create a JS object
-      jsEditor.CreateJSObject(
-        `export default {
-          myFun1: () => {
-            appsmith.store.name.text = 6
-          },
-        }`,
-        {
-          paste: true,
-          completeReplace: true,
-          toRun: false,
-          shouldCreateNewJSObj: true,
-          prettify: false,
-        },
-      );
-
-      agHelper.AssertElementVisibility(locators._lintErrorElement);
-      agHelper.HoverElement(locators._lintErrorElement);
-      agHelper.AssertContains("Use storeValue() method to modify the store");
-      agHelper.Sleep();
-    });
-  },
-);

app/client/packages/ast/index.ts

@@ -5,6 +5,7 @@ import type {
   IdentifierInfo,
   AssignmentExpressionData,
   CallExpressionData,
+  MemberCallExpressionData,
 } from "./src";
 import {
   isIdentifierNode,
@@ -85,6 +86,7 @@ export type {
   JSVarProperty,
   JSFunctionProperty,
   CallExpressionData,
+  MemberCallExpressionData,
 };
 
 export {

app/client/packages/ast/src/index.ts

@@ -597,6 +597,16 @@ export interface CallExpressionData {
   params: NodeWithLocation<MemberExpressionNode | LiteralNode>[];
 }
 
+// This interface is used for storing call expression nodes with callee as member node
+// example of such case is when a function is called on object like obj.func()
+// This is required to understand whether appsmith.store.test.func() is present in script
+// in order to display mutation error on such statements.
+export interface MemberCallExpressionData {
+  property: NodeWithLocation<MemberExpressionNode | LiteralNode>;
+  object: NodeWithLocation<MemberExpressionNode>;
+  parentNode: NodeWithLocation<CallExpressionNode>;
+}
+
 export interface AssignmentExpressionNode extends Node {
   operator: string;
   left: Expression;
@@ -628,9 +638,11 @@ export const extractExpressionsFromCode = (
   invalidTopLevelMemberExpressionsArray: MemberExpressionData[];
   assignmentExpressionsData: AssignmentExpressionData[];
   callExpressionsData: CallExpressionData[];
+  memberCallExpressionData: MemberCallExpressionData[];
 } => {
   const assignmentExpressionsData = new Set<AssignmentExpressionData>();
   const callExpressionsData = new Set<CallExpressionData>();
+  const memberCallExpressionData = new Set<MemberCallExpressionData>();
   const invalidTopLevelMemberExpressions = new Set<MemberExpressionData>();
   const variableDeclarations = new Set<string>();
   let functionalParams = new Set<string>();
@@ -646,6 +658,7 @@ export const extractExpressionsFromCode = (
         invalidTopLevelMemberExpressionsArray: [],
         assignmentExpressionsData: [],
         callExpressionsData: [],
+        memberCallExpressionData: [],
       };
     }
     throw e;
@@ -726,12 +739,24 @@ export const extractExpressionsFromCode = (
       } as AssignmentExpressionData);
     },
     CallExpression(node: Node) {
-      if (isCallExpressionNode(node) && isIdentifierNode(node.callee)) {
+      if (isCallExpressionNode(node)) {
+        if (isIdentifierNode(node.callee)) {
           callExpressionsData.add({
             property: node.callee,
             params: node.arguments,
           } as CallExpressionData);
         }
+
+        if (isMemberExpressionNode(node.callee)) {
+          const { object, property } = node.callee;
+
+          memberCallExpressionData.add({
+            object,
+            property,
+            parentNode: node,
+          } as MemberCallExpressionData);
+        }
+      }
     },
   });
 
@@ -748,6 +773,7 @@ export const extractExpressionsFromCode = (
     invalidTopLevelMemberExpressionsArray,
     assignmentExpressionsData: [...assignmentExpressionsData],
     callExpressionsData: [...callExpressionsData],
+    memberCallExpressionData: [...memberCallExpressionData],
   };
 };
 

app/client/src/plugins/Linting/tests/getLintingErrors.test.ts

@@ -1,4 +1,5 @@
 import { getScriptType } from "workers/Evaluation/evaluate";
+import { CustomLintErrorCode } from "../constants";
 import getLintingErrors from "../utils/getLintingErrors";
 
 describe("getLintingErrors", () => {
@@ -102,4 +103,81 @@ describe("getLintingErrors", () => {
       expect(lintErrors.length).toEqual(1);
     });
   });
+
+  describe("3. Verify lint errors are shown when mutations are performed on unmutable objects", () => {
+    const data = {
+      THIS_CONTEXT: {},
+      Input1: {
+        text: "inputValue",
+        ENTITY_TYPE: "WIDGET",
+      },
+      appsmith: {
+        store: {
+          test: "testValue",
+        },
+      },
+    };
+    it("1. Assigning values to input widget's properties", () => {
+      const originalBinding = "'myFun1() {\n\t\tInput1.text = \"\";\n\t}'";
+      const script =
+        '\n  function $$closedFn () {\n    const $$result = {myFun1() {\n\t\tInput1.text = "";\n\t}}\n    return $$result\n  }\n  $$closedFn.call(THIS_CONTEXT)\n  ';
+      const options = { isJsObject: true };
+
+      const scriptType = getScriptType(false, false);
+
+      const lintErrors = getLintingErrors({
+        data,
+        options,
+        originalBinding,
+        script,
+        scriptType,
+      });
+      expect(lintErrors.length).toEqual(1);
+      expect(lintErrors[0].code).toEqual(
+        CustomLintErrorCode.INVALID_ENTITY_PROPERTY,
+      );
+    });
+
+    it("2. Assigning values to appsmith store variables", () => {
+      const originalBinding = 'myFun1() {\n\t\tappsmith.store.test = "";\n\t}';
+      const script =
+        '\n  function $$closedFn () {\n    const $$result = {myFun1() {\n\t\tappsmith.store.test = "";\n\t}}\n    return $$result\n  }\n  $$closedFn.call(THIS_CONTEXT)\n';
+      const options = { isJsObject: true };
+
+      const scriptType = getScriptType(false, false);
+
+      const lintErrors = getLintingErrors({
+        data,
+        options,
+        originalBinding,
+        script,
+        scriptType,
+      });
+      expect(lintErrors.length).toEqual(1);
+      expect(lintErrors[0].code).toEqual(
+        CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER,
+      );
+    });
+    it("3. Mutating appsmith store values by calling any functions on it", () => {
+      const originalBinding =
+        "myFun1() {\n\t\tappsmith.store.test.push(1);\n\t}";
+      const script =
+        "\n  function $$closedFn () {\n    const $$result = {myFun1() {\n\t\tappsmith.store.test.push(1);\n\t}}\n    return $$result\n  }\n  $$closedFn.call(THIS_CONTEXT)\n";
+      const options = { isJsObject: true };
+
+      const scriptType = getScriptType(false, false);
+
+      const lintErrors = getLintingErrors({
+        data,
+        options,
+        originalBinding,
+        script,
+        scriptType,
+      });
+      expect(lintErrors.length).toEqual(1);
+      expect(lintErrors[0].code).toEqual(
+        CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER,
+      );
+    });
+  });
 });

app/client/src/plugins/Linting/utils/getLintingErrors.ts

@@ -7,6 +7,7 @@ import type {
   MemberExpressionData,
   AssignmentExpressionData,
   CallExpressionData,
+  MemberCallExpressionData,
 } from "@shared/ast";
 import {
   extractExpressionsFromCode,
@@ -271,20 +272,22 @@ function getInvalidWidgetPropertySetterErrors({
 }
 
 function getInvalidAppsmithStoreSetterErrors({
-  assignmentExpressions,
+  appsmithStoreMutationExpressions,
   originalBinding,
   script,
   scriptPos,
 }: {
   data: Record<string, unknown>;
-  assignmentExpressions: AssignmentExpressionData[];
+  appsmithStoreMutationExpressions: Array<
+    AssignmentExpressionData | MemberCallExpressionData
+  >;
   scriptPos: Position;
   originalBinding: string;
   script: string;
 }) {
   const assignmentExpressionErrors: LintError[] = [];
 
-  for (const { object, parentNode } of assignmentExpressions) {
+  for (const { object, parentNode } of appsmithStoreMutationExpressions) {
     if (!isMemberExpressionNode(object)) continue;
     const assignmentExpressionString = generate(parentNode);
     if (!assignmentExpressionString.startsWith("appsmith.store")) continue;
@@ -386,6 +389,7 @@ function getCustomErrorsFromScript(
   let invalidTopLevelMemberExpressions: MemberExpressionData[] = [];
   let assignmentExpressions: AssignmentExpressionData[] = [];
   let callExpressions: CallExpressionData[] = [];
+  let memberCallExpressions: MemberCallExpressionData[] = [];
   try {
     const value = extractExpressionsFromCode(
       script,
@@ -396,6 +400,7 @@ function getCustomErrorsFromScript(
       value.invalidTopLevelMemberExpressionsArray;
     assignmentExpressions = value.assignmentExpressionsData;
     callExpressions = value.callExpressionsData;
+    memberCallExpressions = value.memberCallExpressionData;
   } catch (e) {}
 
   const invalidWidgetPropertySetterErrors =
@@ -416,9 +421,16 @@ function getCustomErrorsFromScript(
     isJSObject,
   );
 
+  // This ensures that all cases where appsmith.store is getting modified
+  // either by assignment using `appsmith.store.test = ""`
+  // or by calling a function like `appsmith.store.test.push()` will result in lint error
+  const appsmithStoreMutationExpressions: Array<
+    AssignmentExpressionData | MemberCallExpressionData
+  > = [...assignmentExpressions, ...memberCallExpressions];
+
   const invalidAppsmithStorePropertyErrors =
     getInvalidAppsmithStoreSetterErrors({
-      assignmentExpressions,
+      appsmithStoreMutationExpressions,
       script,
       scriptPos,
       originalBinding,