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

It is unnecessary to query for groups to suggest if they will be
dropped later because the limit was already reached.

Extend SuggestReviewersIT so that we test the correct behavior when
the suggest limit is exhausted before we start to suggest groups and
when the suggest limit is exhausted while we are in the middle of
suggesting groups.

Change-Id: Ifa04ccb136abbfe2c155e148b33776a78056e688
Signed-off-by: Edwin Kempin <ekempin@google.com>

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 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     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 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     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

@@ -174,11 +174,12 @@ public class ReviewersUtil {
     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(
+      suggestedReviewer.addAll(suggestAccountGroups(suggestReviewers,
-          suggestReviewers, projectControl, visibilityControl));
+          projectControl, visibilityControl, limit - suggestedReviewer.size()));
     }
 
     if (suggestedReviewer.size() <= limit) {
@@ -299,7 +300,8 @@ public class ReviewersUtil {
 
   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 @@ public class ReviewersUtil {
             suggestedReviewerInfo.confirm = true;
           }
           groups.add(suggestedReviewerInfo);
+          if (groups.size() >= limit) {
+            break;
+          }
         }
       }
       return groups;