Refactor SubmitRuleEvaluator to reduce confusing arguments

The multitude of string and optional boolean constructor arguments was
a smell. Use a builder pattern for the optional ones, and add
multiple methods for the 2 valid combinations of 4 string arguments,
rather than requiring callers to know these magic values.

Most other constructor arguments could be inferred directly
from the ChangeData, so eliminate those as well.

Change-Id: I2554e8574722dc022956d3ca8db4ad0dc36f8c0d

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

@@ -14,8 +14,6 @@
 
 package com.google.gerrit.server.change;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
@@ -43,7 +41,6 @@ import com.googlecode.prolog_cafe.lang.Term;
 
 import org.kohsuke.args4j.Option;
 
-import java.io.ByteArrayInputStream;
 import java.util.List;
 import java.util.Map;
 
@@ -87,25 +84,15 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, Input> {
       throw new AuthException("project rules are disabled");
     }
     input.filters = MoreObjects.firstNonNull(input.filters, filters);
-
     SubmitRuleEvaluator evaluator = new SubmitRuleEvaluator(
-        db.get(),
-        rsrc.getPatchSet(),
-        rsrc.getControl().getProjectControl(),
-        rsrc.getControl(),
-        rsrc.getChange(),
-        changeDataFactory.create(db.get(), rsrc.getControl()),
-        false,
-        "locate_submit_rule", "can_submit",
-        "locate_submit_filter", "filter_submit_results",
-        input.filters == Filters.SKIP,
-        input.rule != null
-          ? new ByteArrayInputStream(input.rule.getBytes(UTF_8))
-          : null);
+        changeDataFactory.create(db.get(), rsrc.getControl()));
 
     List<Term> results;
     try {
-      results = eval(evaluator);
+      results = evaluator.setPatchSet(rsrc.getPatchSet())
+          .setSkipSubmitFilters(input.filters == Filters.SKIP)
+          .setRule(input.rule)
+          .evaluate();
     } catch (RuleEvalException e) {
       String msg = Joiner.on(": ").skipNulls().join(Iterables.transform(
           Throwables.getCausalChain(e),
@@ -135,11 +122,6 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, Input> {
     return out;
   }
 
-  private static List<Term> eval(SubmitRuleEvaluator evaluator)
-      throws RuleEvalException {
-    return evaluator.evaluate();
-  }
-
   static class Record {
     SubmitRecord.Status status;
     String errorMessage;

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

@@ -14,8 +14,6 @@
 
 package com.google.gerrit.server.change;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-
 import com.google.common.base.MoreObjects;
 import com.google.gerrit.extensions.common.SubmitType;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -29,6 +27,7 @@ import com.google.gerrit.server.change.TestSubmitRule.Input;
 import com.google.gerrit.server.project.RuleEvalException;
 import com.google.gerrit.server.project.SubmitRuleEvaluator;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -37,7 +36,6 @@ import com.googlecode.prolog_cafe.lang.Term;
 
 import org.kohsuke.args4j.Option;
 
-import java.io.ByteArrayInputStream;
 import java.util.List;
 
 public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
@@ -59,7 +57,7 @@ public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
 
   @Override
   public SubmitType apply(RevisionResource rsrc, Input input)
-      throws AuthException, BadRequestException {
+      throws AuthException, BadRequestException, OrmException {
     if (input == null) {
       input = new Input();
     }
@@ -67,25 +65,15 @@ public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
       throw new AuthException("project rules are disabled");
     }
     input.filters = MoreObjects.firstNonNull(input.filters, filters);
-
     SubmitRuleEvaluator evaluator = new SubmitRuleEvaluator(
-        db.get(),
-        rsrc.getPatchSet(),
-        rsrc.getControl().getProjectControl(),
-        rsrc.getControl(),
-        rsrc.getChange(),
-        changeDataFactory.create(db.get(), rsrc.getControl()),
-        false,
-        "locate_submit_type", "get_submit_type",
-        "locate_submit_type_filter", "filter_submit_type_results",
-        input.filters == Filters.SKIP,
-        input.rule != null
-          ? new ByteArrayInputStream(input.rule.getBytes(UTF_8))
-          : null);
+        changeDataFactory.create(db.get(), rsrc.getControl()));
 
     List<Term> results;
     try {
-      results = evaluator.evaluate();
+      results = evaluator.setPatchSet(rsrc.getPatchSet())
+          .setSkipSubmitFilters(input.filters == Filters.SKIP)
+          .setRule(input.rule)
+          .evaluateSubmitType();
     } catch (RuleEvalException e) {
       throw new BadRequestException(String.format(
           "rule failed with exception: %s",
@@ -125,7 +113,7 @@ public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
 
     @Override
     public SubmitType apply(RevisionResource resource)
-        throws AuthException, BadRequestException {
+        throws AuthException, BadRequestException, OrmException {
       return test.apply(resource, null);
     }
   }

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

@@ -461,14 +461,11 @@ public class ChangeControl {
     List<Term> results;
     SubmitRuleEvaluator evaluator;
     try {
-      evaluator = new SubmitRuleEvaluator(db, patchSet,
-          getProjectControl(),
-          this, getChange(), cd,
-          fastEvalLabels,
-          "locate_submit_rule", "can_submit",
-          "locate_submit_filter", "filter_submit_results");
-      results = evaluator.evaluate();
-    } catch (RuleEvalException e) {
+      evaluator = new SubmitRuleEvaluator(cd);
+      results = evaluator.setPatchSet(patchSet)
+          .setFastEvalLabels(fastEvalLabels)
+          .evaluate();
+    } catch (OrmException | RuleEvalException e) {
       return logRuleError(e.getMessage(), e);
     }
 
@@ -617,13 +614,9 @@ public class ChangeControl {
     List<Term> results;
     SubmitRuleEvaluator evaluator;
     try {
-      evaluator = new SubmitRuleEvaluator(db, patchSet,
-          getProjectControl(), this, getChange(), cd,
-          false,
-          "locate_submit_type", "get_submit_type",
-          "locate_submit_type_filter", "filter_submit_type_results");
-      results = evaluator.evaluate();
-    } catch (RuleEvalException e) {
+      evaluator = new SubmitRuleEvaluator(cd);
+      results = evaluator.evaluateSubmitType();
+    } catch (OrmException | RuleEvalException e) {
       return logTypeRuleError(e.getMessage(), e);
     }
 

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

@@ -14,15 +14,16 @@
 
 package com.google.gerrit.server.project;
 
-import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkArgument;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.collect.Lists;
-import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.rules.PrologEnvironment;
 import com.google.gerrit.rules.StoredValues;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
 
 import com.googlecode.prolog_cafe.compiler.CompileException;
 import com.googlecode.prolog_cafe.lang.ListTerm;
@@ -31,7 +32,7 @@ import com.googlecode.prolog_cafe.lang.PrologException;
 import com.googlecode.prolog_cafe.lang.Term;
 import com.googlecode.prolog_cafe.lang.VariableTerm;
 
-import java.io.InputStream;
+import java.io.ByteArrayInputStream;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -42,91 +43,86 @@ import java.util.List;
  * all the way up to All-Projects.
  */
 public class SubmitRuleEvaluator {
-  private final ReviewDb db;
-  private final PatchSet patchSet;
-  private final ProjectControl projectControl;
-  private final ChangeControl changeControl;
-  private final Change change;
   private final ChangeData cd;
-  private final boolean fastEvalLabels;
-  private final String userRuleLocatorName;
-  private final String userRuleWrapperName;
-  private final String filterRuleLocatorName;
-  private final String filterRuleWrapperName;
-  private final boolean skipFilters;
-  private final InputStream rulesInputStream;
+  private final ChangeControl control;
+
+  private PatchSet patchSet;
+  private boolean fastEvalLabels;
+  private boolean skipFilters;
+  private String rule;
 
   private Term submitRule;
-  private String projectName;
 
-  /**
-   * @param userRuleLocatorName The name of the rule used to locate the
-   *        user-supplied rule.
-   * @param userRuleWrapperName The name of the wrapper rule used to evaluate
-   *        the user-supplied rule.
-   * @param filterRuleLocatorName The name of the rule used to locate the filter
-   *        rule.
-   * @param filterRuleWrapperName The name of the rule used to evaluate the
-   *        filter rule.
-   */
-  public SubmitRuleEvaluator(ReviewDb db, PatchSet patchSet,
-      ProjectControl projectControl,
-      ChangeControl changeControl, Change change, ChangeData cd,
-      boolean fastEvalLabels,
-      String userRuleLocatorName, String userRuleWrapperName,
-      String filterRuleLocatorName, String filterRuleWrapperName) {
-    this(db, patchSet, projectControl, changeControl, change, cd,
-        fastEvalLabels, userRuleLocatorName, userRuleWrapperName,
-        filterRuleLocatorName, filterRuleWrapperName, false, null);
+  public SubmitRuleEvaluator(ChangeData cd) throws OrmException {
+    this.cd = cd;
+    this.control = cd.changeControl();
   }
 
   /**
-   * @param userRuleLocatorName The name of the rule used to locate the
-   *        user-supplied rule.
-   * @param userRuleWrapperName The name of the wrapper rule used to evaluate
-   *        the user-supplied rule.
-   * @param filterRuleLocatorName The name of the rule used to locate the filter
-   *        rule.
-   * @param filterRuleWrapperName The name of the rule used to evaluate the
-   *        filter rule.
-   * @param skipSubmitFilters if {@code true} submit filter will not be
-   *        applied
-   * @param rules when non-null the rules will be read from this input stream
-   *        instead of refs/meta/config:rules.pl file
+   * @param ps patch set to evaluate, rather than current patch set.
+   * @return this
    */
-  public SubmitRuleEvaluator(ReviewDb db, PatchSet patchSet,
-      ProjectControl projectControl,
-      ChangeControl changeControl, Change change, ChangeData cd,
-      boolean fastEvalLabels,
-      String userRuleLocatorName, String userRuleWrapperName,
-      String filterRuleLocatorName, String filterRuleWrapperName,
-      boolean skipSubmitFilters, InputStream rules) {
-    this.db = db;
-    this.patchSet = patchSet;
-    this.projectControl = projectControl;
-    this.changeControl = changeControl;
-    this.change = change;
-    this.cd = checkNotNull(cd, "ChangeData");
-    this.fastEvalLabels = fastEvalLabels;
-    this.userRuleLocatorName = userRuleLocatorName;
-    this.userRuleWrapperName = userRuleWrapperName;
-    this.filterRuleLocatorName = filterRuleLocatorName;
-    this.filterRuleWrapperName = filterRuleWrapperName;
-    this.skipFilters = skipSubmitFilters;
-    this.rulesInputStream = rules;
+  public SubmitRuleEvaluator setPatchSet(PatchSet ps) {
+    checkArgument(ps.getId().getParentKey().equals(cd.getId()));
+    patchSet = ps;
+    return this;
   }
 
   /**
-   * Evaluates the given rule and filters.
-   *
-   * Sets the {@link #submitRule} to the Term found by the
-   * {@link #userRuleLocatorName}. This can be used when reporting error(s) on
-   * unexpected return value of this method.
+   * @param fast if true, infer label information from rules rather than reading
+   *     from project config.
+   * @return this
+   */
+  public SubmitRuleEvaluator setFastEvalLabels(boolean fast) {
+    fastEvalLabels = fast;
+    return this;
+  }
+
+  /**
+   * @param skip if true, submit filter will not be applied.
+   * @return this
+   */
+  public SubmitRuleEvaluator setSkipSubmitFilters(boolean skip) {
+    skipFilters = skip;
+    return this;
+  }
+
+  /**
+   * @param rule custom rule to use, or null to use refs/meta/config:rules.pl.
+   * @return this
+   */
+  public SubmitRuleEvaluator setRule(@Nullable String rule) {
+    this.rule = rule;
+    return this;
+  }
+
+  /**
+   * Evaluate the submit rules.
    *
    * @return List of {@link Term} objects returned from the evaluated rules.
    * @throws RuleEvalException
    */
   public List<Term> evaluate() throws RuleEvalException {
+    return evaluateImpl("locate_submit_rule", "can_submit",
+          "locate_submit_filter", "filter_submit_results");
+  }
+
+  /**
+   * Evaluate the submit type rules.
+   *
+   * @return List of {@link Term} objects returned from the evaluated rules.
+   * @throws RuleEvalException
+   */
+  public List<Term> evaluateSubmitType() throws RuleEvalException {
+    return evaluateImpl("locate_submit_type", "get_submit_type",
+          "locate_submit_type_filter", "filter_submit_type_results");
+  }
+
+  private List<Term> evaluateImpl(
+      String userRuleLocatorName,
+      String userRuleWrapperName,
+      String filterRuleLocatorName,
+      String filterRuleWrapperName) throws RuleEvalException {
     PrologEnvironment env = getPrologEnvironment();
     try {
       submitRule = env.once("gerrit", userRuleLocatorName, new VariableTerm());
@@ -140,19 +136,16 @@ public class SubmitRuleEvaluator {
             submitRule, new VariableTerm())) {
           results.add(template[1]);
         }
-      } catch (PrologException err) {
-        throw new RuleEvalException("Exception calling " + submitRule
-            + " on change " + change.getId() + " of " + getProjectName(),
-            err);
       } catch (RuntimeException err) {
         throw new RuleEvalException("Exception calling " + submitRule
-            + " on change " + change.getId() + " of " + getProjectName(),
+            + " on change " + cd.getId() + " of " + getProjectName(),
             err);
       }
 
       Term resultsTerm = toListTerm(results);
       if (!skipFilters) {
-        resultsTerm = runSubmitFilters(resultsTerm, env);
+        resultsTerm = runSubmitFilters(
+            resultsTerm, env, filterRuleLocatorName, filterRuleWrapperName);
       }
       if (resultsTerm.isList()) {
         List<Term> r = Lists.newArrayList();
@@ -170,27 +163,39 @@ public class SubmitRuleEvaluator {
   }
 
   private PrologEnvironment getPrologEnvironment() throws RuleEvalException {
-    ProjectState projectState = projectControl.getProjectState();
+    if (patchSet == null) {
+      try {
+        patchSet = cd.currentPatchSet();
+      } catch (OrmException err) {
+        throw new RuleEvalException("Missing current patch set on change "
+            + cd.getId() + " of " + getProjectName(),
+            err);
+      }
+    }
+    ProjectState projectState = control.getProjectControl().getProjectState();
     PrologEnvironment env;
     try {
-      if (rulesInputStream == null) {
+      if (rule == null) {
         env = projectState.newPrologEnvironment();
       } else {
-        env = projectState.newPrologEnvironment("stdin", rulesInputStream);
+        env = projectState.newPrologEnvironment(
+            "stdin", new ByteArrayInputStream(rule.getBytes(UTF_8)));
       }
     } catch (CompileException err) {
       throw new RuleEvalException("Cannot consult rules.pl for "
           + getProjectName(), err);
     }
-    env.set(StoredValues.REVIEW_DB, db);
+    env.set(StoredValues.REVIEW_DB, cd.db());
     env.set(StoredValues.CHANGE_DATA, cd);
     env.set(StoredValues.PATCH_SET, patchSet);
-    env.set(StoredValues.CHANGE_CONTROL, changeControl);
+    env.set(StoredValues.CHANGE_CONTROL, control);
     return env;
   }
 
-  private Term runSubmitFilters(Term results, PrologEnvironment env) throws RuleEvalException {
-    ProjectState projectState = projectControl.getProjectState();
+  private Term runSubmitFilters(Term results, PrologEnvironment env,
+      String filterRuleLocatorName, String filterRuleWrapperName)
+      throws RuleEvalException {
+    ProjectState projectState = control.getProjectControl().getProjectState();
     PrologEnvironment childEnv = env;
     for (ProjectState parentState : projectState.parents()) {
       PrologEnvironment parentEnv;
@@ -215,11 +220,11 @@ public class SubmitRuleEvaluator {
         results = template[2];
       } catch (PrologException err) {
         throw new RuleEvalException("Exception calling " + filterRule
-            + " on change " + change.getId() + " of "
+            + " on change " + cd.getId() + " of "
             + parentState.getProject().getName(), err);
       } catch (RuntimeException err) {
         throw new RuleEvalException("Exception calling " + filterRule
-            + " on change " + change.getId() + " of "
+            + " on change " + cd.getId() + " of "
             + parentState.getProject().getName(), err);
       }
       childEnv = parentEnv;
@@ -240,9 +245,6 @@ public class SubmitRuleEvaluator {
   }
 
   private String getProjectName() {
-    if (projectName == null) {
-      projectName = projectControl.getProjectState().getProject().getName();
-    }
-    return projectName;
+    return control.getProjectControl().getProjectState().getProject().getName();
   }
 }

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

@@ -270,6 +270,10 @@ public class ChangeData {
     notes = c.getNotes();
   }
 
+  public ReviewDb db() {
+    return db;
+  }
+
   public boolean isFromSource(ChangeDataSource s) {
     return s == returnedBySource;
   }