Parse explanations in match blocks

PiperOrigin-RevId: 662598633
google · Aug 14, 2024 · 1a59a9a · 1a59a9a
1 parent 595eccd
commit 1a59a9a
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 1 deletion.
diff --git a/policy/src/main/java/dev/cel/policy/CelPolicy.java b/policy/src/main/java/dev/cel/policy/CelPolicy.java
@@ -159,6 +159,9 @@ public abstract static class Match {
 
     public abstract Result result();
 
+    /** Explanation returns the explanation expression, or empty expression if output is not set. */
+    public abstract Optional<ValueString> explanation();
+
     /** Encapsulates the result of this match when condition is met. (either an output or a rule) */
     @AutoOneOf(Match.Result.Kind.class)
     public abstract static class Result {
@@ -191,8 +194,12 @@ public abstract static class Builder implements RequiredFieldsChecker {
 
       public abstract Builder setResult(Result result);
 
+      public abstract Builder setExplanation(ValueString explanation);
+
       abstract Optional<Result> result();
 
+      abstract Optional<ValueString> explanation();
+
       @Override
       public ImmutableList<RequiredField> requiredFields() {
         return ImmutableList.of(RequiredField.of("output or a rule", this::result));

diff --git a/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java b/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java
@@ -202,12 +202,30 @@ public CelPolicy.Match parseMatch(
                     result -> ctx.reportError(tagId, "Only the rule or the output may be set"));
             matchBuilder.setResult(Match.Result.ofOutput(ctx.newValueString(value)));
             break;
+          case "explanation":
+            matchBuilder
+                .result()
+                .filter(result -> result.kind().equals(Match.Result.Kind.RULE))
+                .ifPresent(
+                    result ->
+                        ctx.reportError(
+                            tagId,
+                            "Explanation can only be set on output match cases, not nested rules"));
+            matchBuilder.setExplanation(ctx.newValueString(value));
+            break;
           case "rule":
             matchBuilder
                 .result()
                 .filter(result -> result.kind().equals(Match.Result.Kind.OUTPUT))
                 .ifPresent(
                     result -> ctx.reportError(tagId, "Only the rule or the output may be set"));
+            matchBuilder
+                .explanation()
+                .ifPresent(
+                    result ->
+                        ctx.reportError(
+                            result.id(),
+                            "Explanation can only be set on output match cases, not nested rules"));
             matchBuilder.setResult(Match.Result.ofRule(parseRule(ctx, policyBuilder, value)));
             break;
           default:

diff --git a/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java b/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java
@@ -17,6 +17,7 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
+import com.google.common.collect.Iterables;
 import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import dev.cel.policy.PolicyTestHelper.K8sTagHandler;
@@ -42,6 +43,21 @@ public void parseYamlPolicy_success(@TestParameter TestYamlPolicy yamlPolicy) th
     assertThat(policy.policySource().getDescription()).isEqualTo(description);
   }
 
+  @Test
+  public void parseYamlPolicy_withExplanation() throws Exception {
+    String policySource =
+        "rule:\n"
+            + "  match:\n"
+            + "  - output: 'true'\n"
+            + "    explanation: \"'custom explanation'\"";
+
+    CelPolicy policy = POLICY_PARSER.parse(policySource);
+
+    assertThat(policy.rule().matches()).hasSize(1);
+    assertThat(Iterables.getOnlyElement(policy.rule().matches()).explanation())
+        .hasValue(ValueString.of(11, "'custom explanation'"));
+  }
+
   @Test
   public void parseYamlPolicy_errors(@TestParameter PolicyParseErrorTestCase testCase) {
     CelPolicyValidationException e =
@@ -197,6 +213,28 @@ private enum PolicyParseErrorTestCase {
         "ERROR: <input>:7:7: Only the rule or the output may be set\n"
             + " |       output: \"world\"\n"
             + " | ......^"),
+    MATCH_NESTED_RULE_SET_THEN_EXPLANATION(
+        "rule:\n"
+            + "  match:\n"
+            + "    - condition: \"true\"\n"
+            + "      rule:\n"
+            + "        match:\n"
+            + "          - output: \"hello\"\n"
+            + "      explanation: \"foo\"",
+        "ERROR: <input>:7:7: Explanation can only be set on output match cases, not nested rules\n"
+            + " |       explanation: \"foo\"\n"
+            + " | ......^"),
+    MATCH_EXPLANATION_SET_THEN_NESTED_RULE(
+        "rule:\n"
+            + "  match:\n"
+            + "    - condition: \"true\"\n"
+            + "      explanation: \"foo\"\n"
+            + "      rule:\n"
+            + "        match:\n"
+            + "          - output: \"hello\"\n",
+        "ERROR: <input>:4:21: Explanation can only be set on output match cases, not nested rules\n"
+            + " |       explanation: \"foo\"\n"
+            + " | ....................^"),
     INVALID_ROOT_NODE_TYPE(
         "- rule:\n" + "    id: a",
         "ERROR: <input>:1:1: Got yaml node type tag:yaml.org,2002:seq, wanted type(s)"

diff --git a/policy/src/test/resources/nested_rule/policy.yaml b/policy/src/test/resources/nested_rule/policy.yaml
@@ -35,4 +35,4 @@ rule:
     - condition: resource.origin in variables.permitted_regions
       output: "{'banned': false}"
     - output: "{'banned': true}"
-
+      explanation: "'resource is in the banned region ' + resource.origin"