SONAR-5056 Fix issue when removing characteristic and rules linked with default characteristic linked on it was updated

11 files changed, 113 insertions, 88 deletions

diff --git a/sonar-batch/src/main/java/org/sonar/batch/rule/RulesProvider.java b/sonar-batch/src/main/java/org/sonar/batch/rule/RulesProvider.java
index 75afe603f4f..701a5b9f81e 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/rule/RulesProvider.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/rule/RulesProvider.java
@@ -81,7 +81,7 @@ public class RulesProvider extends ProviderAdapter {
 
       if (ruleDto.hasCharacteristic()) {
         newRule.setDebtCharacteristic(effectiveCharacteristic(ruleDto, ruleKey, debtModel).key());
-        newRule.setDebtRemediationFunction(effectiveFunction(ruleDto, ruleKey, newRule, durations));
+        newRule.setDebtRemediationFunction(effectiveFunction(ruleDto, ruleKey, durations));
       }
 
       for (RuleParamDto ruleParamDto : paramDtosByRuleId.get(ruleDto.getId())) {
@@ -103,7 +103,7 @@ public class RulesProvider extends ProviderAdapter {
     return characteristic;
   }
 
-  private DebtRemediationFunction effectiveFunction(RuleDto ruleDto, RuleKey ruleKey, NewRule newRule, Durations durations) {
+  private DebtRemediationFunction effectiveFunction(RuleDto ruleDto, RuleKey ruleKey, Durations durations) {
     String function = ruleDto.getRemediationFunction();
     String defaultFunction = ruleDto.getDefaultRemediationFunction();
     if (function != null) {
diff --git a/sonar-batch/src/test/java/org/sonar/batch/rule/RulesProviderTest.java b/sonar-batch/src/test/java/org/sonar/batch/rule/RulesProviderTest.java
index d87e6b3189b..08106710bfe 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/rule/RulesProviderTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/rule/RulesProviderTest.java
@@ -174,6 +174,17 @@ public class RulesProviderTest extends AbstractDaoTestCase {
   }
 
   @Test
+  public void build_rules_with_default_characteristic_and_disable_characteristic() throws Exception {
+    setupData("build_rules_with_default_characteristic_and_disable_characteristic");
+
+    Rules rules = provider.provide(ruleDao, debtModel, durations);
+
+    Rule rule = rules.find(RuleKey.of("checkstyle", "AvoidNull"));
+    assertThat(rule.debtCharacteristic()).isNull();
+    assertThat(rule.debtRemediationFunction()).isNull();
+  }
+
+  @Test
   public void fail_if_characteristic_not_found() throws Exception {
     setupData("fail_if_characteristic_not_found");
 
diff --git a/sonar-batch/src/test/resources/org/sonar/batch/rule/RulesProviderTest/build_rules_with_default_characteristic_and_disable_characteristic.xml b/sonar-batch/src/test/resources/org/sonar/batch/rule/RulesProviderTest/build_rules_with_default_characteristic_and_disable_characteristic.xml
new file mode 100644
index 00000000000..695d1dfcb4e
--- /dev/null
+++ b/sonar-batch/src/test/resources/org/sonar/batch/rule/RulesProviderTest/build_rules_with_default_characteristic_and_disable_characteristic.xml
@@ -0,0 +1,9 @@
+<dataset>
+
+  <rules id="1" plugin_rule_key="AvoidNull" plugin_name="checkstyle" name="Avoid Null" description="Should avoid NULL" status="READY" priority="1"
+         characteristic_id="-1" default_characteristic_id="103"
+         remediation_function="[null]" default_remediation_function="LINEAR"
+         remediation_factor="[null]" default_remediation_factor="2h"
+         remediation_offset="[null]" default_remediation_offset="[null]"/>
+
+</dataset>
diff --git a/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java b/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java
index 0324dac5cb0..74878a71ff6 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/RuleDao.java
@@ -63,17 +63,17 @@ public class RuleDao implements BatchComponent, ServerComponent {
     return getMapper(session).selectNonManual();
   }
 
-  public List<RuleDto> selectByCharacteristicOrSubCharacteristicId(Integer characteristicOrSubCharacteristicId) {
+  public List<RuleDto> selectBySubCharacteristicId(Integer characteristicOrSubCharacteristicId) {
     SqlSession session = mybatis.openSession();
     try {
-      return selectByCharacteristicOrSubCharacteristicId(characteristicOrSubCharacteristicId, session);
+      return selectBySubCharacteristicId(characteristicOrSubCharacteristicId, session);
     } finally {
       MyBatis.closeQuietly(session);
     }
   }
 
-  public List<RuleDto> selectByCharacteristicOrSubCharacteristicId(Integer characteristicOrSubCharacteristicId, SqlSession session) {
-    return getMapper(session).selectByCharacteristicOrSubCharacteristicId(characteristicOrSubCharacteristicId);
+  public List<RuleDto> selectBySubCharacteristicId(Integer characteristicOrSubCharacteristicId, SqlSession session) {
+    return getMapper(session).selectBySubCharacteristicId(characteristicOrSubCharacteristicId);
   }
 
   public List<RuleDto> selectOverridingDebt(List<String> repositories) {
diff --git a/sonar-core/src/main/java/org/sonar/core/rule/RuleDto.java b/sonar-core/src/main/java/org/sonar/core/rule/RuleDto.java
index 86ab08a3dcd..e53232b57f9 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/RuleDto.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/RuleDto.java
@@ -315,7 +315,7 @@ public final class RuleDto {
   }
 
   public boolean hasCharacteristic(){
-    return (characteristicId != null && !RuleDto.DISABLED_CHARACTERISTIC_ID.equals(characteristicId)) || defaultCharacteristicId != null;
+    return ((characteristicId != null && !RuleDto.DISABLED_CHARACTERISTIC_ID.equals(characteristicId))) || (characteristicId == null && defaultCharacteristicId != null);
   }
 
   @Override
diff --git a/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java b/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java
index 9b49070b0d0..5976f9c2dd2 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/RuleMapper.java
@@ -30,7 +30,7 @@ public interface RuleMapper {
 
   List<RuleDto> selectNonManual();
 
-  List<RuleDto> selectByCharacteristicOrSubCharacteristicId(int id);
+  List<RuleDto> selectBySubCharacteristicId(int characteristicId);
 
   List<RuleDto> selectOverridingDebt(@Param("repositories") List<String> repositories);
 
diff --git a/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml b/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml
index 7f61cd090d4..16d37d6a426 100644
--- a/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml
+++ b/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml
@@ -4,75 +4,74 @@
 <mapper namespace="org.sonar.core.rule.RuleMapper">
 
   <sql id="selectColumns">
-    id,
-    plugin_rule_key as "ruleKey",
-    plugin_name as "repositoryKey",
-    description,
-    status,
-    name,
-    plugin_config_key as "configKey",
-    priority as "severity",
-    cardinality,
-    language as "language",
-    parent_id as "parentId",
-    note_data as "noteData",
-    note_user_login as "noteUserLogin",
-    note_created_at as "noteCreatedAt",
-    note_updated_at as "noteUpdatedAt",
-    characteristic_id as "characteristicId",
-    default_characteristic_id as "defaultCharacteristicId",
-    remediation_function as "remediationFunction",
-    default_remediation_function as "defaultRemediationFunction",
-    remediation_factor as "remediationFactor",
-    default_remediation_factor as "defaultRemediationFactor",
-    remediation_offset as "remediationOffset",
-    default_remediation_offset as "defaultRemediationOffset",
-    effort_to_fix_description as "effortToFixDescription",
-    created_at as "createdAt",
-    updated_at as "updatedAt"
+    r.id,
+    r.plugin_rule_key as "ruleKey",
+    r.plugin_name as "repositoryKey",
+    r.description,
+    r.status,
+    r.name,
+    r.plugin_config_key as "configKey",
+    r.priority as "severity",
+    r.cardinality,
+    r.language as "language",
+    r.parent_id as "parentId",
+    r.note_data as "noteData",
+    r.note_user_login as "noteUserLogin",
+    r.note_created_at as "noteCreatedAt",
+    r.note_updated_at as "noteUpdatedAt",
+    r.characteristic_id as "characteristicId",
+    r.default_characteristic_id as "defaultCharacteristicId",
+    r.remediation_function as "remediationFunction",
+    r.default_remediation_function as "defaultRemediationFunction",
+    r.remediation_factor as "remediationFactor",
+    r.default_remediation_factor as "defaultRemediationFactor",
+    r.remediation_offset as "remediationOffset",
+    r.default_remediation_offset as "defaultRemediationOffset",
+    r.effort_to_fix_description as "effortToFixDescription",
+    r.created_at as "createdAt",
+    r.updated_at as "updatedAt"
   </sql>
 
   <select id="selectAll" resultType="Rule">
-    select <include refid="selectColumns"/> from rules
+    select <include refid="selectColumns"/> from rules r
   </select>
 
   <select id="selectEnablesAndNonManual" resultType="Rule">
-    select <include refid="selectColumns"/> from rules
+    select <include refid="selectColumns"/> from rules r
     <where>
-      and status != 'REMOVED'
-      and plugin_name != 'manual'
+      and r.status != 'REMOVED'
+      and r.plugin_name != 'manual'
     </where>
   </select>
 
   <select id="selectById" parameterType="Integer" resultType="Rule">
-    select <include refid="selectColumns"/> from rules WHERE id=#{id}
+    select <include refid="selectColumns"/> from rules r WHERE r.id=#{id}
   </select>
 
   <select id="selectByName" parameterType="String" resultType="Rule">
-    select <include refid="selectColumns"/> from rules WHERE name=#{name}
+    select <include refid="selectColumns"/> from rules r WHERE r.name=#{name}
   </select>
 
   <select id="selectNonManual" resultType="Rule">
-    select <include refid="selectColumns"/> from rules
-    where plugin_name != 'manual'
+    select <include refid="selectColumns"/> from rules r
+    where r.plugin_name != 'manual'
   </select>
 
-  <select id="selectByCharacteristicOrSubCharacteristicId" resultType="Rule">
-    select <include refid="selectColumns"/> from rules
+  <select id="selectBySubCharacteristicId" resultType="Rule">
+    select <include refid="selectColumns"/> from rules r
     <where>
-      and characteristic_id in (select c.id from characteristics c where (c.id=#{id} or c.parent_id=#{id}) and c.enabled=${_true})
-      and status!='REMOVED'
+      and (r.characteristic_id=#{characteristicId} or r.default_characteristic_id=#{characteristicId})
     </where>
   </select>
 
   <select id="selectOverridingDebt" resultType="Rule">
-    SELECT <include refid="selectColumns"/> FROM rules
+    SELECT <include refid="selectColumns"/> FROM rules r
     <where>
-      AND (characteristic_id is NOT NULL or remediation_function IS NOT NULL)
+      AND (r.characteristic_id is NOT NULL or r.remediation_function IS NOT NULL)
       <if test="repositories.size()>0">
         AND
         <foreach item="repo" index="index" collection="repositories" open="(" separator=" or " close=")">
-          plugin_name=#{repo}
+          r.plugin_name=#{repo}
         </foreach>
       </if>
     </where>
diff --git a/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java b/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java
index 1c638ff976c..48fc65093c0 100644
--- a/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/rule/RuleDaoTest.java
@@ -137,22 +137,21 @@ public class RuleDaoTest extends AbstractDaoTestCase {
   }
 
   @Test
-  public void select_by_characteristic_or_sub_characteristic_id(){
-    setupData("select_by_characteristic_or_sub_characteristic_id");
+  public void select_by_sub_characteristic_id(){
+    setupData("select_by_sub_characteristic_id");
 
     // Rules from sub characteristic
-    List<RuleDto> ruleDtos = dao.selectByCharacteristicOrSubCharacteristicId(2);
-    assertThat(ruleDtos).hasSize(1);
-    assertThat(ruleDtos.get(0).getId()).isEqualTo(1);
+    List<RuleDto> ruleDtos = dao.selectBySubCharacteristicId(3);
+    assertThat(ruleDtos).hasSize(3);
+    assertThat(idsFromRuleDtos(ruleDtos)).containsExactly(2, 4, 5);
 
-    // Rules from characteristic
-    ruleDtos = dao.selectByCharacteristicOrSubCharacteristicId(1);
-    assertThat(ruleDtos).hasSize(2);
-    assertThat(idsFromRuleDtos(ruleDtos)).containsExactly(1, 2);
+    // Nothing on root characteristic
+    ruleDtos = dao.selectBySubCharacteristicId(1);
+    assertThat(ruleDtos).isEmpty();
 
     // Rules from disabled characteristic
-    ruleDtos = dao.selectByCharacteristicOrSubCharacteristicId(11);
-    assertThat(ruleDtos).isEmpty();
+    ruleDtos = dao.selectBySubCharacteristicId(11);
+    assertThat(idsFromRuleDtos(ruleDtos)).containsExactly(3);
   }
 
   @Test
diff --git a/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_characteristic_or_sub_characteristic_id.xml b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_sub_characteristic_id.xml
index db7d5b3a6c5..b712b6ec0b1 100644
--- a/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_characteristic_or_sub_characteristic_id.xml
+++ b/sonar-core/src/test/resources/org/sonar/core/rule/RuleDaoTest/select_by_sub_characteristic_id.xml
@@ -44,14 +44,14 @@
          remediation_factor="5d" default_remediation_factor="5d"
          remediation_offset="[null]" default_remediation_offset="10h" updated_at="2014-02-19"/>
 
-  <!-- Removed rule linked to one enable sub characteristic -> should never be returned -->
+  <!-- Removed rule linked to one enable sub characteristic -->
   <rules id="4" plugin_rule_key="ObjectFinalizeOverridenCallsSuperFinalizeCheck" plugin_name="squid" name="ObjectFinalizeOverridenCallsSuperFinalizeCheck" description="super.finalize() should be called at the end of Object.finalize() implementations" status="REMOVED"
          characteristic_id="3" default_characteristic_id="50"
          remediation_function="LINEAR" default_remediation_function="LINEAR_OFFSET"
          remediation_factor="5d" default_remediation_factor="5min"
          remediation_offset="[null]" default_remediation_offset="10h" updated_at="2014-02-19"/>
 
-  <!-- Rule linked to a sub characteristic, but only default characteristic is linked -> should never be returned -->
+  <!-- Rule linked to a sub characteristic, but only default characteristic is linked  -->
   <rules id="5" plugin_rule_key="RightCurlyBraceStartLineCheck" plugin_name="squid" name="RightCurlyBraceStartLineCheck" description="Right curly braces should be located at the beginning of lines of code" status="READY"
          characteristic_id="[null]" default_characteristic_id="3"
          remediation_function="[null]" default_remediation_function="LINEAR"
diff --git a/sonar-server/src/main/java/org/sonar/server/debt/DebtModelOperations.java b/sonar-server/src/main/java/org/sonar/server/debt/DebtModelOperations.java
index 4b7c6004746..cbd9c4bcd63 100644
--- a/sonar-server/src/main/java/org/sonar/server/debt/DebtModelOperations.java
+++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtModelOperations.java
@@ -39,6 +39,7 @@ import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.user.UserSession;
 import org.sonar.server.util.Validation;
 
+import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 
 import java.util.Date;
@@ -130,23 +131,13 @@ public class DebtModelOperations implements ServerComponent {
     try {
       final CharacteristicDto dto = findCharacteristic(characteristicId, session);
       int currentOrder = dto.getOrder();
-
-      // characteristics should be order by 'order'
-      List<CharacteristicDto> rootCharacteristics = dao.selectEnabledRootCharacteristics(session);
-      int currentPosition = Iterables.indexOf(rootCharacteristics, new Predicate<CharacteristicDto>() {
-        @Override
-        public boolean apply(CharacteristicDto input) {
-          return input.getKey().equals(dto.getKey());
-        }
-      });
-      Integer nextMove = moveUpOrDown ? (currentPosition > 0 ? currentPosition - 1 : null) : (currentPosition < rootCharacteristics.size()-1 ? currentPosition + 1 : null);
+      CharacteristicDto dtoToSwitchOrderWith = findCharacteristicToSwitchWith(dto, moveUpOrDown, session);
 
       // Do nothing when characteristic is already to the good location
-      if (nextMove == null) {
+      if (dtoToSwitchOrderWith == null) {
         return toCharacteristic(dto);
       }
 
-      CharacteristicDto dtoToSwitchOrderWith = Iterables.get(rootCharacteristics, nextMove);
       int nextOrder = dtoToSwitchOrderWith.getOrder();
       dtoToSwitchOrderWith.setOrder(currentOrder);
       dtoToSwitchOrderWith.setUpdatedAt(new Date(system2.now()));
@@ -163,6 +154,20 @@ public class DebtModelOperations implements ServerComponent {
     }
   }
 
+  @CheckForNull
+  private CharacteristicDto findCharacteristicToSwitchWith(final CharacteristicDto dto, final boolean moveUpOrDown, SqlSession session) {
+    // characteristics should be order by 'order'
+    List<CharacteristicDto> rootCharacteristics = dao.selectEnabledRootCharacteristics(session);
+    int currentPosition = Iterables.indexOf(rootCharacteristics, new Predicate<CharacteristicDto>() {
+      @Override
+      public boolean apply(CharacteristicDto input) {
+        return input.getKey().equals(dto.getKey());
+      }
+    });
+    Integer nextPosition = moveUpOrDown ? (currentPosition > 0 ? currentPosition - 1 : null) : (currentPosition < rootCharacteristics.size() - 1 ? currentPosition + 1 : null);
+    return nextPosition != null ? Iterables.get(rootCharacteristics, nextPosition) : null;
+  }
+
   /**
    * Disable characteristic and its sub characteristics or only sub characteristic.
    * Will also update every rules linked to sub characteristics by setting characteristic id to -1 and remove function, factor and offset.
@@ -174,27 +179,29 @@ public class DebtModelOperations implements ServerComponent {
     SqlSession session = mybatis.openBatchSession();
     try {
       CharacteristicDto characteristicOrSubCharacteristic = findCharacteristic(characteristicId, session);
-      disableDebtRules(
-        ruleDao.selectByCharacteristicOrSubCharacteristicId(characteristicOrSubCharacteristic.getId(), session),
-        updateDate,
-        session
-      );
-
+      // When root characteristic, browse sub characteristics and disable rule debt on each sub characteristic then disable it
       if (characteristicOrSubCharacteristic.getParentId() == null) {
         List<CharacteristicDto> subCharacteristics = dao.selectCharacteristicsByParentId(characteristicOrSubCharacteristic.getId(), session);
         for (CharacteristicDto subCharacteristic : subCharacteristics) {
-          disableCharacteristic(subCharacteristic, updateDate, session);
+          disableSubChracteristic(subCharacteristic, updateDate, session);
         }
+        disableCharacteristic(characteristicOrSubCharacteristic, updateDate, session);
+      } else {
+        // When sub characteristic, disable rule debt on the sub characteristic then disable it
+        disableSubChracteristic(characteristicOrSubCharacteristic, updateDate, session);
       }
-      disableCharacteristic(characteristicOrSubCharacteristic, updateDate, session);
-
       session.commit();
     } finally {
       MyBatis.closeQuietly(session);
     }
   }
 
-  public void disableDebtRules(List<RuleDto> ruleDtos, Date updateDate, SqlSession session){
+  private void disableSubChracteristic(CharacteristicDto subCharacteristic, Date updateDate, SqlSession session) {
+    disableDebtRules(ruleDao.selectBySubCharacteristicId(subCharacteristic.getId(), session), updateDate, session);
+    disableCharacteristic(subCharacteristic, updateDate, session);
+  }
+
+  public void disableDebtRules(List<RuleDto> ruleDtos, Date updateDate, SqlSession session) {
     for (RuleDto ruleDto : ruleDtos) {
       ruleDto.setCharacteristicId(RuleDto.DISABLED_CHARACTERISTIC_ID);
       ruleDto.setRemediationFunction(null);
@@ -206,7 +213,7 @@ public class DebtModelOperations implements ServerComponent {
     }
   }
 
-  public void disableCharacteristic(CharacteristicDto characteristic, Date updateDate, SqlSession session){
+  public void disableCharacteristic(CharacteristicDto characteristic, Date updateDate, SqlSession session) {
     characteristic.setEnabled(false);
     characteristic.setUpdatedAt(updateDate);
     dao.update(characteristic, session);
diff --git a/sonar-server/src/test/java/org/sonar/server/debt/DebtModelOperationsTest.java b/sonar-server/src/test/java/org/sonar/server/debt/DebtModelOperationsTest.java
index f3ab22fe6b3..97570e977c3 100644
--- a/sonar-server/src/test/java/org/sonar/server/debt/DebtModelOperationsTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/debt/DebtModelOperationsTest.java
@@ -303,7 +303,7 @@ public class DebtModelOperationsTest {
     BatchSession batchSession = mock(BatchSession.class);
     when(mybatis.openBatchSession()).thenReturn(batchSession);
 
-    when(ruleDao.selectByCharacteristicOrSubCharacteristicId(2, batchSession)).thenReturn(newArrayList(
+    when(ruleDao.selectBySubCharacteristicId(2, batchSession)).thenReturn(newArrayList(
       new RuleDto()
         .setCharacteristicId(2).setRemediationFunction("LINEAR_OFFSET").setRemediationFactor("2h").setRemediationOffset("5min")
         .setDefaultCharacteristicId(10).setDefaultRemediationFunction("LINEAR_OFFSET").setDefaultRemediationFactor("4h").setDefaultRemediationOffset("15min")
@@ -344,8 +344,8 @@ public class DebtModelOperationsTest {
     BatchSession batchSession = mock(BatchSession.class);
     when(mybatis.openBatchSession()).thenReturn(batchSession);
 
-    when(ruleDao.selectByCharacteristicOrSubCharacteristicId(1, batchSession)).thenReturn(newArrayList(
-      new RuleDto().setCharacteristicId(2).setRemediationFunction("LINEAR_OFFSET").setRemediationFactor("2h").setRemediationOffset("5min")
+    when(ruleDao.selectBySubCharacteristicId(subCharacteristicDto.getId(), batchSession)).thenReturn(newArrayList(
+      new RuleDto().setCharacteristicId(subCharacteristicDto.getId()).setRemediationFunction("LINEAR_OFFSET").setRemediationFactor("2h").setRemediationOffset("5min")
     ));
     when(dao.selectCharacteristicsByParentId(1, batchSession)).thenReturn(newArrayList(
       subCharacteristicDto