Merge "ReviewersUtil: Query for groups only while limit is not reached yet"

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java

diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index 184b174..7abbc46 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -15,7 +15,9 @@
 package com.google.gerrit.acceptance.rest.change;
 
 import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.toList;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GerritConfig;
@@ -97,11 +99,25 @@
     String changeId = createChange().getChangeId();
     List<SuggestedReviewerInfo> reviewers =
         suggestReviewers(changeId, name("u"), 6);
-    assertThat(reviewers).hasSize(6);
+    assertReviewers(reviewers, ImmutableList.of(user1, user2, user3),
+        ImmutableList.of(group1, group2, group3));
+
     reviewers = suggestReviewers(changeId, name("u"), 5);
-    assertThat(reviewers).hasSize(5);
+    assertReviewers(reviewers, ImmutableList.of(user1, user2, user3),
+        ImmutableList.of(group1, group2));
+
     reviewers = suggestReviewers(changeId, group3.getName(), 10);
+    assertReviewers(reviewers, ImmutableList.of(), ImmutableList.of(group3));
+
+    // Suggested accounts are ordered by activity. All users have no activity,
+    // hence we don't know which of the matching accounts we get when the query
+    // is limited to 1.
+    reviewers = suggestReviewers(changeId, name("u"), 1);
     assertThat(reviewers).hasSize(1);
+    assertThat(reviewers.get(0).account).isNotNull();
+    assertThat(ImmutableList.of(reviewers.get(0).account._accountId))
+        .containsAnyIn(ImmutableList.of(user1, user2, user3).stream()
+            .map(u -> u.id.get()).collect(toList()));
   }
 
   @Test
@@ -460,4 +476,26 @@
     ci.branch = "master";
     return gApi.changes().create(ci).get().changeId;
   }
+
+  private void assertReviewers(List<SuggestedReviewerInfo> actual,
+      List<TestAccount> expectedUsers, List<AccountGroup> expectedGroups) {
+    List<Integer> actualAccountIds = actual.stream()
+        .filter(i -> i.account != null)
+        .map(i -> i.account._accountId)
+        .collect(toList());
+    assertThat(actualAccountIds)
+        .containsExactlyElementsIn(
+            expectedUsers.stream().map(u -> u.id.get()).collect(toList()));
+
+    List<String> actualGroupIds = actual.stream()
+        .filter(i -> i.group != null)
+        .map(i -> i.group.id)
+        .collect(toList());
+    assertThat(actualGroupIds)
+        .containsExactlyElementsIn(
+            expectedGroups.stream()
+                .map(g -> g.getGroupUUID().get())
+                .collect(toList()))
+        .inOrder();
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java
index a09537a..d045bfe 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java
@@ -174,11 +174,12 @@
     List<SuggestedReviewerInfo> suggestedReviewer =
         loadAccounts(sortedRecommendations);
 
-    if (!excludeGroups && !Strings.isNullOrEmpty(query)) {
+    if (!excludeGroups && suggestedReviewer.size() < limit
+        && !Strings.isNullOrEmpty(query)) {
       // Add groups at the end as individual accounts are usually more
       // important.
-      suggestedReviewer.addAll(suggestAccountGroups(
-          suggestReviewers, projectControl, visibilityControl));
+      suggestedReviewer.addAll(suggestAccountGroups(suggestReviewers,
+          projectControl, visibilityControl, limit - suggestedReviewer.size()));
     }
 
     if (suggestedReviewer.size() <= limit) {
@@ -299,7 +300,8 @@
 
   private List<SuggestedReviewerInfo> suggestAccountGroups(
       SuggestReviewers suggestReviewers, ProjectControl projectControl,
-      VisibilityControl visibilityControl) throws OrmException, IOException {
+      VisibilityControl visibilityControl, int limit)
+          throws OrmException, IOException {
     try (Timer0.Context ctx = metrics.queryGroupsLatency.start()) {
       List<SuggestedReviewerInfo> groups = new ArrayList<>();
       for (GroupReference g : suggestAccountGroups(suggestReviewers,
@@ -318,6 +320,9 @@
             suggestedReviewerInfo.confirm = true;
           }
           groups.add(suggestedReviewerInfo);
+          if (groups.size() >= limit) {
+            break;
+          }
         }
       }
       return groups;