From: Guillaume Jambet Date: Fri, 8 Dec 2017 16:25:06 +0000 (+0100) Subject: SONAR-10163 Updating description of built-in rules now fails with error 400 X-Git-Tag: 7.0-RC1~92 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=343be23904dcb520ea86c4d6070cd3be272941da;p=sonarqube.git SONAR-10163 Updating description of built-in rules now fails with error 400 --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java index ecd955e1f73..86652c2fa54 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/RuleUpdate.java @@ -30,6 +30,7 @@ import org.sonar.api.rule.RuleStatus; import org.sonar.api.server.debt.DebtRemediationFunction; import org.sonar.db.organization.OrganizationDto; +import static com.google.common.base.Preconditions.checkArgument; import static org.sonar.server.rule.RuleUpdate.RuleUpdateUseCase.CUSTOM_RULE; import static org.sonar.server.rule.RuleUpdate.RuleUpdateUseCase.PLUGIN_RULE; @@ -223,9 +224,7 @@ public class RuleUpdate { } private void checkCustomRule() { - if (useCase != CUSTOM_RULE) { - throw new IllegalStateException("Not a custom rule"); - } + checkArgument(useCase == CUSTOM_RULE, "Not a custom rule"); } public OrganizationDto getOrganization() { diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterTest.java index 65a83e1a8c5..c1ad40acb0b 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/RuleUpdaterTest.java @@ -27,6 +27,7 @@ import com.google.common.collect.Sets; import java.util.List; import java.util.Map; import javax.annotation.Nonnull; +import org.assertj.core.api.ThrowableAssertAlternative; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -60,6 +61,11 @@ import org.sonar.server.tester.UserSessionRule; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; +import static org.sonar.api.rule.RuleStatus.REMOVED; +import static org.sonar.api.rule.Severity.CRITICAL; +import static org.sonar.db.rule.RuleTesting.newRule; +import static org.sonar.server.rule.RuleUpdate.createForCustomRule; +import static org.sonar.server.rule.RuleUpdate.createForPluginRule; public class RuleUpdaterTest { @@ -82,24 +88,22 @@ public class RuleUpdaterTest { private RuleIndex ruleIndex = new RuleIndex(es.client(), system2); private RuleIndexer ruleIndexer = new RuleIndexer(es.client(), db.getDbClient()); private DbSession dbSession = db.getSession(); - private TestDefaultOrganizationProvider defaultOrganizationProvider = TestDefaultOrganizationProvider.from(db); private RuleUpdater underTest = new RuleUpdater(db.getDbClient(), ruleIndexer, system2); @Test public void do_not_update_rule_with_removed_status() { - db.rules().insert(RuleTesting.newRule(RULE_KEY).setStatus(RuleStatus.REMOVED)); + db.rules().insert(newRule(RULE_KEY).setStatus(RuleStatus.REMOVED)); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setTags(Sets.newHashSet("java9")) .setOrganization(db.getDefaultOrganization()); - try { - underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessage("Rule with REMOVED status cannot be updated: squid:S001"); - } + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Rule with REMOVED status cannot be updated: squid:S001"); + + underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); } @Test @@ -116,7 +120,7 @@ public class RuleUpdaterTest { db.rules().insertOrUpdateMetadata(ruleDto.getMetadata().setRuleId(ruleDto.getId())); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY); + RuleUpdate update = createForPluginRule(RULE_KEY); assertThat(update.isEmpty()).isTrue(); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -147,7 +151,7 @@ public class RuleUpdaterTest { db.rules().insertOrUpdateMetadata(ruleDto.getMetadata().setRuleId(ruleDto.getId())); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setMarkdownNote("my *note*") .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -174,7 +178,7 @@ public class RuleUpdaterTest { db.rules().insertOrUpdateMetadata(ruleDto.getMetadata().setRuleId(ruleDto.getId())); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setMarkdownNote(null) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -196,7 +200,7 @@ public class RuleUpdaterTest { dbSession.commit(); // java8 is a system tag -> ignore - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setTags(Sets.newHashSet("bug", "java8")) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -219,7 +223,7 @@ public class RuleUpdaterTest { db.rules().insertOrUpdateMetadata(ruleDto.getMetadata()); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setTags(null) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -236,14 +240,14 @@ public class RuleUpdaterTest { @Test public void override_debt() { - db.rules().insert(RuleTesting.newRule(RULE_KEY) + db.rules().insert(newRule(RULE_KEY) .setDefRemediationFunction(DebtRemediationFunction.Type.LINEAR_OFFSET.name()) .setDefRemediationGapMultiplier("1d") .setDefRemediationBaseEffort("5min")); dbSession.commit(); DefaultDebtRemediationFunction fn = new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.CONSTANT_ISSUE, null, "1min"); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setDebtRemediationFunction(fn) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -262,13 +266,13 @@ public class RuleUpdaterTest { @Test public void override_debt_only_offset() { - db.rules().insert(RuleTesting.newRule(RULE_KEY) + db.rules().insert(newRule(RULE_KEY) .setDefRemediationFunction(DebtRemediationFunction.Type.LINEAR.name()) .setDefRemediationGapMultiplier("1d") .setDefRemediationBaseEffort(null)); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setDebtRemediationFunction(new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.LINEAR, "2d", null)) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -287,13 +291,13 @@ public class RuleUpdaterTest { @Test public void override_debt_from_linear_with_offset_to_constant() { - db.rules().insert(RuleTesting.newRule(RULE_KEY) + db.rules().insert(newRule(RULE_KEY) .setDefRemediationFunction(DebtRemediationFunction.Type.LINEAR_OFFSET.name()) .setDefRemediationGapMultiplier("1d") .setDefRemediationBaseEffort("5min")); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setDebtRemediationFunction(new DefaultDebtRemediationFunction(DebtRemediationFunction.Type.CONSTANT_ISSUE, null, "10min")) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -323,7 +327,7 @@ public class RuleUpdaterTest { db.rules().insertOrUpdateMetadata(ruleDto.getMetadata().setRuleId(ruleDto.getId())); dbSession.commit(); - RuleUpdate update = RuleUpdate.createForPluginRule(RULE_KEY) + RuleUpdate update = createForPluginRule(RULE_KEY) .setDebtRemediationFunction(null) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -360,7 +364,7 @@ public class RuleUpdaterTest { db.rules().insertRuleParam(customRule, param -> param.setName("format").setType("STRING").setDescription("Format").setDefaultValue(null)); // Update custom rule - RuleUpdate update = RuleUpdate.createForCustomRule(customRule.getKey()) + RuleUpdate update = createForCustomRule(customRule.getKey()) .setName("New name") .setMarkdownDescription("New description") .setSeverity("MAJOR") @@ -410,7 +414,7 @@ public class RuleUpdaterTest { dbSession.commit(); // Update custom rule without setting a value for the parameter - RuleUpdate update = RuleUpdate.createForCustomRule(customRule.getKey()) + RuleUpdate update = createForCustomRule(customRule.getKey()) .setName("New name") .setMarkdownDescription("New description") .setSeverity("MAJOR") @@ -464,7 +468,7 @@ public class RuleUpdaterTest { dbSession.commit(); // Update custom rule parameter 'regex', add 'message' and remove 'format' - RuleUpdate update = RuleUpdate.createForCustomRule(customRule.getKey()) + RuleUpdate update = createForCustomRule(customRule.getKey()) .setParameters(ImmutableMap.of("regex", "b.*", "message", "a message")) .setOrganization(db.getDefaultOrganization()); underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); @@ -508,16 +512,15 @@ public class RuleUpdaterTest { dbSession.commit(); // Update custom rule - RuleUpdate update = RuleUpdate.createForCustomRule(customRule.getKey()) + RuleUpdate update = createForCustomRule(customRule.getKey()) .setName("") .setMarkdownDescription("New desc") .setOrganization(db.getDefaultOrganization()); - try { - underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("The name is missing"); - } + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("The name is missing"); + + underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); } @Test @@ -532,71 +535,45 @@ public class RuleUpdaterTest { dbSession.commit(); - // Update custom rule - RuleUpdate update = RuleUpdate.createForCustomRule(customRule.getKey()) - .setName("New name") - .setMarkdownDescription("") - .setOrganization(db.getDefaultOrganization()); - try { - underTest.update(dbSession, update, db.getDefaultOrganization(), userSessionRule); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("The description is missing"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("The description is missing"); + + underTest.update(dbSession, + createForCustomRule(customRule.getKey()).setName("New name").setMarkdownDescription("").setOrganization(db.getDefaultOrganization()), + db.getDefaultOrganization(), userSessionRule); } @Test public void fail_to_update_plugin_rule_if_name_is_set() { - // Create rule rule - RuleDefinitionDto ruleDto = RuleTesting.newRule(RuleKey.of("squid", "S01")); - db.rules().insert(ruleDto); - + RuleDefinitionDto ruleDefinition = db.rules().insert(newRule(RuleKey.of("squid", "S01"))); dbSession.commit(); - try { - // Update rule - RuleUpdate.createForPluginRule(ruleDto.getKey()) - .setName("New name"); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("Not a custom rule"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Not a custom rule"); + + createForPluginRule(ruleDefinition.getKey()).setName("New name"); } @Test public void fail_to_update_plugin_rule_if_description_is_set() { - // Create rule rule - RuleDefinitionDto ruleDto = RuleTesting.newRule(RuleKey.of("squid", "S01")); - db.rules().insert(ruleDto); - + RuleDefinitionDto ruleDefinition = db.rules().insert(newRule(RuleKey.of("squid", "S01"))); dbSession.commit(); - try { - // Update rule - RuleUpdate.createForPluginRule(ruleDto.getKey()) - .setMarkdownDescription("New description"); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("Not a custom rule"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Not a custom rule"); + + createForPluginRule(ruleDefinition.getKey()).setMarkdownDescription("New description"); } @Test public void fail_to_update_plugin_rule_if_severity_is_set() { - // Create rule rule - RuleDefinitionDto ruleDto = RuleTesting.newRule(RuleKey.of("squid", "S01")); - db.rules().insert(ruleDto); - + RuleDefinitionDto ruleDefinition = db.rules().insert(newRule(RuleKey.of("squid", "S01"))); dbSession.commit(); - try { - // Update rule - RuleUpdate.createForPluginRule(ruleDto.getKey()) - .setSeverity(Severity.CRITICAL); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("Not a custom rule"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Not a custom rule"); + + createForPluginRule(ruleDefinition.getKey()).setSeverity(CRITICAL); } private static Map paramsByKey(List params) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/ws/UpdateActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/ws/UpdateActionTest.java index 4e1f82b9d0a..13102953a50 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/ws/UpdateActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/ws/UpdateActionTest.java @@ -310,6 +310,22 @@ public class UpdateActionTest { .execute(); } + @Test + public void throw_IllegalArgumentException_if_trying_to_update_builtin_rule_description() throws Exception { + logInAsQProfileAdministrator(); + RuleDefinitionDto rule = db.rules().insert(); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Not a custom rule"); + + ws.newRequest().setMethod("POST") + .setParam("key", rule.getKey().toString()) + .setParam("name", rule.getName()) + .setParam("markdown_description", "New description") + .execute(); + + } + @Test public void throw_ForbiddenException_if_not_profile_administrator() throws Exception { userSession.logIn();