diff options
author | Simon Brandhof <simon.brandhof@sonarsource.com> | 2014-07-07 21:58:14 +0200 |
---|---|---|
committer | Simon Brandhof <simon.brandhof@sonarsource.com> | 2014-07-07 22:05:55 +0200 |
commit | 4b0f2cd14b2d20d2ed9c214b2110246ede391f2c (patch) | |
tree | d9d8f5116bd5f967ce1f4cfc8eabc517a2fa846d | |
parent | d25d97de634f9591373e9d07d98f9c8b40757398 (diff) | |
download | sonarqube-4b0f2cd14b2d20d2ed9c214b2110246ede391f2c.tar.gz sonarqube-4b0f2cd14b2d20d2ed9c214b2110246ede391f2c.zip |
SONAR-5007 fix bug related to synchronization of rule parameters
* removal of rule parameter must be propagated to active rules
* addition of rule parameter with default value must be propagated to active rules
4 files changed, 121 insertions, 50 deletions
diff --git a/sonar-core/src/main/resources/org/sonar/core/qualityprofile/db/ActiveRuleMapper.xml b/sonar-core/src/main/resources/org/sonar/core/qualityprofile/db/ActiveRuleMapper.xml index fd3c1a00fe2..2f31335b5fd 100644 --- a/sonar-core/src/main/resources/org/sonar/core/qualityprofile/db/ActiveRuleMapper.xml +++ b/sonar-core/src/main/resources/org/sonar/core/qualityprofile/db/ActiveRuleMapper.xml @@ -149,11 +149,11 @@ WHERE id=#{id} </update> - <update id="deleteParameters" parameterType="Integer"> + <update id="deleteParameters" parameterType="int"> DELETE FROM active_rule_parameters WHERE active_rule_id=#{id} </update> - <update id="deleteParameter" parameterType="Integer"> + <update id="deleteParameter" parameterType="int"> DELETE FROM active_rule_parameters WHERE id=#{id} </update> diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/db/ActiveRuleDao.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/db/ActiveRuleDao.java index a79a79a863b..9538f515703 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/db/ActiveRuleDao.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/db/ActiveRuleDao.java @@ -227,4 +227,15 @@ public class ActiveRuleDao extends BaseDao<ActiveRuleMapper, ActiveRuleDto, Acti } return null; } + + public void deleteParamsByRuleParam(DbSession dbSession, RuleDto rule, String paramKey) { + List<ActiveRuleDto> activeRules = findByRule(dbSession, rule); + for (ActiveRuleDto activeRule : activeRules) { + for (ActiveRuleParamDto activeParam : findParamsByActiveRuleKey(dbSession, activeRule.getKey())) { + if (activeParam.getKey().equals(paramKey)) { + deleteParam(dbSession, activeRule, activeParam); + } + } + } + } } 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 6da4f4f4dd7..2fac09447af 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 @@ -23,6 +23,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Maps; import com.google.common.collect.Sets; import org.apache.commons.lang.ObjectUtils; import org.apache.commons.lang.StringUtils; @@ -37,6 +38,8 @@ import org.sonar.api.utils.MessageException; import org.sonar.api.utils.System2; import org.sonar.api.utils.TimeProfiler; import org.sonar.core.persistence.DbSession; +import org.sonar.core.qualityprofile.db.ActiveRuleDto; +import org.sonar.core.qualityprofile.db.ActiveRuleParamDto; import org.sonar.core.rule.RuleDto; import org.sonar.core.rule.RuleParamDto; import org.sonar.core.technicaldebt.db.CharacteristicDao; @@ -290,30 +293,38 @@ public class RegisterRules implements Startable { private void mergeParams(RulesDefinition.Rule ruleDef, RuleDto rule, DbSession session) { List<RuleParamDto> paramDtos = dbClient.ruleDao().findRuleParamsByRuleKey(session, rule.getKey()); - List<String> existingParamDtoNames = new ArrayList<String>(); + Map<String, RuleParamDto> existingParamsByName = Maps.newHashMap(); for (RuleParamDto paramDto : paramDtos) { RulesDefinition.Param paramDef = ruleDef.param(paramDto.getName()); if (paramDef == null) { - // TODO cascade on the activeRule upon RuleDeletion + dbClient.activeRuleDao().deleteParamsByRuleParam(session, rule, paramDto.getName()); dbClient.ruleDao().removeRuleParam(session, rule, paramDto); } else { - // TODO validate that existing active rules still match constraints - // TODO store param name if (mergeParam(paramDto, paramDef)) { dbClient.ruleDao().updateRuleParam(session, rule, paramDto); } - existingParamDtoNames.add(paramDto.getName()); + existingParamsByName.put(paramDto.getName(), paramDto); } } + + // Create newly parameters for (RulesDefinition.Param param : ruleDef.params()) { - if (!existingParamDtoNames.contains(param.key())) { - RuleParamDto paramDto = RuleParamDto.createFor(rule) + RuleParamDto paramDto = existingParamsByName.get(param.key()); + if (paramDto == null) { + paramDto = RuleParamDto.createFor(rule) .setName(param.key()) .setDescription(param.description()) .setDefaultValue(param.defaultValue()) .setType(param.type().toString()); dbClient.ruleDao().addRuleParam(session, rule, paramDto); + if (!StringUtils.isEmpty(param.defaultValue())) { + // Propagate the default value to existing active rules + for (ActiveRuleDto activeRule : dbClient.activeRuleDao().findByRule(session, rule)) { + ActiveRuleParamDto activeParam = ActiveRuleParamDto.createFor(paramDto).setValue(param.defaultValue()); + dbClient.activeRuleDao().addParam(session, activeRule, activeParam); + } + } } } } 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 ad9ac91c5f1..349eb0689f7 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 @@ -35,13 +35,16 @@ import org.sonar.api.server.rule.RulesDefinition; import org.sonar.api.utils.MessageException; import org.sonar.core.permission.GlobalPermissions; import org.sonar.core.persistence.DbSession; +import org.sonar.core.qualityprofile.db.ActiveRuleKey; import org.sonar.core.rule.RuleDto; import org.sonar.core.rule.RuleParamDto; import org.sonar.server.db.DbClient; import org.sonar.server.platform.Platform; +import org.sonar.server.qualityprofile.ActiveRule; import org.sonar.server.qualityprofile.QProfileService; import org.sonar.server.qualityprofile.QProfileTesting; import org.sonar.server.qualityprofile.RuleActivation; +import org.sonar.server.qualityprofile.index.ActiveRuleIndex; import org.sonar.server.rule.index.RuleIndex; import org.sonar.server.rule.index.RuleQuery; import org.sonar.server.search.QueryOptions; @@ -54,6 +57,7 @@ import javax.annotation.Nullable; import java.util.Date; import java.util.List; +import java.util.Map; import static com.google.common.collect.Sets.newHashSet; import static org.fest.assertions.Assertions.assertThat; @@ -66,7 +70,8 @@ public class RegisterRulesMediumTest { @ClassRule public static final ServerTester TESTER = new ServerTester().addComponents(RULE_DEFS); - RuleIndex index; + RuleIndex ruleIndex; + ActiveRuleIndex activeRuleIndex; DbClient db; DbSession dbSession; @@ -76,7 +81,8 @@ public class RegisterRulesMediumTest { db = TESTER.get(DbClient.class); dbSession = TESTER.get(DbClient.class).openSession(false); dbSession.clearCache(); - index = TESTER.get(RuleIndex.class); + ruleIndex = TESTER.get(RuleIndex.class); + activeRuleIndex = TESTER.get(ActiveRuleIndex.class); } @After @@ -95,7 +101,7 @@ public class RegisterRulesMediumTest { db = TESTER.get(DbClient.class); dbSession = TESTER.get(DbClient.class).openSession(false); dbSession.clearCache(); - index = TESTER.get(RuleIndex.class); + ruleIndex = TESTER.get(RuleIndex.class); } @Test @@ -129,10 +135,10 @@ public class RegisterRulesMediumTest { assertThat(ruleParams).hasSize(2); // verify es - Result<Rule> searchResult = index.search(new RuleQuery(), new QueryOptions()); + Result<Rule> searchResult = ruleIndex.search(new RuleQuery(), new QueryOptions()); assertThat(searchResult.getTotal()).isEqualTo(1); assertThat(searchResult.getHits()).hasSize(1); - Rule rule = index.getByKey(RuleKey.of("xoo", "x1")); + Rule rule = ruleIndex.getByKey(RuleKey.of("xoo", "x1")); assertThat(rule.severity()).isEqualTo(Severity.MINOR); assertThat(rule.name()).isEqualTo("x1 name"); assertThat(rule.htmlDescription()).isEqualTo("x1 desc"); @@ -175,8 +181,8 @@ public class RegisterRulesMediumTest { register(rules); // verify that rules are indexed - Result<Rule> searchResult = index.search(new RuleQuery(), new QueryOptions()); - searchResult = index.search(new RuleQuery().setKey("xoo:x1"), new QueryOptions()); + Result<Rule> searchResult = ruleIndex.search(new RuleQuery(), new QueryOptions()); + searchResult = ruleIndex.search(new RuleQuery().setKey("xoo:x1"), new QueryOptions()); assertThat(searchResult.getTotal()).isEqualTo(1); assertThat(searchResult.getHits()).hasSize(1); assertThat(searchResult.getHits().get(0).key()).isEqualTo(RuleKey.of("xoo", "x1")); @@ -226,7 +232,7 @@ public class RegisterRulesMediumTest { } }); - Rule rule = index.getByKey(RuleTesting.XOO_X1); + Rule rule = ruleIndex.getByKey(RuleTesting.XOO_X1); assertThat(rule.severity()).isEqualTo(Severity.INFO); assertThat(rule.name()).isEqualTo("Name2"); assertThat(rule.htmlDescription()).isEqualTo("Desc2"); @@ -256,18 +262,18 @@ public class RegisterRulesMediumTest { register(rules); // Store updated at date - Date updatedAt = index.getByKey(RuleTesting.XOO_X1).updatedAt(); + Date updatedAt = ruleIndex.getByKey(RuleTesting.XOO_X1).updatedAt(); // Re-execute startup tasks register(rules); // Verify rule has not been updated - Rule customRuleReloaded = index.getByKey(RuleTesting.XOO_X1); + Rule customRuleReloaded = ruleIndex.getByKey(RuleTesting.XOO_X1); assertThat(DateUtils.isSameInstant(customRuleReloaded.updatedAt(), updatedAt)); } @Test - public void uninstall_and_reinstall_rules() { + public void disable_then_enable_rules() { Rules rules = new Rules() { @Override public void init(RulesDefinition.NewRepository repository) { @@ -280,14 +286,14 @@ public class RegisterRulesMediumTest { register(null); RuleDto rule = db.ruleDao().getByKey(dbSession, RuleTesting.XOO_X1); assertThat(rule.getStatus()).isEqualTo(RuleStatus.REMOVED); - Rule indexedRule = index.getByKey(RuleTesting.XOO_X1); + Rule indexedRule = ruleIndex.getByKey(RuleTesting.XOO_X1); assertThat(indexedRule.status()).isEqualTo(RuleStatus.REMOVED); // Re-install plugin register(rules); rule = db.ruleDao().getByKey(dbSession, RuleTesting.XOO_X1); assertThat(rule.getStatus()).isEqualTo(RuleStatus.READY); - indexedRule = index.getByKey(RuleTesting.XOO_X1); + indexedRule = ruleIndex.getByKey(RuleTesting.XOO_X1); assertThat(indexedRule.status()).isEqualTo(RuleStatus.READY); } @@ -302,7 +308,6 @@ public class RegisterRulesMediumTest { // Create a profile and activate rule MockUserSession.set().setGlobalPermissions(GlobalPermissions.QUALITY_PROFILE_ADMIN).setLogin("me"); - db.qualityProfileDao().insert(dbSession, QProfileTesting.newXooP1()); dbSession.commit(); dbSession.clearCache(); @@ -316,13 +321,13 @@ public class RegisterRulesMediumTest { repository.createRule("x2").setName("x2 name").setHtmlDescription("x2 desc"); } }); - assertThat(db.ruleDao().getByKey(dbSession, RuleKey.of("xoo", "x1")).getStatus()).isEqualTo(RuleStatus.REMOVED); - assertThat(db.ruleDao().getByKey(dbSession, RuleKey.of("xoo", "x2")).getStatus()).isEqualTo(RuleStatus.READY); - assertThat(db.activeRuleDao().findByProfileKey(dbSession, QProfileTesting.XOO_P1_KEY)).hasSize(0); + assertThat(ruleIndex.getByKey(RuleKey.of("xoo", "x1")).status()).isEqualTo(RuleStatus.REMOVED); + assertThat(ruleIndex.getByKey(RuleKey.of("xoo", "x2")).status()).isEqualTo(RuleStatus.READY); + assertThat(activeRuleIndex.findByProfile(QProfileTesting.XOO_P1_KEY)).hasSize(0); } @Test - public void do_not_deactivate_removed_rules_if_repository_accidentaly_uninstalled() throws Exception { + public void do_not_deactivate_removed_rules_if_repository_accidentally_uninstalled() throws Exception { Rules rules = new Rules() { @Override public void init(RulesDefinition.NewRepository repository) { @@ -341,13 +346,57 @@ public class RegisterRulesMediumTest { // Restart without xoo register(null); - assertThat(db.ruleDao().getByKey(dbSession, RuleTesting.XOO_X1).getStatus()).isEqualTo(RuleStatus.REMOVED); - assertThat(db.activeRuleDao().findByProfileKey(dbSession, QProfileTesting.XOO_P1_KEY)).hasSize(1); + assertThat(ruleIndex.getByKey(RuleTesting.XOO_X1).status()).isEqualTo(RuleStatus.REMOVED); + assertThat(activeRuleIndex.findByProfile(QProfileTesting.XOO_P1_KEY)).hasSize(1); // Re-install register(rules); - assertThat(db.ruleDao().getByKey(dbSession, RuleTesting.XOO_X1).getStatus()).isEqualTo(RuleStatus.READY); - assertThat(db.activeRuleDao().findByProfileKey(dbSession, QProfileTesting.XOO_P1_KEY)).hasSize(1); + assertThat(ruleIndex.getByKey(RuleTesting.XOO_X1).status()).isEqualTo(RuleStatus.READY); + assertThat(activeRuleIndex.findByProfile(QProfileTesting.XOO_P1_KEY)).hasSize(1); + } + + @Test + public void update_active_rules_on_param_changes() throws Exception { + register(new Rules() { + @Override + public void init(RulesDefinition.NewRepository repository) { + RulesDefinition.NewRule x1Rule = repository.createRule("x1").setName("x1 name").setHtmlDescription("x1 desc"); + // has default value + x1Rule.createParam("min").setType(RuleParamType.INTEGER).setDefaultValue("5"); + // no default value + x1Rule.createParam("format").setType(RuleParamType.STRING); + } + }); + + // Create profile and activate rule + MockUserSession.set().setGlobalPermissions(GlobalPermissions.QUALITY_PROFILE_ADMIN).setLogin("me"); + db.qualityProfileDao().insert(dbSession, QProfileTesting.newXooP1()); + dbSession.commit(); + dbSession.clearCache(); + RuleActivation activation = new RuleActivation(RuleTesting.XOO_X1); + activation.setParameter("format", "txt"); + TESTER.get(QProfileService.class).activate(QProfileTesting.XOO_P1_KEY, activation); + + // Default value of "min" is changed, "format" is removed, "format2" is added, "max" is added with a default value + register(new Rules() { + @Override + public void init(RulesDefinition.NewRepository repository) { + RulesDefinition.NewRule x1Rule = repository.createRule("x1").setName("x1 name").setHtmlDescription("x1 desc"); + x1Rule.createParam("min").setType(RuleParamType.INTEGER).setDefaultValue("6"); + x1Rule.createParam("format2").setType(RuleParamType.STRING); + x1Rule.createParam("max").setType(RuleParamType.INTEGER).setDefaultValue("10"); + } + }); + + ActiveRule activeRule = activeRuleIndex.getByKey(ActiveRuleKey.of(QProfileTesting.XOO_P1_KEY, RuleTesting.XOO_X1)); + Map<String, String> params = activeRule.params(); + assertThat(params).hasSize(2); + + // do not change default value on existing active rules -> keep min=5 + assertThat(params.get("min")).isEqualTo("5"); + + // new param with default value + assertThat(params.get("max")).isEqualTo("10"); } @Test @@ -358,14 +407,14 @@ public class RegisterRulesMediumTest { repository.createRule("x1").setName("x1 name").setHtmlDescription("x1 desc").setTags("tag1"); } }); - Rule rule = index.getByKey(RuleTesting.XOO_X1); + Rule rule = ruleIndex.getByKey(RuleTesting.XOO_X1); assertThat(rule.systemTags()).containsOnly("tag1"); assertThat(rule.tags()).isEmpty(); // User adds tag TESTER.get(RuleUpdater.class).update(RuleUpdate.createForPluginRule(RuleTesting.XOO_X1).setTags(newHashSet("tag2")), UserSession.get()); dbSession.clearCache(); - rule = index.getByKey(RuleTesting.XOO_X1); + rule = ruleIndex.getByKey(RuleTesting.XOO_X1); assertThat(rule.systemTags()).containsOnly("tag1"); assertThat(rule.tags()).containsOnly("tag2"); @@ -376,7 +425,7 @@ public class RegisterRulesMediumTest { repository.createRule("x1").setName("x1 name").setHtmlDescription("x1 desc").setTags("tag1", "tag2"); } }); - rule = index.getByKey(RuleTesting.XOO_X1); + rule = ruleIndex.getByKey(RuleTesting.XOO_X1); assertThat(rule.systemTags()).containsOnly("tag1", "tag2"); assertThat(rule.tags()).isEmpty(); } @@ -415,7 +464,7 @@ public class RegisterRulesMediumTest { .setDescription("format parameter"); } }); - Rule template = index.getByKey(RuleKey.of("xoo", "T1")); + Rule template = ruleIndex.getByKey(RuleKey.of("xoo", "T1")); // Create custom rule RuleKey customRuleKey = TESTER.get(RuleCreator.class).create(NewRule.createForCustomRule("CUSTOM_RULE", template.key()) @@ -444,7 +493,7 @@ public class RegisterRulesMediumTest { }); // Verify custom rule has been restore from the template - Rule customRule = index.getByKey(customRuleKey); + Rule customRule = ruleIndex.getByKey(customRuleKey); assertThat(customRule.language()).isEqualTo("xoo"); assertThat(customRule.internalKey()).isEqualTo("new_internal"); assertThat(customRule.severity()).isEqualTo(Severity.BLOCKER); @@ -471,7 +520,7 @@ public class RegisterRulesMediumTest { } }; register(rules); - Rule template = index.getByKey(RuleKey.of("xoo", "T1")); + Rule template = ruleIndex.getByKey(RuleKey.of("xoo", "T1")); // Create custom rule RuleKey customRuleKey = TESTER.get(RuleCreator.class).create(NewRule.createForCustomRule("CUSTOM_RULE", template.key()) @@ -481,12 +530,12 @@ public class RegisterRulesMediumTest { .setStatus(RuleStatus.READY) .setParameters(ImmutableMap.of("format", "txt"))); - Date updatedAt = index.getByKey(customRuleKey).updatedAt(); + Date updatedAt = ruleIndex.getByKey(customRuleKey).updatedAt(); register(rules); // Verify custom rule has been restore from the template - Rule customRuleReloaded = index.getByKey(customRuleKey); + Rule customRuleReloaded = ruleIndex.getByKey(customRuleKey); assertThat(customRuleReloaded.updatedAt()).isEqualTo(updatedAt); } @@ -506,7 +555,7 @@ public class RegisterRulesMediumTest { .setDescription("format parameter"); } }); - Rule templateRule = index.getByKey(RuleKey.of("xoo", "T1")); + Rule templateRule = ruleIndex.getByKey(RuleKey.of("xoo", "T1")); // Create custom rule RuleKey customRuleKey = TESTER.get(RuleCreator.class).create(NewRule.createForCustomRule("CUSTOM_RULE", templateRule.key()) @@ -524,7 +573,7 @@ public class RegisterRulesMediumTest { .setHtmlDescription("template1 desc") .setSeverity(Severity.MAJOR) .setTemplate(true) - // "format" removed, "format2" added + // "format" removed, "format2" added .createParam("format2") .setDefaultValue("csv") .setType(RuleParamType.STRING) @@ -533,7 +582,7 @@ public class RegisterRulesMediumTest { }); // Verify custom rule param has not been changed! - Rule customRuleReloaded = index.getByKey(customRuleKey); + Rule customRuleReloaded = ruleIndex.getByKey(customRuleKey); assertThat(customRuleReloaded.params().get(0).key()).isEqualTo("format"); } @@ -554,7 +603,7 @@ public class RegisterRulesMediumTest { } }; register(rules); - Rule templateRule = index.getByKey(RuleKey.of("xoo", "T1")); + Rule templateRule = ruleIndex.getByKey(RuleKey.of("xoo", "T1")); // Create custom rule RuleKey customRuleKey = TESTER.get(RuleCreator.class).create(NewRule.createForCustomRule("CUSTOM_RULE", templateRule.key()) @@ -563,19 +612,19 @@ public class RegisterRulesMediumTest { .setSeverity(Severity.MAJOR) .setStatus(RuleStatus.READY) .setParameters(ImmutableMap.of("format", "txt"))); - assertThat(index.getByKey(customRuleKey).status()).isEqualTo(RuleStatus.READY); + assertThat(ruleIndex.getByKey(customRuleKey).status()).isEqualTo(RuleStatus.READY); // Restart without template register(null); // Verify custom rule is removed - assertThat(index.getByKey(templateRule.key()).status()).isEqualTo(RuleStatus.REMOVED); - assertThat(index.getByKey(customRuleKey).status()).isEqualTo(RuleStatus.REMOVED); + assertThat(ruleIndex.getByKey(templateRule.key()).status()).isEqualTo(RuleStatus.REMOVED); + assertThat(ruleIndex.getByKey(customRuleKey).status()).isEqualTo(RuleStatus.REMOVED); // Re-install template register(rules); - assertThat(index.getByKey(templateRule.key()).status()).isEqualTo(RuleStatus.READY); - assertThat(index.getByKey(customRuleKey).status()).isEqualTo(RuleStatus.READY); + assertThat(ruleIndex.getByKey(templateRule.key()).status()).isEqualTo(RuleStatus.READY); + assertThat(ruleIndex.getByKey(customRuleKey).status()).isEqualTo(RuleStatus.READY); } @Test @@ -586,13 +635,13 @@ public class RegisterRulesMediumTest { .setHtmlDescription("Some description")); dbSession.commit(); dbSession.clearCache(); - assertThat(index.getByKey(manualRuleKey).status()).isEqualTo(RuleStatus.READY); + assertThat(ruleIndex.getByKey(manualRuleKey).status()).isEqualTo(RuleStatus.READY); // Restart register(null); // Verify manual rule is still ready - assertThat(index.getByKey(manualRuleKey).status()).isEqualTo(RuleStatus.READY); + assertThat(ruleIndex.getByKey(manualRuleKey).status()).isEqualTo(RuleStatus.READY); } interface Rules { |