remove Display for EntityId in favor of explicit .escaped() and .as_ref() (#921)

Signed-off-by: Craig Disselkoen <[email protected]>

Author: cdisselkoen

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

@@ -196,7 +196,7 @@ impl EntityUID {
 
 impl std::fmt::Display for EntityUID {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}::\"{}\"", self.entity_type(), self.eid)
+        write!(f, "{}::\"{}\"", self.entity_type(), self.eid.escaped())
     }
 }
 
@@ -226,7 +226,15 @@ impl<'a> arbitrary::Arbitrary<'a> for EntityUID {
     }
 }
 
-/// EID type is just a SmolStr for now
+/// The `Eid` type represents the id of an `Entity`, without the typename.
+/// Together with the typename it comprises an `EntityUID`.
+/// For example, in `User::"alice"`, the `Eid` is `alice`.
+///
+/// `Eid` does not implement `Display`, partly because it is unclear whether
+/// `Display` should produce an escaped representation or an unescaped representation
+/// (see [#884](https://github.com/cedar-policy/cedar/issues/884)).
+/// To get an escaped representation, use `.escaped()`.
+/// To get an unescaped representation, use `.as_ref()`.
 #[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Hash, PartialOrd, Ord)]
 pub struct Eid(SmolStr);
 
@@ -235,6 +243,11 @@ impl Eid {
     pub fn new(eid: impl Into<SmolStr>) -> Self {
         Eid(eid.into())
     }
+
+    /// Get the contents of the `Eid` as an escaped string
+    pub fn escaped(&self) -> SmolStr {
+        self.0.escape_debug().collect()
+    }
 }
 
 impl AsRef<SmolStr> for Eid {
@@ -257,12 +270,6 @@ impl<'a> arbitrary::Arbitrary<'a> for Eid {
     }
 }
 
-impl std::fmt::Display for Eid {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}", self.0.escape_debug())
-    }
-}
-
 /// Entity datatype
 #[derive(Debug, Clone, Serialize)]
 pub struct Entity {

cedar-policy-core/src/entities.rs

@@ -294,7 +294,7 @@ impl Entities {
             ))?;
             for entity in entities {
                 let euid = to_dot_id(&entity.uid());
-                let label = format!(r#"[label={}]"#, to_dot_id(&entity.uid().eid()));
+                let label = format!(r#"[label={}]"#, to_dot_id(&entity.uid().eid().escaped()));
                 dot_str.write_str(&format!("\t\t{euid} {label}\n"))?;
             }
             dot_str.write_str("\t}\n")?;

cedar-policy-core/src/parser.rs

@@ -747,8 +747,7 @@ mod parse_tests {
     fn test_parse_string() {
         // test idempotence
         assert_eq!(
-            ast::Eid::new(parse_internal_string(r"a\nblock\nid").expect("should parse"))
-                .to_string(),
+            ast::Eid::new(parse_internal_string(r"a\nblock\nid").expect("should parse")).escaped(),
             r"a\nblock\nid",
         );
         parse_internal_string(r#"oh, no, a '! "#).expect("single quote should be fine");

cedar-policy-validator/src/diagnostics.rs

@@ -22,7 +22,7 @@ use thiserror::Error;
 
 use std::collections::BTreeSet;
 
-use cedar_policy_core::ast::{Expr, Name, PolicyID};
+use cedar_policy_core::ast::{Eid, Expr, Name, PolicyID};
 use cedar_policy_core::parser::Loc;
 
 use crate::types::Type;
@@ -209,7 +209,7 @@ impl ValidationError {
     pub(crate) fn unspecified_entity(
         source_loc: Option<Loc>,
         policy_id: PolicyID,
-        entity_id: String,
+        entity_id: Eid,
     ) -> Self {
         validation_errors::UnspecifiedEntity {
             source_loc,

cedar-policy-validator/src/diagnostics/validation_errors.rs

@@ -27,7 +27,7 @@ use cedar_policy_core::parser::Loc;
 use std::collections::BTreeSet;
 
 use cedar_policy_core::ast::{
-    CallStyle, EntityUID, Expr, ExprKind, ExprShapeOnly, Name, PolicyID, Var,
+    CallStyle, Eid, EntityUID, Expr, ExprKind, ExprShapeOnly, Name, PolicyID, Var,
 };
 use cedar_policy_core::parser::join_with_conjunction;
 
@@ -141,12 +141,12 @@ impl Diagnostic for InvalidActionApplication {
 
 /// Structure containing details about an unspecified entity error.
 #[derive(Debug, Clone, Error, Hash, Eq, PartialEq)]
-#[error("for policy `{policy_id}`, unspecified entity with id `{entity_id}`")]
+#[error("for policy `{policy_id}`, unspecified entity with id `{}`", .entity_id.escaped())]
 pub struct UnspecifiedEntity {
     pub source_loc: Option<Loc>,
     pub policy_id: PolicyID,
-    /// EID of the unspecified entity.
-    pub entity_id: String,
+    /// EID of the unspecified entity
+    pub entity_id: Eid,
 }
 
 impl Diagnostic for UnspecifiedEntity {

cedar-policy-validator/src/rbac.rs

@@ -74,7 +74,7 @@ impl Validator {
                         Some(ValidationError::unspecified_entity(
                             euid.loc().cloned(),
                             template.id().clone(),
-                            euid.eid().to_string(),
+                            euid.eid().clone(),
                         ))
                     }
                     cedar_policy_core::ast::EntityType::Specified(_) => None,
@@ -102,7 +102,7 @@ impl Validator {
                 ast::EntityType::Unspecified => Some(ValidationError::unspecified_entity(
                     euid.loc().cloned(),
                     template.id().clone(),
-                    euid.eid().to_string(),
+                    euid.eid().clone(),
                 )),
                 ast::EntityType::Specified(name) => {
                     let is_known_action_entity_id = self.schema.is_known_action_id(euid);
@@ -146,7 +146,7 @@ impl Validator {
                     Some(ValidationError::unspecified_entity(
                         None,
                         policy_id.clone(),
-                        euid.eid().to_string(),
+                        euid.eid().clone(),
                     ))
                 }
                 cedar_policy_core::ast::EntityType::Specified(name) => {
@@ -1572,7 +1572,8 @@ mod test {
         assert_eq!(1, notes.len());
         assert_matches!(notes.first(),
             Some(ValidationError::UnspecifiedEntity(UnspecifiedEntity { entity_id, .. })) => {
-                assert_eq!("foo", entity_id);
+                let eid: &str = entity_id.as_ref();
+                assert_eq!("foo", eid);
             }
         );
 
@@ -1591,7 +1592,8 @@ mod test {
         assert_eq!(1, notes.len());
         assert_matches!(notes.first(),
             Some(ValidationError::UnspecifiedEntity(UnspecifiedEntity { entity_id, .. })) => {
-                assert_eq!("foo", entity_id);
+                let eid: &str = entity_id.as_ref();
+                assert_eq!("foo", eid);
             }
         );
 
@@ -1622,7 +1624,8 @@ mod test {
         assert_eq!(1, notes.len());
         assert_matches!(notes.first(),
             Some(ValidationError::UnspecifiedEntity(UnspecifiedEntity { entity_id, .. })) => {
-                assert_eq!("foo", entity_id);
+                let eid: &str = entity_id.as_ref();
+                assert_eq!("foo", eid);
             }
         );
 

cedar-policy-validator/src/schema.rs

@@ -2105,7 +2105,8 @@ mod test {
             .unwrap();
         let ancestors = view_photo.ancestors().collect::<Vec<_>>();
         let read = ancestors[0];
-        assert_eq!(read.eid().to_string(), "read");
+        let read_eid: &str = read.eid().as_ref();
+        assert_eq!(read_eid, "read");
         assert_eq!(read.entity_type().to_string(), "Foo::Action");
     }
 
@@ -2165,7 +2166,8 @@ mod test {
             .unwrap();
         let ancestors = view_photo.ancestors().collect::<Vec<_>>();
         let read = ancestors[0];
-        assert_eq!(read.eid().to_string(), "read");
+        let read_eid: &str = read.eid().as_ref();
+        assert_eq!(read_eid, "read");
         assert_eq!(
             read.entity_type().to_string(),
             "ExampleCo::Personnel::Action"

cedar-policy/CHANGELOG.md

@@ -22,7 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Removed unnecessary lifetimes from some validation related structs (#715)
 - Changed policy validation to reject comparisons and conditionals between
   record types that differ in whether an attribute is required or optional.
-- Fixed a performance issue when constructing an error for accessing 
+- Fixed a performance issue when constructing an error for accessing
     a non-existent attribute on sufficiently large records/entities
 
 ### Removed
@@ -37,6 +37,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   the rich data provided by `miette::Diagnostic`, for instance `.help()` and
   `labels()`. Callers can continue using the same behavior by calling
   `.iter().map(ToString::to_string)`. (#882, resolving #543)
+- Removed `Display` impl for `EntityId` in favor of explicit `.escaped()` and
+  `.as_ref()` for escaped and unescaped representations (respectively) of the
+  `EntityId`; see note there (#921, resolving #884)
 
 ### Fixed
 

cedar-policy/src/api/id.rs

@@ -24,6 +24,7 @@ use cedar_policy_core::entities::json::err::JsonDeserializationErrorContext;
 use cedar_policy_core::FromNormalizedStr;
 use ref_cast::RefCast;
 use serde::{Deserialize, Serialize};
+use smol_str::SmolStr;
 use std::convert::Infallible;
 use std::str::FromStr;
 
@@ -39,6 +40,12 @@ use std::str::FromStr;
 /// let id : EntityId = "my-id".parse().unwrap_or_else(|never| match never {});
 /// # assert_eq!(id.as_ref(), "my-id");
 /// ```
+///
+/// `EntityId` does not implement `Display`, partly because it is unclear
+/// whether `Display` should produce an escaped representation or an unescaped
+/// representation (see [#884](https://github.com/cedar-policy/cedar/issues/884)).
+/// To get an escaped representation, use `.escaped()`.
+/// To get an unescaped representation, use `.as_ref()`.
 #[repr(transparent)]
 #[allow(clippy::module_name_repetitions)]
 #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, RefCast)]
@@ -52,6 +59,11 @@ impl EntityId {
             Err(infallible) => match infallible {},
         }
     }
+
+    /// Get the contents of the `EntityId` as an escaped string
+    pub fn escaped(&self) -> SmolStr {
+        self.0.escaped()
+    }
 }
 
 impl FromStr for EntityId {
@@ -67,15 +79,6 @@ impl AsRef<str> for EntityId {
     }
 }
 
-// Note that this Display formatter will format the EntityId as it would be expected
-// in the EntityUid string form. For instance, the `"alice"` in `User::"alice"`.
-// This means it adds quotes and potentially performs some escaping.
-impl std::fmt::Display for EntityId {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}", self.0)
-    }
-}
-
 /// Represents an entity type name. Consists of a namespace and the type name.
 ///
 /// An `EntityTypeName` can can be constructed using

cedar-policy/src/tests.rs

@@ -3740,6 +3740,7 @@ mod decimal_ip_constructors {
 
 mod into_iter_entities {
     use super::*;
+    use smol_str::SmolStr;
 
     #[test]
     fn into_iter_entities() {
@@ -3762,9 +3763,9 @@ mod into_iter_entities {
         "#;
 
         let list = Entities::from_json_str(test_data, None).unwrap();
-        let mut list_out: Vec<String> = list
+        let mut list_out: Vec<SmolStr> = list
             .into_iter()
-            .map(|entity| entity.uid().id().to_string())
+            .map(|entity| entity.uid().id().escaped())
             .collect();
         list_out.sort();
         assert_eq!(list_out, &["admin", "alice"]);