SONAR-5056 Fix performance issue when converting new rule to old rule

diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java
index d9a9c293b169a1debd99f5bc42d3aa017cff6db3..0827cb4cb0c2be9944cab006771cad1cc2753e2a 100644 (file)
--- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java
+++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java
@@ -29,7 +29,6 @@ import org.sonar.api.PropertyType;
 import org.sonar.api.batch.*;
 import org.sonar.api.batch.rule.Rule;
 import org.sonar.api.batch.rule.Rules;
-import org.sonar.api.batch.rule.internal.DefaultRule;
 import org.sonar.api.component.ResourcePerspectives;
 import org.sonar.api.config.PropertyDefinition;
 import org.sonar.api.issue.Issuable;
@@ -40,7 +39,7 @@ import org.sonar.api.resources.Project;
 import org.sonar.api.resources.Resource;
 import org.sonar.api.resources.ResourceUtils;
 import org.sonar.api.rule.RuleKey;
-import org.sonar.api.rules.RulePriority;
+import org.sonar.api.rules.RuleFinder;
 import org.sonar.api.technicaldebt.batch.Characteristic;
 import org.sonar.api.technicaldebt.batch.TechnicalDebtModel;
 
@@ -64,10 +63,16 @@ public final class TechnicalDebtDecorator implements Decorator {
   private final TechnicalDebtModel model;
   private final Rules rules;
 
-  public TechnicalDebtDecorator(ResourcePerspectives perspectives, TechnicalDebtModel model, Rules rules) {
+  /**
+   * ruleFinder is needed to load "old" rule in order to persist rule measure
+   */
+  private final RuleFinder ruleFinder;
+
+  public TechnicalDebtDecorator(ResourcePerspectives perspectives, TechnicalDebtModel model, Rules rules, RuleFinder ruleFinder) {
     this.perspectives = perspectives;
     this.model = model;
     this.rules = rules;
+    this.ruleFinder = ruleFinder;
   }
 
   public boolean shouldExecuteOnProject(Project project) {
@@ -88,19 +93,18 @@ public final class TechnicalDebtDecorator implements Decorator {
   }
 
   private void saveMeasures(DecoratorContext context, List<Issue> issues) {
-    // group issues by rules
-    ListMultimap<Rule, Issue> issuesByRule = issuesByRule(issues);
+    // group issues by rule keys
+    ListMultimap<RuleKey, Issue> issuesByRule = issuesByRule(issues);
 
     double total = 0.0;
     Map<Characteristic, Double> characteristicCosts = newHashMap();
-    Map<Rule, Double> ruleDebtCosts = newHashMap();
+    Map<org.sonar.api.rules.Rule, Double> ruleDebtCosts = newHashMap();
 
-    for (Rule rule : rules.findAll()) {
-      String characteristicKey = rule.characteristic();
+    for (Rule newRule : rules.findWithDebt()) {
+      String characteristicKey = newRule.characteristic();
       if (characteristicKey != null) {
-        List<Issue> requirementIssues = issuesByRule.get(rule);
-        double value = computeTechnicalDebt(CoreMetrics.TECHNICAL_DEBT, context, rule, requirementIssues);
-
+        org.sonar.api.rules.Rule rule = ruleFinder.findByKey(newRule.key());
+        double value = computeTechnicalDebt(CoreMetrics.TECHNICAL_DEBT, context, rule, issuesByRule.get(newRule.key()));
         ruleDebtCosts.put(rule, value);
         total += value;
         Characteristic characteristic = model.characteristicByKey(characteristicKey);
@@ -110,7 +114,7 @@ public final class TechnicalDebtDecorator implements Decorator {
 
     context.saveMeasure(CoreMetrics.TECHNICAL_DEBT, total);
     saveOnCharacteristic(context, characteristicCosts);
-    saveOnRequirement(context, ruleDebtCosts);
+    saveOnRule(context, ruleDebtCosts);
   }
 
   private void saveOnCharacteristic(DecoratorContext context, Map<Characteristic, Double> characteristicCosts) {
@@ -119,8 +123,8 @@ public final class TechnicalDebtDecorator implements Decorator {
     }
   }
 
-  private void saveOnRequirement(DecoratorContext context, Map<Rule, Double> requirementCosts) {
-    for (Map.Entry<Rule, Double> entry : requirementCosts.entrySet()) {
+  private void saveOnRule(DecoratorContext context, Map<org.sonar.api.rules.Rule, Double> requirementCosts) {
+    for (Map.Entry<org.sonar.api.rules.Rule, Double> entry : requirementCosts.entrySet()) {
       saveTechnicalDebt(context, entry.getKey(), entry.getValue(), ResourceUtils.isEntity(context.getResource()));
     }
   }
@@ -137,12 +141,11 @@ public final class TechnicalDebtDecorator implements Decorator {
   }
 
   @VisibleForTesting
-  void saveTechnicalDebt(DecoratorContext context, Rule rule, Double value, boolean inMemory) {
+  void saveTechnicalDebt(DecoratorContext context, org.sonar.api.rules.Rule rule, Double value, boolean inMemory) {
     // we need the value on projects (root or module) even if value==0 in order to display correctly the SQALE history chart (see SQALE-122)
     // BUT we don't want to save zero-values for non top-characteristics (see SQALE-147)
     if (value > 0.0) {
-      org.sonar.api.rules.Rule oldRule = toOldRule(rule);
-      RuleMeasure measure = new RuleMeasure(CoreMetrics.TECHNICAL_DEBT, oldRule, null, null);
+      RuleMeasure measure = new RuleMeasure(CoreMetrics.TECHNICAL_DEBT, rule, null, null);
       saveMeasure(context, measure, value, inMemory);
     }
   }
@@ -156,17 +159,15 @@ public final class TechnicalDebtDecorator implements Decorator {
   }
 
   @VisibleForTesting
-  ListMultimap<Rule, Issue> issuesByRule(List<Issue> issues) {
-    ListMultimap<Rule, Issue> result = ArrayListMultimap.create();
+  ListMultimap<RuleKey, Issue> issuesByRule(List<Issue> issues) {
+    ListMultimap<RuleKey, Issue> result = ArrayListMultimap.create();
     for (Issue issue : issues) {
-      RuleKey key = issue.ruleKey();
-      Rule rule = rules.find(key);
-      result.put(rule, issue);
+      result.put(issue.ruleKey(), issue);
     }
     return result;
   }
 
-  private double computeTechnicalDebt(Metric metric, DecoratorContext context, Rule rule, Collection<Issue> issues) {
+  private double computeTechnicalDebt(Metric metric, DecoratorContext context, org.sonar.api.rules.Rule rule, Collection<Issue> issues) {
     long debt = 0L;
     if (issues != null) {
       for (Issue issue : issues) {
@@ -177,11 +178,10 @@ public final class TechnicalDebtDecorator implements Decorator {
       }
     }
 
-    org.sonar.api.rules.Rule oldRule = toOldRule(rule);
-    for (Measure measure : context.getChildrenMeasures(MeasuresFilters.rule(metric, oldRule))) {
+    for (Measure measure : context.getChildrenMeasures(MeasuresFilters.rule(metric, rule))) {
       // Comparison on rule is only used for unit test, otherwise no need to do this check
       RuleMeasure ruleMeasure = (RuleMeasure) measure;
-      if (measure != null && ruleMeasure.getRule().equals(oldRule) && measure.getValue() != null) {
+      if (measure != null && ruleMeasure.getRule().equals(rule) && measure.getValue() != null) {
         debt += measure.getValue();
       }
     }
@@ -216,12 +216,4 @@ public final class TechnicalDebtDecorator implements Decorator {
     );
   }
 
-  private org.sonar.api.rules.Rule toOldRule(Rule rule) {
-    DefaultRule defaultRule = (DefaultRule) rule;
-    org.sonar.api.rules.Rule oldRule = org.sonar.api.rules.Rule.create(rule.key().repository(), rule.key().rule());
-    oldRule.setSeverity(RulePriority.valueOf(rule.severity()));
-    oldRule.setId(defaultRule.id());
-    return oldRule;
-  }
-
 }

diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java
index c62ca8e68438ef40d7a2fcd956b07536b6bc153e..e24fb32d492c37a5bf6631677b6c19c9b3370bb5 100644 (file)
--- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java
+++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java
@@ -32,7 +32,6 @@ import org.mockito.ArgumentMatcher;
 import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
 import org.sonar.api.batch.DecoratorContext;
-import org.sonar.api.batch.rule.Rule;
 import org.sonar.api.batch.rule.Rules;
 import org.sonar.api.batch.rule.internal.RulesBuilder;
 import org.sonar.api.component.ResourcePerspectives;
@@ -44,6 +43,7 @@ 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.RuleFinder;
 import org.sonar.api.technicaldebt.batch.Characteristic;
 import org.sonar.api.technicaldebt.batch.TechnicalDebtModel;
 import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic;
@@ -79,6 +79,9 @@ public class TechnicalDebtDecoratorTest {
   @Mock
   ResourcePerspectives perspectives;
 
+  @Mock
+  RuleFinder ruleFinder;
+
   RuleKey ruleKey1 = RuleKey.of("repo1", "rule1");
   RuleKey ruleKey2 = RuleKey.of("repo2", "rule2");
   Rules rules;
@@ -93,7 +96,10 @@ public class TechnicalDebtDecoratorTest {
     rulesBuilder.add(ruleKey2).setName("rule2").setCharacteristic("MODULARITY");
     rules = rulesBuilder.build();
 
-    decorator = new TechnicalDebtDecorator(perspectives, defaultTechnicalDebtModel, rules);
+    when(ruleFinder.findByKey(ruleKey1)).thenReturn(org.sonar.api.rules.Rule.create(ruleKey1.repository(), ruleKey1.rule()));
+    when(ruleFinder.findByKey(ruleKey2)).thenReturn(org.sonar.api.rules.Rule.create(ruleKey2.repository(), ruleKey2.rule()));
+
+    decorator = new TechnicalDebtDecorator(perspectives, defaultTechnicalDebtModel, rules, ruleFinder);
   }
 
   @Test
@@ -123,11 +129,11 @@ public class TechnicalDebtDecoratorTest {
 
     List<Issue> issues = newArrayList(issue1, issue2, issue3);
 
-    ListMultimap<Rule, Issue> result = decorator.issuesByRule(issues);
+    ListMultimap<RuleKey, Issue> result = decorator.issuesByRule(issues);
 
     assertThat(result.keySet().size()).isEqualTo(2);
-    assertThat(result.get(rules.find(ruleKey1))).containsExactly(issue1, issue2);
-    assertThat(result.get(rules.find(ruleKey2))).containsExactly(issue3);
+    assertThat(result.get(ruleKey1)).containsExactly(issue1, issue2);
+    assertThat(result.get(ruleKey2)).containsExactly(issue3);
   }
 
   @Test

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java
index 14fb6cb7ac341939ab9a7d983dd49080c34c14a5..2a2ff644f327f26dc1bde4abfcacc3b0b8d537f5 100644 (file)
--- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java
@@ -39,4 +39,6 @@ public interface Rules {
 
   Collection<Rule> findByRepository(String repository);
 
+  Collection<Rule> findWithDebt();
+
 }

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java
index 3c14387a31c53b9350b0cb75d08d8d794932ab87..20de45fd89faa8fabda94f57e69e9451421951e5 100644 (file)
--- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java
@@ -31,12 +31,16 @@ import javax.annotation.concurrent.Immutable;
 import java.util.Collection;
 import java.util.List;
 
+import static com.google.common.collect.Lists.newArrayList;
+
 @Immutable
 class DefaultRules implements Rules {
 
   // TODO use disk-backed cache (persistit) instead of full in-memory cache ?
   private final ListMultimap<String, Rule> rulesByRepository;
 
+  private final Collection<Rule> rulesWithDebt;
+
   DefaultRules(Collection<NewRule> newRules) {
     ImmutableListMultimap.Builder<String, Rule> builder = ImmutableListMultimap.builder();
     for (NewRule newRule : newRules) {
@@ -44,6 +48,13 @@ class DefaultRules implements Rules {
       builder.put(r.key().repository(), r);
     }
     rulesByRepository = builder.build();
+
+    rulesWithDebt = newArrayList();
+    for (Rule rule : rulesByRepository.values()) {
+      if (rule.characteristic() != null) {
+        rulesWithDebt.add(rule);
+      }
+    }
   }
 
   @Override
@@ -66,4 +77,9 @@ class DefaultRules implements Rules {
   public Collection<Rule> findByRepository(String repository) {
     return rulesByRepository.get(repository);
   }
+
+  @Override
+  public Collection<Rule> findWithDebt() {
+    return rulesWithDebt;
+  }
 }