SONAR-3837 Fix violation tracking mechanism

2 files changed, 77 insertions, 73 deletions

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 6da3b0153ff..e2e52396996 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
@@ -61,6 +61,11 @@ public class ViolationTrackingDecorator implements Decorator {
   private SonarIndex index;
   private Project project;
 
+  /**
+   * Live collection of unmapped past violations.
+   */
+  private Set<RuleFailureModel> unmappedPastViolations = Sets.newHashSet();
+
   public ViolationTrackingDecorator(Project project, ReferenceAnalysis referenceAnalysis, SonarIndex index) {
     this.referenceAnalysis = referenceAnalysis;
     this.index = index;
@@ -112,6 +117,8 @@ public class ViolationTrackingDecorator implements Decorator {
 
   @VisibleForTesting
   Map<Violation, RuleFailureModel> mapViolations(List<Violation> newViolations, List<RuleFailureModel> pastViolations, String source, Resource resource) {
+    unmappedPastViolations.addAll(pastViolations);
+
     Multimap<Integer, RuleFailureModel> pastViolationsByRule = LinkedHashMultimap.create();
     for (RuleFailureModel pastViolation : pastViolations) {
       pastViolationsByRule.put(pastViolation.getRuleId(), pastViolation);
@@ -126,7 +133,7 @@ public class ViolationTrackingDecorator implements Decorator {
 
     // Try first to match violations on same rule with same line and with same checksum (but not necessarily with same message)
     for (Violation newViolation : newViolations) {
-      if (isNotAlreadyMapped(newViolation, referenceViolationsMap)) {
+      if (isNotAlreadyMapped(newViolation)) {
         mapViolation(newViolation,
             findPastViolationWithSameLineAndChecksum(newViolation, pastViolationsByRule.get(newViolation.getRule().getId())),
             pastViolationsByRule, referenceViolationsMap);
@@ -145,7 +152,7 @@ public class ViolationTrackingDecorator implements Decorator {
           ViolationTrackingBlocksRecognizer rec = new ViolationTrackingBlocksRecognizer(hashedReference, hashedSource, hashedComparator);
 
           Multimap<Integer, Violation> newViolationsByLines = newViolationsByLines(newViolations, rec);
-          Multimap<Integer, RuleFailureModel> pastViolationsByLines = pastViolationsByLines(pastViolations, rec);
+          Multimap<Integer, RuleFailureModel> pastViolationsByLines = pastViolationsByLines(unmappedPastViolations, rec);
 
           RollingHashSequence<HashedSequence<StringText>> a = RollingHashSequence.wrap(hashedReference, hashedComparator, 5);
           RollingHashSequence<HashedSequence<StringText>> b = RollingHashSequence.wrap(hashedSource, hashedComparator, 5);
@@ -176,12 +183,10 @@ public class ViolationTrackingDecorator implements Decorator {
             }
           }
 
-          Set<RuleFailureModel> unmappedPastViolations = Sets.newHashSet(pastViolations);
-
           for (HashOccurrence hashOccurrence : map.values()) {
             if (hashOccurrence.countA == 1 && hashOccurrence.countB == 1) {
               // Guaranteed that lineA has been moved to lineB, so we can map all violations on lineA to all violations on lineB
-              map(newViolationsByLines.get(hashOccurrence.lineB), pastViolationsByLines.get(hashOccurrence.lineA), unmappedPastViolations, pastViolationsByRule);
+              map(newViolationsByLines.get(hashOccurrence.lineB), pastViolationsByLines.get(hashOccurrence.lineA), pastViolationsByRule);
               pastViolationsByLines.removeAll(hashOccurrence.lineA);
               newViolationsByLines.removeAll(hashOccurrence.lineB);
             }
@@ -199,7 +204,7 @@ public class ViolationTrackingDecorator implements Decorator {
             Collections.sort(possibleLinePairs, LINE_PAIR_COMPARATOR);
             for (LinePair linePair : possibleLinePairs) {
               // High probability that lineA has been moved to lineB, so we can map all violations on lineA to all violations on lineB
-              map(newViolationsByLines.get(linePair.lineB), pastViolationsByLines.get(linePair.lineA), unmappedPastViolations, pastViolationsByRule);
+              map(newViolationsByLines.get(linePair.lineB), pastViolationsByLines.get(linePair.lineA), pastViolationsByRule);
             }
           }
         }
@@ -207,7 +212,7 @@ public class ViolationTrackingDecorator implements Decorator {
 
       // Try then to match violations on same rule with same message and with same checksum
       for (Violation newViolation : newViolations) {
-        if (isNotAlreadyMapped(newViolation, referenceViolationsMap)) {
+        if (isNotAlreadyMapped(newViolation)) {
           mapViolation(newViolation,
               findPastViolationWithSameChecksumAndMessage(newViolation, pastViolationsByRule.get(newViolation.getRule().getId())),
               pastViolationsByRule, referenceViolationsMap);
@@ -216,7 +221,7 @@ public class ViolationTrackingDecorator implements Decorator {
 
       // Try then to match violations on same rule with same line and with same message
       for (Violation newViolation : newViolations) {
-        if (isNotAlreadyMapped(newViolation, referenceViolationsMap)) {
+        if (isNotAlreadyMapped(newViolation)) {
           mapViolation(newViolation,
               findPastViolationWithSameLineAndMessage(newViolation, pastViolationsByRule.get(newViolation.getRule().getId())),
               pastViolationsByRule, referenceViolationsMap);
@@ -226,23 +231,24 @@ public class ViolationTrackingDecorator implements Decorator {
       // Last check: match violation if same rule and same checksum but different line and different message
       // See SONAR-2812
       for (Violation newViolation : newViolations) {
-        if (isNotAlreadyMapped(newViolation, referenceViolationsMap)) {
+        if (isNotAlreadyMapped(newViolation)) {
           mapViolation(newViolation,
               findPastViolationWithSameChecksum(newViolation, pastViolationsByRule.get(newViolation.getRule().getId())),
               pastViolationsByRule, referenceViolationsMap);
         }
       }
     }
+
+    unmappedPastViolations.clear();
+
     return referenceViolationsMap;
   }
 
-  private void map(Collection<Violation> newViolations, Collection<RuleFailureModel> pastViolations, Set<RuleFailureModel> unmappedPastViolations,
-      Multimap<Integer, RuleFailureModel> pastViolationsByRule) {
+  private void map(Collection<Violation> newViolations, Collection<RuleFailureModel> pastViolations, Multimap<Integer, RuleFailureModel> pastViolationsByRule) {
     for (Violation newViolation : newViolations) {
-      if (isNotAlreadyMapped(newViolation, referenceViolationsMap)) {
+      if (isNotAlreadyMapped(newViolation)) {
         for (RuleFailureModel pastViolation : pastViolations) {
-          if (unmappedPastViolations.contains(pastViolation) && Objects.equal(newViolation.getRule().getId(), pastViolation.getRuleId())) {
-            unmappedPastViolations.remove(pastViolation);
+          if (isNotAlreadyMapped(pastViolation) && Objects.equal(newViolation.getRule().getId(), pastViolation.getRuleId())) {
             mapViolation(newViolation, pastViolation, pastViolationsByRule, referenceViolationsMap);
             break;
           }
@@ -251,10 +257,10 @@ public class ViolationTrackingDecorator implements Decorator {
     }
   }
 
-  private Multimap<Integer, Violation> newViolationsByLines(List<Violation> newViolations, ViolationTrackingBlocksRecognizer rec) {
+  private Multimap<Integer, Violation> newViolationsByLines(Collection<Violation> newViolations, ViolationTrackingBlocksRecognizer rec) {
     Multimap<Integer, Violation> newViolationsByLines = LinkedHashMultimap.create();
     for (Violation newViolation : newViolations) {
-      if (isNotAlreadyMapped(newViolation, referenceViolationsMap)) {
+      if (isNotAlreadyMapped(newViolation)) {
         if (rec.isValidLineInSource(newViolation.getLineId())) {
           newViolationsByLines.put(newViolation.getLineId(), newViolation);
         }
@@ -263,7 +269,7 @@ public class ViolationTrackingDecorator implements Decorator {
     return newViolationsByLines;
   }
 
-  private Multimap<Integer, RuleFailureModel> pastViolationsByLines(List<RuleFailureModel> pastViolations, ViolationTrackingBlocksRecognizer rec) {
+  private Multimap<Integer, RuleFailureModel> pastViolationsByLines(Collection<RuleFailureModel> pastViolations, ViolationTrackingBlocksRecognizer rec) {
     Multimap<Integer, RuleFailureModel> pastViolationsByLines = LinkedHashMultimap.create();
     for (RuleFailureModel pastViolation : pastViolations) {
       if (rec.isValidLineInSource(pastViolation.getLine())) {
@@ -298,8 +304,12 @@ public class ViolationTrackingDecorator implements Decorator {
     int countB;
   }
 
-  private boolean isNotAlreadyMapped(Violation newViolation, Map<Violation, RuleFailureModel> violationMap) {
-    return !violationMap.containsKey(newViolation);
+  private boolean isNotAlreadyMapped(RuleFailureModel pastViolation) {
+    return unmappedPastViolations.contains(pastViolation);
+  }
+
+  private boolean isNotAlreadyMapped(Violation newViolation) {
+    return !referenceViolationsMap.containsKey(newViolation);
   }
 
   private RuleFailureModel findPastViolationWithSameChecksum(Violation newViolation, Collection<RuleFailureModel> pastViolations) {
@@ -373,7 +383,7 @@ public class ViolationTrackingDecorator implements Decorator {
       newViolation.setNew(false);
       pastViolationsByRule.remove(newViolation.getRule().getId(), pastViolation);
       violationMap.put(newViolation, pastViolation);
-
+      unmappedPastViolations.remove(pastViolation);
     } else {
       newViolation.setNew(true);
       newViolation.setCreatedAt(project.getAnalysisDate());
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 911f66ff443..5b718405220 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
@@ -32,10 +32,7 @@ import java.util.Collections;
 import java.util.Date;
 import java.util.Map;
 
-import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.nullValue;
-import static org.junit.Assert.assertThat;
+import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -58,11 +55,9 @@ public class ViolationTrackingDecoratorTest {
     Violation newViolation = newViolation("message", 10, 1, "checksum1"); // exactly the fields of referenceViolation1
     newViolation.setPermanentId(200);
 
-    Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation),
-        Lists.newArrayList(referenceViolation1, referenceViolation2));
-
-    assertThat(mapping.get(newViolation), equalTo(referenceViolation2));// same permanent id
-    assertThat(newViolation.isNew(), is(false));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation1, referenceViolation2));
+    assertThat(decorator.getReferenceViolation(newViolation)).isSameAs(referenceViolation2); // same permanent id
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   @Test
@@ -73,25 +68,24 @@ public class ViolationTrackingDecoratorTest {
     Violation newViolation1 = newViolation("message", 3, 50, "checksum1");
     Violation newViolation2 = newViolation("message", 5, 50, "checksum2");
 
-    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));
+    decorator.mapViolations(Lists.newArrayList(newViolation1, newViolation2), Lists.newArrayList(referenceViolation1, referenceViolation2));
+    assertThat(decorator.getReferenceViolation(newViolation1)).isSameAs(referenceViolation1);
+    assertThat(newViolation1.isNew()).isFalse();
+    assertThat(decorator.getReferenceViolation(newViolation2)).isSameAs(referenceViolation2);
+    assertThat(newViolation2.isNew()).isFalse();
   }
 
   /**
-   * See SONAR-2928
+   * 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));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isSameAs(referenceViolation);
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   @Test
@@ -99,19 +93,19 @@ public class ViolationTrackingDecoratorTest {
     Violation newViolation = newViolation("new message", 1, 50, "checksum1");
     RuleFailureModel referenceViolation = newReferenceViolation("old message", 1, 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));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isSameAs(referenceViolation);
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   @Test
   public void sameRuleAndLineMessage() {
     Violation newViolation = newViolation("message", 1, 50, "checksum1");
-    RuleFailureModel refernceViolation = newReferenceViolation("message", 1, 50, "checksum2");
+    RuleFailureModel referenceViolation = newReferenceViolation("message", 1, 50, "checksum2");
 
-    Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(refernceViolation));
-    assertThat(mapping.get(newViolation), equalTo(refernceViolation));
-    assertThat(newViolation.isNew(), is(false));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isSameAs(referenceViolation);
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   @Test
@@ -119,9 +113,9 @@ public class ViolationTrackingDecoratorTest {
     Violation newViolation = newViolation("message", 1, 50, null);
     RuleFailureModel referenceViolation = newReferenceViolation("message", 1, 51, null);
 
-    Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
-    assertThat(mapping.get(newViolation), is(nullValue()));
-    assertThat(newViolation.isNew(), is(true));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isNull();
+    assertThat(newViolation.isNew()).isTrue();
   }
 
   @Test
@@ -129,22 +123,22 @@ public class ViolationTrackingDecoratorTest {
     Violation newViolation = newViolation("message", 1, 50, "checksum1");
     RuleFailureModel referenceViolation = newReferenceViolation("message", 2, 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));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isSameAs(referenceViolation);
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   /**
-   * See https://jira.codehaus.org/browse/SONAR-2812
+   * SONAR-2812
    */
   @Test
   public void sameChecksumAndRuleButDifferentLineAndDifferentMessage() {
     Violation newViolation = newViolation("new message", 1, 50, "checksum1");
     RuleFailureModel referenceViolation = newReferenceViolation("old message", 2, 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));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isSameAs(referenceViolation);
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   @Test
@@ -152,9 +146,9 @@ public class ViolationTrackingDecoratorTest {
     Violation newViolation = newViolation("message", 1, 50, "checksum1");
     RuleFailureModel referenceViolation = newReferenceViolation("message", 2, 50, "checksum2");
 
-    Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
-    assertThat(mapping.get(newViolation), is(nullValue()));
-    assertThat(newViolation.isNew(), is(true));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isNull();
+    assertThat(newViolation.isNew()).isTrue();
   }
 
   @Test
@@ -162,9 +156,9 @@ public class ViolationTrackingDecoratorTest {
     Violation newViolation = newViolation("message", 1, 50, "checksum1");
     RuleFailureModel referenceViolation = newReferenceViolation("message", 1, 51, "checksum1");
 
-    Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
-    assertThat(mapping.get(newViolation), is(nullValue()));
-    assertThat(newViolation.isNew(), is(true));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isNull();
+    assertThat(newViolation.isNew()).isTrue();
   }
 
   @Test
@@ -174,20 +168,20 @@ public class ViolationTrackingDecoratorTest {
     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));
-    assertThat(mapping.get(newViolation), equalTo(referenceViolation));
-    assertThat(newViolation.isNew(), is(false));
+    decorator.mapViolations(Lists.newArrayList(newViolation), Lists.newArrayList(referenceViolation));
+    assertThat(decorator.getReferenceViolation(newViolation)).isSameAs(referenceViolation);
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   @Test
   public void shouldSetDateOfNewViolations() {
     Violation newViolation = newViolation("message", 1, 50, "checksum");
-    assertThat(newViolation.getCreatedAt(), nullValue());
+    assertThat(newViolation.getCreatedAt()).isNull();
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Collections.<RuleFailureModel> emptyList());
-    assertThat(mapping.size(), is(0));
-    assertThat(newViolation.getCreatedAt(), is(analysisDate));
-    assertThat(newViolation.isNew(), is(true));
+    assertThat(mapping.size()).isEqualTo(0);
+    assertThat(newViolation.getCreatedAt()).isEqualTo(analysisDate);
+    assertThat(newViolation.isNew()).isTrue();
   }
 
   @Test
@@ -196,12 +190,12 @@ public class ViolationTrackingDecoratorTest {
     RuleFailureModel referenceViolation = newReferenceViolation("", 1, 50, "checksum");
     Date referenceDate = DateUtils.parseDate("2009-05-18");
     referenceViolation.setCreatedAt(referenceDate);
-    assertThat(newViolation.getCreatedAt(), nullValue());
+    assertThat(newViolation.getCreatedAt()).isNull();
 
     Map<Violation, RuleFailureModel> mapping = decorator.mapViolations(Lists.newArrayList(newViolation), Lists.<RuleFailureModel> newArrayList(referenceViolation));
-    assertThat(mapping.size(), is(1));
-    assertThat(newViolation.getCreatedAt(), is(referenceDate));
-    assertThat(newViolation.isNew(), is(false));
+    assertThat(mapping.size()).isEqualTo(1);
+    assertThat(newViolation.getCreatedAt()).isEqualTo(referenceDate);
+    assertThat(newViolation.isNew()).isFalse();
   }
 
   private Violation newViolation(String message, Integer lineId, int ruleId) {