From 9ea3c24c6b88e31130fe67a2566d760a5a71303e Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 13 Jun 2014 15:55:00 +0200 Subject: [PATCH] SONAR-5376 Custom rules should not be updated on each server startup if there's no modification --- .../org/sonar/core/rule/RuleMapper.xml | 1 - .../org/sonar/server/rule/RegisterRules.java | 52 ++++-- .../server/rule/RegisterRulesMediumTest.java | 151 ++++++++++++++++-- 3 files changed, 175 insertions(+), 29 deletions(-) diff --git a/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml b/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml index 7d6ef5dbc1b..7fba3960e6e 100644 --- a/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml +++ b/sonar-core/src/main/resources/org/sonar/core/rule/RuleMapper.xml @@ -170,7 +170,6 @@ UPDATE rules_parameters SET - name=#{name}, param_type=#{type}, default_value=#{defaultValue}, description=#{description} 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 5e759e1f974..10e4265e6fd 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java @@ -43,12 +43,8 @@ import org.sonar.server.db.DbClient; import org.sonar.server.qualityprofile.RuleActivator; import javax.annotation.Nullable; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; + +import java.util.*; import static com.google.common.collect.Lists.newArrayList; @@ -329,14 +325,9 @@ public class RegisterRules implements Startable { if (ruleDto.getTemplateId() != null) { RuleDto template = dbClient.ruleDao().getTemplate(ruleDto, session); if (template != null && RuleStatus.REMOVED != template.getStatus()) { - ruleDto.setLanguage(template.getLanguage()); - ruleDto.setStatus(template.getStatus()); - ruleDto.setDefaultSubCharacteristicId(template.getDefaultSubCharacteristicId()); - ruleDto.setDefaultRemediationFunction(template.getDefaultRemediationFunction()); - ruleDto.setDefaultRemediationCoefficient(template.getDefaultRemediationCoefficient()); - ruleDto.setDefaultRemediationOffset(template.getDefaultRemediationOffset()); - ruleDto.setEffortToFixDescription(template.getEffortToFixDescription()); - dbClient.ruleDao().update(session, ruleDto); + if (updateCustomRuleFromTemplateRule(ruleDto, template)){ + dbClient.ruleDao().update(session, ruleDto); + } toBeRemoved = false; } } @@ -359,6 +350,39 @@ public class RegisterRules implements Startable { return removedRules; } + private static boolean updateCustomRuleFromTemplateRule(RuleDto customRule, RuleDto templateRule) { + boolean changed = false; + if (!StringUtils.equals(customRule.getLanguage(), templateRule.getLanguage())) { + customRule.setLanguage(templateRule.getLanguage()); + changed = true; + } + if (!StringUtils.equals(customRule.getConfigKey(), templateRule.getConfigKey())) { + customRule.setConfigKey(templateRule.getConfigKey()); + changed = true; + } + if (!ObjectUtils.equals(customRule.getDefaultSubCharacteristicId(), templateRule.getDefaultSubCharacteristicId())) { + customRule.setDefaultSubCharacteristicId(templateRule.getDefaultSubCharacteristicId()); + changed = true; + } + if (!StringUtils.equals(customRule.getDefaultRemediationFunction(), templateRule.getDefaultRemediationFunction())) { + customRule.setDefaultRemediationFunction(templateRule.getDefaultRemediationFunction()); + changed = true; + } + if (!StringUtils.equals(customRule.getDefaultRemediationCoefficient(), templateRule.getDefaultRemediationCoefficient())) { + customRule.setDefaultRemediationCoefficient(templateRule.getDefaultRemediationCoefficient()); + changed = true; + } + if (!StringUtils.equals(customRule.getDefaultRemediationOffset(), templateRule.getDefaultRemediationOffset())) { + customRule.setDefaultRemediationOffset(templateRule.getDefaultRemediationOffset()); + changed = true; + } + if (!StringUtils.equals(customRule.getEffortToFixDescription(), templateRule.getEffortToFixDescription())) { + customRule.setEffortToFixDescription(templateRule.getEffortToFixDescription()); + changed = true; + } + return changed; + } + /** * SONAR-4642 *

diff --git a/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java b/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java index 7fbf4d129aa..1ef6b268824 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java @@ -20,6 +20,7 @@ package org.sonar.server.rule; +import com.google.common.collect.ImmutableMap; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; @@ -47,6 +48,7 @@ import org.sonar.server.search.QueryOptions; import org.sonar.server.tester.ServerTester; import org.sonar.server.user.MockUserSession; +import java.util.Date; import java.util.List; import static org.fest.assertions.Assertions.assertThat; @@ -66,6 +68,7 @@ public class RegisterRulesMediumTest { tester.clearDbAndIndexes(); rulesDefinition.includeX1 = true; rulesDefinition.includeX2 = true; + rulesDefinition.includeTemplate1 = true; tester.get(Platform.class).executeStartupTasks(); db = tester.get(DbClient.class); dbSession = tester.get(DbClient.class).openSession(false); @@ -79,13 +82,13 @@ public class RegisterRulesMediumTest { @Test public void register_rules_at_startup() throws Exception { - verifyTwoRulesInDb(); + verifyRulesInDb(); RuleIndex index = tester.get(RuleIndex.class); RuleResult searchResult = index.search(new RuleQuery(), new QueryOptions()); - assertThat(searchResult.getTotal()).isEqualTo(2); - assertThat(searchResult.getHits()).hasSize(2); + assertThat(searchResult.getTotal()).isEqualTo(3); + assertThat(searchResult.getHits()).hasSize(3); } /** @@ -99,11 +102,11 @@ public class RegisterRulesMediumTest { public void index_even_if_no_changes() throws Exception { RuleIndex index = tester.get(RuleIndex.class); - verifyTwoRulesInDb(); + verifyRulesInDb(); // clear ES but keep db tester.clearIndexes(); - verifyTwoRulesInDb(); + verifyRulesInDb(); RuleResult searchResult = index.search(new RuleQuery(), new QueryOptions()); assertThat(searchResult.getTotal()).isEqualTo(0); assertThat(searchResult.getHits()).hasSize(0); @@ -113,7 +116,7 @@ public class RegisterRulesMediumTest { index = tester.get(RuleIndex.class); - verifyTwoRulesInDb(); + verifyRulesInDb(); searchResult = index.search(new RuleQuery().setKey("xoo:x1"), new QueryOptions()); assertThat(searchResult.getTotal()).isEqualTo(1); assertThat(searchResult.getHits()).hasSize(1); @@ -122,12 +125,12 @@ public class RegisterRulesMediumTest { @Test public void mark_rule_as_removed() throws Exception { - verifyTwoRulesInDb(); + verifyRulesInDb(); rulesDefinition.includeX2 = false; tester.get(Platform.class).executeStartupTasks(); - verifyTwoRulesInDb(); + verifyRulesInDb(); RuleDto rule = db.ruleDao().getByKey(dbSession, RuleKey.of("xoo", "x2")); assertThat(rule.getStatus()).isEqualTo(RuleStatus.REMOVED); } @@ -170,9 +173,10 @@ public class RegisterRulesMediumTest { tester.get(QProfileService.class).activate(activation); dbSession.clearCache(); - // restart without x1 and x2 -> keep active rule of x1 + // restart without x1, x2, template1 -> keep active rule of x1 rulesDefinition.includeX1 = false; rulesDefinition.includeX2 = false; + rulesDefinition.includeTemplate1 = false; tester.get(Platform.class).executeStartupTasks(); dbSession.clearCache(); assertThat(db.ruleDao().getByKey(dbSession, RuleKey.of("xoo", "x1")).getStatus()).isEqualTo(RuleStatus.REMOVED); @@ -180,20 +184,126 @@ public class RegisterRulesMediumTest { assertThat(db.activeRuleDao().findByProfileKey(dbSession, profileKey)).hasSize(1); } - private void verifyTwoRulesInDb() { + @Test + public void update_custom_rule_from_template() throws Exception { + RuleIndex index = tester.get(RuleIndex.class); + Rule templateRule = index.getByKey(RuleKey.of("xoo", "template1")); + + // Create custom rule + RuleKey customRuleKey = tester.get(RuleCreator.class).create(new NewRule() + .setRuleKey("CUSTOM_RULE") + .setTemplateKey(templateRule.key()) + .setName("My custom") + .setHtmlDescription("Some description") + .setSeverity(Severity.MAJOR) + .setStatus(RuleStatus.READY) + .setParameters(ImmutableMap.of("format", "txt"))); + + // Update custom rule + RuleDto customRuleDto = db.ruleDao().getByKey(dbSession, customRuleKey); + db.ruleDao().update(dbSession, customRuleDto + .setLanguage("other language") + .setConfigKey("other config key") + .setDefaultSubCharacteristicId(45) + .setDefaultRemediationFunction("LINEAR_OFFSET") + .setDefaultRemediationCoefficient("1h") + .setDefaultRemediationOffset("5min") + .setEffortToFixDescription("effort to fix desc") + ); + dbSession.commit(); + dbSession.clearCache(); + + // Re-execute startup tasks + tester.get(Platform.class).executeStartupTasks(); + + // Verify custom rule has been restore from the template + Rule customRule = index.getByKey(customRuleKey); + assertThat(customRule.language()).isEqualTo("xoo"); + assertThat(customRule.internalKey()).isNull(); + assertThat(customRule.debtSubCharacteristicKey()).isNull(); + assertThat(customRule.debtRemediationFunction()).isNull(); + } + + @Test + public void not_update_custom_rule_from_template_if_no_change() throws Exception { + RuleIndex index = tester.get(RuleIndex.class); + Rule templateRule = index.getByKey(RuleKey.of("xoo", "template1")); + + // Create custom rule + RuleKey customRuleKey = tester.get(RuleCreator.class).create(new NewRule() + .setRuleKey("CUSTOM_RULE") + .setTemplateKey(templateRule.key()) + .setName("My custom") + .setHtmlDescription("Some description") + .setSeverity(Severity.MAJOR) + .setStatus(RuleStatus.READY) + .setParameters(ImmutableMap.of("format", "txt"))); + dbSession.commit(); + dbSession.clearCache(); + + // Store updated at date + Date updatedAt = index.getByKey(customRuleKey).updatedAt(); + + // Re-execute startup tasks + tester.get(Platform.class).executeStartupTasks(); + + // Verify custom rule has not been updated + Rule customRuleReloaded = index.getByKey(customRuleKey); + assertThat(customRuleReloaded.updatedAt()).isEqualTo(updatedAt); + } + + @Test + public void not_update_custom_rule_params_from_template() throws Exception { + RuleIndex index = tester.get(RuleIndex.class); + Rule templateRule = index.getByKey(RuleKey.of("xoo", "template1")); + + // Create custom rule + RuleKey customRuleKey = tester.get(RuleCreator.class).create(new NewRule() + .setRuleKey("CUSTOM_RULE") + .setTemplateKey(templateRule.key()) + .setName("My custom") + .setHtmlDescription("Some description") + .setSeverity(Severity.MAJOR) + .setStatus(RuleStatus.READY) + .setParameters(ImmutableMap.of("format", "txt"))); + dbSession.commit(); + dbSession.clearCache(); + + // Update custom rule param name + RuleDto customRuleDto = db.ruleDao().getByKey(dbSession, customRuleKey); + RuleParamDto customRuleParamDto = db.ruleDao().findRuleParamsByRuleKey(dbSession, customRuleKey).get(0); + db.ruleDao().removeRuleParam(dbSession, customRuleDto, customRuleParamDto); + db.ruleDao().addRuleParam(dbSession, customRuleDto, customRuleParamDto.setName("format2")); + dbSession.commit(); + dbSession.clearCache(); + + // Verify param has been updated + Rule customRule = index.getByKey(customRuleKey); + assertThat(customRule.params()).hasSize(1); + assertThat(customRule.params().get(0).key()).isEqualTo("format2"); + + // Re-execute startup tasks + tester.get(Platform.class).executeStartupTasks(); + + // Verify custom rule param has not been changed! + Rule customRuleReloaded = index.getByKey(customRuleKey); + assertThat(customRuleReloaded.params().get(0).key()).isEqualTo("format2"); + } + + private void verifyRulesInDb() { List rules = db.ruleDao().findAll(dbSession); - assertThat(rules).hasSize(2); + assertThat(rules).hasSize(3); List ruleParams = db.ruleDao().findAllRuleParams(dbSession); - assertThat(ruleParams).hasSize(1); + assertThat(ruleParams).hasSize(2); } public static class XooRulesDefinition implements RulesDefinition { - boolean includeX1 = true, includeX2 = true; + boolean includeX1 = true, includeX2 = true, includeTemplate1 = true; @Override public void define(Context context) { - if (includeX1 || includeX2) { + if (includeX1 || includeX2 || includeTemplate1) { NewRepository repository = context.createRepository("xoo", "xoo").setName("Xoo Repo"); if (includeX1) { repository.createRule("x1") @@ -212,6 +322,19 @@ public class RegisterRulesMediumTest { .setHtmlDescription("x2 desc") .setSeverity(Severity.MAJOR); } + + if (includeTemplate1) { + repository.createRule("template1") + .setName("template1 name") + .setHtmlDescription("template1 desc") + .setSeverity(Severity.MAJOR) + .setTemplate(true) + .createParam("format") + .setDefaultValue("csv") + .setType(RuleParamType.STRING) + .setDescription("format parameter"); + } + repository.done(); } } -- 2.39.5