From 1a192f1e3d0e30e69be744fa0fbb1a5ad6866786 Mon Sep 17 00:00:00 2001 From: Jeff Bartolotta Date: Tue, 7 Jul 2020 19:23:48 -0700 Subject: [PATCH 1/3] Filter out invalid catalog files Handle situations where the json catalogs are out of sync with the file system because the referenced file no longer exists. Skip the file and show the user a warning. --- messages/EventKeyTemplates.js | 5 +- src/lib/pmd/PmdCatalogWrapper.ts | 16 ++++-- test/lib/pmd/PmdCatalogWrapper.test.ts | 69 ++++++++++++++++++++++---- 3 files changed, 75 insertions(+), 15 deletions(-) diff --git a/messages/EventKeyTemplates.js b/messages/EventKeyTemplates.js index 2b9151b1b..13f918b10 100644 --- a/messages/EventKeyTemplates.js +++ b/messages/EventKeyTemplates.js @@ -8,8 +8,9 @@ module.exports = { "invalidCategorySkipped": "Cataloger skipped invalid PMD Category file '%s'.", "invalidRulesetSkipped": "Cataloger skipped invalid PMD Ruleset file '%s'.", "xmlDropped": "Dropping XML file [%s] since its path does not conform to Rulesets or Category.", - "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s" - }, + "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s", + "catalogFileNotFound": "Catalog file [%s] for language [%s] was not found.", +}, "error": { "internal": { "unexpectedError": "Unexpected error occurred while cataloging rules: %s", diff --git a/src/lib/pmd/PmdCatalogWrapper.ts b/src/lib/pmd/PmdCatalogWrapper.ts index 7a55a7449..1a8ee8114 100644 --- a/src/lib/pmd/PmdCatalogWrapper.ts +++ b/src/lib/pmd/PmdCatalogWrapper.ts @@ -9,6 +9,7 @@ import {OutputProcessor} from './OutputProcessor'; import * as PmdLanguageManager from './PmdLanguageManager'; import {PMD_LIB, PMD_VERSION, PmdSupport} from './PmdSupport'; import path = require('path'); +import {uxEvents} from '../ScannerEvents'; Messages.importMessagesDirectory(__dirname); const messages = Messages.loadMessages('@salesforce/sfdx-scanner', 'EventKeyTemplates'); @@ -98,8 +99,11 @@ export class PmdCatalogWrapper extends PmdSupport { const pathSetMap = new Map>(); const customPathEntries: Map> = await this.getCustomRulePathEntries(); + const fileHandler = new FileHandler(); + + // Iterate through the custom paths. - customPathEntries.forEach(async (paths: Set, langKey: string) => { + for (const [langKey, paths] of customPathEntries) { // If the language by which these paths are mapped can be de-aliased into one of PMD's default-supported // languages, we should use the name PMD recognizes. That way, if they have custom paths for 'ecmascript' // and 'js', we'll turn both of those into 'javascript'. @@ -112,11 +116,17 @@ export class PmdCatalogWrapper extends PmdSupport { const pathSet = pathSetMap.get(lang) || new Set(); if (paths) { for (const value of paths.values()) { - pathSet.add(value); + const exists = (await fileHandler.exists(value)); + if (exists) { + pathSet.add(value); + } else { + // The catalog file may have been deleted or moved. Show the user a warning. + uxEvents.emit('warning-always', messages.getMessage('warning.catalogFileNotFound', [value, lang])); + } } } pathSetMap.set(lang, pathSet); - }); + } // Now, we'll want to add the default PMD JARs for any activated languages. const supportedLanguages = await PmdLanguageManager.getSupportedLanguages(); diff --git a/test/lib/pmd/PmdCatalogWrapper.test.ts b/test/lib/pmd/PmdCatalogWrapper.test.ts index 5ded08498..047af07fb 100644 --- a/test/lib/pmd/PmdCatalogWrapper.test.ts +++ b/test/lib/pmd/PmdCatalogWrapper.test.ts @@ -5,7 +5,10 @@ import {Config} from '../../../src/lib/util/Config'; import {expect} from 'chai'; import Sinon = require('sinon'); import path = require('path'); -import { ENGINE } from '../../../src/Constants'; +import {ENGINE,LANGUAGE} from '../../../src/Constants'; +import {FileHandler} from '../../../src/lib/util/FileHandler'; +import {uxEvents} from '../../../src/lib/ScannerEvents'; + // In order to get access to PmdCatalogWrapper's protected methods, we're going to extend it with a test class here. class TestablePmdCatalogWrapper extends PmdCatalogWrapper { public async buildCommandArray(): Promise<[string, string[]]> { @@ -25,11 +28,12 @@ describe('PmdCatalogWrapper', () => { before(() => { Sinon.createSandbox(); // Spoof a config that claims that only Apex's default PMD JAR is enabled. - Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves(['apex']); + Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves([LANGUAGE.APEX]); // Spoof a CustomPathManager that claims that a custom JAR exists for Java. const customJars: Map> = new Map(); - customJars.set('java', new Set([irrelevantPath])); + customJars.set(LANGUAGE.JAVA, new Set([irrelevantPath])); Sinon.stub(CustomRulePathManager.prototype, 'getRulePathEntries').withArgs(PmdEngine.NAME).resolves(customJars); + Sinon.stub(FileHandler.prototype, 'exists').resolves(true); }); after(() => { @@ -42,7 +46,7 @@ describe('PmdCatalogWrapper', () => { const params = (await target.buildCommandArray())[1]; // Expect there to be 6 parameters. - expect(params.length).to.equal(6, 'Should have been 6 parameters'); + expect(params.length).to.equal(6, `Should have been 6 parameters: ${params}`); // Expect the first two parameters to be the catalog and -cp flag. expect(params[0]).to.equal('-DcatalogName=PmdCatalog.json', 'First parameter should be catalog override'); expect(params[1]).to.equal('-cp', 'Second parameter should be -cp flag'); @@ -61,11 +65,12 @@ describe('PmdCatalogWrapper', () => { before(() => { Sinon.createSandbox(); // Spoof a config that claims that only Apex's default PMD JAR is enabled. - Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves(['apex']); + Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves([LANGUAGE.APEX]); // Spoof a CustomPathManager that claims that a custom JAR exists for plsql, using a weird alias for that language. const customJars: Map> = new Map(); - customJars.set('Pl/SqL', new Set([irrelevantPath])); + customJars.set(LANGUAGE.PLSQL, new Set([irrelevantPath])); Sinon.stub(CustomRulePathManager.prototype, 'getRulePathEntries').withArgs(PmdEngine.NAME).resolves(customJars); + Sinon.stub(FileHandler.prototype, 'exists').resolves(true); }); after(() => { @@ -78,7 +83,7 @@ describe('PmdCatalogWrapper', () => { const params = (await target.buildCommandArray())[1]; // Expect there to be 6 parameters. - expect(params.length).to.equal(6, 'Should have been 6 parameters'); + expect(params.length).to.equal(6, `Should have been 6 parameters: ${params}`); // Expect the first two parameters to be the catalog and -cp flag. expect(params[0]).to.equal('-DcatalogName=PmdCatalog.json', 'First parameter should be catalog override'); expect(params[1]).to.equal('-cp', 'Second parameter should be -cp flag'); @@ -97,11 +102,12 @@ describe('PmdCatalogWrapper', () => { before(() => { Sinon.createSandbox(); // Spoof a config that claims that only apex is the supported language - Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves(['apex']); + Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves([LANGUAGE.APEX]); const customJars: Map> = new Map(); - customJars.set('pl/sql', new Set([irrelevantPath])); - customJars.set('java', new Set()); + customJars.set(LANGUAGE.PLSQL, new Set([irrelevantPath])); + customJars.set(LANGUAGE.JAVA, new Set()); Sinon.stub(CustomRulePathManager.prototype, 'getRulePathEntries').withArgs(PmdEngine.NAME).resolves(customJars); + Sinon.stub(FileHandler.prototype, 'exists').resolves(true); }); after(() => { @@ -126,6 +132,49 @@ describe('PmdCatalogWrapper', () => { expect(params[5]).to.match(/^apex=.*jar$/, 'Sixth parameter should be apex-specific'); }); }); + + describe('Missing Rule Files are Handled Gracefully', () => { + const validJar = 'jar-that-exists.jar'; + const missingJar = 'jar-that-is-missing.jar'; + // This jar is automatically included by the PmdCatalogWrapper + const pmdJar = path.resolve(path.join('dist', 'pmd', 'lib', 'pmd-java-6.22.0.jar')); + let uxSpy = null; + + before(() => { + Sinon.createSandbox(); + Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves([LANGUAGE.JAVA]); + const customJars: Map> = new Map(); + // Simulate CustomPaths.json contains a jar that has been deleted or moved + customJars.set(LANGUAGE.JAVA, new Set([validJar, missingJar])); + Sinon.stub(CustomRulePathManager.prototype, 'getRulePathEntries').withArgs(PmdEngine.NAME).resolves(customJars); + const stub = Sinon.stub(FileHandler.prototype, 'exists'); + stub.withArgs(validJar).resolves(true); + stub.withArgs(missingJar).resolves(false); + uxSpy = Sinon.spy(uxEvents, 'emit'); + }); + + after(() => { + Sinon.restore(); + }); + + it('Missing file should be filtered out and display warning', async () => { + const target = await TestablePmdCatalogWrapper.create({}); + const entries = await target.getRulePathEntries(); + + // The rule path entries should only include the jar that exists + expect(entries.size).to.equal(1, `Entries: ${Array.from(entries.keys())}`); + const jars = entries.get(LANGUAGE.JAVA); + const jarsErrorMessage = `Jars: ${Array.from(jars)}`; + expect(jars.size).to.equal(2, jarsErrorMessage); + expect(jars).to.contain(validJar, jarsErrorMessage); + expect(jars).to.contain(pmdJar, jarsErrorMessage); + expect(jars).to.not.contain(missingJar, jarsErrorMessage); + + // A warning should be displayed + Sinon.assert.calledOnce(uxSpy); + Sinon.assert.calledWith(uxSpy, 'warning-always', `Catalog file [${missingJar}] for language [${LANGUAGE.JAVA}] was not found.`); + }); + }); }); }); }); From 252df638dd0f9985ebe4b19efd1ef560505cc031 Mon Sep 17 00:00:00 2001 From: Jeff Bartolotta Date: Tue, 7 Jul 2020 19:36:15 -0700 Subject: [PATCH 2/3] Code style cleanup Removed unnecessary temp variable. --- src/lib/pmd/PmdCatalogWrapper.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib/pmd/PmdCatalogWrapper.ts b/src/lib/pmd/PmdCatalogWrapper.ts index 1a8ee8114..cc830d1cd 100644 --- a/src/lib/pmd/PmdCatalogWrapper.ts +++ b/src/lib/pmd/PmdCatalogWrapper.ts @@ -116,8 +116,7 @@ export class PmdCatalogWrapper extends PmdSupport { const pathSet = pathSetMap.get(lang) || new Set(); if (paths) { for (const value of paths.values()) { - const exists = (await fileHandler.exists(value)); - if (exists) { + if (await fileHandler.exists(value)) { pathSet.add(value); } else { // The catalog file may have been deleted or moved. Show the user a warning. From c8b09e69f55840a7db2c1fb7f91ffc2b14ee5629 Mon Sep 17 00:00:00 2001 From: Jeff Bartolotta Date: Wed, 8 Jul 2020 13:16:57 -0700 Subject: [PATCH 3/3] Changes based on review Changed error message Restore unit tests that shouldn't use constants Use .entries() to iterate over map contents --- messages/EventKeyTemplates.js | 2 +- src/lib/pmd/PmdCatalogWrapper.ts | 6 +++--- test/lib/pmd/PmdCatalogWrapper.test.ts | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/messages/EventKeyTemplates.js b/messages/EventKeyTemplates.js index 13f918b10..65f4bebbd 100644 --- a/messages/EventKeyTemplates.js +++ b/messages/EventKeyTemplates.js @@ -9,7 +9,7 @@ module.exports = { "invalidRulesetSkipped": "Cataloger skipped invalid PMD Ruleset file '%s'.", "xmlDropped": "Dropping XML file [%s] since its path does not conform to Rulesets or Category.", "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s", - "catalogFileNotFound": "Catalog file [%s] for language [%s] was not found.", + "customRuleFileNotFound": "Custom rule file path [%s] for language [%s] was not found.", }, "error": { "internal": { diff --git a/src/lib/pmd/PmdCatalogWrapper.ts b/src/lib/pmd/PmdCatalogWrapper.ts index cc830d1cd..497592d75 100644 --- a/src/lib/pmd/PmdCatalogWrapper.ts +++ b/src/lib/pmd/PmdCatalogWrapper.ts @@ -98,12 +98,12 @@ export class PmdCatalogWrapper extends PmdSupport { protected async getRulePathEntries(): Promise>> { const pathSetMap = new Map>(); - const customPathEntries: Map> = await this.getCustomRulePathEntries(); + const customRulePaths: Map> = await this.getCustomRulePathEntries(); const fileHandler = new FileHandler(); // Iterate through the custom paths. - for (const [langKey, paths] of customPathEntries) { + for (const [langKey, paths] of customRulePaths.entries()) { // If the language by which these paths are mapped can be de-aliased into one of PMD's default-supported // languages, we should use the name PMD recognizes. That way, if they have custom paths for 'ecmascript' // and 'js', we'll turn both of those into 'javascript'. @@ -120,7 +120,7 @@ export class PmdCatalogWrapper extends PmdSupport { pathSet.add(value); } else { // The catalog file may have been deleted or moved. Show the user a warning. - uxEvents.emit('warning-always', messages.getMessage('warning.catalogFileNotFound', [value, lang])); + uxEvents.emit('warning-always', messages.getMessage('warning.customRuleFileNotFound', [value, lang])); } } } diff --git a/test/lib/pmd/PmdCatalogWrapper.test.ts b/test/lib/pmd/PmdCatalogWrapper.test.ts index 047af07fb..2655babf2 100644 --- a/test/lib/pmd/PmdCatalogWrapper.test.ts +++ b/test/lib/pmd/PmdCatalogWrapper.test.ts @@ -68,7 +68,7 @@ describe('PmdCatalogWrapper', () => { Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves([LANGUAGE.APEX]); // Spoof a CustomPathManager that claims that a custom JAR exists for plsql, using a weird alias for that language. const customJars: Map> = new Map(); - customJars.set(LANGUAGE.PLSQL, new Set([irrelevantPath])); + customJars.set('Pl/SqL', new Set([irrelevantPath])); Sinon.stub(CustomRulePathManager.prototype, 'getRulePathEntries').withArgs(PmdEngine.NAME).resolves(customJars); Sinon.stub(FileHandler.prototype, 'exists').resolves(true); }); @@ -104,7 +104,7 @@ describe('PmdCatalogWrapper', () => { // Spoof a config that claims that only apex is the supported language Sinon.stub(Config.prototype, 'getSupportedLanguages').withArgs(ENGINE.PMD).resolves([LANGUAGE.APEX]); const customJars: Map> = new Map(); - customJars.set(LANGUAGE.PLSQL, new Set([irrelevantPath])); + customJars.set('pl/sql', new Set([irrelevantPath])); customJars.set(LANGUAGE.JAVA, new Set()); Sinon.stub(CustomRulePathManager.prototype, 'getRulePathEntries').withArgs(PmdEngine.NAME).resolves(customJars); Sinon.stub(FileHandler.prototype, 'exists').resolves(true); @@ -172,7 +172,7 @@ describe('PmdCatalogWrapper', () => { // A warning should be displayed Sinon.assert.calledOnce(uxSpy); - Sinon.assert.calledWith(uxSpy, 'warning-always', `Catalog file [${missingJar}] for language [${LANGUAGE.JAVA}] was not found.`); + Sinon.assert.calledWith(uxSpy, 'warning-always', `Custom rule file path [${missingJar}] for language [${LANGUAGE.JAVA}] was not found.`); }); }); });