SONAR-5056 Fix issue when exporting model to XML

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/server/debt/internal/DefaultDebtRemediationFunction.java b/sonar-plugin-api/src/main/java/org/sonar/api/server/debt/internal/DefaultDebtRemediationFunction.java
index cc8d58b2002b2a4252392bcca24be58502eecf24..66c8e337f0cf823c9b256f9d2728bb74a6a17fe0 100644 (file)
--- a/sonar-plugin-api/src/main/java/org/sonar/api/server/debt/internal/DefaultDebtRemediationFunction.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/server/debt/internal/DefaultDebtRemediationFunction.java
@@ -38,9 +38,6 @@ public class DefaultDebtRemediationFunction implements DebtRemediationFunction {
   private final String offset;
 
   public DefaultDebtRemediationFunction(@Nullable Type type, @Nullable String coefficient, @Nullable String offset) {
-    if (type == null) {
-      throw new IllegalArgumentException("Remediation function type cannot be null");
-    }
     this.type = type;
     this.coefficient = sanitizeValue("coefficient", coefficient);
     this.offset = sanitizeValue("offset", offset);
@@ -78,6 +75,9 @@ public class DefaultDebtRemediationFunction implements DebtRemediationFunction {
   }
 
   private void validate() {
+    if (type == null) {
+      throw new IllegalArgumentException("Remediation function type cannot be null");
+    }
     switch (type) {
       case LINEAR:
         if (this.coefficient == null || this.offset != null) {

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 0979ad5d54faf81a82c41722bc3c60ec79dbc7d5..abe57f2dc8054d5eb7ef35157ff5566c49e3dc4c 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
@@ -123,10 +123,9 @@ public class DebtModelBackup implements ServerComponent {
       List<RuleDebt> rules = newArrayList();
       for (RuleDto rule : ruleDao.selectEnablesAndNonManual(session)) {
         if (languageKey == null || languageKey.equals(rule.getLanguage())) {
-          Integer effectiveSubCharacteristicId = rule.getSubCharacteristicId() != null ? rule.getSubCharacteristicId() : rule.getDefaultSubCharacteristicId();
-          String effectiveFunction = rule.getRemediationFunction() != null ? rule.getRemediationFunction() : rule.getDefaultRemediationFunction();
-          if (!RuleDto.DISABLED_CHARACTERISTIC_ID.equals(effectiveSubCharacteristicId) && effectiveSubCharacteristicId != null && effectiveFunction != null) {
-            rules.add(toRuleDebt(rule, debtModel.characteristicById(effectiveSubCharacteristicId).key(), effectiveFunction));
+          RuleDebt ruleDebt = toRuleDebt(rule, debtModel);
+          if (ruleDebt != null) {
+            rules.add(ruleDebt);
           }
         }
       }
@@ -213,6 +212,8 @@ public class DebtModelBackup implements ServerComponent {
       restoreRules(allCharacteristicDtos, rules(languageKey, session), rulesXMLImporter.importXML(xml, validationMessages), validationMessages, updateDate, session);
 
       session.commit();
+    } catch (IllegalArgumentException e) {
+      validationMessages.addErrorText(e.getMessage());
     } finally {
       MyBatis.closeQuietly(session);
     }
@@ -356,18 +357,27 @@ public class DebtModelBackup implements ServerComponent {
     }));
   }
 
-  private static RuleDebt toRuleDebt(RuleDto rule, String subCharacteristicKey, String function) {
-    RuleDebt ruleDebt = new RuleDebt().setRuleKey(RuleKey.of(rule.getRepositoryKey(), rule.getRuleKey())).setSubCharacteristicKey(subCharacteristicKey);
-
-    String coefficient = rule.getRemediationCoefficient();
-    String offset = rule.getRemediationOffset();
-    String effectiveCoefficient = coefficient != null ? coefficient : rule.getDefaultRemediationCoefficient();
-    String effectiveOffset = offset != null ? offset : rule.getDefaultRemediationOffset();
-
-    ruleDebt.setFunction(function);
-    ruleDebt.setCoefficient(effectiveCoefficient);
-    ruleDebt.setOffset(effectiveOffset);
-    return ruleDebt;
+  @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;
+    if (subCharacteristic != null) {
+      ruleDebt.setSubCharacteristicKey(subCharacteristic.key());
+      if (rule.getRemediationFunction() != null) {
+        ruleDebt.setFunction(rule.getRemediationFunction());
+        ruleDebt.setCoefficient(rule.getRemediationCoefficient());
+        ruleDebt.setOffset(rule.getRemediationOffset());
+        return ruleDebt;
+      } else if (rule.getDefaultRemediationFunction() != null) {
+        ruleDebt.setFunction(rule.getDefaultRemediationFunction());
+        ruleDebt.setCoefficient(rule.getDefaultRemediationCoefficient());
+        ruleDebt.setOffset(rule.getDefaultRemediationOffset());
+        return ruleDebt;
+      }
+    }
+    return null;
   }
 
   private static CharacteristicDto toDto(DebtCharacteristic characteristic, @Nullable Integer parentId) {

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
index 1ed18c8df2a6f3981fe1c5b84280404123a11ab3..0de63eeb860de6ff60c09e65204c10a79cdc0139 100644 (file)
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/application_controller.rb
@@ -224,6 +224,12 @@ class ApplicationController < ActionController::Base
       else
         flash[:error] = java_error_message(exception)
       end
+    rescue Java::JavaLang::IllegalArgumentException => exception
+      if request.xhr?
+        raise exception
+      else
+        flash[:error] = java_error_message(exception)
+      end
     end
   end
 

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 cf14a6bf46b7a92c6bf0dd30ccbb3abeb6ee193b..dbf476b1766cbe81ba55618a86dc0fc06cc7d877 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
@@ -212,6 +212,40 @@ public class DebtModelBackupTest {
     assertThat(ruleDebtListCaptor.getValue()).isEmpty();
   }
 
+  @Test
+  public void backup_with_rule_having_default_linear_and_overridden_offset() throws Exception {
+    when(dao.selectEnabledCharacteristics(session)).thenReturn(newArrayList(
+      new CharacteristicDto().setId(1).setKey("PORTABILITY").setName("Portability updated").setOrder(2),
+      new CharacteristicDto().setId(2).setKey("COMPILER").setName("Compiler updated").setParentId(1)
+    ));
+
+    when(ruleDao.selectEnablesAndNonManual(session)).thenReturn(newArrayList(
+      // Rule with default debt values : default value is linear (only coefficient is set) and overridden value is constant per issue (only offset is set)
+      // -> Ony offset should be set
+      new RuleDto().setRepositoryKey("squid").setRuleKey("AvoidNPE")
+        .setDefaultSubCharacteristicId(2).setDefaultRemediationFunction("LINEAR").setDefaultRemediationCoefficient("2h")
+        .setSubCharacteristicId(2).setRemediationFunction("CONSTANT_ISSUE").setRemediationOffset("15min")
+    ));
+
+    debtModelBackup.backup();
+
+    ArgumentCaptor<DebtModel> debtModelArgument = ArgumentCaptor.forClass(DebtModel.class);
+    verify(debtModelXMLExporter).export(debtModelArgument.capture(), ruleDebtListCaptor.capture());
+    assertThat(debtModelArgument.getValue().rootCharacteristics()).hasSize(1);
+    assertThat(debtModelArgument.getValue().subCharacteristics("PORTABILITY")).hasSize(1);
+
+    List<RuleDebt> rules = ruleDebtListCaptor.getValue();
+    assertThat(rules).hasSize(1);
+
+    RuleDebt rule = rules.get(0);
+    assertThat(rule.ruleKey().repository()).isEqualTo("squid");
+    assertThat(rule.ruleKey().rule()).isEqualTo("AvoidNPE");
+    assertThat(rule.subCharacteristicKey()).isEqualTo("COMPILER");
+    assertThat(rule.function()).isEqualTo("CONSTANT_ISSUE");
+    assertThat(rule.offset()).isEqualTo("15min");
+    assertThat(rule.coefficient()).isNull();
+  }
+
   @Test
   public void backup_from_language() throws Exception {
     when(dao.selectEnabledCharacteristics(session)).thenReturn(newArrayList(
@@ -641,7 +675,7 @@ public class DebtModelBackupTest {
   }
 
   @Test
-  public void add_warning_message_when_rule_from_xml_is_not_found() throws Exception {
+  public void restore_from_xml_add_warning_message_when_rule_from_xml_is_not_found() throws Exception {
     when(characteristicsXMLImporter.importXML(anyString())).thenReturn(new DebtModel()
       .addRootCharacteristic(new DefaultDebtCharacteristic().setKey("PORTABILITY").setName("Portability").setOrder(1))
       .addSubCharacteristic(new DefaultDebtCharacteristic().setKey("COMPILER").setName("Compiler"), "PORTABILITY"));
@@ -657,10 +691,39 @@ public class DebtModelBackupTest {
 
     assertThat(debtModelBackup.restoreFromXml("<xml/>").getWarnings()).hasSize(1);
 
+    verifyZeroInteractions(ruleOperations);
+
     verify(ruleDao).selectEnablesAndNonManual(session);
-    verifyNoMoreInteractions(ruleDao);
     verify(ruleRegistry).reindex(ruleCaptor.getAllValues(), session);
     verify(session).commit();
   }
 
+  @Test
+  public void restore_from_xml_add_error_message_when_illegal_argument_exception() throws Exception {
+    when(characteristicsXMLImporter.importXML(anyString())).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").setOrder(1).setCreatedAt(oldDate),
+      new CharacteristicDto().setId(2).setKey("COMPILER").setName("Compiler").setParentId(1).setCreatedAt(oldDate)));
+
+    when(rulesXMLImporter.importXML(anyString(), any(ValidationMessages.class))).thenReturn(newArrayList(new RuleDebt()
+      .setRuleKey(RuleKey.of("squid", "UselessImportCheck")).setSubCharacteristicKey("COMPILER").setFunction(DebtRemediationFunction.Type.LINEAR.name()).setCoefficient("2h")));
+
+    when(ruleDao.selectEnablesAndNonManual(session)).thenReturn(newArrayList(
+      new RuleDto().setId(1).setRepositoryKey("squid").setRuleKey("UselessImportCheck")
+        .setDefaultSubCharacteristicId(3).setDefaultRemediationFunction("LINEAR").setDefaultRemediationCoefficient("2h")
+        .setCreatedAt(oldDate).setUpdatedAt(oldDate)
+    ));
+
+    when(ruleOperations.updateRule(any(RuleDto.class), any(CharacteristicDto.class), anyString(), anyString(), anyString(), any(Date.class), eq(session))).thenThrow(IllegalArgumentException.class);
+
+    assertThat(debtModelBackup.restoreFromXml("<xml/>").getErrors()).hasSize(1);
+
+    verify(ruleDao).selectEnablesAndNonManual(session);
+    verifyZeroInteractions(ruleRegistry);
+    verify(session, never()).commit();
+  }
+
 }