Add support for annotations without explicit value

cedar-policy-core/src/ast/policy.rs

@@ -20,7 +20,7 @@ use itertools::Itertools;
 use miette::Diagnostic;
 use nonempty::{nonempty, NonEmpty};
 use serde::{Deserialize, Serialize};
-use smol_str::SmolStr;
+use smol_str::{SmolStr, ToSmolStr};
 use std::collections::BTreeMap;
 use std::{collections::HashMap, sync::Arc};
 use thiserror::Error;
@@ -1170,6 +1170,19 @@ pub struct Annotation {
     pub loc: Option<Loc>,
 }
 
+impl Annotation {
+    /// Construct an Annotation with an optional value.  This function is used
+    /// to construct annotations from the CST and EST representation where a
+    /// value is not required, but an absent value is equivalent to `""`.
+    /// Here, a `None` constructs an annotation containing the value `""`.`
+    pub fn with_optional_value(val: Option<SmolStr>, loc: Option<Loc>) -> Self {
+        Self {
+            val: val.unwrap_or_else(|| "".to_smolstr()),
+            loc,
+        }
+    }
+}
+
 impl AsRef<str> for Annotation {
     fn as_ref(&self) -> &str {
         &self.val

cedar-policy-core/src/est.rs

@@ -67,7 +67,7 @@ pub struct Policy {
     #[serde(skip_serializing_if = "BTreeMap::is_empty")]
     #[serde_as(as = "serde_with::MapPreventDuplicates<_,_>")]
     #[cfg_attr(feature = "wasm", tsify(type = "Record<string, string>"))]
-    annotations: BTreeMap<ast::AnyId, SmolStr>,
+    annotations: BTreeMap<ast::AnyId, Option<SmolStr>>,
 }
 
 /// Serde JSON structure for a `when` or `unless` clause in the EST format
@@ -151,7 +151,7 @@ impl TryFrom<cst::Policy> for Policy {
     fn try_from(policy: cst::Policy) -> Result<Policy, ParseErrors> {
         let maybe_effect = policy.effect.to_effect();
         let maybe_scope = policy.extract_scope();
-        let maybe_annotations = policy.get_ast_annotations();
+        let maybe_annotations = policy.get_ast_annotations(|v, _| v);
         let maybe_conditions = ParseErrors::transpose(policy.conds.into_iter().map(|node| {
             let (cond, loc) = node.into_inner();
             let cond = cond.ok_or_else(|| {
@@ -172,7 +172,7 @@ impl TryFrom<cst::Policy> for Policy {
             action: action.into(),
             resource: resource.into(),
             conditions,
-            annotations: annotations.into_iter().map(|(k, v)| (k, v.val)).collect(),
+            annotations,
         })
     }
 }
@@ -262,7 +262,7 @@ impl Policy {
             None,
             self.annotations
                 .into_iter()
-                .map(|(key, val)| (key, ast::Annotation { val, loc: None }))
+                .map(|(key, val)| (key, ast::Annotation::with_optional_value(val, None)))
                 .collect(),
             self.effect,
             self.principal.try_into()?,
@@ -308,7 +308,10 @@ impl From<ast::Policy> for Policy {
             conditions: vec![ast.non_scope_constraints().clone().into()],
             annotations: ast
                 .annotations()
-                .map(|(k, v)| (k.clone(), v.val.clone()))
+                // When converting from AST to EST, we will always interpret an
+                // empty-string annotation as an explicit `""` rather than
+                // `null` (which is implicitly equivalent to `""`).
+                .map(|(k, v)| (k.clone(), Some(v.val.clone())))
                 .collect(),
         }
     }
@@ -325,7 +328,10 @@ impl From<ast::Template> for Policy {
             conditions: vec![ast.non_scope_constraints().clone().into()],
             annotations: ast
                 .annotations()
-                .map(|(k, v)| (k.clone(), v.val.clone()))
+                // When converting from AST to EST, we will always interpret an
+                // empty-string annotation as an explicit `""` rather than
+                // `null` (which is implicitly equivalent to `""`)
+                .map(|(k, v)| (k.clone(), Some(v.val.clone())))
                 .collect(),
         }
     }
@@ -340,7 +346,11 @@ impl<T: Clone> From<ast::Expr<T>> for Clause {
 impl std::fmt::Display for Policy {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         for (k, v) in self.annotations.iter() {
-            writeln!(f, "@{k}(\"{}\") ", v.escape_debug())?;
+            write!(f, "@{k}")?;
+            if let Some(v) = v {
+                write!(f, "(\"{}\")", v.escape_debug())?;
+            }
+            writeln!(f)?;
         }
         write!(
             f,
@@ -618,6 +628,85 @@ mod test {
         );
     }
 
+    #[test]
+    fn annotated_without_value_policy() {
+        let policy = r#"@foo permit(principal, action, resource);"#;
+        let cst = parser::text_to_cst::parse_policy(policy)
+            .unwrap()
+            .node
+            .unwrap();
+        let est: Policy = cst.try_into().unwrap();
+        let expected_json = json!(
+            {
+                "effect": "permit",
+                "principal": {
+                    "op": "All",
+                },
+                "action": {
+                    "op": "All",
+                },
+                "resource": {
+                    "op": "All",
+                },
+                "conditions": [],
+                "annotations": { "foo": null, }
+            }
+        );
+        assert_eq!(
+            serde_json::to_value(&est).unwrap(),
+            expected_json,
+            "\nExpected:\n{}\n\nActual:\n{}\n\n",
+            serde_json::to_string_pretty(&expected_json).unwrap(),
+            serde_json::to_string_pretty(&est).unwrap()
+        );
+        let old_est = est.clone();
+        let roundtripped = est_roundtrip(est);
+        assert_eq!(&old_est, &roundtripped);
+        let est = text_roundtrip(&old_est);
+        assert_eq!(&old_est, &est);
+
+        // during the lossy transform to AST, the `null` annotation becomes an empty string
+        let expected_json_after_roundtrip = json!(
+            {
+                "effect": "permit",
+                "principal": {
+                    "op": "All",
+                },
+                "action": {
+                    "op": "All",
+                },
+                "resource": {
+                    "op": "All",
+                },
+                "conditions": [
+                    {
+                        "kind": "when",
+                        "body": {
+                            "Value": true
+                        }
+                    }
+                ],
+                "annotations": { "foo": "", }
+            }
+        );
+        let roundtripped = serde_json::to_value(ast_roundtrip(est.clone())).unwrap();
+        assert_eq!(
+            roundtripped,
+            expected_json_after_roundtrip,
+            "\nExpected after roundtrip:\n{}\n\nActual after roundtrip:\n{}\n\n",
+            serde_json::to_string_pretty(&expected_json_after_roundtrip).unwrap(),
+            serde_json::to_string_pretty(&roundtripped).unwrap()
+        );
+        let roundtripped = serde_json::to_value(circular_roundtrip(est)).unwrap();
+        assert_eq!(
+            roundtripped,
+            expected_json_after_roundtrip,
+            "\nExpected after roundtrip:\n{}\n\nActual after roundtrip:\n{}\n\n",
+            serde_json::to_string_pretty(&expected_json_after_roundtrip).unwrap(),
+            serde_json::to_string_pretty(&roundtripped).unwrap()
+        );
+    }
+
     /// Test that we can use Cedar reserved words like `if` and `has` as annotation keys
     #[test]
     fn reserved_words_as_annotations() {

cedar-policy-core/src/parser/cst.rs

@@ -31,7 +31,7 @@ pub struct Annotation {
     /// key
     pub key: Node<Ident>,
     /// value
-    pub value: Node<Str>,
+    pub value: Option<Node<Str>>,
 }
 
 /// Literal strings