From 682c8c55b3594652b2c4ec1180ede35139885958 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Lesaint?= Date: Mon, 29 Jan 2018 15:23:00 +0100 Subject: [PATCH] SONAR-10313 Do not retrieve rules in request twice When building IssueQuery and loading SearchResponseData --- .../org/sonar/server/issue/IssueQuery.java | 9 +-- .../sonar/server/issue/IssueQueryFactory.java | 11 ++-- .../sonar/server/issue/ws/SearchAction.java | 5 +- .../server/issue/ws/SearchResponseLoader.java | 56 ++----------------- .../sonar/server/issue/IssueQueryTest.java | 9 +-- .../server/issue/index/IssueIndexTest.java | 4 +- 6 files changed, 25 insertions(+), 69 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java index fd3508f252d1..123de44f1032 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQuery.java @@ -28,6 +28,7 @@ import javax.annotation.CheckForNull; import javax.annotation.Nullable; import org.apache.commons.lang.builder.ReflectionToStringBuilder; +import org.sonar.db.rule.RuleDefinitionDto; import static com.google.common.base.Preconditions.checkArgument; import static org.sonar.server.es.SearchOptions.MAX_LIMIT; @@ -63,7 +64,7 @@ public class IssueQuery { private final Collection directories; private final Collection files; private final Collection views; - private final Collection rules; + private final Collection rules; private final Collection assignees; private final Collection authors; private final Collection languages; @@ -162,7 +163,7 @@ public Collection viewUuids() { return views; } - public Collection rules() { + public Collection rules() { return rules; } @@ -273,7 +274,7 @@ public static class Builder { private Collection directories; private Collection files; private Collection views; - private Collection rules; + private Collection rules; private Collection assignees; private Collection authors; private Collection languages; @@ -353,7 +354,7 @@ public Builder viewUuids(@Nullable Collection l) { return this; } - public Builder rules(@Nullable Collection rules) { + public Builder rules(@Nullable Collection rules) { this.rules = rules; return this; } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java index 5eea597845c3..18dd1eef55a4 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueQueryFactory.java @@ -28,6 +28,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.Date; import java.util.HashSet; import java.util.List; @@ -44,7 +45,6 @@ import org.sonar.api.rule.RuleKey; import org.sonar.api.server.ServerSide; import org.sonar.api.web.UserRole; -import org.sonar.core.util.stream.MoreCollectors; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.component.ComponentDto; @@ -364,14 +364,11 @@ private List getComponentsFromUuids(DbSession dbSession, Collectio } @CheckForNull - private Collection ruleKeysToRuleId(DbSession dbSession, @Nullable Collection rules) { + private Collection ruleKeysToRuleId(DbSession dbSession, @Nullable Collection rules) { if (rules != null) { - return dbClient.ruleDao().selectDefinitionByKeys(dbSession, transform(rules, RuleKey::parse)) - .stream() - .map(RuleDefinitionDto::getId) - .collect(MoreCollectors.toList()); + return dbClient.ruleDao().selectDefinitionByKeys(dbSession, transform(rules, RuleKey::parse)); } - return null; + return Collections.emptyList(); } private static String toProjectUuid(ComponentDto componentDto) { 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 d70760a5cbbb..9237803edc1e 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 @@ -21,6 +21,7 @@ import com.google.common.base.Joiner; import com.google.common.collect.Collections2; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import java.util.Arrays; import java.util.Collection; @@ -363,7 +364,9 @@ private SearchWsResponse doHandle(SearchRequest request, Request wsRequest) { (!query.projectUuids().isEmpty()) || query.organizationUuid() != null, "Facet(s) '%s' require to also filter by project or organization", COMA_JOINER.join(facetsRequiringProjectOrOrganizationParameter)); } - SearchResponseData data = searchResponseLoader.load(collector, facets); + SearchResponseData preloadedData = new SearchResponseData(emptyList()); + preloadedData.setRules(ImmutableList.copyOf(query.rules())); + SearchResponseData data = searchResponseLoader.load(preloadedData, collector, facets); // format response diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java index 7647b198edb2..7e03b90f08c3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java @@ -29,7 +29,6 @@ import java.util.Map; import java.util.Set; import javax.annotation.Nullable; -import org.sonar.api.rule.RuleKey; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.util.stream.MoreCollectors; import org.sonar.db.DbClient; @@ -78,32 +77,11 @@ public SearchResponseLoader(UserSession userSession, DbClient dbClient, Transiti this.transitionService = transitionService; } - /** - * The issue keys are given by the multi-criteria search in Elasticsearch index. - */ - public SearchResponseData load(Collector collector, @Nullable Facets facets) { - try (DbSession dbSession = dbClient.openSession(false)) { - SearchResponseData result = new SearchResponseData(dbClient.issueDao().selectByOrderedKeys(dbSession, collector.getIssueKeys())); - collector.collect(result.getIssues()); - - loadRules(collector, dbSession, result); - // order is important - loading of comments complete the list of users: loadComments() is - // before loadUsers() - loadComments(collector, dbSession, result); - loadUsers(collector, dbSession, result); - loadComponents(collector, dbSession, result); - loadOrganizations(dbSession, result); - loadActionsAndTransitions(collector, result); - completeTotalEffortFromFacet(facets, result); - return result; - } - } - /** * The issue keys are given by the multi-criteria search in Elasticsearch index. *

- * Same as {@link #load(Collector, Facets)} but will only retrieve from DB data which is not already provided by the - * specified preloaded {@link SearchResponseData}.
+ * Same as {@link #load(SearchResponseData, Collector, Facets)} but will only retrieve from DB data which is not + * already provided by the specified preloaded {@link SearchResponseData}.
* The returned {@link SearchResponseData} is not the one specified as argument. *

*/ @@ -186,23 +164,17 @@ private void addProjectUuids(Collector collector, DbSession dbSession, SearchRes private void loadRules(SearchResponseData preloadedResponseData, Collector collector, DbSession dbSession, SearchResponseData result) { if (collector.contains(RULES)) { List preloadedRules = firstNonNull(preloadedResponseData.getRules(), emptyList()); - Set preloaedRuleKeys = preloadedRules.stream().map(RuleDefinitionDto::getKey).collect(MoreCollectors.toSet()); - Set ruleKeysToLoad = copyOf(difference(collector.get(RULES), preloaedRuleKeys)); + Set preloaedRuleKeys = preloadedRules.stream().map(RuleDefinitionDto::getId).collect(MoreCollectors.toSet()); + Set ruleKeysToLoad = copyOf(difference(collector.get(RULES), preloaedRuleKeys)); if (ruleKeysToLoad.isEmpty()) { result.setRules(preloadedResponseData.getRules()); } else { - List loadedRules = dbClient.ruleDao().selectDefinitionByKeys(dbSession, ruleKeysToLoad); + List loadedRules = dbClient.ruleDao().selectDefinitionByIds(dbSession, ruleKeysToLoad); result.setRules(concat(preloadedRules.stream(), loadedRules.stream()).collect(toList(preloadedRules.size() + loadedRules.size()))); } } } - private void loadUsers(Collector collector, DbSession dbSession, SearchResponseData result) { - if (collector.contains(USERS)) { - result.setUsers(dbClient.userDao().selectByLogins(dbSession, collector.getList(USERS))); - } - } - private void loadComments(Collector collector, DbSession dbSession, SearchResponseData result) { if (collector.contains(COMMENTS)) { List comments = dbClient.issueChangeDao().selectByTypeAndIssueKeys(dbSession, collector.getIssueKeys(), IssueChangeDto.TYPE_COMMENT); @@ -220,20 +192,6 @@ private boolean canEditOrDelete(IssueChangeDto dto) { return userSession.isLoggedIn() && userSession.getLogin().equals(dto.getUserLogin()); } - private void loadRules(Collector collector, DbSession dbSession, SearchResponseData result) { - if (collector.contains(RULES)) { - result.setRules(dbClient.ruleDao().selectDefinitionByIds(dbSession, collector.getList(RULES))); - } - } - - private void loadComponents(Collector collector, DbSession dbSession, SearchResponseData result) { - // always load components and projects, because some issue fields still relate to component ids/keys. - // They should be dropped but are kept for backward-compatibility (see SearchResponseFormat) - result.addComponents(dbClient.componentDao().selectByUuids(dbSession, collector.getComponentUuids())); - result.addComponents(dbClient.componentDao().selectSubProjectsByComponentUuids(dbSession, collector.getComponentUuids())); - addProjectUuids(collector, dbSession, result); - } - private void loadOrganizations(DbSession dbSession, SearchResponseData result) { Collection components = result.getComponents(); dbClient.organizationDao().selectByUuids( @@ -364,10 +322,6 @@ Set get(SearchAdditionalField key) { return (Set) fieldValues.get(key); } - List getList(SearchAdditionalField key) { - return newArrayList(get(key)); - } - boolean contains(SearchAdditionalField field) { return fields.contains(field); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java index 8c89d26a8a96..c1cb1df4585c 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueQueryTest.java @@ -25,6 +25,7 @@ import org.junit.Test; import org.sonar.api.issue.Issue; import org.sonar.api.rule.Severity; +import org.sonar.db.rule.RuleDefinitionDto; import org.sonar.server.issue.IssueQuery.PeriodStart; import static com.google.common.collect.Lists.newArrayList; @@ -36,8 +37,8 @@ public class IssueQueryTest { @Test public void build_query() { - int ruleId = nextInt(1000); - PeriodStart filterDate = new IssueQuery.PeriodStart(new Date(10_000_000_000L),false); + RuleDefinitionDto rule = new RuleDefinitionDto().setId(nextInt(1000)); + PeriodStart filterDate = new IssueQuery.PeriodStart(new Date(10_000_000_000L), false); IssueQuery query = IssueQuery.builder() .issueKeys(newArrayList("ABCDE")) .severities(newArrayList(Severity.BLOCKER)) @@ -46,7 +47,7 @@ public void build_query() { .projectUuids(newArrayList("PROJECT")) .componentUuids(newArrayList("org/struts/Action.java")) .moduleUuids(newArrayList("org.struts:core")) - .rules(newArrayList(ruleId)) + .rules(newArrayList(rule)) .assignees(newArrayList("gargantua")) .languages(newArrayList("xoo")) .tags(newArrayList("tag1", "tag2")) @@ -77,7 +78,7 @@ public void build_query() { assertThat(query.branchUuid()).isEqualTo("my_branch"); assertThat(query.createdAfterByProjectUuids()).containsOnly(entry("PROJECT", filterDate)); assertThat(query.assigned()).isTrue(); - assertThat(query.rules()).containsOnly(ruleId); + assertThat(query.rules()).containsOnly(rule); assertThat(query.createdAfter()).isNotNull(); assertThat(query.createdBefore()).isNotNull(); assertThat(query.createdAt()).isNotNull(); diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java index 70767cbf4054..8552cd1dee99 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/index/IssueIndexTest.java @@ -526,8 +526,8 @@ public void filter_by_rules() { indexIssues(newDoc("I1", file).setRuleId(ruleDefinitionDto.getId())); - assertThatSearchReturnsOnly(IssueQuery.builder().rules(singletonList(ruleDefinitionDto.getId())), "I1"); - assertThatSearchReturnsEmpty(IssueQuery.builder().rules(singletonList(-1))); + assertThatSearchReturnsOnly(IssueQuery.builder().rules(singletonList(ruleDefinitionDto)), "I1"); + assertThatSearchReturnsEmpty(IssueQuery.builder().rules(singletonList(new RuleDefinitionDto().setId(-1)))); } @Test