From 70be4124a2c51ca9b4119d16785662e2c551989a Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Mon, 13 Jan 2014 13:37:35 +0100 Subject: [PATCH] SONAR-4923 Implements search inactive / active rules ids --- .../org/sonar/server/rule/ProfileRules.java | 208 +++++++++++------- .../sonar/server/rule/ProfileRulesTest.java | 92 +++++++- 2 files changed, 205 insertions(+), 95 deletions(-) diff --git a/sonar-server/src/main/java/org/sonar/server/rule/ProfileRules.java b/sonar-server/src/main/java/org/sonar/server/rule/ProfileRules.java index e709a8fdeec..aa1e2ff95a3 100644 --- a/sonar-server/src/main/java/org/sonar/server/rule/ProfileRules.java +++ b/sonar-server/src/main/java/org/sonar/server/rule/ProfileRules.java @@ -19,8 +19,7 @@ */ package org.sonar.server.rule; -import com.google.common.base.Function; -import com.google.common.collect.Iterables; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Maps; import org.apache.commons.lang.StringUtils; import org.elasticsearch.action.get.GetResponse; @@ -52,6 +51,8 @@ import static org.sonar.server.rule.RuleRegistry.*; public class ProfileRules implements ServerExtension { + private static final int PAGE_SIZE = 100; + private static final String FIELD_PARENT = "_parent"; private static final String FIELD_SOURCE = "_source"; @@ -78,76 +79,69 @@ public class ProfileRules implements ServerExtension { } public QProfileRuleResult searchProfileRules(ProfileRuleQuery query, Paging paging) { - SearchRequestBuilder builder = index.client().prepareSearch(INDEX_RULES).setTypes(TYPE_RULE) - .setPostFilter(ruleFilter(query) - .must(hasChildFilter(TYPE_ACTIVE_RULE, childFilter(query)))) - .setSize(paging.pageSize()) - .setFrom(paging.offset()); - addOrder(query, builder); - SearchHits ruleHits = index.executeRequest(builder); - + SearchHits ruleHits = searchRules(query, paging, ruleFilterForActiveRuleSearch(query).must(hasChildFilter(TYPE_ACTIVE_RULE, activeRuleFilter(query)))); List ruleIds = Lists.newArrayList(); - for (SearchHit ruleHit: ruleHits) { + for (SearchHit ruleHit : ruleHits) { ruleIds.add(Integer.valueOf(ruleHit.id())); } List result = Lists.newArrayList(); - if (!ruleIds.isEmpty()) { - SearchRequestBuilder activeRuleBuilder = index.client().prepareSearch(INDEX_RULES).setTypes(TYPE_ACTIVE_RULE) - .setPostFilter(boolFilter() - .must( - termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId()), - hasParentFilter(TYPE_RULE, termsFilter(RuleDocument.FIELD_ID, ruleIds)) - )) - .addFields(FIELD_SOURCE, FIELD_PARENT) - .setSize(ruleHits.getHits().length); - SearchHits activeRuleHits = index.executeRequest(activeRuleBuilder); + SearchHits activeRuleHits = searchActiveRules(query, ruleIds, FIELD_SOURCE, FIELD_PARENT); Map activeRuleByParent = Maps.newHashMap(); - for (SearchHit activeRuleHit: activeRuleHits) { + for (SearchHit activeRuleHit : activeRuleHits) { activeRuleByParent.put((String) activeRuleHit.field(FIELD_PARENT).getValue(), activeRuleHit); } - for (SearchHit ruleHit: ruleHits) { + for (SearchHit ruleHit : ruleHits) { result.add(new QProfileRule(ruleHit.sourceAsMap(), activeRuleByParent.get(ruleHit.id()).sourceAsMap())); } } return new QProfileRuleResult(result, PagingResult.create(paging.pageSize(), paging.pageIndex(), ruleHits.getTotalHits())); } - // FIXME Due to a bug in E/S, As the query filter contain a filter with has_parent, nothing will be returned - public List searchProfileRuleIds(ProfileRuleQuery query) { - BoolFilterBuilder filter = activeRuleFilter(query); + public List searchProfileRuleIds(final ProfileRuleQuery query) { + return searchProfileRuleIds(query, PAGE_SIZE); + } - SearchRequestBuilder builder = index.client() - .prepareSearch(INDEX_RULES) - .setTypes(TYPE_ACTIVE_RULE) - .setPostFilter(filter); - List documentIds = index.findDocumentIds(builder, 2); - return newArrayList(Iterables.transform(documentIds, new Function() { + @VisibleForTesting + List searchProfileRuleIds(final ProfileRuleQuery query, int pageSize) { + final List activeRuleIds = newArrayList(); + new Search(pageSize){ @Override - public Integer apply(String input) { - return Integer.valueOf(input); + public int search(int currentPage) { + Paging paging = Paging.create(pageSize, currentPage); + SearchHits ruleHits = searchRules(query, paging, ruleFilterForActiveRuleSearch(query).must(hasChildFilter(TYPE_ACTIVE_RULE, activeRuleFilter(query)))); + List ruleIds = Lists.newArrayList(); + for (SearchHit ruleHit : ruleHits) { + ruleIds.add(Integer.valueOf(ruleHit.id())); + } + + if (!ruleIds.isEmpty()) { + SearchHits activeRuleHits = searchActiveRules(query, ruleIds, ActiveRuleDocument.FIELD_ID); + for (SearchHit activeRuleHit : activeRuleHits) { + activeRuleIds.add((Integer) activeRuleHit.field(ActiveRuleDocument.FIELD_ID).getValue()); + } + } + return ruleHits.getHits().length; } - })); + }.execute(); + return activeRuleIds; } - public QProfileRuleResult searchInactiveProfileRules(ProfileRuleQuery query, Paging paging) { - BoolFilterBuilder filter = ruleFilter(query); - addMustTermOrTerms(filter, RuleDocument.FIELD_SEVERITY, query.severities()); - filter.mustNot( - hasChildFilter(TYPE_ACTIVE_RULE, - termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId()))); - - SearchRequestBuilder builder = index.client().prepareSearch(INDEX_RULES).setTypes(TYPE_RULE) - .setPostFilter(filter) - .addFields(FIELD_SOURCE, FIELD_PARENT) - .setSize(paging.pageSize()) - .setFrom(paging.offset()); - addOrder(query, builder); + public long countProfileRules(ProfileRuleQuery query) { + return index.executeCount( + index.client() + .prepareCount(INDEX_RULES) + .setTypes(TYPE_ACTIVE_RULE) + .setQuery(QueryBuilders.filteredQuery(QueryBuilders.matchAllQuery(), + activeRuleFilter(query).must(hasParentFilter(TYPE_RULE, ruleFilterForActiveRuleSearch(query))))) + ); + } - SearchHits hits = index.executeRequest(builder); + public QProfileRuleResult searchInactiveProfileRules(ProfileRuleQuery query, Paging paging) { + SearchHits hits = searchRules(query, paging, ruleFilterForInactiveRuleSearch(query), FIELD_SOURCE, FIELD_PARENT); List result = Lists.newArrayList(); for (SearchHit hit : hits.getHits()) { result.add(new QProfileRule(hit.sourceAsMap())); @@ -155,25 +149,59 @@ public class ProfileRules implements ServerExtension { return new QProfileRuleResult(result, PagingResult.create(paging.pageSize(), paging.pageIndex(), hits.getTotalHits())); } - protected BoolFilterBuilder childFilter(ProfileRuleQuery query) { - BoolFilterBuilder filter = boolFilter().must( - termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId()) - ); - addMustTermOrTerms(filter, ActiveRuleDocument.FIELD_SEVERITY, query.severities()); - String inheritance = query.inheritance(); - if (inheritance != null) { - addMustTermOrTerms(filter, ActiveRuleDocument.FIELD_INHERITANCE, newArrayList(inheritance)); - } else if (query.noInheritance()) { - filter.mustNot(getTermOrTerms(ActiveRuleDocument.FIELD_INHERITANCE, newArrayList(QProfileRule.INHERITED, QProfileRule.OVERRIDES))); + public List searchInactiveProfileRuleIds(final ProfileRuleQuery query) { + final List ruleIds = newArrayList(); + + new Search(PAGE_SIZE){ + @Override + public int search(int currentPage) { + Paging paging = Paging.create(pageSize, currentPage); + SearchHits hits = searchRules(query, paging, ruleFilterForInactiveRuleSearch(query), RuleDocument.FIELD_ID); + for (SearchHit hit : hits.getHits()) { + ruleIds.add((Integer) hit.field(RuleDocument.FIELD_ID).getValue()); + } + return hits.getHits().length; + } + }.execute(); + + return ruleIds; + } + + public long countInactiveProfileRules(ProfileRuleQuery query) { + return index.executeCount(index.client().prepareCount(INDEX_RULES).setTypes(TYPE_RULE) + .setQuery(QueryBuilders.filteredQuery(QueryBuilders.matchAllQuery(), + boolFilter() + .must(ruleFilterForInactiveRuleSearch(query))))); + } + + private SearchHits searchRules(ProfileRuleQuery query, Paging paging, FilterBuilder filterBuilder, String... fields) { + SearchRequestBuilder builder = index.client().prepareSearch(INDEX_RULES).setTypes(TYPE_RULE) + .setPostFilter(filterBuilder) + .setSize(paging.pageSize()) + .setFrom(paging.offset()); + if (fields.length > 0) { + builder.addFields(fields); } - return filter; + addOrder(query, builder); + return index.executeRequest(builder); } - protected BoolFilterBuilder activeRuleFilter(ProfileRuleQuery query) { - BoolFilterBuilder filter = boolFilter().must( - termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId()), - hasParentFilter(TYPE_RULE, ruleFilter(query)) - ); + private SearchHits searchActiveRules(ProfileRuleQuery query, List ruleIds, String... fields) { + SearchRequestBuilder activeRuleBuilder = index.client().prepareSearch(INDEX_RULES).setTypes(TYPE_ACTIVE_RULE) + .setPostFilter(boolFilter() + .must( + termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId()), + hasParentFilter(TYPE_RULE, termsFilter(RuleDocument.FIELD_ID, ruleIds)) + )) + .setSize(ruleIds.size()); + if (fields.length > 0) { + activeRuleBuilder.addFields(fields); + } + return index.executeRequest(activeRuleBuilder); + } + + private BoolFilterBuilder activeRuleFilter(ProfileRuleQuery query) { + BoolFilterBuilder filter = boolFilter().must(termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId())); addMustTermOrTerms(filter, ActiveRuleDocument.FIELD_SEVERITY, query.severities()); String inheritance = query.inheritance(); if (inheritance != null) { @@ -184,24 +212,7 @@ public class ProfileRules implements ServerExtension { return filter; } - public long countProfileRules(ProfileRuleQuery query) { - return index.executeCount( - index.client() - .prepareCount(INDEX_RULES) - .setTypes(TYPE_ACTIVE_RULE) - .setQuery(QueryBuilders.filteredQuery(QueryBuilders.matchAllQuery(), activeRuleFilter(query))) - ); - } - - public long countInactiveProfileRules(ProfileRuleQuery query) { - return index.executeCount(index.client().prepareCount(INDEX_RULES).setTypes(TYPE_RULE) - .setQuery(QueryBuilders.filteredQuery(QueryBuilders.matchAllQuery(), - boolFilter() - .must(ruleFilter(query)) - .mustNot(hasChildFilter(TYPE_ACTIVE_RULE, termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId())))))); - } - - private BoolFilterBuilder ruleFilter(ProfileRuleQuery query) { + private BoolFilterBuilder ruleFilterForActiveRuleSearch(ProfileRuleQuery query) { BoolFilterBuilder result = boolFilter(); if (StringUtils.isNotBlank(query.language())) { @@ -225,6 +236,13 @@ public class ProfileRules implements ServerExtension { return result; } + private BoolFilterBuilder ruleFilterForInactiveRuleSearch(ProfileRuleQuery query) { + BoolFilterBuilder filter = ruleFilterForActiveRuleSearch(query) + .mustNot(hasChildFilter(TYPE_ACTIVE_RULE, termFilter(ActiveRuleDocument.FIELD_PROFILE_ID, query.profileId()))); + addMustTermOrTerms(filter, RuleDocument.FIELD_SEVERITY, query.severities()); + return filter; + } + private void addMustTermOrTerms(BoolFilterBuilder filter, String field, Collection terms) { FilterBuilder termOrTerms = getTermOrTerms(field, terms); if (termOrTerms != null) { @@ -252,4 +270,28 @@ public class ProfileRules implements ServerExtension { builder.addSort(RuleDocument.FIELD_CREATED_AT, sortOrder); } } + + private abstract static class Search { + + int pageSize = 100; + + protected Search(int pageSize) { + this.pageSize = pageSize; + } + + abstract int search(int currentPage); + + void execute() { + int currentPage = 1; + boolean hasNextPage = true; + while (hasNextPage) { + int resultSize = search(currentPage); + if (resultSize < pageSize) { + hasNextPage = false; + } else { + currentPage++; + } + } + } + } } diff --git a/sonar-server/src/test/java/org/sonar/server/rule/ProfileRulesTest.java b/sonar-server/src/test/java/org/sonar/server/rule/ProfileRulesTest.java index b4f4b27e37c..6121c111660 100644 --- a/sonar-server/src/test/java/org/sonar/server/rule/ProfileRulesTest.java +++ b/sonar-server/src/test/java/org/sonar/server/rule/ProfileRulesTest.java @@ -24,7 +24,6 @@ import org.apache.commons.io.IOUtils; import org.elasticsearch.client.Requests; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.sonar.api.config.Settings; import org.sonar.api.rule.Severity; @@ -63,7 +62,7 @@ public class ProfileRulesTest { profileRules = new ProfileRules(index); esSetup.client().prepareBulk() - // On profile 1 + // On profile 1 .add(Requests.indexRequest().index("rules").type("rule").source(testFileAsString("should_find_active_rules/rule25.json"))) .add(Requests.indexRequest().index("rules").type("active_rule").parent("25").source(testFileAsString("should_find_active_rules/active_rule25.json"))) // On profile 1 and 2 @@ -76,7 +75,7 @@ public class ProfileRulesTest { // Rules on no profile .add(Requests.indexRequest().index("rules").type("rule").source(testFileAsString("should_find_active_rules/rule944.json"))) .add(Requests.indexRequest().index("rules").type("rule").source(testFileAsString("should_find_active_rules/rule719.json"))) - // Removed rule + // Removed rule .add(Requests.indexRequest().index("rules").type("rule").source(testFileAsString("should_find_active_rules/rule860.json"))) .setRefresh(true).execute().actionGet(); } @@ -102,12 +101,6 @@ public class ProfileRulesTest { assertThat(rules2.get(0).id()).isEqualTo(759); assertThat(rules2.get(0).activeRuleId()).isEqualTo(523); - // Inexistent profile - assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(3), paging).rules()).hasSize(0); - - // Inexistent name/key - assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(1).setNameOrKey("polop"), paging).rules()).hasSize(0); - // Match on key assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(1).setNameOrKey("DM_CONVERT_CASE"), paging).rules()).hasSize(1); @@ -117,10 +110,20 @@ public class ProfileRulesTest { // Match on repositoryKey assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(1).addRepositoryKeys("findbugs"), paging).rules()).hasSize(1); + assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(1).addSeverities(Severity.CRITICAL), paging).rules()).hasSize(1); + // Active rule 25 is in MINOR (rule 25 is in INFO) + assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(1).addSeverities(Severity.INFO), paging).rules()).isEmpty(); + // Match on key, rule has parameters List rulesWParam = profileRules.searchProfileRules(ProfileRuleQuery.create(1).setNameOrKey("ArchitecturalConstraint"), paging).rules(); assertThat(rulesWParam).hasSize(1); assertThat(rulesWParam.get(0).params()).hasSize(2); + + // Inexistent profile + assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(3), paging).rules()).hasSize(0); + + // Inexistent name/key + assertThat(profileRules.searchProfileRules(ProfileRuleQuery.create(1).setNameOrKey("polop"), paging).rules()).hasSize(0); } @Test @@ -181,12 +184,44 @@ public class ProfileRulesTest { } @Test - @Ignore("bug in E/S : fail to do a scroll when filter contain has_parent -> return good total_hits but hits is empty") + public void find_profile_rules_with_paging() { + List rules = profileRules.searchProfileRules(ProfileRuleQuery.create(1), Paging.create(2, 1)).rules(); + assertThat(rules).hasSize(2); + assertThat(rules.get(0).key()).isEqualTo("ArchitecturalConstraint"); + assertThat(rules.get(1).key()).isEqualTo("DM_CONVERT_CASE"); + + rules = profileRules.searchProfileRules(ProfileRuleQuery.create(1), Paging.create(2, 2)).rules(); + assertThat(rules).hasSize(1); + assertThat(rules.get(0).key()).isEqualTo("UnusedNullCheckInEquals"); + } + + @Test public void find_profile_rule_ids() { - // All rules for profile 1 + // All active rules for profile 1 List result = profileRules.searchProfileRuleIds(ProfileRuleQuery.create(1)); assertThat(result).hasSize(3); - assertThat(result.get(0)).isEqualTo(1); + assertThat(result).containsOnly(25, 391, 2702); + + assertThat(profileRules.searchProfileRuleIds(ProfileRuleQuery.create(1).addSeverities(Severity.CRITICAL))).hasSize(1); + assertThat(profileRules.searchProfileRuleIds(ProfileRuleQuery.create(1).addSeverities(Severity.INFO))).isEmpty(); + } + + @Test + public void find_profile_rule_ids_overriding_page_size() { + List result = profileRules.searchProfileRuleIds(ProfileRuleQuery.create(1), 1); + assertThat(result).hasSize(3); + } + + @Test + public void count_profile_rules() { + // All rules for profile 1 + assertThat(profileRules.countProfileRules(ProfileRuleQuery.create(1))).isEqualTo(3); + + // All rules for profile 2 + assertThat(profileRules.countProfileRules(ProfileRuleQuery.create(2))).isEqualTo(1); + + // Match on key + assertThat(profileRules.countProfileRules(ProfileRuleQuery.create(1).setNameOrKey("DM_CONVERT_CASE"))).isEqualTo(1); } @Test @@ -201,6 +236,9 @@ public class ProfileRulesTest { // Match on key assertThat(profileRules.searchInactiveProfileRules(ProfileRuleQuery.create(2).setNameOrKey("Boolean expressions"), paging).rules()).hasSize(1); + + // Mach on severity + assertThat(profileRules.searchInactiveProfileRules(ProfileRuleQuery.create(2).addSeverities(Severity.INFO), paging).rules()).hasSize(1); } @Test @@ -233,6 +271,36 @@ public class ProfileRulesTest { assertThat(rules.get(1).key()).isEqualTo("com.puppycrawl.tools.checkstyle.checks.coding.DoubleCheckedLockingCheck"); } + @Test + public void find_inactive_profile_rule_ids() { + // Search of inactive rule on profile 2 + assertThat(profileRules.searchInactiveProfileRuleIds(ProfileRuleQuery.create(2))).hasSize(4); + + // Search of inactive rule on profile 1 + assertThat(profileRules.searchInactiveProfileRuleIds(ProfileRuleQuery.create(1))).hasSize(2); + + // Match on key + assertThat(profileRules.searchInactiveProfileRuleIds(ProfileRuleQuery.create(2).setNameOrKey("Boolean expressions"))).hasSize(1); + + // Mach on severity + assertThat(profileRules.searchInactiveProfileRuleIds(ProfileRuleQuery.create(2).addSeverities(Severity.INFO))).hasSize(1); + } + + @Test + public void count_inactive_profile_rules() { + // All rules for profile 1 + assertThat(profileRules.countInactiveProfileRules(ProfileRuleQuery.create(2))).isEqualTo(4); + + // All rules for profile 2 + assertThat(profileRules.countInactiveProfileRules(ProfileRuleQuery.create(1))).isEqualTo(2); + + // Match on key + assertThat(profileRules.countInactiveProfileRules(ProfileRuleQuery.create(2).setNameOrKey("Boolean expressions"))).isEqualTo(1); + + // Mach on severity + assertThat(profileRules.countInactiveProfileRules(ProfileRuleQuery.create(2).addSeverities(Severity.INFO))).isEqualTo(1); + } + @Test public void get_from_active_rule() { assertThat(profileRules.getFromActiveRuleId(391)).isNotNull(); -- 2.39.5