From d2cd57b629123d02f9334f0642502f5d5a800d60 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 3 Apr 2014 09:19:03 +0200 Subject: [PATCH] SONAR-5056 When updating a rule, set characteristic and remediation function only if different from default values --- .../sonar/server/debt/DebtModelBackup.java | 17 ++-- .../server/debt/DebtModelOperations.java | 2 + .../org/sonar/server/rule/RuleOperations.java | 19 +++-- .../server/debt/DebtModelBackupTest.java | 59 ++++++++++++++ .../sonar/server/rule/RuleOperationsTest.java | 80 ++++++++++++++++++- 5 files changed, 156 insertions(+), 21 deletions(-) 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 4561f6a5c48..108a8402391 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 @@ -156,13 +156,13 @@ public class DebtModelBackup implements ServerComponent { List 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 allCharacteristicDtos, List rules, List 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 ruleDebts) { + private static RuleDebt ruleDebt(final String repo, final String key, List ruleDebts) { if (ruleDebts.isEmpty()) { return null; } return Iterables.find(ruleDebts, new Predicate() { @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 rules) { + private static RulesDefinition.Rule ruleDef(final String repo, final String key, List rules) { return Iterables.find(rules, new Predicate() { @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 fb30b883097..31451331074 100644 --- 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 5708ef4a757..39e35d468c7 100644 --- 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 50a2915876c..d88a15493ba 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 @@ -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 85b660c989e..fa73db2fec1 100644 --- 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 -- 2.39.5