opendev
/
gerrit
Code Issues Proposed changes

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
This commit is contained in:
Patrick Hiesel 2018-10-16 12:55:43 +00:00 committed by Gerrit Code Review
parent c18ef2b77a d7c89a8dbc
commit acf73405d8
6 changed files with 201 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/extensions/api/changes/RevisionApi.java
View File

@ -23,6 +23,7 @@ import com.google.gerrit.extensions.common.EditInfo;
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 @@ public interface RevisionApi {
SubmitType testSubmitType(TestSubmitRuleInput in) throws RestApiException;
List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException;
MergeListRequest getMergeList() throws RestApiException;
abstract class MergeListRequest {
@ -357,6 +360,11 @@ public interface RevisionApi {
throw new NotImplementedException();
}
@Override
public List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException {
throw new NotImplementedException();
}
@Override
public MergeListRequest getMergeList() throws RestApiException {
throw new NotImplementedException();

70
java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java Normal file
View File

@ -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();
}
}

14
java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
View File

@ -43,6 +43,7 @@ import com.google.gerrit.extensions.common.FileInfo;
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.Reviewed;
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 @@ class RevisionApiImpl implements RevisionApi {
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 @@ class RevisionApiImpl implements RevisionApi {
GetRevisionActions revisionActions,
TestSubmitType testSubmitType,
TestSubmitType.Get getSubmitType,
Provider<TestSubmitRule> testSubmitRule,
Provider<GetMergeList> getMergeList,
PutDescription putDescription,
GetDescription getDescription,
@ -201,6 +205,7 @@ class RevisionApiImpl implements RevisionApi {
this.revisionActions = revisionActions;
this.testSubmitType = testSubmitType;
this.getSubmitType = getSubmitType;
this.testSubmitRule = testSubmitRule;
this.getMergeList = getMergeList;
this.putDescription = putDescription;
this.getDescription = getDescription;
@ -558,6 +563,15 @@ class RevisionApiImpl implements RevisionApi {
}
}
@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() {

139
java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
View File

@ -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 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
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 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
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 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
.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);
}
}
}
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;
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);
}
}
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;
}
}
}

4
java/com/google/gerrit/server/rules/PrologRule.java
View File

@ -39,8 +39,8 @@ public class PrologRule implements SubmitRule {
@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();
}

31
javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
View File

@ -29,6 +29,7 @@ import com.google.gerrit.acceptance.PushOneCommit;
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 @@ public class SubmitTypeRuleIT extends AbstractDaemonTest {
}
}
@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)) {