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
This commit is contained in:
Shawn Pearce 2013-04-19 10:25:39 -07:00
parent f3388c0ec0
commit 23050e196c
4 changed files with 32 additions and 16 deletions

View File

@ -129,10 +129,9 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, Input> {
return out; return out;
} }
@SuppressWarnings("unchecked")
private static List<Term> eval(SubmitRuleEvaluator evaluator) private static List<Term> eval(SubmitRuleEvaluator evaluator)
throws RuleEvalException { throws RuleEvalException {
return evaluator.evaluate().toJava(); return evaluator.evaluate();
} }
static class Record { static class Record {

View File

@ -30,12 +30,12 @@ import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.googlecode.prolog_cafe.lang.ListTerm;
import com.googlecode.prolog_cafe.lang.Term; import com.googlecode.prolog_cafe.lang.Term;
import org.kohsuke.args4j.Option; import org.kohsuke.args4j.Option;
import java.io.ByteArrayInputStream; import java.io.ByteArrayInputStream;
import java.util.List;
public class TestSubmitType implements RestModifyView<RevisionResource, Input> { public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
private final ReviewDb db; private final ReviewDb db;
@ -76,7 +76,7 @@ public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
? new ByteArrayInputStream(input.rule.getBytes(Charsets.UTF_8)) ? new ByteArrayInputStream(input.rule.getBytes(Charsets.UTF_8))
: null); : null);
ListTerm results; List<Term> results;
try { try {
results = evaluator.evaluate(); results = evaluator.evaluate();
} catch (RuleEvalException e) { } catch (RuleEvalException e) {
@ -84,12 +84,12 @@ public class TestSubmitType implements RestModifyView<RevisionResource, Input> {
"rule failed with exception: %s", "rule failed with exception: %s",
e.getMessage())); e.getMessage()));
} }
if (results.isNil()) { if (results.isEmpty()) {
throw new BadRequestException(String.format( throw new BadRequestException(String.format(
"rule %s has no solution", "rule %s has no solution",
evaluator.getSubmitRule())); evaluator.getSubmitRule()));
} }
Term type = results.car(); Term type = results.get(0);
if (!type.isSymbol()) { if (!type.isSymbol()) {
throw new BadRequestException(String.format( throw new BadRequestException(String.format(
"rule %s produced invalid result: %s", "rule %s produced invalid result: %s",

View File

@ -36,6 +36,7 @@ import com.googlecode.prolog_cafe.lang.IntegerTerm;
import com.googlecode.prolog_cafe.lang.ListTerm; import com.googlecode.prolog_cafe.lang.ListTerm;
import com.googlecode.prolog_cafe.lang.Prolog; import com.googlecode.prolog_cafe.lang.Prolog;
import com.googlecode.prolog_cafe.lang.StructureTerm; import com.googlecode.prolog_cafe.lang.StructureTerm;
import com.googlecode.prolog_cafe.lang.SymbolTerm;
import com.googlecode.prolog_cafe.lang.Term; import com.googlecode.prolog_cafe.lang.Term;
import org.slf4j.Logger; import org.slf4j.Logger;
@ -355,7 +356,7 @@ public class ChangeControl {
fastEvalLabels, fastEvalLabels,
"locate_submit_rule", "can_submit", "locate_submit_rule", "can_submit",
"locate_submit_filter", "filter_submit_results"); "locate_submit_filter", "filter_submit_results");
results = evaluator.evaluate().toJava(); results = evaluator.evaluate();
} catch (RuleEvalException e) { } catch (RuleEvalException e) {
return logRuleError(e.getMessage(), e); return logRuleError(e.getMessage(), e);
} }
@ -492,7 +493,7 @@ public class ChangeControl {
err); err);
} }
List<String> results; List<Term> results;
SubmitRuleEvaluator evaluator; SubmitRuleEvaluator evaluator;
try { try {
evaluator = new SubmitRuleEvaluator(db, patchSet, evaluator = new SubmitRuleEvaluator(db, patchSet,
@ -500,7 +501,7 @@ public class ChangeControl {
false, false,
"locate_submit_type", "get_submit_type", "locate_submit_type", "get_submit_type",
"locate_submit_type_filter", "filter_submit_type_results"); "locate_submit_type_filter", "filter_submit_type_results");
results = evaluator.evaluate().toJava(); results = evaluator.evaluate();
} catch (RuleEvalException e) { } catch (RuleEvalException e) {
return logTypeRuleError(e.getMessage(), e); return logTypeRuleError(e.getMessage(), e);
} }
@ -513,10 +514,15 @@ public class ChangeControl {
return typeRuleError("Project submit rule has no solution"); return typeRuleError("Project submit rule has no solution");
} }
// Take only the first result and convert it to SubmitTypeRecord Term typeTerm = results.get(0);
// This logic will need to change once we support multiple submit types if (!typeTerm.isSymbol()) {
// in the UI log.error("Submit rule '" + evaluator.getSubmitRule() + "' for change "
String typeName = results.get(0); + change.getId() + " of " + getProject().getName()
+ " did not return a symbol.");
return typeRuleError("Project submit rule has invalid solution");
}
String typeName = ((SymbolTerm)typeTerm).name();
try { try {
return SubmitTypeRecord.OK( return SubmitTypeRecord.OK(
Project.SubmitType.valueOf(typeName.toUpperCase())); Project.SubmitType.valueOf(typeName.toUpperCase()));

View File

@ -14,6 +14,7 @@
package com.google.gerrit.server.project; 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.Change;
import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
@ -30,6 +31,7 @@ import com.googlecode.prolog_cafe.lang.VariableTerm;
import java.io.InputStream; import java.io.InputStream;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collections;
import java.util.List; import java.util.List;
import javax.annotation.Nullable; import javax.annotation.Nullable;
@ -124,8 +126,7 @@ public class SubmitRuleEvaluator {
* @return List of {@link Term} objects returned from the evaluated rules. * @return List of {@link Term} objects returned from the evaluated rules.
* @throws RuleEvalException * @throws RuleEvalException
*/ */
public ListTerm evaluate() throws RuleEvalException { public List<Term> evaluate() throws RuleEvalException {
List<Term> results = new ArrayList<Term>();
PrologEnvironment env = getPrologEnvironment(); PrologEnvironment env = getPrologEnvironment();
try { try {
submitRule = env.once("gerrit", userRuleLocatorName, new VariableTerm()); submitRule = env.once("gerrit", userRuleLocatorName, new VariableTerm());
@ -133,6 +134,7 @@ public class SubmitRuleEvaluator {
env.once("gerrit", "assume_range_from_label"); env.once("gerrit", "assume_range_from_label");
} }
List<Term> results = new ArrayList<Term>();
try { try {
for (Term[] template : env.all("gerrit", userRuleWrapperName, for (Term[] template : env.all("gerrit", userRuleWrapperName,
submitRule, new VariableTerm())) { submitRule, new VariableTerm())) {
@ -152,7 +154,16 @@ public class SubmitRuleEvaluator {
if (!skipFilters) { if (!skipFilters) {
resultsTerm = runSubmitFilters(resultsTerm, env); 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 { } finally {
env.close(); env.close();
} }