Read reviewedBy set from secondary index

We now no longer read the database when rendering search results even
with the REVIEWED option used on the account dashboard screen.

Tweak query tests to allow assertions over specific fields set in the
result, to ensure that the value is being set correctly from the
stored field.

Change-Id: I21c43496d2a5b6a1533c45a24e027257f5e4944e

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -463,9 +463,10 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(gApi.changes().query()
           .withQuery(
             "project:{" + project.get() + "} (status:open OR status:closed)")
-          // Options should match defaults in ChangeTable.
+          // Options should match defaults in AccountDashboardScreen.
           .withOption(ListChangesOption.LABELS)
           .withOption(ListChangesOption.DETAILED_ACCOUNTS)
+          .withOption(ListChangesOption.REVIEWED)
           .get())
         .hasSize(2);
   }

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java

@@ -33,6 +33,8 @@ import java.util.EnumSet;
 import java.util.Set;
 
 public class AccountDashboardScreen extends Screen implements ChangeListScreen {
+  // If changing default options, also update in
+  // ChangeIT#defaultSearchDoesNotTouchDatabase().
   private static final Set<ListChangesOption> MY_DASHBOARD_OPTIONS;
   static {
     EnumSet<ListChangesOption> options = EnumSet.copyOf(ChangeTable.OPTIONS);

gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java

@@ -30,6 +30,7 @@ import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -132,12 +133,14 @@ public class LuceneChangeIndex implements ChangeIndex {
       sortFieldName(ChangeField.LEGACY_ID);
   private static final String MERGEABLE_FIELD = ChangeField.MERGEABLE.getName();
   private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName();
+  private static final String REVIEWEDBY_FIELD =
+      ChangeField.REVIEWEDBY.getName();
   private static final String UPDATED_SORT_FIELD =
       sortFieldName(ChangeField.UPDATED);
 
   private static final ImmutableSet<String> FIELDS = ImmutableSet.of(
       ADDED_FIELD, APPROVAL_FIELD, CHANGE_FIELD, DELETED_FIELD, ID_FIELD,
-      MERGEABLE_FIELD, PATCH_SET_FIELD);
+      MERGEABLE_FIELD, PATCH_SET_FIELD, REVIEWEDBY_FIELD);
 
   private static final Map<String, String> CUSTOM_CHAR_MAPPING = ImmutableMap.of(
       "_", " ", ".", " ");
@@ -506,6 +509,21 @@ public class LuceneChangeIndex implements ChangeIndex {
       cd.setMergeable(false);
     }
 
+    // Reviewed-by.
+    IndexableField[] reviewedBy = doc.getFields(REVIEWEDBY_FIELD);
+    if (reviewedBy.length > 0) {
+      Set<Account.Id> accounts =
+          Sets.newHashSetWithExpectedSize(reviewedBy.length);
+      for (IndexableField r : reviewedBy) {
+        int id = r.numericValue().intValue();
+        if (reviewedBy.length == 1 && id == ChangeField.NOT_REVIEWED) {
+          break;
+        }
+        accounts.add(new Account.Id(id));
+      }
+      cd.setReviewedBy(accounts);
+    }
+
     return cd;
   }
 

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.query.change;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.concurrent.TimeUnit.HOURS;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
@@ -1101,10 +1102,25 @@ public abstract class AbstractQueryChangesTest {
         .revision(ps3_1.get())
         .review(new ReviewInput().message("comment"));
 
-    assertQuery("is:reviewed", change3, change2);
-    assertQuery("-is:reviewed", change1);
-    assertQuery("reviewedby:" + userId.get());
-    assertQuery("reviewedby:" + user2.get(), change3, change2);
+    List<ChangeInfo> actual;
+    actual = assertQuery(
+        newQuery("is:reviewed").withOption(REVIEWED),
+        change3, change2);
+    assertThat(actual.get(0).reviewed).isTrue();
+    assertThat(actual.get(1).reviewed).isTrue();
+
+    actual = assertQuery(
+        newQuery("-is:reviewed").withOption(REVIEWED),
+        change1);
+    assertThat(actual.get(0).reviewed).isNull();
+
+    actual = assertQuery("reviewedby:" + userId.get());
+
+    actual = assertQuery(
+        newQuery("reviewedby:" + user2.get()).withOption(REVIEWED),
+        change3, change2);
+    assertThat(actual.get(0).reviewed).isTrue();
+    assertThat(actual.get(1).reviewed).isTrue();
   }
 
   protected ChangeInserter newChange(
@@ -1181,29 +1197,18 @@ public abstract class AbstractQueryChangesTest {
     return gApi.changes().query(query.toString());
   }
 
-  protected void assertQuery(Object query, Change... changes)
+  protected List<ChangeInfo> assertQuery(Object query, Change... changes)
       throws Exception {
-    assertQuery(newQuery(query), changes);
+    return assertQuery(newQuery(query), changes);
   }
 
-  protected void assertQuery(QueryRequest query, Change... changes)
+  protected List<ChangeInfo> assertQuery(QueryRequest query, Change... changes)
       throws Exception {
-    assertThat(query(query)).named(query.toString())
+    List<ChangeInfo> result = query.get();
+    Iterable<Integer> ids = ids(result);
+    assertThat(ids).named(ids.toString())
         .containsExactlyElementsIn(ids(changes)).inOrder();
-  }
-
-  protected List<Integer> query(Object query) throws Exception {
-    return query(newQuery(query));
-  }
-
-  protected static List<Integer> query(QueryRequest query) throws Exception {
-    return FluentIterable.from(query.get())
-        .transform(new Function<ChangeInfo, Integer>() {
-          @Override
-          public Integer apply(ChangeInfo in) {
-            return in._number;
-          }
-        }).toList();
+    return result;
   }
 
   protected static Iterable<Integer> ids(Change... changes) {
@@ -1216,6 +1221,16 @@ public abstract class AbstractQueryChangesTest {
         });
   }
 
+  protected static Iterable<Integer> ids(Iterable<ChangeInfo> changes) {
+    return FluentIterable.from(changes).transform(
+        new Function<ChangeInfo, Integer>() {
+          @Override
+          public Integer apply(ChangeInfo in) {
+            return in._number;
+          }
+        });
+  }
+
   protected static long lastUpdatedMs(Change c) {
     return c.getLastUpdatedOn().getTime();
   }