From 6c7dd4cd5b87af9b1057f725ee6b21b2e8feaa73 Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Thu, 18 Nov 2021 11:24:55 -0600 Subject: [PATCH] SONAR-14315 Cannot paginate /api/project_tags/search --- .../org/sonar/server/rule/RegisterRules.java | 3 +- .../measure/index/ProjectMeasuresIndex.java | 11 ++-- .../index/ProjectMeasuresIndexTest.java | 62 ++++++++++++++++--- .../server/projecttag/ws/SearchAction.java | 7 ++- .../projecttag/ws/SearchActionTest.java | 2 +- 5 files changed, 68 insertions(+), 17 deletions(-) diff --git a/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java b/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java index bab17bc450c..3857596e28a 100644 --- a/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java +++ b/server/sonar-webserver-core/src/main/java/org/sonar/server/rule/RegisterRules.java @@ -745,8 +745,7 @@ public class RegisterRules implements Startable { * The side effect of this approach is that extended repositories will not be managed the same way. * If an extended repository do not exists anymore, then related active rules will be removed. */ - private List removeActiveRulesOnStillExistingRepositories(DbSession dbSession, RegisterRulesContext recorder, - List context) { + private List removeActiveRulesOnStillExistingRepositories(DbSession dbSession, RegisterRulesContext recorder, List context) { List repositoryKeys = context.stream() .map(RulesDefinition.ExtendedRepository::key) .collect(MoreCollectors.toList(context.size())); diff --git a/server/sonar-webserver-es/src/main/java/org/sonar/server/measure/index/ProjectMeasuresIndex.java b/server/sonar-webserver-es/src/main/java/org/sonar/server/measure/index/ProjectMeasuresIndex.java index 2eafd49b2e6..5b20ee0d1ef 100644 --- a/server/sonar-webserver-es/src/main/java/org/sonar/server/measure/index/ProjectMeasuresIndex.java +++ b/server/sonar-webserver-es/src/main/java/org/sonar/server/measure/index/ProjectMeasuresIndex.java @@ -435,16 +435,19 @@ public class ProjectMeasuresIndex { } } - public List searchTags(@Nullable String textQuery, int size) { - int maxPageSize = 500; + public List searchTags(@Nullable String textQuery, int page, int size) { + int maxPageSize = 100; + int maxPage = 20; checkArgument(size <= maxPageSize, "Page size must be lower than or equals to " + maxPageSize); + checkArgument(page > 0 && page <= maxPage, "Page must be between 0 and " + maxPage); + if (size <= 0) { return emptyList(); } TermsAggregationBuilder tagFacet = AggregationBuilders.terms(FIELD_TAGS) .field(FIELD_TAGS) - .size(size) + .size(size*page) .minDocCount(1) .order(BucketOrder.key(true)); if (textQuery != null) { @@ -454,7 +457,6 @@ public class ProjectMeasuresIndex { SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() .query(authorizationTypeSupport.createQueryFilter()) .fetchSource(false) - .size(0) .aggregation(tagFacet); SearchResponse response = client.search(EsClient.prepareSearch(TYPE_PROJECT_MEASURES.getMainType()) @@ -463,6 +465,7 @@ public class ProjectMeasuresIndex { Terms aggregation = response.getAggregations().get(FIELD_TAGS); return aggregation.getBuckets().stream() + .skip((page-1) * size) .map(Bucket::getKeyAsString) .collect(MoreCollectors.toList()); } diff --git a/server/sonar-webserver-es/src/test/java/org/sonar/server/measure/index/ProjectMeasuresIndexTest.java b/server/sonar-webserver-es/src/test/java/org/sonar/server/measure/index/ProjectMeasuresIndexTest.java index 87fe834978c..4510f12b08d 100644 --- a/server/sonar-webserver-es/src/test/java/org/sonar/server/measure/index/ProjectMeasuresIndexTest.java +++ b/server/sonar-webserver-es/src/test/java/org/sonar/server/measure/index/ProjectMeasuresIndexTest.java @@ -1517,7 +1517,7 @@ public class ProjectMeasuresIndexTest { newDoc().setTags(newArrayList("finance", "offshore")), newDoc().setTags(newArrayList("offshore"))); - List result = underTest.searchTags("off", 10); + List result = underTest.searchTags("off", 1,10); assertThat(result).containsOnly("offshore", "official", "Madhoff"); } @@ -1532,7 +1532,7 @@ public class ProjectMeasuresIndexTest { newDoc().setTags(newArrayList("finance", "offshore")), newDoc().setTags(newArrayList("offshore"))); - List result = underTest.searchTags(null, 10); + List result = underTest.searchTags(null, 1, 10); assertThat(result).containsOnly("offshore", "official", "Madhoff", "finance", "marketing", "java", "javascript"); } @@ -1547,11 +1547,49 @@ public class ProjectMeasuresIndexTest { newDoc().setTags(newArrayList("finance", "offshore")), newDoc().setTags(newArrayList("offshore"))); - List result = underTest.searchTags(null, 10); + List result = underTest.searchTags(null, 1,10); assertThat(result).containsExactly("Madhoff", "finance", "java", "javascript", "marketing", "official", "offshore"); } + @Test + public void search_tags_follows_paging() { + index( + newDoc().setTags(newArrayList("finance", "offshore", "java")), + newDoc().setTags(newArrayList("official", "javascript")), + newDoc().setTags(newArrayList("marketing", "official")), + newDoc().setTags(newArrayList("marketing", "Madhoff")), + newDoc().setTags(newArrayList("finance", "offshore")), + newDoc().setTags(newArrayList("offshore"))); + + List result = underTest.searchTags(null, 1,3); + assertThat(result).containsExactly("Madhoff", "finance", "java"); + + result = underTest.searchTags(null, 2,3); + assertThat(result).containsExactly("javascript", "marketing", "official"); + + result = underTest.searchTags(null, 3,3); + assertThat(result).containsExactly("offshore"); + + result = underTest.searchTags(null, 3,4); + assertThat(result).isEmpty(); + } + + @Test + public void search_tags_returns_nothing_if_page_too_large() { + index( + newDoc().setTags(newArrayList("finance", "offshore", "java")), + newDoc().setTags(newArrayList("official", "javascript")), + newDoc().setTags(newArrayList("marketing", "official")), + newDoc().setTags(newArrayList("marketing", "Madhoff")), + newDoc().setTags(newArrayList("finance", "offshore")), + newDoc().setTags(newArrayList("offshore"))); + + List result = underTest.searchTags(null, 10,2); + + assertThat(result).isEmpty(); + } + @Test public void search_tags_only_of_authorized_projects() { indexForUser(USER1, @@ -1562,14 +1600,14 @@ public class ProjectMeasuresIndexTest { userSession.logIn(USER1); - List result = underTest.searchTags(null, 10); + List result = underTest.searchTags(null, 1,10); assertThat(result).containsOnly("finance", "marketing"); } @Test public void search_tags_with_no_tags() { - List result = underTest.searchTags("whatever", 10); + List result = underTest.searchTags("whatever", 1,10); assertThat(result).isEmpty(); } @@ -1578,7 +1616,7 @@ public class ProjectMeasuresIndexTest { public void search_tags_with_page_size_at_0() { index(newDoc().setTags(newArrayList("offshore"))); - List result = underTest.searchTags(null, 0); + List result = underTest.searchTags(null, 1,0); assertThat(result).isEmpty(); } @@ -1650,9 +1688,17 @@ public class ProjectMeasuresIndexTest { @Test public void fail_if_page_size_greater_than_500() { expectedException.expect(IllegalArgumentException.class); - expectedException.expectMessage("Page size must be lower than or equals to 500"); + expectedException.expectMessage("Page size must be lower than or equals to 100"); + + underTest.searchTags("whatever", 1, 101); + } + + @Test + public void fail_if_page_greater_than_20() { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Page must be between 0 and 20"); - underTest.searchTags("whatever", 501); + underTest.searchTags("whatever", 21, 100); } private void index(ProjectMeasuresDoc... docs) { diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/projecttag/ws/SearchAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/projecttag/ws/SearchAction.java index f2098f894cc..bdae1ca0eeb 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/projecttag/ws/SearchAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/projecttag/ws/SearchAction.java @@ -20,12 +20,14 @@ package org.sonar.server.projecttag.ws; import java.util.List; +import org.sonar.api.server.ws.Change; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; import org.sonar.server.measure.index.ProjectMeasuresIndex; import org.sonarqube.ws.ProjectTags; +import static org.sonar.api.server.ws.WebService.Param.PAGE; import static org.sonar.api.server.ws.WebService.Param.PAGE_SIZE; import static org.sonar.api.server.ws.WebService.Param.TEXT_QUERY; import static org.sonar.server.ws.WsUtils.writeProtobuf; @@ -43,10 +45,11 @@ public class SearchAction implements ProjectTagsWsAction { .setDescription("Search tags") .setSince("6.4") .setResponseExample(getClass().getResource("search-example.json")) + .setChangelog(new Change("9.2", "Parameter 'page' added")) .setHandler(this); action.addSearchQuery("off", "tags"); - action.createPageSize(10, 100); + action.addPagingParams(10, 100); } @Override @@ -56,7 +59,7 @@ public class SearchAction implements ProjectTagsWsAction { } private ProjectTags.SearchResponse doHandle(Request request) { - List tags = index.searchTags(request.param(TEXT_QUERY), request.mandatoryParamAsInt(PAGE_SIZE)); + List tags = index.searchTags(request.param(TEXT_QUERY), request.mandatoryParamAsInt(PAGE), request.mandatoryParamAsInt(PAGE_SIZE)); return ProjectTags.SearchResponse.newBuilder().addAllTags(tags).build(); } } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/projecttag/ws/SearchActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/projecttag/ws/SearchActionTest.java index 6edf6be148b..0e330309cef 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/projecttag/ws/SearchActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/projecttag/ws/SearchActionTest.java @@ -102,7 +102,7 @@ public class SearchActionTest { assertThat(definition.isPost()).isFalse(); assertThat(definition.responseExampleAsString()).isNotEmpty(); assertThat(definition.since()).isEqualTo("6.4"); - assertThat(definition.params()).extracting(WebService.Param::key).containsOnly("q", "ps"); + assertThat(definition.params()).extracting(WebService.Param::key).containsOnly("q", "p", "ps"); } private void index(ProjectMeasuresDoc... docs) { -- 2.39.5