Improve error messages for request validation errors (#1357)

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

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

@@ -427,10 +427,21 @@ impl Context {
             .from_json_file(json)
     }
 
+    /// Get the number of keys in this `Context`.
+    pub fn num_keys(&self) -> usize {
+        match self {
+            Context::Value(record) => record.len(),
+            Context::RestrictedResidual(record) => record.len(),
+        }
+    }
+
     /// Private helper function to implement `into_iter()` for `Context`.
     /// Gets an iterator over the (key, value) pairs in the `Context`, cloning
     /// only if necessary.
-    fn into_values(self) -> Box<dyn Iterator<Item = (SmolStr, RestrictedExpr)>> {
+    ///
+    /// Note that some error messages rely on this function returning keys in
+    /// sorted order, or else the error message will not be fully deterministic.
+    fn into_pairs(self) -> Box<dyn Iterator<Item = (SmolStr, RestrictedExpr)>> {
         match self {
             Context::Value(record) => Box::new(
                 Arc::unwrap_or_clone(record)
@@ -507,11 +518,10 @@ mod iter {
 
 impl IntoIterator for Context {
     type Item = (SmolStr, RestrictedExpr);
-
     type IntoIter = iter::IntoIter;
 
     fn into_iter(self) -> Self::IntoIter {
-        iter::IntoIter(self.into_values())
+        iter::IntoIter(self.into_pairs())
     }
 }
 

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

@@ -17,6 +17,7 @@
 use crate::ast::*;
 use crate::parser::Loc;
 use std::collections::{BTreeMap, BTreeSet, HashSet};
+use std::str::FromStr;
 use std::sync::Arc;
 
 use educe::Educe;
@@ -147,6 +148,19 @@ impl Value {
     pub fn eq_and_same_source_loc(&self, other: &Self) -> bool {
         self == other && self.source_loc() == other.source_loc()
     }
+
+    /// Alternate `Display` impl, that truncates large sets/records (including recursively).
+    ///
+    /// `n`: the maximum number of set elements, or record key-value pairs, that
+    /// will be shown before eliding the rest with `..`.
+    /// `None` means no bound.
+    pub fn bounded_display(
+        &self,
+        f: &mut std::fmt::Formatter<'_>,
+        n: Option<usize>,
+    ) -> std::fmt::Result {
+        self.value.bounded_display(f, n)
+    }
 }
 
 impl ValueKind {
@@ -194,6 +208,99 @@ impl ValueKind {
             _ => None,
         }
     }
+
+    /// Alternate `Display` impl, that truncates large sets/records (including recursively).
+    ///
+    /// `n`: the maximum number of set elements, or record key-value pairs, that
+    /// will be shown before eliding the rest with `..`.
+    /// `None` means no bound.
+    pub fn bounded_display(
+        &self,
+        f: &mut std::fmt::Formatter<'_>,
+        n: Option<usize>,
+    ) -> std::fmt::Result {
+        match self {
+            Self::Lit(lit) => write!(f, "{lit}"),
+            Self::Set(Set {
+                fast,
+                authoritative,
+            }) => {
+                write!(f, "[")?;
+                let truncated = n.map(|n| authoritative.len() > n).unwrap_or(false);
+                if let Some(rc) = fast {
+                    // sort the elements, because we want the Display output to be
+                    // deterministic, particularly for tests which check equality
+                    // of error messages
+                    let elements = match n {
+                        Some(n) => Box::new(rc.as_ref().iter().sorted_unstable().take(n))
+                            as Box<dyn Iterator<Item = &Literal>>,
+                        None => Box::new(rc.as_ref().iter().sorted_unstable())
+                            as Box<dyn Iterator<Item = &Literal>>,
+                    };
+                    for (i, item) in elements.enumerate() {
+                        write!(f, "{item}")?;
+                        if i < authoritative.len() - 1 {
+                            write!(f, ", ")?;
+                        }
+                    }
+                } else {
+                    // don't need to sort the elements in this case because BTreeSet iterates
+                    // in a deterministic order already
+                    let elements = match n {
+                        Some(n) => Box::new(authoritative.as_ref().iter().take(n))
+                            as Box<dyn Iterator<Item = &Value>>,
+                        None => Box::new(authoritative.as_ref().iter())
+                            as Box<dyn Iterator<Item = &Value>>,
+                    };
+                    for (i, item) in elements.enumerate() {
+                        item.bounded_display(f, n)?;
+                        if i < authoritative.len() - 1 {
+                            write!(f, ", ")?;
+                        }
+                    }
+                }
+                if truncated {
+                    write!(f, ".. ")?;
+                }
+                write!(f, "]")?;
+                Ok(())
+            }
+            Self::Record(record) => {
+                write!(f, "{{")?;
+                let truncated = n.map(|n| record.len() > n).unwrap_or(false);
+                // no need to sort the elements because BTreeMap iterates in a
+                // deterministic order already
+                let elements = match n {
+                    Some(n) => Box::new(record.as_ref().iter().take(n))
+                        as Box<dyn Iterator<Item = (&SmolStr, &Value)>>,
+                    None => Box::new(record.as_ref().iter())
+                        as Box<dyn Iterator<Item = (&SmolStr, &Value)>>,
+                };
+                for (i, (k, v)) in elements.enumerate() {
+                    match UnreservedId::from_str(k) {
+                        Ok(k) => {
+                            // we can omit the quotes around the key, it's a valid identifier and not a reserved keyword
+                            write!(f, "{k}: ")?;
+                        }
+                        Err(_) => {
+                            // put quotes around the key
+                            write!(f, "\"{k}\": ")?;
+                        }
+                    }
+                    v.bounded_display(f, n)?;
+                    if i < record.len() - 1 {
+                        write!(f, ", ")?;
+                    }
+                }
+                if truncated {
+                    write!(f, ".. ")?;
+                }
+                write!(f, "}}")?;
+                Ok(())
+            }
+            Self::ExtensionValue(ev) => write!(f, "{}", RestrictedExpr::from(ev.as_ref().clone())),
+        }
+    }
 }
 
 #[derive(Debug, Error)]
@@ -457,47 +564,7 @@ impl std::fmt::Display for Value {
 
 impl std::fmt::Display for ValueKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            Self::Lit(lit) => write!(f, "{}", lit),
-            Self::Set(Set {
-                fast,
-                authoritative,
-            }) => {
-                match authoritative.len() {
-                    0 => write!(f, "[]"),
-                    n @ 1..=5 => {
-                        write!(f, "[")?;
-                        if let Some(rc) = fast {
-                            // sort the elements, because we want the Display output to be
-                            // deterministic, particularly for tests which check equality
-                            // of error messages
-                            for (i, item) in rc.as_ref().iter().sorted_unstable().enumerate() {
-                                write!(f, "{item}")?;
-                                if i < n - 1 {
-                                    write!(f, ", ")?;
-                                }
-                            }
-                        } else {
-                            // don't need to sort the elements in this case because BTreeSet iterates
-                            // in a deterministic order already
-                            for (i, item) in authoritative.as_ref().iter().enumerate() {
-                                write!(f, "{item}")?;
-                                if i < n - 1 {
-                                    write!(f, ", ")?;
-                                }
-                            }
-                        }
-                        write!(f, "]")?;
-                        Ok(())
-                    }
-                    n => write!(f, "<set with {} elements>", n),
-                }
-            }
-            Self::Record(record) => {
-                write!(f, "<first-class record with {} fields>", record.len())
-            }
-            Self::ExtensionValue(ev) => write!(f, "{}", RestrictedExpr::from(ev.as_ref().clone())),
-        }
+        self.bounded_display(f, None)
     }
 }