NEW(cpd): @W-16866836@: Add java_command and rule_langage configurability to cpd engine (#123)

Author: stephen-carter-at-sf

packages/code-analyzer-pmd-engine/gradle/libs.versions.toml

@@ -19,11 +19,10 @@ junit-jupiter = { module = "org.junit.jupiter:junit-jupiter", version.ref = "jun
 pmd-apex = { module = "net.sourceforge.pmd:pmd-apex", version.ref = "pmd" }
 pmd-core = { module = "net.sourceforge.pmd:pmd-core", version.ref = "pmd" }
 pmd-html = { module = "net.sourceforge.pmd:pmd-html", version.ref = "pmd" }
-pmd-java = { module = "net.sourceforge.pmd:pmd-java", version.ref = "pmd" }
 pmd-javascript = { module = "net.sourceforge.pmd:pmd-javascript", version.ref = "pmd" }
 pmd-visualforce = { module = "net.sourceforge.pmd:pmd-visualforce", version.ref = "pmd" }
 pmd-xml = { module = "net.sourceforge.pmd:pmd-xml", version.ref = "pmd" }
 slf4j-nop = { module = "org.slf4j:slf4j-nop", version.ref = "slf4j-nop" }
 
 [bundles]
-pmd7 = ["pmd-apex", "pmd-core", "pmd-html", "pmd-java", "pmd-javascript", "pmd-visualforce", "pmd-xml"]
+pmd7 = ["pmd-apex", "pmd-core", "pmd-html", "pmd-javascript", "pmd-visualforce", "pmd-xml"]

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.13.0",
+    "version": "0.13.1",
     "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/cpdwrapper/CpdRunner.java

@@ -13,9 +13,12 @@
 
 import javax.annotation.Nullable;
 import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.text.MessageFormat;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -62,7 +65,9 @@ private CpdLanguageRunResults runLanguage(String language, List<Path> pathsToSca
         config.setMinimumTileSize(minimumTokens);
         config.setInputPathList(pathsToScan);
         config.setSkipDuplicates(skipDuplicateFiles);
-        config.setReporter(new CpdErrorListener());
+        CpdErrorListener errorListener = new CpdErrorListener();
+
+        config.setReporter(errorListener);
 
         CpdLanguageRunResults languageRunResults = new CpdLanguageRunResults();
 
@@ -105,6 +110,16 @@ private CpdLanguageRunResults runLanguage(String language, List<Path> pathsToSca
                     languageRunResults.matches.add(cpdMatch);
                 }
             });
+
+            // Instead of throwing exceptions and causing the entire run to fail, instead we report exceptions as
+            // if they are processing errors so that they can better be handled on the typescript side
+            for (Exception ex : errorListener.exceptionsCaught) {
+                CpdLanguageRunResults.ProcessingError processingErr = new CpdLanguageRunResults.ProcessingError();
+                processingErr.file = "unknown";
+                processingErr.message = getStackTraceAsString(ex);
+                processingErr.detail = "[TERMINATING_EXCEPTION]"; // Marker to help typescript side know this isn't just a normal processing error
+                languageRunResults.processingErrors.add(processingErr);
+            }
         }
 
         return languageRunResults;
@@ -119,19 +134,28 @@ private void validateRunInputData(CpdRunInputData runInputData) {
             throw new RuntimeException("The \"minimumTokens\" field was not set to a positive number.");
         }
     }
+
+    private static String getStackTraceAsString(Throwable e) {
+        StringWriter sw = new StringWriter();
+        try (PrintWriter pw = new PrintWriter(sw)) {
+            e.printStackTrace(pw);
+        }
+        return sw.toString();
+    }
 }
 
 // This class simply helps us process any errors that may be thrown by CPD. By default, CPD suppresses errors so that
 // they are not thrown. So here, we look out for the errors that we care about and process it to throw a better
 // error messages. We override the logEx method in particular because all other error methods call through to logEx.
 class CpdErrorListener implements PmdReporter {
+    List<Exception> exceptionsCaught = new ArrayList<>();
     @Override
     public void logEx(Level level, @javax.annotation.Nullable String s, Object[] objects, @Nullable Throwable throwable) {
         if (throwable != null) {
-            throw new RuntimeException("CPD threw an unexpected exception:\n" + throwable.getMessage(), throwable);
+            exceptionsCaught.add(new RuntimeException("CPD threw an unexpected exception:\n" + throwable.getMessage(), throwable));
         } else if (s != null) {
             String message = MessageFormat.format(s, objects);
-            throw new RuntimeException("CPD threw an unexpected exception:\n" + message);
+            exceptionsCaught.add(new RuntimeException("CPD threw an unexpected exception:\n" + message));
         }
     }
 

packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/main/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriber.java

@@ -43,16 +43,6 @@ class PmdRuleDescriber {
             "html", Set.of(
                     "category/html/bestpractices.xml"
             ),
-            "java", Set.of(
-                    "category/java/bestpractices.xml",
-                    "category/java/codestyle.xml",
-                    "category/java/design.xml",
-                    "category/java/documentation.xml",
-                    "category/java/errorprone.xml",
-                    "category/java/multithreading.xml",
-                    "category/java/performance.xml",
-                    "category/java/security.xml"
-            ),
             "pom", Set.of(
                     "category/pom/errorprone.xml"
             ),

packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/test/java/com/salesforce/sfca/cpdwrapper/CpdWrapperTest.java

@@ -182,7 +182,7 @@ void whenCallingRunWithNegativeMinimumTokensValue_thenError(@TempDir Path tempDi
     }
 
     @Test
-    void whenCallingRunWithFileToScanThatDoesNotExist_thenError(@TempDir Path tempDir) throws Exception {
+    void whenCallingRunWithFileToScanThatDoesNotExist_thenExceptionIsForwardedAsProcessingErrorWithTerminatingExceptionMarker(@TempDir Path tempDir) throws Exception {
         String doesNotExist = tempDir.resolve("doesNotExist.cls").toAbsolutePath().toString();
         String inputFileContents = "{" +
                 "  \"filesToScanPerLanguage\": {" +
@@ -192,11 +192,16 @@ void whenCallingRunWithFileToScanThatDoesNotExist_thenError(@TempDir Path tempDi
                 "  \"skipDuplicateFiles\": false " +
                 "}";
         String inputFile = createTempFile(tempDir, "inputFile.json", inputFileContents);
+        String outputFile = tempDir.resolve("output.json").toAbsolutePath().toString();
 
-        String[] args = {"run", inputFile, "/does/not/matter"};
-        RuntimeException thrown = assertThrows(RuntimeException.class, () -> callCpdWrapper(args));
-        assertThat(thrown.getMessage(), is(
-                "Error while attempting to invoke CpdRunner.run: CPD threw an unexpected exception:\nNo such file " + doesNotExist));
+        String[] args = {"run", inputFile, outputFile};
+        callCpdWrapper(args);
+
+        String resultsJsonString = new String(Files.readAllBytes(Paths.get(outputFile)));
+        assertThat(resultsJsonString, allOf(
+                containsString("\"processingErrors\":[{"),
+                containsString("No such file"),
+                containsString("\"detail\":\"[TERMINATING_EXCEPTION]\"")));
     }
 
     @Test

packages/code-analyzer-pmd-engine/pmd-cpd-wrappers/src/test/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriberTest.java

@@ -71,23 +71,6 @@ void whenDescribeRulesForApex_thenCorrectRulesAreReturned() {
         assertThat(ruleInfo.ruleSetFile, is("category/apex/performance.xml"));
     }
 
-    @Test
-    void whenDescribeRulesForJava_thenCorrectRulesAreReturned()  {
-        List<PmdRuleInfo> ruleInfoList = ruleDescriber.describeRulesFor(List.of(), Set.of("java"));
-        assertThat(ruleInfoList.size(), is(greaterThan(0))); // Leaving this flexible. The actual list of rules are tested by typescript tests.
-        for (PmdRuleInfo ruleInfo : ruleInfoList) {
-            assertThat(ruleInfo.language, is("java"));
-        }
-
-        // Sanity check one of the rules:
-        PmdRuleInfo ruleInfo = assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "AvoidReassigningParameters", "java");
-        assertThat(ruleInfo.description, is("Reassigning values to incoming parameters of a method or constructor is not recommended, as this can make the code more difficult to understand. The code is often read with the assumption that parameter values don't change and an assignment violates therefore the principle of least astonishment. This is especially a problem if the parameter is documented e.g. in the method's... Learn more: " + ruleInfo.externalInfoUrl));
-        assertThat(ruleInfo.externalInfoUrl, allOf(startsWith("https://"), endsWith(".html#avoidreassigningparameters")));
-        assertThat(ruleInfo.ruleSet, is("Best Practices"));
-        assertThat(ruleInfo.priority, is("Medium High"));
-        assertThat(ruleInfo.ruleSetFile, is("category/java/bestpractices.xml"));
-    }
-
     @Test
     void whenDescribeRulesForEcmascript_thenCorrectRulesAreReturned() {
         List<PmdRuleInfo> ruleInfoList = ruleDescriber.describeRulesFor(List.of(), Set.of("ecmascript"));
@@ -192,10 +175,10 @@ void whenDescribeRulesIsGivenCustomRulesetThatIsOnJavaClasspath_thenReturnAssoci
         // cause any conflicts or errors.
         try (StdOutCaptor stdoutCaptor = new StdOutCaptor()) {
             List<PmdRuleInfo> ruleInfoList = ruleDescriber.describeRulesFor(
-                    List.of("category/java/codestyle.xml"),
-                    Set.of("java"));
+                    List.of("category/apex/codestyle.xml"),
+                    Set.of("apex"));
 
-            assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "AtLeastOneConstructor", "java");
+            assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "ClassNamingConventions", "apex");
             assertThat(stdoutCaptor.getCapturedOutput(), containsString("Skipping rule "));
         }
     }
@@ -209,11 +192,11 @@ void whenDescribeRulesIsGivenCustomRulesetButCustomRuleLanguageIsNotSpecified_th
 
         List<PmdRuleInfo> ruleInfoList = ruleDescriber.describeRulesFor(
                 List.of(rulesetFile1.toAbsolutePath().toString(), rulesetFile2.toAbsolutePath().toString()),
-                Set.of("java", "visualforce")); // ... but we don't have apex here but we do have visualforce...
+                Set.of("ecmascript", "visualforce")); // ... but we don't have apex here but we do have visualforce...
 
         assertContainsNoRuleWithNameAndLanguage(ruleInfoList, "sampleRule1", "apex"); // ... thus this rule should not show
         assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "sampleRule2", "visualforce"); // Should show since visualforce is provided
-        assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "AtLeastOneConstructor", "java"); // Should show since visualforce is provided
+        assertContainsOneRuleWithNameAndLanguage(ruleInfoList, "AvoidWithStatement", "ecmascript"); // Should show since ecmascript is provided
     }
 
     @Test