diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts deleted file mode 100644 index cc72dc807d..0000000000 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts +++ /dev/null @@ -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(); - }); - }, -); diff --git a/app/client/packages/ast/index.ts b/app/client/packages/ast/index.ts index d5dfb3d145..e5fe99afd0 100644 --- a/app/client/packages/ast/index.ts +++ b/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 { diff --git a/app/client/packages/ast/src/index.ts b/app/client/packages/ast/src/index.ts index da11d206e8..83a9f93541 100644 --- a/app/client/packages/ast/src/index.ts +++ b/app/client/packages/ast/src/index.ts @@ -597,6 +597,16 @@ export interface CallExpressionData { params: NodeWithLocation[]; } +// 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; + object: NodeWithLocation; + parentNode: NodeWithLocation; +} + 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(); const callExpressionsData = new Set(); + const memberCallExpressionData = new Set(); const invalidTopLevelMemberExpressions = new Set(); const variableDeclarations = new Set(); let functionalParams = new Set(); @@ -646,6 +658,7 @@ export const extractExpressionsFromCode = ( invalidTopLevelMemberExpressionsArray: [], assignmentExpressionsData: [], callExpressionsData: [], + memberCallExpressionData: [], }; } throw e; @@ -726,11 +739,23 @@ export const extractExpressionsFromCode = ( } as AssignmentExpressionData); }, CallExpression(node: Node) { - if (isCallExpressionNode(node) && isIdentifierNode(node.callee)) { - callExpressionsData.add({ - property: node.callee, - params: node.arguments, - } as CallExpressionData); + 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], }; }; diff --git a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts b/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts index 3a7f0014f5..89d7b61886 100644 --- a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts +++ b/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, + ); + }); + }); }); diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index 4cacfcb923..12901d4a5d 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/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; - 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,