diff --git a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index 5879430a63..96eff882b4 100644 --- a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/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 ruleError(String err, Exception e) { - if (opts.logErrors()) { - logger.atSevere().withCause(e).log(err); - return defaultRuleError(); - } - return createRuleError(err); + logger.atSevere().withCause(e).log(err); + return defaultRuleError(); } /** @@ -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); - return defaultTypeError(); - } - return SubmitTypeRecord.error(err); + logger.atSevere().withCause(e).log(err); + return defaultTypeError(); } } diff --git a/java/com/google/gerrit/server/project/SubmitRuleOptions.java b/java/com/google/gerrit/server/project/SubmitRuleOptions.java index a4340b2bad..ad077c0aaa 100644 --- a/java/com/google/gerrit/server/project/SubmitRuleOptions.java +++ b/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() - .allowClosed(false) - .skipFilters(false) - .logErrors(true) - .rule(null) - .build(); + new AutoValue_SubmitRuleOptions.Builder().allowClosed(false).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(); } } diff --git a/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java b/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java index 1a8f0fa899..d5ed9a40f1 100644 --- a/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java +++ b/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 records = ImmutableList.copyOf(prologRule.evaluate(cd, opts)); + List records = + ImmutableList.copyOf( + prologRule.evaluate( + cd, PrologOptions.dryRunOptions(input.rule, input.filters == Filters.SKIP))); List out = Lists.newArrayListWithCapacity(records.size()); AccountLoader accounts = accountInfoFactory.create(true); for (SubmitRecord r : records) { diff --git a/java/com/google/gerrit/server/restapi/change/TestSubmitType.java b/java/com/google/gerrit/server/restapi/change/TestSubmitType.java index c5e1841b46..cb52fcba3c 100644 --- a/java/com/google/gerrit/server/restapi/change/TestSubmitType.java +++ b/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 evaluate(ChangeData cd, SubmitRuleOptions options) { + public Collection 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. diff --git a/java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java b/java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java index 4695800e58..ff5d99ecdd 100644 --- a/java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java +++ b/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 evaluate(ChangeData cd, SubmitRuleOptions options) { + public Collection evaluate(ChangeData cd) { List labelTypes; List approvals; try { diff --git a/java/com/google/gerrit/server/rules/PrologOptions.java b/java/com/google/gerrit/server/rules/PrologOptions.java new file mode 100644 index 0000000000..da9b3aba64 --- /dev/null +++ b/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 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(); + } +} diff --git a/java/com/google/gerrit/server/rules/PrologRule.java b/java/com/google/gerrit/server/rules/PrologRule.java index 0c54f40b28..e15b4b5e2f 100644 --- a/java/com/google/gerrit/server/rules/PrologRule.java +++ b/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 evaluate(ChangeData cd, SubmitRuleOptions opts) { + public Collection 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 evaluate(ChangeData cd, PrologOptions opts) { return getEvaluator(cd, opts).evaluate(); } - private PrologRuleEvaluator getEvaluator(ChangeData cd, SubmitRuleOptions opts) { - return factory.create(cd, opts); + public SubmitTypeRecord getSubmitType(ChangeData cd) { + 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); + } } diff --git a/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java b/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java index 4fbde637b8..7f6450d2df 100644 --- a/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java +++ b/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); } diff --git a/java/com/google/gerrit/server/rules/SubmitRule.java b/java/com/google/gerrit/server/rules/SubmitRule.java index 2a68683db5..20cb8fb18d 100644 --- a/java/com/google/gerrit/server/rules/SubmitRule.java +++ b/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 evaluate(ChangeData changeData, SubmitRuleOptions options); + Collection evaluate(ChangeData changeData); } diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java index f087b78c07..1842a9ec69 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java +++ b/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 evaluate(ChangeData changeData, SubmitRuleOptions options) { + public Collection evaluate(ChangeData changeData) { SubmitRecord record = new SubmitRecord(); record.labels = new ArrayList<>(); record.status = SubmitRecord.Status.NOT_READY; diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java index 56a9b69c4e..15b9a935bf 100644 --- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java +++ b/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 evaluate(ChangeData changeData, SubmitRuleOptions options) { + public Collection evaluate(ChangeData changeData) { if (failOnce) { failOnce = false; throw new IllegalStateException("forced failure from test"); diff --git a/javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java b/javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java index 83782c912e..37237c6abd 100644 --- a/javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java +++ b/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 submitRecords = - rule.evaluate(r.getChange(), SubmitRuleOptions.defaults()); + Collection submitRecords = rule.evaluate(r.getChange()); 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 submitRecords = - rule.evaluate(r.getChange(), SubmitRuleOptions.defaults()); + Collection submitRecords = rule.evaluate(r.getChange()); assertThat(submitRecords).isEmpty(); } @@ -81,8 +78,7 @@ public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest { PushOneCommit.Result r = createChange(); approve(r.getChangeId()); - Collection submitRecords = - rule.evaluate(r.getChange(), SubmitRuleOptions.defaults()); + Collection submitRecords = rule.evaluate(r.getChange()); assertThat(submitRecords).isEmpty(); } diff --git a/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java b/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java index c6f2024c76..efc3b5b9c5 100644 --- a/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java +++ b/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()); } }