Surface the submit/override expressions even if change is NOT_APPLICABLE
In change Iede127dc7, we made it such that submittablity and override
expressions of submit requirements are not evaluated if the SR does not
apply for the change. On the API, we didn't ship results for these.
Front-end will still need to get the actual expression strings
associated with submittability/override expressions in order to hide
labels (mentioned in submittability/override expressions) that are
associated with non applicable SRs, hence in this change we ship a
SubmitRequirementExpressionInfo but mark the status as 'NOT_EVALUATED'.
We are keeping the boolean field
`SubmitRequirementExpressionInfo#fulfilled` for compatibility with
clients (e.g. Gerrit's Frontend) who use it.
Google-Bug-Id: b/214020165
Release-Notes: Added the value "NOT_EVALUATED" to the SubmitRequirementExpressionInfo in the Change API and in NoteDb.
Change-Id: Idfe76d3edf63ebcd5c3c1161af7b2e7e75e90463
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index e739cfe..f754846 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -8242,6 +8242,13 @@
`branch:refs/heads/foo and label:verified=+1`.
|`fulfilled`||
True if the submit requirement is fulfilled for the change.
+|`status`||
+A string containing the status of evaluating the expression which can be one
+of the following: +
+ * `PASS` - expression was evaluated and result is true. +
+ * `FAIL` - expression was evaluated and result is false. +
+ * `ERROR` - an error occurred while evaluating the expression. +
+ * `NOT_EVALUATED` - expression was not evaluated.
|`passing_atoms`|optional|
A list of passing atoms as strings. For the above expression,
`passing_atoms` can contain ["branch:refs/heads/foo"] if the branch predicate is
@@ -8306,16 +8313,17 @@
submit requirement did not define an applicability expression.
Note that fields `expression`, `passing_atoms` and `failing_atoms` are always
omitted for the `applicability_expression_result`.
-|`submittability_expression_result`|optional|
+|`submittability_expression_result`||
A link:#submit-requirement-expression-info[SubmitRequirementExpressionInfo]
containing the result of evaluating the submittability expression. +
-If the submit requirement does not apply, the expression is not evaluated and
-the field is not set.
+If the submit requirement does not apply, the `status` field of the result
+will be set to `NOT_EVALUATED`.
|`override_expression_result`|optional|
A link:#submit-requirement-expression-info[SubmitRequirementExpressionInfo]
containing the result of evaluating the override expression. +
-Not set if the submit requirement did not define an override expression or
-if it does not apply.
+Not set if the submit requirement did not define an override expression. If the
+submit requirement does not apply, the `status` field of the result will be set
+to `NOT_EVALUATED`.
|===========================
[[submitted-together-info]]
diff --git a/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java b/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
index fcf32eb..c24227d 100644
--- a/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
+++ b/java/com/google/gerrit/entities/SubmitRequirementExpressionResult.java
@@ -95,6 +95,11 @@
ImmutableList.of());
}
+ public static SubmitRequirementExpressionResult notEvaluated(SubmitRequirementExpression expr) {
+ return SubmitRequirementExpressionResult.create(
+ expr, Status.NOT_EVALUATED, ImmutableList.of(), ImmutableList.of());
+ }
+
public static TypeAdapter<SubmitRequirementExpressionResult> typeAdapter(Gson gson) {
return new AutoValue_SubmitRequirementExpressionResult.GsonTypeAdapter(gson);
}
@@ -124,7 +129,10 @@
FAIL,
/** Submit requirement expression contains invalid syntax and is not parsable. */
- ERROR
+ ERROR,
+
+ /** Submit requirement expression was not evaluated. */
+ NOT_EVALUATED
}
/**
diff --git a/java/com/google/gerrit/extensions/common/SubmitRequirementExpressionInfo.java b/java/com/google/gerrit/extensions/common/SubmitRequirementExpressionInfo.java
index e9549c9..038b6f8 100644
--- a/java/com/google/gerrit/extensions/common/SubmitRequirementExpressionInfo.java
+++ b/java/com/google/gerrit/extensions/common/SubmitRequirementExpressionInfo.java
@@ -16,7 +16,10 @@
import java.util.List;
-/** Result of evaluating a single submit requirement expression. */
+/**
+ * Result of evaluating a single submit requirement expression. This API entity is populated from
+ * {@link com.google.gerrit.entities.SubmitRequirementExpressionResult}.
+ */
public class SubmitRequirementExpressionInfo {
/** Submit requirement expression as a String. */
@@ -25,6 +28,9 @@
/** A boolean indicating if the expression is fulfilled on a change. */
public boolean fulfilled;
+ /** A status indicating if the expression is fulfilled, non-fulfilled or not evaluated. */
+ public Status status;
+
/**
* A list of all atoms that are passing, for example query "branch:refs/heads/foo and project:bar"
* has two atoms: ["branch:refs/heads/foo", "project:bar"].
@@ -42,4 +48,22 @@
* during its evaluation.
*/
public String errorMessage;
+
+ /**
+ * Values in this enum should match with values in {@link
+ * com.google.gerrit.entities.SubmitRequirementExpressionResult.Status}.
+ */
+ public enum Status {
+ /** Expression was evaluated and the result was true. */
+ PASS,
+
+ /** Expression was evaluated and the result was false. */
+ FAIL,
+
+ /** An error occurred while evaluating the expression. */
+ ERROR,
+
+ /** Expression was not evaluated. */
+ NOT_EVALUATED
+ }
}
diff --git a/java/com/google/gerrit/server/change/SubmitRequirementsJson.java b/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
index 96c863e..fcd9e90 100644
--- a/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
+++ b/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
@@ -65,7 +65,10 @@
boolean hide) {
SubmitRequirementExpressionInfo info = new SubmitRequirementExpressionInfo();
info.expression = hide ? null : expression.expressionString();
- info.fulfilled = result.status().equals(SubmitRequirementExpressionResult.Status.PASS);
+ info.fulfilled =
+ result.status().equals(SubmitRequirementExpressionResult.Status.PASS)
+ || result.status().equals(SubmitRequirementExpressionResult.Status.NOT_EVALUATED);
+ info.status = SubmitRequirementExpressionInfo.Status.valueOf(result.status().name());
info.passingAtoms = hide ? null : result.passingAtoms();
info.failingAtoms = hide ? null : result.failingAtoms();
info.errorMessage = result.errorMessage().isPresent() ? result.errorMessage().get() : null;
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index c57fe27..3628d43 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
@@ -105,8 +105,14 @@
sr.applicabilityExpression().isPresent()
? Optional.of(evaluateExpression(sr.applicabilityExpression().get(), cd))
: Optional.empty();
- Optional<SubmitRequirementExpressionResult> submittabilityResult = Optional.empty();
- Optional<SubmitRequirementExpressionResult> overrideResult = Optional.empty();
+ Optional<SubmitRequirementExpressionResult> submittabilityResult =
+ Optional.of(
+ SubmitRequirementExpressionResult.notEvaluated(sr.submittabilityExpression()));
+ Optional<SubmitRequirementExpressionResult> overrideResult =
+ sr.overrideExpression().isPresent()
+ ? Optional.of(
+ SubmitRequirementExpressionResult.notEvaluated(sr.overrideExpression().get()))
+ : Optional.empty();
if (!sr.applicabilityExpression().isPresent()
|| SubmitRequirementResult.assertPass(applicabilityResult)) {
submittabilityResult = Optional.of(evaluateExpression(sr.submittabilityExpression(), cd));
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
index 61e55ff..c8e44e7 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
@@ -1895,6 +1895,7 @@
/* expression= */ null,
/* passingAtoms= */ null,
/* failingAtoms= */ null,
+ /* status= */ SubmitRequirementExpressionInfo.Status.PASS,
/* fulfilled= */ true);
assertThat(requirement.submittabilityExpressionResult).isNotNull();
}
@@ -1928,9 +1929,16 @@
/* expression= */ null,
/* passingAtoms= */ null,
/* failingAtoms= */ null,
+ /* status= */ SubmitRequirementExpressionInfo.Status.FAIL,
/* fulfilled= */ false);
- assertThat(requirement.submittabilityExpressionResult).isNull();
- assertThat(requirement.overrideExpressionResult).isNull();
+ assertThat(requirement.submittabilityExpressionResult.status)
+ .isEqualTo(SubmitRequirementExpressionInfo.Status.NOT_EVALUATED);
+ assertThat(requirement.submittabilityExpressionResult.expression)
+ .isEqualTo(SubmitRequirementExpression.maxCodeReview().expressionString());
+ assertThat(requirement.overrideExpressionResult.status)
+ .isEqualTo(SubmitRequirementExpressionInfo.Status.NOT_EVALUATED);
+ assertThat(requirement.overrideExpressionResult.expression)
+ .isEqualTo("project:" + project.get());
}
@Test
@@ -1963,12 +1971,14 @@
/* passingAtoms= */ ImmutableList.of(
SubmitRequirementExpression.maxCodeReview().expressionString()),
/* failingAtoms= */ ImmutableList.of(),
+ /* status= */ SubmitRequirementExpressionInfo.Status.PASS,
/* fulfilled= */ true);
assertSubmitRequirementExpression(
requirement.overrideExpressionResult,
/* expression= */ "project:non-existent",
/* passingAtoms= */ ImmutableList.of(),
/* failingAtoms= */ ImmutableList.of("project:non-existent"),
+ /* status= */ SubmitRequirementExpressionInfo.Status.FAIL,
/* fulfilled= */ false);
}
@@ -2003,12 +2013,14 @@
/* passingAtoms= */ ImmutableList.of(),
/* failingAtoms= */ ImmutableList.of(
SubmitRequirementExpression.maxCodeReview().expressionString()),
+ /* status= */ SubmitRequirementExpressionInfo.Status.FAIL,
/* fulfilled= */ false);
assertSubmitRequirementExpression(
requirement.overrideExpressionResult,
/* expression= */ "project:" + project.get(),
/* passingAtoms= */ ImmutableList.of("project:" + project.get()),
/* failingAtoms= */ ImmutableList.of(),
+ /* status= */ SubmitRequirementExpressionInfo.Status.PASS,
/* fulfilled= */ true);
}
@@ -2810,6 +2822,7 @@
@Nullable String expression,
@Nullable List<String> passingAtoms,
@Nullable List<String> failingAtoms,
+ SubmitRequirementExpressionInfo.Status status,
boolean fulfilled) {
assertThat(result.expression).isEqualTo(expression);
if (passingAtoms == null) {
@@ -2822,6 +2835,7 @@
} else {
assertThat(result.failingAtoms).containsExactlyElementsIn(failingAtoms);
}
+ assertThat(result.status).isEqualTo(status);
assertThat(result.fulfilled).isEqualTo(fulfilled);
}
diff --git a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
index 731e0df..8541fc0 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
@@ -283,7 +283,10 @@
SubmitRequirementResult result = evaluator.evaluateRequirement(sr, changeData);
assertThat(result.status()).isEqualTo(SubmitRequirementResult.Status.NOT_APPLICABLE);
assertThat(result.applicabilityExpressionResult().get().status()).isEqualTo(Status.FAIL);
- assertThat(result.submittabilityExpressionResult().isPresent()).isFalse();
+ assertThat(result.submittabilityExpressionResult().get().status())
+ .isEqualTo(Status.NOT_EVALUATED);
+ assertThat(result.submittabilityExpressionResult().get().expression().expressionString())
+ .isEqualTo("message:\"Fix bug\"");
assertThat(result.overrideExpressionResult().isPresent()).isFalse();
}
