From 23050e196c2258c7e2c6296a086c182f3a2e59d4 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 19 Apr 2013 10:25:39 -0700 Subject: [PATCH] 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 and handle the conversion, ensuring an empty list is returned to callers when there are no results. Change-Id: Iea1211afb2f3970ba98ea1eff14df07452cf5ccd --- .../gerrit/server/change/TestSubmitRule.java | 3 +-- .../gerrit/server/change/TestSubmitType.java | 8 ++++---- .../gerrit/server/project/ChangeControl.java | 20 ++++++++++++------- .../server/project/SubmitRuleEvaluator.java | 17 +++++++++++++--- 4 files changed, 32 insertions(+), 16 deletions(-) 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 170f8e8488..db18f0d131 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 @@ public class TestSubmitRule implements RestModifyView { return out; } - @SuppressWarnings("unchecked") private static List 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 f58fe75149..e7e1f3201c 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.gerrit.server.query.change.ChangeData; 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 { private final ReviewDb db; @@ -76,7 +76,7 @@ public class TestSubmitType implements RestModifyView { ? new ByteArrayInputStream(input.rule.getBytes(Charsets.UTF_8)) : null); - ListTerm results; + List results; try { results = evaluator.evaluate(); } catch (RuleEvalException e) { @@ -84,12 +84,12 @@ public class TestSubmitType implements RestModifyView { "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 48b0731e0d..74b6232e64 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.IntegerTerm; 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 @@ public class ChangeControl { 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 @@ public class ChangeControl { err); } - List results; + List results; SubmitRuleEvaluator evaluator; try { evaluator = new SubmitRuleEvaluator(db, patchSet, @@ -500,7 +501,7 @@ public class ChangeControl { 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 @@ public class ChangeControl { 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 bcea2c9836..349567cbd8 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 com.googlecode.prolog_cafe.lang.VariableTerm; import java.io.InputStream; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import javax.annotation.Nullable; @@ -124,8 +126,7 @@ public class SubmitRuleEvaluator { * @return List of {@link Term} objects returned from the evaluated rules. * @throws RuleEvalException */ - public ListTerm evaluate() throws RuleEvalException { - List results = new ArrayList(); + public List evaluate() throws RuleEvalException { PrologEnvironment env = getPrologEnvironment(); try { submitRule = env.once("gerrit", userRuleLocatorName, new VariableTerm()); @@ -133,6 +134,7 @@ public class SubmitRuleEvaluator { env.once("gerrit", "assume_range_from_label"); } + List results = new ArrayList(); try { for (Term[] template : env.all("gerrit", userRuleWrapperName, submitRule, new VariableTerm())) { @@ -152,7 +154,16 @@ public class SubmitRuleEvaluator { if (!skipFilters) { resultsTerm = runSubmitFilters(resultsTerm, env); } - return (ListTerm) resultsTerm; + if (resultsTerm.isList()) { + List 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(); }