SONAR-6644 fix debt when characteristic is none

5 files changed, 77 insertions, 27 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/RuleImpl.java b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/RuleImpl.java
index a30d9cd9d72..483ed15c924 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/computation/issue/RuleImpl.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/computation/issue/RuleImpl.java
@@ -48,7 +48,7 @@ public class RuleImpl implements Rule {
     this.key = dto.getKey();
     this.name = dto.getName();
     this.status = dto.getStatus();
-    this.subCharacteristicId = effectiveCharacteristicId(dto);
+    this.subCharacteristicId = dto.getEffectiveSubCharacteristicId();
     this.tags = union(dto.getSystemTags(), dto.getTags());
     this.remediationFunction = effectiveRemediationFunction(dto);
   }
@@ -118,21 +118,6 @@ public class RuleImpl implements Rule {
   }
 
   @CheckForNull
-  private static Integer effectiveCharacteristicId(RuleDto dto) {
-    if (isEnabledCharacteristicId(dto.getSubCharacteristicId())) {
-      return dto.getSubCharacteristicId();
-    }
-    if (isEnabledCharacteristicId(dto.getDefaultSubCharacteristicId())) {
-      return dto.getDefaultSubCharacteristicId();
-    }
-    return null;
-  }
-
-  private static boolean isEnabledCharacteristicId(@Nullable Integer id) {
-    return (id != null) && (id.intValue() != RuleDto.DISABLED_CHARACTERISTIC_ID);
-  }
-
-  @CheckForNull
   private static DebtRemediationFunction effectiveRemediationFunction(RuleDto dto) {
     String fn = dto.getRemediationFunction();
     if (fn != null) {
diff --git a/server/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java b/server/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java
index a16ea010fba..f3ab358f804 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java
@@ -346,9 +346,8 @@ public class DebtModelBackup {
   @CheckForNull
   private static RuleDebt toRuleDebt(RuleDto rule, DebtModel debtModel) {
     RuleDebt ruleDebt = new RuleDebt().setRuleKey(RuleKey.of(rule.getRepositoryKey(), rule.getRuleKey()));
-    Integer effectiveSubCharacteristicId = rule.getSubCharacteristicId() != null ? rule.getSubCharacteristicId() : rule.getDefaultSubCharacteristicId();
-    DebtCharacteristic subCharacteristic = (effectiveSubCharacteristicId != null && !RuleDto.DISABLED_CHARACTERISTIC_ID.equals(effectiveSubCharacteristicId)) ?
-      debtModel.characteristicById(effectiveSubCharacteristicId) : null;
+    Integer effectiveSubCharacteristicId = rule.getEffectiveSubCharacteristicId();
+    DebtCharacteristic subCharacteristic = effectiveSubCharacteristicId != null ? debtModel.characteristicById(effectiveSubCharacteristicId) : null;
     if (subCharacteristic != null) {
       ruleDebt.setSubCharacteristicKey(subCharacteristic.key());
 
diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java
index 8bbc7521acb..3172f6c9b18 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java
@@ -111,7 +111,8 @@ public class RuleOperations {
       // No sub-characteristic is given -> disable rule debt if not already disabled
     } else {
       // Rule characteristic is not already disabled -> update it
-      if (ruleDto.getSubCharacteristicId() == null || !RuleDto.DISABLED_CHARACTERISTIC_ID.equals(ruleDto.getSubCharacteristicId())) {
+      Integer subCharacteristicId = ruleDto.getSubCharacteristicId();
+      if (subCharacteristicId == null || subCharacteristicId != RuleDto.DISABLED_CHARACTERISTIC_ID) {
         // If default characteristic is not defined, set the overridden characteristic to null in order to be able to track debt plugin
         // update
         ruleDto.setSubCharacteristicId(ruleDto.getDefaultSubCharacteristicId() != null ? RuleDto.DISABLED_CHARACTERISTIC_ID : null);
diff --git a/sonar-db/src/main/java/org/sonar/db/rule/RuleDto.java b/sonar-db/src/main/java/org/sonar/db/rule/RuleDto.java
index d3188ec87db..2c5a17b4f59 100644
--- a/sonar-db/src/main/java/org/sonar/db/rule/RuleDto.java
+++ b/sonar-db/src/main/java/org/sonar/db/rule/RuleDto.java
@@ -38,7 +38,7 @@ import org.sonar.db.Dto;
 
 public class RuleDto extends Dto<RuleKey> {
 
-  public static final Integer DISABLED_CHARACTERISTIC_ID = -1;
+  public static final int DISABLED_CHARACTERISTIC_ID = -1;
 
   public enum Format {
     HTML, MARKDOWN
@@ -259,6 +259,15 @@ public class RuleDto extends Dto<RuleKey> {
   }
 
   @CheckForNull
+  public Integer getEffectiveSubCharacteristicId() {
+    Integer effective = subCharacteristicId == null ? defaultSubCharacteristicId : subCharacteristicId;
+    if (effective != null && effective != DISABLED_CHARACTERISTIC_ID) {
+      return effective;
+    }
+    return null;
+  }
+
+  @CheckForNull
   public String getRemediationFunction() {
     return remediationFunction;
   }
@@ -329,15 +338,11 @@ public class RuleDto extends Dto<RuleKey> {
   }
 
   public Set<String> getTags() {
-    return tags == null ?
-      new HashSet<String>() :
-      new TreeSet<>(Arrays.asList(StringUtils.split(tags, ',')));
+    return tags == null ? new HashSet<String>() : new TreeSet<>(Arrays.asList(StringUtils.split(tags, ',')));
   }
 
   public Set<String> getSystemTags() {
-    return systemTags == null ?
-      new HashSet<String>() :
-      new TreeSet<>(Arrays.asList(StringUtils.split(systemTags, ',')));
+    return systemTags == null ? new HashSet<String>() : new TreeSet<>(Arrays.asList(StringUtils.split(systemTags, ',')));
   }
 
   private String getTagsField() {
diff --git a/sonar-db/src/test/java/org/sonar/db/rule/RuleDtoTest.java b/sonar-db/src/test/java/org/sonar/db/rule/RuleDtoTest.java
new file mode 100644
index 00000000000..7c170e7c8df
--- /dev/null
+++ b/sonar-db/src/test/java/org/sonar/db/rule/RuleDtoTest.java
@@ -0,0 +1,60 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2014 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.db.rule;
+
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.sonar.db.rule.RuleDto.DISABLED_CHARACTERISTIC_ID;
+
+public class RuleDtoTest {
+
+  public static final int FAKE_SUB_CHAR_1 = 27;
+  public static final int FAKE_SUB_CHAR_2 = 42;
+
+  @Test
+  public void effective_sub_characteristic_id() {
+    RuleDto dto = new RuleDto();
+
+    // characteristic is not set
+    dto.setSubCharacteristicId(null).setDefaultSubCharacteristicId(null);
+    assertThat(dto.getEffectiveSubCharacteristicId()).isNull();
+
+    // default characteristic is set
+    dto.setSubCharacteristicId(null).setDefaultSubCharacteristicId(FAKE_SUB_CHAR_2);
+    assertThat(dto.getEffectiveSubCharacteristicId()).isEqualTo(FAKE_SUB_CHAR_2);
+
+    // default characteristic is set to "none"
+    dto.setSubCharacteristicId(null).setDefaultSubCharacteristicId(DISABLED_CHARACTERISTIC_ID);
+    assertThat(dto.getEffectiveSubCharacteristicId()).isNull();
+
+    // characteristic is overridden
+    dto.setSubCharacteristicId(FAKE_SUB_CHAR_1).setDefaultSubCharacteristicId(FAKE_SUB_CHAR_2);
+    assertThat(dto.getEffectiveSubCharacteristicId()).isEqualTo(FAKE_SUB_CHAR_1);
+
+    // characteristic is set, no defaults
+    dto.setSubCharacteristicId(FAKE_SUB_CHAR_1).setDefaultSubCharacteristicId(null);
+    assertThat(dto.getEffectiveSubCharacteristicId()).isEqualTo(FAKE_SUB_CHAR_1);
+
+    // characteristic is set to "none"
+    dto.setSubCharacteristicId(DISABLED_CHARACTERISTIC_ID).setDefaultSubCharacteristicId(FAKE_SUB_CHAR_2);
+    assertThat(dto.getEffectiveSubCharacteristicId()).isNull();
+  }
+}