From 866594401d96c33a59877666a9ec9d03128ab644 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 4 Apr 2014 10:24:38 +0200 Subject: [PATCH] SONAR-5056 Fix issue when exporting model to XML --- .../DefaultDebtRemediationFunction.java | 6 +- .../sonar/server/debt/DebtModelBackup.java | 42 +++++++----- .../app/controllers/application_controller.rb | 6 ++ .../server/debt/DebtModelBackupTest.java | 67 ++++++++++++++++++- 4 files changed, 100 insertions(+), 21 deletions(-) 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 cc8d58b2002..66c8e337f0c 100644 --- 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 0979ad5d54f..abe57f2dc80 100644 --- 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 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 1ed18c8df2a..0de63eeb860 100644 --- 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 cf14a6bf46b..dbf476b1766 100644 --- 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 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 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("").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("").getErrors()).hasSize(1); + + verify(ruleDao).selectEnablesAndNonManual(session); + verifyZeroInteractions(ruleRegistry); + verify(session, never()).commit(); + } + } -- 2.39.5