FIX(pmd): @W-17486457@: Prevent errors when custom rules do not have non-required fields

packages/code-analyzer-pmd-engine/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@salesforce/code-analyzer-pmd-engine",
   "description": "Plugin package that adds 'pmd' and 'cpd' as engines into Salesforce Code Analyzer",
-  "version": "0.17.0",
+  "version": "0.17.1-SNAPSHOT",
   "author": "The Salesforce Code Analyzer Team",
   "license": "BSD-3-Clause",
   "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview",

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<PmdRuleInfo> describeRulesFor(List<String> customRulesets, Set<String> 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<PmdRuleInfo> describeRulesFor(List<String> customRulesets, Set<String> 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](<url>)" with "phrase"
         if (ruleDescription.length() > 500) {
@@ -152,15 +153,16 @@ public void logEx(Level level, @Nullable String s, Object[] objects, @Nullable T
         if (throwable != null) {
             String message = throwable.getMessage();
             if (throwable instanceof RuleSetLoadException && message.contains("Cannot load ruleset ")) {
-                Pattern pattern = Pattern.compile("Cannot load ruleset (.+?):");
+                Pattern pattern = Pattern.compile("Cannot load ruleset (.+?): ");
                 Matcher matcher = pattern.matcher(message);
                 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);
                 }
             }

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, "<oops></oops>".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",
+                "<rule name=\"sampleRule\"\n" +
+                        "      language=\"apex\"\n" +
+                        "      class=\"net.sourceforge.pmd.lang.rule.xpath.XPathRule\">\n" +
+                        "    <properties>\n" +
+                        "        <property name=\"xpath\">\n" +
+                        "            <value><![CDATA[\n" +
+                        "//MethodCallExpression[lower-case(@FullMethodName)='foo']\n" +
+                        "            ]]></value>\n" +
+                        "        </property>\n" +
+                        "    </properties>\n" +
+                        "</rule>");
+        Files.write(rulesetFile, contents.getBytes());
+
+        try (StdOutCaptor stdoutCaptor = new StdOutCaptor()) {
+            List<PmdRuleInfo> 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,
+                "<rule name=\"" + ruleName + "\"\n" +
+                "      language=\"" + language + "\"\n" +
+                "      message=\"Sample " + ruleName + " message\"\n" +
+                "      class=\"net.sourceforge.pmd.lang.rule.xpath.XPathRule\"\n" +
+                "      externalInfoUrl=\"https://" + ruleName + ".com\">\n" +
+                "    <description>Sample " + ruleName + " description</description>\n" +
+                "    <priority>" + priority + "</priority>\n" +
+                "    <properties>\n" +
+                "        <property name=\"xpath\">\n" +
+                "            <value>\n" +
+                "<![CDATA[\n" +
+                "//MethodCallExpression[lower-case(@FullMethodName)='" + ruleName + "']\n" +
+                "]]>\n" +
+                "            </value>\n" +
+                "        </property>\n" +
+                "    </properties>\n" +
+                "</rule>");
+    }
+
+    static String createSampleRuleset(String rulesetName, String ruleXml) {
         return "<ruleset name=\"" + rulesetName + "\"\n" +
                 "         xmlns=\"http://pmd.sourceforge.net/ruleset/2.0.0\"\n" +
                 "         xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n" +
                 "         xsi:schemaLocation=\"http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd\">\n" +
                 "    <description>Sample " + rulesetName + " Description</description>\n" +
-                "    <rule name=\"" + ruleName + "\" language=\"" + language + "\" message=\"Sample " + ruleName + " message\" class=\"net.sourceforge.pmd.lang.rule.xpath.XPathRule\" externalInfoUrl=\"https://" + ruleName + ".com\">\n" +
-                "        <description>Sample " + ruleName + " description</description>\n" +
-                "        <priority>" + priority + "</priority>\n" +
-                "        <properties>\n" +
-                "            <property name=\"xpath\">\n" +
-                "                <value>\n" +
-                "<![CDATA[\n" +
-                "//MethodCallExpression[lower-case(@FullMethodName)='" + ruleName + "']\n" +
-                "]]>\n" +
-                "                </value>\n" +
-                "            </property>\n" +
-                "        </properties>\n" +
-                "    </rule>\n" +
+                ruleXml.lines().map(l -> "    " + l).collect(Collectors.joining("\n")) + "\n" +
                 "</ruleset>";
     }
 

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}`)

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', () => {

packages/code-analyzer-pmd-engine/test/test-data/custom rules/subfolder/somecat4.xml

@@ -12,28 +12,17 @@
 	</description>
 
 	<rule name="fakerule10"
-		  message="Avoid using with - it's bad news"
 		  language="ecmascript"
 		  since="5.0.1"
-		  class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"> <!-- Removed the externalInfoUrl attribute so that we can test what happens when this is missing -->
-		<description>Avoid using with - it's bad news</description>
-		<priority>1</priority>
+		  class="net.sourceforge.pmd.lang.rule.xpath.XPathRule">
+		<!-- Missing message, priority, description, and externalUrl on purpose because PMD doesn't require them - so we can test this scenario -->
 		<properties>
 			<property name="xpath">
-				<value>
-					<![CDATA[
+				<value><![CDATA[
 //WithStatement
-]]>
-				</value>
+				]]></value>
 			</property>
 		</properties>
-		<example>
-			<![CDATA[
-with (object) {
-    property = 3; // Might be on object, might be on window: who knows.
-}
-]]>
-		</example>
 	</rule>
 
 	<rule name="fakerule11"

packages/code-analyzer-pmd-engine/test/test-data/samplePmdWorkspace/sampleViolations/fakerule10.js

@@ -0,0 +1,4 @@
+let car = {color: 'red'}
+with(car){
+    console.log(color)
+}