Merge pull request #3788 from

adangel:issue-3768-sarif-multiple-locations [core] Fix #3768 SARIF: report each violation as separate result #3788 * pr-3788: [core] Refactoring renderer tests [core] Fix SARIF formatter reporting multiple locations
pmd · Feb 25, 2022 · 9a437b0 · 9a437b0
2 parents 47901b7 + 3eed116
commit 9a437b0
Show file tree

Hide file tree

Showing 18 changed files with 257 additions and 144 deletions.
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
@@ -18,6 +18,7 @@ This is a {{ site.pmd.release_type }} release.
 
 *   core
     *   [#3427](https://github.com/pmd/pmd/issues/3427): \[core] Stop printing CLI usage text when exiting due to invalid parameters
+    *   [#3768](https://github.com/pmd/pmd/issues/3768): \[core] SARIF formatter reports multiple locations when it should report multiple results
 *   doc
     *   [#2502](https://github.com/pmd/pmd/issues/2502): \[doc] Add floating table-of-contents (toc) on the right
 *   java

diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLogBuilder.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLogBuilder.java
@@ -4,38 +4,36 @@
 
 package net.sourceforge.pmd.renderers.internal.sarif;
 
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ArtifactLocation;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.AssociatedRule;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Component;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Exception;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Invocation;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Location;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Message;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.MultiformatMessage;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.PhysicalLocation;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.PropertyBag;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Region;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ReportingDescriptor;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Result;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Run;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Tool;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ToolConfigurationNotification;
-import static net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ToolExecutionNotification;
-
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 
 import net.sourceforge.pmd.PMDVersion;
 import net.sourceforge.pmd.Report;
 import net.sourceforge.pmd.RuleViolation;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ArtifactLocation;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.AssociatedRule;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Component;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Exception;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Invocation;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Location;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Message;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.MultiformatMessage;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.PhysicalLocation;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.PropertyBag;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Region;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ReportingDescriptor;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Result;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Run;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.Tool;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ToolConfigurationNotification;
+import net.sourceforge.pmd.renderers.internal.sarif.SarifLog.ToolExecutionNotification;
 
 public class SarifLogBuilder {
-    private final Map<ReportingDescriptor, List<Location>> locationsByRule = new HashMap<>();
+    private final List<ReportingDescriptor> rules = new ArrayList<>();
+    private final List<Result> results = new ArrayList<>();
     private final List<ToolConfigurationNotification> toolConfigurationNotifications = new ArrayList<>();
     private final List<ToolExecutionNotification> toolExecutionNotifications = new ArrayList<>();
 
@@ -45,11 +43,15 @@ public static SarifLogBuilder sarifLogBuilder() {
 
     public SarifLogBuilder add(RuleViolation violation) {
         final ReportingDescriptor ruleDescriptor = getReportingDescriptor(violation);
-        final Location location = getRuleViolationLocation(violation);
+        int ruleIndex = rules.indexOf(ruleDescriptor);
+        if (ruleIndex == -1) {
+            rules.add(ruleDescriptor);
+            ruleIndex = rules.size() - 1;
+        }
 
-        final List<Location> ruleLocation = locationsByRule.containsKey(ruleDescriptor) ? locationsByRule.get(ruleDescriptor) : new ArrayList<Location>();
-        ruleLocation.add(location);
-        locationsByRule.put(ruleDescriptor, ruleLocation);
+        final Location location = getRuleViolationLocation(violation);
+        final Result result = resultFrom(ruleDescriptor, ruleIndex, location);
+        results.add(result);
 
         return this;
     }
@@ -105,15 +107,6 @@ public SarifLogBuilder addConfigurationError(Report.ConfigurationError error) {
     }
 
     public SarifLog build() {
-        final List<ReportingDescriptor> rules = new ArrayList<>(locationsByRule.keySet());
-
-        final List<Result> results = new ArrayList<>();
-        for (int i = 0, size = rules.size(); i < size; i++) {
-            ReportingDescriptor rule = rules.get(i);
-            List<Location> locations = locationsByRule.get(rule);
-            results.add(resultFrom(rule, i, locations));
-        }
-
         final Component driver = getDriverComponent().toBuilder().rules(rules).build();
         final Tool tool = Tool.builder().driver(driver).build();
         final Invocation invocation = Invocation.builder()
@@ -136,7 +129,7 @@ private boolean isExecutionSuccessful() {
         return toolExecutionNotifications.isEmpty() && toolConfigurationNotifications.isEmpty();
     }
 
-    private Result resultFrom(ReportingDescriptor rule, Integer ruleIndex, List<Location> locations) {
+    private Result resultFrom(ReportingDescriptor rule, Integer ruleIndex, Location location) {
         final Result result = Result.builder()
                 .ruleId(rule.getId())
                 .ruleIndex(ruleIndex)
@@ -147,7 +140,7 @@ private Result resultFrom(ReportingDescriptor rule, Integer ruleIndex, List<Loca
                 .build();
 
         result.setMessage(message);
-        result.setLocations(locations);
+        result.setLocations(Collections.singletonList(location));
 
         return result;
     }

diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/AbstractRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/AbstractRendererTest.java
@@ -15,6 +15,7 @@
 import net.sourceforge.pmd.Report.ConfigurationError;
 import net.sourceforge.pmd.Report.ProcessingError;
 import net.sourceforge.pmd.ReportTest;
+import net.sourceforge.pmd.Rule;
 import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.RulePriority;
 import net.sourceforge.pmd.RuleViolation;
@@ -64,26 +65,53 @@ public void testNullPassedIn() throws Exception {
 
     protected Report reportOneViolation() {
         Report report = new Report();
-        report.addRuleViolation(newRuleViolation(1));
+        report.addRuleViolation(newRuleViolation(1, 1, 1, 1, createFooRule()));
         return report;
     }
 
-    private Report reportTwoViolations() {
+    protected Report reportTwoViolations() {
         Report report = new Report();
-        RuleViolation informationalRuleViolation = newRuleViolation(1);
-        informationalRuleViolation.getRule().setPriority(RulePriority.LOW);
+        RuleViolation informationalRuleViolation = newRuleViolation(1, 1, 1, 1, createFooRule());
         report.addRuleViolation(informationalRuleViolation);
-        RuleViolation severeRuleViolation = newRuleViolation(2);
-        severeRuleViolation.getRule().setPriority(RulePriority.HIGH);
+        RuleViolation severeRuleViolation = newRuleViolation(1, 1, 1, 2, createBooRule());
         report.addRuleViolation(severeRuleViolation);
         return report;
     }
 
-    protected RuleViolation newRuleViolation(int endColumn) {
-        DummyNode node = createNode(endColumn);
+    protected DummyNode createNode(int beginLine, int beginColumn, int endLine, int endColumn) {
+        DummyNode node = new DummyNode(1);
+        node.testingOnlySetBeginLine(beginLine);
+        node.testingOnlySetBeginColumn(beginColumn);
+        node.testingOnlySetEndLine(endLine);
+        node.testingOnlySetEndColumn(endColumn);
+        return node;
+    }
+
+    protected RuleViolation newRuleViolation(int beginLine, int beginColumn, int endLine, int endColumn, Rule rule) {
+        DummyNode node = createNode(beginLine, beginColumn, endLine, endColumn);
         RuleContext ctx = new RuleContext();
         ctx.setSourceCodeFile(new File(getSourceCodeFilename()));
-        return new ParametricRuleViolation<Node>(new FooRule(), ctx, node, "blah");
+        return new ParametricRuleViolation<Node>(rule, ctx, node, "blah");
+    }
+
+    /**
+     * Creates a new rule instance with name "Boo" and priority {@link RulePriority#HIGH}.
+     */
+    protected Rule createBooRule() {
+        Rule booRule = new FooRule();
+        booRule.setName("Boo");
+        booRule.setPriority(RulePriority.HIGH);
+        return booRule;
+    }
+
+    /**
+     * Creates a new rule instance with name "Foo" and priority {@link RulePriority#LOW}.
+     */
+    protected Rule createFooRule() {
+        Rule fooRule = new FooRule();
+        fooRule.setName("Foo");
+        fooRule.setPriority(RulePriority.LOW);
+        return fooRule;
     }
 
     protected static DummyNode createNode(int endColumn) {
@@ -97,14 +125,11 @@ protected static DummyNode createNode(int endColumn) {
 
     @Test
     public void testRuleWithProperties() throws Exception {
-        DummyNode node = createNode(1);
-        RuleContext ctx = new RuleContext();
-        ctx.setSourceCodeFile(new File(getSourceCodeFilename()));
         Report report = new Report();
         RuleWithProperties theRule = new RuleWithProperties();
         theRule.setProperty(RuleWithProperties.STRING_PROPERTY_DESCRIPTOR,
                 "the string value\nsecond line with \"quotes\"");
-        report.addRuleViolation(new ParametricRuleViolation<Node>(theRule, ctx, node, "blah"));
+        report.addRuleViolation(newRuleViolation(1, 1, 1, 1, theRule));
         String rendered = ReportTest.render(getRenderer(), report);
         assertEquals(filter(getExpectedWithProperties()), filter(rendered));
     }

diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/CSVRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/CSVRendererTest.java
@@ -30,7 +30,7 @@ public String getExpectedEmpty() {
     public String getExpectedMultiple() {
         return getHeader()
                 + "\"1\",\"\",\"" + getSourceCodeFilename() + "\",\"5\",\"1\",\"blah\",\"RuleSet\",\"Foo\"" + PMD.EOL
-                + "\"2\",\"\",\"" + getSourceCodeFilename() + "\",\"1\",\"1\",\"blah\",\"RuleSet\",\"Foo\"" + PMD.EOL;
+                + "\"2\",\"\",\"" + getSourceCodeFilename() + "\",\"1\",\"1\",\"blah\",\"RuleSet\",\"Boo\"" + PMD.EOL;
     }
 
     @Override
@@ -46,8 +46,4 @@ public String getExpectedError(ConfigurationError error) {
     private String getHeader() {
         return "\"Problem\",\"Package\",\"File\",\"Priority\",\"Line\",\"Description\",\"Rule set\",\"Rule\"" + PMD.EOL;
     }
-
-    public static junit.framework.Test suite() {
-        return new junit.framework.JUnit4TestAdapter(CSVRendererTest.class);
-    }
 }
diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/CodeClimateRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/CodeClimateRendererTest.java
@@ -6,17 +6,11 @@
 
 import static org.junit.Assert.assertEquals;
 
-import java.io.File;
-
 import org.junit.Test;
 
 import net.sourceforge.pmd.PMD;
 import net.sourceforge.pmd.Report;
 import net.sourceforge.pmd.ReportTest;
-import net.sourceforge.pmd.RuleContext;
-import net.sourceforge.pmd.lang.ast.DummyNode;
-import net.sourceforge.pmd.lang.ast.Node;
-import net.sourceforge.pmd.lang.rule.ParametricRuleViolation;
 import net.sourceforge.pmd.lang.rule.XPathRule;
 
 public class CodeClimateRendererTest extends AbstractRendererTest {
@@ -75,8 +69,8 @@ public String getExpectedMultiple() {
                 + "violationSuppressRegex | | Suppress violations with messages matching a regular expression\\n"
                 + "violationSuppressXPath | | Suppress violations on nodes which match a given relative XPath expression.\\n"
                 + "\"},\"categories\":[\"Style\"],\"location\":{\"path\":\"" + getSourceCodeFilename() + "\",\"lines\":{\"begin\":1,\"end\":1}},\"severity\":\"info\",\"remediation_points\":50000}"
-                + "\u0000" + PMD.EOL + "{\"type\":\"issue\",\"check_name\":\"Foo\",\"description\":\"blah\","
-                + "\"content\":{\"body\":\"## Foo\\n\\nSince: PMD null\\n\\nPriority: High\\n\\n"
+                + "\u0000" + PMD.EOL + "{\"type\":\"issue\",\"check_name\":\"Boo\",\"description\":\"blah\","
+                + "\"content\":{\"body\":\"## Boo\\n\\nSince: PMD null\\n\\nPriority: High\\n\\n"
                 + "[Categories](https://github.com/codeclimate/platform/blob/master/spec/analyzers/SPEC.md#categories): Style\\n\\n"
                 + "[Remediation Points](https://github.com/codeclimate/platform/blob/master/spec/analyzers/SPEC.md#remediation-points): 50000\\n\\n"
                 + "desc\\n\\n"
@@ -90,18 +84,15 @@ public String getExpectedMultiple() {
 
     @Test
     public void testXPathRule() throws Exception {
-        DummyNode node = createNode(1);
-        RuleContext ctx = new RuleContext();
-        ctx.setSourceCodeFile(new File(getSourceCodeFilename()));
         Report report = new Report();
+
         XPathRule theRule = new XPathRule();
         theRule.setProperty(XPathRule.XPATH_DESCRIPTOR, "//dummyNode");
-
         // Setup as FooRule
         theRule.setDescription("desc");
         theRule.setName("Foo");
 
-        report.addRuleViolation(new ParametricRuleViolation<Node>(theRule, ctx, node, "blah"));
+        report.addRuleViolation(newRuleViolation(1, 1, 1, 2, theRule));
         String rendered = ReportTest.render(getRenderer(), report);
 
         // Output should be the exact same as for non xpath rules

diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/JsonRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/JsonRendererTest.java
@@ -15,6 +15,7 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+import net.sourceforge.pmd.FooRule;
 import net.sourceforge.pmd.Report;
 import net.sourceforge.pmd.Report.ConfigurationError;
 import net.sourceforge.pmd.Report.ProcessingError;
@@ -90,7 +91,7 @@ public void suppressedViolations() throws IOException {
         Map<Integer, String> suppressedLines = new HashMap<>();
         suppressedLines.put(1, "test");
         rep.suppress(suppressedLines);
-        rep.addRuleViolation(newRuleViolation(1));
+        rep.addRuleViolation(newRuleViolation(1, 1, 1, 1, new FooRule()));
         String actual = ReportTest.render(getRenderer(), rep);
         String expected = readFile("expected-suppressed.json");
         Assert.assertEquals(filter(expected), filter(actual));

diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/PapariTextRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/PapariTextRendererTest.java
@@ -42,7 +42,7 @@ public String getExpectedEmpty() {
     public String getExpectedMultiple() {
         return "* file: " + getSourceCodeFilename() + PMD.EOL + "    src:  " + getSourceCodeFilename() + ":1:1" + PMD.EOL + "    rule: Foo" + PMD.EOL
                 + "    msg:  blah" + PMD.EOL + "    code: public class Foo {}" + PMD.EOL + PMD.EOL + "    src:  "
-                + getSourceCodeFilename() + ":1:1" + PMD.EOL + "    rule: Foo" + PMD.EOL + "    msg:  blah" + PMD.EOL
+                + getSourceCodeFilename() + ":1:1" + PMD.EOL + "    rule: Boo" + PMD.EOL + "    msg:  blah" + PMD.EOL
                 + "    code: public class Foo {}" + PMD.EOL + PMD.EOL + PMD.EOL + PMD.EOL + "Summary:" + PMD.EOL
                 + PMD.EOL + " : 2" + PMD.EOL + "* warnings: 2" + PMD.EOL;
     }
@@ -67,8 +67,4 @@ public String getExpectedError(ConfigurationError error) {
                 + "    err:  a configuration error" + PMD.EOL + PMD.EOL
                 + "* errors:   1" + PMD.EOL + "* warnings: 0" + PMD.EOL;
     }
-
-    public static junit.framework.Test suite() {
-        return new junit.framework.JUnit4TestAdapter(PapariTextRendererTest.class);
-    }
 }
diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/RenderersTests.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/RenderersTests.java
@@ -20,9 +20,12 @@
     EmacsRendererTest.class,
     HTMLRendererTest.class,
     IDEAJRendererTest.class,
+    JsonRendererTest.class,
     PapariTextRendererTest.class,
+    SarifRendererTest.class,
     SummaryHTMLRendererTest.class,
     TextPadRendererTest.class,
+    TextRendererTest.class,
     VBHTMLRendererTest.class,
     XMLRendererTest.class,
     XSLTRendererTest.class,