SONAR-5007 Fix registration of technical debt on rules at startup

sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java

@@ -32,6 +32,7 @@ import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rule.RuleStatus;
 import org.sonar.api.server.debt.DebtRemediationFunction;
 import org.sonar.api.server.rule.RulesDefinition;
+import org.sonar.api.utils.MessageException;
 import org.sonar.api.utils.System2;
 import org.sonar.api.utils.TimeProfiler;
 import org.sonar.core.persistence.DbSession;
@@ -42,6 +43,7 @@ import org.sonar.core.technicaldebt.db.CharacteristicDto;
 import org.sonar.server.db.DbClient;
 import org.sonar.server.qualityprofile.RuleActivator;
 
+import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 
 import java.util.*;
@@ -60,16 +62,14 @@ public class RegisterRules implements Startable {
   private final DbClient dbClient;
   private final CharacteristicDao characteristicDao;
 
-
   public RegisterRules(RuleDefinitionsLoader defLoader, RuleActivator ruleActivator,
-                       DbClient dbClient) {
+    DbClient dbClient) {
     this(defLoader, ruleActivator, dbClient, System2.INSTANCE);
   }
 
-
   @VisibleForTesting
   RegisterRules(RuleDefinitionsLoader defLoader, RuleActivator ruleActivator,
-                DbClient dbClient, System2 system) {
+    DbClient dbClient, System2 system) {
     this.defLoader = defLoader;
     this.ruleActivator = ruleActivator;
     this.dbClient = dbClient;
@@ -82,6 +82,7 @@ public class RegisterRules implements Startable {
     DbSession session = dbClient.openSession(false);
     try {
       Map<RuleKey, RuleDto> allRules = loadRules(session);
+      Map<String, CharacteristicDto> allCharacteristics = loadCharacteristics(session);
 
       RulesDefinition.Context context = defLoader.load();
       for (RulesDefinition.ExtendedRepository repoDef : getRepositories(context)) {
@@ -95,11 +96,9 @@ public class RegisterRules implements Startable {
             executeUpdate = true;
           }
 
-          if (rule.getSubCharacteristicId() != null) {
-            CharacteristicDto characteristicDto = characteristicDao.selectById(rule.getSubCharacteristicId(), session);
-            if (characteristicDto != null && mergeDebtDefinitions(ruleDef, rule, characteristicDto)) {
-              executeUpdate = true;
-            }
+          CharacteristicDto subCharacteristic = characteristic(ruleDef, rule.getSubCharacteristicId(), allCharacteristics);
+          if (mergeDebtDefinitions(ruleDef, rule, subCharacteristic)) {
+            executeUpdate = true;
           }
 
           if (mergeTags(ruleDef, rule)) {
@@ -138,6 +137,36 @@ public class RegisterRules implements Startable {
     return rules;
   }
 
+  private Map<String, CharacteristicDto> loadCharacteristics(DbSession session) {
+    Map<String, CharacteristicDto> characteristics = new HashMap<String, CharacteristicDto>();
+    for (CharacteristicDto characteristicDto : characteristicDao.selectEnabledCharacteristics(session)) {
+      characteristics.put(characteristicDto.getKey(), characteristicDto);
+    }
+    return characteristics;
+  }
+
+  @CheckForNull
+  private CharacteristicDto characteristic(RulesDefinition.Rule ruleDef, @Nullable Integer overridingCharacteristicId, Map<String, CharacteristicDto> allCharacteristics) {
+    String subCharacteristic = ruleDef.debtSubCharacteristic();
+    String repo = ruleDef.repository().key();
+    String ruleKey = ruleDef.key();
+
+    // Rule is not linked to a default characteristic or characteristic has been disabled by user
+    if (subCharacteristic == null) {
+      return null;
+    }
+    CharacteristicDto characteristicDto = allCharacteristics.get(subCharacteristic);
+    if (characteristicDto == null) {
+      // Log a warning only if rule has not been overridden by user
+      if (overridingCharacteristicId == null) {
+        LOG.warn(String.format("Characteristic '%s' has not been found on rule '%s:%s'", subCharacteristic, repo, ruleKey));
+      }
+    } else if (characteristicDto.getParentId() == null) {
+      throw MessageException.of(String.format("Rule '%s:%s' cannot be linked on the root characteristic '%s'", repo, ruleKey, subCharacteristic));
+    }
+    return characteristicDto;
+  }
+
   private List<RulesDefinition.ExtendedRepository> getRepositories(RulesDefinition.Context context) {
     List<RulesDefinition.ExtendedRepository> repositories = new ArrayList<RulesDefinition.ExtendedRepository>();
     for (RulesDefinition.Repository repoDef : context.repositories()) {
@@ -205,10 +234,6 @@ public class RegisterRules implements Startable {
       dto.setLanguage(def.repository().language());
       changed = true;
     }
-    if (!StringUtils.equals(dto.getEffortToFixDescription(), def.effortToFixDescription())) {
-      dto.setEffortToFixDescription(def.effortToFixDescription());
-      changed = true;
-    }
     return changed;
   }
 
@@ -228,7 +253,7 @@ public class RegisterRules implements Startable {
   }
 
   private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto, @Nullable Integer characteristicId, @Nullable String remediationFunction,
-                                       @Nullable String remediationCoefficient, @Nullable String remediationOffset, @Nullable String effortToFixDescription) {
+    @Nullable String remediationCoefficient, @Nullable String remediationOffset, @Nullable String effortToFixDescription) {
     boolean changed = false;
 
     if (!ObjectUtils.equals(dto.getDefaultSubCharacteristicId(), characteristicId)) {
@@ -261,8 +286,8 @@ public class RegisterRules implements Startable {
     for (RuleParamDto paramDto : paramDtos) {
       RulesDefinition.Param paramDef = ruleDef.param(paramDto.getName());
       if (paramDef == null) {
-        //TODO cascade on the activeRule upon RuleDeletion
-        //activeRuleDao.removeRuleParam(paramDto, sqlSession);
+        // TODO cascade on the activeRule upon RuleDeletion
+        // activeRuleDao.removeRuleParam(paramDto, sqlSession);
         dbClient.ruleDao().removeRuleParam(session, rule, paramDto);
       } else {
         // TODO validate that existing active rules still match constraints
@@ -325,7 +350,7 @@ public class RegisterRules implements Startable {
       if (ruleDto.getTemplateId() != null) {
         RuleDto template = dbClient.ruleDao().getTemplate(ruleDto, session);
         if (template != null && RuleStatus.REMOVED != template.getStatus()) {
-          if (updateCustomRuleFromTemplateRule(ruleDto, template)){
+          if (updateCustomRuleFromTemplateRule(ruleDto, template)) {
             dbClient.ruleDao().update(session, ruleDto);
           }
           toBeRemoved = false;
@@ -396,12 +421,12 @@ public class RegisterRules implements Startable {
    */
   private void removeActiveRulesOnStillExistingRepositories(DbSession session, Collection<RuleDto> removedRules, RulesDefinition.Context context) {
     List<String> repositoryKeys = newArrayList(Iterables.transform(context.repositories(), new Function<RulesDefinition.Repository, String>() {
-        @Override
-        public String apply(RulesDefinition.Repository input) {
-          return input.key();
-        }
+      @Override
+      public String apply(RulesDefinition.Repository input) {
+        return input.key();
       }
-    ));
+    }
+      ));
 
     for (RuleDto rule : removedRules) {
       // SONAR-4642 Remove active rules only when repository still exists

sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java

@@ -28,8 +28,10 @@ import org.junit.Test;
 import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rule.RuleStatus;
 import org.sonar.api.rule.Severity;
+import org.sonar.api.server.debt.DebtRemediationFunction;
 import org.sonar.api.server.rule.RuleParamType;
 import org.sonar.api.server.rule.RulesDefinition;
+import org.sonar.api.utils.MessageException;
 import org.sonar.core.permission.GlobalPermissions;
 import org.sonar.core.persistence.DbSession;
 import org.sonar.core.rule.RuleDto;
@@ -50,6 +52,7 @@ import java.util.Date;
 import java.util.List;
 
 import static org.fest.assertions.Assertions.assertThat;
+import static org.fest.assertions.Fail.fail;
 
 public class RegisterRulesMediumTest {
 
@@ -67,6 +70,7 @@ public class RegisterRulesMediumTest {
     rulesDefinition.includeX1 = true;
     rulesDefinition.includeX2 = true;
     rulesDefinition.includeTemplate1 = true;
+    rulesDefinition.includeRuleLinkedToRootCharacteristic = false;
     tester.get(Platform.class).executeStartupTasks();
     db = tester.get(DbClient.class);
     dbSession = tester.get(DbClient.class).openSession(false);
@@ -178,6 +182,74 @@ public class RegisterRulesMediumTest {
     assertThat(db.activeRuleDao().findByProfileKey(dbSession, QProfileTesting.XOO_P1_KEY)).hasSize(1);
   }
 
+  @Test
+  public void update_debt_rule() throws Exception {
+    verifyRulesInDb();
+
+    RuleIndex index = tester.get(RuleIndex.class);
+
+    // Update x1 rule
+    RuleDto ruleDto = db.ruleDao().getByKey(dbSession, RuleTesting.XOO_X1);
+    db.ruleDao().update(dbSession, ruleDto
+        .setDefaultSubCharacteristicId(123456)
+        .setDefaultRemediationFunction("LINEAR_OFFSET")
+        .setDefaultRemediationCoefficient("2h")
+        .setDefaultRemediationOffset("35min")
+    );
+    dbSession.commit();
+    dbSession.clearCache();
+
+    // Re-execute startup tasks
+    tester.get(Platform.class).executeStartupTasks();
+
+    // Verify default debt has been reset to plugin definition
+    Rule ruleReloaded = index.getByKey(RuleTesting.XOO_X1);
+    assertThat(ruleReloaded.debtSubCharacteristicKey()).isEqualTo(RulesDefinition.SubCharacteristics.INTEGRATION_TESTABILITY);
+    assertThat(ruleReloaded.debtRemediationFunction().type()).isEqualTo(DebtRemediationFunction.Type.LINEAR_OFFSET);
+    assertThat(ruleReloaded.debtRemediationFunction().coefficient()).isEqualTo("1h");
+    assertThat(ruleReloaded.debtRemediationFunction().offset()).isEqualTo("30min");
+  }
+
+  @Test
+  public void remove_debt_rule() throws Exception {
+    verifyRulesInDb();
+
+    RuleIndex index = tester.get(RuleIndex.class);
+
+    // Set some default debt on x2 rule, which has no debt provided by th plugin
+    RuleDto ruleDto = db.ruleDao().getByKey(dbSession, RuleTesting.XOO_X2);
+    db.ruleDao().update(dbSession, ruleDto
+        .setDefaultSubCharacteristicId(db.debtCharacteristicDao().selectByKey(RulesDefinition.SubCharacteristics.INTEGRATION_TESTABILITY, dbSession).getId())
+        .setDefaultRemediationFunction("LINEAR_OFFSET")
+        .setDefaultRemediationCoefficient("2h")
+        .setDefaultRemediationOffset("35min")
+    );
+    dbSession.commit();
+    dbSession.clearCache();
+
+    // Re-execute startup tasks
+    tester.get(Platform.class).executeStartupTasks();
+
+    // Verify default debt has been removed
+    Rule ruleReloaded = index.getByKey(RuleTesting.XOO_X2);
+    assertThat(ruleReloaded.debtSubCharacteristicKey()).isNull();
+    assertThat(ruleReloaded.debtRemediationFunction()).isNull();
+  }
+
+  @Test
+  public void fail_when_rule_is_linked_on_root_characteristic() throws Exception {
+    verifyRulesInDb();
+
+    rulesDefinition.includeRuleLinkedToRootCharacteristic = true;
+    try {
+      // Re-execute startup tasks
+      tester.get(Platform.class).executeStartupTasks();
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(MessageException.class).hasMessage("Rule 'xoo:RuleLinkedToRootCharacteristic' cannot be linked on the root characteristic 'REUSABILITY'");
+    }
+  }
+
   @Test
   public void update_custom_rule_from_template() throws Exception {
     RuleIndex index = tester.get(RuleIndex.class);
@@ -287,30 +359,45 @@ public class RegisterRulesMediumTest {
 
   public static class XooRulesDefinition implements RulesDefinition {
 
-    boolean includeX1 = true, includeX2 = true, includeTemplate1 = true;
+    boolean includeX1 = true, includeX2 = true, includeTemplate1 = true, includeRuleLinkedToRootCharacteristic = false;
 
     @Override
     public void define(Context context) {
       if (includeX1 || includeX2 || includeTemplate1) {
         NewRepository repository = context.createRepository("xoo", "xoo").setName("Xoo Repo");
         if (includeX1) {
-          repository.createRule("x1")
+          NewRule x1Rule = repository.createRule(RuleTesting.XOO_X1.rule())
             .setName("x1 name")
             .setHtmlDescription("x1 desc")
             .setSeverity(Severity.MINOR)
-            .createParam("acceptWhitespace")
-            .setDefaultValue("false")
+            .setEffortToFixDescription("x1 effort to fix");
+          x1Rule.createParam("acceptWhitespace")
             .setType(RuleParamType.BOOLEAN)
+            .setDefaultValue("false")
             .setDescription("Accept whitespaces on the line");
+          x1Rule
+            .setDebtSubCharacteristic(SubCharacteristics.INTEGRATION_TESTABILITY)
+            .setDebtRemediationFunction(x1Rule.debtRemediationFunctions().linearWithOffset("1h", "30min"));
         }
 
         if (includeX2) {
-          repository.createRule("x2")
+          repository.createRule(RuleTesting.XOO_X2.rule())
             .setName("x2 name")
             .setHtmlDescription("x2 desc")
             .setSeverity(Severity.MAJOR);
         }
 
+        if (includeRuleLinkedToRootCharacteristic) {
+          NewRule x1Rule = repository.createRule("RuleLinkedToRootCharacteristic")
+            .setName("RuleLinkedToRootCharacteristic name")
+            .setHtmlDescription("RuleLinkedToRootCharacteristic desc")
+            .setSeverity(Severity.MINOR);
+          x1Rule
+            // Link to a root characteristic -> fail
+            .setDebtSubCharacteristic("REUSABILITY")
+            .setDebtRemediationFunction(x1Rule.debtRemediationFunctions().linearWithOffset("1h", "30min"));
+        }
+
         if (includeTemplate1) {
           repository.createRule("template1")
             .setName("template1 name")