From c7662baa5b3c54e0627f88796728e2e0da1f70fb Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Wed, 6 Apr 2016 10:31:09 +0200 Subject: [PATCH] Fix quality flaws --- .../api/MeasureComputerContextImpl.java | 17 ++- .../org/sonar/server/issue/SetTypeAction.java | 2 +- .../org/sonar/server/issue/ws/IssuesWs.java | 2 +- .../sonar/server/issue/ws/SearchAction.java | 2 +- .../sonar/server/issue/ws/SetTypeAction.java | 2 +- .../org/sonar/server/rule/NewCustomRule.java | 4 - .../sonar/server/rule/ws/SearchAction.java | 6 +- .../ReplaceIssueFiltersProjectKeyByUuid.java | 2 +- .../db/qualityprofile/ActiveRuleDaoTest.java | 118 +++++++++++++++++- .../java/org/sonar/api/rules/RuleType.java | 23 +++- .../org/sonar/api/rules/RuleTypeTest.java | 4 +- 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> { 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 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 actionPlans = Lists.newArrayList(""); List 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 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 mandatoryValues) { + protected void addMandatoryFacetValues(SearchResult results, String facetName, @Nullable Collection mandatoryValues) { Map facetValues = results.facets.get(facetName); if (facetValues != null) { - List valuesToAdd = mandatoryValues == null ? Lists.newArrayList() : mandatoryValues; + Collection valuesToAdd = mandatoryValues == null ? Lists.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 @@ -232,6 +230,30 @@ public class ActiveRuleDaoTest { assertThat(result.getUpdatedAt()).isEqualTo(2000L); } + @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) @@ -262,6 +284,30 @@ public class ActiveRuleDaoTest { assertThat(result.getUpdatedAt()).isEqualTo(4000L); } + @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) @@ -278,6 +324,11 @@ public class ActiveRuleDaoTest { assertThat(underTest.selectByKey(dbSession, ActiveRuleKey.of(profile1.getKey(), rule1.getKey()))).isAbsent(); } + @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); @@ -354,6 +405,36 @@ public class ActiveRuleDaoTest { assertThat(result.getValue()).isEqualTo("activeValue1"); } + @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); @@ -401,6 +482,22 @@ public class ActiveRuleDaoTest { assertThat(underTest.selectParamByKeyAndName(activeRule.getKey(), activeRuleParam1.getKey(), dbSession)).isNull(); } + @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); @@ -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 ALL_NAMES = Lists.transform(Lists.newArrayList(values()), Enums.stringConverter(RuleType.class).reverse()); + private static final Set 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 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 { + 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"); } } -- 2.39.5