SONAR-9904 extract traversal of issue changesets in Changeset

4 files changed, 166 insertions, 48 deletions

diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueCreationDateCalculator.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueCreationDateCalculator.java
index 64816ce6b2a..83cc898e07e 100644
--- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueCreationDateCalculator.java
+++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueCreationDateCalculator.java
@@ -21,22 +21,11 @@ package org.sonar.ce.task.projectanalysis.issue;
 
 import java.util.Comparator;
 import java.util.Date;
-import java.util.HashSet;
-import java.util.Objects;
 import java.util.Optional;
-import java.util.Set;
 import java.util.function.Supplier;
-import java.util.stream.IntStream;
 import javax.annotation.Nullable;
 import org.sonar.api.rule.RuleKey;
 import org.sonar.api.utils.DateUtils;
-import org.sonar.ce.task.projectanalysis.component.Component;
-import org.sonar.core.issue.DefaultIssue;
-import org.sonar.core.issue.IssueChangeContext;
-import org.sonar.db.protobuf.DbCommons.TextRange;
-import org.sonar.db.protobuf.DbIssues;
-import org.sonar.db.protobuf.DbIssues.Flow;
-import org.sonar.db.protobuf.DbIssues.Location;
 import org.sonar.ce.task.projectanalysis.analysis.Analysis;
 import org.sonar.ce.task.projectanalysis.analysis.AnalysisMetadataHolder;
 import org.sonar.ce.task.projectanalysis.analysis.ScannerPlugin;
@@ -46,6 +35,8 @@ import org.sonar.ce.task.projectanalysis.qualityprofile.ActiveRulesHolder;
 import org.sonar.ce.task.projectanalysis.scm.Changeset;
 import org.sonar.ce.task.projectanalysis.scm.ScmInfo;
 import org.sonar.ce.task.projectanalysis.scm.ScmInfoRepository;
+import org.sonar.core.issue.DefaultIssue;
+import org.sonar.core.issue.IssueChangeContext;
 import org.sonar.server.issue.IssueFieldsSetter;
 
 import static org.sonar.core.issue.IssueChangeContext.createScan;
@@ -84,13 +75,13 @@ public class IssueCreationDateCalculator extends IssueVisitor {
       .orElseThrow(illegalStateException("The rule with key '%s' raised an issue, but no rule with that key was found", issue.getRuleKey()));
 
     if (rule.isExternal()) {
-      getScmChangeDate(component, issue).ifPresent(changeDate -> updateDate(issue, changeDate));
+      getDateOfLatestChange(component, issue).ifPresent(changeDate -> updateDate(issue, changeDate));
     } else {
       // Rule can't be inactive (see contract of IssueVisitor)
       ActiveRule activeRule = activeRulesHolder.get(issue.getRuleKey()).get();
       if (firstAnalysis || activeRuleIsNew(activeRule, lastAnalysisOptional.get())
         || ruleImplementationChanged(activeRule.getRuleKey(), activeRule.getPluginKey(), lastAnalysisOptional.get())) {
-        getScmChangeDate(component, issue).ifPresent(changeDate -> updateDate(issue, changeDate));
+        getDateOfLatestChange(component, issue).ifPresent(changeDate -> updateDate(issue, changeDate));
       }
     }
   }
@@ -124,9 +115,9 @@ public class IssueCreationDateCalculator extends IssueVisitor {
     return lastAnalysisDate < ruleCreationDate;
   }
 
-  private Optional<Date> getScmChangeDate(Component component, DefaultIssue issue) {
+  private Optional<Date> getDateOfLatestChange(Component component, DefaultIssue issue) {
     return getScmInfo(component)
-      .flatMap(scmInfo -> getChangeset(component, scmInfo, issue))
+      .flatMap(scmInfo -> getLatestChangeset(component, scmInfo, issue))
       .map(IssueCreationDateCalculator::getChangeDate);
   }
 
@@ -138,36 +129,17 @@ public class IssueCreationDateCalculator extends IssueVisitor {
     return scmInfoRepository.getScmInfo(component);
   }
 
-  private static Optional<Changeset> getChangeset(Component component, ScmInfo scmInfo, DefaultIssue issue) {
-    Set<Integer> involvedLines = new HashSet<>();
-    DbIssues.Locations locations = issue.getLocations();
-    if (locations != null) {
-      if (locations.hasTextRange()) {
-        addLines(involvedLines, locations.getTextRange());
-      }
-      for (Flow f : locations.getFlowList()) {
-        for (Location l : f.getLocationList()) {
-          if (Objects.equals(l.getComponentId(), component.getUuid())) {
-            // Ignore locations in other files, since it is currently not very common, and this is hard to load SCM by component UUID.
-            addLines(involvedLines, l.getTextRange());
-          }
-        }
-      }
-      if (!involvedLines.isEmpty()) {
-        return involvedLines.stream()
-          .filter(scmInfo::hasChangesetForLine)
-          .map(scmInfo::getChangesetForLine)
-          .max(Comparator.comparingLong(Changeset::getDate));
-      }
+  private static Optional<Changeset> getLatestChangeset(Component component, ScmInfo scmInfo, DefaultIssue issue) {
+    Optional<Changeset> mostRecentChangeset = IssueLocations.allLinesFor(issue, component.getUuid())
+      .filter(scmInfo::hasChangesetForLine)
+      .mapToObj(scmInfo::getChangesetForLine)
+      .max(Comparator.comparingLong(Changeset::getDate));
+    if (mostRecentChangeset.isPresent()) {
+      return mostRecentChangeset;
     }
-
     return Optional.of(scmInfo.getLatestChangeset());
   }
 
-  private static void addLines(Set<Integer> involvedLines, TextRange range) {
-    IntStream.rangeClosed(range.getStartLine(), range.getEndLine()).forEach(involvedLines::add);
-  }
-
   private static Date getChangeDate(Changeset changesetForLine) {
     return DateUtils.longToDate(changesetForLine.getDate());
   }
diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLocations.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLocations.java
new file mode 100644
index 00000000000..ed356f02427
--- /dev/null
+++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/IssueLocations.java
@@ -0,0 +1,57 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.ce.task.projectanalysis.issue;
+
+import java.util.Objects;
+import java.util.stream.IntStream;
+import java.util.stream.Stream;
+import org.sonar.core.issue.DefaultIssue;
+import org.sonar.db.protobuf.DbCommons;
+import org.sonar.db.protobuf.DbIssues;
+
+class IssueLocations {
+
+  private IssueLocations() {
+    // do not instantiate
+  }
+
+  /**
+   * Extract the lines of all the locations in the specified component. All the flows and secondary locations
+   * are taken into account. The lines present in multiple flows and locations are kept
+   * duplicated. Ordering of results is not guaranteed.
+   * <p>
+   * TODO should be a method of DefaultIssue, as soon as it's no
+   * longer in sonar-core and can access sonar-db-dao.
+   */
+  public static IntStream allLinesFor(DefaultIssue issue, String componentId) {
+    DbIssues.Locations locations = issue.getLocations();
+    if (locations == null) {
+      return IntStream.empty();
+    }
+
+    Stream<DbCommons.TextRange> textRanges = Stream.concat(
+      locations.hasTextRange() ? Stream.of(locations.getTextRange()) : Stream.empty(),
+      locations.getFlowList().stream()
+        .flatMap(f -> f.getLocationList().stream())
+        .filter(l -> Objects.equals(l.getComponentId(), componentId))
+        .map(DbIssues.Location::getTextRange));
+    return textRanges.flatMapToInt(range -> IntStream.rangeClosed(range.getStartLine(), range.getEndLine()));
+  }
+}
diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLocationsTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLocationsTest.java
new file mode 100644
index 00000000000..4d23cf46457
--- /dev/null
+++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/IssueLocationsTest.java
@@ -0,0 +1,89 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.ce.task.projectanalysis.issue;
+
+import org.junit.Test;
+import org.sonar.core.issue.DefaultIssue;
+import org.sonar.db.protobuf.DbCommons;
+import org.sonar.db.protobuf.DbIssues;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class IssueLocationsTest {
+
+  @Test
+  public void allLinesFor_filters_lines_for_specified_component() {
+    DbIssues.Locations.Builder locations = DbIssues.Locations.newBuilder();
+    locations.addFlowBuilder()
+      .addLocation(newLocation("file1", 5, 5))
+      .addLocation(newLocation("file1", 10, 12))
+      .addLocation(newLocation("file1", 15, 15))
+      .addLocation(newLocation("file2", 10, 11))
+      .build();
+    DefaultIssue issue = new DefaultIssue().setLocations(locations.build());
+
+    assertThat(IssueLocations.allLinesFor(issue, "file1")).containsExactlyInAnyOrder(5, 10, 11, 12, 15);
+    assertThat(IssueLocations.allLinesFor(issue, "file2")).containsExactlyInAnyOrder(10, 11);
+    assertThat(IssueLocations.allLinesFor(issue, "file3")).isEmpty();
+  }
+
+  @Test
+  public void allLinesFor_traverses_all_flows() {
+    DbIssues.Locations.Builder locations = DbIssues.Locations.newBuilder();
+    locations.addFlowBuilder()
+      .addLocation(newLocation("file1", 5, 5))
+      .addLocation(newLocation("file2", 10, 11))
+      .build();
+    locations.addFlowBuilder()
+      .addLocation(newLocation("file1", 7, 9))
+      .addLocation(newLocation("file2", 12, 12))
+      .build();
+    DefaultIssue issue = new DefaultIssue().setLocations(locations.build());
+
+    assertThat(IssueLocations.allLinesFor(issue, "file1")).containsExactlyInAnyOrder(5, 7, 8, 9);
+    assertThat(IssueLocations.allLinesFor(issue, "file2")).containsExactlyInAnyOrder(10, 11, 12);
+  }
+
+  @Test
+  public void allLinesFor_keeps_duplicated_lines() {
+    DbIssues.Locations.Builder locations = DbIssues.Locations.newBuilder();
+    locations.addFlowBuilder()
+      .addLocation(newLocation("file1", 5, 5))
+      .addLocation(newLocation("file1", 4, 6))
+      .build();
+    DefaultIssue issue = new DefaultIssue().setLocations(locations.build());
+
+    assertThat(IssueLocations.allLinesFor(issue, "file1")).containsExactlyInAnyOrder(4, 5, 5, 6);
+  }
+
+  @Test
+  public void allLinesFor_returns_empty_if_no_locations_are_set() {
+    DefaultIssue issue = new DefaultIssue().setLocations(null);
+
+    assertThat(IssueLocations.allLinesFor(issue, "file1")).isEmpty();
+  }
+
+  private static DbIssues.Location newLocation(String componentId, int startLine, int endLine) {
+    return DbIssues.Location.newBuilder()
+      .setComponentId(componentId)
+      .setTextRange(DbCommons.TextRange.newBuilder().setStartLine(startLine).setEndLine(endLine).build())
+      .build();
+  }
+}
diff --git a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ChangesetTest.java b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ChangesetTest.java
index 9137f911e25..70bbca1435c 100644
--- a/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ChangesetTest.java
+++ b/server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/scm/ChangesetTest.java
@@ -28,7 +28,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 public class ChangesetTest {
 
   @Rule
-  public ExpectedException thrown = ExpectedException.none();
+  public ExpectedException expectedException = ExpectedException.none();
 
   @Test
   public void create_changeset() {
@@ -55,17 +55,17 @@ public class ChangesetTest {
   }
 
   @Test
-  public void fail_with_NPE_when_setting_null_date() throws Exception {
-    thrown.expect(NullPointerException.class);
-    thrown.expectMessage("Date cannot be null");
+  public void fail_with_NPE_when_setting_null_date() {
+    expectedException.expect(NullPointerException.class);
+    expectedException.expectMessage("Date cannot be null");
 
     Changeset.newChangesetBuilder().setDate(null);
   }
 
   @Test
   public void fail_with_NPE_when_building_without_date() {
-    thrown.expect(NullPointerException.class);
-    thrown.expectMessage("Date cannot be null");
+    expectedException.expect(NullPointerException.class);
+    expectedException.expectMessage("Date cannot be null");
 
     Changeset.newChangesetBuilder()
       .setAuthor("john")
@@ -85,7 +85,7 @@ public class ChangesetTest {
   }
 
   @Test
-  public void equals_and_hashcode_are_based_on_all_fields() throws Exception {
+  public void equals_and_hashcode_are_based_on_all_fields() {
     Changeset.Builder changesetBuilder = Changeset.newChangesetBuilder()
       .setAuthor("john")
       .setDate(123456789L)