SONAR-16583 Collection of hashes of issue locations takes too long

2 files changed, 120 insertions, 9 deletions

diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java b/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java
index 2b027eb37ac..1b7afe26d32 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java
@@ -35,6 +35,7 @@ import org.sonar.api.utils.Duration;
 import org.sonar.core.issue.DefaultIssue;
 import org.sonar.core.issue.DefaultIssueComment;
 import org.sonar.core.issue.IssueChangeContext;
+import org.sonar.db.protobuf.DbIssues;
 import org.sonar.db.user.UserDto;
 
 import static com.google.common.base.Preconditions.checkState;
@@ -171,15 +172,54 @@ public class IssueFieldsSetter {
     return false;
   }
 
+  /**
+   * New value will be set if the locations are different, ignoring the hashes. If that's the case, we mark the issue as changed,
+   * and we also flag that the locations have changed, so that we calculate all the hashes later, in an efficient way.
+   * WARNING: It is possible that the hashes changes without the text ranges changing, but for optimization we take that risk.
+   *
+   * @see ComputeLocationHashesVisitor
+   */
   public boolean setLocations(DefaultIssue issue, @Nullable Object locations) {
-    if (!Objects.equals(locations, issue.getLocations())) {
+    if (!locationsEqualsIgnoreHashes(locations, issue.getLocations())) {
       issue.setLocations(locations);
       issue.setChanged(true);
+      issue.setLocationsChanged(true);
       return true;
     }
     return false;
   }
 
+  private static boolean locationsEqualsIgnoreHashes(@Nullable Object l1, @Nullable DbIssues.Locations l2) {
+    if (l1 == null && l2 == null) {
+      return true;
+    }
+
+    if (l2 == null || !(l1 instanceof DbIssues.Locations)) {
+      return false;
+    }
+
+    DbIssues.Locations l1c = (DbIssues.Locations) l1;
+    if (!Objects.equals(l1c.getTextRange(), l2.getTextRange()) || l1c.getFlowCount() != l2.getFlowCount()) {
+      return false;
+    }
+
+    for (int i = 0; i < l1c.getFlowCount(); i++) {
+      if (l1c.getFlow(i).getLocationCount() != l2.getFlow(i).getLocationCount()) {
+        return false;
+      }
+      for (int j = 0; j < l1c.getFlow(i).getLocationCount(); j++) {
+        if (!locationEqualsIgnoreHashes(l1c.getFlow(i).getLocation(j), l2.getFlow(i).getLocation(j))) {
+          return false;
+        }
+      }
+    }
+    return true;
+  }
+
+  private static boolean locationEqualsIgnoreHashes(DbIssues.Location l1, DbIssues.Location l2) {
+    return Objects.equals(l1.getComponentId(), l2.getComponentId()) && Objects.equals(l1.getTextRange(), l2.getTextRange()) && Objects.equals(l1.getMsg(), l2.getMsg());
+  }
+
   public boolean setPastLocations(DefaultIssue issue, @Nullable Object previousLocations) {
     Object currentLocations = issue.getLocations();
     issue.setLocations(previousLocations);
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java
index c0835a1b701..65801c0e591 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java
@@ -28,6 +28,8 @@ import org.sonar.api.utils.Duration;
 import org.sonar.core.issue.DefaultIssue;
 import org.sonar.core.issue.FieldDiffs;
 import org.sonar.core.issue.IssueChangeContext;
+import org.sonar.db.protobuf.DbCommons;
+import org.sonar.db.protobuf.DbIssues;
 import org.sonar.db.user.UserDto;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -269,21 +271,90 @@ public class IssueFieldsSetterTest {
   }
 
   @Test
-  public void change_locations() {
-    issue.setLocations("[1-3]");
-    boolean updated = underTest.setLocations(issue, "[1-4]");
+  public void change_locations_if_primary_text_rage_changed() {
+    DbCommons.TextRange range = DbCommons.TextRange.newBuilder().setStartLine(1).build();
+    DbIssues.Locations locations = DbIssues.Locations.newBuilder()
+      .setTextRange(range)
+      .build();
+    DbIssues.Locations locations2 = locations.toBuilder().setTextRange(range.toBuilder().setEndLine(2).build()).build();
+    issue.setLocations(locations);
+    boolean updated = underTest.setLocations(issue, locations2);
     assertThat(updated).isTrue();
-    assertThat(issue.getLocations().toString()).isEqualTo("[1-4]");
+    assertThat((Object) issue.getLocations()).isEqualTo(locations2);
+    assertThat(issue.locationsChanged()).isTrue();
     assertThat(issue.currentChange()).isNull();
     assertThat(issue.mustSendNotifications()).isFalse();
   }
 
   @Test
-  public void do_not_change_locations() {
-    issue.setLocations("[1-3]");
-    boolean updated = underTest.setLocations(issue, "[1-3]");
+  public void change_locations_if_secondary_text_rage_changed() {
+    DbCommons.TextRange range = DbCommons.TextRange.newBuilder().setStartLine(1).build();
+    DbIssues.Locations locations = DbIssues.Locations.newBuilder()
+      .addFlow(DbIssues.Flow.newBuilder()
+        .addLocation(DbIssues.Location.newBuilder().setTextRange(range))
+        .build())
+      .build();
+    issue.setLocations(locations);
+    DbIssues.Locations.Builder builder = locations.toBuilder();
+    builder.getFlowBuilder(0).getLocationBuilder(0).setTextRange(range.toBuilder().setEndLine(2));
+    boolean updated = underTest.setLocations(issue, builder.build());
+    assertThat(updated).isTrue();
+  }
+
+  @Test
+  public void change_locations_if_secondary_message_changed() {
+    DbIssues.Locations locations = DbIssues.Locations.newBuilder()
+      .addFlow(DbIssues.Flow.newBuilder()
+        .addLocation(DbIssues.Location.newBuilder().setMsg("msg1"))
+        .build())
+      .build();
+    issue.setLocations(locations);
+    DbIssues.Locations.Builder builder = locations.toBuilder();
+    builder.getFlowBuilder(0).getLocationBuilder(0).setMsg("msg2");
+    boolean updated = underTest.setLocations(issue, builder.build());
+    assertThat(updated).isTrue();
+  }
+
+  @Test
+  public void change_locations_if_different_flow_count() {
+    DbIssues.Locations locations = DbIssues.Locations.newBuilder()
+      .addFlow(DbIssues.Flow.newBuilder()
+        .addLocation(DbIssues.Location.newBuilder())
+        .build())
+      .build();
+    issue.setLocations(locations);
+    DbIssues.Locations.Builder builder = locations.toBuilder();
+    builder.clearFlow();
+    boolean updated = underTest.setLocations(issue, builder.build());
+    assertThat(updated).isTrue();
+  }
+
+  @Test
+  public void do_not_change_locations_if_primary_hash_changed() {
+    DbCommons.TextRange range = DbCommons.TextRange.newBuilder().setStartLine(1).build();
+    DbIssues.Locations locations = DbIssues.Locations.newBuilder()
+      .setTextRange(range)
+      .setChecksum("1")
+      .build();
+    issue.setLocations(locations);
+    boolean updated = underTest.setLocations(issue, locations.toBuilder().setChecksum("2").build());
+    assertThat(updated).isFalse();
+  }
+
+  @Test
+  public void do_not_change_locations_if_secondary_hash_changed() {
+    DbCommons.TextRange range = DbCommons.TextRange.newBuilder().setStartLine(1).build();
+    DbIssues.Locations locations = DbIssues.Locations.newBuilder()
+      .addFlow(DbIssues.Flow.newBuilder()
+        .addLocation(DbIssues.Location.newBuilder().setTextRange(range))
+        .build())
+      .setChecksum("1")
+      .build();
+    issue.setLocations(locations);
+    DbIssues.Locations.Builder builder = locations.toBuilder();
+    builder.getFlowBuilder(0).getLocationBuilder(0).setChecksum("2");
+    boolean updated = underTest.setLocations(issue, builder.build());
     assertThat(updated).isFalse();
-    assertThat(issue.getLocations().toString()).isEqualTo("[1-3]");
     assertThat(issue.currentChange()).isNull();
     assertThat(issue.mustSendNotifications()).isFalse();
   }