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
This commit is contained in:
Favour Ohanekwu 2023-09-29 13:30:23 +01:00 committed by GitHub
parent 70e464056b
commit ace2d3b104
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 213 additions and 66 deletions

View File

@ -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();
});
});

View File

@ -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();

View File

@ -584,9 +584,8 @@ export interface MemberExpressionData {
export interface AssignmentExpressionData {
property: NodeWithLocation<IdentifierNode | LiteralNode>;
object: NodeWithLocation<IdentifierNode>;
start: number;
end: number;
object: NodeWithLocation<IdentifierNode | MemberExpressionNode>;
parentNode: NodeWithLocation<AssignmentExpressionNode>;
}
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);
},
});

View File

@ -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";
},
};

View File

@ -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<string, Position> = {};
@ -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<string, unknown>;
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<string, unknown>,
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<string, unknown>,
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(