SONAR-5056 When updating a rule, set characteristic and remediation function only...

diff --git a/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java b/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java
index 4561f6a5c48c60da3bbdffd55dd9a4c0de4ec9db..108a8402391b537358e44d40eee51086a0d0b441 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java
+++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtModelBackup.java
@@ -156,13 +156,13 @@ public class DebtModelBackup implements ServerComponent {
       List<RuleDto> ruleDtos = rules(null, session);
       for (RuleDto rule : ruleDtos) {
         // Restore default debt definitions
-        RulesDefinition.Rule ruleDef = ruleDef(rule, rules);
+        RulesDefinition.Rule ruleDef = ruleDef(rule.getRepositoryKey(), rule.getRuleKey(), rules);
         if (ruleDef != null) {
-          // TODO when can it be null ?
           String subCharacteristicKey = ruleDef.debtSubCharacteristic();
           CharacteristicDto subCharacteristicDto = characteristicByKey(subCharacteristicKey, allCharacteristicDtos);
           DebtRemediationFunction remediationFunction = ruleDef.debtRemediationFunction();
           boolean hasDebtDefinition = subCharacteristicDto != null && remediationFunction != null;
+
           rule.setDefaultSubCharacteristicId(hasDebtDefinition ? subCharacteristicDto.getId() : null);
           rule.setDefaultRemediationFunction(hasDebtDefinition ? remediationFunction.type().name() : null);
           rule.setDefaultRemediationCoefficient(hasDebtDefinition ? remediationFunction.coefficient() : null);
@@ -175,6 +175,8 @@ public class DebtModelBackup implements ServerComponent {
         rule.setRemediationCoefficient(null);
         rule.setRemediationOffset(null);
         rule.setUpdatedAt(updateDate);
+
+        // TODO update only if modification ?
         ruleDao.update(rule, session);
       }
       ruleRegistry.reindex(ruleDtos, session);
@@ -218,7 +220,7 @@ public class DebtModelBackup implements ServerComponent {
   private void restoreRules(List<CharacteristicDto> allCharacteristicDtos, List<RuleDto> rules, List<RuleDebt> ruleDebts,
                             ValidationMessages validationMessages, Date updateDate, SqlSession session) {
     for (RuleDto rule : rules) {
-      RuleDebt ruleDebt = ruleDebt(rule, ruleDebts);
+      RuleDebt ruleDebt = ruleDebt(rule.getRepositoryKey(), rule.getRuleKey(), ruleDebts);
       if (ruleDebt == null) {
         // rule does not exists in the XML
         disabledOverriddenRuleDebt(rule);
@@ -307,6 +309,7 @@ public class DebtModelBackup implements ServerComponent {
   }
 
   private void disabledOverriddenRuleDebt(RuleDto rule) {
+    // TODO ?
     rule.setSubCharacteristicId(rule.getDefaultSubCharacteristicId() != null ? RuleDto.DISABLED_CHARACTERISTIC_ID : null);
     rule.setRemediationFunction(null);
     rule.setRemediationCoefficient(null);
@@ -338,24 +341,24 @@ public class DebtModelBackup implements ServerComponent {
   }
 
   @CheckForNull
-  private static RuleDebt ruleDebt(final RuleDto rule, List<RuleDebt> ruleDebts) {
+  private static RuleDebt ruleDebt(final String repo, final String key, List<RuleDebt> ruleDebts) {
     if (ruleDebts.isEmpty()) {
       return null;
     }
     return Iterables.find(ruleDebts, new Predicate<RuleDebt>() {
       @Override
       public boolean apply(@Nullable RuleDebt input) {
-        return input != null && rule.getRepositoryKey().equals(input.ruleKey().repository()) && rule.getRuleKey().equals(input.ruleKey().rule());
+        return input != null && repo.equals(input.ruleKey().repository()) && key.equals(input.ruleKey().rule());
       }
     }, null);
   }
 
   @CheckForNull
-  private static RulesDefinition.Rule ruleDef(final RuleDto rule, List<RulesDefinition.Rule> rules) {
+  private static RulesDefinition.Rule ruleDef(final String repo, final String key, List<RulesDefinition.Rule> rules) {
     return Iterables.find(rules, new Predicate<RulesDefinition.Rule>() {
       @Override
       public boolean apply(@Nullable RulesDefinition.Rule input) {
-        return input != null && rule.getRepositoryKey().equals(input.repository().key()) && rule.getRuleKey().equals(input.key());
+        return input != null && repo.equals(input.repository().key()) && key.equals(input.key());
       }
     }, null);
   }

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 fb30b883097a5808554c61d63bde81eb0437d53a..31451331074c17ea05e8d941bdd6720a0aa1ae0b 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/debt/DebtModelOperations.java
+++ b/sonar-server/src/main/java/org/sonar/server/debt/DebtModelOperations.java
@@ -242,6 +242,8 @@ public class DebtModelOperations implements ServerComponent {
       }
       if (subCharacteristicId.equals(ruleDto.getDefaultSubCharacteristicId())) {
         ruleDto.setDefaultSubCharacteristicId(null);
+
+        // TODO should we really set these fields to null ?
         ruleDto.setDefaultRemediationFunction(null);
         ruleDto.setDefaultRemediationCoefficient(null);
         ruleDto.setDefaultRemediationOffset(null);

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 5708ef4a757cdc574e02dea56b4ffecc04c349bd..39e35d468c79c1461a14f16ba713f471e96e1f76 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
@@ -275,16 +275,15 @@ public class RuleOperations implements ServerComponent {
           throw new NotFoundException(String.format("Unknown sub characteristic '%s'", ruleChange.debtCharacteristicKey()));
         }
 
-        // New sub characteristic is not equals to existing one -> update it
-        // TODO check characteristic is not equals to default one, if so do nothing
-        if (!subCharacteristic.getId().equals(ruleDto.getSubCharacteristicId())) {
+        // New sub characteristic is not equals to existing one and not equals to default value -> update it
+        if (!subCharacteristic.getId().equals(ruleDto.getSubCharacteristicId()) && !subCharacteristic.getId().equals(ruleDto.getDefaultSubCharacteristicId()) ) {
           ruleDto.setSubCharacteristicId(subCharacteristic.getId());
           updated = true;
         }
 
-        // New remediation function is not equals to existing one -> update it
-        // TODO check remediaiton function is not equals to default one, if so do nothing
-        if (!isSameRemediationFunction(ruleChange.debtRemediationFunction(), ruleChange.debtRemediationCoefficient(), ruleChange.debtRemediationOffset(), ruleDto)) {
+        // New remediation function is not equals to existing one and not equals to default value -> update it
+        if (!isSameRemediationFunction(ruleChange, ruleDto.getRemediationFunction(), ruleDto.getRemediationCoefficient(), ruleDto.getRemediationOffset())
+          && !isSameRemediationFunction(ruleChange, ruleDto.getDefaultRemediationFunction(), ruleDto.getDefaultRemediationCoefficient(), ruleDto.getDefaultRemediationOffset())) {
           DefaultDebtRemediationFunction debtRemediationFunction = new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.valueOf(ruleChange.debtRemediationFunction()),
             ruleChange.debtRemediationCoefficient(), ruleChange.debtRemediationOffset());
           ruleDto.setRemediationFunction(debtRemediationFunction.type().name());
@@ -316,11 +315,11 @@ public class RuleOperations implements ServerComponent {
     }
   }
 
-  private static boolean isSameRemediationFunction(String function, @Nullable String coefficient, @Nullable String offset, RuleDto rule) {
+  private static boolean isSameRemediationFunction(RuleChange ruleChange, String function, @Nullable String coefficient, @Nullable String offset) {
     return new EqualsBuilder()
-      .append(function, rule.getRemediationFunction())
-      .append(coefficient, rule.getRemediationCoefficient())
-      .append(offset, rule.getRemediationOffset())
+      .append(function, ruleChange.debtRemediationFunction())
+      .append(coefficient, ruleChange.debtRemediationCoefficient())
+      .append(offset, ruleChange.debtRemediationOffset())
       .isEquals();
   }
 

diff --git a/sonar-server/src/test/java/org/sonar/server/debt/DebtModelBackupTest.java b/sonar-server/src/test/java/org/sonar/server/debt/DebtModelBackupTest.java
index 50a2915876c9a238cc578d8f61b8d0dd7eb41828..d88a15493ba3c5f0ac7d1b4b1e902370e9670e6f 100644 (file)
--- a/sonar-server/src/test/java/org/sonar/server/debt/DebtModelBackupTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/debt/DebtModelBackupTest.java
@@ -465,6 +465,65 @@ public class DebtModelBackupTest {
     assertThat(rule.getUpdatedAt()).isEqualTo(now);
   }
 
+  @Test
+  public void reset_model_for_custom_rules() throws Exception {
+    when(characteristicsXMLImporter.importXML(any(Reader.class))).thenReturn(new DebtModel()
+      .addRootCharacteristic(new DefaultDebtCharacteristic().setKey("PORTABILITY").setName("Portability").setOrder(1))
+      .addSubCharacteristic(new DefaultDebtCharacteristic().setKey("COMPILER").setName("Compiler"), "PORTABILITY"));
+
+    when(dao.selectEnabledCharacteristics(session)).thenReturn(newArrayList(
+      new CharacteristicDto().setId(1).setKey("PORTABILITY").setName("Portability updated").setOrder(2).setCreatedAt(oldDate),
+      new CharacteristicDto().setId(2).setKey("COMPILER").setName("Compiler updated").setParentId(1).setCreatedAt(oldDate)
+    ));
+
+    when(ruleDao.selectEnablesAndNonManual(session)).thenReturn(newArrayList(
+      new RuleDto().setId(10).setRepositoryKey("squid").setRuleKey("Xpath").setCreatedAt(oldDate).setUpdatedAt(oldDate),
+      new RuleDto().setId(11).setRepositoryKey("squid").setRuleKey("XPath_1369910135").setParentId(10)
+        .setDefaultSubCharacteristicId(10).setDefaultRemediationFunction("LINEAR").setDefaultRemediationCoefficient("2h")
+        .setSubCharacteristicId(2).setRemediationFunction("LINEAR_OFFSET").setRemediationCoefficient("2h").setRemediationOffset("15min")
+        .setCreatedAt(oldDate).setUpdatedAt(oldDate)
+    ));
+
+    RulesDefinition.Context context = new RulesDefinition.Context();
+    RulesDefinition.NewRepository repo = context.createRepository("squid", "java").setName("Squid");
+    RulesDefinition.NewRule newRule = repo.createRule("Xpath")
+      .setName("Xpath")
+      .setHtmlDescription("Xpath")
+      .setSeverity(Severity.BLOCKER)
+      .setStatus(RuleStatus.BETA)
+      .setDebtSubCharacteristic("COMPILER");
+    newRule.setDebtRemediationFunction(newRule.debtRemediationFunctions().linearWithOffset("4h", "20min"));
+    repo.done();
+    when(defLoader.load()).thenReturn(context);
+
+    debtModelBackup.reset();
+
+    verify(dao).selectEnabledCharacteristics(session);
+    verify(dao, times(2)).update(any(CharacteristicDto.class), eq(session));
+    verifyNoMoreInteractions(dao);
+
+    verify(ruleDao).selectEnablesAndNonManual(session);
+    verify(ruleDao).update(ruleCaptor.capture(), eq(session));
+    verifyNoMoreInteractions(ruleDao);
+    verify(ruleRegistry).reindex(ruleCaptor.getAllValues(), session);
+
+    verify(session).commit();
+
+    RuleDto rule = ruleCaptor.getValue();
+
+    assertThat(rule.getDefaultSubCharacteristicId()).isEqualTo(2);
+    assertThat(rule.getDefaultRemediationFunction()).isEqualTo("LINEAR_OFFSET");
+    assertThat(rule.getDefaultRemediationCoefficient()).isEqualTo("4h");
+    assertThat(rule.getDefaultRemediationOffset()).isEqualTo("20min");
+    assertThat(rule.getUpdatedAt()).isEqualTo(now);
+
+    assertThat(rule.getSubCharacteristicId()).isNull();
+    assertThat(rule.getRemediationFunction()).isNull();
+    assertThat(rule.getRemediationCoefficient()).isNull();
+    assertThat(rule.getRemediationOffset()).isNull();
+    assertThat(rule.getUpdatedAt()).isEqualTo(now);
+  }
+
   @Test
   public void restore_from_xml_with_different_characteristic_and_same_function() throws Exception {
     when(characteristicsXMLImporter.importXML(anyString())).thenReturn(new DebtModel()

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 85b660c989ec49fc450baaeb8b18c8f0a69ff5b2..fa73db2fec1ba35bd1a592fdea00c7fd170733bc 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
@@ -396,6 +396,9 @@ public class RuleOperationsTest {
     );
 
     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);
@@ -404,9 +407,78 @@ public class RuleOperationsTest {
     assertThat(result.getRemediationCoefficient()).isEqualTo("2h");
     assertThat(result.getRemediationOffset()).isEqualTo("20min");
     assertThat(result.getUpdatedAt()).isEqualTo(now);
+  }
+
+  @Test
+  public void update_rule_set_characteristic_only_if_different_from_default_one() throws Exception {
+    RuleDto dto = new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck")
+      .setDefaultSubCharacteristicId(2).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(
+      // Characteristic is the same as the default one -> Overridden characteristic should not be set
+      // Remediation function is not the same as default one -> Overridden remediation function should be set
+      new RuleChange().setRuleKey(ruleKey).setDebtCharacteristicKey("COMPILER")
+        .setDebtRemediationFunction("LINEAR_OFFSET").setDebtRemediationCoefficient("2h").setDebtRemediationOffset("20min"),
+      authorizedUserSession
+    );
+
+    verify(ruleDao).update(ruleCaptor.capture(), eq(session));
+    verify(ruleRegistry).reindex(eq(ruleCaptor.getValue()), eq(session));
     verify(session).commit();
-    verify(ruleRegistry).reindex(eq(result), eq(session));
+
+    RuleDto result = ruleCaptor.getValue();
+
+    assertThat(result.getId()).isEqualTo(1);
+    assertThat(result.getSubCharacteristicId()).isNull();
+    assertThat(result.getRemediationFunction()).isEqualTo("LINEAR_OFFSET");
+    assertThat(result.getRemediationCoefficient()).isEqualTo("2h");
+    assertThat(result.getRemediationOffset()).isEqualTo("20min");
+    assertThat(result.getUpdatedAt()).isEqualTo(now);
+  }
+
+  @Test
+  public void update_rule_set_remediation_function_only_if_different_from_default_one() 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(
+      // Characteristic is the not same as the default one -> Overridden characteristic should be set
+      // Remediation function is the same as default one -> Overridden remediation function should not be set
+      new RuleChange().setRuleKey(ruleKey).setDebtCharacteristicKey("COMPILER")
+        .setDebtRemediationFunction("CONSTANT_ISSUE").setDebtRemediationOffset("10min"),
+      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(2);
+    assertThat(result.getRemediationFunction()).isNull();
+    assertThat(result.getRemediationCoefficient()).isNull();
+    assertThat(result.getRemediationOffset()).isNull();
+    assertThat(result.getUpdatedAt()).isEqualTo(now);
   }
 
   @Test
@@ -445,6 +517,9 @@ public class RuleOperationsTest {
     );
 
     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);
@@ -453,9 +528,6 @@ public class RuleOperationsTest {
     assertThat(result.getRemediationCoefficient()).isNull();
     assertThat(result.getRemediationOffset()).isNull();
     assertThat(result.getUpdatedAt()).isEqualTo(now);
-
-    verify(session).commit();
-    verify(ruleRegistry).reindex(eq(result), eq(session));
   }
 
   @Test