Fix quality flaws

11 files changed, 153 insertions, 29 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/measure/api/MeasureComputerContextImpl.java b/server/sonar-server/src/main/java/org/sonar/server/computation/measure/api/MeasureComputerContextImpl.java
index 8a0c597be6e..d3112cdd51d 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/computation/measure/api/MeasureComputerContextImpl.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/computation/measure/api/MeasureComputerContextImpl.java
@@ -68,15 +68,6 @@ public class MeasureComputerContextImpl implements MeasureComputerContext {
     this.componentIssues = componentIssuesRepository.getIssues(component);
   }
 
-  private Component newComponent(org.sonar.server.computation.component.Component component) {
-    return new ComponentImpl(
-      component.getKey(),
-      Component.Type.valueOf(component.getType().name()),
-      component.getType() == org.sonar.server.computation.component.Component.Type.FILE ?
-        new ComponentImpl.FileAttributesImpl(component.getFileAttributes().getLanguageKey(), component.getFileAttributes().isUnitTest()) :
-        null);
-  }
-
   /**
    * Definition needs to be reset each time a new computer is processed.
    * Defining it by a setter allows to reduce the number of this class to be created (one per component instead of one per component and per computer).
@@ -191,6 +182,14 @@ public class MeasureComputerContextImpl implements MeasureComputerContext {
     return componentIssues;
   }
 
+  private static Component newComponent(org.sonar.server.computation.component.Component component) {
+    return new ComponentImpl(
+      component.getKey(),
+      Component.Type.valueOf(component.getType().name()),
+      component.getType() == org.sonar.server.computation.component.Component.Type.FILE
+        ? new ComponentImpl.FileAttributesImpl(component.getFileAttributes().getLanguageKey(), component.getFileAttributes().isUnitTest()) : null);
+  }
+
   private class ComponentToMeasure implements Function<org.sonar.server.computation.component.Component, Optional<org.sonar.server.computation.measure.Measure>> {
 
     private final Metric metric;
diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/SetTypeAction.java b/server/sonar-server/src/main/java/org/sonar/server/issue/SetTypeAction.java
index 9a7887cf23c..a1b2fbb1b98 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/issue/SetTypeAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/issue/SetTypeAction.java
@@ -58,7 +58,7 @@ public class SetTypeAction extends Action {
   private static String newValue(Map<String, Object> properties) {
     String type = (String) properties.get(TYPE_PARAMETER);
     Preconditions.checkArgument(!isNullOrEmpty(type), "Missing parameter: '%s'", TYPE_PARAMETER);
-    Preconditions.checkArgument(RuleType.ALL_NAMES.contains(type), "Unknown type: %s", type);
+    Preconditions.checkArgument(RuleType.names().contains(type), "Unknown type: %s", type);
     return type;
   }
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/IssuesWs.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/IssuesWs.java
index f79064ed211..8b0c4db9f28 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/IssuesWs.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/IssuesWs.java
@@ -164,7 +164,7 @@ public class IssuesWs implements WebService {
     action.createParam("set_type.type")
       .setDescription("To change the type of the list of issues")
       .setExampleValue(RuleType.BUG)
-      .setPossibleValues(RuleType.ALL_NAMES)
+      .setPossibleValues(RuleType.names())
       .setSince("5.5");
     action.createParam("plan.plan")
       .setDescription("In 5.5, action plans are dropped. Has no effect. To plan the list of issues to a specific action plan (key), or unlink all the issues from an action plan")
diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchAction.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchAction.java
index bef890e4a95..09dfe2cb7db 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchAction.java
@@ -361,7 +361,7 @@ public class SearchAction implements IssuesWsAction {
     addMandatoryValuesToFacet(facets, RULES, request.getRules());
     addMandatoryValuesToFacet(facets, LANGUAGES, request.getLanguages());
     addMandatoryValuesToFacet(facets, TAGS, request.getTags());
-    addMandatoryValuesToFacet(facets, TYPES, RuleType.ALL_NAMES);
+    addMandatoryValuesToFacet(facets, TYPES, RuleType.names());
     List<String> actionPlans = Lists.newArrayList("");
     List<String> actionPlansFromRequest = request.getActionPlans();
     if (actionPlansFromRequest != null) {
diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SetTypeAction.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SetTypeAction.java
index af082ffb670..d02b73cadea 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SetTypeAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SetTypeAction.java
@@ -53,7 +53,7 @@ public class SetTypeAction implements IssuesWsAction {
     action.createParam("type")
       .setDescription("New type")
       .setRequired(true)
-      .setPossibleValues(RuleType.ALL_NAMES);
+      .setPossibleValues(RuleType.names());
   }
 
   @Override
diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/NewCustomRule.java b/server/sonar-server/src/main/java/org/sonar/server/rule/NewCustomRule.java
index 15c3314ddd7..f83be6f9511 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/NewCustomRule.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/NewCustomRule.java
@@ -104,10 +104,6 @@ public class NewCustomRule {
     return this;
   }
 
-  public Map<String, String> parameters() {
-    return parameters;
-  }
-
   @CheckForNull
   public String parameter(final String paramKey) {
     return parameters.get(paramKey);
diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java b/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java
index d931ebf9162..f59dd90752c 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/rule/ws/SearchAction.java
@@ -434,7 +434,7 @@ public class SearchAction implements RulesWsAction {
     addMandatoryFacetValues(results, FACET_SEVERITIES, Severity.ALL);
     addMandatoryFacetValues(results, FACET_ACTIVE_SEVERITIES, Severity.ALL);
     addMandatoryFacetValues(results, FACET_TAGS, request.getTags());
-    addMandatoryFacetValues(results, FACET_TYPES, RuleType.ALL_NAMES);
+    addMandatoryFacetValues(results, FACET_TYPES, RuleType.names());
 
     Common.Facet.Builder facet = Common.Facet.newBuilder();
     Common.FacetValue.Builder value = Common.FacetValue.newBuilder();
@@ -478,10 +478,10 @@ public class SearchAction implements RulesWsAction {
     }
   }
 
-  protected void addMandatoryFacetValues(SearchResult results, String facetName, @Nullable List<String> mandatoryValues) {
+  protected void addMandatoryFacetValues(SearchResult results, String facetName, @Nullable Collection<String> mandatoryValues) {
     Map<String, Long> facetValues = results.facets.get(facetName);
     if (facetValues != null) {
-      List<String> valuesToAdd = mandatoryValues == null ? Lists.<String>newArrayList() : mandatoryValues;
+      Collection<String> valuesToAdd = mandatoryValues == null ? Lists.<String>newArrayList() : mandatoryValues;
       for (String item : valuesToAdd) {
         if (!facetValues.containsKey(item)) {
           facetValues.put(item, 0L);
diff --git a/sonar-db/src/main/java/org/sonar/db/version/v50/ReplaceIssueFiltersProjectKeyByUuid.java b/sonar-db/src/main/java/org/sonar/db/version/v50/ReplaceIssueFiltersProjectKeyByUuid.java
index d8945971a12..edcba952483 100644
--- a/sonar-db/src/main/java/org/sonar/db/version/v50/ReplaceIssueFiltersProjectKeyByUuid.java
+++ b/sonar-db/src/main/java/org/sonar/db/version/v50/ReplaceIssueFiltersProjectKeyByUuid.java
@@ -82,7 +82,7 @@ public class ReplaceIssueFiltersProjectKeyByUuid extends BaseDataChange {
     }
   }
 
-  private String convertData(PreparedStatement pstmt, String data) throws SQLException {
+  private static String convertData(PreparedStatement pstmt, String data) throws SQLException {
     StringBuilder newFields = new StringBuilder();
     String[] fields = data.split("\\|");
     for (int i = 0; i < fields.length; i++) {
diff --git a/sonar-db/src/test/java/org/sonar/db/qualityprofile/ActiveRuleDaoTest.java b/sonar-db/src/test/java/org/sonar/db/qualityprofile/ActiveRuleDaoTest.java
index 17d05a9d09b..9567f8ecaa9 100644
--- a/sonar-db/src/test/java/org/sonar/db/qualityprofile/ActiveRuleDaoTest.java
+++ b/sonar-db/src/test/java/org/sonar/db/qualityprofile/ActiveRuleDaoTest.java
@@ -48,8 +48,6 @@ import static org.sonar.db.qualityprofile.ActiveRuleDto.INHERITED;
 import static org.sonar.db.qualityprofile.ActiveRuleDto.OVERRIDES;
 import static org.sonar.db.qualityprofile.ActiveRuleDto.createFor;
 
-// TODO add missing tests
-
 public class ActiveRuleDaoTest {
 
   @Rule
@@ -233,6 +231,30 @@ public class ActiveRuleDaoTest {
   }
 
   @Test
+  public void fail_to_insert_when_profile_id_is_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Quality profile is not persisted (missing id)");
+
+    underTest.insert(dbTester.getSession(), createFor(profile1, rule1).setProfileId(null));
+  }
+
+  @Test
+  public void fail_to_insert_when_rule_id_is_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Rule is not persisted");
+
+    underTest.insert(dbTester.getSession(), createFor(profile1, rule1).setRuleId(null));
+  }
+
+  @Test
+  public void fail_to_insert_when_id_is_not_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("ActiveRule is already persisted");
+
+    underTest.insert(dbTester.getSession(), createFor(profile1, rule1).setId(100));
+  }
+
+  @Test
   public void update() throws Exception {
     ActiveRuleDto activeRule = createFor(profile1, rule1)
       .setSeverity(BLOCKER)
@@ -263,6 +285,30 @@ public class ActiveRuleDaoTest {
   }
 
   @Test
+  public void fail_to_update_when_profile_id_is_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Quality profile is not persisted (missing id)");
+
+    underTest.update(dbTester.getSession(), createFor(profile1, rule1).setId(100).setProfileId(null));
+  }
+
+  @Test
+  public void fail_to_update_when_rule_id_is_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Rule is not persisted");
+
+    underTest.update(dbTester.getSession(), createFor(profile1, rule1).setId(100).setRuleId(null));
+  }
+
+  @Test
+  public void fail_to_update_when_id_is_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("ActiveRule is not persisted");
+
+    underTest.update(dbTester.getSession(), createFor(profile1, rule1).setId(null));
+  }
+
+  @Test
   public void delete() throws Exception {
     ActiveRuleDto activeRule = createFor(profile1, rule1)
       .setSeverity(BLOCKER)
@@ -279,6 +325,11 @@ public class ActiveRuleDaoTest {
   }
 
   @Test
+  public void does_not_fail_when_active_rule_does_not_exist() throws Exception {
+    underTest.delete(dbSession, ActiveRuleKey.of(profile1.getKey(), rule1.getKey()));
+  }
+
+  @Test
   public void select_params_by_active_rule_id() throws Exception {
     ActiveRuleDto activeRule = createFor(profile1, rule1).setSeverity(BLOCKER);
     underTest.insert(dbTester.getSession(), activeRule);
@@ -355,6 +406,36 @@ public class ActiveRuleDaoTest {
   }
 
   @Test
+  public void fail_to_insert_param_when_active_rule_id_is_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("ActiveRule is not persisted");
+
+    underTest.insertParam(dbTester.getSession(),
+      createFor(profile1, rule1).setId(null),
+      ActiveRuleParamDto.createFor(rule1Param1).setValue("activeValue1"));
+  }
+
+  @Test
+  public void fail_to_insert_param_when_active_rule_param_id_is_null() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("ActiveRuleParam is already persisted");
+
+    underTest.insertParam(dbTester.getSession(),
+      createFor(profile1, rule1).setId(100),
+      ActiveRuleParamDto.createFor(rule1Param1).setValue("activeValue1").setId(100));
+  }
+
+  @Test
+  public void fail_to_insert_param_when_id_is_not_null() throws Exception {
+    thrown.expect(NullPointerException.class);
+    thrown.expectMessage("Rule param is not persisted");
+
+    underTest.insertParam(dbTester.getSession(),
+      createFor(profile1, rule1).setId(100),
+      ActiveRuleParamDto.createFor(rule1Param1).setValue("activeValue1").setRulesParameterId(null));
+  }
+
+  @Test
   public void update_param() throws Exception {
     ActiveRuleDto activeRule = createFor(profile1, rule1).setSeverity(BLOCKER);
     underTest.insert(dbTester.getSession(), activeRule);
@@ -402,6 +483,22 @@ public class ActiveRuleDaoTest {
   }
 
   @Test
+  public void does_not_fail_to_delete_param_by_key_and_name_when_active_rule_does_not_exist() throws Exception {
+    underTest.deleteParamByKeyAndName(dbSession, ActiveRuleKey.of(profile1.getKey(), rule1.getKey()), rule1Param1.getName());
+  }
+
+  @Test
+  public void does_not_fail_to_delete_param_by_key_and_name_when_active_rule_param_does_not_exist() throws Exception {
+    ActiveRuleDto activeRule = createFor(profile1, rule1).setSeverity(BLOCKER);
+    underTest.insert(dbTester.getSession(), activeRule);
+    ActiveRuleParamDto activeRuleParam1 = ActiveRuleParamDto.createFor(rule1Param1).setValue("activeValue1");
+    underTest.insertParam(dbSession, activeRule, activeRuleParam1);
+    dbSession.commit();
+
+    underTest.deleteParamByKeyAndName(dbSession, activeRule.getKey(), "unknown");
+  }
+
+  @Test
   public void delete_param_by_rule_param() throws Exception {
     ActiveRuleDto activeRule1 = createFor(profile1, rule1).setSeverity(BLOCKER);
     underTest.insert(dbTester.getSession(), activeRule1);
@@ -421,4 +518,21 @@ public class ActiveRuleDaoTest {
     assertThat(underTest.selectParamByKeyAndName(activeRule1.getKey(), activeRuleParam1.getKey(), dbSession)).isNull();
     assertThat(underTest.selectParamByKeyAndName(activeRule2.getKey(), activeRuleParam2.getKey(), dbSession)).isNull();
   }
+
+  @Test
+  public void does_not_fail_to_delete_param_by_rule_param_when_active_param_name_not_found() throws Exception {
+    ActiveRuleDto activeRule1 = createFor(profile1, rule1).setSeverity(BLOCKER);
+    underTest.insert(dbTester.getSession(), activeRule1);
+    ActiveRuleParamDto activeRuleParam1 = ActiveRuleParamDto.createFor(rule1Param1).setValue("activeValue1");
+    underTest.insertParam(dbSession, activeRule1, activeRuleParam1);
+
+    ActiveRuleDto activeRule2 = createFor(profile2, rule1).setSeverity(BLOCKER);
+    underTest.insert(dbTester.getSession(), activeRule2);
+    ActiveRuleParamDto activeRuleParam2 = ActiveRuleParamDto.createFor(rule1Param1).setValue("activeValue2");
+    underTest.insertParam(dbSession, activeRule2, activeRuleParam2);
+
+    dbSession.commit();
+
+    underTest.deleteParamsByRuleParam(dbSession, rule1, "unknown");
+  }
 }
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleType.java b/sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleType.java
index 66222702a69..5755b8050fb 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleType.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/rules/RuleType.java
@@ -19,16 +19,18 @@
  */
 package org.sonar.api.rules;
 
-import com.google.common.base.Enums;
-import com.google.common.collect.Lists;
-import java.util.List;
+import com.google.common.base.Function;
+import java.util.Arrays;
+import java.util.Set;
+import javax.annotation.Nonnull;
 
+import static com.google.common.collect.FluentIterable.from;
 import static java.lang.String.format;
 
 public enum RuleType {
   CODE_SMELL(1), BUG(2), VULNERABILITY(3);
 
-  public static final List<String> ALL_NAMES = Lists.transform(Lists.newArrayList(values()), Enums.stringConverter(RuleType.class).reverse());
+  private static final Set<String> ALL_NAMES = from(Arrays.asList(values())).transform(ToName.INSTANCE).toSet();
 
   private final int dbConstant;
 
@@ -40,6 +42,10 @@ public enum RuleType {
     return dbConstant;
   }
 
+  public static Set<String> names() {
+    return ALL_NAMES;
+  }
+
   /**
    * Returns the enum constant of the specified DB column value.
    */
@@ -53,4 +59,13 @@ public enum RuleType {
     throw new IllegalArgumentException(format("Unsupported type value : %d", dbConstant));
   }
 
+  private enum ToName implements Function<RuleType, String> {
+    INSTANCE;
+
+    @Override
+    public String apply(@Nonnull RuleType input) {
+      return input.name();
+    }
+  }
+
 }
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/rules/RuleTypeTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/rules/RuleTypeTest.java
index 54f914049bf..a2ecc6cddf9 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/rules/RuleTypeTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/rules/RuleTypeTest.java
@@ -45,12 +45,12 @@ public class RuleTypeTest {
 
   @Test
   public void test_ALL_NAMES() {
-    assertThat(RuleType.ALL_NAMES).containsOnly("BUG", "VULNERABILITY", "CODE_SMELL");
+    assertThat(RuleType.names()).containsOnly("BUG", "VULNERABILITY", "CODE_SMELL");
   }
 
   @Test
   public void ALL_NAMES_is_immutable() {
     expectedException.expect(UnsupportedOperationException.class);
-    RuleType.ALL_NAMES.add("foo");
+    RuleType.names().add("foo");
   }
 }