mirrors
/
sonarqube
mirror of https://github.com/SonarSource/sonarqube.git
Watch 1
Star 0
Fork 0
Code Issues 0 Releases 237 Wiki Activity
Browse Source

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
tags/4.5-RC1
Simon Brandhof 10 years ago
parent
d25d97de63
commit
4b0f2cd14b
4 changed files with 121 additions and 50 deletions
Split View Show Diff Stats
  1. 2
    2
      sonar-core/src/main/resources/org/sonar/core/qualityprofile/db/ActiveRuleMapper.xml
  2. 11
    0
      sonar-server/src/main/java/org/sonar/server/qualityprofile/db/ActiveRuleDao.java
  3. 18
    7
      sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java
  4. 90
    41
      sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java

+ 2
- 2
sonar-core/src/main/resources/org/sonar/core/qualityprofile/db/ActiveRuleMapper.xml View File

@@ -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>


+ 11
- 0
sonar-server/src/main/java/org/sonar/server/qualityprofile/db/ActiveRuleDao.java View File

@@ -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);
}
}
}
}
}

+ 18
- 7
sonar-server/src/main/java/org/sonar/server/rule/RegisterRules.java View File

@@ -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);
}
}
}
}
}

+ 90
- 41
sonar-server/src/test/java/org/sonar/server/rule/RegisterRulesMediumTest.java View File

@@ -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 {

Write Preview
Loading…
Cancel
Save