Gracefully fail when submit_rule/1 has no solutions
If a submit_rule is coded with no solution for a change, e.g.:
submit_rule(submit()) :- fail.
the server used to crash with 500s from a number of REST APIs,
caused by a ClassCastException inside of SubmitRuleEvaluator.
An empty result list is not a ListTerm, it is a SymbolTerm and using
the NIL instance '[]'. This cannot be cast to a ListTerm for return
to the caller.
Instead return List<Term> and handle the conversion, ensuring an
empty list is returned to callers when there are no results.
Change-Id: Iea1211afb2f3970ba98ea1eff14df07452cf5ccd
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java
index 170f8e8..db18f0d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java
@@ -129,10 +129,9 @@
return out;
}
- @SuppressWarnings("unchecked")
private static List<Term> eval(SubmitRuleEvaluator evaluator)
throws RuleEvalException {
- return evaluator.evaluate().toJava();
+ return evaluator.evaluate();
}
static class Record {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java
index f58fe75..e7e1f32 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java
@@ -30,12 +30,12 @@
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
-import com.googlecode.prolog_cafe.lang.ListTerm;
import com.googlecode.prolog_cafe.lang.Term;
import org.kohsuke.args4j.Option;
import java.io.ByteArrayInputStream;
+import java.util.List;
public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
private final ReviewDb db;
@@ -76,7 +76,7 @@
? new ByteArrayInputStream(input.rule.getBytes(Charsets.UTF_8))
: null);
- ListTerm results;
+ List<Term> results;
try {
results = evaluator.evaluate();
} catch (RuleEvalException e) {
@@ -84,12 +84,12 @@
"rule failed with exception: %s",
e.getMessage()));
}
- if (results.isNil()) {
+ if (results.isEmpty()) {
throw new BadRequestException(String.format(
"rule %s has no solution",
evaluator.getSubmitRule()));
}
- Term type = results.car();
+ Term type = results.get(0);
if (!type.isSymbol()) {
throw new BadRequestException(String.format(
"rule %s produced invalid result: %s",
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 48b0731..74b6232 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -36,6 +36,7 @@
import com.googlecode.prolog_cafe.lang.ListTerm;
import com.googlecode.prolog_cafe.lang.Prolog;
import com.googlecode.prolog_cafe.lang.StructureTerm;
+import com.googlecode.prolog_cafe.lang.SymbolTerm;
import com.googlecode.prolog_cafe.lang.Term;
import org.slf4j.Logger;
@@ -355,7 +356,7 @@
fastEvalLabels,
"locate_submit_rule", "can_submit",
"locate_submit_filter", "filter_submit_results");
- results = evaluator.evaluate().toJava();
+ results = evaluator.evaluate();
} catch (RuleEvalException e) {
return logRuleError(e.getMessage(), e);
}
@@ -492,7 +493,7 @@
err);
}
- List<String> results;
+ List<Term> results;
SubmitRuleEvaluator evaluator;
try {
evaluator = new SubmitRuleEvaluator(db, patchSet,
@@ -500,7 +501,7 @@
false,
"locate_submit_type", "get_submit_type",
"locate_submit_type_filter", "filter_submit_type_results");
- results = evaluator.evaluate().toJava();
+ results = evaluator.evaluate();
} catch (RuleEvalException e) {
return logTypeRuleError(e.getMessage(), e);
}
@@ -513,10 +514,15 @@
return typeRuleError("Project submit rule has no solution");
}
- // Take only the first result and convert it to SubmitTypeRecord
- // This logic will need to change once we support multiple submit types
- // in the UI
- String typeName = results.get(0);
+ Term typeTerm = results.get(0);
+ if (!typeTerm.isSymbol()) {
+ log.error("Submit rule '" + evaluator.getSubmitRule() + "' for change "
+ + change.getId() + " of " + getProject().getName()
+ + " did not return a symbol.");
+ return typeRuleError("Project submit rule has invalid solution");
+ }
+
+ String typeName = ((SymbolTerm)typeTerm).name();
try {
return SubmitTypeRecord.OK(
Project.SubmitType.valueOf(typeName.toUpperCase()));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
index bcea2c9..349567c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.project;
+import com.google.common.collect.Lists;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -30,6 +31,7 @@
import java.io.InputStream;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import javax.annotation.Nullable;
@@ -124,8 +126,7 @@
* @return List of {@link Term} objects returned from the evaluated rules.
* @throws RuleEvalException
*/
- public ListTerm evaluate() throws RuleEvalException {
- List<Term> results = new ArrayList<Term>();
+ public List<Term> evaluate() throws RuleEvalException {
PrologEnvironment env = getPrologEnvironment();
try {
submitRule = env.once("gerrit", userRuleLocatorName, new VariableTerm());
@@ -133,6 +134,7 @@
env.once("gerrit", "assume_range_from_label");
}
+ List<Term> results = new ArrayList<Term>();
try {
for (Term[] template : env.all("gerrit", userRuleWrapperName,
submitRule, new VariableTerm())) {
@@ -152,7 +154,16 @@
if (!skipFilters) {
resultsTerm = runSubmitFilters(resultsTerm, env);
}
- return (ListTerm) resultsTerm;
+ if (resultsTerm.isList()) {
+ List<Term> r = Lists.newArrayList();
+ for (Term t = resultsTerm; t.isList();) {
+ ListTerm l = (ListTerm) t;
+ r.add(l.car().dereference());
+ t = l.cdr().dereference();
+ }
+ return r;
+ }
+ return Collections.emptyList();
} finally {
env.close();
}
