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 ecd955e1f73a146ed062eac16a4d3e8877640170..86652c2fa54a9d249561c6c584ec841d6236212a 100644 (file)
--- 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 65a83e1a8c53770b8c6a859b3b6f67ed77cc9339..c1ad40acb0b6069de48a86b8614bb54391a9ba11 100644 (file)
--- 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<String, RuleParamDto> paramsByKey(List<RuleParamDto> 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 4e1f82b9d0a0b8577a25391914c6387bb50561d0..13102953a50e3726572b9329b2adaeb7ae98edc9 100644 (file)
--- 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();