Store SubmitRecords in index

We can quickly evaluate labels and the submittable bit in ChangeJson
if we store the full submit records in the index. We would also like
to be able to search for submittable changes, i.e. changes that pass
the submit rule evaluator. Support this with a triumvirate of new
fields.

We now support `is:submittable` and `submit:STATUS` operators, as well
as a new flavor of `label:` that takes a submit record label status
rather than a numeric vote. These allow for more precise queries than
some of the heuristics that were previously documented in the search
docs.

Change-Id: Ie8a185a7cdae998be168900186fb64905246e7cf

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

@@ -109,9 +109,8 @@ import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.QueryResult;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
@@ -144,6 +143,22 @@ import java.util.TreeMap;
 
 public class ChangeJson {
   private static final Logger log = LoggerFactory.getLogger(ChangeJson.class);
+
+  // Submit rule options in this class should always use fastEvalLabels for
+  // efficiency reasons. Callers that care about submittability after taking
+  // vote squashing into account should be looking at the submit action.
+  public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_LENIENT =
+      ChangeField.SUBMIT_RULE_OPTIONS_LENIENT
+          .toBuilder()
+          .fastEvalLabels(true)
+          .build();
+
+  public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_STRICT =
+      ChangeField.SUBMIT_RULE_OPTIONS_STRICT
+          .toBuilder()
+          .fastEvalLabels(true)
+          .build();
+
   public static final Set<ListChangesOption> NO_OPTIONS =
       Collections.emptySet();
 
@@ -180,7 +195,6 @@ public class ChangeJson {
 
   private boolean lazyLoad = true;
   private AccountLoader accountLoader;
-  private Map<Change.Id, List<SubmitRecord>> submitRecords;
   private FixInput fix;
 
   @AssistedInject
@@ -556,25 +570,13 @@ public class ChangeJson {
   }
 
   private boolean submittable(ChangeData cd) throws OrmException {
-    List<SubmitRecord> records = new SubmitRuleEvaluator(cd)
-        .setFastEvalLabels(true)
-        .evaluate();
-    for (SubmitRecord sr : records) {
-      if (sr.status == SubmitRecord.Status.OK) {
-        return true;
-      }
-    }
-    return false;
+    return SubmitRecord.findOkRecord(
+            cd.submitRecords(SUBMIT_RULE_OPTIONS_STRICT))
+        .isPresent();
   }
 
-  private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS =
-      SubmitRuleOptions.defaults()
-          .fastEvalLabels(true)
-          .allowDraft(true)
-          .build();
-
   private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException {
-    return cd.submitRecords(SUBMIT_RULE_OPTIONS);
+    return cd.submitRecords(SUBMIT_RULE_OPTIONS_LENIENT);
   }
 
   private Map<String, LabelInfo> labelsFor(ChangeControl ctl,

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java

@@ -81,7 +81,6 @@ import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 
 /**
@@ -255,15 +254,6 @@ public class MergeOp implements AutoCloseable {
     orm.close();
   }
 
-  private static Optional<SubmitRecord> findOkRecord(
-      Collection<SubmitRecord> in) {
-    if (in == null) {
-      return Optional.empty();
-    }
-    return in.stream().filter(r -> r.status == SubmitRecord.Status.OK)
-        .findAny();
-  }
-
   public static void checkSubmitRule(ChangeData cd)
       throws ResourceConflictException, OrmException {
     PatchSet patchSet = cd.currentPatchSet();
@@ -272,7 +262,7 @@ public class MergeOp implements AutoCloseable {
           "missing current patch set for change " + cd.getId());
     }
     List<SubmitRecord> results = getSubmitRecords(cd);
-    if (findOkRecord(results).isPresent()) {
+    if (SubmitRecord.findOkRecord(results).isPresent()) {
       // Rules supplied a valid solution.
       return;
     } else if (results.isEmpty()) {

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

@@ -15,7 +15,9 @@
 package com.google.gerrit.server.index.change;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.common.base.Preconditions.checkArgument;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -27,21 +29,25 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.common.collect.Table;
+import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.Comment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.server.OutputFormat;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.index.FieldDef;
 import com.google.gerrit.server.index.FieldType;
 import com.google.gerrit.server.index.SchemaUtil;
 import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeQueryBuilder;
 import com.google.gerrit.server.query.change.ChangeStatusPredicate;
+import com.google.gson.Gson;
 import com.google.gwtorm.protobuf.CodecFactory;
 import com.google.gwtorm.protobuf.ProtobufCodec;
 import com.google.gwtorm.server.OrmException;
@@ -74,6 +80,8 @@ import java.util.Set;
 public class ChangeField {
   public static final int NO_ASSIGNEE = -1;
 
+  private static final Gson GSON = OutputFormat.JSON_COMPACT.newGson();
+
   /** Legacy change ID. */
   public static final FieldDef<ChangeData, Integer> LEGACY_ID =
       new FieldDef.Single<ChangeData, Integer>("legacy_id",
@@ -774,6 +782,169 @@ public class ChangeField {
         }
       };
 
+  // Submit rule options in this class should never use fastEvalLabels. This
+  // slows down indexing slightly but produces correct search results.
+  public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_LENIENT =
+      SubmitRuleOptions.defaults()
+          .allowClosed(true)
+          .allowDraft(true)
+          .build();
+
+  public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_STRICT =
+      SubmitRuleOptions.defaults().build();
+
+  /**
+   * JSON type for storing SubmitRecords.
+   * <p>
+   * Stored fields need to use a stable format over a long period; this type
+   * insulates the index from implementation changes in SubmitRecord itself.
+   */
+  static class StoredSubmitRecord {
+    static class StoredLabel {
+      String label;
+      SubmitRecord.Label.Status status;
+      Integer appliedBy;
+    }
+
+    SubmitRecord.Status status;
+    List<StoredLabel> labels;
+    String errorMessage;
+
+    StoredSubmitRecord(SubmitRecord rec) {
+      this.status = rec.status;
+      this.errorMessage = rec.errorMessage;
+      if (rec.labels != null) {
+        this.labels = new ArrayList<>(rec.labels.size());
+        for (SubmitRecord.Label label : rec.labels) {
+          StoredLabel sl = new StoredLabel();
+          sl.label = label.label;
+          sl.status = label.status;
+          sl.appliedBy =
+              label.appliedBy != null ? label.appliedBy.get() : null;
+          this.labels.add(sl);
+        }
+      }
+    }
+
+    private SubmitRecord toSubmitRecord() {
+      SubmitRecord rec = new SubmitRecord();
+      rec.status = status;
+      rec.errorMessage = errorMessage;
+      if (labels != null) {
+        rec.labels = new ArrayList<>(labels.size());
+        for (StoredLabel label : labels) {
+          SubmitRecord.Label srl = new SubmitRecord.Label();
+          srl.label = label.label;
+          srl.status = label.status;
+          srl.appliedBy = label.appliedBy != null
+              ? new Account.Id(label.appliedBy)
+              : null;
+          rec.labels.add(srl);
+        }
+      }
+      return rec;
+    }
+  }
+
+  public static final FieldDef<ChangeData, Iterable<String>> SUBMIT_RECORD =
+      new FieldDef.Repeatable<ChangeData, String>(
+          "submit_record", FieldType.EXACT, false) {
+        @Override
+        public Iterable<String> get(ChangeData input, FillArgs args)
+            throws OrmException {
+          return formatSubmitRecordValues(input);
+        }
+      };
+
+  public static final FieldDef<ChangeData, Iterable<byte[]>>
+      STORED_SUBMIT_RECORD_STRICT =
+          new FieldDef.Repeatable<ChangeData, byte[]>(
+              "full_submit_record_strict", FieldType.STORED_ONLY, true) {
+            @Override
+            public Iterable<byte[]> get(ChangeData input, FillArgs args)
+                throws OrmException {
+              return storedSubmitRecords(input, SUBMIT_RULE_OPTIONS_STRICT);
+            }
+          };
+
+  public static final FieldDef<ChangeData, Iterable<byte[]>>
+      STORED_SUBMIT_RECORD_LENIENT =
+          new FieldDef.Repeatable<ChangeData, byte[]>(
+              "full_submit_record_lenient", FieldType.STORED_ONLY, true) {
+            @Override
+            public Iterable<byte[]> get(ChangeData input, FillArgs args)
+                throws OrmException {
+              return storedSubmitRecords(input, SUBMIT_RULE_OPTIONS_LENIENT);
+            }
+          };
+
+  public static void parseSubmitRecords(
+      Collection<String> values, SubmitRuleOptions opts, ChangeData out) {
+    checkArgument(!opts.fastEvalLabels());
+    List<SubmitRecord> records = parseSubmitRecords(values);
+    if (records.isEmpty()) {
+      // Assume no values means the field is not in the index;
+      // SubmitRuleEvaluator ensures the list is non-empty.
+      return;
+    }
+    out.setSubmitRecords(opts, records);
+
+    // Cache the fastEvalLabels variant as well so it can be used by
+    // ChangeJson.
+    out.setSubmitRecords(
+        opts.toBuilder().fastEvalLabels(true).build(),
+        records);
+  }
+
+  @VisibleForTesting
+  static List<SubmitRecord> parseSubmitRecords(Collection<String> values) {
+    return values.stream()
+        .map(v -> GSON.fromJson(v, StoredSubmitRecord.class).toSubmitRecord())
+        .collect(toList());
+  }
+
+  @VisibleForTesting
+  static List<byte[]> storedSubmitRecords(List<SubmitRecord> records) {
+    return Lists.transform(
+        records, r -> GSON.toJson(new StoredSubmitRecord(r)).getBytes(UTF_8));
+  }
+
+  private static Iterable<byte[]> storedSubmitRecords(
+      ChangeData cd, SubmitRuleOptions opts) throws OrmException {
+    return storedSubmitRecords(cd.submitRecords(opts));
+  }
+
+  public static List<String> formatSubmitRecordValues(ChangeData cd)
+      throws OrmException {
+    return formatSubmitRecordValues(
+        cd.submitRecords(SUBMIT_RULE_OPTIONS_STRICT),
+        cd.change().getOwner());
+  }
+
+  @VisibleForTesting
+  static List<String> formatSubmitRecordValues(List<SubmitRecord> records,
+      Account.Id changeOwner) {
+    List<String> result = new ArrayList<>();
+    for (SubmitRecord rec : records) {
+      result.add(rec.status.name());
+      if (rec.labels == null) {
+        continue;
+      }
+      for (SubmitRecord.Label label : rec.labels) {
+        String sl = label.status.toString() + ',' + label.label.toLowerCase();
+        result.add(sl);
+        String slc = sl + ',';
+        if (label.appliedBy != null) {
+          result.add(slc + label.appliedBy.get());
+          if (label.appliedBy.equals(changeOwner)) {
+            result.add(slc + ChangeQueryBuilder.OWNER_ACCOUNT_ID.get());
+          }
+        }
+      }
+    }
+    return result;
+  }
+
   public static final Integer NOT_REVIEWED = -1;
 
   private static String getTopic(ChangeData input) throws OrmException {

gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java

@@ -66,13 +66,19 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
   static final Schema<ChangeData> V33 =
       schema(V32, ChangeField.ASSIGNEE);
 
-  @SuppressWarnings("deprecation")
+  @Deprecated
   static final Schema<ChangeData> V34 = new Schema.Builder<ChangeData>()
       .add(V33)
       .remove(ChangeField.LABEL)
       .add(ChangeField.LABEL2)
       .build();
 
+  static final Schema<ChangeData> V35 =
+      schema(V34,
+          ChangeField.SUBMIT_RECORD,
+          ChangeField.STORED_SUBMIT_RECORD_LENIENT,
+          ChangeField.STORED_SUBMIT_RECORD_STRICT);
+
   public static final String NAME = "changes";
   public static final ChangeSchemaDefinitions INSTANCE =
       new ChangeSchemaDefinitions();

gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java

@@ -120,12 +120,7 @@ public class SubmitRuleEvaluator {
   public SubmitRuleEvaluator setOptions(SubmitRuleOptions opts) {
     checkNotStarted();
     if (opts != null) {
-      optsBuilder = SubmitRuleOptions.builder()
-          .fastEvalLabels(opts.fastEvalLabels())
-          .allowDraft(opts.allowDraft())
-          .allowClosed(opts.allowClosed())
-          .skipFilters(opts.skipFilters())
-          .rule(opts.rule());
+      optsBuilder = opts.toBuilder();
     } else {
       optsBuilder = SubmitRuleOptions.defaults();
     }
@@ -273,7 +268,10 @@ public class SubmitRuleEvaluator {
       }
       return createRuleError("Cannot submit draft changes");
     } catch (OrmException err) {
-      String msg = "Cannot check visibility of patch set " + patchSet.getId();
+      PatchSet.Id psId = patchSet != null
+          ? patchSet.getId()
+          : control.getChange().currentPatchSetId();
+      String msg = "Cannot check visibility of patch set " + psId;
       log.error(msg, err);
       return createRuleError(msg);
     }

gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleOptions.java

@@ -55,4 +55,13 @@ public abstract class SubmitRuleOptions {
 
     public abstract SubmitRuleOptions build();
   }
+
+  public Builder toBuilder() {
+    return builder()
+        .fastEvalLabels(fastEvalLabels())
+        .allowDraft(allowDraft())
+        .allowClosed(allowClosed())
+        .skipFilters(skipFilters())
+        .rule(rule());
+  }
 }

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

@@ -19,11 +19,13 @@ import static com.google.gerrit.server.query.change.ChangeData.asChanges;
 import static java.util.stream.Collectors.toSet;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Enums;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.common.errors.NotSignedInException;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.reviewdb.client.Account;
@@ -488,6 +490,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
       return new AssigneePredicate(new Account.Id(ChangeField.NO_ASSIGNEE));
     }
 
+    if ("submittable".equalsIgnoreCase(value)) {
+      return new SubmittablePredicate(SubmitRecord.Status.OK);
+    }
+
     try {
       return status(value);
     } catch (IllegalArgumentException e) {
@@ -644,7 +650,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
           try {
             group = parseGroup(value).getUUID();
           } catch (QueryParseException e) {
-            throw error("Neither user nor group " + value + " found");
+            throw error("Neither user nor group " + value + " found", e);
           }
         }
       }
@@ -665,9 +671,35 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
       }
     }
 
+    // If the vote piece looks like Code-Review=NEED with a valid non-numeric
+    // submit record status, interpret as a submit record query.
+    int eq = name.indexOf('=');
+    if (args.getSchema().hasField(ChangeField.SUBMIT_RECORD) && eq > 0) {
+      String statusName = name.substring(eq + 1).toUpperCase();
+      if (!isInt(statusName)) {
+        SubmitRecord.Label.Status status = Enums.getIfPresent(
+            SubmitRecord.Label.Status.class, statusName).orNull();
+        if (status == null) {
+          throw error("Invalid label status " + statusName + " in " + name);
+        }
+        return SubmitRecordPredicate.create(
+            name.substring(0, eq), status, accounts);
+      }
+    }
+
     return new LabelPredicate(args, name, accounts, group);
   }
 
+  private static boolean isInt(String s) {
+    if (s == null) {
+      return false;
+    }
+    if (s.startsWith("+")) {
+      s = s.substring(1);
+    }
+    return Ints.tryParse(s) != null;
+  }
+
   @Operator
   public Predicate<ChangeData> message(String text) {
     return new MessagePredicate(args.index, text);
@@ -964,6 +996,17 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     return new CommitterPredicate(who);
   }
 
+  @Operator
+  public Predicate<ChangeData> submittable(String str)
+      throws QueryParseException {
+    SubmitRecord.Status status = Enums.getIfPresent(
+        SubmitRecord.Status.class, str.toUpperCase()).orNull();
+    if (status == null) {
+      throw error("invalid value for submittable:" + str);
+    }
+    return new SubmittablePredicate(status);
+  }
+
   @Override
   protected Predicate<ChangeData> defaultField(String query) throws QueryParseException {
     if (query.startsWith("refs/")) {

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

@@ -0,0 +1,54 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.change;
+
+import static java.util.stream.Collectors.toList;
+
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.query.Predicate;
+import com.google.gwtorm.server.OrmException;
+
+import java.util.Set;
+
+class SubmitRecordPredicate extends ChangeIndexPredicate {
+  static Predicate<ChangeData> create(String label,
+      SubmitRecord.Label.Status status, Set<Account.Id> accounts) {
+    String lowerLabel = label.toLowerCase();
+    if (accounts == null || accounts.isEmpty()) {
+      return new SubmitRecordPredicate(status.name() + ',' + lowerLabel);
+    }
+    return Predicate.or(
+        accounts.stream()
+            .map(a -> new SubmitRecordPredicate(
+                status.name() + ',' + lowerLabel + ',' + a.get()))
+            .collect(toList()));
+  }
+
+  private SubmitRecordPredicate(String value) {
+    super(ChangeField.SUBMIT_RECORD, value);
+  }
+
+  @Override
+  public boolean match(ChangeData in) throws OrmException {
+    return ChangeField.formatSubmitRecordValues(in).contains(getValue());
+  }
+
+  @Override
+  public int getCost() {
+    return 1;
+  }
+}

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

@@ -0,0 +1,39 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.change;
+
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gwtorm.server.OrmException;
+
+class SubmittablePredicate extends ChangeIndexPredicate {
+  private final SubmitRecord.Status status;
+
+  SubmittablePredicate(SubmitRecord.Status status) {
+    super(ChangeField.SUBMIT_RECORD, status.name());
+    this.status = status;
+  }
+
+  @Override
+  public boolean match(ChangeData cd) throws OrmException {
+    return cd.submitRecords(ChangeField.SUBMIT_RULE_OPTIONS_STRICT).stream()
+        .anyMatch(r -> r.status == status);
+  }
+
+  @Override
+  public int getCost() {
+    return 1;
+  }
+}