diff --git a/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/main/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriber.java b/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/main/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriber.java index 25ebc02c..604c2496 100644 --- a/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/main/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriber.java +++ b/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/main/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriber.java @@ -13,6 +13,7 @@ import java.util.*; import java.util.regex.Pattern; import java.util.regex.Matcher; +import java.util.stream.Collectors; class PmdRuleDescriber { /** @@ -109,7 +110,7 @@ List describeRulesFor(List customRulesets, Set lang pmdRuleInfo.description = getLimitedDescription(rule); pmdRuleInfo.externalInfoUrl = rule.getExternalInfoUrl(); pmdRuleInfo.ruleSet = rule.getRuleSetName(); - pmdRuleInfo.priority = rule.getPriority().toString(); + pmdRuleInfo.priority = rule.getPriority().toString(); // If priority isn't set in ruleset file, then PMD uses "Low" by default. So no need for null check here. pmdRuleInfo.ruleSetFile = ruleSet.getFileName(); ruleInfoList.add(pmdRuleInfo); } @@ -122,7 +123,7 @@ List describeRulesFor(List customRulesets, Set lang private static String getLimitedDescription(Rule rule) { // Since the rule description can be long, we remove unnecessary whitespace and clip the message // so that users can read more on the website if it is over 500 characters long. - String ruleDescription = rule.getDescription().trim() + String ruleDescription = Objects.requireNonNullElse(rule.getDescription(),"").trim() .replaceAll("\\s+"," ") // Switch to only use a single space whenever whitespace is found .replaceAll("\\[([^]]+)]\\([^)]+\\)", "$1"); // Remove markdown urls. i.e. replace "[phrase]()" with "phrase" if (ruleDescription.length() > 500) { @@ -157,10 +158,11 @@ public void logEx(Level level, @Nullable String s, Object[] objects, @Nullable T if (matcher.find()) { String ruleset = matcher.group(1).trim(); String errorMessage = "PMD errored when attempting to load a custom ruleset \"" + ruleset + "\". " + - "Make sure the resource is a valid file on disk or on the Java classpath."; + "Make sure the resource is a valid ruleset file on disk or on the Java classpath.\n\n" + + "PMD Exception: \n" + message.lines().map(l -> " | " + l).collect(Collectors.joining("n")); // The typescript side can more easily handle error messages that come from stdout with "[Error] " marker - System.out.println("[Error] " + errorMessage); + System.out.println("[Error] " + errorMessage.replaceAll("\n","{NEWLINE}")); throw new RuntimeException(errorMessage, throwable); } } diff --git a/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/test/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriberTest.java b/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/test/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriberTest.java index eec0a52e..58d709ff 100644 --- a/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/test/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriberTest.java +++ b/packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/test/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriberTest.java @@ -135,8 +135,8 @@ void whenDescribeRulesIsGivenCustomRulesetThatDoesNotExist_thenErrorWithNiceMess try (StdOutCaptor stdoutCaptor = new StdOutCaptor()) { Exception thrown = assertThrows(Exception.class, () -> ruleDescriber.describeRulesFor(List.of("does/not/exist.xml"), Set.of("apex", "visualforce"))); - assertThat(thrown.getMessage(), is("PMD errored when attempting to load a custom ruleset \"does/not/exist.xml\". " + - "Make sure the resource is a valid file on disk or on the Java classpath.")); + assertThat(thrown.getMessage(), containsString("PMD errored when attempting to load a custom ruleset \"does/not/exist.xml\". " + + "Make sure the resource is a valid ruleset file on disk or on the Java classpath.")); } } @@ -221,25 +221,79 @@ void whenDescribeRulesIsGivenCustomRulesetThatContainsNameOfStandardRule_thenCus } } + @Test + void whenDescribeRulesIsGivenCustomRulesetThatIsNotValid_thenError(@TempDir Path tempDir) throws Exception { + Path rulesetFile = tempDir.resolve("sampleRulesetFile.xml"); + Files.write(rulesetFile, "".getBytes()); + String rulesetFileStr = rulesetFile.toAbsolutePath().toString(); + + try (StdOutCaptor stdoutCaptor = new StdOutCaptor()) { + Exception thrown = assertThrows(Exception.class, () -> + ruleDescriber.describeRulesFor(List.of(rulesetFileStr), Set.of("apex"))); + assertThat(thrown.getMessage(), containsString("PMD errored when attempting to load a custom ruleset \"" + rulesetFileStr + "\". " + + "Make sure the resource is a valid ruleset file on disk or on the Java classpath.")); + } + } + + @Test + void whenDescribeRulesIsGivenCustomRulesetWithMissingRuleDescriptionPriorityAndUrl_thenDefaultsAreGiven(@TempDir Path tempDir) throws Exception { + Path rulesetFile = tempDir.resolve("sampleRulesetFile.xml"); + String contents = createSampleRuleset("sampleRuleset", + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""); + Files.write(rulesetFile, contents.getBytes()); + + try (StdOutCaptor stdoutCaptor = new StdOutCaptor()) { + List ruleInfoList = ruleDescriber.describeRulesFor( + List.of(rulesetFile.toAbsolutePath().toString()), + Set.of("apex")); + + PmdRuleInfo ruleInfo = assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "sampleRule", "apex"); + assertThat(ruleInfo.ruleSet, is("sampleRuleset")); + assertThat(ruleInfo.description, is("")); + assertThat(ruleInfo.externalInfoUrl, is(nullValue())); + assertThat(ruleInfo.priority, is("Low")); // PMD's default priority when ruleset doesn't specify + assertThat(ruleInfo.ruleSetFile, is(rulesetFile.toAbsolutePath().toString())); + } + } + static String createSampleRuleset(String rulesetName, String ruleName, String language, int priority) { + return createSampleRuleset(rulesetName, + "\n" + + " Sample " + ruleName + " description\n" + + " " + priority + "\n" + + " \n" + + " \n" + + " \n" + + "\n" + + " \n" + + " \n" + + " \n" + + ""); + } + + static String createSampleRuleset(String rulesetName, String ruleXml) { return "\n" + " Sample " + rulesetName + " Description\n" + - " \n" + - " Sample " + ruleName + " description\n" + - " " + priority + "\n" + - " \n" + - " \n" + - " \n" + - "\n" + - " \n" + - " \n" + - " \n" + - " \n" + + ruleXml.lines().map(l -> " " + l).collect(Collectors.joining("\n")) + "\n" + ""; } diff --git a/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts b/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts index 612e098b..f4f2d056 100644 --- a/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts +++ b/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts @@ -78,7 +78,7 @@ export class PmdWrapperInvoker { this.emitLogEvent(LogLevel.Fine, `Calling JAVA command with class path containing ${JSON.stringify(javaClassPaths)} and arguments: ${JSON.stringify(javaCmdArgs)}`); await this.javaCommandExecutor.exec(javaCmdArgs, javaClassPaths, (stdOutMsg) => { if (stdOutMsg.startsWith(STDOUT_ERROR_MARKER)) { - const errorMessage: string = stdOutMsg.slice(STDOUT_ERROR_MARKER.length); + const errorMessage: string = stdOutMsg.slice(STDOUT_ERROR_MARKER.length).replaceAll('{NEWLINE}','\n'); throw new Error(errorMessage); } else { this.emitLogEvent(LogLevel.Fine, `[JAVA StdOut]: ${stdOutMsg}`) diff --git a/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts b/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts index 5591b110..416bcd3a 100644 --- a/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts +++ b/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts @@ -160,7 +160,8 @@ describe('Tests for the describeRules method of PmdEngine', () => { expectContainsRuleWithName(ruleDescriptions, 'fakerule8'); // From somecat3.xml expectContainsRuleWithName(ruleDescriptions, 'fakerule9'); // From somecat3.xml const fakeRule10Description: RuleDescription = expectContainsRuleWithName(ruleDescriptions, 'fakerule10'); // From somecat4.xml - expect(fakeRule10Description.severityLevel).toEqual(SeverityLevel.High); + expect(fakeRule10Description.severityLevel).toEqual(SeverityLevel.Low); // Default when priority is not in ruleset file + expect(fakeRule10Description.description).toEqual(''); // Default when no description is in ruleset file expect(fakeRule10Description.tags).toEqual(['Recommended', 'SomeCat4', 'Javascript', 'Custom']); expect(fakeRule10Description.resourceUrls).toHaveLength(0); // This particular rule purposely has no externalInfoUrl defined, so we confirm that it gives no resourceUrls. expectContainsRuleWithName(ruleDescriptions, 'fakerule11'); // From somecat4.xml @@ -275,8 +276,9 @@ describe('Tests for the describeRules method of PmdEngine', () => { 'does/not/exist.xml' ] }); + // Note that toThrow always checks for a substring instead of exact match, which is why this works: await expect(engine.describeRules({})).rejects.toThrow('PMD errored when attempting to load a custom ruleset "does/not/exist.xml". ' + - 'Make sure the resource is a valid file on disk or on the Java classpath.'); + 'Make sure the resource is a valid ruleset file on disk or on the Java classpath.\n\nPMD Exception:'); }); it('When file_extensions associates .txt file to javascript language and workspace only has .txt file, then javascript rules are returned', async () => { @@ -552,6 +554,25 @@ describe('Tests for the runRules method of PmdEngine', () => { expect(results.violations[0].ruleName).toEqual('WhileLoopsMustUseBraces-javascript'); expect(results.violations[0].codeLocations[0].file).toEqual(path.join(TEST_DATA_FOLDER, 'samplePmdWorkspace','sampleViolations','WhileLoopsMustUseBraces.txt')); }); + + it('When custom rule does not define a violation message in the ruleset file, then a processing error should report exist`', async () => { + const engine: PmdEngine = new PmdEngine({ + ... DEFAULT_PMD_ENGINE_CONFIG, + rule_languages: [Language.JAVASCRIPT], + custom_rulesets: [ + path.join(TEST_DATA_FOLDER, 'custom rules', 'subfolder', 'somecat4.xml') // Contains fakerule10 which has no violation message + ] + }); + + const logEvents: LogEvent[] = []; + engine.onEvent(EventType.LogEvent, (event: LogEvent) => logEvents.push(event)); + + const workspace: Workspace = new Workspace([path.join(TEST_DATA_FOLDER, 'samplePmdWorkspace', 'sampleViolations', 'fakerule10.js')]); + await engine.runRules(['fakerule10'], {workspace: workspace}); + const errorLogEvents: LogEvent[] = logEvents.filter(event => event.logLevel === LogLevel.Error); + expect(errorLogEvents).toHaveLength(1); + expect(errorLogEvents[0].message).toContain('Message was null'); + }); }); describe('Tests for the getEngineVersion method of PmdEngine', () => { diff --git a/packages/code-analyzer-pmd-engine/test/test-data/custom rules/subfolder/somecat4.xml b/packages/code-analyzer-pmd-engine/test/test-data/custom rules/subfolder/somecat4.xml index 816dcd13..568b3fec 100644 --- a/packages/code-analyzer-pmd-engine/test/test-data/custom rules/subfolder/somecat4.xml +++ b/packages/code-analyzer-pmd-engine/test/test-data/custom rules/subfolder/somecat4.xml @@ -12,28 +12,17 @@ - Avoid using with - it's bad news - 1 + class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"> + - - - + ]]> - - -