SubmitRule: Remove SubmitRuleOptions arg from evaluate method

The SubmitRuleOptions contain mostly Prolog-specific settings and are
only used by the PrologRule. All other implementation of SubmitRule
ignore the options, hence the interface is cleaner without the
SubmitRuleOptions arg.

All Prolog-specific settings from SubmitRuleOptions are now moved into a
new PrologOptions AutoValue class that is only used by PrologRule and
PrologRuleEvaluator. There are only 2 variants of the prolog options
that are used, one for checking the submittability of changes against
the Prolog rules that are configured in the project, one for testing
Prolog rules that are passed in into the TestSubmitRule / TestSubmitType
REST endpoints. For the first case we go through SubmitRuleEvaluator. In
this case we pass no options to PrologRule, but PrologRule just uses the
default options which work for production. For the second case we invoke
PrologRule directly and pass in the dry run options.

SubmitRuleOptions contains only a single option now (allowClosed), but
this options is checked before submit rules are invoked. Hence this
option is not relevant for SubmitRule implementations.

The SubmitRuleOptions that are used for computing submit records that
are stored in the change index (SUBMIT_RULE_OPTIONS_LENIENT and
SUBMIT_RULE_OPTIONS_STRICT) are not changed since they only use the one
field in SubmitRuleOptions that is still there (allowClosed).

Change-Id: Ifa474e2c16fee02f52b6e6339bdf980b8f4f3a50
Signed-off-by: Edwin Kempin <ekempin@google.com>

java/com/google/gerrit/server/project/SubmitRuleEvaluator.java

@@ -112,17 +112,14 @@ public class SubmitRuleEvaluator {
     // We evaluate all the plugin-defined evaluators,
     // and then we collect the results in one list.
     return Streams.stream(submitRules)
-        .map(c -> c.call(s -> s.evaluate(cd, opts)))
+        .map(c -> c.call(s -> s.evaluate(cd)))
         .flatMap(Collection::stream)
         .collect(Collectors.toList());
   }
 
   private List<SubmitRecord> ruleError(String err, Exception e) {
-    if (opts.logErrors()) {
+    logger.atSevere().withCause(e).log(err);
-      logger.atSevere().withCause(e).log(err);
+    return defaultRuleError();
-      return defaultRuleError();
-    }
-    return createRuleError(err);
   }
 
   /**
@@ -142,14 +139,11 @@ public class SubmitRuleEvaluator {
       return typeError("Error looking up change " + cd.getId(), e);
     }
 
-    return prologRule.getSubmitType(cd, opts);
+    return prologRule.getSubmitType(cd);
   }
 
   private SubmitTypeRecord typeError(String err, Exception e) {
-    if (opts.logErrors()) {
+    logger.atSevere().withCause(e).log(err);
-      logger.atSevere().withCause(e).log(err);
+    return defaultTypeError();
-      return defaultTypeError();
-    }
-    return SubmitTypeRecord.error(err);
   }
 }

java/com/google/gerrit/server/project/SubmitRuleOptions.java

@@ -15,7 +15,6 @@
 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.
@@ -26,12 +25,7 @@ import com.google.gerrit.common.Nullable;
 @AutoValue
 public abstract class SubmitRuleOptions {
   private static final SubmitRuleOptions defaults =
-      new AutoValue_SubmitRuleOptions.Builder()
+      new AutoValue_SubmitRuleOptions.Builder().allowClosed(false).build();
-          .allowClosed(false)
-          .skipFilters(false)
-          .logErrors(true)
-          .rule(null)
-          .build();
 
   public static SubmitRuleOptions defaults() {
     return defaults;
@@ -43,25 +37,12 @@ public abstract class SubmitRuleOptions {
 
   public abstract boolean allowClosed();
 
-  public abstract boolean skipFilters();
-
-  public abstract boolean logErrors();
-
-  @Nullable
-  public abstract String rule();
-
   public abstract Builder toBuilder();
 
   @AutoValue.Builder
   public abstract static class Builder {
     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.Builder logErrors(boolean logErrors);
-
     public abstract SubmitRuleOptions build();
   }
 }

java/com/google/gerrit/server/restapi/change/TestSubmitRule.java

@@ -31,8 +31,8 @@ import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.PrologOptions;
 import com.google.gerrit.server.rules.PrologRule;
 import com.google.gerrit.server.rules.RulesCache;
 import com.google.inject.Inject;
@@ -78,19 +78,15 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
     }
     input.filters = MoreObjects.firstNonNull(input.filters, filters);
 
-    SubmitRuleOptions opts =
-        SubmitRuleOptions.builder()
-            .skipFilters(input.filters == Filters.SKIP)
-            .rule(input.rule)
-            .logErrors(false)
-            .build();
-
     ProjectState projectState = projectCache.get(rsrc.getProject());
     if (projectState == null) {
       throw new BadRequestException("project not found");
     }
     ChangeData cd = changeDataFactory.create(rsrc.getNotes());
-    List<SubmitRecord> records = ImmutableList.copyOf(prologRule.evaluate(cd, opts));
+    List<SubmitRecord> records =
+        ImmutableList.copyOf(
+            prologRule.evaluate(
+                cd, PrologOptions.dryRunOptions(input.rule, input.filters == Filters.SKIP)));
     List<TestSubmitRuleInfo> out = Lists.newArrayListWithCapacity(records.size());
     AccountLoader accounts = accountInfoFactory.create(true);
     for (SubmitRecord r : records) {

java/com/google/gerrit/server/restapi/change/TestSubmitType.java

@@ -29,6 +29,7 @@ import com.google.gerrit.server.change.RevisionResource;
 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.rules.PrologOptions;
 import com.google.gerrit.server.rules.PrologRule;
 import com.google.gerrit.server.rules.RulesCache;
 import com.google.inject.Inject;
@@ -63,15 +64,10 @@ public class TestSubmitType implements RestModifyView<RevisionResource, TestSubm
     }
     input.filters = MoreObjects.firstNonNull(input.filters, filters);
 
-    SubmitRuleOptions opts =
-        SubmitRuleOptions.builder()
-            .logErrors(false)
-            .skipFilters(input.filters == Filters.SKIP)
-            .rule(input.rule)
-            .build();
-
     ChangeData cd = changeDataFactory.create(rsrc.getNotes());
-    SubmitTypeRecord rec = prologRule.getSubmitType(cd, opts);
+    SubmitTypeRecord rec =
+        prologRule.getSubmitType(
+            cd, PrologOptions.dryRunOptions(input.rule, input.filters == Filters.SKIP));
 
     if (rec.status != SubmitTypeRecord.Status.OK) {
       throw new BadRequestException(String.format("rule produced invalid result: %s", rec));

java/com/google/gerrit/server/rules/DefaultSubmitRule.java

@@ -26,7 +26,6 @@ import com.google.gerrit.extensions.config.FactoryModule;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -63,7 +62,7 @@ public final class DefaultSubmitRule implements SubmitRule {
   }
 
   @Override
-  public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions options) {
+  public Collection<SubmitRecord> evaluate(ChangeData cd) {
     ProjectState projectState = projectCache.get(cd.project());
 
     // In case at least one project has a rules.pl file, we let Prolog handle it.

java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java

@@ -28,7 +28,6 @@ import com.google.gerrit.extensions.annotations.Exports;
 import com.google.gerrit.extensions.config.FactoryModule;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -60,7 +59,7 @@ public class IgnoreSelfApprovalRule implements SubmitRule {
   IgnoreSelfApprovalRule() {}
 
   @Override
-  public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions options) {
+  public Collection<SubmitRecord> evaluate(ChangeData cd) {
     List<LabelType> labelTypes;
     List<PatchSetApproval> approvals;
     try {

java/com/google/gerrit/server/rules/PrologOptions.java

@@ -0,0 +1,57 @@
+// Copyright (C) 2019 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.rules;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.Nullable;
+import java.util.Optional;
+
+@AutoValue
+public abstract class PrologOptions {
+  public static PrologOptions defaultOptions() {
+    return new AutoValue_PrologOptions.Builder().logErrors(true).skipFilters(false).build();
+  }
+
+  public static PrologOptions dryRunOptions(String ruleToTest, boolean skipFilters) {
+    return new AutoValue_PrologOptions.Builder()
+        .logErrors(false)
+        .skipFilters(skipFilters)
+        .rule(ruleToTest)
+        .build();
+  }
+
+  /** Whether errors should be logged. */
+  abstract boolean logErrors();
+
+  /** Whether Prolog filters from parent projects should be skipped. */
+  abstract boolean skipFilters();
+
+  /**
+   * Prolog rule that should be run. If not given, the Prolog rule that is configured for the
+   * project is used (the rule from rules.pl in refs/meta/config).
+   */
+  abstract Optional<String> rule();
+
+  @AutoValue.Builder
+  abstract static class Builder {
+    abstract PrologOptions.Builder logErrors(boolean logErrors);
+
+    abstract PrologOptions.Builder skipFilters(boolean skipFilters);
+
+    abstract PrologOptions.Builder rule(@Nullable String rule);
+
+    abstract PrologOptions build();
+  }
+}

java/com/google/gerrit/server/rules/PrologRule.java

@@ -18,7 +18,6 @@ import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.common.data.SubmitTypeRecord;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -37,21 +36,29 @@ public class PrologRule implements SubmitRule {
   }
 
   @Override
-  public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions opts) {
+  public Collection<SubmitRecord> evaluate(ChangeData cd) {
     ProjectState projectState = projectCache.get(cd.project());
     // We only want to run the Prolog engine if we have at least one rules.pl file to use.
-    if ((projectState == null || !projectState.hasPrologRules()) && opts.rule() == null) {
+    if ((projectState == null || !projectState.hasPrologRules())) {
       return Collections.emptyList();
     }
 
+    return evaluate(cd, PrologOptions.defaultOptions());
+  }
+
+  public Collection<SubmitRecord> evaluate(ChangeData cd, PrologOptions opts) {
     return getEvaluator(cd, opts).evaluate();
   }
 
-  private PrologRuleEvaluator getEvaluator(ChangeData cd, SubmitRuleOptions opts) {
+  public SubmitTypeRecord getSubmitType(ChangeData cd) {
-    return factory.create(cd, opts);
+    return getSubmitType(cd, PrologOptions.defaultOptions());
   }
 
-  public SubmitTypeRecord getSubmitType(ChangeData cd, SubmitRuleOptions opts) {
+  public SubmitTypeRecord getSubmitType(ChangeData cd, PrologOptions opts) {
     return getEvaluator(cd, opts).getSubmitType();
   }
+
+  private PrologRuleEvaluator getEvaluator(ChangeData cd, PrologOptions opts) {
+    return factory.create(cd, opts);
+  }
 }

java/com/google/gerrit/server/rules/PrologRuleEvaluator.java

@@ -35,7 +35,6 @@ import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.project.RuleEvalException;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
@@ -73,7 +72,7 @@ public class PrologRuleEvaluator {
 
   public interface Factory {
     /** Returns a new {@link PrologRuleEvaluator} with the specified options */
-    PrologRuleEvaluator create(ChangeData cd, SubmitRuleOptions options);
+    PrologRuleEvaluator create(ChangeData cd, PrologOptions options);
   }
 
   /**
@@ -95,7 +94,7 @@ public class PrologRuleEvaluator {
   private final PrologEnvironment.Factory envFactory;
   private final ChangeData cd;
   private final ProjectState projectState;
-  private final SubmitRuleOptions opts;
+  private final PrologOptions opts;
   private Term submitRule;
 
   @AssistedInject
@@ -107,7 +106,7 @@ public class PrologRuleEvaluator {
       PrologEnvironment.Factory envFactory,
       ProjectCache projectCache,
       @Assisted ChangeData cd,
-      @Assisted SubmitRuleOptions options) {
+      @Assisted PrologOptions options) {
     this.accountCache = accountCache;
     this.accounts = accounts;
     this.emails = emails;
@@ -459,22 +458,22 @@ public class PrologRuleEvaluator {
     PrologEnvironment env;
     try {
       PrologMachineCopy pmc;
-      if (opts.rule() == null) {
+      if (opts.rule().isPresent()) {
+        pmc = rulesCache.loadMachine("stdin", new StringReader(opts.rule().get()));
+      } else {
         pmc =
             rulesCache.loadMachine(
                 projectState.getNameKey(), projectState.getConfig().getRulesId());
-      } else {
-        pmc = rulesCache.loadMachine("stdin", new StringReader(opts.rule()));
       }
       env = envFactory.create(pmc);
     } catch (CompileException err) {
       String msg;
-      if (opts.rule() == null) {
+      if (opts.rule().isPresent()) {
+        msg = err.getMessage();
+      } else {
         msg =
             String.format(
                 "Cannot load rules.pl for %s: %s", projectState.getName(), err.getMessage());
-      } else {
-        msg = err.getMessage();
       }
       throw new RuleEvalException(msg, err);
     }

java/com/google/gerrit/server/rules/SubmitRule.java

@@ -15,7 +15,6 @@ package com.google.gerrit.server.rules;
 
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.extensions.annotations.ExtensionPoint;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import java.util.Collection;
 
@@ -40,5 +39,5 @@ import java.util.Collection;
 @ExtensionPoint
 public interface SubmitRule {
   /** Returns a {@link Collection} of {@link SubmitRecord} status for the change. */
-  Collection<SubmitRecord> evaluate(ChangeData changeData, SubmitRuleOptions options);
+  Collection<SubmitRecord> evaluate(ChangeData changeData);
 }

javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java

@@ -26,7 +26,6 @@ import com.google.gerrit.extensions.annotations.Exports;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.SubmitRequirementInfo;
 import com.google.gerrit.extensions.config.FactoryModule;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.rules.SubmitRule;
 import com.google.inject.Module;
@@ -67,7 +66,7 @@ public class ChangeSubmitRequirementIT extends AbstractDaemonTest {
 
   private static class CustomSubmitRule implements SubmitRule {
     @Override
-    public Collection<SubmitRecord> evaluate(ChangeData changeData, SubmitRuleOptions options) {
+    public Collection<SubmitRecord> evaluate(ChangeData changeData) {
       SubmitRecord record = new SubmitRecord();
       record.labels = new ArrayList<>();
       record.status = SubmitRecord.Status.NOT_READY;

javatests/com/google/gerrit/acceptance/rest/TraceIT.java

@@ -46,7 +46,6 @@ import com.google.gerrit.server.logging.Metadata;
 import com.google.gerrit.server.logging.PerformanceLogger;
 import com.google.gerrit.server.logging.TraceContext;
 import com.google.gerrit.server.project.CreateProjectArgs;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.rules.SubmitRule;
 import com.google.gerrit.server.validators.ProjectCreationValidationListener;
@@ -680,7 +679,7 @@ public class TraceIT extends AbstractDaemonTest {
     boolean failOnce;
 
     @Override
-    public Collection<SubmitRecord> evaluate(ChangeData changeData, SubmitRuleOptions options) {
+    public Collection<SubmitRecord> evaluate(ChangeData changeData) {
       if (failOnce) {
         failOnce = false;
         throw new IllegalStateException("forced failure from test");

javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java

@@ -22,7 +22,6 @@ import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.common.data.SubmitRequirement;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.rules.IgnoreSelfApprovalRule;
 import com.google.inject.Inject;
 import java.util.Collection;
@@ -42,8 +41,7 @@ public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest {
     PushOneCommit.Result r = createChange();
     approve(r.getChangeId());
 
-    Collection<SubmitRecord> submitRecords =
+    Collection<SubmitRecord> submitRecords = rule.evaluate(r.getChange());
-        rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
 
     assertThat(submitRecords).hasSize(1);
     SubmitRecord result = submitRecords.iterator().next();
@@ -69,8 +67,7 @@ public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest {
     // Approve as admin
     approve(r.getChangeId());
 
-    Collection<SubmitRecord> submitRecords =
+    Collection<SubmitRecord> submitRecords = rule.evaluate(r.getChange());
-        rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
     assertThat(submitRecords).isEmpty();
   }
 
@@ -81,8 +78,7 @@ public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest {
     PushOneCommit.Result r = createChange();
     approve(r.getChangeId());
 
-    Collection<SubmitRecord> submitRecords =
+    Collection<SubmitRecord> submitRecords = rule.evaluate(r.getChange());
-        rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
     assertThat(submitRecords).isEmpty();
   }
 

javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java

@@ -21,8 +21,8 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.PrologOptions;
 import com.google.gerrit.server.rules.PrologRuleEvaluator;
 import com.google.gerrit.testing.TestChanges;
 import com.google.inject.Inject;
@@ -156,6 +156,6 @@ public class PrologRuleEvaluatorIT extends AbstractDaemonTest {
   }
 
   private PrologRuleEvaluator makeEvaluator() {
-    return evaluatorFactory.create(makeChangeData(), SubmitRuleOptions.defaults());
+    return evaluatorFactory.create(makeChangeData(), PrologOptions.defaultOptions());
   }
 }