Add option to populate 'starred' field for change query requests

We've recently moved the drafts and stars out of the change index (see
change I36c1e2f5) after which the returned ChangeInfo(s) from the 'Query
Changes' API always had the 'starred' field as null, since this field
is no longer populated from the change index.

We discovered this bug since the dashboard's data is populated from the
change index. To reproduce:
1) Go to dashboard and star a change (press the star icon).
2) Refresh the page. The star is gone.

Navigating to the change page after starring from dashboard shows the
star icon correctly in the change page. This is because the change page
is powered by the 'Get Change Detail' endpoint which does not use the
index.

To fix this, we add a change option to populate this field. This is done
by opening the All-Users repo once, and doing a call to
RefDatabase#exactRef(String... refs). Calls to #exactRef take
microseconds, and since this operation is gated with a list change
option (to be used by the gerrit dashboard), this operation should be
performant.

Google-Bug-Id: b/276296940
Release-Notes: Add option to populate 'starred' field for change queries
Change-Id: Idc0cac3e3aa715d18ddc1696d7324d13bcf84ce1
(cherry picked from commit f4f494800d33114ebb7cc84ef4d628874a937401)

5 files changed, 100 insertions, 2 deletions

diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 99a3485969..b9de6abd8a 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -377,6 +377,13 @@ link:#approval-info[ApprovalInfo] of the `all` attribute.
   as link:#tracking-id-info[TrackingIdInfo].
 --
 
+[[star]]
+--
+* `STAR`: include the `starred` field in
+   link:#change-info[ChangeInfo], which indicates if the change is starred
+   by the current user or not.
+--
+
 .Request
 ----
   GET /changes/?q=97&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=CURRENT_FILES&o=DOWNLOAD_COMMANDS HTTP/1.0
@@ -6692,6 +6699,7 @@ The user who submitted the change, as an
 link:rest-api-accounts.html#account-info[ AccountInfo] entity.
 |`starred`            |not set if `false`|
 Whether the calling user has starred this change with the default label.
+Only set if link:#star[requested].
 |`stars`              |optional|
 A list of star labels that are applied by the calling user to this
 change. The labels are lexicographically sorted.
diff --git a/java/com/google/gerrit/extensions/client/ListChangesOption.java b/java/com/google/gerrit/extensions/client/ListChangesOption.java
index f1f7831c8a..48a3502df0 100644
--- a/java/com/google/gerrit/extensions/client/ListChangesOption.java
+++ b/java/com/google/gerrit/extensions/client/ListChangesOption.java
@@ -88,7 +88,10 @@ public enum ListChangesOption implements ListOption {
   SKIP_DIFFSTAT(23),
 
   /** Include the evaluated submit requirements for the caller. */
-  SUBMIT_REQUIREMENTS(24);
+  SUBMIT_REQUIREMENTS(24),
+
+  /** Include the 'starred' field, that is if the change is starred by the current user . */
+  STAR(25);
 
   private final int value;
 
diff --git a/java/com/google/gerrit/server/StarredChangesUtil.java b/java/com/google/gerrit/server/StarredChangesUtil.java
index 0f5629eeb2..c845415ac5 100644
--- a/java/com/google/gerrit/server/StarredChangesUtil.java
+++ b/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -57,6 +57,7 @@ import java.util.List;
 import java.util.NavigableSet;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.stream.Collectors;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.NullProgressMonitor;
@@ -223,6 +224,29 @@ public class StarredChangesUtil {
   }
 
   /**
+   * Returns a subset of change IDs among the input {@code changeIds} list that are starred by the
+   * {@code caller} user.
+   */
+  public Set<Change.Id> areStarred(
+      Repository allUsersRepo, List<Change.Id> changeIds, Account.Id caller) {
+    List<String> starRefs =
+        changeIds.stream()
+            .map(c -> RefNames.refsStarredChanges(c, caller))
+            .collect(Collectors.toList());
+    try {
+      return allUsersRepo.getRefDatabase().exactRef(starRefs.toArray(new String[0])).keySet()
+          .stream()
+          .map(r -> Change.Id.fromAllUsersRef(r))
+          .collect(Collectors.toSet());
+    } catch (IOException e) {
+      logger.atWarning().withCause(e).log(
+          "Failed getting starred changes for account %d within changes: %s",
+          caller.get(), Joiner.on(", ").join(changeIds));
+      return ImmutableSet.of();
+    }
+  }
+
+  /**
    * Unstar the given change for all users.
    *
    * <p>Intended for use only when we're about to delete a change. For that reason, the change is
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 02b0a60aec..a9063bb891 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -30,6 +30,7 @@ import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
 import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
 import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWER_UPDATES;
 import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_DIFFSTAT;
+import static com.google.gerrit.extensions.client.ListChangesOption.STAR;
 import static com.google.gerrit.extensions.client.ListChangesOption.SUBMITTABLE;
 import static com.google.gerrit.extensions.client.ListChangesOption.SUBMIT_REQUIREMENTS;
 import static com.google.gerrit.extensions.client.ListChangesOption.TRACKING_IDS;
@@ -98,8 +99,10 @@ import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.account.AccountInfoComparator;
 import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.cancellation.RequestCancelledException;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.TrackingFooters;
+import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.index.change.ChangeField;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ReviewerStateInternal;
@@ -129,6 +132,7 @@ import java.util.Set;
 import java.util.stream.Collectors;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
 
 /**
  * Produces {@link ChangeInfo} (which is serialized to JSON afterwards) from {@link ChangeData}.
@@ -218,12 +222,15 @@ public class ChangeJson {
     }
   }
 
+  private final GitRepositoryManager repoManager;
+  private final AllUsersName allUsers;
   private final Provider<CurrentUser> userProvider;
   private final PermissionBackend permissionBackend;
   private final ChangeData.Factory changeDataFactory;
   private final AccountLoader.Factory accountLoaderFactory;
   private final ImmutableSet<ListChangesOption> options;
   private final ChangeMessagesUtil cmUtil;
+  private final StarredChangesUtil starredChangesUtil;
   private final Provider<ConsistencyChecker> checkerProvider;
   private final ActionJson actionJson;
   private final ChangeNotes.Factory notesFactory;
@@ -242,11 +249,14 @@ public class ChangeJson {
 
   @Inject
   ChangeJson(
+      GitRepositoryManager repoManager,
+      AllUsersName allUsers,
       Provider<CurrentUser> user,
       PermissionBackend permissionBackend,
       ChangeData.Factory cdf,
       AccountLoader.Factory ailf,
       ChangeMessagesUtil cmUtil,
+      StarredChangesUtil starredChangesUtil,
       Provider<ConsistencyChecker> checkerProvider,
       ActionJson actionJson,
       ChangeNotes.Factory notesFactory,
@@ -258,11 +268,14 @@ public class ChangeJson {
       @GerritServerConfig Config cfg,
       @Assisted Iterable<ListChangesOption> options,
       @Assisted Optional<PluginDefinedInfosFactory> pluginDefinedInfosFactory) {
+    this.repoManager = repoManager;
+    this.allUsers = allUsers;
     this.userProvider = user;
     this.changeDataFactory = cdf;
     this.permissionBackend = permissionBackend;
     this.accountLoaderFactory = ailf;
     this.cmUtil = cmUtil;
+    this.starredChangesUtil = starredChangesUtil;
     this.checkerProvider = checkerProvider;
     this.actionJson = actionJson;
     this.notesFactory = notesFactory;
@@ -525,6 +538,9 @@ public class ChangeJson {
               "Omitting corrupt change %s from results", cd.getId());
         }
       }
+      if (has(STAR)) {
+        populateStarField(changeInfos);
+      }
       return changeInfos;
     }
   }
@@ -938,6 +954,25 @@ public class ChangeJson {
     return map;
   }
 
+  /** Populate the 'starred' field. */
+  private void populateStarField(List<ChangeInfo> changeInfos) {
+    // We populate the 'starred' field for all change infos together so that we open the All-Users
+    // repository only once
+    try (Repository allUsersRepo = repoManager.openRepository(allUsers)) {
+      List<Change.Id> changeIds =
+          changeInfos.stream().map(c -> Change.id(c._number)).collect(Collectors.toList());
+      Set<Change.Id> starredChanges =
+          starredChangesUtil.areStarred(
+              allUsersRepo, changeIds, userProvider.get().asIdentifiedUser().getAccountId());
+      if (starredChanges.isEmpty()) {
+        return;
+      }
+      changeInfos.stream().forEach(c -> c.starred = starredChanges.contains(Change.id(c._number)));
+    } catch (IOException e) {
+      logger.atWarning().withCause(e).log("Failed to open All-Users repo.");
+    }
+  }
+
   private List<PluginDefinedInfo> getPluginInfos(ChangeData cd) {
     return getPluginInfos(Collections.singleton(cd)).get(cd.getId());
   }
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 556d5f05d6..9b3e116e0a 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -82,6 +82,7 @@ import com.google.gerrit.extensions.api.groups.GroupInput;
 import com.google.gerrit.extensions.api.projects.ConfigInput;
 import com.google.gerrit.extensions.api.projects.ProjectInput;
 import com.google.gerrit.extensions.client.InheritableBoolean;
+import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.ProjectWatchInfo;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.common.AccountInfo;
@@ -2742,7 +2743,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   }
 
   @Test
-  public void byStar() throws Exception {
+  public void byStar_withStarOptionSet() throws Exception {
+    // When star option is set, the 'starred' field is set in the change infos in response.
     TestRepository<Repo> repo = createProject("repo");
     Change change1 = insert(repo, newChangeWithStatus(repo, Change.Status.MERGED));
 
@@ -2755,6 +2757,32 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     // check default star
     assertQuery("has:star", change1);
     assertQuery("is:starred", change1);
+
+    // The 'Star' bit in the change data is also set correctly
+    List<ChangeInfo> changeInfos =
+        gApi.changes().query("has:star").withOptions(ListChangesOption.STAR).get();
+    assertThat(changeInfos.get(0).starred).isTrue();
+  }
+
+  @Test
+  public void byStar_withStarOptionNotSet() throws Exception {
+    // When star option is not set, the 'starred' field is not set in the change infos in response.
+    TestRepository<Repo> repo = createProject("repo");
+    Change change1 = insert(repo, newChangeWithStatus(repo, Change.Status.MERGED));
+
+    Account.Id user2 =
+        accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+    requestContext.setContext(newRequestContext(user2));
+
+    gApi.accounts().self().starChange(change1.getId().toString());
+
+    // check default star
+    assertQuery("has:star", change1);
+    assertQuery("is:starred", change1);
+
+    // The 'Star' bit in the change data is not set if the backfilling option is not set
+    List<ChangeInfo> changeInfos = gApi.changes().query("has:star").get();
+    assertThat(changeInfos.get(0).starred).isNull();
   }
 
   @Test