From 9b6b424a391cbe9a4059e4aa74ecd526bbaaf60f Mon Sep 17 00:00:00 2001
From: Edwin Kempin <ekempin@google.com>
Date: Mon, 7 Nov 2016 09:31:30 +0100
Subject: [PATCH] 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>
---
 .../rest/change/SuggestReviewersIT.java       | 42 ++++++++++++++++++-
 .../google/gerrit/server/ReviewersUtil.java   | 13 ++++--
 2 files changed, 49 insertions(+), 6 deletions(-)

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 184b1740c7..7abbc46cb0 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 @@ 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();
+  }
 }
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 a09537ae85..d045bfe5ce 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 @@ 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(
-          suggestReviewers, projectControl, visibilityControl));
+      suggestedReviewer.addAll(suggestAccountGroups(suggestReviewers,
+          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;