SONAR-5189 Fix regression when searching measures in cache

4 files changed, 97 insertions, 30 deletions

diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/Cache.java b/sonar-batch/src/main/java/org/sonar/batch/index/Cache.java
index 13e238fce5b..39815dcfcb3 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/index/Cache.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/index/Cache.java
@@ -322,20 +322,6 @@ public class Cache<V extends Serializable> {
   }
 
   /**
-   * Lazy-loading values for a given key
-   */
-  public Iterable<V> allValues(Object key) {
-    try {
-      exchange.clear();
-      exchange.append(key).append(Key.BEFORE);
-      Exchange iteratorExchange = new Exchange(exchange);
-      return new ValueIterable<V>(iteratorExchange, true);
-    } catch (Exception e) {
-      throw new IllegalStateException("Fail to get values from cache " + name, e);
-    }
-  }
-
-  /**
    * Lazy-loading values
    */
   public Iterable<V> values() {
diff --git a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java
index cea7aa71729..5a9c3148b23 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/index/DefaultIndex.java
@@ -184,12 +184,7 @@ public class DefaultIndex extends SonarIndex {
   public <M> M getMeasures(Resource resource, MeasuresFilter<M> filter) {
     // Reload resource so that effective key is populated
     Resource indexedResource = getResource(resource);
-    Iterable<Measure> unfiltered;
-    if (filter instanceof MeasuresFilters.MetricFilter) {
-      unfiltered = measureCache.byMetric(indexedResource, ((MeasuresFilters.MetricFilter) filter).filterOnMetricKey());
-    } else {
-      unfiltered = measureCache.byResource(indexedResource);
-    }
+    Iterable<Measure> unfiltered = measureCache.byResource(indexedResource);
     return filter.filter(unfiltered);
   }
 
diff --git a/sonar-batch/src/main/java/org/sonar/batch/scan/measure/MeasureCache.java b/sonar-batch/src/main/java/org/sonar/batch/scan/measure/MeasureCache.java
index 1f668614d8f..5068a4734be 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/scan/measure/MeasureCache.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/scan/measure/MeasureCache.java
@@ -22,6 +22,7 @@ package org.sonar.batch.scan.measure;
 import com.google.common.base.Preconditions;
 import org.sonar.api.BatchComponent;
 import org.sonar.api.measures.Measure;
+import org.sonar.api.measures.RuleMeasure;
 import org.sonar.api.resources.Resource;
 import org.sonar.batch.index.Cache;
 import org.sonar.batch.index.Cache.Entry;
@@ -43,19 +44,33 @@ public class MeasureCache implements BatchComponent {
   }
 
   public Iterable<Measure> byResource(Resource r) {
-    return cache.allValues(r.getEffectiveKey());
+    return cache.values(r.getEffectiveKey());
   }
 
   public MeasureCache put(Resource resource, Measure measure) {
     Preconditions.checkNotNull(resource.getEffectiveKey());
     Preconditions.checkNotNull(measure.getMetricKey());
-    cache.put(resource.getEffectiveKey(), measure.getMetricKey(), measure.hashCode(), measure);
+    cache.put(resource.getEffectiveKey(), computeMeasureKey(measure), measure);
     return this;
   }
 
-  public Iterable<Measure> byMetric(Resource resource, String metricKey) {
-    Preconditions.checkNotNull(resource.getEffectiveKey());
-    Preconditions.checkNotNull(metricKey);
-    return cache.values(resource.getEffectiveKey(), metricKey);
+  private static String computeMeasureKey(Measure m) {
+    StringBuilder sb = new StringBuilder();
+    if (m.getMetricKey() != null) {
+      sb.append(m.getMetricKey());
+    }
+    sb.append("|");
+    if (m.getCharacteristic() != null) {
+      sb.append(m.getCharacteristic().key());
+    }
+    sb.append("|");
+    if (m.getPersonId() != null) {
+      sb.append(m.getPersonId());
+    }
+    if (m instanceof RuleMeasure) {
+      sb.append("|");
+      sb.append(((RuleMeasure) m).ruleKey());
+    }
+    return sb.toString();
   }
 }
diff --git a/sonar-batch/src/test/java/org/sonar/batch/scan/measure/MeasureCacheTest.java b/sonar-batch/src/test/java/org/sonar/batch/scan/measure/MeasureCacheTest.java
index a1791ff676d..0b1db5e1d75 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/scan/measure/MeasureCacheTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/scan/measure/MeasureCacheTest.java
@@ -27,11 +27,19 @@ import org.junit.rules.TemporaryFolder;
 import org.sonar.api.measures.CoreMetrics;
 import org.sonar.api.measures.Measure;
 import org.sonar.api.measures.RuleMeasure;
+import org.sonar.api.resources.Directory;
+import org.sonar.api.resources.File;
 import org.sonar.api.resources.Project;
+import org.sonar.api.resources.Resource;
+import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rules.RulePriority;
+import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic;
+import org.sonar.batch.index.Cache.Entry;
 import org.sonar.batch.index.Caches;
 import org.sonar.batch.index.CachesTest;
 
+import java.util.Iterator;
+
 import static org.fest.assertions.Assertions.assertThat;
 
 public class MeasureCacheTest {
@@ -60,15 +68,17 @@ public class MeasureCacheTest {
     assertThat(cache.entries()).hasSize(0);
 
     assertThat(cache.byResource(p)).hasSize(0);
-    assertThat(cache.byMetric(p, "ncloc")).hasSize(0);
 
     Measure m = new Measure(CoreMetrics.NCLOC, 1.0);
     cache.put(p, m);
 
     assertThat(cache.entries()).hasSize(1);
+    Iterator<Entry<Measure>> iterator = cache.entries().iterator();
+    iterator.hasNext();
+    Entry<Measure> next = iterator.next();
+    assertThat(next.value()).isEqualTo(m);
+    assertThat(next.key()[0]).isEqualTo("struts");
 
-    assertThat(cache.byMetric(p, "ncloc")).hasSize(1);
-    assertThat(cache.byMetric(p, "ncloc").iterator().next()).isEqualTo(m);
     assertThat(cache.byResource(p)).hasSize(1);
     assertThat(cache.byResource(p).iterator().next()).isEqualTo(m);
 
@@ -78,6 +88,67 @@ public class MeasureCacheTest {
     assertThat(cache.entries()).hasSize(2);
 
     assertThat(cache.byResource(p)).hasSize(2);
-    assertThat(cache.byMetric(p, "ncloc")).hasSize(1);
+  }
+
+  @Test
+  public void should_add_measure_with_same_metric() throws Exception {
+    MeasureCache cache = new MeasureCache(caches);
+    Project p = new Project("struts");
+
+    assertThat(cache.entries()).hasSize(0);
+    assertThat(cache.byResource(p)).hasSize(0);
+
+    Measure m1 = new Measure(CoreMetrics.NCLOC, 1.0);
+    Measure m2 = new Measure(CoreMetrics.NCLOC, 1.0).setCharacteristic(new DefaultCharacteristic().setKey("charac"));
+    Measure m3 = new Measure(CoreMetrics.NCLOC, 1.0).setPersonId(2);
+    Measure m4 = new RuleMeasure(CoreMetrics.NCLOC, RuleKey.of("repo", "rule"), RulePriority.BLOCKER, null);
+    cache.put(p, m1);
+    cache.put(p, m2);
+    cache.put(p, m3);
+    cache.put(p, m4);
+
+    assertThat(cache.entries()).hasSize(4);
+
+    assertThat(cache.byResource(p)).hasSize(4);
+  }
+
+  @Test
+  public void should_get_measures() throws Exception {
+    MeasureCache cache = new MeasureCache(caches);
+    Project p = new Project("struts");
+    Resource dir = new Directory("foo/bar").setEffectiveKey("struts:foo/bar");
+    Resource file1 = new File("foo/bar/File1.txt").setEffectiveKey("struts:foo/bar/File1.txt");
+    Resource file2 = new File("foo/bar/File2.txt").setEffectiveKey("struts:foo/bar/File2.txt");
+
+    assertThat(cache.entries()).hasSize(0);
+
+    assertThat(cache.byResource(p)).hasSize(0);
+    assertThat(cache.byResource(dir)).hasSize(0);
+
+    Measure mFile1 = new Measure(CoreMetrics.NCLOC, 1.0);
+    cache.put(file1, mFile1);
+    Measure mFile2 = new Measure(CoreMetrics.NCLOC, 3.0);
+    cache.put(file2, mFile2);
+
+    assertThat(cache.entries()).hasSize(2);
+    assertThat(cache.byResource(p)).hasSize(0);
+    assertThat(cache.byResource(dir)).hasSize(0);
+
+    Measure mDir = new Measure(CoreMetrics.NCLOC, 4.0);
+    cache.put(dir, mDir);
+
+    assertThat(cache.entries()).hasSize(3);
+    assertThat(cache.byResource(p)).hasSize(0);
+    assertThat(cache.byResource(dir)).hasSize(1);
+    assertThat(cache.byResource(dir).iterator().next()).isEqualTo(mDir);
+
+    Measure mProj = new Measure(CoreMetrics.NCLOC, 4.0);
+    cache.put(p, mProj);
+
+    assertThat(cache.entries()).hasSize(4);
+    assertThat(cache.byResource(p)).hasSize(1);
+    assertThat(cache.byResource(p).iterator().next()).isEqualTo(mProj);
+    assertThat(cache.byResource(dir)).hasSize(1);
+    assertThat(cache.byResource(dir).iterator().next()).isEqualTo(mDir);
   }
 }