Fix quality flaws

diff --git a/sonar-core/src/test/java/org/sonar/core/component/ComponentDtoTest.java b/sonar-core/src/test/java/org/sonar/core/component/ComponentDtoTest.java
new file mode 100644 (file)
index 0000000..1b2ea6a
--- /dev/null
+++ b/sonar-core/src/test/java/org/sonar/core/component/ComponentDtoTest.java
@@ -0,0 +1,65 @@
+/*
+ * 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.core.component;
+
+import org.junit.Test;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+public class ComponentDtoTest {
+
+  @Test
+  public void setters_and_getters() throws Exception {
+    ComponentDto componentDto = new ComponentDto()
+      .setId(1L)
+      .setKey("org.struts:struts-core:src/org/struts/RequestContext.java")
+      .setName("RequestContext.java")
+      .setLongName("org.struts.RequestContext")
+      .setQualifier("FIL")
+      .setPath("src/org/struts/RequestContext.java")
+      .setProjectId(2L)
+      .setSubProjectId(3L);
+
+    assertThat(componentDto.getId()).isEqualTo(1L);
+    assertThat(componentDto.key()).isEqualTo("org.struts:struts-core:src/org/struts/RequestContext.java");
+    assertThat(componentDto.name()).isEqualTo("RequestContext.java");
+    assertThat(componentDto.longName()).isEqualTo("org.struts.RequestContext");
+    assertThat(componentDto.qualifier()).isEqualTo("FIL");
+    assertThat(componentDto.path()).isEqualTo("src/org/struts/RequestContext.java");
+    assertThat(componentDto.projectId()).isEqualTo(2L);
+    assertThat(componentDto.subProjectId()).isEqualTo(3L);
+  }
+
+  @Test
+  public void equals_and_hashcode() throws Exception {
+    ComponentDto dto = new ComponentDto().setId(1L);
+    ComponentDto dtoWithSameId = new ComponentDto().setId(1L);
+    ComponentDto dtoWithDifferentId = new ComponentDto().setId(2L);
+
+    assertThat(dto).isEqualTo(dto);
+    assertThat(dto).isEqualTo(dtoWithSameId);
+    assertThat(dto).isNotEqualTo(dtoWithDifferentId);
+
+    assertThat(dto.hashCode()).isEqualTo(dto.hashCode());
+    assertThat(dto.hashCode()).isEqualTo(dtoWithSameId.hashCode());
+    assertThat(dto.hashCode()).isNotEqualTo(dtoWithDifferentId.hashCode());
+  }
+}

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java b/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java
index e5de8f01f4a9e6dd3eae7c17d23adc3b8c37384f..1f0beba8d2c7e0383ea591caa4783e7fb68f362e 100644 (file)
--- a/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/internal/DefaultCharacteristic.java
@@ -83,7 +83,6 @@ public class DefaultCharacteristic implements Characteristic {
     return this;
   }
 
-  // TODO Replace this is a TechnicalDebtFactory class
   public DefaultCharacteristic setName(String s, boolean asKey) {
     this.name = StringUtils.trimToNull(s);
     if (asKey) {
@@ -157,11 +156,12 @@ public class DefaultCharacteristic implements Characteristic {
     return this;
   }
 
+  @CheckForNull
   public Date updatedAt() {
     return updatedAt;
   }
 
-  public DefaultCharacteristic setUpdatedAt(Date updatedAt) {
+  public DefaultCharacteristic setUpdatedAt(@Nullable Date updatedAt) {
     this.updatedAt = updatedAt;
     return this;
   }

diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtCharacteristicTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtCharacteristicTest.java
new file mode 100644 (file)
index 0000000..2a09fe5
--- /dev/null
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtCharacteristicTest.java
@@ -0,0 +1,81 @@
+/*
+ * 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.api.batch.debt.internal;
+
+import org.junit.Test;
+
+import java.util.Date;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+public class DefaultDebtCharacteristicTest {
+
+  @Test
+  public void setter_and_getter_on_characteristic() throws Exception {
+    DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic()
+      .setId(1)
+      .setKey("PORTABILITY")
+      .setName("Portability")
+      .setOrder(1)
+      .setCreatedAt(new Date())
+      .setUpdatedAt(new Date());
+
+    assertThat(debtCharacteristic.id()).isEqualTo(1);
+    assertThat(debtCharacteristic.key()).isEqualTo("PORTABILITY");
+    assertThat(debtCharacteristic.name()).isEqualTo("Portability");
+    assertThat(debtCharacteristic.order()).isEqualTo(1);
+    assertThat(debtCharacteristic.parentId()).isNull();
+    assertThat(debtCharacteristic.isSub()).isFalse();
+    assertThat(debtCharacteristic.createdAt()).isNotNull();
+    assertThat(debtCharacteristic.updatedAt()).isNotNull();
+  }
+
+  @Test
+  public void setter_and_getter_on_sub_characteristic() throws Exception {
+    DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic()
+      .setId(1)
+      .setKey("COMPILER")
+      .setName("Compiler")
+      .setParentId(2)
+      .setCreatedAt(new Date())
+      .setUpdatedAt(new Date());
+
+    assertThat(debtCharacteristic.id()).isEqualTo(1);
+    assertThat(debtCharacteristic.key()).isEqualTo("COMPILER");
+    assertThat(debtCharacteristic.name()).isEqualTo("Compiler");
+    assertThat(debtCharacteristic.order()).isNull();
+    assertThat(debtCharacteristic.parentId()).isEqualTo(2);
+    assertThat(debtCharacteristic.isSub()).isTrue();
+    assertThat(debtCharacteristic.createdAt()).isNotNull();
+    assertThat(debtCharacteristic.updatedAt()).isNotNull();
+  }
+
+  @Test
+  public void to_string() throws Exception {
+    assertThat(new DefaultDebtCharacteristic()
+      .setId(1)
+      .setKey("PORTABILITY")
+      .setName("Portability")
+      .setOrder(1)
+      .setCreatedAt(new Date())
+      .setUpdatedAt(new Date())).isNotNull();
+  }
+}

diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java
index 561cb5ed3ae296e5b09a9eaac94c4046411af1f3..a5dfe422bf25f7345de3e48a1d93a93c7a021127 100644 (file)
--- a/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/batch/debt/internal/DefaultDebtModelTest.java
@@ -72,6 +72,9 @@ public class DefaultDebtModelTest {
     assertThat(debtCharacteristic.order()).isEqualTo(1);
     assertThat(debtCharacteristic.parentId()).isNull();
     assertThat(debtCharacteristic.isSub()).isFalse();
+
+
+    assertThat(debtModel.characteristicById(555)).isNull();
   }
 
   @Test
@@ -84,5 +87,7 @@ public class DefaultDebtModelTest {
     assertThat(debtCharacteristic.order()).isNull();
     assertThat(debtCharacteristic.parentId()).isEqualTo(1);
     assertThat(debtCharacteristic.isSub()).isTrue();
+
+    assertThat(debtModel.characteristicByKey("UNKNOWN")).isNull();
   }
 }

diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/server/debt/internal/DefaultDebtCharacteristicTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/server/debt/internal/DefaultDebtCharacteristicTest.java
new file mode 100644 (file)
index 0000000..f7d84ce
--- /dev/null
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/server/debt/internal/DefaultDebtCharacteristicTest.java
@@ -0,0 +1,81 @@
+/*
+ * 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.api.server.debt.internal;
+
+import org.junit.Test;
+
+import java.util.Date;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+public class DefaultDebtCharacteristicTest {
+
+  @Test
+  public void setter_and_getter_on_characteristic() throws Exception {
+    DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic()
+      .setId(1)
+      .setKey("PORTABILITY")
+      .setName("Portability")
+      .setOrder(1)
+      .setCreatedAt(new Date())
+      .setUpdatedAt(new Date());
+
+    assertThat(debtCharacteristic.id()).isEqualTo(1);
+    assertThat(debtCharacteristic.key()).isEqualTo("PORTABILITY");
+    assertThat(debtCharacteristic.name()).isEqualTo("Portability");
+    assertThat(debtCharacteristic.order()).isEqualTo(1);
+    assertThat(debtCharacteristic.parentId()).isNull();
+    assertThat(debtCharacteristic.isSub()).isFalse();
+    assertThat(debtCharacteristic.createdAt()).isNotNull();
+    assertThat(debtCharacteristic.updatedAt()).isNotNull();
+  }
+
+  @Test
+  public void setter_and_getter_on_sub_characteristic() throws Exception {
+    DefaultDebtCharacteristic debtCharacteristic = new DefaultDebtCharacteristic()
+      .setId(1)
+      .setKey("COMPILER")
+      .setName("Compiler")
+      .setParentId(2)
+      .setCreatedAt(new Date())
+      .setUpdatedAt(new Date());
+
+    assertThat(debtCharacteristic.id()).isEqualTo(1);
+    assertThat(debtCharacteristic.key()).isEqualTo("COMPILER");
+    assertThat(debtCharacteristic.name()).isEqualTo("Compiler");
+    assertThat(debtCharacteristic.order()).isNull();
+    assertThat(debtCharacteristic.parentId()).isEqualTo(2);
+    assertThat(debtCharacteristic.isSub()).isTrue();
+    assertThat(debtCharacteristic.createdAt()).isNotNull();
+    assertThat(debtCharacteristic.updatedAt()).isNotNull();
+  }
+
+  @Test
+  public void to_string() throws Exception {
+    assertThat(new DefaultDebtCharacteristic()
+      .setId(1)
+      .setKey("PORTABILITY")
+      .setName("Portability")
+      .setOrder(1)
+      .setCreatedAt(new Date())
+      .setUpdatedAt(new Date())).isNotNull();
+  }
+}

diff --git a/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java b/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java
index d0855fe34d8a2e629be69961d909d4b013ad473f..3ff96509ce84bd736548f6472fb76ecb992e5a96 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java
+++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtCharacteristicsXMLImporter.java
@@ -85,7 +85,6 @@ public class DebtCharacteristicsXMLImporter implements ServerComponent {
     while (cursor.getNext() != null) {
       String node = cursor.getLocalName();
       if (StringUtils.equals(node, CHARACTERISTIC_KEY)) {
-        // TODO Attached to parent only if a key is existing, otherwise characteristic with empty key can be added.
         characteristic.setKey(cursor.collectDescendantText().trim());
         if (parent == null) {
           characteristic.setOrder(debtModel.rootCharacteristics().size() + 1);

diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java
index 89db8d2154400bf70a27554c4faee70f0673d2b6..e320369efdfe315e4d2b8285e92ff5e749ebe617 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java
+++ b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java
@@ -268,27 +268,31 @@ public class RegisterRules implements Startable {
   }
 
   private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto, @Nullable CharacteristicDto subCharacteristic) {
-    boolean changed = false;
+    // Debt definitions are set to null if the sub-characteristic and the remediation function are null
+    DebtRemediationFunction debtRemediationFunction = subCharacteristic != null ? def.debtRemediationFunction() : null;
+    boolean hasDebt = subCharacteristic != null && debtRemediationFunction != null;
+    return mergeDebtDefinitions(def, dto,
+      hasDebt ? subCharacteristic.getId() : null,
+      debtRemediationFunction != null ? debtRemediationFunction.type().name() : null,
+      hasDebt ? debtRemediationFunction.coefficient() : null,
+      hasDebt ? debtRemediationFunction.offset() : null,
+      hasDebt ? def.effortToFixDescription() : null);
+  }
 
-    // Debt definitions are set to null if the sub-characteristic is null or unknown
-    boolean hasCharacteristic = subCharacteristic != null;
-    DebtRemediationFunction debtRemediationFunction = hasCharacteristic ? def.debtRemediationFunction() : null;
-    Integer characteristicId = hasCharacteristic ? subCharacteristic.getId() : null;
-    String remediationFactor = hasCharacteristic ? debtRemediationFunction.coefficient() : null;
-    String remediationOffset = hasCharacteristic ? debtRemediationFunction.offset() : null;
-    String effortToFixDescription = hasCharacteristic ? def.effortToFixDescription() : null;
+  private boolean mergeDebtDefinitions(RulesDefinition.Rule def, RuleDto dto,@Nullable  Integer characteristicId, @Nullable String remediationFunction,
+                                       @Nullable String remediationCoefficient, @Nullable String remediationOffset, @Nullable String effortToFixDescription) {
+    boolean changed = false;
 
     if (!ObjectUtils.equals(dto.getDefaultSubCharacteristicId(), characteristicId)) {
       dto.setDefaultSubCharacteristicId(characteristicId);
       changed = true;
     }
-    String remediationFunctionString = debtRemediationFunction != null ? debtRemediationFunction.type().name() : null;
-    if (!StringUtils.equals(dto.getDefaultRemediationFunction(), remediationFunctionString)) {
-      dto.setDefaultRemediationFunction(remediationFunctionString);
+    if (!StringUtils.equals(dto.getDefaultRemediationFunction(), remediationFunction)) {
+      dto.setDefaultRemediationFunction(remediationFunction);
       changed = true;
     }
-    if (!StringUtils.equals(dto.getDefaultRemediationCoefficient(), remediationFactor)) {
-      dto.setDefaultRemediationCoefficient(remediationFactor);
+    if (!StringUtils.equals(dto.getDefaultRemediationCoefficient(), remediationCoefficient)) {
+      dto.setDefaultRemediationCoefficient(remediationCoefficient);
       changed = true;
     }
     if (!StringUtils.equals(dto.getDefaultRemediationOffset(), remediationOffset)) {

diff --git a/sonar-server/src/main/java/org/sonar/server/rule/Rule.java b/sonar-server/src/main/java/org/sonar/server/rule/Rule.java
index 6bfab78a5308390c6460bf83165b4806ec188a54..ba5c933524c602c907074e249dd55150c97d92ef 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/rule/Rule.java
+++ b/sonar-server/src/main/java/org/sonar/server/rule/Rule.java
@@ -180,9 +180,7 @@ public class Rule {
     private Collection<String> adminTags;
     private Collection<RuleParam> params;
     private String debtCharacteristicKey;
-    private String debtCharacteristicName;
     private String debtSubCharacteristicKey;
-    private String debtSubCharacteristicName;
     private DebtRemediationFunction debtRemediationFunction;
     private Date createdAt;
     private Date updatedAt;

diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java
index 09c08cb2d6588dda1cba57ae3acbc417b1309600..ac6c87e3c956b0e9d3f986958ae7496094d5ff90 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java
+++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleDocumentParser.java
@@ -54,22 +54,28 @@ public class RuleDocumentParser {
       .setCreatedAt(parseOptionalDate(RuleDocument.FIELD_CREATED_AT, ruleSource))
       .setUpdatedAt(parseOptionalDate(RuleDocument.FIELD_UPDATED_AT, ruleSource));
 
+    addRuleDebt(ruleSource, ruleBuilder);
+    addRuleNote(ruleSource, ruleBuilder);
+    addRuleParams(ruleSource, ruleBuilder);
+    addRuleTags(ruleSource, ruleBuilder);
+    return ruleBuilder.build();
+  }
+
+  private static void addRuleDebt(Map<String, Object> ruleSource, Rule.Builder ruleBuilder) {
     if (ruleSource.containsKey(RuleDocument.FIELD_CHARACTERISTIC_KEY)) {
-      try {
-        ruleBuilder
-          .setDebtCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_CHARACTERISTIC_KEY))
-          .setDebtSubCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY))
-          .setDebtRemediationFunction(
-            new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.valueOf((String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_FUNCTION)),
-              (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_COEFFICIENT),
-              (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_OFFSET))
-          )
-        ;
-      } catch (IllegalArgumentException e) {
-        throw new RuntimeException("Fail on rule '" + ruleSource.get(RuleDocument.FIELD_KEY) + "', " + e.getMessage(), e);
-      }
+      ruleBuilder
+        .setDebtCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_CHARACTERISTIC_KEY))
+        .setDebtSubCharacteristicKey((String) ruleSource.get(RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY))
+        .setDebtRemediationFunction(
+          new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.valueOf((String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_FUNCTION)),
+            (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_COEFFICIENT),
+            (String) ruleSource.get(RuleDocument.FIELD_REMEDIATION_OFFSET))
+        )
+      ;
     }
+  }
 
+  private static void addRuleNote(Map<String, Object> ruleSource, Rule.Builder ruleBuilder) {
     if (ruleSource.containsKey(RuleDocument.FIELD_NOTE)) {
       Map<String, Object> ruleNoteDocument = (Map<String, Object>) ruleSource.get(RuleDocument.FIELD_NOTE);
       ruleBuilder.setRuleNote(new RuleNote(
@@ -79,7 +85,9 @@ public class RuleDocumentParser {
         parseOptionalDate(RuleDocument.FIELD_NOTE_UPDATED_AT, ruleNoteDocument)
       ));
     }
+  }
 
+  private static void addRuleParams(Map<String, Object> ruleSource, Rule.Builder ruleBuilder) {
     List<RuleParam> params = Lists.newArrayList();
     if (ruleSource.containsKey(RuleDocument.FIELD_PARAMS)) {
       Map<String, Map<String, Object>> ruleParams = Maps.newHashMap();
@@ -97,7 +105,9 @@ public class RuleDocumentParser {
       }
     }
     ruleBuilder.setParams(params);
+  }
 
+  private static void addRuleTags(Map<String, Object> ruleSource, Rule.Builder ruleBuilder) {
     List<String> systemTags = newArrayList();
     if (ruleSource.containsKey(RuleDocument.FIELD_SYSTEM_TAGS)) {
       for (String tag : (List<String>) ruleSource.get(RuleDocument.FIELD_SYSTEM_TAGS)) {
@@ -113,8 +123,6 @@ public class RuleDocumentParser {
       }
     }
     ruleBuilder.setAdminTags(adminTags);
-
-    return ruleBuilder.build();
   }
 
   public static Date parseOptionalDate(String field, Map<String, Object> ruleSource) {

diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java
index 75c4b3c26e347f35e9d9dc11fda8e3bc060a917e..be2b3022ccedf8f34bc3e6716d53a534e444d911 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java
+++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleOperations.java
@@ -287,20 +287,13 @@ public class RuleOperations implements ServerComponent {
   }
 
   public boolean updateRule(RuleDto ruleDto, @Nullable CharacteristicDto newSubCharacteristic, @Nullable String newFunction,
-                            @Nullable String newCoefficient, @Nullable String newOffset, Date updateDate, SqlSession session){
+                            @Nullable String newCoefficient, @Nullable String newOffset, Date updateDate, SqlSession session) {
     boolean needUpdate = false;
 
-    // A sub-characteristic is given -> update rule debt
-    if (newSubCharacteristic != null) {
-      boolean iSameAsDefaultValues = newSubCharacteristic.getId().equals(ruleDto.getDefaultSubCharacteristicId())
-        && isSameRemediationFunction(newFunction, newCoefficient, newOffset,
-        ruleDto.getDefaultRemediationFunction(), ruleDto.getDefaultRemediationCoefficient(), ruleDto.getDefaultRemediationOffset());
-      boolean iSameAsOverriddenValues = newSubCharacteristic.getId().equals(ruleDto.getSubCharacteristicId())
-        && isSameRemediationFunction(newFunction, newCoefficient, newOffset,
-        ruleDto.getRemediationFunction(), ruleDto.getRemediationCoefficient(), ruleDto.getRemediationOffset());
-
+    // A sub-characteristic and a remediation function is given -> update rule debt
+    if (newSubCharacteristic != null && newFunction != null) {
       // New values are the same as the default values -> set overridden values to null
-      if (iSameAsDefaultValues) {
+      if (isRuleDebtSameAsDefaultValues(ruleDto, newSubCharacteristic, newFunction, newCoefficient, newOffset)) {
         ruleDto.setSubCharacteristicId(null);
         ruleDto.setRemediationFunction(null);
         ruleDto.setRemediationCoefficient(null);
@@ -308,7 +301,7 @@ public class RuleOperations implements ServerComponent {
         needUpdate = true;
 
         // New values are not the same as the overridden values -> update overridden values with new values
-      } else if (!iSameAsOverriddenValues) {
+      } else if (!isRuleDebtSameAsOverriddenValues(ruleDto, newSubCharacteristic, newFunction, newCoefficient, newOffset)) {
         ruleDto.setSubCharacteristicId(newSubCharacteristic.getId());
 
         DefaultDebtRemediationFunction debtRemediationFunction = new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.valueOf(newFunction), newCoefficient, newOffset);
@@ -338,6 +331,19 @@ public class RuleOperations implements ServerComponent {
     return needUpdate;
   }
 
+  private static boolean isRuleDebtSameAsDefaultValues(RuleDto ruleDto, CharacteristicDto newSubCharacteristic, @Nullable String newFunction,
+                                              @Nullable String newCoefficient, @Nullable String newOffset) {
+    return newSubCharacteristic.getId().equals(ruleDto.getDefaultSubCharacteristicId()) &&
+      isSameRemediationFunction(newFunction, newCoefficient, newOffset, ruleDto.getDefaultRemediationFunction(), ruleDto.getDefaultRemediationCoefficient(),
+        ruleDto.getDefaultRemediationOffset());
+  }
+
+  private static boolean isRuleDebtSameAsOverriddenValues(RuleDto ruleDto, CharacteristicDto newSubCharacteristic, @Nullable String newFunction,
+                                                 @Nullable String newCoefficient, @Nullable String newOffset) {
+    return newSubCharacteristic.getId().equals(ruleDto.getSubCharacteristicId())
+      && isSameRemediationFunction(newFunction, newCoefficient, newOffset, ruleDto.getRemediationFunction(), ruleDto.getRemediationCoefficient(), ruleDto.getRemediationOffset());
+  }
+
   private static boolean isSameRemediationFunction(@Nullable String newFunction, @Nullable String newCoefficient, @Nullable String newOffset,
                                                    String oldFunction, @Nullable String oldCoefficient, @Nullable String oldOffset) {
     return new EqualsBuilder()

diff --git a/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java b/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java
index 5258139b56db1ff6fcdb855b07f91fd2753b845a..6bc01497bd597dd5bb0cfe597849cf8721afdb21 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java
+++ b/sonar-server/src/main/java/org/sonar/server/rule/RuleRegistry.java
@@ -176,7 +176,9 @@ public class RuleRegistry {
       Integer ruleId = rule.getId();
       Integer effectiveSubCharacteristicId = rule.getSubCharacteristicId() != null ? rule.getSubCharacteristicId() : rule.getDefaultSubCharacteristicId();
       CharacteristicDto subCharacteristic = effectiveSubCharacteristicId != null ? characteristicDao.selectById(effectiveSubCharacteristicId, session) : null;
-      CharacteristicDto characteristic = subCharacteristic != null ? characteristicDao.selectById(subCharacteristic.getParentId(), session) : null;
+      // The parent id of the sub-characteristic should never be null
+      CharacteristicDto characteristic = (subCharacteristic != null && subCharacteristic.getParentId() != null) ?
+        characteristicDao.selectById(subCharacteristic.getParentId(), session) : null;
       searchIndex.putSynchronous(INDEX_RULES, TYPE_RULE, Long.toString(ruleId), ruleDocument(rule,
         characteristic, subCharacteristic,
         ruleDao.selectParametersByRuleIds(newArrayList(ruleId), session),
@@ -240,6 +242,16 @@ public class RuleRegistry {
       .field(RuleDocument.FIELD_CARDINALITY, rule.getCardinality())
       .field(RuleDocument.FIELD_CREATED_AT, rule.getCreatedAt())
       .field(RuleDocument.FIELD_UPDATED_AT, rule.getUpdatedAt());
+    addRuleDebt(document, rule, characteristicDto, subCharacteristicDto);
+    addRuleNote(document, rule);
+    addRuleParams(document, rule, params);
+    addRuleTags(document, rule, tags);
+    document.endObject();
+    return document;
+  }
+
+  private void addRuleDebt(XContentBuilder document, RuleDto rule, @Nullable CharacteristicDto characteristicDto, @Nullable CharacteristicDto subCharacteristicDto)
+    throws IOException {
     if (characteristicDto != null && subCharacteristicDto != null) {
       boolean isFunctionOverridden = rule.getRemediationFunction() != null;
       document
@@ -251,7 +263,9 @@ public class RuleRegistry {
         .field(RuleDocument.FIELD_REMEDIATION_COEFFICIENT, isFunctionOverridden ? rule.getRemediationCoefficient() : rule.getDefaultRemediationCoefficient())
         .field(RuleDocument.FIELD_REMEDIATION_OFFSET, isFunctionOverridden ? rule.getRemediationOffset() : rule.getDefaultRemediationOffset());
     }
+  }
 
+  private void addRuleNote(XContentBuilder document, RuleDto rule) throws IOException {
     if (rule.getNoteData() != null || rule.getNoteUserLogin() != null) {
       document.startObject(RuleDocument.FIELD_NOTE)
         .field(RuleDocument.FIELD_NOTE_DATA, rule.getNoteData())
@@ -260,6 +274,9 @@ public class RuleRegistry {
         .field(RuleDocument.FIELD_NOTE_UPDATED_AT, rule.getNoteUpdatedAt())
         .endObject();
     }
+  }
+
+  private void addRuleParams(XContentBuilder document, RuleDto rule, Collection<RuleParamDto> params) throws IOException {
     if (!params.isEmpty()) {
       document.startArray(RuleDocument.FIELD_PARAMS);
       for (RuleParamDto param : params) {
@@ -272,6 +289,9 @@ public class RuleRegistry {
       }
       document.endArray();
     }
+  }
+
+  private void addRuleTags(XContentBuilder document, RuleDto rule, Collection<RuleRuleTagDto> tags) throws IOException {
     List<String> systemTags = Lists.newArrayList();
     List<String> adminTags = Lists.newArrayList();
     for (RuleRuleTagDto tag : tags) {
@@ -287,9 +307,6 @@ public class RuleRegistry {
     if (!adminTags.isEmpty()) {
       document.array(RuleDocument.FIELD_ADMIN_TAGS, adminTags.toArray());
     }
-
-    document.endObject();
-    return document;
   }
 
 
@@ -334,32 +351,7 @@ public class RuleRegistry {
   }
 
   public PagedResult<Rule> find(RuleQuery query) {
-    BoolFilterBuilder mainFilter = boolFilter().mustNot(termFilter(RuleDocument.FIELD_STATUS, STATUS_REMOVED));
-    if (StringUtils.isNotBlank(query.searchQuery())) {
-      mainFilter.must(FilterBuilders.queryFilter(
-        QueryBuilders.multiMatchQuery(query.searchQuery(), RuleDocument.FIELD_NAME + ".search", RuleDocument.FIELD_KEY).operator(Operator.AND)));
-    }
-    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_LANGUAGE, query.languages());
-    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_REPOSITORY_KEY, query.repositories());
-    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_SEVERITY, query.severities());
-    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_STATUS, query.statuses());
-    if (!query.tags().isEmpty()) {
-      mainFilter.must(FilterBuilders.queryFilter(
-          QueryBuilders.multiMatchQuery(query.tags(), RuleDocument.FIELD_ADMIN_TAGS, RuleDocument.FIELD_SYSTEM_TAGS).operator(Operator.OR))
-      );
-    }
-    if (!query.debtCharacteristics().isEmpty()) {
-      mainFilter.must(FilterBuilders.queryFilter(
-          QueryBuilders.multiMatchQuery(query.debtCharacteristics(), RuleDocument.FIELD_CHARACTERISTIC_KEY, RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY).operator(Operator.OR))
-      );
-    }
-    if (query.hasDebtCharacteristic() != null) {
-      if (Boolean.TRUE.equals(query.hasDebtCharacteristic())) {
-        mainFilter.must(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY));
-      } else {
-        mainFilter.mustNot(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY));
-      }
-    }
+    BoolFilterBuilder mainFilter = convertRuleQueryToFilterBuilder(query);
 
     Builder<Rule> rulesBuilder = ImmutableList.builder();
     SearchRequestBuilder searchRequestBuilder =
@@ -401,6 +393,36 @@ public class RuleRegistry {
     }
   }
 
+  private static BoolFilterBuilder convertRuleQueryToFilterBuilder(RuleQuery query){
+    BoolFilterBuilder mainFilter = boolFilter().mustNot(termFilter(RuleDocument.FIELD_STATUS, STATUS_REMOVED));
+    if (StringUtils.isNotBlank(query.searchQuery())) {
+      mainFilter.must(FilterBuilders.queryFilter(
+        QueryBuilders.multiMatchQuery(query.searchQuery(), RuleDocument.FIELD_NAME + ".search", RuleDocument.FIELD_KEY).operator(Operator.AND)));
+    }
+    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_LANGUAGE, query.languages());
+    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_REPOSITORY_KEY, query.repositories());
+    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_SEVERITY, query.severities());
+    addMustTermOrTerms(mainFilter, RuleDocument.FIELD_STATUS, query.statuses());
+    if (!query.tags().isEmpty()) {
+      mainFilter.must(FilterBuilders.queryFilter(
+          QueryBuilders.multiMatchQuery(query.tags(), RuleDocument.FIELD_ADMIN_TAGS, RuleDocument.FIELD_SYSTEM_TAGS).operator(Operator.OR))
+      );
+    }
+    if (!query.debtCharacteristics().isEmpty()) {
+      mainFilter.must(FilterBuilders.queryFilter(
+          QueryBuilders.multiMatchQuery(query.debtCharacteristics(), RuleDocument.FIELD_CHARACTERISTIC_KEY, RuleDocument.FIELD_SUB_CHARACTERISTIC_KEY).operator(Operator.OR))
+      );
+    }
+    if (query.hasDebtCharacteristic() != null) {
+      if (Boolean.TRUE.equals(query.hasDebtCharacteristic())) {
+        mainFilter.must(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY));
+      } else {
+        mainFilter.mustNot(FilterBuilders.existsFilter(RuleDocument.FIELD_CHARACTERISTIC_KEY));
+      }
+    }
+    return mainFilter;
+  }
+
   private static void addMustTermOrTerms(BoolFilterBuilder filter, String field, Collection<String> terms) {
     FilterBuilder termOrTerms = getTermOrTerms(field, terms);
     if (termOrTerms != null) {

diff --git a/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java b/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java
index a7153b0db2ad0b1fd6531a7aa86085e60f4b5efc..88aab6ced84e4ce1257489289e5fe51e632a4e50 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java
+++ b/sonar-server/src/main/java/org/sonar/server/startup/CopyRequirementsFromCharacteristicsToRules.java
@@ -31,6 +31,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.platform.ServerUpgradeStatus;
 import org.sonar.api.rules.Rule;
+import org.sonar.api.server.debt.DebtRemediationFunction;
 import org.sonar.api.utils.Duration;
 import org.sonar.api.utils.System2;
 import org.sonar.core.persistence.Database;
@@ -173,50 +174,67 @@ public class CopyRequirementsFromCharacteristicsToRules {
     }
 
     private boolean convert(RuleRow ruleRow, PreparedStatement updateStatement, Collection<RequirementDto> requirementsForRule) throws SQLException {
-      RequirementDto enabledRequirement = Iterables.find(requirementsForRule, new Predicate<RequirementDto>() {
-        @Override
-        public boolean apply(RequirementDto input) {
-          return input.isEnabled();
-        }
-      }, null);
+      RequirementDto enabledRequirement = enabledRequirement(requirementsForRule);
 
       if (enabledRequirement == null && !Rule.STATUS_REMOVED.equals(ruleRow.getStatus())) {
         // If no enabled requirement is found, it means that the requirement has been disabled for this rule
-        updateStatement.setInt(1, RuleDto.DISABLED_CHARACTERISTIC_ID);
-        updateStatement.setNull(2, Types.VARCHAR);
-        updateStatement.setNull(3, Types.VARCHAR);
-        updateStatement.setNull(4, Types.VARCHAR);
-        updateStatement.setTimestamp(5, new Timestamp(system2.now()));
-        updateStatement.setInt(6, ruleRow.getId());
-        return true;
+        return convertDisableRequirement(ruleRow, updateStatement);
 
       } else if (enabledRequirement != null) {
         // If one requirement is enable, it means either that this requirement has been set from SQALE, or that it come from a XML model definition
+        return convertEnabledRequirement(ruleRow, updateStatement, enabledRequirement);
 
-        ruleRow.setCharacteristicId(enabledRequirement.getParentId());
-        ruleRow.setFunction(enabledRequirement.getFunction().toUpperCase());
-        ruleRow.setCoefficient(convertDuration(enabledRequirement.getCoefficientValue(), enabledRequirement.getCoefficientUnit()));
-        ruleRow.setOffset(convertDuration(enabledRequirement.getOffsetValue(), enabledRequirement.getOffsetUnit()));
-
-        // If the coefficient of a linear or linear with offset function is null, it should be replaced by 0
-        if (("LINEAR".equals(ruleRow.getFunction()) || "LINEAR_OFFSET".equals(ruleRow.getFunction())) && ruleRow.getCoefficient() == null) {
-          ruleRow.setCoefficient("0" + convertUnit(enabledRequirement.getCoefficientUnit()));
-          // If the offset of a constant per issue or linear with offset function is null, it should be replaced by 0
-        } else if (("CONSTANT_ISSUE".equals(ruleRow.getFunction()) || "LINEAR_OFFSET".equals(ruleRow.getFunction())) && ruleRow.getOffset() == null) {
-          ruleRow.setOffset("0" + convertUnit(enabledRequirement.getOffsetUnit()));
-        }
+        // When default values on debt are the same that ones set by SQALE, nothing to do
+      }
+      return false;
+    }
 
-        if (!isDebtDefaultValuesSameAsOverriddenValues(ruleRow)) {
-          // Default values on debt are not the same that ones set by SQALE, update the rule
-          updateStatement.setInt(1, ruleRow.getCharacteristicId());
-          updateStatement.setString(2, ruleRow.getFunction());
-          updateStatement.setString(3, ruleRow.getCoefficient());
-          updateStatement.setString(4, ruleRow.getOffset());
-          updateStatement.setTimestamp(5, new Timestamp(system2.now()));
-          updateStatement.setInt(6, ruleRow.getId());
-          return true;
+    private static RequirementDto enabledRequirement(Collection<RequirementDto> requirementsForRule) {
+      return Iterables.find(requirementsForRule, new Predicate<RequirementDto>() {
+        @Override
+        public boolean apply(RequirementDto input) {
+          return input.isEnabled();
         }
-        // When default values on debt are the same that ones set by SQALE, nothing to do
+      }, null);
+    }
+
+    private boolean convertDisableRequirement(RuleRow ruleRow, PreparedStatement updateStatement) throws SQLException {
+      updateStatement.setInt(1, RuleDto.DISABLED_CHARACTERISTIC_ID);
+      updateStatement.setNull(2, Types.VARCHAR);
+      updateStatement.setNull(3, Types.VARCHAR);
+      updateStatement.setNull(4, Types.VARCHAR);
+      updateStatement.setTimestamp(5, new Timestamp(system2.now()));
+      updateStatement.setInt(6, ruleRow.getId());
+      return true;
+    }
+
+    private boolean convertEnabledRequirement(RuleRow ruleRow, PreparedStatement updateStatement, RequirementDto enabledRequirement) throws SQLException {
+      ruleRow.setCharacteristicId(enabledRequirement.getParentId());
+      ruleRow.setFunction(enabledRequirement.getFunction().toUpperCase());
+      ruleRow.setCoefficient(convertDuration(enabledRequirement.getCoefficientValue(), enabledRequirement.getCoefficientUnit()));
+      ruleRow.setOffset(convertDuration(enabledRequirement.getOffsetValue(), enabledRequirement.getOffsetUnit()));
+
+      // If the coefficient of a linear or linear with offset function is null, it should be replaced by 0
+      if ((DebtRemediationFunction.Type.LINEAR.name().equals(ruleRow.getFunction()) ||
+        DebtRemediationFunction.Type.LINEAR_OFFSET.name().equals(ruleRow.getFunction()))
+        && ruleRow.getCoefficient() == null) {
+        ruleRow.setCoefficient("0" + convertUnit(enabledRequirement.getCoefficientUnit()));
+        // If the offset of a constant per issue or linear with offset function is null, it should be replaced by 0
+      } else if ((DebtRemediationFunction.Type.CONSTANT_ISSUE.name().equals(ruleRow.getFunction())
+        || DebtRemediationFunction.Type.LINEAR_OFFSET.name().equals(ruleRow.getFunction()))
+        && ruleRow.getOffset() == null) {
+        ruleRow.setOffset("0" + convertUnit(enabledRequirement.getOffsetUnit()));
+      }
+
+      if (!isDebtDefaultValuesSameAsOverriddenValues(ruleRow)) {
+        // Default values on debt are not the same that ones set by SQALE, update the rule
+        updateStatement.setInt(1, ruleRow.getCharacteristicId());
+        updateStatement.setString(2, ruleRow.getFunction());
+        updateStatement.setString(3, ruleRow.getCoefficient());
+        updateStatement.setString(4, ruleRow.getOffset());
+        updateStatement.setTimestamp(5, new Timestamp(system2.now()));
+        updateStatement.setInt(6, ruleRow.getId());
+        return true;
       }
       return false;
     }

diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java
index 0c6fb636888031de42f81837ccf147e4e85c768b..39cf65daaf2595b8f70bf27d841f5d0210cb3f8a 100644 (file)
--- a/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/rule/RuleOperationsTest.java
@@ -533,7 +533,7 @@ public class RuleOperationsTest {
   }
 
   @Test
-  public void disable_characteristic_and_remove_remediation_function_when_update_rule_with_no_sub_characteristic() throws Exception {
+  public void disable_rule_debt_when_update_rule_with_no_sub_characteristic() throws Exception {
     RuleDto dto = new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck")
       .setDefaultSubCharacteristicId(6).setDefaultRemediationFunction("LINEAR").setDefaultRemediationCoefficient("10min")
       .setSubCharacteristicId(6).setRemediationFunction("CONSTANT_ISSUE").setRemediationOffset("10min");
@@ -557,6 +557,36 @@ public class RuleOperationsTest {
     assertThat(result.getUpdatedAt()).isEqualTo(now);
   }
 
+  @Test
+  public void disable_rule_debt_when_update_rule_with_no_function() throws Exception {
+    RuleDto dto = new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck")
+      .setDefaultSubCharacteristicId(6).setDefaultRemediationFunction("CONSTANT_ISSUE").setDefaultRemediationOffset("10min");
+    RuleKey ruleKey = RuleKey.of("squid", "UselessImportCheck");
+
+    when(ruleDao.selectByKey(ruleKey, session)).thenReturn(dto);
+
+    CharacteristicDto subCharacteristic = new CharacteristicDto().setId(2).setKey("COMPILER").setName("Compiler").setParentId(1);
+    when(characteristicDao.selectByKey("COMPILER", session)).thenReturn(subCharacteristic);
+    when(characteristicDao.selectById(2, session)).thenReturn(subCharacteristic);
+    CharacteristicDto characteristic = new CharacteristicDto().setId(1).setKey("PORTABILITY").setName("Portability").setOrder(2);
+    when(characteristicDao.selectById(1, session)).thenReturn(characteristic);
+
+    operations.updateRule(new RuleChange().setRuleKey(ruleKey).setDebtCharacteristicKey("COMPILER"), authorizedUserSession);
+
+    verify(ruleDao).update(ruleCaptor.capture(), eq(session));
+    verify(ruleRegistry).reindex(eq(ruleCaptor.getValue()), eq(session));
+    verify(session).commit();
+
+    RuleDto result = ruleCaptor.getValue();
+
+    assertThat(result.getId()).isEqualTo(1);
+    assertThat(result.getSubCharacteristicId()).isEqualTo(-1);
+    assertThat(result.getRemediationFunction()).isNull();
+    assertThat(result.getRemediationCoefficient()).isNull();
+    assertThat(result.getRemediationOffset()).isNull();
+    assertThat(result.getUpdatedAt()).isEqualTo(now);
+  }
+
   @Test
   public void disable_characteristic_on_rule_having_no_debt_info() throws Exception {
     RuleDto dto = new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck");