NEW(cpd): @W-16866916@: Add minimum_tokens and skip_duplicate_files to cpd engine config (#126)

stephen-carter-at-sf · web-flow · commit fb070202b0be · 2024-11-13T16:57:13.000-05:00
diff --git a/packages/code-analyzer-pmd-engine/src/config.ts b/packages/code-analyzer-pmd-engine/src/config.ts
@@ -27,6 +27,8 @@ export const CPD_ENGINE_CONFIG_DESCRIPTION: ConfigDescription = {
     fieldDescriptions: {
         java_command: getMessage('SharedConfigFieldDescription_java_command', CPD_ENGINE_NAME),
         rule_languages: getMessage('CpdConfigFieldDescription_rule_languages', toAvailableLanguagesText(CPD_AVAILABLE_LANGUAGES)),
+        minimum_tokens: getMessage('CpdConfigFieldDescription_minimum_tokens'),
+        skip_duplicate_files: getMessage('CpdConfigFieldDescription_skip_duplicate_files')
     }
 }
 
@@ -75,11 +77,22 @@ export type CpdEngineConfig = {
     // List of languages associated with CPD to be made available for 'cpd' engine rule selection.
     // The languages that you may choose from are: 'apex', 'html', 'javascript' (or 'ecmascript'), 'typescript', 'visualforce', 'xml'
     rule_languages: string[]
+
+    // The minimum number of tokens required to be in a duplicate block of code in order to be reported as a violation.
+    // The concept of a token may be defined differently per language, but in general it a distinct basic element of source code.
+    // For example, this could be language specific keywords, identifiers, operators, literals, and more.
+    // See https://docs.pmd-code.org/latest/pmd_userdocs_cpd.html to learn more.
+    minimum_tokens: number
+
+    // Indicates whether to ignore multiple copies of files of the same name and length.
+    skip_duplicate_files: boolean
 }
 
 export const DEFAULT_CPD_ENGINE_CONFIG: CpdEngineConfig = {
     java_command: DEFAULT_JAVA_COMMAND,
-    rule_languages: ['apex', 'html', 'javascript', 'typescript', 'visualforce', 'xml']
+    rule_languages: ['apex', 'html', 'javascript', 'typescript', 'visualforce', 'xml'],
+    minimum_tokens: 100,
+    skip_duplicate_files: false
 }
 
 export async function validateAndNormalizePmdConfig(configValueExtractor: ConfigValueExtractor,
@@ -103,7 +116,9 @@ export async function validateAndNormalizeCpdConfig(configValueExtractor: Config
 
     return {
         java_command: await cpdConfigValueExtractor.extractJavaCommand(),
-        rule_languages: cpdConfigValueExtractor.extractRuleLanguages()
+        rule_languages: cpdConfigValueExtractor.extractRuleLanguages(),
+        minimum_tokens: cpdConfigValueExtractor.extractMinimumTokens(),
+        skip_duplicate_files: cpdConfigValueExtractor.extractSkipDuplicateFiles()
     }
 }
 
@@ -278,6 +293,18 @@ class CpdConfigValueExtractor extends SharedConfigValueExtractor {
     protected getDefaultRuleLanguages(): string[] {
         return DEFAULT_CPD_ENGINE_CONFIG.rule_languages;
     }
+
+    extractMinimumTokens(): number {
+        const minimumTokens: number = this.configValueExtractor.extractNumber('minimum_tokens', DEFAULT_CPD_ENGINE_CONFIG.minimum_tokens)!;
+        if (minimumTokens <= 0 || Math.floor(minimumTokens) != minimumTokens) {
+            throw new Error(getMessage('InvalidPositiveInteger', this.configValueExtractor.getFieldPath('minimum_tokens')));
+        }
+        return minimumTokens;
+    }
+
+    extractSkipDuplicateFiles(): boolean {
+        return this.configValueExtractor.extractBoolean('skip_duplicate_files', DEFAULT_CPD_ENGINE_CONFIG.skip_duplicate_files)!;
+    }
 }
 
 function toAvailableLanguagesText(languages: string[]): string {
diff --git a/packages/code-analyzer-pmd-engine/src/cpd-engine.ts b/packages/code-analyzer-pmd-engine/src/cpd-engine.ts
@@ -29,9 +29,7 @@ const RULE_NAME_PREFIX: string = 'DetectCopyPasteFor';
 
 export class CpdEngine extends Engine {
     private readonly cpdWrapperInvoker: CpdWrapperInvoker;
-    private readonly selectedLanguages: LanguageId[];
-    private readonly minimumTokens: number;
-    private readonly skipDuplicateFiles: boolean;
+    private readonly config: CpdEngineConfig;
 
     private workspaceLiaisonCache: Map<string, WorkspaceLiaison> = new Map();
 
@@ -40,10 +38,7 @@ export class CpdEngine extends Engine {
         const javaCommandExecutor: JavaCommandExecutor = new JavaCommandExecutor(config.java_command);
         this.cpdWrapperInvoker = new CpdWrapperInvoker(javaCommandExecutor,
             (logLevel: LogLevel, message: string) => this.emitLogEvent(logLevel, message));
-
-        this.selectedLanguages = config.rule_languages as LanguageId[];
-        this.minimumTokens = 100; // Will be configurable soon
-        this.skipDuplicateFiles = false; // Will be configurable soon
+        this.config = config;
     }
 
     getName(): string {
@@ -79,8 +74,8 @@ export class CpdEngine extends Engine {
 
         const inputData: CpdRunInputData = {
             filesToScanPerLanguage: filesToScanPerLanguage,
-            minimumTokens: this.minimumTokens,
-            skipDuplicateFiles: this.skipDuplicateFiles
+            minimumTokens: this.config.minimum_tokens,
+            skipDuplicateFiles: this.config.skip_duplicate_files
         }
         this.emitRunRulesProgressEvent(5);
 
@@ -115,7 +110,7 @@ export class CpdEngine extends Engine {
     private getWorkspaceLiaison(workspace?: Workspace) : WorkspaceLiaison {
         const cacheKey: string = getCacheKey(workspace);
         if (!this.workspaceLiaisonCache.has(cacheKey)) {
-            this.workspaceLiaisonCache.set(cacheKey, new WorkspaceLiaison(workspace, this.selectedLanguages));
+            this.workspaceLiaisonCache.set(cacheKey, new WorkspaceLiaison(workspace, this.config.rule_languages as LanguageId[]));
         }
         return this.workspaceLiaisonCache.get(cacheKey)!
     }
diff --git a/packages/code-analyzer-pmd-engine/src/messages.ts b/packages/code-analyzer-pmd-engine/src/messages.ts
@@ -38,6 +38,15 @@ const MESSAGE_CATALOG : { [key: string]: string } = {
         `List of languages associated with CPD to be made available for 'cpd' engine rule selection.\n` +
         `The languages that you may choose from are: %s.`,
 
+    CpdConfigFieldDescription_minimum_tokens:
+        `The minimum number of tokens required to be in a duplicate block of code in order to be reported as a violation.\n` +
+        `The concept of a token may be defined differently per language, but in general it a distinct basic element of source code.\n` +
+        `For example, this could be language specific keywords, identifiers, operators, literals, and more.\n` +
+        `See https://docs.pmd-code.org/latest/pmd_userdocs_cpd.html to learn more.`,
+
+    CpdConfigFieldDescription_skip_duplicate_files:
+        `Indicates whether to ignore multiple copies of files of the same name and length.`,
+
     UnsupportedEngineName:
         `The PmdCpdEnginesPlugin does not support an engine with name '%s'.`,
 
@@ -84,7 +93,10 @@ const MESSAGE_CATALOG : { [key: string]: string } = {
         `Identify duplicate code blocks within your workspace files associated with the '%s' language.`,
 
     DetectCopyPasteForLanguageViolationMessage:
-        `Duplicate code detected for language '%s'. Found %d code locations containing the same block of code consisting of %d tokens across %d lines.`
+        `Duplicate code detected for language '%s'. Found %d code locations containing the same block of code consisting of %d tokens across %d lines.`,
+
+    InvalidPositiveInteger:
+        `The '%s' configuration value is invalid. The value must be a positive integer.`,
 }
 
 /**
diff --git a/packages/code-analyzer-pmd-engine/test/cpd-engine.test.ts b/packages/code-analyzer-pmd-engine/test/cpd-engine.test.ts
@@ -96,7 +96,7 @@ async function expectRulesToMatchGoldFile(actualRuleDescriptions: RuleDescriptio
 describe('Tests for the runRules method of CpdEngine', () => {
     it('When zero rules names are provided then return zero violations', async () => {
         const engine: CpdEngine = new CpdEngine(DEFAULT_CPD_ENGINE_CONFIG);
-        expect(await engine.runRules([],{workspace: new Workspace([__dirname])})).toEqual({violations: []});
+        expect(await engine.runRules([], {workspace: new Workspace([__dirname])})).toEqual({violations: []});
     });
 
     it('When rule name is not associated with a language that CPD knows about, then throw error', async () => {
@@ -214,8 +214,103 @@ describe('Tests for the runRules method of CpdEngine', () => {
         expect(results.violations).toContainEqual(expViolation2);
         expect(results.violations).toContainEqual(expViolation3);
 
+        // Notice how with the default 100 minimum_tokens that the sampleJavascript1_ItselfContainsDuplicateBlocksButWithVeryFewTokens
+        // file doesn't get picked up even though we specified the DetectCopyPasteForJavascript rule. See the next test.
+
         // Also check that we have all the correct progress events
         expect(progressEvents.map(e => e.percentComplete)).toEqual([2, 5, 9.65, 14.3, 17.4, 20.5,
             26.7, 32.9, 39.1, 42.2, 45.3, 51.5, 57.7, 63.9, 67, 70.1, 76.3, 82.5, 88.7, 93.35, 98, 100]);
     });
+
+    it('When specifying a minimum_tokens length that is small enough to pick up smaller code blocks, then violations are returned', async () => {
+        const engine: CpdEngine = new CpdEngine({
+            ...DEFAULT_CPD_ENGINE_CONFIG,
+            minimum_tokens: 10
+        });
+        const progressEvents: RunRulesProgressEvent[] = [];
+        engine.onEvent(EventType.RunRulesProgressEvent, (e: RunRulesProgressEvent) => progressEvents.push(e));
+
+        const workspace: Workspace = new Workspace([path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace')]);
+        const ruleNames: string[] = ['DetectCopyPasteForJavascript'];
+
+        const results: EngineRunResults = await engine.runRules(ruleNames, {workspace: workspace});
+
+        const expViolation1: Violation = {
+            ruleName: "DetectCopyPasteForJavascript",
+            message: "Duplicate code detected for language 'javascript'. Found 2 code locations containing the same block of code consisting of 36 tokens across 10 lines.",
+            primaryLocationIndex: 0,
+            codeLocations: [
+                {
+                    file: path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace', 'sampleJavascript1_ItselfContainsDuplicateBlocksButWithVeryFewTokens.js'),
+                    startLine: 1,
+                    startColumn: 14,
+                    endLine: 10,
+                    endColumn: 2
+                },
+                {
+                    file: path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace', 'sampleJavascript2_ContainsNearlyAllTheSameTokensAsSampleJavascript1.js'),
+                    startLine: 1,
+                    startColumn: 14,
+                    endLine: 10,
+                    endColumn: 2
+                }
+            ]
+        };
+
+        const expViolation2: Violation = {
+            ruleName: "DetectCopyPasteForJavascript",
+            message: "Duplicate code detected for language 'javascript'. Found 4 code locations containing the same block of code consisting of 13 tokens across 4 lines.",
+            primaryLocationIndex: 0,
+            codeLocations: [
+                {
+                    file: path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace', 'sampleJavascript1_ItselfContainsDuplicateBlocksButWithVeryFewTokens.js'),
+                    startLine: 1,
+                    startColumn: 15,
+                    endLine: 4,
+                    endColumn: 2
+                },
+                {
+                    file: path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace', 'sampleJavascript1_ItselfContainsDuplicateBlocksButWithVeryFewTokens.js'),
+                    startLine: 6,
+                    startColumn: 10,
+                    endLine: 9,
+                    endColumn: 4
+                },
+                {
+                    file: path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace', 'sampleJavascript2_ContainsNearlyAllTheSameTokensAsSampleJavascript1.js'),
+                    startLine: 1,
+                    startColumn: 15,
+                    endLine: 4,
+                    endColumn: 2
+                },
+                {
+                    file: path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace', 'sampleJavascript2_ContainsNearlyAllTheSameTokensAsSampleJavascript1.js'),
+                    startLine: 6,
+                    startColumn: 10,
+                    endLine: 9,
+                    endColumn: 4
+                }
+            ]
+        };
+
+        expect(results.violations).toHaveLength(2);
+        expect(results.violations).toContainEqual(expViolation1);
+        expect(results.violations).toContainEqual(expViolation2);
+    });
+
+    it('When skipping duplicate files, then results should not include duplicates from files of same name and length', async () => {
+        const engine: CpdEngine = new CpdEngine({
+            ... DEFAULT_CPD_ENGINE_CONFIG,
+            skip_duplicate_files: true
+        });
+        const progressEvents: RunRulesProgressEvent[] = [];
+        engine.onEvent(EventType.RunRulesProgressEvent, (e: RunRulesProgressEvent) => progressEvents.push(e));
+
+        const workspace: Workspace = new Workspace([path.join(TEST_DATA_FOLDER, 'sampleCpdWorkspace')]);
+        const ruleNames: string[] = ['DetectCopyPasteForHtml'];
+
+        const results: EngineRunResults = await engine.runRules(ruleNames, {workspace: workspace});
+
+        expect(results.violations).toHaveLength(0); // Should not pick up the someReplicatedFileWithOver100Tokens.html files
+    });
 });
diff --git a/packages/code-analyzer-pmd-engine/test/plugin.test.ts b/packages/code-analyzer-pmd-engine/test/plugin.test.ts
@@ -57,7 +57,9 @@ describe('Tests for the PmdCpdEnginesPlugin', () => {
         expect(resolvedConfig.java_command.endsWith('java')).toEqual(true);
         expect(resolvedConfig).toEqual({
             java_command: resolvedConfig.java_command, // Already checked that it ends with 'java'
-            rule_languages: ['apex', 'html', 'javascript', 'typescript', 'visualforce', 'xml']
+            rule_languages: ['apex', 'html', 'javascript', 'typescript', 'visualforce', 'xml'],
+            minimum_tokens: 100,
+            skip_duplicate_files: false
         });
     });
 
@@ -271,7 +273,7 @@ describe('Tests for the PmdCpdEnginesPlugin', () => {
             getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType', 'engines.pmd.custom_rulesets[2]', 'string', 'object'));
     });
 
-    it(`When createEngineConfig is given valid custom ruleset values, then make sure they are in resolved config`, async() => {
+    it(`When createEngineConfig for 'pmd' is given valid custom ruleset values, then make sure they are in resolved config`, async () => {
         const rawConfig: ConfigObject = {
             java_classpath_entries: [
                 path.join('test-data','custom rules','category_joshapex_somecat2.jar'),
@@ -294,6 +296,41 @@ describe('Tests for the PmdCpdEnginesPlugin', () => {
         ]);
     });
 
+    it(`When createEngineConfig for 'cpd' is given a minimum_tokens value that is not a number, then error`, async () => {
+        const rawConfig: ConfigObject = {minimum_tokens: 'oops'};
+        const configValueExtractor: ConfigValueExtractor = new ConfigValueExtractor(rawConfig, 'engines.cpd');
+        await expect(plugin.createEngineConfig('cpd', configValueExtractor)).rejects.toThrow(
+            getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType', 'engines.cpd.minimum_tokens', 'number', 'string'));
+    });
+
+    it.each([-5,0,2.5])(`When createEngineConfig for 'cpd' is given a minumum_tokens number %s that is not a positive integer, then error`, async (invalidValue) => {
+        const rawConfig: ConfigObject = {minimum_tokens: invalidValue};
+        const configValueExtractor: ConfigValueExtractor = new ConfigValueExtractor(rawConfig, 'engines.cpd');
+        await expect(plugin.createEngineConfig('cpd', configValueExtractor)).rejects.toThrow(
+            getMessage('InvalidPositiveInteger', 'engines.cpd.minimum_tokens'));
+    });
+
+    it(`When createEngineConfig for 'cpd' is given a valid minimum_tokens value, then it is used`, async() => {
+        const rawConfig: ConfigObject = {minimum_tokens: 18};
+        const configValueExtractor: ConfigValueExtractor = new ConfigValueExtractor(rawConfig, 'engines.cpd');
+        const resolvedConfig: ConfigObject = await plugin.createEngineConfig('cpd', configValueExtractor);
+        expect(resolvedConfig['minimum_tokens']).toEqual(18);
+    });
+
+    it(`When createEngineConfig for 'cpd' is given a skip_duplicate_files value that is not a boolean, then error`, async () => {
+        const rawConfig: ConfigObject = {skip_duplicate_files: 3};
+        const configValueExtractor: ConfigValueExtractor = new ConfigValueExtractor(rawConfig, 'engines.cpd');
+        await expect(plugin.createEngineConfig('cpd', configValueExtractor)).rejects.toThrow(
+            getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType', 'engines.cpd.skip_duplicate_files', 'boolean', 'number'));
+    });
+
+    it(`When createEngineConfig for 'cpd' is given a valid skip_duplicate_files value, then it is used`, async() => {
+        const rawConfig: ConfigObject = {skip_duplicate_files: true};
+        const configValueExtractor: ConfigValueExtractor = new ConfigValueExtractor(rawConfig, 'engines.cpd');
+        const resolvedConfig: ConfigObject = await plugin.createEngineConfig('cpd', configValueExtractor);
+        expect(resolvedConfig['skip_duplicate_files']).toEqual(true);
+    });
+
     it('When createEngineConfig is called with an unsupported engine name, then an error is thrown', async () => {
         await expect(plugin.createEngineConfig('oops', new ConfigValueExtractor({}))).rejects.toThrow(
             getMessage('UnsupportedEngineName', 'oops'));
