From ace2d3b1048738f16cb31bd2b3bd2d23a87dda95 Mon Sep 17 00:00:00 2001 From: Favour Ohanekwu Date: Fri, 29 Sep 2023 13:30:23 +0100 Subject: [PATCH] feat: show lint error for imperative store update (#27708) ## Description This PR introduces lint errors for an imperative update of the appsmith store object #### PR fixes following issue(s) Fixes #5822 #### Media ![Screenshot 2023-09-29 at 07 47 49](https://github.com/appsmithorg/appsmith/assets/46670083/f89a8dde-88c3-4b3c-8354-6797db440bac) #### DP https://ce-27708.dp.appsmith.com/ #### Type of change - New feature (non-breaking change which adds functionality) ## Testing > #### How Has This Been Tested? > Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration. > Delete anything that is not relevant - [x] Manual - [ ] JUnit - [ ] Jest - [x] Cypress > > #### Test Plan Store value modification in a js object, widget, query > > #### Issues raised during DP testing https://github.com/appsmithorg/appsmith/pull/27708#issuecomment-1740767974 > > > ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [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 --- .../Widgets/ImperativeStoreUpdate_spec.ts | 50 +++++ .../Widgets/WidgetPropertySetters_spec.ts | 2 +- app/client/packages/ast/src/index.ts | 12 +- app/client/src/plugins/Linting/constants.ts | 22 ++ .../plugins/Linting/utils/getLintingErrors.ts | 193 ++++++++++++------ 5 files changed, 213 insertions(+), 66 deletions(-) create mode 100644 app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts new file mode 100644 index 0000000000..b8570da4fc --- /dev/null +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/ImperativeStoreUpdate_spec.ts @@ -0,0 +1,50 @@ +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", 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/cypress/e2e/Regression/ClientSide/Widgets/WidgetPropertySetters_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/WidgetPropertySetters_spec.ts index 460c460787..dfeb14d87c 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/WidgetPropertySetters_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/WidgetPropertySetters_spec.ts @@ -180,7 +180,7 @@ describe("Linting warning for setter methods", function () { agHelper.AssertElementVisibility(locators._lintErrorElement); agHelper.HoverElement(locators._lintErrorElement); agHelper.AssertContains( - "Direct mutation of widget properties aren't supported. Use Button1.setVisibility(value) instead.", + "Direct mutation of widget properties is not supported. Use Button1.setVisibility(value) instead.", ); agHelper.Sleep(); diff --git a/app/client/packages/ast/src/index.ts b/app/client/packages/ast/src/index.ts index 27ac5c5e9a..78e19623d7 100644 --- a/app/client/packages/ast/src/index.ts +++ b/app/client/packages/ast/src/index.ts @@ -584,9 +584,8 @@ export interface MemberExpressionData { export interface AssignmentExpressionData { property: NodeWithLocation; - object: NodeWithLocation; - start: number; - end: number; + object: NodeWithLocation; + parentNode: NodeWithLocation; } export interface AssignmentExpressionNode extends Node { @@ -706,15 +705,12 @@ export const extractExpressionsFromCode = ( ) return; - const { object, property } = node.left as MemberExpressionNode; - if (!isIdentifierNode(object)) return; - if (!(object.name in data)) return; + const { object, property } = node.left; assignmentExpressionsData.add({ object, property, - start: node.start, - end: node.end, + parentNode: node, } as AssignmentExpressionData); }, }); diff --git a/app/client/src/plugins/Linting/constants.ts b/app/client/src/plugins/Linting/constants.ts index 25d75990fa..c985f1a90b 100644 --- a/app/client/src/plugins/Linting/constants.ts +++ b/app/client/src/plugins/Linting/constants.ts @@ -62,6 +62,10 @@ export const SUPPORTED_WEB_APIS = { export enum CustomLintErrorCode { INVALID_ENTITY_PROPERTY = "INVALID_ENTITY_PROPERTY", ASYNC_FUNCTION_BOUND_TO_SYNC_FIELD = "ASYNC_FUNCTION_BOUND_TO_SYNC_FIELD", + // ButtonWidget.text = "test" + INVALID_WIDGET_PROPERTY_SETTER = "INVALID_WIDGET_PROPERTY_SETTER", + // appsmith.store.value = "test" + INVALID_APPSMITH_STORE_PROPERTY_SETTER = "INVALID_APPSMITH_STORE_PROPERTY_SETTER", } export const CUSTOM_LINT_ERRORS: Record< @@ -93,4 +97,22 @@ export const CUSTOM_LINT_ERRORS: Record< hasMultipleBindings ? "fields" : "field" }: ${bindings}`; }, + [CustomLintErrorCode.INVALID_WIDGET_PROPERTY_SETTER]: ( + methodName: string, + objectName: string, + propertyName: string, + isValidProperty: boolean, + ) => { + const suggestionSentence = methodName + ? `Use ${methodName}(value) instead.` + : `Use ${objectName} setter method instead.`; + + const lintErrorMessage = !isValidProperty + ? `${objectName} doesn't have a property named ${propertyName}` + : `Direct mutation of widget properties is not supported. ${suggestionSentence}`; + return lintErrorMessage; + }, + [CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER]: () => { + return "Use storeValue() method to modify the store"; + }, }; diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index b62d26188e..abc7adfb0c 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/app/client/src/plugins/Linting/utils/getLintingErrors.ts @@ -7,7 +7,11 @@ import type { MemberExpressionData, AssignmentExpressionData, } from "@shared/ast"; -import { extractExpressionsFromCode, isLiteralNode } from "@shared/ast"; +import { + extractExpressionsFromCode, + isIdentifierNode, + isLiteralNode, +} from "@shared/ast"; import { PropertyEvaluationErrorType } from "utils/DynamicBindingUtils"; import type { EvaluationScriptType } from "workers/Evaluation/evaluate"; import { EvaluationScripts, ScriptTemplate } from "workers/Evaluation/evaluate"; @@ -27,6 +31,8 @@ import { APPSMITH_GLOBAL_FUNCTIONS } from "components/editorComponents/ActionCre import { last } from "lodash"; import { isWidget } from "@appsmith/workers/Evaluation/evaluationUtils"; import setters from "workers/Evaluation/setters"; +import { isMemberExpressionNode } from "@shared/ast/src"; +import { generate } from "astring"; const EvaluationScriptPositions: Record = {}; @@ -197,7 +203,7 @@ export default function getLintingErrors({ return jshintErrors.concat(customLintErrors); } -function getAssignmentExpressionErrors({ +function getInvalidWidgetPropertySetterErrors({ assignmentExpressions, data, originalBinding, @@ -210,10 +216,12 @@ function getAssignmentExpressionErrors({ originalBinding: string; script: string; }) { - const assignmentExpressionErrors: LintError[] = []; + const invalidWidgetPropertySetterErrors: LintError[] = []; const setterAccessorMap = setters.getSetterAccessorMap(); - for (const { end, object, property, start } of assignmentExpressions) { + for (const { object, parentNode, property } of assignmentExpressions) { + if (!isIdentifierNode(object)) continue; + const assignmentExpressionString = generate(parentNode); const objectName = object.name; const propertyName = isLiteralNode(property) ? (property.value as string) @@ -226,13 +234,9 @@ function getAssignmentExpressionErrors({ const methodName = get(setterAccessorMap, `${objectName}.${propertyName}`); - const suggestionSentence = methodName - ? `Use ${methodName}(value) instead.` - : `Use ${objectName} setter method instead.`; - - const lintErrorMessage = !isValidProperty - ? `${objectName} doesn't have a property named ${propertyName}` - : `Direct mutation of widget properties aren't supported. ${suggestionSentence}`; + const lintErrorMessage = CUSTOM_LINT_ERRORS[ + CustomLintErrorCode.INVALID_WIDGET_PROPERTY_SETTER + ](methodName, objectName, propertyName, isValidProperty); // line position received after AST parsing is 1 more than the actual line of code, hence we subtract 1 to get the actual line number const objectStartLine = object.loc.start.line - 1; @@ -240,31 +244,20 @@ function getAssignmentExpressionErrors({ // AST parsing start column position from index 0 whereas codemirror start ch position from index 1, hence we add 1 to get the actual ch position const objectStartCol = object.loc.start.column + 1; - let variable = isLiteralNode(property) - ? `${object.name}["${propertyName}"]` - : `${object.name}.${propertyName}`; - - const messageLength = end - start; - - variable = variable.concat( - variable, - " ".repeat(messageLength - variable.length), - ); - - assignmentExpressionErrors.push({ + invalidWidgetPropertySetterErrors.push({ errorType: PropertyEvaluationErrorType.LINT, raw: script, severity: getLintSeverity( - CustomLintErrorCode.INVALID_ENTITY_PROPERTY, + CustomLintErrorCode.INVALID_WIDGET_PROPERTY_SETTER, lintErrorMessage, ), errorMessage: { name: "LintingError", message: lintErrorMessage, }, - errorSegment: variable, + errorSegment: assignmentExpressionString, originalBinding, - variables: [variable, null, null], + variables: [assignmentExpressionString, null, null], code: CustomLintErrorCode.INVALID_ENTITY_PROPERTY, line: objectStartLine - scriptPos.line, ch: @@ -273,43 +266,73 @@ function getAssignmentExpressionErrors({ : objectStartCol, }); } + return invalidWidgetPropertySetterErrors; +} + +function getInvalidAppsmithStoreSetterErrors({ + assignmentExpressions, + originalBinding, + script, + scriptPos, +}: { + data: Record; + assignmentExpressions: AssignmentExpressionData[]; + scriptPos: Position; + originalBinding: string; + script: string; +}) { + const assignmentExpressionErrors: LintError[] = []; + + for (const { object, parentNode } of assignmentExpressions) { + if (!isMemberExpressionNode(object)) continue; + const assignmentExpressionString = generate(parentNode); + if (!assignmentExpressionString.startsWith("appsmith.store")) continue; + + const lintErrorMessage = + CUSTOM_LINT_ERRORS[ + CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER + ](); + + // line position received after AST parsing is 1 more than the actual line of code, hence we subtract 1 to get the actual line number + const objectStartLine = object.loc.start.line - 1; + + // AST parsing start column position from index 0 whereas codemirror start ch position from index 1, hence we add 1 to get the actual ch position + const objectStartCol = object.loc.start.column + 1; + + assignmentExpressionErrors.push({ + errorType: PropertyEvaluationErrorType.LINT, + raw: script, + severity: getLintSeverity( + CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER, + lintErrorMessage, + ), + errorMessage: { + name: "LintingError", + message: lintErrorMessage, + }, + errorSegment: assignmentExpressionString, + originalBinding, + variables: [assignmentExpressionString, null, null], + code: CustomLintErrorCode.INVALID_APPSMITH_STORE_PROPERTY_SETTER, + line: objectStartLine - scriptPos.line, + ch: + objectStartLine === scriptPos.line + ? objectStartCol - scriptPos.ch + : objectStartCol, + }); + } return assignmentExpressionErrors; } -// returns lint errors caused by -// 1. accessing invalid properties. Eg. jsObject.unknownProperty -// 2. direct mutation of widget properties. Eg. widget1.left = 10 -function getCustomErrorsFromScript( +function getInvalidEntityPropertyErrors( + invalidTopLevelMemberExpressions: MemberExpressionData[], script: string, data: Record, scriptPos: Position, originalBinding: string, - isJSObject = false, -): LintError[] { - let invalidTopLevelMemberExpressions: MemberExpressionData[] = []; - let assignmentExpressions: AssignmentExpressionData[] = []; - try { - const value = extractExpressionsFromCode( - script, - data, - self.evaluationVersion, - ); - invalidTopLevelMemberExpressions = - value.invalidTopLevelMemberExpressionsArray; - assignmentExpressions = - value.assignmentExpressionsData as AssignmentExpressionData[]; - // eslint-disable-next-line no-console - } catch (e) {} - - const assignmentExpressionErrors = getAssignmentExpressionErrors({ - assignmentExpressions, - script, - scriptPos, - originalBinding, - data, - }); - - const invalidPropertyErrors = invalidTopLevelMemberExpressions.map( + isJSObject: boolean, +) { + return invalidTopLevelMemberExpressions.map( ({ object, property }): LintError => { const propertyName = isLiteralNode(property) ? (property.value as string) @@ -346,8 +369,64 @@ function getCustomErrorsFromScript( }; }, ); +} - return [...invalidPropertyErrors, ...assignmentExpressionErrors]; +// returns custom lint errors caused by +// 1. accessing invalid properties. Eg. jsObject.unknownProperty +// 2. direct mutation of widget properties. Eg. widget1.left = 10 +// 3. imperative update of appsmith store object. Eg. appsmith.store.val = 10 +function getCustomErrorsFromScript( + script: string, + data: Record, + scriptPos: Position, + originalBinding: string, + isJSObject = false, +): LintError[] { + let invalidTopLevelMemberExpressions: MemberExpressionData[] = []; + let assignmentExpressions: AssignmentExpressionData[] = []; + try { + const value = extractExpressionsFromCode( + script, + data, + self.evaluationVersion, + ); + invalidTopLevelMemberExpressions = + value.invalidTopLevelMemberExpressionsArray; + assignmentExpressions = value.assignmentExpressionsData; + } catch (e) {} + + const invalidWidgetPropertySetterErrors = + getInvalidWidgetPropertySetterErrors({ + assignmentExpressions, + script, + scriptPos, + originalBinding, + data, + }); + + const invalidPropertyErrors = getInvalidEntityPropertyErrors( + invalidTopLevelMemberExpressions, + script, + data, + scriptPos, + originalBinding, + isJSObject, + ); + + const invalidAppsmithStorePropertyErrors = + getInvalidAppsmithStoreSetterErrors({ + assignmentExpressions, + script, + scriptPos, + originalBinding, + data, + }); + + return [ + ...invalidPropertyErrors, + ...invalidWidgetPropertySetterErrors, + ...invalidAppsmithStorePropertyErrors, + ]; } function getRefinedW117Error(