From 04718ce56995f6a53fc99d5091146a86964bfd9f Mon Sep 17 00:00:00 2001 From: ravipanguluri <85590689+ravipanguluri@users.noreply.github.com> Date: Thu, 8 Aug 2024 11:15:28 -0700 Subject: [PATCH] CHANGE @W-16438030@ Make file extensions an optional field for regex engine rules (#65) --- package-lock.json | 9 +- .../code-analyzer-regex-engine/package.json | 81 +++++++++--------- .../code-analyzer-regex-engine/src/config.ts | 9 +- .../code-analyzer-regex-engine/src/engine.ts | 33 ++++--- .../test/end-to-end.test.ts | 4 +- .../test/engine.test.ts | 66 ++++++++++++-- .../test/plugin.test.ts | 24 +++++- .../test/test-data/sampleWorkspace/dummy3.js | 2 +- .../test/test-data/sampleWorkspace/dummy4.txt | 1 + .../workspaceWithNoTextFiles/blank.exe | Bin 0 -> 10 bytes .../src/utils.ts | 1 + 11 files changed, 158 insertions(+), 72 deletions(-) create mode 100644 packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy4.txt create mode 100644 packages/code-analyzer-regex-engine/test/test-data/workspaceWithNoTextFiles/blank.exe diff --git a/package-lock.json b/package-lock.json index 894e4924..d23e4050 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3937,6 +3937,8 @@ }, "node_modules/isbinaryfile": { "version": "5.0.2", + "resolved": "https://registry.npmjs.org/isbinaryfile/-/isbinaryfile-5.0.2.tgz", + "integrity": "sha512-GvcjojwonMjWbTkfMpnVHVqXW/wKMYDfEpY94/8zy8HFMOqb/VL6oeONq9v87q4ttVlaTLnGXnJD4B5B1OTGIg==", "license": "MIT", "engines": { "node": ">= 18.0.0" @@ -6528,10 +6530,12 @@ } }, "packages/code-analyzer-flowtest-engine": { + "name": "@salesforce/code-analyzer-flowtest-engine", "version": "0.8.0", "license": "BSD-3-Clause license", "dependencies": { - "@salesforce/code-analyzer-engine-api": "0.8.0" + "@salesforce/code-analyzer-engine-api": "0.8.0", + "@types/node": "^20.0.0" }, "devDependencies": { "@eslint/js": "^8.57.0", @@ -6597,7 +6601,8 @@ "license": "BSD-3-Clause license", "dependencies": { "@salesforce/code-analyzer-engine-api": "0.8.0", - "@types/node": "^20.0.0" + "@types/node": "^20.0.0", + "isbinaryfile": "^5.0.2" }, "devDependencies": { "@eslint/js": "^8.57.0", diff --git a/packages/code-analyzer-regex-engine/package.json b/packages/code-analyzer-regex-engine/package.json index 5b51523f..8ce11e6c 100644 --- a/packages/code-analyzer-regex-engine/package.json +++ b/packages/code-analyzer-regex-engine/package.json @@ -6,58 +6,59 @@ "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", "repository": { - "type": "git", - "url": "git+https://github.com/forcedotcom/code-analyzer-core.git", - "directory": "packages/code-analyzer-regex-engine" + "type": "git", + "url": "git+https://github.com/forcedotcom/code-analyzer-core.git", + "directory": "packages/code-analyzer-regex-engine" }, "main": "dist/index.js", "types": "dist/index.d.ts", "dependencies": { - "@types/node": "^20.0.0", - "@salesforce/code-analyzer-engine-api": "0.8.0" + "@salesforce/code-analyzer-engine-api": "0.8.0", + "@types/node": "^20.0.0", + "isbinaryfile": "^5.0.2" }, "devDependencies": { - "@eslint/js": "^8.57.0", - "@types/jest": "^29.0.0", - "eslint": "^8.57.0", - "jest": "^29.0.0", - "rimraf": "*", - "ts-jest": "^29.0.0", - "typescript": "^5.4.5", - "typescript-eslint": "^7.8.0" + "@eslint/js": "^8.57.0", + "@types/jest": "^29.0.0", + "eslint": "^8.57.0", + "jest": "^29.0.0", + "rimraf": "*", + "ts-jest": "^29.0.0", + "typescript": "^5.4.5", + "typescript-eslint": "^7.8.0" }, "engines": { - "node": ">=20.0.0" + "node": ">=20.0.0" }, "files": [ - "dist", - "LICENSE", - "package.json" + "dist", + "LICENSE", + "package.json" ], "scripts": { - "build": "tsc --build tsconfig.build.json --verbose", - "test": "jest --coverage", - "lint": "eslint src/**/*.ts", - "package": "npm pack", - "all": "npm run build && npm run test && npm run lint && npm run package", - "clean": "tsc --build tsconfig.build.json --clean", - "postclean": "rimraf dist && rimraf coverage && rimraf ./*.tgz && rimraf vulnerabilities", - "scrub": "npm run clean && rimraf node_modules", - "showcoverage": "open ./coverage/lcov-report/index.html" + "build": "tsc --build tsconfig.build.json --verbose", + "test": "jest --coverage", + "lint": "eslint src/**/*.ts", + "package": "npm pack", + "all": "npm run build && npm run test && npm run lint && npm run package", + "clean": "tsc --build tsconfig.build.json --clean", + "postclean": "rimraf dist && rimraf coverage && rimraf ./*.tgz && rimraf vulnerabilities", + "scrub": "npm run clean && rimraf node_modules", + "showcoverage": "open ./coverage/lcov-report/index.html" }, "jest": { - "preset": "ts-jest", - "testEnvironment": "node", - "testMatch": [ - "**/*.test.ts" - ], - "testPathIgnorePatterns": [ - "/node_modules/", - "/dist/" - ], - "collectCoverageFrom": [ - "src/**/*.ts", - "!src/index.ts" - ] + "preset": "ts-jest", + "testEnvironment": "node", + "testMatch": [ + "**/*.test.ts" + ], + "testPathIgnorePatterns": [ + "/node_modules/", + "/dist/" + ], + "collectCoverageFrom": [ + "src/**/*.ts", + "!src/index.ts" + ] } - } +} diff --git a/packages/code-analyzer-regex-engine/src/config.ts b/packages/code-analyzer-regex-engine/src/config.ts index b40bacb0..a628f69a 100644 --- a/packages/code-analyzer-regex-engine/src/config.ts +++ b/packages/code-analyzer-regex-engine/src/config.ts @@ -27,8 +27,9 @@ export type RegexRules = { // The regular expression that triggers a violation when matched against the contents of a file. regex: RegExp; - /// The extensions of the files that you would like to test the regular expression against. - file_extensions: string[]; + // The extensions of the files that you would like to test the regular expression against. + // If not defined, or equal to null, then all text-based files of any file extension will be tested. + file_extensions?: string[]; // A description of the rule's purpose. description: string; @@ -55,7 +56,7 @@ export function validateAndNormalizeConfig(rawConfig: ConfigObject): RegexEngine const description: string = ruleExtractor.extractRequiredString('description') const rawRegexString: string = ruleExtractor.extractRequiredString('regex') const regex: RegExp = validateRegex(rawRegexString, ruleExtractor.getFieldPath('regex')) - const rawFileExtensions: string[] = ruleExtractor.extractRequiredArray('file_extensions', + const rawFileExtensions: string[] | undefined = ruleExtractor.extractArray('file_extensions', (element, fieldPath) => ValueValidator.validateString(element, fieldPath, FILE_EXT_PATTERN)); customRules[ruleName] = { @@ -63,7 +64,7 @@ export function validateAndNormalizeConfig(rawConfig: ConfigObject): RegexEngine description: description, violation_message: ruleExtractor.extractString('violation_message', getDefaultRuleViolationMessage(regex, ruleName, description))!, - file_extensions: normalizeFileExtensions(rawFileExtensions), + ...(rawFileExtensions ? { file_extensions: normalizeFileExtensions(rawFileExtensions) } : {}), // Additional fields to complete the RuleDescription: name: ruleName, diff --git a/packages/code-analyzer-regex-engine/src/engine.ts b/packages/code-analyzer-regex-engine/src/engine.ts index 4fe0e3de..a8b8a1af 100644 --- a/packages/code-analyzer-regex-engine/src/engine.ts +++ b/packages/code-analyzer-regex-engine/src/engine.ts @@ -11,6 +11,14 @@ import path from "node:path"; import fs from "node:fs"; import os from "node:os"; import {RegexRules} from "./config"; +import {isBinaryFileSync} from "isbinaryfile"; + +const TEXT_BASED_FILE_EXTS = new Set( + [ + '.cjs', '.cls', '.cmp', '.css', '.csv', '.htm', '.html', '.js', '.json', '.jsx', '.md', '.mdt', '.mjs', '.page', + '.rtf', '.trigger', '.ts', '.tsx', '.txt', '.xml', '.xsl', '.xslt', '.yaml', '.yml' + ] +) export class RegexEngine extends Engine { static readonly NAME = "regex"; @@ -34,12 +42,12 @@ export class RegexEngine extends Engine { if (!describeOptions.workspace) { return Object.values(this.regexRules); } - const fullFileList: string[] = await describeOptions.workspace.getExpandedFiles(); - const fileExtSet: Set = getSetOfFileExtensionsFrom(fullFileList); + const fullFileList: string[] = await describeOptions.workspace.getExpandedFiles(); const ruleDescriptions: RuleDescription[] = []; + for (const regexRule of Object.values(this.regexRules)) { - if (regexRule.file_extensions.some(fileExt => fileExtSet.has(fileExt))) { + if (fullFileList.some(fileName => this.shouldScanFile(fileName, regexRule.name))){ ruleDescriptions.push(regexRule); } } @@ -62,7 +70,9 @@ export class RegexEngine extends Engine { private shouldScanFile(fileName: string, ruleName: string): boolean { const ext: string = path.extname(fileName).toLowerCase(); - return this.regexRules[ruleName].file_extensions.includes(ext); + return this.regexRules[ruleName].file_extensions ? + this.regexRules[ruleName].file_extensions.includes(ext) : + TEXT_BASED_FILE_EXTS.has(ext) || isTextFile(fileName); } private async scanFile(fileName: string, ruleName: string): Promise { @@ -94,14 +104,6 @@ export class RegexEngine extends Engine { } } -function getSetOfFileExtensionsFrom(files: string[]): Set { - const fileExtSet: Set = new Set(); - for (const file of files) { - fileExtSet.add(path.extname(file).toLowerCase()); - } - return fileExtSet; -} - function getColumnNumber(charIndex: number, newlineIndexes: number[]): number { const idxOfNextNewline = newlineIndexes.findIndex(el => el >= charIndex); const idxOfCurrentLine = idxOfNextNewline === -1 ? newlineIndexes.length - 1: idxOfNextNewline - 1; @@ -121,10 +123,15 @@ function getLineNumber(charIndex: number, newlineIndexes: number[]): number{ function getNewlineIndices(fileContents: string): number[] { const newlineRegex: RegExp = new RegExp(os.EOL, "g"); const matches = fileContents.matchAll(newlineRegex); - const newlineIndexes = [0]; + const newlineIndexes = [-1]; for (const match of matches) { newlineIndexes.push(match.index) } return newlineIndexes; +} + +// TODO: Make this async and cache results to improve performance +function isTextFile(fileName: string): boolean { + return !isBinaryFileSync(fileName); } \ No newline at end of file diff --git a/packages/code-analyzer-regex-engine/test/end-to-end.test.ts b/packages/code-analyzer-regex-engine/test/end-to-end.test.ts index 2597610f..1c9a209d 100644 --- a/packages/code-analyzer-regex-engine/test/end-to-end.test.ts +++ b/packages/code-analyzer-regex-engine/test/end-to-end.test.ts @@ -32,11 +32,13 @@ describe('End to end test', () => { const ruleDescriptions: RuleDescription[] = await engine.describeRules({workspace: workspace}); const recommendedRuleNames: string[] = ruleDescriptions.filter(rd => rd.tags.includes('Recommended')).map(rd => rd.name); const engineRunResults: EngineRunResults = await engine.runRules(recommendedRuleNames, {workspace: workspace}); - const violationsFromTsFile: Violation[] = engineRunResults.violations.filter(v => path.extname(v.codeLocations[0].file) === '.ts'); + expect(violationsFromTsFile).toHaveLength(1); expect(violationsFromTsFile.map(v => v.ruleName)).toEqual(['NoTodos']); + const violationsFromClsFile: Violation[] = engineRunResults.violations.filter(v => path.extname(v.codeLocations[0].file) === '.cls'); + expect(violationsFromClsFile).toHaveLength(1); expect(violationsFromClsFile.map(v => v.ruleName)).toEqual(['NoTrailingWhitespace']); }); diff --git a/packages/code-analyzer-regex-engine/test/engine.test.ts b/packages/code-analyzer-regex-engine/test/engine.test.ts index db89fd57..2c25abdb 100644 --- a/packages/code-analyzer-regex-engine/test/engine.test.ts +++ b/packages/code-analyzer-regex-engine/test/engine.test.ts @@ -27,6 +27,16 @@ const SAMPLE_CUSTOM_RULES: RegexRules = { severityLevel: SeverityLevel.Moderate, tags: ['Recommended'], resourceUrls: [] + }, + NoHellos: { + regex: /hello/gi, + description: "Detects hellos in project.", + violation_message: "sample violation message", + name: "NoHellos", + type: RuleType.Standard, + severityLevel: SeverityLevel.Moderate, + tags: ['Recommended'], + resourceUrls: [] } }; @@ -48,6 +58,15 @@ const EXPECTED_NoTodos_RULE_DESCRIPTION = { resourceUrls: [] }; +const EXPECTED_NoHellos_RULE_DESCRIPTION = { + name: "NoHellos", + severityLevel: SeverityLevel.Moderate, + type: RuleType.Standard, + tags: ["Recommended"], + description: "Detects hellos in project.", + resourceUrls: [] +}; + describe("Tests for RegexEngine's getName and describeRules methods", () => { let engine: RegexEngine; beforeAll(() => { @@ -64,34 +83,38 @@ describe("Tests for RegexEngine's getName and describeRules methods", () => { it('Calling describeRules without workspace, returns all available rules', async () => { const rulesDescriptions: RuleDescription[] = await engine.describeRules({}); - expect(rulesDescriptions).toHaveLength(2); + expect(rulesDescriptions).toHaveLength(3); expect(rulesDescriptions[0]).toMatchObject(EXPECTED_NoTrailingWhitespace_RULE_DESCRIPTION); expect(rulesDescriptions[1]).toMatchObject(EXPECTED_NoTodos_RULE_DESCRIPTION); + expect(rulesDescriptions[2]).toMatchObject(EXPECTED_NoHellos_RULE_DESCRIPTION); }); it("When workspace contains zero applicable files, then describeRules returns no rules", async () => { const rulesDescriptions: RuleDescription[] = await engine.describeRules({workspace: new Workspace([ - path.resolve(__dirname, 'test-data', 'sampleWorkspace', 'dummy2.ts') // .ts files aren't applicable to the rules + path.resolve(__dirname, 'test-data', 'workspaceWithNoTextFiles') // ])}); expect(rulesDescriptions).toHaveLength(0); }); - it("When workspace contains files only applicable to one of the rules, then describeRules only returns that one rule", async () => { + it("When workspace contains files only applicable to only some of the rules, then describeRules only returns those rules", async () => { const rulesDescriptions: RuleDescription[] = await engine.describeRules({workspace: new Workspace([ path.resolve(__dirname, 'test-data', 'sampleWorkspace', 'dummy3.js') ])}); - expect(rulesDescriptions).toHaveLength(1); + expect(rulesDescriptions).toHaveLength(2); expect(rulesDescriptions[0]).toMatchObject(EXPECTED_NoTodos_RULE_DESCRIPTION); + expect(rulesDescriptions[1]).toMatchObject(EXPECTED_NoHellos_RULE_DESCRIPTION); }); it("When workspace contains files are applicable to all available rules, then describeRules returns all rules", async () => { const rulesDescriptions: RuleDescription[] = await engine.describeRules({workspace: new Workspace([ path.resolve(__dirname, 'test-data', 'sampleWorkspace') ])}); - expect(rulesDescriptions).toHaveLength(2); + expect(rulesDescriptions).toHaveLength(3); expect(rulesDescriptions[0]).toMatchObject(EXPECTED_NoTrailingWhitespace_RULE_DESCRIPTION); expect(rulesDescriptions[1]).toMatchObject(EXPECTED_NoTodos_RULE_DESCRIPTION); + expect(rulesDescriptions[2]).toMatchObject(EXPECTED_NoHellos_RULE_DESCRIPTION) }); + }); describe('Tests for runRules', () => { @@ -113,7 +136,7 @@ describe('Tests for runRules', () => { expect(runResults.violations).toHaveLength(0); }); - it("Ensure runRules when called on a list Apex classes, properly emits violations", async () => { + it("Ensure runRules when called on a list Apex classes, properly emits violations", async () => { const runOptions: RunOptions = {workspace: new Workspace([ path.resolve(__dirname, "test-data", "apexClassWhitespace") ])}; @@ -182,7 +205,7 @@ describe('Tests for runRules', () => { const runOptions: RunOptions = {workspace: new Workspace([ path.resolve(__dirname, "test-data", "sampleWorkspace") ])}; - const runResults: EngineRunResults = await engine.runRules(["NoTodos"], runOptions); + const runResults: EngineRunResults = await engine.runRules(["NoTodos", "NoHellos"], runOptions); const expectedViolations: Violation[] = [ { @@ -212,7 +235,36 @@ describe('Tests for runRules', () => { endColumn: 22 } ] + }, + { + ruleName: "NoHellos", + message: "sample violation message", + primaryLocationIndex: 0, + codeLocations: [ + { + file: path.resolve(__dirname, "test-data", "sampleWorkspace", "dummy3.js"), + startLine: 3, + startColumn: 29, + endLine: 3, + endColumn: 34 + } + ] + }, + { + ruleName: "NoHellos", + message: "sample violation message", + primaryLocationIndex: 0, + codeLocations: [ + { + file: path.resolve(__dirname, "test-data", "sampleWorkspace", "dummy4.txt"), + startLine: 1, + startColumn: 1, + endLine: 1, + endColumn: 6 + } + ] } + ]; expect(runResults.violations).toHaveLength(expectedViolations.length); diff --git a/packages/code-analyzer-regex-engine/test/plugin.test.ts b/packages/code-analyzer-regex-engine/test/plugin.test.ts index 0d970cb6..43d3bbb0 100644 --- a/packages/code-analyzer-regex-engine/test/plugin.test.ts +++ b/packages/code-analyzer-regex-engine/test/plugin.test.ts @@ -18,6 +18,10 @@ const SAMPLE_RAW_CUSTOM_RULE_DEFINITION = { file_extensions: [".js", ".ts", ".cls"] }; +const SAMPLE_RAW_CUSTOM_RULE_NO_FILE_EXTS_DEFINITION = { + regex: String.raw`/hello/gi`, + description: "Detects hellos in project", +} describe('RegexEnginePlugin No Custom Config Tests' , () => { let enginePlugin: RegexEnginePlugin; beforeAll(() => { @@ -49,23 +53,35 @@ describe('RegexEnginePlugin Custom Config Tests', () => { it("When valid custom rules are provided, then they are appended to base rules", async () => { const rawConfig: ConfigObject = { custom_rules: { - NoTodos: SAMPLE_RAW_CUSTOM_RULE_DEFINITION + NoTodos: SAMPLE_RAW_CUSTOM_RULE_DEFINITION, + NoHellos: SAMPLE_RAW_CUSTOM_RULE_NO_FILE_EXTS_DEFINITION } }; const engine: RegexEngine = await enginePlugin.createEngine('regex', rawConfig) as RegexEngine; - const customRuleRegex: RegExp = /TODO:\s/gi + const customNoTodoRuleRegex: RegExp = /TODO:\s/gi; + const customNoHelloRuleRegex: RegExp = /hello/gi; const expRegexRules: RegexRules = { ...BASE_REGEX_RULES, NoTodos: { - regex: customRuleRegex, + regex: customNoTodoRuleRegex, description: SAMPLE_RAW_CUSTOM_RULE_DEFINITION.description, file_extensions: SAMPLE_RAW_CUSTOM_RULE_DEFINITION.file_extensions, - violation_message: getMessage('RuleViolationMessage', customRuleRegex.toString(), 'NoTodos', 'Detects TODO comments in code base.'), + violation_message: getMessage('RuleViolationMessage', customNoTodoRuleRegex.toString(), 'NoTodos', 'Detects TODO comments in code base.'), name: "NoTodos", type: RuleType.Standard, severityLevel: SeverityLevel.Moderate, tags: ['Recommended'], resourceUrls: [] + }, + NoHellos: { + regex: customNoHelloRuleRegex, + description: SAMPLE_RAW_CUSTOM_RULE_NO_FILE_EXTS_DEFINITION.description, + violation_message: getMessage('RuleViolationMessage', customNoHelloRuleRegex.toString(), 'NoHellos', 'Detects hellos in project'), + name: "NoHellos", + type: RuleType.Standard, + severityLevel: SeverityLevel.Moderate, + tags: ['Recommended'], + resourceUrls: [] } }; expect(engine._getRegexRules()).toEqual(expRegexRules); diff --git a/packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy3.js b/packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy3.js index 687926ca..bc22f56c 100644 --- a/packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy3.js +++ b/packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy3.js @@ -1,3 +1,3 @@ /*TODO: write code */ -const a = 3; // TODO: oops \ No newline at end of file +const a = 3; // TODO: oops, hello \ No newline at end of file diff --git a/packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy4.txt b/packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy4.txt new file mode 100644 index 00000000..b6fc4c62 --- /dev/null +++ b/packages/code-analyzer-regex-engine/test/test-data/sampleWorkspace/dummy4.txt @@ -0,0 +1 @@ +hello \ No newline at end of file diff --git a/packages/code-analyzer-regex-engine/test/test-data/workspaceWithNoTextFiles/blank.exe b/packages/code-analyzer-regex-engine/test/test-data/workspaceWithNoTextFiles/blank.exe new file mode 100644 index 0000000000000000000000000000000000000000..cb43b5ce1342e5d73830ac8b6a37ea870fae2632 GIT binary patch literal 10 KcmZQzfB^si3IG8B literal 0 HcmV?d00001 diff --git a/packages/code-analyzer-retirejs-engine/src/utils.ts b/packages/code-analyzer-retirejs-engine/src/utils.ts index bcfaf252..4bc4c231 100644 --- a/packages/code-analyzer-retirejs-engine/src/utils.ts +++ b/packages/code-analyzer-retirejs-engine/src/utils.ts @@ -59,6 +59,7 @@ export function isZipFile(file: string) { * Determines if a file is a non-binary text file * @param file a file path or the Buffer of its contents */ +/* TODO: Make this function async */ export function isTextFile(file: string | Buffer): boolean { return !isBinaryFileSync(file); }