opendev/gerrit
Code Issues Proposed changes

TestSubmitRule/TestSubmitType: Require input rule

Browse Source 
If the input doesn't contain a rule to test we now reject the request as
'400 Bad Request'.

In the documentation the rule field in RuleInput was already marked as
required and the behavior when the rule was not set was undocumented.
Hence changing this API should ok.

If the input didn't contain a rule the REST endpoints were "testing"
the rules that have been configured in the project. This functionality
seems to be unneeded. For the current revision this information is
available from ChangeInfo, and for other revisions this information is
not really interesting.

Unfortunately the TestSubmitRule.Get REST endpoint exposes the submit
type for an arbitrary revision. This is likely only needed for tests,
but since it's in the REST and extension API we cannot easily remove it.
Since the TestSubmitType.Get REST endpoint delegated to TestSubmitType
with no rule input it had to be rewritten. This fixes some issues with
this REST endpoints:

* error were not logged, because by delegating to TestSubmitType it was
  in "test" mode which suppressed errors
* '400 Bad Request' was returned if there was a rule error, but the rule
  was not provided by the caller, hence it was not a bad request,
  returning '409 Conflict' in this case is more appropriate

TestSubmitRule can now invoke PrologRule directly without going through
SubmitRuleEvaluator which makes the code a little simpler.

This is a preparation to cleanup the SubmitRule interface and remove the
SubmitRuleOptions arg from it.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I728988722ff916f2cbe25b33c54d3a0c2aec1e28
This commit is contained in:
Edwin Kempin
2019-08-07 15:17:58 +02:00
parent 4bf1608967
commit 4bc54e5fdc
2 changed files with 33 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -33,7 +33,6 @@ 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.inject.Inject;
@@ -46,7 +45,6 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
private final RulesCache rules;
private final AccountLoader.Factory accountInfoFactory;
private final ProjectCache projectCache;
private final DefaultSubmitRule defaultSubmitRule;
private final PrologRule prologRule;
@Option(name = "--filters", usage = "impact of filters in parent projects")
@@ -58,13 +56,11 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
RulesCache rules,
AccountLoader.Factory infoFactory,
ProjectCache projectCache,
DefaultSubmitRule defaultSubmitRule,
PrologRule prologRule) {
this.changeDataFactory = changeDataFactory;
this.rules = rules;
this.accountInfoFactory = infoFactory;
this.projectCache = projectCache;
this.defaultSubmitRule = defaultSubmitRule;
this.prologRule = prologRule;
}
@@ -74,7 +70,10 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
if (input == null) {
input = new TestSubmitRuleInput();
}
if (input.rule != null && !rules.isProjectRulesEnabled()) {
if (input.rule == null) {
throw new BadRequestException("rule is required");
}
if (!rules.isProjectRulesEnabled()) {
throw new AuthException("project rules are disabled");
}
input.filters = MoreObjects.firstNonNull(input.filters, filters);
@@ -91,16 +90,7 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
throw new BadRequestException("project not found");
}
ChangeData cd = changeDataFactory.create(rsrc.getNotes());
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<SubmitRecord> records = ImmutableList.copyOf(prologRule.evaluate(cd, opts));
List<TestSubmitRuleInfo> out = Lists.newArrayListWithCapacity(records.size());
AccountLoader accounts = accountInfoFactory.create(true);
for (SubmitRecord r : records) {

42
java/com/google/gerrit/server/restapi/change/TestSubmitType.java
View File

@@ -21,6 +21,7 @@ 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.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.RestReadView;
@@ -28,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.PrologRule;
import com.google.gerrit.server.rules.RulesCache;
import com.google.inject.Inject;
import org.kohsuke.args4j.Option;
@@ -35,19 +37,16 @@ import org.kohsuke.args4j.Option;
public class TestSubmitType implements RestModifyView<RevisionResource, TestSubmitRuleInput> {
private final ChangeData.Factory changeDataFactory;
private final RulesCache rules;
private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
private final PrologRule prologRule;
@Option(name = "--filters", usage = "impact of filters in parent projects")
private Filters filters = Filters.RUN;
@Inject
TestSubmitType(
ChangeData.Factory changeDataFactory,
RulesCache rules,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) {
TestSubmitType(ChangeData.Factory changeDataFactory, RulesCache rules, PrologRule prologRule) {
this.changeDataFactory = changeDataFactory;
this.rules = rules;
this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
this.prologRule = prologRule;
}
@Override
@@ -56,7 +55,10 @@ public class TestSubmitType implements RestModifyView<RevisionResource, TestSubm
if (input == null) {
input = new TestSubmitRuleInput();
}
if (input.rule != null && !rules.isProjectRulesEnabled()) {
if (input.rule == null) {
throw new BadRequestException("rule is required");
}
if (!rules.isProjectRulesEnabled()) {
throw new AuthException("project rules are disabled");
}
input.filters = MoreObjects.firstNonNull(input.filters, filters);
@@ -68,9 +70,8 @@ public class TestSubmitType implements RestModifyView<RevisionResource, TestSubm
.rule(input.rule)
.build();
SubmitRuleEvaluator evaluator = submitRuleEvaluatorFactory.create(opts);
ChangeData cd = changeDataFactory.create(rsrc.getNotes());
SubmitTypeRecord rec = evaluator.getSubmitType(cd);
SubmitTypeRecord rec = prologRule.getSubmitType(cd, opts);
if (rec.status != SubmitTypeRecord.Status.OK) {
throw new BadRequestException(String.format("rule produced invalid result: %s", rec));
@@ -80,17 +81,30 @@ public class TestSubmitType implements RestModifyView<RevisionResource, TestSubm
}
public static class Get implements RestReadView<RevisionResource> {
private final TestSubmitType test;
private final ChangeData.Factory changeDataFactory;
private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
@Inject
Get(TestSubmitType test) {
this.test = test;
Get(
ChangeData.Factory changeDataFactory,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) {
this.changeDataFactory = changeDataFactory;
this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
}
@Override
public Response<SubmitType> apply(RevisionResource resource)
throws AuthException, BadRequestException {
return test.apply(resource, null);
throws AuthException, ResourceConflictException {
SubmitRuleEvaluator evaluator =
submitRuleEvaluatorFactory.create(SubmitRuleOptions.defaults());
ChangeData cd = changeDataFactory.create(resource.getNotes());
SubmitTypeRecord rec = evaluator.getSubmitType(cd);
if (rec.status != SubmitTypeRecord.Status.OK) {
throw new ResourceConflictException(String.format("rule produced invalid result: %s", rec));
}
return Response.ok(rec.type);
}
}
}