Store PatchSetApprovals instead of labels in the index

The goal of storing SubmitRecord.Labels in the index was to render the
change table, but that requires both the results of the submit rule
evaluator and, in fact, all of the current PatchSetApprovals.

Instead, store PatchSetApprovals, which are trivial to find at
indexing time. Note that we do not just use the formatted label
representation stored in the index because it does not contain all the
necessary columns (e.g. timestamp). These may not be strictly speaking
required for rendering the change table, but having them set to null
in some cases might have caused errors down the line.

Delete the old label-based code entirely and change schema v2 rather
than revving to v3. This mistake was short-lived enough that we don't
expect people to have production data using it, and the old code was
sufficiently complex and incorrect that keeping it around isn't a good
idea.

Change-Id: I805a8fd6c60d4818910aed299260f56ef3006287

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

@@ -25,12 +25,12 @@ import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.SubmitRecord;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.index.ChangeField;
 import com.google.gerrit.server.index.ChangeField.ChangeProtoField;
-import com.google.gerrit.server.index.ChangeField.SubmitLabelProtoField;
+import com.google.gerrit.server.index.ChangeField.PatchSetApprovalProtoField;
 import com.google.gerrit.server.index.ChangeIndex;
 import com.google.gerrit.server.index.FieldDef;
 import com.google.gerrit.server.index.FieldDef.FillArgs;
@@ -104,8 +104,7 @@ public class LuceneChangeIndex implements ChangeIndex {
   public static final String CHANGES_CLOSED = "closed";
   private static final String ID_FIELD = ChangeField.LEGACY_ID.getName();
   private static final String CHANGE_FIELD = ChangeField.CHANGE.getName();
-  private static final String SUBMIT_LABEL_FIELD =
-      ChangeField.SUBMIT_RECORD_LABEL.getName();
+  private static final String APPROVAL_FIELD = ChangeField.APPROVAL.getName();
 
   static interface Factory {
     LuceneChangeIndex create(Schema<ChangeData> schema, String base);
@@ -265,7 +264,7 @@ public class LuceneChangeIndex implements ChangeIndex {
 
   private static class QuerySource implements ChangeDataSource {
     private static final ImmutableSet<String> FIELDS =
-        ImmutableSet.of(ID_FIELD, CHANGE_FIELD, SUBMIT_LABEL_FIELD);
+        ImmutableSet.of(ID_FIELD, CHANGE_FIELD, APPROVAL_FIELD);
 
     private final List<SubIndex> indexes;
     private final Query query;
@@ -359,15 +358,15 @@ public class LuceneChangeIndex implements ChangeIndex {
         cb.bytes, cb.offset, cb.length);
     ChangeData cd = new ChangeData(change);
 
-    BytesRef[] labelsBytes = doc.getBinaryValues(SUBMIT_LABEL_FIELD);
-    if (labelsBytes != null) {
-      List<SubmitRecord.Label> labels =
-          Lists.newArrayListWithCapacity(labelsBytes.length);
-      for (BytesRef lb : labelsBytes) {
-        labels.add(SubmitLabelProtoField.CODEC.decode(
-            lb.bytes, lb.offset, lb.length));
+    BytesRef[] approvalsBytes = doc.getBinaryValues(APPROVAL_FIELD);
+    if (approvalsBytes != null) {
+      List<PatchSetApproval> approvals =
+          Lists.newArrayListWithCapacity(approvalsBytes.length);
+      for (BytesRef ab : approvalsBytes) {
+        approvals.add(PatchSetApprovalProtoField.CODEC.decode(
+            ab.bytes, ab.offset, ab.length));
       }
-      cd.setSubmitRecordLabels(labels);
+      cd.setCurrentApprovals(approvals);
     }
     return cd;
   }

gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java

@@ -15,13 +15,9 @@
 package com.google.gerrit.pgm;
 
 import static com.google.gerrit.server.schema.DataSourceProvider.Context.MULTI_USER;
-import static com.google.inject.Scopes.SINGLETON;
 
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
-import com.google.gerrit.common.ChangeHooks;
-import com.google.gerrit.common.DisabledChangeHooks;
-import com.google.gerrit.common.errors.EmailException;
 import com.google.gerrit.extensions.events.LifecycleListener;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.lifecycle.LifecycleManager;
@@ -31,28 +27,8 @@ import com.google.gerrit.pgm.util.SiteProgram;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.rules.PrologModule;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.InternalUser;
-import com.google.gerrit.server.PluginUser;
-import com.google.gerrit.server.account.AccountByEmailCacheImpl;
-import com.google.gerrit.server.account.AccountCacheImpl;
-import com.google.gerrit.server.account.CapabilityControl;
-import com.google.gerrit.server.account.EmailExpander;
-import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.account.GroupCacheImpl;
-import com.google.gerrit.server.account.GroupControl;
-import com.google.gerrit.server.account.GroupIncludeCacheImpl;
-import com.google.gerrit.server.account.IncludingGroupMembership;
-import com.google.gerrit.server.account.InternalGroupBackend;
-import com.google.gerrit.server.account.UniversalGroupBackend;
 import com.google.gerrit.server.cache.CacheRemovalListener;
 import com.google.gerrit.server.cache.h2.DefaultCacheFactory;
-import com.google.gerrit.server.config.AuthModule;
-import com.google.gerrit.server.config.CanonicalWebUrlModule;
-import com.google.gerrit.server.config.CanonicalWebUrlProvider;
-import com.google.gerrit.server.config.EmailExpanderProvider;
-import com.google.gerrit.server.config.FactoryModule;
 import com.google.gerrit.server.index.ChangeBatchIndexer;
 import com.google.gerrit.server.index.ChangeIndex;
 import com.google.gerrit.server.index.ChangeSchemas;
@@ -60,25 +36,14 @@ import com.google.gerrit.server.index.IndexCollection;
 import com.google.gerrit.server.index.IndexModule;
 import com.google.gerrit.server.index.IndexModule.IndexType;
 import com.google.gerrit.server.index.NoIndexModule;
-import com.google.gerrit.server.mail.Address;
-import com.google.gerrit.server.mail.EmailHeader;
-import com.google.gerrit.server.mail.EmailSender;
-import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
 import com.google.gerrit.server.patch.PatchListCacheImpl;
-import com.google.gerrit.server.plugins.PluginModule;
-import com.google.gerrit.server.project.AccessControlModule;
-import com.google.gerrit.server.project.CommentLinkInfo;
-import com.google.gerrit.server.project.CommentLinkProvider;
-import com.google.gerrit.server.project.ProjectCacheImpl;
-import com.google.gerrit.server.project.ProjectControl;
-import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.SectionSortCache;
-import com.google.gerrit.server.ssh.NoSshKeyCache;
 import com.google.gerrit.solr.SolrIndexModule;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
+import com.google.inject.AbstractModule;
 import com.google.inject.Injector;
 import com.google.inject.Key;
+import com.google.inject.Module;
 import com.google.inject.Provider;
 import com.google.inject.ProvisionException;
 import com.google.inject.TypeLiteral;
@@ -88,10 +53,8 @@ import org.eclipse.jgit.lib.TextProgressMonitor;
 import org.eclipse.jgit.util.io.NullOutputStream;
 import org.kohsuke.args4j.Option;
 
-import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
@@ -147,99 +110,33 @@ public class Reindex extends SiteProgram {
   }
 
   private Injector createSysInjector() {
-    return dbInjector.createChildInjector(new GlobalModule());
-  }
-
-  /**
-   * Subset of GerritGlobalModule including just bindings needed for indexing.
-   *
-   * Does <b>not</b> include thread local request scoping behavior of ReviewDbs;
-   * uses a custom {@link ReviewDbModule} instead.
-   */
-  private class GlobalModule extends FactoryModule {
-    @SuppressWarnings("rawtypes")
-    @Override
-    protected void configure() {
-      install(PatchListCacheImpl.module());
-      install(dbInjector.getInstance(AuthModule.class));
-      install(AccountByEmailCacheImpl.module());
-      install(AccountCacheImpl.module());
-      install(new ReviewDbModule());
-      install(ProjectCacheImpl.module());
-      install(new PluginModule());
-      install(new PrologModule());
-      install(new DefaultCacheFactory.Module());
-      install(NoSshKeyCache.module());
-      install(new SignedTokenEmailTokenVerifier.Module());
-      install(GroupCacheImpl.module());
-      install(new AccessControlModule());
-      install(SectionSortCache.module());
-      install(new CanonicalWebUrlModule() {
-        @Override
-        protected Class<? extends Provider<String>> provider() {
-          return CanonicalWebUrlProvider.class;
-        }
-      });
-
-      switch (IndexModule.getIndexType(dbInjector)) {
-        case LUCENE:
-          install(new LuceneIndexModule(version, threads, outputBase));
-          break;
-        case SOLR:
-          install(new SolrIndexModule(false, threads, outputBase));
-          break;
-        default:
-          install(new NoIndexModule());
-      }
-
-      // No need to support live plugin removal or other live updates of
-      // caches behind our back, so don't worry about cache removal.
-      bind(new TypeLiteral<DynamicSet<CacheRemovalListener>>() {})
-          .toInstance(DynamicSet.<CacheRemovalListener> emptySet());
-
-      // Assume the current user is always the Gerrit internal user; no
-      // indexed data should depend on what user is doing the indexing.
-      bind(CurrentUser.class).to(InternalUser.class);
-
-      bind(ChangeHooks.class).to(DisabledChangeHooks.class);
-      bind(EmailExpander.class).toProvider(EmailExpanderProvider.class).in(
-          SINGLETON);
-      bind(new TypeLiteral<List<CommentLinkInfo>>() {})
-          .toProvider(CommentLinkProvider.class).in(SINGLETON);
-      bind(ProjectControl.GenericFactory.class);
-
-      factory(CapabilityControl.Factory.class);
-      factory(IncludingGroupMembership.Factory.class);
-      factory(InternalUser.Factory.class);
-      factory(PluginUser.Factory.class);
-      factory(ProjectState.Factory.class);
-
-      bind(GroupBackend.class).to(UniversalGroupBackend.class).in(SINGLETON);
-      install(GroupIncludeCacheImpl.module());
-      bind(GroupControl.Factory.class).in(SINGLETON);
-      bind(GroupControl.GenericFactory.class).in(SINGLETON);
-      bind(InternalGroupBackend.class).in(SINGLETON);
-      DynamicSet.setOf(binder(), GroupBackend.class);
-      DynamicSet.bind(binder(), GroupBackend.class).to(InternalGroupBackend.class);
-
-      bind(EmailSender.class).toInstance(new EmailSender() {
-        @Override
-        public boolean isEnabled() {
-          return false;
-        }
-
-        @Override
-        public boolean canEmail(String address) {
-          return false;
-        }
-
-        @Override
-        public void send(Address from, Collection<Address> rcpt,
-            Map<String, EmailHeader> headers, String body)
-            throws EmailException {
-        }
-      });
+    List<Module> modules = Lists.newArrayList();
+    modules.add(PatchListCacheImpl.module());
+    AbstractModule changeIndexModule;
+    switch (IndexModule.getIndexType(dbInjector)) {
+      case LUCENE:
+        changeIndexModule = new LuceneIndexModule(version, threads, outputBase);
+        break;
+      case SOLR:
+        changeIndexModule = new SolrIndexModule(false, threads, outputBase);
+        break;
+      default:
+        changeIndexModule = new NoIndexModule();
     }
+    modules.add(changeIndexModule);
+    modules.add(new ReviewDbModule());
+    modules.add(new AbstractModule() {
+      @SuppressWarnings("rawtypes")
+      @Override
+      protected void configure() {
+        // Plugins are not loaded and we're just running through each change
+        // once, so don't worry about cache removal.
+        bind(new TypeLiteral<DynamicSet<CacheRemovalListener>>() {})
+            .toInstance(DynamicSet.<CacheRemovalListener> emptySet());
+        install(new DefaultCacheFactory.Module());
+      }
+    });
+    return dbInjector.createChildInjector(modules);
   }
 
   private class ReviewDbModule extends LifecycleModule {

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -380,10 +380,6 @@ public class ChangeJson {
     if (!standard && !detailed) {
       return null;
     }
-    if (cd.getSubmitRecordLabels() != null && standard && !detailed) {
-      // Use saved labels to avoid recomputing on the fly.
-      return labelsFromSavedState(cd.getSubmitRecordLabels());
-    }
 
     ChangeControl ctl = control(cd);
     if (ctl == null) {
@@ -398,15 +394,6 @@ public class ChangeJson {
     }
   }
 
-  private Map<String, LabelInfo> labelsFromSavedState(
-      List<SubmitRecord.Label> savedLabels) {
-    Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
-    for (SubmitRecord.Label r : savedLabels) {
-      labels.put(r.label, newLabelFromSubmitRecord(r, true));
-    }
-    return labels;
-  }
-
   private Map<String, LabelInfo> labelsForOpenChange(ChangeData cd,
       LabelTypes labelTypes, boolean standard, boolean detailed)
       throws OrmException {
@@ -447,34 +434,29 @@ public class ChangeJson {
       for (SubmitRecord.Label r : rec.labels) {
         LabelInfo p = labels.get(r.label);
         if (p == null || p._status.compareTo(r.status) < 0) {
-          labels.put(r.label, newLabelFromSubmitRecord(r, standard));
+          LabelInfo n = new LabelInfo();
+          n._status = r.status;
+          if (standard) {
+            switch (r.status) {
+              case OK:
+                n.approved = accountLoader.get(r.appliedBy);
+                break;
+              case REJECT:
+                n.rejected = accountLoader.get(r.appliedBy);
+                break;
+              default:
+                break;
+            }
+          }
+
+          n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
+          labels.put(r.label, n);
         }
       }
     }
     return labels;
   }
 
-  private LabelInfo newLabelFromSubmitRecord(SubmitRecord.Label r,
-      boolean standard) {
-    LabelInfo n = new LabelInfo();
-    n._status = r.status;
-    if (standard) {
-      switch (r.status) {
-        case OK:
-          n.approved = accountLoader.get(r.appliedBy);
-          break;
-        case REJECT:
-          n.rejected = accountLoader.get(r.appliedBy);
-          break;
-        default:
-          break;
-      }
-    }
-
-    n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
-    return n;
-  }
-
   private void setLabelScores(LabelType type,
       LabelInfo label, short score, Account.Id accountId)
       throws OrmException {

gerrit-server/src/main/java/com/google/gerrit/server/config/EmailExpanderProvider.java

@@ -21,7 +21,7 @@ import com.google.inject.Provider;
 
 import org.eclipse.jgit.lib.Config;
 
-public class EmailExpanderProvider implements Provider<EmailExpander> {
+class EmailExpanderProvider implements Provider<EmailExpander> {
   private final EmailExpander expander;
 
   @Inject

gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java

@@ -14,9 +14,7 @@
 
 package com.google.gerrit.server.index;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -24,11 +22,8 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.client.SubmitRecord;
 import com.google.gerrit.reviewdb.client.TrackingId;
 import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeQueryBuilder;
 import com.google.gerrit.server.query.change.ChangeStatusPredicate;
@@ -42,7 +37,6 @@ import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
 /**
@@ -265,63 +259,28 @@ public class ChangeField {
   /** Serialized change object, used for pre-populating results. */
   public static final ChangeProtoField CHANGE = new ChangeProtoField();
 
-  public static class SubmitLabelProtoField
+  public static class PatchSetApprovalProtoField
       extends FieldDef.Repeatable<ChangeData, byte[]> {
-    public static final ProtobufCodec<SubmitRecord.Label> CODEC =
-        CodecFactory.encoder(SubmitRecord.Label.class);
+    public static final ProtobufCodec<PatchSetApproval> CODEC =
+        CodecFactory.encoder(PatchSetApproval.class);
 
-    private SubmitLabelProtoField() {
-      super("_label", FieldType.STORED_ONLY, true);
+    private PatchSetApprovalProtoField() {
+      super("_approval", FieldType.STORED_ONLY, true);
     }
 
     @Override
     public Iterable<byte[]> get(ChangeData input, FillArgs args)
         throws OrmException {
-      // Flatten the highest-valued labels to mimic the results from ChangeJson
-      // with standard labels.
-      Map<String, SubmitRecord.Label> labels = Maps.newLinkedHashMap();
-      for (SubmitRecord rec : getSubmitRecords(input, args)) {
-        if (rec.labels == null) {
-          continue;
-        }
-        for (SubmitRecord.Label r : rec.labels) {
-          SubmitRecord.Label p = labels.get(r.label);
-          if (p == null || p.status.compareTo(r.status) < 0) {
-            labels.put(r.label, r);
-          }
-        }
-      }
-      return toProtos(CODEC, labels.values());
-    }
-
-    private List<SubmitRecord> getSubmitRecords(ChangeData input,
-        FillArgs args) throws OrmException {
-      ChangeControl ctl;
-      try {
-        // Use the ChangeControl for InternalUser. This will give bogus
-        // results for whether or not the change is submittable, but does
-        // not affect label calculation.
-        ctl = args.changeControlFor(input.change(args.db));
-      } catch (NoSuchChangeException e) {
-        throw new OrmException(e);
-      }
-      if (ctl == null) {
-        return ImmutableList.of();
-      }
-      PatchSet ps = input.currentPatchSet(args.db);
-      if (ps == null) {
-        return ImmutableList.of();
-      }
-      return ctl.canSubmit(args.db.get(), ps, input, true, true, true);
+      return toProtos(CODEC, input.currentApprovals(args.db));
     }
   }
 
   /**
-   * Serialized labels from the submit rule evaluator, used for pre-populating
+   * Serialized approvals for the current patch set, used for pre-populating
    * results.
    */
-  public static final SubmitLabelProtoField SUBMIT_RECORD_LABEL =
-      new SubmitLabelProtoField();
+  public static final PatchSetApprovalProtoField APPROVAL =
+      new PatchSetApprovalProtoField();
 
   public static String formatLabel(String label, int value) {
     return formatLabel(label, value, null);

gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java

@@ -69,7 +69,7 @@ public class ChangeSchemas {
         ChangeField.COMMIT_MESSAGE,
         ChangeField.COMMENT,
         ChangeField.CHANGE,
-        ChangeField.SUBMIT_RECORD_LABEL);
+        ChangeField.APPROVAL);
 
   private static Schema<ChangeData> release(FieldDef<ChangeData, ?>... fields) {
     return new Schema<ChangeData>(true, Arrays.asList(fields));

gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java

@@ -14,13 +14,9 @@
 
 package com.google.gerrit.server.index;
 
-import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.InternalUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -64,31 +60,14 @@ public abstract class FieldDef<I, T> {
     final Provider<ReviewDb> db;
     final GitRepositoryManager repoManager;
     final PatchListCache patchListCache;
-    final ChangeControl.GenericFactory changeControlFactory;
-
-    private final InternalUser internalUser;
 
     @Inject
-    FillArgs(Provider<ReviewDb> db, GitRepositoryManager repoManager,
-        PatchListCache patchListCache,
-        ChangeControl.GenericFactory changeControlFactory,
-        InternalUser internalUser) {
+    FillArgs(Provider<ReviewDb> db,
+        GitRepositoryManager repoManager,
+        PatchListCache patchListCache) {
       this.db = db;
       this.repoManager = repoManager;
       this.patchListCache = patchListCache;
-      this.changeControlFactory = changeControlFactory;
-      this.internalUser = internalUser;
-    }
-
-    /**
-     * Create a change control.
-     *
-     * @param change change object.
-     * @return change control as seen by Gerrit's internal user; index fields
-     *     may not depend on any per-user state.
-     */
-    ChangeControl changeControlFor(Change change) throws NoSuchChangeException {
-      return changeControlFactory.controlFor(change, internalUser);
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -148,7 +148,6 @@ public class ChangeData {
   private ChangeControl changeControl;
   private List<ChangeMessage> messages;
   private List<SubmitRecord> submitRecords;
-  private List<SubmitRecord.Label> submitRecordLabels;
   private boolean patchesLoaded;
 
   public ChangeData(final Change.Id id) {
@@ -312,6 +311,10 @@ public class ChangeData {
     return currentApprovals;
   }
 
+  public void setCurrentApprovals(List<PatchSetApproval> approvals) {
+    currentApprovals = approvals;
+  }
+
   public String commitMessage(GitRepositoryManager repoManager,
       Provider<ReviewDb> db) throws IOException, OrmException {
     if (commitMessage == null) {
@@ -460,12 +463,4 @@ public class ChangeData {
   public List<SubmitRecord> getSubmitRecords() {
     return submitRecords;
   }
-
-  public void setSubmitRecordLabels(List<SubmitRecord.Label> labels) {
-    submitRecordLabels = labels;
-  }
-
-  public List<SubmitRecord.Label> getSubmitRecordLabels() {
-    return submitRecordLabels;
-  }
 }