IOC to abstract rule engines from rule manager code

package.json

@@ -12,7 +12,9 @@
 		"@salesforce/core": "^2.1.6",
 		"find-java-home": "^1.1.0",
 		"globby": "^11.0.0",
+		"inversify": "^5.0.1",
 		"normalize-path": "^3.0.0",
+		"reflect-metadata": "^0.1.13",
 		"ts-node": "^8",
 		"tslib": "^1",
 		"typescript": "^3.8.2",
@@ -26,8 +28,8 @@
 		"@oclif/test": "^1",
 		"@salesforce/dev-config": "1.5.0",
 		"@types/chai": "^4",
-		"@types/mocha": "^5",
-		"@types/node": "^13.1.8",
+		"@types/mocha": "^7.0.2",
+		"@types/node": "^13.11.1",
 		"@typescript-eslint/eslint-plugin": "^2.21.0",
 		"@typescript-eslint/parser": "^2.21.0",
 		"chai": "^4",

src/commands/scanner/rule/add.ts

@@ -48,7 +48,7 @@ export default class Add extends SfdxCommand {
 		this.logger.trace(`Rule path: ${path}`);
 
 		// Add to Custom Classpath registry
-		const manager = await CustomRulePathManager.create({});
+		const manager = await CustomRulePathManager.create();
 		const classpathEntries = await manager.addPathsForLanguage(language, path);
 		this.ux.log(`Successfully added rules for ${language}.`);
 		this.ux.log(`${classpathEntries.length} Path(s) added: ${classpathEntries}`);

src/commands/scanner/rule/describe.ts

@@ -39,7 +39,7 @@ export default class Describe extends ScannerCommand {
 		const ruleFilters = this.buildRuleFilters();
 		// It's possible for this line to throw an error, but that's fine because the error will be an SfdxError that we can
 		// allow to boil over.
-		const ruleManager = await RuleManager.create({});
+		const ruleManager = await RuleManager.create();
 		const rules = await ruleManager.getRulesMatchingCriteria(ruleFilters);
 		if (rules.length === 0) {
 			// If we couldn't find any rules that fit the criteria, we'll let the user know. We'll use .warn() instead of .log()

src/commands/scanner/rule/list.ts

@@ -66,7 +66,7 @@ export default class List extends ScannerCommand {
 		const ruleFilters = this.buildRuleFilters();
 		// It's possible for this line to throw an error, but that's fine because the error will be an SfdxError that we can
 		// allow to boil over.
-		const ruleManager = await RuleManager.create({});
+		const ruleManager = await RuleManager.create();
 		const rules = await ruleManager.getRulesMatchingCriteria(ruleFilters);
 		const formattedRules = this.formatRulesForDisplay(rules);
 		this.ux.table(formattedRules, columns);

src/commands/scanner/run.ts

@@ -105,7 +105,7 @@ export default class Run extends ScannerCommand {
 		// Else, default to table format.  We can't use the default attribute of the flag here because we need to differentiate
 		// between 'table' being defaulted and 'table' being explicitly chosen by the user.
 		const format: OUTPUT_FORMAT = this.flags.format || (this.flags.outfile ? this.deriveFormatFromOutfile() : OUTPUT_FORMAT.TABLE);
-		const ruleManager = await RuleManager.create({});
+		const ruleManager = await RuleManager.create();
 		// It's possible for this line to throw an error, but that's fine because the error will be an SfdxError that we can
 		// allow to boil over.
 		const output = await ruleManager.runRulesMatchingCriteria(filters, target, format);

src/ioc.config.ts

@@ -0,0 +1,12 @@
+import { Container as IOC} from "inversify";
+import "reflect-metadata";
+
+export const Services = {
+	RuleEngine: Symbol("RuleEngine")
+};
+
+import {PmdEngine} from './lib/pmd/PmdEngine';
+import {RuleEngine} from './lib/services/RuleEngine';
+
+export const Container = new IOC();
+Container.bind<RuleEngine>(Services.RuleEngine).to(PmdEngine);

src/lib/CustomRulePathManager.ts

@@ -1,29 +1,34 @@
 import path = require('path');
+import {Container,Services} from '../ioc.config';
+import {RuleEngine} from './services/RuleEngine';
 import {FileHandler} from './util/FileHandler';
 import {Logger, SfdxError} from '@salesforce/core';
-import {AsyncCreatable} from '@salesforce/kit';
 import {CUSTOM_PATHS, SFDX_SCANNER_PATH} from '../Constants';
 import * as PrettyPrinter from './util/PrettyPrinter';
 
-export enum ENGINE {
-	PMD = 'pmd'
-}
-
 type RulePathEntry = Map<string, Set<string>>;
-type RulePathMap = Map<ENGINE, RulePathEntry>;
-
-export const CUSTOM_CLASSPATH_REGISTER = path.join(SFDX_SCANNER_PATH, CUSTOM_PATHS);
-export const CUSTOM_CLASSPATH_REGISTER_TMP = path.join(SFDX_SCANNER_PATH, `Tmp${CUSTOM_PATHS}`);
+type RulePathMap = Map<string, RulePathEntry>;
 
 const EMPTY_JSON_FILE = '{}';
 
 
-export class CustomRulePathManager extends AsyncCreatable {
+export class CustomRulePathManager {
+	public static async create(): Promise<CustomRulePathManager> {
+		const engines = Container.getAll<RuleEngine>(Services.RuleEngine);
+		const manager = new CustomRulePathManager(engines);
+		await manager.init();
+		return manager;
+	}
+
 	private logger!: Logger;
+	private engines: RuleEngine[];
 	private pathsByLanguageByEngine: RulePathMap;
 	private initialized: boolean;
 	private fileHandler: FileHandler;
 
+	constructor(engines: RuleEngine[]) {
+		this.engines = engines;
+	}
 
 	protected async init(): Promise<void> {
 		this.logger = await Logger.child('CustomRulePathManager');
@@ -65,7 +70,7 @@ export class CustomRulePathManager extends AsyncCreatable {
 		}
 		// Now that we've got the file contents, let's turn it into a JSON.
 		const json = JSON.parse(data);
-		this.pathsByLanguageByEngine = this.convertJsonDataToMap(json);
+		this.pathsByLanguageByEngine = CustomRulePathManager.convertJsonDataToMap(json);
 		this.logger.trace(`Initialized CustomRulePathManager. pathsByLanguageByEngine: ${PrettyPrinter.stringifyMapOfMaps(this.pathsByLanguageByEngine)}`);
 		this.initialized = true;
 	}
@@ -77,24 +82,24 @@ export class CustomRulePathManager extends AsyncCreatable {
 		const classpathEntries = await this.expandClasspaths(paths);
 		// Identify the engine for each path and put them in the appropriate map and inner map.
 		classpathEntries.forEach((entry) => {
-			const e = this.determineEngineForPath(entry);
-			if (!this.pathsByLanguageByEngine.has(e)) {
-				this.logger.trace(`Creating new entry for engine ${e}`);
-				this.pathsByLanguageByEngine.set(e, new Map());
+			const engine = this.determineEngineForPath(entry);
+			if (!this.pathsByLanguageByEngine.has(engine.getName())) {
+				this.logger.trace(`Creating new entry for engine ${engine.getName()}`);
+				this.pathsByLanguageByEngine.set(engine.getName(), new Map());
 			}
-			if (!this.pathsByLanguageByEngine.get(e).has(language)) {
-				this.logger.trace(`Creating new entry for language ${language} in engine ${e}`);
-				this.pathsByLanguageByEngine.get(e).set(language, new Set([entry]));
+			if (!this.pathsByLanguageByEngine.get(engine.getName()).has(language)) {
+				this.logger.trace(`Creating new entry for language ${language} in engine ${engine.getName()}`);
+				this.pathsByLanguageByEngine.get(engine.getName()).set(language, new Set([entry]));
 			} else {
-				this.pathsByLanguageByEngine.get(e).get(language).add(entry);
+				this.pathsByLanguageByEngine.get(engine.getName()).get(language).add(entry);
 			}
 		});
 		// Now, write the changes to the file.
 		await this.saveCustomClasspaths();
 		return classpathEntries;
 	}
 
-	public async getRulePathEntries(engine: ENGINE): Promise<Map<string, Set<string>>> {
+	public async getRulePathEntries(engine: string): Promise<Map<string, Set<string>>> {
 		await this.initialize();
 
 		if (!this.pathsByLanguageByEngine.has(engine)) {
@@ -120,10 +125,10 @@ export class CustomRulePathManager extends AsyncCreatable {
 		}
 	}
 
-	private convertJsonDataToMap(json): RulePathMap {
+	private static convertJsonDataToMap(json): RulePathMap {
 		const map = new Map();
 		for (const key of Object.keys(json)) {
-			const engine = key as ENGINE;
+			const engine = key as string;
 			const val = json[key];
 			const innerMap = new Map();
 			for (const lang of Object.keys(val)) {
@@ -135,9 +140,8 @@ export class CustomRulePathManager extends AsyncCreatable {
 	}
 
 	// eslint-disable-next-line @typescript-eslint/no-unused-vars
-	private determineEngineForPath(path: string): ENGINE {
-		// TODO: Once we support other engines, we'll need more logic here.
-		return ENGINE.PMD;
+	private determineEngineForPath(path: string): RuleEngine {
+		return this.engines.find(e => e.matchPath(path));
 	}
 
 	private convertMapToJson(): object {

src/lib/RuleManager.ts

@@ -1,9 +1,9 @@
-import {SfdxError, Logger} from '@salesforce/core';
-import {AsyncCreatable} from '@salesforce/kit';
+import {Logger, SfdxError} from '@salesforce/core';
+import {multiInject} from 'inversify';
+import {Container, Services} from '../ioc.config';
 import {Rule} from '../types';
-import {PmdCatalogWrapper} from './pmd/PmdCatalogWrapper';
-import PmdWrapper from './pmd/PmdWrapper';
 import {RuleResultRecombinator} from './RuleResultRecombinator';
+import {RuleEngine} from './services/RuleEngine';
 import * as PrettyPrinter from './util/PrettyPrinter';
 
 export enum RULE_FILTER_TYPE {
@@ -30,13 +30,27 @@ export class RuleFilter {
 	}
 }
 
-export class RuleManager extends AsyncCreatable {
-	private pmdCatalogWrapper: PmdCatalogWrapper;
+export class RuleManager {
+	public static async create(): Promise<RuleManager> {
+		const engines = Container.getAll<RuleEngine>(Services.RuleEngine);
+		const manager = new RuleManager(engines);
+		await manager.init();
+		return manager;
+	}
+
 	private logger: Logger;
+	private readonly engines: RuleEngine[] = [];
+
+
+	constructor(@multiInject("RuleEngine") engines: RuleEngine[]) {
+		this.engines = engines;
+	}
 
 	protected async init(): Promise<void> {
-		this.pmdCatalogWrapper = await PmdCatalogWrapper.create({});
 		this.logger = await Logger.child('RuleManager');
+		for (const e of this.engines) {
+			await e.init();
+		}
 	}
 
 	public async getRulesMatchingCriteria(filters: RuleFilter[]): Promise<Rule[]> {
@@ -53,70 +67,29 @@ export class RuleManager extends AsyncCreatable {
 	}
 
 	public async runRulesMatchingCriteria(filters: RuleFilter[], target: string[] | string, format: OUTPUT_FORMAT): Promise<string> {
-		// If target is a string, it means it's an alias for an org, instead of a bunch of local filepaths. We can't handle
+		// If target is a string, it means it's an alias for an org, instead of a bunch of local file paths. We can't handle
 		// running rules against an org yet, so we'll just throw an error for now.
 		if (typeof target === 'string') {
 			throw new SfdxError('Running rules against orgs is not yet supported');
 		}
-		// TODO: Eventually, we'll need a bunch more promises to run rules existing in other engines.
-		const [pmdResults] = await Promise.all([this.runPmdRulesMatchingCriteria(filters, target)]);
-		this.logger.trace(`Received rule violations: ${pmdResults}`);
+		const ps = this.engines.map(e => e.run(filters, target));
+		const [results] = await Promise.all(ps);
+		this.logger.trace(`Received rule violations: ${results}`);
 
 		// Once all of the rules finish running, we'll need to combine their results into a single set of the desired type,
 		// which we can then return.
 		this.logger.trace(`Recombining results into requested format ${format}`);
-		return RuleResultRecombinator.recombineAndReformatResults([pmdResults], format);
+		return RuleResultRecombinator.recombineAndReformatResults([results], format);
 	}
 
 	private async getAllRules(): Promise<Rule[]> {
 		this.logger.trace('Getting all rules.');
 
-		// TODO: Eventually, we'll need a bunch more promises to load rules from their source files in other engines.
-		const [pmdRules]: Rule[][] = await Promise.all([this.getPmdRules()]);
-		return [...pmdRules];
-	}
-
-	private async getPmdRules(): Promise<Rule[]> {
-		// PmdCatalogWrapper is a layer of abstraction between the commands and PMD, facilitating code reuse and other goodness.
-		this.logger.trace('Getting PMD rules.');
-		const catalog = await this.pmdCatalogWrapper.getCatalog();
-		return catalog.rules;
+		const ps = this.engines.map(e => e.getAll());
+		const [rules]: Rule[][] = await Promise.all(ps);
+		return [...rules];
 	}
 
-	private async runPmdRulesMatchingCriteria(filters: RuleFilter[], target: string[]): Promise<string> {
-		this.logger.trace(`About to run PMD rules. Target count: ${target.length}, filter count: ${filters.length}`);
-		try {
-			// Convert our filters into paths that we can feed back into PMD.
-			const paths: string[] = await this.pmdCatalogWrapper.getPathsMatchingFilters(filters);
-			// If we didn't find any paths, we're done.
-			if (paths == null || paths.length === 0) {
-				this.logger.trace('No Rule paths found. Nothing to execute.')
-				return '';
-			}
-			// Otherwise, run PMD and see what we get.
-			// TODO: Weird translation to next layer. target=path and path=rule path. Consider renaming
-			const [violationsFound, stdout] = await PmdWrapper.execute(target.join(','), paths.join(','));
-
-			if (violationsFound) {
-				this.logger.trace('Found rule violations.');
-				// If we found any violations, they'll be in an XML document somewhere in stdout, which we'll need to find and process.
-				const xmlStart = stdout.indexOf('<?xml');
-				const xmlEnd = stdout.lastIndexOf('</pmd>') + 6;
-				const ruleViolationsXml = stdout.slice(xmlStart, xmlEnd);
-
-				this.logger.trace(`Rule violations in the original XML format: ${ruleViolationsXml}`);
-				return ruleViolationsXml;
-			} else {
-				// If we didn't find any violations, we can just return an empty string.
-				this.logger.trace('No rule violations found.');
-				return '';
-			}
-		} catch (e) {
-			throw new SfdxError(e.message || e);
-		}
-	}
-
-
 	private ruleSatisfiesFilterConstraints(rule: Rule, filters: RuleFilter[]): boolean {
 		// If no filters were provided, then the rule is vacuously acceptable and we can just return true.
 		if (filters == null || filters.length === 0) {

src/lib/pmd/PmdEngine.ts

@@ -0,0 +1,71 @@
+import {Logger, SfdxError} from '@salesforce/core';
+import {injectable} from 'inversify';
+import {Rule} from '../../types';
+import {RuleFilter} from '../RuleManager';
+import {RuleEngine} from '../services/RuleEngine';
+import {PmdCatalogWrapper} from './PmdCatalogWrapper';
+import PmdWrapper from './PmdWrapper';
+
+@injectable()
+export class PmdEngine implements RuleEngine {
+	public static NAME = "pmd";
+
+	private logger: Logger;
+	private pmdCatalogWrapper: PmdCatalogWrapper;
+
+	public getName(): string {
+		return PmdEngine.NAME;
+	}
+
+	public matchPath(path: string): boolean {
+		// TODO implement this for realz
+		return path != null;
+	}
+
+	public async init(): Promise<RuleEngine> {
+		this.pmdCatalogWrapper = await PmdCatalogWrapper.create({});
+		this.logger = await Logger.child('PmdEngine');
+		return this;
+	}
+
+	public async getAll(): Promise<Rule[]> {
+		// PmdCatalogWrapper is a layer of abstraction between the commands and PMD, facilitating code reuse and other goodness.
+		this.logger.trace('Getting PMD rules.');
+		const catalog = await this.pmdCatalogWrapper.getCatalog();
+		return catalog.rules;
+	}
+
+	public async run(filters: RuleFilter[], target: string[]): Promise<string> {
+		this.logger.trace(`About to run PMD rules. Target count: ${target.length}, filter count: ${filters.length}`);
+		try {
+			// Convert our filters into paths that we can feed back into PMD.
+			const paths: string[] = await this.pmdCatalogWrapper.getPathsMatchingFilters(filters);
+			// If we didn't find any paths, we're done.
+			if (paths == null || paths.length === 0) {
+				this.logger.trace('No Rule paths found. Nothing to execute.');
+				return '';
+			}
+			// Otherwise, run PMD and see what we get.
+			// TODO: Weird translation to next layer. target=path and path=rule path. Consider renaming
+			const [violationsFound, stdout] = await PmdWrapper.execute(target.join(','), paths.join(','));
+
+			if (violationsFound) {
+				this.logger.trace('Found rule violations.');
+				// If we found any violations, they'll be in an XML document somewhere in stdout, which we'll need to find and process.
+				const xmlStart = stdout.indexOf('<?xml');
+				const xmlEnd = stdout.lastIndexOf('</pmd>') + 6;
+				const ruleViolationsXml = stdout.slice(xmlStart, xmlEnd);
+
+				this.logger.trace(`Rule violations in the original XML format: ${ruleViolationsXml}`);
+				return ruleViolationsXml;
+			} else {
+				// If we didn't find any violations, we can just return an empty string.
+				this.logger.trace('No rule violations found.');
+				return '';
+			}
+		} catch (e) {
+			throw new SfdxError(e.message || e);
+		}
+	}
+
+}

src/lib/pmd/PmdSupport.ts

@@ -1,7 +1,8 @@
 import childProcess = require('child_process');
 import {ChildProcessWithoutNullStreams} from 'child_process';
 import {AsyncCreatable} from '@salesforce/kit';
-import {CustomRulePathManager, ENGINE} from '../CustomRulePathManager';
+import {CustomRulePathManager} from '../CustomRulePathManager';
+import {PmdEngine} from './PmdEngine';
 import path = require('path');
 
 export const PMD_VERSION = '6.22.0';
@@ -54,7 +55,7 @@ export abstract class PmdSupport extends AsyncCreatable {
 	}
 
 	protected async getRulePathEntries(): Promise<Map<string, Set<string>>> {
-		const customRulePathManager = await CustomRulePathManager.create({});
-		return await customRulePathManager.getRulePathEntries(ENGINE.PMD);
+		const customRulePathManager = await CustomRulePathManager.create();
+		return await customRulePathManager.getRulePathEntries(PmdEngine.NAME);
 	}
 }