diff --git a/app/client/package.json b/app/client/package.json index 8a365d4286..1a4652a738 100644 --- a/app/client/package.json +++ b/app/client/package.json @@ -261,6 +261,7 @@ "@types/deep-diff": "^1.0.0", "@types/dom-view-transitions": "^1.0.5", "@types/downloadjs": "^1.4.2", + "@types/estree": "^1.0.6", "@types/jest": "^29.5.3", "@types/jshint": "^2.12.0", "@types/lodash": "^4.14.120", diff --git a/app/client/src/ce/hooks/index.ts b/app/client/src/ce/hooks/index.ts index cd84f5ca1d..07f4b12bcd 100644 --- a/app/client/src/ce/hooks/index.ts +++ b/app/client/src/ce/hooks/index.ts @@ -18,7 +18,9 @@ export const editorType: EditorType = { [BUILDER_BASE_PATH_DEPRECATED]: EditorNames.APPLICATION, }; -export const useEditorType = (path: string) => { +// Utility function to get editor type based on path +// to be used in non react functions +export const getEditorType = (path: string) => { const basePath = matchPath(path, { path: [BUILDER_VIEWER_PATH_PREFIX, BUILDER_BASE_PATH_DEPRECATED], }); @@ -28,6 +30,11 @@ export const useEditorType = (path: string) => { : editorType[BUILDER_VIEWER_PATH_PREFIX]; }; +// custom hook to get editor type based on path +export const useEditorType = (path: string) => { + return getEditorType(path); +}; + export function useOutsideClick( ref: RefObject, inputRef: RefObject, diff --git a/app/client/src/ce/utils/lintRulesHelpers.ts b/app/client/src/ce/utils/lintRulesHelpers.ts new file mode 100644 index 0000000000..fc857cad6b --- /dev/null +++ b/app/client/src/ce/utils/lintRulesHelpers.ts @@ -0,0 +1,15 @@ +interface ContextType { + editorType: string; +} + +/** + * Function used to conditionally enable/disable custom rules based on context provided + * + * @returns {Record} Object with settings for the rule + * */ +export const getLintRulesBasedOnContext = ({}: ContextType): Record< + string, + "off" | "error" +> => { + return {}; +}; diff --git a/app/client/src/ee/utils/lintRulesHelpers.ts b/app/client/src/ee/utils/lintRulesHelpers.ts new file mode 100644 index 0000000000..9e1547fd34 --- /dev/null +++ b/app/client/src/ee/utils/lintRulesHelpers.ts @@ -0,0 +1 @@ +export * from "ce/utils/lintRulesHelpers"; diff --git a/app/client/src/plugins/Linting/constants.ts b/app/client/src/plugins/Linting/constants.ts index 433fb0a3b1..40f0ab2d5c 100644 --- a/app/client/src/plugins/Linting/constants.ts +++ b/app/client/src/plugins/Linting/constants.ts @@ -1,7 +1,8 @@ import { ECMA_VERSION } from "@shared/ast"; import type { LintOptions } from "jshint"; import isEntityFunction from "./utils/isEntityFunction"; -import type { Linter } from "eslint-linter-browserify"; +import { noFloatingPromisesLintRule } from "./customRules/no-floating-promises"; +import { getLintRulesBasedOnContext } from "ee/utils/lintRulesHelpers"; export enum LINTER_TYPE { "JSHINT" = "JSHint", @@ -9,7 +10,9 @@ export enum LINTER_TYPE { } export const lintOptions = ( - globalData: Record, + globalData: Record, + asyncFunctions: string[], + editorType: string, linterType: LINTER_TYPE = LINTER_TYPE.JSHINT, ) => { if (linterType === LINTER_TYPE.JSHINT) { @@ -39,25 +42,28 @@ export const lintOptions = ( loopfunc: true, } as LintOptions; } else { - const eslintGlobals: Record = { - setTimeout: "readonly", - clearTimeout: "readonly", - console: "readonly", - }; - - for (const key in globalData) { - if (globalData.hasOwnProperty(key)) { - eslintGlobals[key] = "readonly"; - } - } + const extraRules = getLintRulesBasedOnContext({ editorType }); return { languageOptions: { ecmaVersion: ECMA_VERSION, - globals: eslintGlobals, + globals: globalData, sourceType: "script", }, + // Need to pass for custom rules + settings: { + globalData, + asyncFunctions, + }, + plugins: { + customRules: { + rules: { + "no-floating-promises": noFloatingPromisesLintRule, + }, + }, + }, rules: { + ...extraRules, eqeqeq: "off", curly: "off", "no-extend-native": "error", @@ -76,7 +82,7 @@ export const lintOptions = ( "no-unused-expressions": "off", "no-loop-func": "off", }, - } as Linter.Config; + } as const; } }; diff --git a/app/client/src/plugins/Linting/customRules/no-floating-promises.ts b/app/client/src/plugins/Linting/customRules/no-floating-promises.ts new file mode 100644 index 0000000000..df174e3f37 --- /dev/null +++ b/app/client/src/plugins/Linting/customRules/no-floating-promises.ts @@ -0,0 +1,138 @@ +import type { Rule } from "eslint"; +import type * as ESTree from "estree"; + +export const noFloatingPromisesLintRule: Rule.RuleModule = { + meta: { + type: "problem", + docs: { + description: "Requires handling of Promises (using await or .then())", + category: "Possible Errors", + recommended: false, + }, + messages: { + unhandledPromise: + "Unhandled Promise detected. Handle using await or .then()", + }, + schema: [], // Rule does not accept configuration options + }, + create: function (context: Rule.RuleContext) { + // Access async functions from settings + const asyncFunctions = context.settings?.asyncFunctions || []; + + return { + FunctionDeclaration(node: ESTree.FunctionDeclaration) { + // Start traversal from the function body + traverseNode(node.body, null); + }, + }; + + /** + * Recursively traverses the AST node and its children. + * Processes CallExpressions and continues traversing child nodes. + */ + function traverseNode( + node: ESTree.Node | null, + parent: ESTree.Node | null, + ) { + if (!node) return; + + if (node.type === "CallExpression") { + checkCallExpression(node as ESTree.CallExpression, parent); + } + + // Retrieve keys for child nodes and traverse them + const visitorKeys = context.getSourceCode().visitorKeys[node.type] || []; + + for (const key of visitorKeys) { + const child = (node as any)[key]; // eslint-disable-line @typescript-eslint/no-explicit-any + + if (Array.isArray(child)) { + child.forEach( + (c) => c && typeof c.type === "string" && traverseNode(c, node), + ); + } else if (child && typeof child.type === "string") { + traverseNode(child, node); + } + } + } + + /** + * Determines if a node is inside an async function by traversing its parent chain. + */ + function isInAsyncFunction(node: ESTree.Node | null): boolean { + while (node) { + if ( + (node.type === "FunctionDeclaration" || + node.type === "FunctionExpression") && + node.async + ) { + return true; + } + + // Move to the parent node in the AST + // @ts-expect-error: Types may not always define `parent` + node = node.parent; + } + + return false; + } + + /** + * Checks if a CallExpression represents an unhandled Promise. + * Reports an error if the promise is not awaited or chained with `.then()`, `.catch()`, or `.finally()`. + */ + function checkCallExpression( + node: ESTree.CallExpression, + parent: ESTree.Node | null, + ) { + const callee = node.callee; + let isPotentialAsyncCall = false; + + // Identify async calls by matching against the asyncFunctions list + if (callee.type === "MemberExpression") { + const object = callee.object; + const property = callee.property; + + if ( + property.type === "Identifier" && + object.type === "Identifier" && + asyncFunctions.includes(`${object.name}.${property.name}`) + ) { + isPotentialAsyncCall = true; + } + } + + // Report if the async call is unhandled and not properly awaited + if (isPotentialAsyncCall && isInAsyncFunction(parent)) { + if ( + parent && + parent.type !== "AwaitExpression" && + parent.type !== "ReturnStatement" && + !isHandledWithPromiseMethods(parent) + ) { + context.report({ + node, + messageId: "unhandledPromise", + }); + } + } + } + + /** + * Determines if a CallExpression is handled with `.then()`, `.catch()`, or `.finally()`. + */ + function isHandledWithPromiseMethods(parent: ESTree.Node): boolean { + if ( + parent.type === "MemberExpression" && + parent.property && + ["then", "catch", "finally"].includes( + (parent.property as ESTree.Identifier).name, + ) + ) { + return true; + } + + return false; + } + }, +}; diff --git a/app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts b/app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts new file mode 100644 index 0000000000..efa9e1f6bc --- /dev/null +++ b/app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts @@ -0,0 +1,99 @@ +import { ENTITY_TYPE } from "ee/entities/DataTree/types"; +import { LINTER_TYPE } from "../constants"; +import { generateLintingGlobalData } from "../utils/getLintingErrors"; + +jest.mock("ee/hooks", () => { + const actualModule = jest.requireActual("ee/hooks"); + + return { + ...actualModule, // Spread the original module exports + getEditorType: jest.fn(() => "TestEditorType"), // Override `getEditorType` + }; +}); + +describe("generateLintingGlobalData", () => { + it("should generate the correct response type", () => { + const mockData = { + Query1: { + ENTITY_TYPE: ENTITY_TYPE.ACTION, + }, + JSObject1: { + ENTITY_TYPE: ENTITY_TYPE.JSACTION, + myFun1: "function() {}", + myFun2: "async function() {}", + test: "async function() {}", + body: "Some body text", + actionId: "action-id", + }, + appsmith: { + ENTITY_TYPE: ENTITY_TYPE.APPSMITH, + URL: { pathname: "/test/editor/path" }, + }, + }; + + const result = generateLintingGlobalData(mockData, LINTER_TYPE.ESLINT); + + expect(result.globalData).toEqual({ + setTimeout: "readonly", + clearTimeout: "readonly", + console: "readonly", + Query1: "readonly", + JSObject1: "readonly", + appsmith: "readonly", + crypto: "readonly", + forge: "readonly", + moment: "readonly", + _: "readonly", + fetch: "readonly", + }); + + expect(result.asyncFunctions).toEqual([ + "Query1.run", + "JSObject1.myFun2", + "JSObject1.test", + ]); + + expect(result.editorType).toEqual("TestEditorType"); + }); + + it("should handle an empty input and return default values", () => { + const result = generateLintingGlobalData({}, LINTER_TYPE.ESLINT); + + expect(result.globalData).toEqual({ + setTimeout: "readonly", + clearTimeout: "readonly", + console: "readonly", + crypto: "readonly", + forge: "readonly", + moment: "readonly", + _: "readonly", + fetch: "readonly", + }); + + expect(result.asyncFunctions).toEqual([]); + }); + + it("should handle unsupported ENTITY_TYPE gracefully", () => { + const mockData = { + UnknownEntity: { + ENTITY_TYPE: "UNKNOWN", + }, + }; + + const result = generateLintingGlobalData(mockData, LINTER_TYPE.ESLINT); + + expect(result.globalData).toEqual({ + setTimeout: "readonly", + clearTimeout: "readonly", + console: "readonly", + UnknownEntity: "readonly", + crypto: "readonly", + forge: "readonly", + moment: "readonly", + _: "readonly", + fetch: "readonly", + }); + + expect(result.asyncFunctions).toEqual([]); + }); +}); diff --git a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts b/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts index a488e90dd4..99c883d82e 100644 --- a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts +++ b/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts @@ -1,8 +1,18 @@ -import { getScriptType } from "workers/Evaluation/evaluate"; +import { + EvaluationScriptType, + getScriptType, +} from "workers/Evaluation/evaluate"; import { CustomLintErrorCode, LINTER_TYPE } from "../constants"; import getLintingErrors from "../utils/getLintingErrors"; import { Severity } from "entities/AppsmithConsole"; +// Define all the custom eslint rules you want to test here +jest.mock("ee/utils/lintRulesHelpers", () => ({ + getLintRulesBasedOnContext: jest.fn(() => ({ + "customRules/no-floating-promises": "error", + })), +})); + const webworkerTelemetry = {}; const linterTypes = [ @@ -598,3 +608,72 @@ describe.each(linterTypes)( }); }, ); + +describe("Custom lint checks", () => { + // This is done since all custom lint rules need eslint as linter type + const getLinterTypeFn = () => LINTER_TYPE.ESLINT; + + // Test for 'no floating promises lint rule' + it("1. should show error for unhandled promises", () => { + const data = { + ARGUMENTS: undefined, + Query1: { + actionId: "671b2fcc-e574", + isLoading: false, + responseMeta: { + isExecutionSuccess: false, + }, + config: { + timeoutInMillisecond: 10000, + paginationType: "NONE", + encodeParamsToggle: true, + body: "SELECT * FROM <> LIMIT 10;\n\n-- Please enter a valid table name and hit RUN\n", + pluginSpecifiedTemplates: [ + { + value: true, + }, + ], + }, + ENTITY_TYPE: "ACTION", + datasourceUrl: "", + name: "Query1", + run: async function () {}, + clear: async function () {}, + }, + JSObject1: { + body: "export default {\n\tasync handledAsync() {\n\t\tawait Query1.run(); \n\t},\n\tasync unhandledAsync() {\n\t\tQuery1.run();\n\t}\n}", + ENTITY_TYPE: "JSACTION", + actionId: "d24fc04a-910b", + handledAsync: "async function () {\n await Query1.run();\n}", + "handledAsync.data": {}, + unhandledAsync: "async function () {\n Query1.run();\n}", + "unhandledAsync.data": {}, + }, + THIS_CONTEXT: {}, + }; + + const originalBinding = "async unhandledAsync() {\n\t\tQuery1.run();\n\t}"; + const script = + "\n function $$closedFn () {\n const $$result = {async unhandledAsync() {\n\t\tQuery1.run();\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n "; + const options = { isJsObject: true }; + + const lintErrors = getLintingErrors({ + getLinterTypeFn, + data, + originalBinding, + script, + scriptType: EvaluationScriptType.EXPRESSION, + options, + webworkerTelemetry, + }); + + expect(Array.isArray(lintErrors)).toBe(true); + expect(lintErrors.length).toEqual(1); + //expect(lintErrors[0].code).toEqual( + // CustomLintErrorCode.INVALID_ENTITY_PROPERTY, + //); + expect(lintErrors[0].errorMessage.message).toContain( + "Unhandled Promise detected.", + ); + }); +}); diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index af7e9a41cd..a66476af4c 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/app/client/src/plugins/Linting/utils/getLintingErrors.ts @@ -42,6 +42,10 @@ import { profileFn } from "instrumentation/generateWebWorkerTraces"; import { WorkerEnv } from "workers/Evaluation/handlers/workerEnv"; import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; import { Linter } from "eslint-linter-browserify"; +import { ENTITY_TYPE } from "entities/DataTree/dataTreeFactory"; +import type { DataTreeEntity } from "entities/DataTree/dataTreeTypes"; +import { EditorNames, getEditorType } from "ee/hooks"; +import type { AppsmithEntity } from "ee/entities/DataTree/types"; const EvaluationScriptPositions: Record = {}; @@ -71,25 +75,88 @@ function getEvaluationScriptPosition(scriptType: EvaluationScriptType) { return EvaluationScriptPositions[scriptType]; } -function generateLintingGlobalData(data: Record) { - const globalData: Record = {}; +export function generateLintingGlobalData( + data: Record, + linterType = LINTER_TYPE.JSHINT, +) { + const asyncFunctions: string[] = []; + let editorType = EditorNames.APPLICATION; + let globalData: Record = {}; - for (const dataKey in data) { - globalData[dataKey] = true; + if (linterType === LINTER_TYPE.JSHINT) { + // TODO: cleanup jshint implementation once rollout is complete + + for (const dataKey in data) { + globalData[dataKey] = true; + } + + // Add all js libraries + const libAccessors = ([] as string[]).concat( + ...JSLibraries.map((lib) => lib.accessor), + ); + + libAccessors.forEach((accessor) => (globalData[accessor] = true)); + // Add all supported web apis + objectKeys(SUPPORTED_WEB_APIS).forEach( + (apiName) => (globalData[apiName] = true), + ); + } else { + globalData = { + setTimeout: "readonly", + clearTimeout: "readonly", + console: "readonly", + }; + + for (const dataKey in data) { + globalData[dataKey] = "readonly"; + + const dataValue = data[dataKey] as DataTreeEntity; + + if ( + !!dataValue && + dataValue.hasOwnProperty("ENTITY_TYPE") && + !!dataValue["ENTITY_TYPE"] + ) { + const { ENTITY_TYPE: dataValueEntityType } = dataValue; + + if (dataValueEntityType === ENTITY_TYPE.ACTION) { + asyncFunctions.push(`${dataKey}.run`); + } else if (dataValueEntityType === ENTITY_TYPE.JSACTION) { + const ignoreKeys = ["body", "ENTITY_TYPE", "actionId"]; + + for (const key in dataValue) { + if (!ignoreKeys.includes(key)) { + const value = dataValue[key]; + + if ( + typeof value === "string" && + value.startsWith("async function") + ) { + asyncFunctions.push(`${dataKey}.${key}`); + } + } + } + } else if (dataValueEntityType === ENTITY_TYPE.APPSMITH) { + const appsmithEntity: AppsmithEntity = dataValue; + + editorType = getEditorType(appsmithEntity.URL.pathname); + } + } + } + + // Add all js libraries + const libAccessors = ([] as string[]).concat( + ...JSLibraries.map((lib) => lib.accessor), + ); + + libAccessors.forEach((accessor) => (globalData[accessor] = "readonly")); + // Add all supported web apis + objectKeys(SUPPORTED_WEB_APIS).forEach( + (apiName) => (globalData[apiName] = "readonly"), + ); } - // Add all js libraries - const libAccessors = ([] as string[]).concat( - ...JSLibraries.map((lib) => lib.accessor), - ); - - libAccessors.forEach((accessor) => (globalData[accessor] = true)); - // Add all supported web apis - objectKeys(SUPPORTED_WEB_APIS).forEach( - (apiName) => (globalData[apiName] = true), - ); - - return globalData; + return { globalData, asyncFunctions, editorType }; } function sanitizeESLintErrors( @@ -302,8 +369,17 @@ export default function getLintingErrors({ }: getLintingErrorsProps): LintError[] { const linterType = getLinterTypeFn(); const scriptPos = getEvaluationScriptPosition(scriptType); - const lintingGlobalData = generateLintingGlobalData(data); - const lintingOptions = lintOptions(lintingGlobalData, linterType); + const { + asyncFunctions, + editorType, + globalData: lintingGlobalData, + } = generateLintingGlobalData(data, linterType); + const lintingOptions = lintOptions( + lintingGlobalData, + asyncFunctions, + editorType, + linterType, + ); let messages: Linter.LintMessage[] = []; let lintErrors: LintError[] = []; diff --git a/app/client/yarn.lock b/app/client/yarn.lock index a76a9e99a6..6d3cf69e75 100644 --- a/app/client/yarn.lock +++ b/app/client/yarn.lock @@ -10749,10 +10749,10 @@ __metadata: languageName: node linkType: hard -"@types/estree@npm:*, @types/estree@npm:1.0.5, @types/estree@npm:^1.0.0": - version: 1.0.5 - resolution: "@types/estree@npm:1.0.5" - checksum: dd8b5bed28e6213b7acd0fb665a84e693554d850b0df423ac8076cc3ad5823a6bc26b0251d080bdc545af83179ede51dd3f6fa78cad2c46ed1f29624ddf3e41a +"@types/estree@npm:*, @types/estree@npm:^1.0.0, @types/estree@npm:^1.0.6": + version: 1.0.6 + resolution: "@types/estree@npm:1.0.6" + checksum: 8825d6e729e16445d9a1dd2fb1db2edc5ed400799064cd4d028150701031af012ba30d6d03fe9df40f4d7a437d0de6d2b256020152b7b09bde9f2e420afdffd9 languageName: node linkType: hard @@ -10763,6 +10763,13 @@ __metadata: languageName: node linkType: hard +"@types/estree@npm:1.0.5": + version: 1.0.5 + resolution: "@types/estree@npm:1.0.5" + checksum: dd8b5bed28e6213b7acd0fb665a84e693554d850b0df423ac8076cc3ad5823a6bc26b0251d080bdc545af83179ede51dd3f6fa78cad2c46ed1f29624ddf3e41a + languageName: node + linkType: hard + "@types/estree@npm:^0.0.51": version: 0.0.51 resolution: "@types/estree@npm:0.0.51" @@ -13179,6 +13186,7 @@ __metadata: "@types/deep-diff": ^1.0.0 "@types/dom-view-transitions": ^1.0.5 "@types/downloadjs": ^1.4.2 + "@types/estree": ^1.0.6 "@types/google.maps": ^3.51.0 "@types/jest": ^29.5.3 "@types/jshint": ^2.12.0