Merge changes I4dab0a6c,I25445abc,I8fee313e
* changes:
Make TestSubmitRule compliant with docs
Add tests for revisions/current/test.submit_rule and fix subtle bug
Add RevisionApi#testSubmitRule
diff --git a/java/com/google/gerrit/extensions/api/changes/RevisionApi.java b/java/com/google/gerrit/extensions/api/changes/RevisionApi.java
index 3665706..4658eb3 100644
--- a/java/com/google/gerrit/extensions/api/changes/RevisionApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/RevisionApi.java
@@ -23,6 +23,7 @@
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.common.MergeableInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
+import com.google.gerrit.extensions.common.TestSubmitRuleInfo;
import com.google.gerrit.extensions.common.TestSubmitRuleInput;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.NotImplementedException;
@@ -128,6 +129,8 @@
SubmitType testSubmitType(TestSubmitRuleInput in) throws RestApiException;
+ List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException;
+
MergeListRequest getMergeList() throws RestApiException;
abstract class MergeListRequest {
@@ -358,6 +361,11 @@
}
@Override
+ public List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public MergeListRequest getMergeList() throws RestApiException {
throw new NotImplementedException();
}
diff --git a/java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java b/java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java
new file mode 100644
index 0000000..bd7ebcb
--- /dev/null
+++ b/java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java
@@ -0,0 +1,70 @@
+// Copyright (C) 2018 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.extensions.common;
+
+import com.google.common.base.MoreObjects;
+import java.util.Map;
+import java.util.Objects;
+
+public class TestSubmitRuleInfo {
+ /** @see com.google.gerrit.common.data.SubmitRecord.Status */
+ public String status;
+
+ public String errorMessage;
+ public Map<String, AccountInfo> ok;
+ public Map<String, AccountInfo> reject;
+ public Map<String, None> need;
+ public Map<String, AccountInfo> may;
+ public Map<String, None> impossible;
+
+ public static class None {
+ private None() {}
+
+ public static None INSTANCE = new None();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof TestSubmitRuleInfo) {
+ TestSubmitRuleInfo other = (TestSubmitRuleInfo) o;
+ return Objects.equals(status, other.status)
+ && Objects.equals(errorMessage, other.errorMessage)
+ && Objects.equals(ok, other.ok)
+ && Objects.equals(reject, other.reject)
+ && Objects.equals(need, other.need)
+ && Objects.equals(may, other.may)
+ && Objects.equals(impossible, other.impossible);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(status, errorMessage, ok, reject, need, may, impossible);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("status", status)
+ .add("errorMessage", errorMessage)
+ .add("ok", ok)
+ .add("reject", reject)
+ .add("need", need)
+ .add("may", may)
+ .add("impossible", impossible)
+ .toString();
+ }
+}
diff --git a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
index bd602a8..33f211d 100644
--- a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
@@ -43,6 +43,7 @@
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.common.MergeableInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
+import com.google.gerrit.extensions.common.TestSubmitRuleInfo;
import com.google.gerrit.extensions.common.TestSubmitRuleInput;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.IdString;
@@ -76,6 +77,7 @@
import com.google.gerrit.server.restapi.change.RevisionReviewers;
import com.google.gerrit.server.restapi.change.RobotComments;
import com.google.gerrit.server.restapi.change.Submit;
+import com.google.gerrit.server.restapi.change.TestSubmitRule;
import com.google.gerrit.server.restapi.change.TestSubmitType;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -125,6 +127,7 @@
private final GetRevisionActions revisionActions;
private final TestSubmitType testSubmitType;
private final TestSubmitType.Get getSubmitType;
+ private final Provider<TestSubmitRule> testSubmitRule;
private final Provider<GetMergeList> getMergeList;
private final PutDescription putDescription;
private final GetDescription getDescription;
@@ -164,6 +167,7 @@
GetRevisionActions revisionActions,
TestSubmitType testSubmitType,
TestSubmitType.Get getSubmitType,
+ Provider<TestSubmitRule> testSubmitRule,
Provider<GetMergeList> getMergeList,
PutDescription putDescription,
GetDescription getDescription,
@@ -201,6 +205,7 @@
this.revisionActions = revisionActions;
this.testSubmitType = testSubmitType;
this.getSubmitType = getSubmitType;
+ this.testSubmitRule = testSubmitRule;
this.getMergeList = getMergeList;
this.putDescription = putDescription;
this.getDescription = getDescription;
@@ -559,6 +564,15 @@
}
@Override
+ public List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException {
+ try {
+ return testSubmitRule.get().apply(revision, in);
+ } catch (Exception e) {
+ throw asRestApiException("Cannot test submit rule", e);
+ }
+ }
+
+ @Override
public MergeListRequest getMergeList() throws RestApiException {
return new MergeListRequest() {
@Override
diff --git a/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java b/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
index cdd7426..c7eb781 100644
--- a/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
+++ b/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
@@ -15,27 +15,32 @@
package com.google.gerrit.server.restapi.change;
import com.google.common.base.MoreObjects;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.TestSubmitRuleInfo;
import com.google.gerrit.extensions.common.TestSubmitRuleInput;
import com.google.gerrit.extensions.common.TestSubmitRuleInput.Filters;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.SubmitRuleEvaluator;
+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.DefaultSubmitRule;
+import com.google.gerrit.server.rules.PrologRule;
import com.google.gerrit.server.rules.RulesCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.LinkedHashMap;
import java.util.List;
-import java.util.Map;
import org.kohsuke.args4j.Option;
public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubmitRuleInput> {
@@ -43,7 +48,9 @@
private final ChangeData.Factory changeDataFactory;
private final RulesCache rules;
private final AccountLoader.Factory accountInfoFactory;
- private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
+ private final ProjectCache projectCache;
+ private final DefaultSubmitRule defaultSubmitRule;
+ private final PrologRule prologRule;
@Option(name = "--filters", usage = "impact of filters in parent projects")
private Filters filters = Filters.RUN;
@@ -54,17 +61,21 @@
ChangeData.Factory changeDataFactory,
RulesCache rules,
AccountLoader.Factory infoFactory,
- SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) {
+ ProjectCache projectCache,
+ DefaultSubmitRule defaultSubmitRule,
+ PrologRule prologRule) {
this.db = db;
this.changeDataFactory = changeDataFactory;
this.rules = rules;
this.accountInfoFactory = infoFactory;
- this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
+ this.projectCache = projectCache;
+ this.defaultSubmitRule = defaultSubmitRule;
+ this.prologRule = prologRule;
}
@Override
- public List<Record> apply(RevisionResource rsrc, TestSubmitRuleInput input)
- throws AuthException, OrmException, PermissionBackendException {
+ public List<TestSubmitRuleInfo> apply(RevisionResource rsrc, TestSubmitRuleInput input)
+ throws AuthException, OrmException, PermissionBackendException, BadRequestException {
if (input == null) {
input = new TestSubmitRuleInput();
}
@@ -80,74 +91,76 @@
.logErrors(false)
.build();
+ ProjectState projectState = projectCache.get(rsrc.getProject());
+ if (projectState == null) {
+ throw new BadRequestException("project not found");
+ }
ChangeData cd = changeDataFactory.create(db.get(), rsrc.getNotes());
- List<SubmitRecord> records = submitRuleEvaluatorFactory.create(opts).evaluate(cd);
+ List<SubmitRecord> records;
+ if (projectState.hasPrologRules() || input.rule != null) {
+ records = ImmutableList.copyOf(prologRule.evaluate(cd, opts));
+ } else {
+ // No rules were provided as input and we have no rules.pl. This means we are supposed to run
+ // the default rules. Nowadays, the default rules are implemented in Java, not Prolog.
+ // Therefore, we call the DefaultRuleEvaluator instead.
+ records = ImmutableList.copyOf(defaultSubmitRule.evaluate(cd, opts));
+ }
- List<Record> out = Lists.newArrayListWithCapacity(records.size());
+ List<TestSubmitRuleInfo> out = Lists.newArrayListWithCapacity(records.size());
AccountLoader accounts = accountInfoFactory.create(true);
for (SubmitRecord r : records) {
- out.add(new Record(r, accounts));
+ out.add(newSubmitRuleInfo(r, accounts));
}
accounts.fill();
return out;
}
- static class Record {
- SubmitRecord.Status status;
- String errorMessage;
- Map<String, AccountInfo> ok;
- Map<String, AccountInfo> reject;
- Map<String, None> need;
- Map<String, AccountInfo> may;
- Map<String, None> impossible;
+ private static TestSubmitRuleInfo newSubmitRuleInfo(SubmitRecord r, AccountLoader accounts) {
+ TestSubmitRuleInfo info = new TestSubmitRuleInfo();
+ info.status = r.status.name();
+ info.errorMessage = r.errorMessage;
- Record(SubmitRecord r, AccountLoader accounts) {
- this.status = r.status;
- this.errorMessage = r.errorMessage;
-
- if (r.labels != null) {
- for (SubmitRecord.Label n : r.labels) {
- AccountInfo who = n.appliedBy != null ? accounts.get(n.appliedBy) : new AccountInfo(null);
- label(n, who);
- }
+ if (r.labels != null) {
+ for (SubmitRecord.Label n : r.labels) {
+ AccountInfo who = n.appliedBy != null ? accounts.get(n.appliedBy) : new AccountInfo(null);
+ label(info, n, who);
}
}
-
- private void label(SubmitRecord.Label n, AccountInfo who) {
- switch (n.status) {
- case OK:
- if (ok == null) {
- ok = new LinkedHashMap<>();
- }
- ok.put(n.label, who);
- break;
- case REJECT:
- if (reject == null) {
- reject = new LinkedHashMap<>();
- }
- reject.put(n.label, who);
- break;
- case NEED:
- if (need == null) {
- need = new LinkedHashMap<>();
- }
- need.put(n.label, new None());
- break;
- case MAY:
- if (may == null) {
- may = new LinkedHashMap<>();
- }
- may.put(n.label, who);
- break;
- case IMPOSSIBLE:
- if (impossible == null) {
- impossible = new LinkedHashMap<>();
- }
- impossible.put(n.label, new None());
- break;
- }
- }
+ return info;
}
- static class None {}
+ private static void label(TestSubmitRuleInfo info, SubmitRecord.Label n, AccountInfo who) {
+ switch (n.status) {
+ case OK:
+ if (info.ok == null) {
+ info.ok = new LinkedHashMap<>();
+ }
+ info.ok.put(n.label, who);
+ break;
+ case REJECT:
+ if (info.reject == null) {
+ info.reject = new LinkedHashMap<>();
+ }
+ info.reject.put(n.label, who);
+ break;
+ case NEED:
+ if (info.need == null) {
+ info.need = new LinkedHashMap<>();
+ }
+ info.need.put(n.label, TestSubmitRuleInfo.None.INSTANCE);
+ break;
+ case MAY:
+ if (info.may == null) {
+ info.may = new LinkedHashMap<>();
+ }
+ info.may.put(n.label, who);
+ break;
+ case IMPOSSIBLE:
+ if (info.impossible == null) {
+ info.impossible = new LinkedHashMap<>();
+ }
+ info.impossible.put(n.label, TestSubmitRuleInfo.None.INSTANCE);
+ break;
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/rules/PrologRule.java b/java/com/google/gerrit/server/rules/PrologRule.java
index deddc36..0c54f40 100644
--- a/java/com/google/gerrit/server/rules/PrologRule.java
+++ b/java/com/google/gerrit/server/rules/PrologRule.java
@@ -39,8 +39,8 @@
@Override
public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions opts) {
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()) {
+ // 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) {
return Collections.emptyList();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
index f73a59c..507205d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
@@ -29,6 +29,7 @@
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.TestSubmitRuleInfo;
import com.google.gerrit.extensions.common.TestSubmitRuleInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -255,6 +256,36 @@
}
}
+ @Test
+ public void invalidSubmitRuleWithNoRulesInProject() throws Exception {
+ String changeId = createChange("master", "change 1").getChangeId();
+
+ TestSubmitRuleInput in = new TestSubmitRuleInput();
+ in.rule = "invalid prolog rule";
+ // We have no rules.pl by default. The fact that the default rules are showing up here is a bug.
+ List<TestSubmitRuleInfo> response = gApi.changes().id(changeId).current().testSubmitRule(in);
+ assertThat(response).containsExactly(invalidPrologRuleInfo());
+ }
+
+ @Test
+ public void invalidSubmitRuleWithRulesInProject() throws Exception {
+ setRulesPl(SUBMIT_TYPE_FROM_SUBJECT);
+
+ String changeId = createChange("master", "change 1").getChangeId();
+
+ TestSubmitRuleInput in = new TestSubmitRuleInput();
+ in.rule = "invalid prolog rule";
+ List<TestSubmitRuleInfo> response = gApi.changes().id(changeId).current().testSubmitRule(in);
+ assertThat(response).containsExactly(invalidPrologRuleInfo());
+ }
+
+ private static TestSubmitRuleInfo invalidPrologRuleInfo() {
+ TestSubmitRuleInfo info = new TestSubmitRuleInfo();
+ info.status = "RULE_ERROR";
+ info.errorMessage = "operator expected after expression at: invalid prolog rule end_of_file.";
+ return info;
+ }
+
private List<RevCommit> log(String commitish, int n) throws Exception {
try (Repository repo = repoManager.openRepository(project);
Git git = new Git(repo)) {
