From 2b263da9c8b0eb6fd824934edd4a3033d75d75f5 Mon Sep 17 00:00:00 2001 From: Daniel Schwarz Date: Thu, 6 Apr 2017 11:18:47 +0200 Subject: [PATCH] SONAR-8950 add selected tags to api/rules/search tags facet --- .../sonar/server/es/StickyFacetBuilder.java | 13 ++- .../sonar/server/issue/index/IssueIndex.java | 4 +- .../sonar/server/rule/index/RuleIndex.java | 2 +- .../server/rule/index/RuleIndexTest.java | 107 ++++++++++++++++-- .../server/rule/ws/SearchActionTest.java | 96 +++++++++++----- 5 files changed, 174 insertions(+), 48 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/es/StickyFacetBuilder.java b/server/sonar-server/src/main/java/org/sonar/server/es/StickyFacetBuilder.java index 805aa576db4..e8e398929d6 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/es/StickyFacetBuilder.java +++ b/server/sonar-server/src/main/java/org/sonar/server/es/StickyFacetBuilder.java @@ -92,7 +92,7 @@ public class StickyFacetBuilder { Object... selected) { BoolQueryBuilder facetFilter = getStickyFacetFilter(fieldName); FilterAggregationBuilder facetTopAggregation = buildTopFacetAggregation(fieldName, facetName, facetFilter, size, additionalAggregationFilter); - facetTopAggregation = addSelectedItemsToFacet(fieldName, facetName, facetTopAggregation, selected); + facetTopAggregation = addSelectedItemsToFacet(fieldName, facetName, facetTopAggregation, additionalAggregationFilter, selected); return AggregationBuilders .global(facetName) @@ -116,11 +116,11 @@ public class StickyFacetBuilder { private FilterAggregationBuilder buildTopFacetAggregation(String fieldName, String facetName, BoolQueryBuilder facetFilter, int size, Function> additionalAggregationFilter) { TermsBuilder termsAggregation = buildTermsFacetAggregation(fieldName, facetName, size); - AggregationBuilder innerAggregation = additionalAggregationFilter.apply(termsAggregation); + AggregationBuilder improvedAggregation = additionalAggregationFilter.apply(termsAggregation); return AggregationBuilders .filter(facetName + "_filter") .filter(facetFilter) - .subAggregation(innerAggregation); + .subAggregation(improvedAggregation); } private TermsBuilder buildTermsFacetAggregation(String fieldName, String facetName, int size) { @@ -135,7 +135,8 @@ public class StickyFacetBuilder { return termsAggregation; } - public FilterAggregationBuilder addSelectedItemsToFacet(String fieldName, String facetName, FilterAggregationBuilder facetTopAggregation, Object... selected) { + public FilterAggregationBuilder addSelectedItemsToFacet(String fieldName, String facetName, FilterAggregationBuilder facetTopAggregation, + Function> additionalAggregationFilter, Object... selected) { if (selected.length <= 0) { return facetTopAggregation; } @@ -150,7 +151,9 @@ public class StickyFacetBuilder { if (subAggregation != null) { selectedTerms = selectedTerms.subAggregation(subAggregation); } - facetTopAggregation.subAggregation(selectedTerms); + + AggregationBuilder improvedAggregation = additionalAggregationFilter.apply(selectedTerms); + facetTopAggregation.subAggregation(improvedAggregation); return facetTopAggregation; } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java index 2fce4dcafc5..28818f9e6c3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/index/IssueIndex.java @@ -510,7 +510,7 @@ public class IssueIndex { Collection assigneesEscaped = escapeValuesForFacetInclusion(query.assignees()); if (!assigneesEscaped.isEmpty()) { - facetTopAggregation = assigneeFacetBuilder.addSelectedItemsToFacet(fieldName, facetName, facetTopAggregation, assigneesEscaped.toArray()); + facetTopAggregation = assigneeFacetBuilder.addSelectedItemsToFacet(fieldName, facetName, facetTopAggregation, t -> t, assigneesEscaped.toArray()); } // Add missing facet for unassigned issues @@ -568,7 +568,7 @@ public class IssueIndex { StickyFacetBuilder assigneeFacetBuilder = newStickyFacetBuilder(query, resolutionFilters, esQuery); BoolQueryBuilder facetFilter = assigneeFacetBuilder.getStickyFacetFilter(fieldName); FilterAggregationBuilder facetTopAggregation = assigneeFacetBuilder.buildTopFacetAggregation(fieldName, facetName, facetFilter, DEFAULT_FACET_SIZE); - facetTopAggregation = assigneeFacetBuilder.addSelectedItemsToFacet(fieldName, facetName, facetTopAggregation); + facetTopAggregation = assigneeFacetBuilder.addSelectedItemsToFacet(fieldName, facetName, facetTopAggregation, t -> t); // Add missing facet for unresolved issues facetTopAggregation.subAggregation( diff --git a/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java b/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java index a8c3b71beb5..d9b2fa048b2 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java +++ b/server/sonar-server/src/main/java/org/sonar/server/rule/index/RuleIndex.java @@ -379,7 +379,7 @@ public class RuleIndex { RuleExtensionScope.organization(organizationUuid).getScope())) .subAggregation(termsAggregation); - return AggregationBuilders.children("children_for_" + FACET_TAGS) + return AggregationBuilders.children("children_for_" + termsAggregation.getName()) .childType(INDEX_TYPE_RULE_EXTENSION.getType()) .subAggregation(scopeAggregation); }; diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexTest.java index 80d91df9425..891c4f2cd11 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/index/RuleIndexTest.java @@ -49,7 +49,7 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptySet; import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.data.MapEntry.entry; +import static org.assertj.guava.api.Assertions.entry; import static org.junit.Assert.fail; import static org.sonar.api.rule.Severity.BLOCKER; import static org.sonar.api.rule.Severity.CRITICAL; @@ -569,7 +569,7 @@ public class RuleIndexTest { } @Test - public void all_tags() { + public void listTags_should_return_both_system_tags_and_organization_specific_tags() { OrganizationDto organization = dbTester.organizations().insert(); RuleDefinitionDto rule1 = createRule(setSystemTags("sys1", "sys2")); @@ -586,7 +586,7 @@ public class RuleIndexTest { } @Test - public void all_tags_minds_the_oranization() { + public void listTags_must_not_return_tags_of_other_organizations() { OrganizationDto organization1 = dbTester.organizations().insert(); RuleDefinitionDto rule1 = createRule(setSystemTags("sys1")); createRuleMetadata(rule1, organization1, @@ -599,8 +599,11 @@ public class RuleIndexTest { setOrganizationUuid(organization2.getUuid()), setTags("tag2")); + OrganizationDto organization3 = dbTester.organizations().insert(); + assertThat(index.listTags(organization1, null, 10)).containsOnly("tag1", "sys1", "sys2"); assertThat(index.listTags(organization2, null, 10)).containsOnly("tag2", "sys1", "sys2"); + assertThat(index.listTags(organization3, null, 10)).containsOnly("sys1", "sys2"); } @Test @@ -722,8 +725,7 @@ public class RuleIndexTest { OrganizationDto organization = dbTester.organizations().insert(); RuleDefinitionDto rule = insertRuleDefinition(setSystemTags()); - dbTester.rules().insertOrUpdateMetadata(rule, organization, setTags("bla")); - ruleIndexer.indexRuleExtension(organization, rule.getKey()); + insertRuleMetaData(organization, rule, setTags("bla")); RuleQuery query = new RuleQuery() .setOrganizationUuid(organization.getUuid()); @@ -733,20 +735,61 @@ public class RuleIndexTest { assertThat(result.getFacets().get(FACET_TAGS)).contains(entry("bla", 1L)); } + @Test + public void tags_facet_should_return_top_10_items() { + // default number of items returned in facets = 10 + RuleDefinitionDto rule = insertRuleDefinition(setSystemTags("tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7", "tag8", "tag9", "tagA", "tagB")); + + RuleQuery query = new RuleQuery() + .setOrganizationUuid(dbTester.getDefaultOrganization().getUuid()); + SearchOptions options = new SearchOptions().addFacets(singletonList(FACET_TAGS)); + SearchIdResult result = index.search(query, options); + assertThat(result.getFacets().get(FACET_TAGS)).containsExactly(entry("tag1", 1L), entry("tag2", 1L), entry("tag3", 1L), entry("tag4", 1L), entry("tag5", 1L), + entry("tag6", 1L), entry("tag7", 1L), entry("tag8", 1L), entry("tag9", 1L), entry("tagA", 1L)); + } + + @Test + public void tags_facet_should_include_matching_selected_items() { + // default number of items returned in facets = 10 + RuleDefinitionDto rule = insertRuleDefinition(setSystemTags("tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7", "tag8", "tag9", "tagA", "tagB")); + + RuleQuery query = new RuleQuery() + .setOrganizationUuid(dbTester.getDefaultOrganization().getUuid()) + .setTags(singletonList("tagB")); + SearchOptions options = new SearchOptions().addFacets(singletonList(FACET_TAGS)); + SearchIdResult result = index.search(query, options); + assertThat(result.getFacets().get(FACET_TAGS).entrySet()).extracting(e -> entry(e.getKey(), e.getValue())).containsExactly( + + // check that selected item is added, although there are 10 other items + entry("tagB", 1L), + + entry("tag1", 1L), entry("tag2", 1L), entry("tag3", 1L), entry("tag4", 1L), entry("tag5", 1L), entry("tag6", 1L), entry("tag7", 1L), entry("tag8", 1L), entry("tag9", 1L), + entry("tagA", 1L)); + } + @Test public void tags_facet_should_not_find_tags_of_any_other_organization() { OrganizationDto organization1 = dbTester.organizations().insert(); OrganizationDto organization2 = dbTester.organizations().insert(); RuleDefinitionDto rule = insertRuleDefinition(setSystemTags()); - dbTester.rules().insertOrUpdateMetadata(rule, organization1, setTags("bla")); + insertRuleMetaData(organization1, rule, setTags("bla1")); + insertRuleMetaData(organization2, rule, setTags("bla2")); RuleQuery query = new RuleQuery() .setOrganizationUuid(organization2.getUuid()); SearchOptions options = new SearchOptions().addFacets(singletonList(FACET_TAGS)); SearchIdResult result = index.search(query, options); - assertThat(result.getFacets().get(FACET_TAGS)).contains(); + assertThat(result.getFacets().get(FACET_TAGS).entrySet()).extracting(e -> entry(e.getKey(), e.getValue())).containsExactly( + entry("bla2", 1L) + ); + } + + @SafeVarargs + private final void insertRuleMetaData(OrganizationDto organization, RuleDefinitionDto rule, Consumer... consumers) { + dbTester.rules().insertOrUpdateMetadata(rule, organization, consumers); + ruleIndexer.indexRuleExtension(organization, rule.getKey()); } @Test @@ -809,7 +852,7 @@ public class RuleIndexTest { .setTypes(asList(BUG, CODE_SMELL)); SearchIdResult result = index.search(query, new SearchOptions().addFacets(asList(FACET_LANGUAGES, FACET_REPOSITORIES, FACET_TAGS, - FACET_TYPES))); + FACET_TYPES))); assertThat(result.getIds()).hasSize(2); assertThat(result.getFacets().getAll()).hasSize(4); assertThat(result.getFacets().get(FACET_LANGUAGES).keySet()).containsOnly("cpp", "java"); @@ -922,16 +965,56 @@ public class RuleIndexTest { assertThat(tester.countDocuments(INDEX_TYPE_ACTIVE_RULE)).isEqualTo(3); // 1. get all active rules. - assertThat(index.searchAll(new RuleQuery().setActivation(true))).containsOnly(rule1Key, rule2Key); + assertThat(index.searchAll(new RuleQuery().setActivation(true))) + .containsOnly(rule1Key, rule2Key); + + // 2. get all inactive rules. + assertThat(index.searchAll(new RuleQuery().setActivation(false))) + .containsOnly(rule3Key); + + // 3. get all rules not active on profile + assertThat(index.searchAll(new RuleQuery().setActivation(false).setQProfileKey(QUALITY_PROFILE_KEY2))) + .containsOnly(rule2Key, rule3Key); + + // 4. get all active rules on profile + assertThat(index.searchAll(new RuleQuery().setActivation(true).setQProfileKey(QUALITY_PROFILE_KEY2))) + .containsOnly(rule1Key); + } + + @Test + public void search_all_keys_by_profile_in_specific_organization() { + RuleDefinitionDto rule1 = insertRuleDefinition(); + RuleDefinitionDto rule2 = insertRuleDefinition(); + RuleDefinitionDto rule3 = insertRuleDefinition(); + + RuleKey rule1Key = rule1.getKey(); + RuleKey rule2Key = rule2.getKey(); + RuleKey rule3Key = rule3.getKey(); + + OrganizationDto organization = dbTester.organizations().insert(); + + indexActiveRules( + ActiveRuleDocTesting.newDoc(ActiveRuleKey.of(QUALITY_PROFILE_KEY1, rule1Key)).setOrganizationUuid(organization.getUuid()), + ActiveRuleDocTesting.newDoc(ActiveRuleKey.of(QUALITY_PROFILE_KEY2, rule1Key)).setOrganizationUuid(organization.getUuid()), + ActiveRuleDocTesting.newDoc(ActiveRuleKey.of(QUALITY_PROFILE_KEY1, rule2Key)).setOrganizationUuid(organization.getUuid())); + + assertThat(tester.countDocuments(INDEX_TYPE_ACTIVE_RULE)).isEqualTo(3); + + // 1. get all active rules. + assertThat(index.searchAll(new RuleQuery().setActivation(true).setOrganizationUuid(organization.getUuid()))) + .containsOnly(rule1Key, rule2Key); // 2. get all inactive rules. - assertThat(index.searchAll(new RuleQuery().setActivation(false))).containsOnly(rule3Key); + assertThat(index.searchAll(new RuleQuery().setActivation(false).setOrganizationUuid(organization.getUuid()))) + .containsOnly(rule3Key); // 3. get all rules not active on profile - assertThat(index.searchAll(new RuleQuery().setActivation(false).setQProfileKey(QUALITY_PROFILE_KEY2))).containsOnly(rule2Key, rule3Key); + assertThat(index.searchAll(new RuleQuery().setActivation(false).setOrganizationUuid(organization.getUuid()).setQProfileKey(QUALITY_PROFILE_KEY2))) + .containsOnly(rule2Key, rule3Key); // 4. get all active rules on profile - assertThat(index.searchAll(new RuleQuery().setActivation(true).setQProfileKey(QUALITY_PROFILE_KEY2))).containsOnly(rule1Key); + assertThat(index.searchAll(new RuleQuery().setActivation(true).setOrganizationUuid(organization.getUuid()).setQProfileKey(QUALITY_PROFILE_KEY2))) + .containsOnly(rule1Key); } @SafeVarargs diff --git a/server/sonar-server/src/test/java/org/sonar/server/rule/ws/SearchActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/rule/ws/SearchActionTest.java index 4f86c58a565..21a54dacbb6 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/rule/ws/SearchActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/rule/ws/SearchActionTest.java @@ -25,6 +25,7 @@ import java.io.InputStream; import java.util.function.Consumer; import java.util.stream.Collectors; import org.assertj.core.api.iterable.Extractor; +import org.junit.Before; import org.junit.Test; import org.sonar.api.config.MapSettings; import org.sonar.api.resources.Languages; @@ -48,9 +49,11 @@ import org.sonarqube.ws.Rules.Rule; import org.sonarqube.ws.Rules.SearchResponse; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.guava.api.Assertions.entry; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.sonar.db.rule.RuleTesting.setSystemTags; import static org.sonar.db.rule.RuleTesting.setTags; import static org.sonarqube.ws.MediaTypes.PROTOBUF; @@ -77,9 +80,14 @@ public class SearchActionTest { private RuleIndexer ruleIndexer = new RuleIndexer(esClient, dbClient); + @Before + public void before() { + doReturn("interpreted").when(macroInterpreter).interpret(anyString()); + } + @Test public void should_find_rule() throws IOException { - RuleDefinitionDto rule = insertRule(); + RuleDefinitionDto rule = insertRuleDefinition(); doReturn("interpreted").when(macroInterpreter).interpret(anyString()); @@ -94,9 +102,9 @@ public class SearchActionTest { @Test public void should_filter_on_organization_specific_tags() throws IOException { OrganizationDto organization = dbTester.organizations().insert(); - RuleDefinitionDto rule1 = insertRule(); + RuleDefinitionDto rule1 = insertRuleDefinition(); RuleMetadataDto metadata1 = insertMetadata(organization, rule1, setTags("tag1", "tag2")); - RuleDefinitionDto rule2 = insertRule(); + RuleDefinitionDto rule2 = insertRuleDefinition(); RuleMetadataDto metadata2 = insertMetadata(organization, rule2); InputStream inputStream = actionTester.newRequest() @@ -106,19 +114,60 @@ public class SearchActionTest { .setParam("organization", organization.getKey()) .execute() .getInputStream(); - SearchResponse result; - try { - result = SearchResponse.parseFrom(inputStream); - } catch (IOException e) { - throw new RuntimeException("Cannot parse search response", e); - } + SearchResponse result = SearchResponse.parseFrom(inputStream); assertThat(result.getRulesList()).extracting(Rule::getKey).containsExactly(rule1.getKey().toString()); } + @Test + public void should_list_tags_in_tags_facet() throws IOException { + OrganizationDto organization = dbTester.organizations().insert(); + RuleDefinitionDto rule = insertRuleDefinition(setSystemTags("tag1", "tag3", "tag5", "tag7", "tag9", "x")); + RuleMetadataDto metadata = insertMetadata(organization, rule, setTags("tag2", "tag4", "tag6", "tag8", "tagA")); + + InputStream inputStream = actionTester.newRequest() + .setMediaType(PROTOBUF) + .setParam("facets", "tags") + .setParam("organization", organization.getKey()) + .execute() + .getInputStream(); + SearchResponse result = SearchResponse.parseFrom(inputStream); + assertThat(result.getFacets().getFacets(0).getValuesList()).extracting(v -> entry(v.getVal(), v.getCount())) + .containsExactly(entry("tag1", 1L), entry("tag2", 1L), entry("tag3", 1L), entry("tag4", 1L), entry("tag5", 1L), entry("tag6", 1L), entry("tag7", 1L), entry("tag8", 1L), + entry("tag9", 1L), entry("tagA", 1L)); + } + + @Test + public void should_include_selected_matching_tag_in_facet() throws IOException { + insertRuleDefinition(setSystemTags("tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7", "tag8", "tag9", "tagA", "x")); + + InputStream inputStream = actionTester.newRequest() + .setMediaType(PROTOBUF) + .setParam("facets", "tags") + .setParam("tags", "x") + .execute() + .getInputStream(); + SearchResponse result = SearchResponse.parseFrom(inputStream); + assertThat(result.getFacets().getFacets(0).getValuesList()).extracting(v -> entry(v.getVal(), v.getCount())).contains(entry("x", 1L)); + } + + @Test + public void should_included_selected_non_matching_tag_in_facet() throws IOException { + insertRuleDefinition(setSystemTags("tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7", "tag8", "tag9", "tagA")); + + InputStream inputStream = actionTester.newRequest() + .setMediaType(PROTOBUF) + .setParam("facets", "tags") + .setParam("tags", "x") + .execute() + .getInputStream(); + SearchResponse result = SearchResponse.parseFrom(inputStream); + assertThat(result.getFacets().getFacets(0).getValuesList()).extracting(v -> entry(v.getVal(), v.getCount())).contains(entry("x", 0L)); + } + @Test public void should_return_organization_specific_tags() throws IOException { OrganizationDto organization = dbTester.organizations().insert(); - RuleDefinitionDto rule = insertRule(); + RuleDefinitionDto rule = insertRuleDefinition(); RuleMetadataDto metadata = insertMetadata(organization, rule, setTags("tag1", "tag2")); InputStream inputStream = actionTester.newRequest() @@ -127,12 +176,7 @@ public class SearchActionTest { .setParam("organization", organization.getKey()) .execute() .getInputStream(); - SearchResponse result; - try { - result = SearchResponse.parseFrom(inputStream); - } catch (IOException e) { - throw new RuntimeException("Cannot parse search response", e); - } + SearchResponse result = SearchResponse.parseFrom(inputStream); assertThat(result.getRulesList()).extracting(Rule::getKey).containsExactly(rule.getKey().toString()); assertThat(result.getRulesList()) .extracting(Rule::getTags).flatExtracting(Rules.Tags::getTagsList) @@ -141,7 +185,7 @@ public class SearchActionTest { @Test public void should_return_specified_fields() throws IOException { - RuleDefinitionDto rule = insertRule(); + RuleDefinitionDto rule = insertRuleDefinition(); checkField(rule, "repo", Rule::getRepo, rule.getRepositoryKey()); checkField(rule, "name", Rule::getName, rule.getName()); @@ -159,26 +203,22 @@ public class SearchActionTest { } @SafeVarargs - private final void checkField(RuleDefinitionDto rule, String fieldName, Extractor responseExtractor, T... expected) { + private final void checkField(RuleDefinitionDto rule, String fieldName, Extractor responseExtractor, T... expected) throws IOException { InputStream inputStream = actionTester.newRequest() .setMediaType(PROTOBUF) .setParam("f", fieldName) .execute() .getInputStream(); - SearchResponse result; - try { - result = SearchResponse.parseFrom(inputStream); - } catch (IOException e) { - throw new RuntimeException("Cannot parse search response", e); - } + SearchResponse result = SearchResponse.parseFrom(inputStream); assertThat(result.getRulesList()).extracting(Rule::getKey).containsExactly(rule.getKey().toString()); assertThat(result.getRulesList()).extracting(responseExtractor).containsExactly(expected); } - private RuleDefinitionDto insertRule() { - RuleDefinitionDto rule = dbTester.rules().insert(); - ruleIndexer.indexRuleDefinition(rule.getKey()); - return rule; + @SafeVarargs + private final RuleDefinitionDto insertRuleDefinition(Consumer... populaters) { + RuleDefinitionDto ruleDefinitionDto = dbTester.rules().insert(populaters); + ruleIndexer.indexRuleDefinition(ruleDefinitionDto.getKey()); + return ruleDefinitionDto; } @SafeVarargs -- 2.39.5