From 59ce6faf6fc4ad3c1d3117d2d6ecadb1214d4886 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 27 May 2014 10:21:55 +0200 Subject: [PATCH] SONAR-5007 fix some quality flaws --- .../core/qualityprofile/db/ActiveRuleDao.java | 279 +----------------- .../qualityprofile/db/ActiveRuleDaoTest.java | 201 ------------- .../qualityprofile/ActiveRuleService.java | 12 +- .../QProfileActiveRuleOperations.java | 5 - .../persistence/ActiveRuleDao.java | 3 + .../sonar/server/rule/index/RuleQuery.java | 3 + .../sonar/server/search/ws/BaseMapping.java | 4 +- .../java/org/sonar/server/ws/ListingWs.java | 12 +- 8 files changed, 22 insertions(+), 497 deletions(-) diff --git a/sonar-core/src/main/java/org/sonar/core/qualityprofile/db/ActiveRuleDao.java b/sonar-core/src/main/java/org/sonar/core/qualityprofile/db/ActiveRuleDao.java index f7069f58ced..a7f01729ccc 100644 --- a/sonar-core/src/main/java/org/sonar/core/qualityprofile/db/ActiveRuleDao.java +++ b/sonar-core/src/main/java/org/sonar/core/qualityprofile/db/ActiveRuleDao.java @@ -20,19 +20,12 @@ package org.sonar.core.qualityprofile.db; -import com.google.common.collect.Lists; import org.apache.ibatis.session.SqlSession; import org.sonar.api.ServerComponent; import org.sonar.core.persistence.MyBatis; -import javax.annotation.CheckForNull; - -import java.util.Collection; -import java.util.Collections; import java.util.List; -import static com.google.common.collect.Lists.newArrayList; - public class ActiveRuleDao implements ServerComponent { private final MyBatis mybatis; @@ -55,154 +48,15 @@ public class ActiveRuleDao implements ServerComponent { } } - public void update(ActiveRuleDto dto, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).update(dto); - } - - public void update(ActiveRuleDto dto) { - SqlSession session = mybatis.openSession(false); - try { - update(dto, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - public void delete(int activeRuleId, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).delete(activeRuleId); - } - - public void delete(int activeRuleId) { - SqlSession session = mybatis.openSession(false); - try { - delete(activeRuleId, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - public void deleteFromRule(int ruleId, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).deleteFromRule(ruleId); - } - - public void deleteFromRule(int ruleId) { - SqlSession session = mybatis.openSession(false); - try { - deleteFromRule(ruleId, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - public void deleteFromProfile(int profileId, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).deleteFromProfile(profileId); - } - - public void deleteFromProfile(int profileId) { - SqlSession session = mybatis.openSession(false); - try { - deleteFromProfile(profileId, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectByIds(List ids) { - SqlSession session = mybatis.openSession(false); - try { - return selectByIds(ids, session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectByIds(Collection ids, SqlSession session) { - if (ids.isEmpty()) { - return Collections.emptyList(); - } - List dtosList = newArrayList(); - List> idsPartitionList = Lists.partition(newArrayList(ids), 1000); - for (List idsPartition : idsPartitionList) { - List dtos = session.selectList("org.sonar.core.qualityprofile.db.ActiveRuleMapper.selectByIds", newArrayList(idsPartition)); - dtosList.addAll(dtos); - } - return dtosList; - } - - public List selectAll() { - SqlSession session = mybatis.openSession(false); - try { - return selectAll(session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectAll(SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectAll(); - } - - public List selectByRuleId(int ruleId) { - SqlSession session = mybatis.openSession(false); - try { - return selectByRuleId(ruleId, session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectByRuleId(int ruleId, SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectByRuleId(ruleId); - } - public List selectByProfileId(int profileId) { SqlSession session = mybatis.openSession(false); try { - return selectByProfileId(profileId, session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectByProfileId(int profileId, SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectByProfileId(profileId); - } - - - @CheckForNull - public ActiveRuleDto selectById(int id) { - SqlSession session = mybatis.openSession(false); - try { - return selectById(id, session); - } finally { - MyBatis.closeQuietly(session); - } - } - - @CheckForNull - public ActiveRuleDto selectById(int id, SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectById(id); - } - - @CheckForNull - public ActiveRuleDto selectByProfileAndRule(int profileId, int ruleId) { - SqlSession session = mybatis.openSession(false); - try { - return selectByProfileAndRule(profileId, ruleId, session); + return session.getMapper(ActiveRuleMapper.class).selectByProfileId(profileId); } finally { MyBatis.closeQuietly(session); } } - @CheckForNull - public ActiveRuleDto selectByProfileAndRule(int profileId, int ruleId, SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectByProfileAndRule(profileId, ruleId); - } - public void insert(ActiveRuleParamDto dto, SqlSession session) { session.getMapper(ActiveRuleMapper.class).insertParameter(dto); } @@ -217,137 +71,6 @@ public class ActiveRuleDao implements ServerComponent { } } - public void update(ActiveRuleParamDto dto, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).updateParameter(dto); - } - - public void update(ActiveRuleParamDto dto) { - SqlSession session = mybatis.openSession(false); - try { - update(dto, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - - public void deleteParameter(int activeRuleParamId, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).deleteParameter(activeRuleParamId); - } - - public void deleteParameter(int activeRuleParamId) { - SqlSession session = mybatis.openSession(false); - try { - deleteParameter(activeRuleParamId, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - public void deleteParameters(int activeRuleId, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).deleteParameters(activeRuleId); - } - - public void deleteParameters(int activeRuleId) { - SqlSession session = mybatis.openSession(false); - try { - deleteParameters(activeRuleId, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - public void deleteParametersWithParamId(int id, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).deleteParametersWithParamId(id); - } - - public void deleteParametersFromProfile(int profileId) { - SqlSession session = mybatis.openSession(false); - try { - deleteParametersFromProfile(profileId, session); - session.commit(); - } finally { - MyBatis.closeQuietly(session); - } - } - - public void deleteParametersFromProfile(int profileId, SqlSession session) { - session.getMapper(ActiveRuleMapper.class).deleteParametersFromProfile(profileId); - } - - public ActiveRuleParamDto selectParamById(Integer activeRuleParamId) { - SqlSession session = mybatis.openSession(false); - try { - return session.getMapper(ActiveRuleMapper.class).selectParamById(activeRuleParamId); - } finally { - MyBatis.closeQuietly(session); - } - } - - public ActiveRuleParamDto selectParamByActiveRuleAndKey(int activeRuleId, String key) { - SqlSession session = mybatis.openSession(false); - try { - return selectParamByActiveRuleAndKey(activeRuleId, key, session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public ActiveRuleParamDto selectParamByActiveRuleAndKey(int activeRuleId, String key, SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectParamByActiveRuleAndKey(activeRuleId, key); - } - - public List selectParamsByActiveRuleId(int activeRuleId) { - SqlSession session = mybatis.openSession(false); - try { - return selectParamsByActiveRuleId(activeRuleId, session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectParamsByActiveRuleId(int activeRuleId, SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectParamsByActiveRuleId(activeRuleId); - } - - public List selectParamsByActiveRuleIds(List activeRuleIds) { - SqlSession session = mybatis.openSession(false); - try { - return selectParamsByActiveRuleIds(activeRuleIds, session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectParamsByActiveRuleIds(Collection activeRuleIds, SqlSession session) { - if (activeRuleIds.isEmpty()) { - return Collections.emptyList(); - } - List dtosList = newArrayList(); - List> idsPartitionList = Lists.partition(newArrayList(activeRuleIds), 1000); - for (List idsPartition : idsPartitionList) { - List dtos = session.selectList("org.sonar.core.qualityprofile.db.ActiveRuleMapper.selectParamsByActiveRuleIds", newArrayList(idsPartition)); - dtosList.addAll(dtos); - } - return dtosList; - } - - public List selectAllParams() { - SqlSession session = mybatis.openSession(false); - try { - return selectAllParams(session); - } finally { - MyBatis.closeQuietly(session); - } - } - - public List selectAllParams(SqlSession session) { - return session.getMapper(ActiveRuleMapper.class).selectAllParams(); - } - public List selectParamsByProfileId(int profileId) { SqlSession session = mybatis.openSession(false); try { diff --git a/sonar-core/src/test/java/org/sonar/core/qualityprofile/db/ActiveRuleDaoTest.java b/sonar-core/src/test/java/org/sonar/core/qualityprofile/db/ActiveRuleDaoTest.java index 86dbf2ea461..b95d00bf9da 100644 --- a/sonar-core/src/test/java/org/sonar/core/qualityprofile/db/ActiveRuleDaoTest.java +++ b/sonar-core/src/test/java/org/sonar/core/qualityprofile/db/ActiveRuleDaoTest.java @@ -20,9 +20,6 @@ package org.sonar.core.qualityprofile.db; -import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; import org.junit.Before; import org.junit.Test; import org.sonar.api.rule.Severity; @@ -56,92 +53,6 @@ public class ActiveRuleDaoTest extends AbstractDaoTestCase { checkTables("insert", "active_rules"); } - @Test - public void update() { - setupData("shared"); - - ActiveRuleDto dto = new ActiveRuleDto() - .setId(1) - .setProfileId(1) - .setRuleId(10) - .setSeverity(Severity.BLOCKER) - .setInheritance(null); - - dao.update(dto); - - checkTables("update", "active_rules"); - } - - @Test - public void delete() { - setupData("shared"); - - dao.delete(1); - - checkTables("delete", "active_rules"); - } - - @Test - public void delete_from_rule() { - setupData("shared"); - - dao.deleteFromRule(11); - - checkTables("delete_from_rule", "active_rules"); - } - - @Test - public void delete_from_profile() { - setupData("shared"); - - dao.deleteFromProfile(2); - - checkTables("delete_from_profile", "active_rules"); - } - - @Test - public void select_by_id() { - setupData("shared"); - - ActiveRuleDto result = dao.selectById(1); - assertThat(result.getId()).isEqualTo(1); - assertThat(result.getProfileId()).isEqualTo(1); - assertThat(result.getRulId()).isEqualTo(10); - assertThat(result.getSeverityString()).isEqualTo(Severity.MAJOR); - assertThat(result.getInheritance()).isEqualTo("INHERITED"); - } - - @Test - public void select_by_ids() { - setupData("shared"); - - List result = dao.selectByIds(ImmutableList.of(1)); - assertThat(result).hasSize(1); - assertThat(result.get(0).getParentId()).isEqualTo(2); - - assertThat(dao.selectByIds(ImmutableList.of(1, 2))).hasSize(2); - } - - @Test - public void select_by_profile_and_rule() { - setupData("shared"); - - ActiveRuleDto result = dao.selectByProfileAndRule(1, 10); - assertThat(result.getId()).isEqualTo(1); - assertThat(result.getProfileId()).isEqualTo(1); - assertThat(result.getRulId()).isEqualTo(10); - assertThat(result.getSeverityString()).isEqualTo(Severity.MAJOR); - assertThat(result.getInheritance()).isEqualTo("INHERITED"); - } - - @Test - public void select_by_rule() { - setupData("shared"); - - List result = dao.selectByRuleId(10); - assertThat(result).hasSize(2); - } - @Test public void select_by_profile() { setupData("shared"); @@ -150,17 +61,6 @@ public class ActiveRuleDaoTest extends AbstractDaoTestCase { assertThat(result).hasSize(2); } - @Test - public void select_all() { - setupData("shared"); - - List result = dao.selectAll(); - assertThat(result).hasSize(3); - - assertThat(find(1, result).getParentId()).isEqualTo(2); - assertThat(find(2, result).getParentId()).isNull(); - } - @Test public void insert_parameter() { setupData("empty"); @@ -176,111 +76,10 @@ public class ActiveRuleDaoTest extends AbstractDaoTestCase { checkTables("insert_parameter", "active_rule_parameters"); } - @Test - public void update_parameter() { - setupData("shared"); - - ActiveRuleParamDto dto = new ActiveRuleParamDto() - .setId(1) - .setActiveRuleId(2) - .setRulesParameterId(3) - .setKey("newMax") - .setValue("30"); - - dao.update(dto); - - checkTables("update_parameter", "active_rule_parameters"); - } - - @Test - public void delete_parameters() { - setupData("shared"); - - dao.deleteParameters(1); - - checkTables("delete_parameters", "active_rule_parameters"); - } - - @Test - public void delete_parameter() { - setupData("shared"); - - dao.deleteParameter(1); - - checkTables("delete_parameter", "active_rule_parameters"); - } - - @Test - public void delete_parameters_from_profile_id() { - setupData("delete_parameters_from_profile_id"); - - dao.deleteParametersFromProfile(2); - - checkTables("delete_parameters_from_profile_id", "active_rule_parameters"); - } - - @Test - public void select_param_by_id() { - setupData("shared"); - - ActiveRuleParamDto result = dao.selectParamById(1); - assertThat(result.getId()).isEqualTo(1); - assertThat(result.getActiveRuleId()).isEqualTo(1); - assertThat(result.getRulesParameterId()).isEqualTo(1); - assertThat(result.getKey()).isEqualTo("max"); - assertThat(result.getValue()).isEqualTo("20"); - } - - @Test - public void select_params_by_active_rule_id() { - setupData("shared"); - - assertThat(dao.selectParamsByActiveRuleId(1)).hasSize(2); - assertThat(dao.selectParamsByActiveRuleId(2)).hasSize(1); - } - - @Test - public void select_param_by_active_rule_and_key() { - setupData("shared"); - - ActiveRuleParamDto result = dao.selectParamByActiveRuleAndKey(1, "max"); - assertThat(result.getId()).isEqualTo(1); - assertThat(result.getActiveRuleId()).isEqualTo(1); - assertThat(result.getRulesParameterId()).isEqualTo(1); - assertThat(result.getKey()).isEqualTo("max"); - assertThat(result.getValue()).isEqualTo("20"); - } - - @Test - public void select_params_by_active_rule_ids() { - setupData("shared"); - - assertThat(dao.selectParamsByActiveRuleIds(ImmutableList.of(1))).hasSize(2); - assertThat(dao.selectParamsByActiveRuleIds(ImmutableList.of(2))).hasSize(1); - assertThat(dao.selectParamsByActiveRuleIds(ImmutableList.of(1, 2))).hasSize(3); - } - @Test public void select_params_by_profile_id() { setupData("shared"); assertThat(dao.selectParamsByProfileId(1)).hasSize(2); } - - @Test - public void select_all_params() { - setupData("shared"); - - assertThat(dao.selectAllParams()).hasSize(3); - } - - private ActiveRuleDto find(final Integer id, List dtos) { - return Iterables.find(dtos, new Predicate() { - @Override - public boolean apply(ActiveRuleDto input) { - return input.getId().equals(id); - } - }); - } - } diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleService.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleService.java index 2032d2b5eb8..1831af846c7 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleService.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/ActiveRuleService.java @@ -90,12 +90,12 @@ public class ActiveRuleService implements ServerComponent { List changes = activate(activation, dbSession); if (!changes.isEmpty()) { dbSession.commit(); - previewCache.reportGlobalModification(); - } - return changes; -} finally { - dbSession.close(); - } + previewCache.reportGlobalModification(); + } + return changes; + } finally { + dbSession.close(); + } } /** diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileActiveRuleOperations.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileActiveRuleOperations.java index a3e1d98b3c1..310139320c2 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileActiveRuleOperations.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/QProfileActiveRuleOperations.java @@ -42,10 +42,6 @@ import java.util.List; import static com.google.common.collect.Lists.newArrayList; -/** - * @deprecated to be dropped in 4.4 - */ -@Deprecated public class QProfileActiveRuleOperations implements ServerComponent { private final ActiveRuleDao activeRuleDao; @@ -63,7 +59,6 @@ public class QProfileActiveRuleOperations implements ServerComponent { ActiveRuleDto createActiveRule(QualityProfileKey profileKey, RuleKey ruleKey, String severity, DbSession session) { RuleDto ruleDto = ruleDao.getByKey(session, ruleKey); - //TODO use BaseDao for profileDao QualityProfileDto profileDto = profileDao.selectByNameAndLanguage(profileKey.name(), profileKey.lang(), session); ActiveRuleDto activeRule = ActiveRuleDto.createFor(profileDto, ruleDto) .setSeverity(severity); diff --git a/sonar-server/src/main/java/org/sonar/server/qualityprofile/persistence/ActiveRuleDao.java b/sonar-server/src/main/java/org/sonar/server/qualityprofile/persistence/ActiveRuleDao.java index 1a24bb4eaa3..c34935f4bc9 100644 --- a/sonar-server/src/main/java/org/sonar/server/qualityprofile/persistence/ActiveRuleDao.java +++ b/sonar-server/src/main/java/org/sonar/server/qualityprofile/persistence/ActiveRuleDao.java @@ -78,6 +78,9 @@ public class ActiveRuleDao extends BaseDao values = doc.getNullableField(indexFields[0]); - json.name(key).beginArray().values(values).endArray(); + if (values != null) { + json.name(key).beginArray().values(values).endArray(); + } } } diff --git a/sonar-server/src/main/java/org/sonar/server/ws/ListingWs.java b/sonar-server/src/main/java/org/sonar/server/ws/ListingWs.java index 6cafb46670b..08e0b16d1d5 100644 --- a/sonar-server/src/main/java/org/sonar/server/ws/ListingWs.java +++ b/sonar-server/src/main/java/org/sonar/server/ws/ListingWs.java @@ -77,7 +77,11 @@ public class ListingWs implements WebService { @Override public void handle(Request request, Response response) throws Exception { Controller controller = context.controller(request.mandatoryParam("controller")); - Action action = controller.action(request.mandatoryParam("action")); + String actionKey = request.mandatoryParam("action"); + Action action = controller.action(actionKey); + if (action == null) { + throw new IllegalArgumentException("Action does not exist: " + actionKey); + } handleResponseExample(action, response); } }); @@ -180,11 +184,7 @@ public class ListingWs implements WebService { writer.prop("defaultValue", param.defaultValue()); writer.prop("exampleValue", param.exampleValue()); if (param.possibleValues() != null) { - writer.name("possibleValues").beginArray(); - for (String s : param.possibleValues()) { - writer.value(s); - } - writer.endArray(); + writer.name("possibleValues").beginArray().values(param.possibleValues()).endArray(); } writer.endObject(); } -- 2.39.5