ChangeData: Cache submit records keyed by options

This allows ChangeData to use a generic cache of submit records which
also supports running an evaluator multiple times with multiple
options.

Change-Id: Id78024e1270d94b3ab8335717fe14da6cea8f226

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

@@ -109,6 +109,7 @@ 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.query.QueryResult;
@@ -566,23 +567,14 @@ public class ChangeJson {
     return false;
   }
 
+  private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS =
+      SubmitRuleOptions.defaults()
+          .fastEvalLabels(true)
+          .allowDraft(true)
+          .build();
+
   private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException {
-    // Maintain our own cache rather than using cd.getSubmitRecords(),
-    // since the latter may not have used the same values for
-    // fastEvalLabels/allowDraft/etc.
-    // TODO(dborowitz): Handle this better at the ChangeData level.
-    if (submitRecords == null) {
-      submitRecords = new HashMap<>();
-    }
-    List<SubmitRecord> records = submitRecords.get(cd.getId());
-    if (records == null) {
-      records = new SubmitRuleEvaluator(cd)
-        .setFastEvalLabels(true)
-        .setAllowDraft(true)
-        .evaluate();
-      submitRecords.put(cd.getId(), records);
-    }
-    return records;
+    return cd.submitRecords(SUBMIT_RULE_OPTIONS);
   }
 
   private Map<String, LabelInfo> labelsFor(ChangeControl ctl,

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

@@ -57,7 +57,7 @@ import com.google.gerrit.server.git.validators.MergeValidationException;
 import com.google.gerrit.server.git.validators.MergeValidators;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.util.RequestId;
@@ -101,6 +101,9 @@ import java.util.Set;
 public class MergeOp implements AutoCloseable {
   private static final Logger log = LoggerFactory.getLogger(MergeOp.class);
 
+  private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS =
+      SubmitRuleOptions.defaults().build();
+
   public static class CommitStatus {
     private final ImmutableMap<Change.Id, ChangeData> changes;
     private final ImmutableSetMultimap<Branch.NameKey, Change.Id> byBranch;
@@ -173,7 +176,7 @@ public class MergeOp implements AutoCloseable {
       // However, do NOT expose that ChangeData directly, as it is way out of
       // date by this point.
       ChangeData cd = checkNotNull(changes.get(id), "ChangeData for %s", id);
-      return checkNotNull(cd.getSubmitRecords(),
+      return checkNotNull(cd.getSubmitRecords(SUBMIT_RULE_OPTIONS),
           "getSubmitRecord only valid after submit rules are evalutated");
     }
 
@@ -309,12 +312,7 @@ public class MergeOp implements AutoCloseable {
 
   private static List<SubmitRecord> getSubmitRecords(ChangeData cd)
       throws OrmException {
-    List<SubmitRecord> results = cd.getSubmitRecords();
-    if (results == null) {
-      results = new SubmitRuleEvaluator(cd).evaluate();
-      cd.setSubmitRecords(results);
-    }
-    return results;
+    return cd.submitRecords(SUBMIT_RULE_OPTIONS);
   }
 
   private static String describeLabels(ChangeData cd,
@@ -387,7 +385,7 @@ public class MergeOp implements AutoCloseable {
       SubmitRecord forced = new SubmitRecord();
       forced.status = SubmitRecord.Status.FORCED;
       records.add(forced);
-      cd.setSubmitRecords(records);
+      cd.setSubmitRecords(SUBMIT_RULE_OPTIONS, records);
     }
   }
 

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

@@ -91,12 +91,9 @@ public class SubmitRuleEvaluator {
   private final ChangeData cd;
   private final ChangeControl control;
 
+  private SubmitRuleOptions.Builder optsBuilder = SubmitRuleOptions.defaults();
+  private SubmitRuleOptions opts;
   private PatchSet patchSet;
-  private boolean fastEvalLabels;
-  private boolean allowDraft;
-  private boolean allowClosed;
-  private boolean skipFilters;
-  private String rule;
   private boolean logErrors = true;
   private long reductionsConsumed;
 
@@ -107,6 +104,34 @@ public class SubmitRuleEvaluator {
     this.control = cd.changeControl();
   }
 
+  /**
+   * @return immutable snapshot of options configured so far. If neither {@link
+   *     #getSubmitRule()} nor {@link #getSubmitType()} have been called yet,
+   *     state within this instance is still mutable, so may change before
+   *     evaluation. The instance's options are frozen at evaluation time.
+   */
+  public SubmitRuleOptions getOptions() {
+    if (opts != null) {
+      return opts;
+    }
+    return optsBuilder.build();
+  }
+
+  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());
+    } else {
+      optsBuilder = SubmitRuleOptions.defaults();
+    }
+    return this;
+  }
+
   /**
    * @param ps patch set of the change to evaluate. If not set, the current
    * patch set will be loaded from {@link #evaluate()} or {@link
@@ -127,7 +152,8 @@ public class SubmitRuleEvaluator {
    * @return this
    */
   public SubmitRuleEvaluator setFastEvalLabels(boolean fast) {
-    fastEvalLabels = fast;
+    checkNotStarted();
+    optsBuilder.fastEvalLabels(fast);
     return this;
   }
 
@@ -136,7 +162,8 @@ public class SubmitRuleEvaluator {
    * @return this
    */
   public SubmitRuleEvaluator setAllowClosed(boolean allow) {
-    allowClosed = allow;
+    checkNotStarted();
+    optsBuilder.allowClosed(allow);
     return this;
   }
 
@@ -145,7 +172,8 @@ public class SubmitRuleEvaluator {
    * @return this
    */
   public SubmitRuleEvaluator setAllowDraft(boolean allow) {
-    allowDraft = allow;
+    checkNotStarted();
+    optsBuilder.allowDraft(allow);
     return this;
   }
 
@@ -154,7 +182,8 @@ public class SubmitRuleEvaluator {
    * @return this
    */
   public SubmitRuleEvaluator setSkipSubmitFilters(boolean skip) {
-    skipFilters = skip;
+    checkNotStarted();
+    optsBuilder.skipFilters(skip);
     return this;
   }
 
@@ -163,7 +192,8 @@ public class SubmitRuleEvaluator {
    * @return this
    */
   public SubmitRuleEvaluator setRule(@Nullable String rule) {
-    this.rule = rule;
+    checkNotStarted();
+    optsBuilder.rule(rule);
     return this;
   }
 
@@ -188,13 +218,14 @@ public class SubmitRuleEvaluator {
    *     rules, including any errors.
    */
   public List<SubmitRecord> evaluate() {
+    initOptions();
     Change c = control.getChange();
-    if (!allowClosed && c.getStatus().isClosed()) {
+    if (!opts.allowClosed() && c.getStatus().isClosed()) {
       SubmitRecord rec = new SubmitRecord();
       rec.status = SubmitRecord.Status.CLOSED;
       return Collections.singletonList(rec);
     }
-    if (!allowDraft) {
+    if (!opts.allowDraft()) {
       if (c.getStatus() == Change.Status.DRAFT) {
         return cannotSubmitDraft();
       }
@@ -369,6 +400,7 @@ public class SubmitRuleEvaluator {
    * @return record from the evaluated rules.
    */
   public SubmitTypeRecord getSubmitType() {
+    initOptions();
     try {
       initPatchSet();
     } catch (OrmException e) {
@@ -453,7 +485,7 @@ public class SubmitRuleEvaluator {
     PrologEnvironment env = getPrologEnvironment(user);
     try {
       Term sr = env.once("gerrit", userRuleLocatorName, new VariableTerm());
-      if (fastEvalLabels) {
+      if (opts.fastEvalLabels()) {
         env.once("gerrit", "assume_range_from_label");
       }
 
@@ -476,7 +508,7 @@ public class SubmitRuleEvaluator {
       }
 
       Term resultsTerm = toListTerm(results);
-      if (!skipFilters) {
+      if (!opts.skipFilters()) {
         resultsTerm = runSubmitFilters(
             resultsTerm, env, filterRuleLocatorName, filterRuleWrapperName);
       }
@@ -503,18 +535,19 @@ public class SubmitRuleEvaluator {
     ProjectState projectState = control.getProjectControl().getProjectState();
     PrologEnvironment env;
     try {
-      if (rule == null) {
+      if (opts.rule() == null) {
         env = projectState.newPrologEnvironment();
       } else {
-        env = projectState.newPrologEnvironment("stdin", new StringReader(rule));
+        env = projectState.newPrologEnvironment(
+            "stdin", new StringReader(opts.rule()));
       }
     } catch (CompileException err) {
       String msg;
-      if (rule == null && control.getProjectControl().isOwner()) {
+      if (opts.rule() == null && control.getProjectControl().isOwner()) {
         msg = String.format(
             "Cannot load rules.pl for %s: %s",
             getProjectName(), err.getMessage());
-      } else if (rule != null) {
+      } else if (opts.rule() != null) {
         msg = err.getMessage();
       } else {
         msg = String.format("Cannot load rules.pl for %s", getProjectName());
@@ -548,7 +581,7 @@ public class SubmitRuleEvaluator {
       Term filterRule =
           parentEnv.once("gerrit", filterRuleLocatorName, new VariableTerm());
       try {
-        if (fastEvalLabels) {
+        if (opts.fastEvalLabels()) {
           env.once("gerrit", "assume_range_from_label");
         }
 
@@ -608,6 +641,17 @@ public class SubmitRuleEvaluator {
     return submitRule != null ? submitRule.toString() : "<unknown rule>";
   }
 
+  private void checkNotStarted() {
+    checkState(opts == null, "cannot set options after starting evaluation");
+  }
+
+  private void initOptions() {
+    if (opts == null) {
+      opts = optsBuilder.build();
+      optsBuilder = null;
+    }
+  }
+
   private void initPatchSet() throws OrmException {
     if (patchSet == null) {
       patchSet = cd.currentPatchSet();

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

@@ -0,0 +1,58 @@
+// 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.project;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.Nullable;
+
+/**
+ * Stable identifier for options passed to a particular submit rule evaluator.
+ * <p>
+ * Used to test whether it is ok to reuse a cached list of submit records. Does
+ * not include a change or patch set ID; callers are responsible for checking
+ * those on their own.
+ */
+@AutoValue
+public abstract class SubmitRuleOptions {
+  public static Builder builder() {
+    return new AutoValue_SubmitRuleOptions.Builder();
+  }
+
+  public static Builder defaults() {
+    return builder()
+        .fastEvalLabels(false)
+        .allowDraft(false)
+        .allowClosed(false)
+        .skipFilters(false)
+        .rule(null);
+  }
+
+  public abstract boolean fastEvalLabels();
+  public abstract boolean allowDraft();
+  public abstract boolean allowClosed();
+  public abstract boolean skipFilters();
+  @Nullable public abstract String rule();
+
+  @AutoValue.Builder
+  public abstract static class Builder {
+    public abstract SubmitRuleOptions.Builder fastEvalLabels(boolean fastEvalLabels);
+    public abstract SubmitRuleOptions.Builder allowDraft(boolean allowDraft);
+    public abstract SubmitRuleOptions.Builder allowClosed(boolean allowClosed);
+    public abstract SubmitRuleOptions.Builder skipFilters(boolean skipFilters);
+    public abstract SubmitRuleOptions.Builder rule(@Nullable String rule);
+
+    public abstract SubmitRuleOptions build();
+  }
+}

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

@@ -63,6 +63,7 @@ import com.google.gerrit.server.patch.PatchListCache;
 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.gwtorm.server.OrmException;
@@ -325,6 +326,9 @@ public class ChangeData {
   private final MergeabilityCache mergeabilityCache;
   private final StarredChangesUtil starredChangesUtil;
   private final Change.Id legacyId;
+  private final Map<SubmitRuleOptions, List<SubmitRecord>>
+      submitRecords = Maps.newLinkedHashMapWithExpectedSize(1);
+
   private Project.NameKey project;
   private Change change;
   private ChangeNotes notes;
@@ -341,7 +345,6 @@ public class ChangeData {
   private CurrentUser visibleTo;
   private ChangeControl changeControl;
   private List<ChangeMessage> messages;
-  private List<SubmitRecord> submitRecords;
   private Optional<ChangedLines> changedLines;
   private SubmitTypeRecord submitTypeRecord;
   private Boolean mergeable;
@@ -1011,12 +1014,30 @@ public class ChangeData {
     return messages;
   }
 
-  public void setSubmitRecords(List<SubmitRecord> records) {
-    submitRecords = records;
+  public List<SubmitRecord> submitRecords(
+      SubmitRuleOptions options) throws OrmException {
+    List<SubmitRecord> records = submitRecords.get(options);
+    if (records == null) {
+      if (!lazyLoad) {
+        return Collections.emptyList();
+      }
+      records = new SubmitRuleEvaluator(this)
+          .setOptions(options)
+          .evaluate();
+      submitRecords.put(options, records);
+    }
+    return records;
   }
 
-  public List<SubmitRecord> getSubmitRecords() {
-    return submitRecords;
+  @Nullable
+  public List<SubmitRecord> getSubmitRecords(
+      SubmitRuleOptions options) {
+    return submitRecords.get(options);
+  }
+
+  public void setSubmitRecords(SubmitRuleOptions options,
+      List<SubmitRecord> records) {
+    submitRecords.put(options, records);
   }
 
   public SubmitTypeRecord submitTypeRecord() throws OrmException {