SONAR-2928 Fix handling of new violations in case of null checksum

4 files changed, 52 insertions, 25 deletions

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/SourceChecksum.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/SourceChecksum.java
index 8d6c327e749..c6eba62ca15 100644
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/SourceChecksum.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/SourceChecksum.java
@@ -33,6 +33,17 @@ public final class SourceChecksum {
     // only static methods
   }
 
+  /**
+   * @param line line number (first line has number 1)
+   * @return checksum or null if checksum not exists for line
+   */
+  public static String getChecksumForLine(List<String> checksums, Integer line) {
+    if (line == null || line < 1 || line > checksums.size()) {
+      return null;
+    }
+    return checksums.get(line - 1);
+  }
+
   public static List<String> lineChecksumsOfFile(String file) {
     List<String> result = Lists.newArrayList();
     if (file != null) {
diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecorator.java
index 67ca1ec63c5..d766618d6be 100644
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecorator.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecorator.java
@@ -74,7 +74,7 @@ public class ViolationTrackingDecorator implements Decorator {
     List<Violation> result = Lists.newArrayList();
     List<String> checksums = SourceChecksum.lineChecksumsOfFile(index.getSource(context.getResource()));
     for (Violation violation : context.getViolations()) {
-      violation.setChecksum(getChecksumForLine(checksums, violation.getLineId()));
+      violation.setChecksum(SourceChecksum.getChecksumForLine(checksums, violation.getLineId()));
       result.add(violation);
     }
     return result;
@@ -171,14 +171,10 @@ public class ViolationTrackingDecorator implements Decorator {
   }
 
   private boolean isSameChecksum(Violation newViolation, RuleFailureModel pastViolation) {
-    return pastViolation.getChecksum() != null
-        && StringUtils.equals(pastViolation.getChecksum(), newViolation.getChecksum());
+    return StringUtils.equals(pastViolation.getChecksum(), newViolation.getChecksum());
   }
 
   private boolean isSameLine(Violation newViolation, RuleFailureModel pastViolation) {
-    if (pastViolation.getLine() == null && newViolation.getLineId() == null) {
-      return true;
-    }
     return ObjectUtils.equals(pastViolation.getLine(), newViolation.getLineId());
   }
 
@@ -201,16 +197,6 @@ public class ViolationTrackingDecorator implements Decorator {
     }
   }
 
-  /**
-   * @return checksum or null if checksum not exists for line
-   */
-  private String getChecksumForLine(List<String> checksums, Integer line) {
-    if (line == null || line < 1 || line > checksums.size()) {
-      return null;
-    }
-    return checksums.get(line - 1);
-  }
-
   @Override
   public String toString() {
     return getClass().getSimpleName();
diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/SourceChecksumTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/SourceChecksumTest.java
index 3c361aa61da..92c8b954874 100644
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/SourceChecksumTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/SourceChecksumTest.java
@@ -19,14 +19,23 @@
  */
 package org.sonar.plugins.core.timemachine;
 
-import org.junit.Test;
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.assertThat;
 
 import java.util.List;
 
-import static org.hamcrest.Matchers.*;
-import static org.junit.Assert.assertThat;
+import org.junit.Test;
 
 public class SourceChecksumTest {
+  @Test
+  public void shouldGetChecksumForLine() {
+    List<String> checksums = SourceChecksum.lineChecksumsOfFile("line");
+    assertThat(SourceChecksum.getChecksumForLine(checksums, null), nullValue());
+    assertThat(SourceChecksum.getChecksumForLine(checksums, 0), nullValue());
+    assertThat(SourceChecksum.getChecksumForLine(checksums, 1), notNullValue());
+    assertThat(SourceChecksum.getChecksumForLine(checksums, 2), nullValue());
+  }
+
   /**
    * See http://jira.codehaus.org/browse/SONAR-2358
    */
diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecoratorTest.java
index 1da2991ece6..5aee8dc0586 100644
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecoratorTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/timemachine/ViolationTrackingDecoratorTest.java
@@ -31,7 +31,6 @@ import org.sonar.api.utils.DateUtils;
 import java.text.ParseException;
 import java.util.Collections;
 import java.util.Date;
-import java.util.List;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.*;
@@ -62,7 +61,22 @@ public class ViolationTrackingDecoratorTest {
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation1, newViolation2),
         Lists.newArrayList(referenceViolation1, referenceViolation2));
     assertThat(mapping.get(newViolation1), equalTo(referenceViolation1));
+    assertThat(newViolation1.isNew(), is(false));
     assertThat(mapping.get(newViolation2), equalTo(referenceViolation2));
+    assertThat(newViolation2.isNew(), is(false));
+  }
+
+  /**
+   * See SONAR-2928
+   */
+  @Test
+  public void sameRuleAndNullLineAndChecksumButDifferentMessages() {
+    Violation newViolation = newViolation("new message", null, 50, "checksum1");
+    RuleFailureModel referenceViolation = newReferenceViolation("old message", null, 50, "checksum1");
+
+    Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(mapping.get(newViolation), equalTo(referenceViolation));
+    assertThat(newViolation.isNew(), is(false));
   }
 
   @Test
@@ -72,6 +86,7 @@ public class ViolationTrackingDecoratorTest {
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
     assertThat(mapping.get(newViolation), equalTo(referenceViolation));
+    assertThat(newViolation.isNew(), is(false));
   }
 
   @Test
@@ -91,6 +106,7 @@ public class ViolationTrackingDecoratorTest {
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
     assertThat(mapping.get(newViolation), is(nullValue()));
+    assertThat(newViolation.isNew(), is(true));
   }
 
   @Test
@@ -100,6 +116,7 @@ public class ViolationTrackingDecoratorTest {
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
     assertThat(mapping.get(newViolation), equalTo(referenceViolation));
+    assertThat(newViolation.isNew(), is(false));
   }
 
   /**
@@ -112,6 +129,7 @@ public class ViolationTrackingDecoratorTest {
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
     assertThat(mapping.get(newViolation), equalTo(referenceViolation));
+    assertThat(newViolation.isNew(), is(false));
   }
 
   @Test
@@ -121,6 +139,7 @@ public class ViolationTrackingDecoratorTest {
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
     assertThat(mapping.get(newViolation), is(nullValue()));
+    assertThat(newViolation.isNew(), is(true));
   }
 
   @Test
@@ -137,7 +156,7 @@ public class ViolationTrackingDecoratorTest {
   public void shouldCompareViolationsWithDatabaseFormat() {
     // violation messages are trimmed and can be abbreviated when persisted in database.
     // Comparing violation messages must use the same format.
-    Violation newViolation = newViolation("message", 1, 50, "checksum1");
+    Violation newViolation = newViolation(" message ", 1, 50, "checksum1");
     RuleFailureModel referenceViolation = newReferenceViolation("       message       ", 1, 50, "checksum2");
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
@@ -170,19 +189,19 @@ public class ViolationTrackingDecoratorTest {
     assertThat(newViolation.isNew(), is(false));
   }
 
-  private Violation newViolation(String message, int lineId, int ruleId) {
+  private Violation newViolation(String message, Integer lineId, int ruleId) {
     Rule rule = Rule.create().setKey("rule");
     rule.setId(ruleId);
     return Violation.create(rule, null).setLineId(lineId).setMessage(message);
   }
 
-  private Violation newViolation(String message, int lineId, int ruleId, String lineChecksum) {
+  private Violation newViolation(String message, Integer lineId, int ruleId, String lineChecksum) {
     return newViolation(message, lineId, ruleId).setChecksum(lineChecksum);
   }
 
-  private RuleFailureModel newReferenceViolation(String message, int lineId, int ruleId, String lineChecksum) {
+  private RuleFailureModel newReferenceViolation(String message, Integer lineId, int ruleId, String lineChecksum) {
     RuleFailureModel referenceViolation = new RuleFailureModel();
-    referenceViolation.setId(lineId + ruleId);
+    referenceViolation.setId(violationId++);
     referenceViolation.setLine(lineId);
     referenceViolation.setMessage(message);
     referenceViolation.setRuleId(ruleId);
@@ -190,4 +209,6 @@ public class ViolationTrackingDecoratorTest {
     return referenceViolation;
   }
 
+  private int violationId = 0;
+
 }