Skip to content

Commit

Permalink
FIX(pmd): @W-17486457@: Prevent errors when custom rules do not have …
Browse files Browse the repository at this point in the history 
…non-required fields
  • Loading branch information
@stephen-carter-at-sf
stephen-carter-at-sf committed Dec 23, 2024
1 parent 9b81fbc commit bb3c0b8
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 37 deletions.
10 changes: 6 additions & 4 deletions ...ngine/pmd-cpd-wrappers/src/main/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriber.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.util.*;
import java.util.regex.Pattern;
import java.util.regex.Matcher;
import java.util.stream.Collectors;

class PmdRuleDescriber {
/**
Expand Down Expand Up @@ -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);
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
}
}
Expand Down
84 changes: 69 additions & 15 deletions ...e/pmd-cpd-wrappers/src/test/java/com/salesforce/sfca/pmdwrapper/PmdRuleDescriberTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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."));
}
}

Expand Down Expand Up @@ -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>";
}

Expand Down
2 changes: 1 addition & 1 deletion packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}`)
Expand Down
25 changes: 23 additions & 2 deletions packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
19 changes: 4 additions & 15 deletions packages/code-analyzer-pmd-engine/test/test-data/custom rules/subfolder/somecat4.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
4 changes: 4 additions & 0 deletions ...code-analyzer-pmd-engine/test/test-data/samplePmdWorkspace/sampleViolations/fakerule10.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
let car = {color: 'red'}
with(car){
console.log(color)
}

0 comments on commit bb3c0b8

Please sign in to comment.