From 9ed6664d22efa17ab0f86ffe1135df9c9802c38a Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Wed, 5 Nov 2014 19:05:15 +0100 Subject: [PATCH] SONAR-5841 Empty parameter on integer, boolean and float types should be authorized when setting a rule parameter --- .../server/qualityprofile/RuleActivation.java | 4 +- .../server/qualityprofile/RuleActivator.java | 15 +++--- .../RuleActivatorMediumTest.java | 54 ++++++++++++++----- .../api/server/rule/RulesDefinition.java | 5 +- .../api/server/rule/RulesDefinitionTest.java | 19 +++++++ 5 files changed, 76 insertions(+), 21 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java index e89c21d1e32..2df174d3625 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivation.java @@ -93,7 +93,9 @@ public class RuleActivation { public RuleActivation setParameters(Map m) { parameters.clear(); - parameters.putAll(m); + for (Map.Entry entry : m.entrySet()) { + setParameter(entry.getKey(), entry.getValue()); + } return this; } diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java index a456b77eea9..e0203e4e90c 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RuleActivator.java @@ -68,8 +68,8 @@ public class RuleActivator implements ServerComponent { private final ActivityService log; public RuleActivator(DbClient db, IndexClient index, - RuleActivatorContextFactory contextFactory, TypeValidations typeValidations, - PreviewCache previewCache, ActivityService log) { + RuleActivatorContextFactory contextFactory, TypeValidations typeValidations, + PreviewCache previewCache, ActivityService log) { this.db = db; this.index = index; this.contextFactory = contextFactory; @@ -201,10 +201,11 @@ public class RuleActivator implements ServerComponent { context.defaultSeverity())); for (RuleParamDto ruleParamDto : context.ruleParams()) { String paramKey = ruleParamDto.getName(); - change.setParameter(paramKey, validateParam(ruleParamDto, firstNonNull( - context.requestParamValue(request, paramKey), - context.parentParamValue(paramKey), - context.defaultParamValue(paramKey)))); + change.setParameter(paramKey, validateParam(ruleParamDto, + firstNonNull( + context.requestParamValue(request, paramKey), + context.parentParamValue(paramKey), + context.defaultParamValue(paramKey)))); } } } @@ -212,7 +213,7 @@ public class RuleActivator implements ServerComponent { @CheckForNull String firstNonNull(String... strings) { for (String s : strings) { - if (s!=null) { + if (s != null) { return s; } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java index a9f7c6f8c52..a7da04a5039 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/RuleActivatorMediumTest.java @@ -157,19 +157,49 @@ public class RuleActivatorMediumTest { ImmutableMap.of("max", "10")); } + /** + * SONAR-5841 + */ @Test - public void activate_rule_with_negative_integer_value_on_parameter_without_default_value() throws Exception { - RuleKey ruleKey = RuleKey.of("negative", "value"); - RuleDto rule = RuleTesting.newDto(ruleKey).setLanguage("xoo").setSeverity(Severity.MINOR); - db.ruleDao().insert(dbSession, rule); - db.ruleDao().addRuleParam(dbSession, rule, RuleParamDto.createFor(rule) - .setName("max").setType(RuleParamType.INTEGER.type())); + public void activate_with_empty_parameter_having_no_default_value() throws Exception { + activate(new RuleActivation(RuleTesting.XOO_X1) + .setParameter("min", ""), + XOO_P1_KEY); - activate(new RuleActivation(ruleKey).setParameter("max", "-10"), XOO_P1_KEY); + assertThat(countActiveRules(XOO_P1_KEY)).isEqualTo(1); + verifyHasActiveRule(ActiveRuleKey.of(XOO_P1_KEY, RuleTesting.XOO_X1), Severity.MINOR, null, + // Max should be set to default value, min has not value it should be ignored + ImmutableMap.of("max", "10")); + } + + /** + * SONAR-5841 + */ + @Test + public void activate_with_empty_parameters() throws Exception { + activate(new RuleActivation(RuleTesting.XOO_X1) + .setParameters(ImmutableMap.of("max", "", "min", "")), + XOO_P1_KEY); + + assertThat(countActiveRules(XOO_P1_KEY)).isEqualTo(1); + // Max should be set to default value, min has not value it should be ignored + verifyHasActiveRule(ActiveRuleKey.of(XOO_P1_KEY, RuleTesting.XOO_X1), Severity.MINOR, null, + ImmutableMap.of("max", "10")); + } + + /** + * SONAR-5840 + */ + @Test + public void activate_rule_with_negative_integer_value_on_parameter_having_no_default_value() throws Exception { + activate(new RuleActivation(RuleTesting.XOO_X1) + .setParameter("min", "-10"), + XOO_P1_KEY); assertThat(countActiveRules(XOO_P1_KEY)).isEqualTo(1); - verifyHasActiveRule(ActiveRuleKey.of(XOO_P1_KEY, ruleKey), Severity.MINOR, null, - ImmutableMap.of("max", "-10")); + // Max should be set to default value, min should be set to -10 + verifyHasActiveRule(ActiveRuleKey.of(XOO_P1_KEY, RuleTesting.XOO_X1), Severity.MINOR, null, + ImmutableMap.of("max", "10", "min", "-10")); } @Test @@ -1007,18 +1037,18 @@ public class RuleActivatorMediumTest { } private void verifyOneActiveRule(String profileKey, RuleKey ruleKey, String expectedSeverity, - @Nullable String expectedInheritance, Map expectedParams) { + @Nullable String expectedInheritance, Map expectedParams) { assertThat(countActiveRules(profileKey)).isEqualTo(1); verifyHasActiveRule(profileKey, ruleKey, expectedSeverity, expectedInheritance, expectedParams); } private void verifyHasActiveRule(String profileKey, RuleKey ruleKey, String expectedSeverity, - @Nullable String expectedInheritance, Map expectedParams) { + @Nullable String expectedInheritance, Map expectedParams) { verifyHasActiveRule(ActiveRuleKey.of(profileKey, ruleKey), expectedSeverity, expectedInheritance, expectedParams); } private void verifyHasActiveRule(ActiveRuleKey activeRuleKey, String expectedSeverity, - @Nullable String expectedInheritance, Map expectedParams) { + @Nullable String expectedInheritance, Map expectedParams) { // verify db boolean found = false; List activeRuleDtos = db.activeRuleDao().findByProfileKey(dbSession, activeRuleKey.qProfile()); diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java b/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java index ac9e7c34fa3..75a5a2bf077 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java @@ -881,8 +881,11 @@ public interface RulesDefinition extends ServerExtension { return this; } + /** + * Empty default value will be converted to null. + */ public NewParam setDefaultValue(@Nullable String s) { - this.defaultValue = s; + this.defaultValue = Strings.emptyToNull(s); return this; } } diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java index aa4c87cffde..56bf1b1bf6a 100644 --- a/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java +++ b/sonar-plugin-api/src/test/java/org/sonar/api/server/rule/RulesDefinitionTest.java @@ -172,6 +172,25 @@ public class RulesDefinitionTest { assertThat(level.hashCode()).isEqualTo(level.hashCode()); } + @Test + public void define_rule_parameter_with_empty_default_value() { + RulesDefinition.NewRepository newFindbugs = context.createRepository("findbugs", "java"); + RulesDefinition.NewRule newNpe = newFindbugs.createRule("NPE").setName("NPE").setHtmlDescription("NPE"); + newNpe.createParam("level").setDefaultValue("").setName("Level").setDescription("The level").setType(RuleParamType.INTEGER); + newFindbugs.done(); + + RulesDefinition.Rule rule = context.repository("findbugs").rule("NPE"); + assertThat(rule.params()).hasSize(1); + + RulesDefinition.Param level = rule.param("level"); + assertThat(level.key()).isEqualTo("level"); + assertThat(level.name()).isEqualTo("Level"); + assertThat(level.description()).isEqualTo("The level"); + // Empty value is converted in null value + assertThat(level.defaultValue()).isNull(); + assertThat(level.type()).isEqualTo(RuleParamType.INTEGER); + } + @Test public void sanitize_rule_name() { RulesDefinition.NewRepository newFindbugs = context.createRepository("findbugs", "java"); -- 2.39.5