Fix reviewer suggestion

Reviewer suggestion for non-empty queries was slow and inaccurate. Before this
change, the behavior was as following:

1. For an empty query, check the previous 25 changes of the current user,
and find all the users that reviewed his change, and those who reviewed
the most changes will be ranked higher.

2. For a non-empty query, generate n*3 (n = 6 in the frontend) account
names that start with the query. E.g, for query "a" it would generate
18 account names that start with "a", and then it would rank them based
on many things (comments, approvals, etc). This is bad, since there are
much more than 18 accounts that start with "a", so the results are not
good.

In this change, the behavior for an empty query stayed the same, and for
a non-empty query the behavior is as following:
Check the previous 25 changes of the current user and find all the
people that reviewed his change that their name or email starts with
the query. E.g, if I reviewed a change, and then the owner of the change
queries "Ga" it would find me (unless 6 other people that their name
starts with "Ga" and reviewed his change).
In case there are less than 6 people that fit the query described above,
we will also find up to 6 people that just start with the name. For "a"
it will still be quite random, but for "paiki" it likely will yield good
results even if I never reviewed this user's changes.

Therefore, users are no longer ranked by comments, owning a change, etc.
They are ranked only by reviews. It also doesn't matter which project it
is they reviewed (reviewerRanking and reviewerRankingProjectIsolation
are both irrelevant tests that were deleted).

Change-Id: I0ca718d35e52b3e0794419d1f56c431af52df0b6

java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java

@@ -21,14 +21,14 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
-import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.extensions.client.ReviewerState;
-import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.QueryParseException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.FanOutExecutor;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.change.ReviewerSuggestion;
 import com.google.gerrit.server.change.SuggestedReviewer;
 import com.google.gerrit.server.config.GerritServerConfig;
@@ -45,11 +45,11 @@ import com.google.inject.Provider;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
@@ -79,6 +79,7 @@ public class ReviewerRecommender {
   private final Provider<InternalChangeQuery> queryProvider;
   private final ExecutorService executor;
   private final ApprovalsUtil approvalsUtil;
+  private final AccountCache accountCache;
 
   @Inject
   ReviewerRecommender(
@@ -87,13 +88,15 @@ public class ReviewerRecommender {
       Provider<InternalChangeQuery> queryProvider,
       @FanOutExecutor ExecutorService executor,
       ApprovalsUtil approvalsUtil,
-      @GerritServerConfig Config config) {
+      @GerritServerConfig Config config,
+      AccountCache accountCache) {
     this.changeQueryBuilder = changeQueryBuilder;
     this.config = config;
     this.queryProvider = queryProvider;
     this.reviewerSuggestionPluginMap = reviewerSuggestionPluginMap;
     this.executor = executor;
     this.approvalsUtil = approvalsUtil;
+    this.accountCache = accountCache;
   }
 
   public List<Account.Id> suggestReviewers(
@@ -111,12 +114,7 @@ public class ReviewerRecommender {
     double baseWeight = config.getInt("addReviewer", "baseWeight", 1);
     logger.atFine().log("base weight: %s", baseWeight);
 
-    Map<Account.Id, MutableDouble> reviewerScores;
-    if (Strings.isNullOrEmpty(query)) {
-      reviewerScores = baseRankingForEmptyQuery(baseWeight);
-    } else {
-      reviewerScores = baseRankingForCandidateList(candidateList, projectState, baseWeight);
-    }
+    Map<Account.Id, MutableDouble> reviewerScores = baseRanking(baseWeight, query, candidateList);
     logger.atFine().log("Base reviewer scores: %s", reviewerScores);
 
     // Send the query along with a candidate list to all plugins and merge the
@@ -198,7 +196,18 @@ public class ReviewerRecommender {
     return sortedSuggestions;
   }
 
-  private Map<Account.Id, MutableDouble> baseRankingForEmptyQuery(double baseWeight)
+  /**
+   * @param baseWeight The weight applied to the ordering of the reviewers.
+   * @param query Query to match. For example, it can try to match all users that start with "Ab".
+   * @param candidateList The list of candidates based on the query. If query is empty, this list is
+   *     also empty.
+   * @return Map of account ids that match the query and their appropriate ranking (the better the
+   *     ranking, the better it is to suggest them as reviewers).
+   * @throws IOException Can't find owner="self" account.
+   * @throws ConfigInvalidException Can't find owner="self" account.
+   */
+  private Map<Account.Id, MutableDouble> baseRanking(
+      double baseWeight, String query, List<Account.Id> candidateList)
       throws IOException, ConfigInvalidException {
     // Get the user's last 25 changes, check approvals
     try {
@@ -208,14 +217,15 @@ public class ReviewerRecommender {
               .setLimit(25)
               .setRequestedFields(ChangeField.APPROVAL)
               .query(changeQueryBuilder.owner("self"));
-      Map<Account.Id, MutableDouble> suggestions = new HashMap<>();
+      Map<Account.Id, MutableDouble> suggestions = new LinkedHashMap<>();
+      // Put those candidates at the bottom of the list
+      candidateList.stream().forEach(id -> suggestions.put(id, new MutableDouble(0)));
+
       for (ChangeData cd : result) {
         for (PatchSetApproval approval : cd.currentApprovals()) {
           Account.Id id = approval.accountId();
-          if (suggestions.containsKey(id)) {
-            suggestions.get(id).add(baseWeight);
-          } else {
-            suggestions.put(id, new MutableDouble(baseWeight));
+          if (Strings.isNullOrEmpty(query) || accountMatchesQuery(id, query)) {
+            suggestions.computeIfAbsent(id, (ignored) -> new MutableDouble(0)).add(baseWeight);
           }
         }
       }
@@ -227,63 +237,15 @@ public class ReviewerRecommender {
     }
   }
 
-  private Map<Account.Id, MutableDouble> baseRankingForCandidateList(
-      List<Account.Id> candidates, ProjectState projectState, double baseWeight)
-      throws IOException, ConfigInvalidException {
-    // Get each reviewer's activity based on number of applied labels
-    // (weighted 10d), number of comments (weighted 0.5d) and number of owned
-    // changes (weighted 1d).
-    Map<Account.Id, MutableDouble> reviewers = new LinkedHashMap<>();
-    if (candidates.size() == 0) {
-      return reviewers;
-    }
-    List<Predicate<ChangeData>> predicates = new ArrayList<>();
-    for (Account.Id id : candidates) {
-      try {
-        Predicate<ChangeData> projectQuery = changeQueryBuilder.project(projectState.getName());
-
-        // Get all labels for this project and create a compound OR query to
-        // fetch all changes where users have applied one of these labels
-        List<LabelType> labelTypes = projectState.getLabelTypes().getLabelTypes();
-        List<Predicate<ChangeData>> labelPredicates = new ArrayList<>(labelTypes.size());
-        for (LabelType type : labelTypes) {
-          labelPredicates.add(changeQueryBuilder.label(type.getName() + ",user=" + id));
-        }
-        Predicate<ChangeData> reviewerQuery =
-            Predicate.and(projectQuery, Predicate.or(labelPredicates));
-
-        Predicate<ChangeData> ownerQuery =
-            Predicate.and(projectQuery, changeQueryBuilder.owner(id.toString()));
-        Predicate<ChangeData> commentedByQuery =
-            Predicate.and(projectQuery, changeQueryBuilder.commentby(id.toString()));
-
-        predicates.add(reviewerQuery);
-        predicates.add(ownerQuery);
-        predicates.add(commentedByQuery);
-        reviewers.put(id, new MutableDouble());
-      } catch (QueryParseException e) {
-        // Unhandled: If an exception is thrown, we won't increase the
-        // candidates's score
-        logger.atSevere().withCause(e).log("Exception while suggesting reviewers");
+  private boolean accountMatchesQuery(Account.Id id, String query) {
+    Optional<Account> account = accountCache.get(id).map(AccountState::account);
+    if (account.isPresent() && account.get().isActive()) {
+      if ((account.get().fullName() != null && account.get().fullName().startsWith(query))
+          || (account.get().preferredEmail() != null
+              && account.get().preferredEmail().startsWith(query))) {
+        return true;
       }
     }
-
-    List<List<ChangeData>> result = queryProvider.get().setLimit(25).noFields().query(predicates);
-
-    Iterator<List<ChangeData>> queryResultIterator = result.iterator();
-    Iterator<Account.Id> reviewersIterator = reviewers.keySet().iterator();
-
-    int i = 0;
-    Account.Id currentId = null;
-    while (queryResultIterator.hasNext()) {
-      List<ChangeData> currentResult = queryResultIterator.next();
-      if (i % WEIGHTS.length == 0) {
-        currentId = reviewersIterator.next();
-      }
-
-      reviewers.get(currentId).add(WEIGHTS[i % WEIGHTS.length] * baseWeight * currentResult.size());
-      i++;
-    }
-    return reviewers;
+    return false;
   }
 }

java/com/google/gerrit/server/restapi/change/ReviewersUtil.java

@@ -251,7 +251,7 @@ public class ReviewersUtil {
                   QueryOptions.create(
                       indexConfig,
                       0,
-                      suggestReviewers.getLimit() * CANDIDATE_LIST_MULTIPLIER,
+                      suggestReviewers.getLimit(),
                       ImmutableSet.of(AccountField.ID.getName())))
               .readRaw();
       List<Account.Id> matches =

javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java

@@ -432,107 +432,27 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     assertThat(reviewers).isEmpty();
   }
 
-  @Test
-  @GerritConfig(name = "suggest.maxSuggestedReviewers", value = "10")
-  public void reviewerRanking() throws Exception {
-    // Assert that user are ranked by the number of times they have applied a
-    // a label to a change (highest), added comments (medium) or owned a
-    // change (low).
-    String fullName = "Primum Finalis";
-    TestAccount userWhoOwns = user("customuser1", fullName);
-    TestAccount reviewer1 = user("customuser2", fullName);
-    TestAccount reviewer2 = user("customuser3", fullName);
-    TestAccount userWhoComments = user("customuser4", fullName);
-    TestAccount userWhoLooksForSuggestions = user("customuser5", fullName);
-
-    // Create a change as userWhoOwns and add some reviews
-    requestScopeOperations.setApiUser(userWhoOwns.id());
-    String changeId1 = createChangeFromApi();
-
-    requestScopeOperations.setApiUser(reviewer1.id());
-    reviewChange(changeId1);
-
-    requestScopeOperations.setApiUser(user1.id());
-    String changeId2 = createChangeFromApi();
-
-    requestScopeOperations.setApiUser(reviewer1.id());
-    reviewChange(changeId2);
-
-    requestScopeOperations.setApiUser(reviewer2.id());
-    reviewChange(changeId2);
-
-    // Create a comment as a different user
-    requestScopeOperations.setApiUser(userWhoComments.id());
-    ReviewInput ri = new ReviewInput();
-    ri.message = "Test";
-    gApi.changes().id(changeId1).revision(1).review(ri);
-
-    // Create a change as a new user to assert that we receive the correct
-    // ranking
-
-    requestScopeOperations.setApiUser(userWhoLooksForSuggestions.id());
-    List<SuggestedReviewerInfo> reviewers = suggestReviewers(createChangeFromApi(), "Pri", 4);
-    assertThat(reviewers.stream().map(r -> r.account._accountId).collect(toList()))
-        .containsExactly(
-            reviewer1.id().get(),
-            reviewer2.id().get(),
-            userWhoOwns.id().get(),
-            userWhoComments.id().get())
-        .inOrder();
-  }
-
-  @Test
-  public void reviewerRankingProjectIsolation() throws Exception {
-    // Create new project
-    Project.NameKey newProject = projectOperations.newProject().create();
-
-    // Create users who review changes in both the default and the new project
-    String fullName = "Primum Finalis";
-    TestAccount userWhoOwns = user("customuser1", fullName);
-    TestAccount reviewer1 = user("customuser2", fullName);
-    TestAccount reviewer2 = user("customuser3", fullName);
-
-    requestScopeOperations.setApiUser(userWhoOwns.id());
-    String changeId1 = createChangeFromApi();
-
-    requestScopeOperations.setApiUser(reviewer1.id());
-    reviewChange(changeId1);
-
-    requestScopeOperations.setApiUser(userWhoOwns.id());
-    String changeId2 = createChangeFromApi(newProject);
-
-    requestScopeOperations.setApiUser(reviewer2.id());
-    reviewChange(changeId2);
-
-    requestScopeOperations.setApiUser(userWhoOwns.id());
-    String changeId3 = createChangeFromApi(newProject);
-
-    requestScopeOperations.setApiUser(reviewer2.id());
-    reviewChange(changeId3);
-
-    requestScopeOperations.setApiUser(userWhoOwns.id());
-    List<SuggestedReviewerInfo> reviewers = suggestReviewers(createChangeFromApi(), "Prim", 4);
-
-    // Assert that reviewer1 is on top, even though reviewer2 has more reviews
-    // in other projects
-    assertThat(reviewers.stream().map(r -> r.account._accountId).collect(toList()))
-        .containsExactly(reviewer1.id().get(), reviewer2.id().get())
-        .inOrder();
-  }
-
   @Test
   public void suggestNoInactiveAccounts() throws Exception {
+    requestScopeOperations.setApiUser(user.id());
+    String changeIdReviewed = createChangeFromApi();
+    String changeId = createChangeFromApi();
+
     String name = name("foo");
     TestAccount foo1 = accountCreator.create(name + "-1");
+    requestScopeOperations.setApiUser(foo1.id());
+    reviewChange(changeIdReviewed);
     assertThat(gApi.accounts().id(foo1.username()).getActive()).isTrue();
 
     TestAccount foo2 = accountCreator.create(name + "-2");
+    requestScopeOperations.setApiUser(foo2.id());
+    reviewChange(changeIdReviewed);
     assertThat(gApi.accounts().id(foo2.username()).getActive()).isTrue();
 
-    String changeId = createChange().getChangeId();
     assertReviewers(
         suggestReviewers(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
 
+    requestScopeOperations.setApiUser(user.id());
     gApi.accounts().id(foo2.username()).setActive(false);
     assertThat(gApi.accounts().id(foo2.id().get()).getActive()).isFalse();
     assertReviewers(suggestReviewers(changeId, name), ImmutableList.of(foo1), ImmutableList.of());
@@ -540,11 +460,19 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
 
   @Test
   public void suggestNoExistingReviewers() throws Exception {
+    requestScopeOperations.setApiUser(user.id());
+    String changeId = createChangeFromApi();
+    String changeIdReviewed = createChangeFromApi();
+
     String name = name("foo");
     TestAccount foo1 = accountCreator.create(name + "-1");
-    TestAccount foo2 = accountCreator.create(name + "-2");
+    requestScopeOperations.setApiUser(foo1.id());
+    reviewChange(changeIdReviewed);
+
+    TestAccount foo2 = accountCreator.create(name + "-2");
+    requestScopeOperations.setApiUser(foo2.id());
+    reviewChange(changeIdReviewed);
 
-    String changeId = createChange().getChangeId();
     assertReviewers(
         suggestReviewers(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
 
@@ -554,11 +482,19 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
 
   @Test
   public void suggestCcAsReviewer() throws Exception {
+    requestScopeOperations.setApiUser(user.id());
+    String changeId = createChangeFromApi();
+    String changeIdReviewed = createChangeFromApi();
+
     String name = name("foo");
     TestAccount foo1 = accountCreator.create(name + "-1");
-    TestAccount foo2 = accountCreator.create(name + "-2");
+    requestScopeOperations.setApiUser(foo1.id());
+    reviewChange(changeIdReviewed);
+
+    TestAccount foo2 = accountCreator.create(name + "-2");
+    requestScopeOperations.setApiUser(foo2.id());
+    reviewChange(changeIdReviewed);
 
-    String changeId = createChange().getChangeId();
     assertReviewers(
         suggestReviewers(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
 
@@ -572,11 +508,19 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
 
   @Test
   public void suggestReviewerAsCc() throws Exception {
+    requestScopeOperations.setApiUser(user.id());
+    String changeId = createChangeFromApi();
+    String changeIdReviewed = createChangeFromApi();
+
     String name = name("foo");
     TestAccount foo1 = accountCreator.create(name + "-1");
-    TestAccount foo2 = accountCreator.create(name + "-2");
+    requestScopeOperations.setApiUser(foo1.id());
+    reviewChange(changeIdReviewed);
+
+    TestAccount foo2 = accountCreator.create(name + "-2");
+    requestScopeOperations.setApiUser(foo2.id());
+    reviewChange(changeIdReviewed);
 
-    String changeId = createChange().getChangeId();
     assertReviewers(suggestCcs(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
 
     AddReviewerInput reviewerInput = new AddReviewerInput();
@@ -591,25 +535,17 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     String secondaryEmail = "foo.secondary@example.com";
     TestAccount foo = createAccountWithSecondaryEmail("foo", secondaryEmail);
 
-    List<SuggestedReviewerInfo> reviewers =
-        suggestReviewers(createChange().getChangeId(), secondaryEmail, 4);
-    assertReviewers(reviewers, ImmutableList.of(foo), ImmutableList.of());
-
-    reviewers = suggestReviewers(createChange().getChangeId(), "secondary", 4);
+    List<SuggestedReviewerInfo> reviewers = suggestReviewers(createChangeFromApi(), "secondary", 4);
     assertReviewers(reviewers, ImmutableList.of(foo), ImmutableList.of());
   }
 
   @Test
   public void cannotSuggestBySecondaryEmailWithoutModifyAccount() throws Exception {
     String secondaryEmail = "foo.secondary@example.com";
-    createAccountWithSecondaryEmail("foo", secondaryEmail);
+    TestAccount foo = createAccountWithSecondaryEmail("foo", secondaryEmail);
 
     requestScopeOperations.setApiUser(user.id());
-    List<SuggestedReviewerInfo> reviewers =
-        suggestReviewers(createChange().getChangeId(), secondaryEmail, 4);
-    assertThat(reviewers).isEmpty();
-
-    reviewers = suggestReviewers(createChange().getChangeId(), "secondary2", 4);
+    List<SuggestedReviewerInfo> reviewers = suggestReviewers(createChangeFromApi(), "secondary", 4);
     assertThat(reviewers).isEmpty();
   }
 
@@ -630,6 +566,24 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     assertThat(Iterables.getOnlyElement(reviewers).account.secondaryEmails).isNull();
   }
 
+  @Test
+  public void suggestsPeopleWithNoReviewsWhenExplicitlyQueried() throws Exception {
+    TestAccount newTeamMember = accountCreator.create("newTeamMember");
+
+    requestScopeOperations.setApiUser(user.id());
+    String changeId = createChangeFromApi();
+    String changeIdReviewed = createChangeFromApi();
+
+    TestAccount reviewer = accountCreator.create("newReviewer");
+    requestScopeOperations.setApiUser(reviewer.id());
+    reviewChange(changeIdReviewed);
+
+    List<SuggestedReviewerInfo> reviewers = suggestReviewers(changeId, "new", 4);
+    assertThat(reviewers.stream().map(r -> r.account._accountId).collect(toList()))
+        .containsExactly(reviewer.id().get(), newTeamMember.id().get())
+        .inOrder();
+  }
+
   private TestAccount createAccountWithSecondaryEmail(String name, String secondaryEmail)
       throws Exception {
     TestAccount foo = accountCreator.create(name(name), "foo.primary@example.com", "Foo");