cedar-policy · khieta · Aug 9, 2024 · Jun 28, 2024
diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs
@@ -1000,9 +1000,11 @@ impl<T> ExprBuilder<T> {
         for (k, v) in pairs {
             match map.entry(k) {
                 btree_map::Entry::Occupied(oentry) => {
-                    return Err(ExprConstructionError::DuplicateKeyInRecordLiteral {
+                    return Err(expression_construction_errors::DuplicateKeyError {
                         key: oentry.key().clone(),
-                    });
+                        context: "in record literal",
+                    }
+                    .into());
                 }
                 btree_map::Entry::Vacant(ventry) => {
                     ventry.insert(v);
@@ -1148,12 +1150,39 @@ impl<T: Clone> ExprBuilder<T> {
 /// Errors when constructing an `Expr`
 #[derive(Debug, PartialEq, Eq, Clone, Diagnostic, Error)]
 pub enum ExprConstructionError {
-    /// The same key occurred two or more times in a single record literal
-    #[error("duplicate key `{key}` in record literal")]
-    DuplicateKeyInRecordLiteral {
-        /// The key which occurred two or more times in the record literal
-        key: SmolStr,
-    },
+    /// The same key occurred two or more times
+    #[error(transparent)]
+    #[diagnostic(transparent)]
+    DuplicateKey(#[from] expression_construction_errors::DuplicateKeyError),
+}
+
+/// Error subtypes for [`ExprConstructionError`]
+pub mod expression_construction_errors {
+    use miette::Diagnostic;
+    use smol_str::SmolStr;
+    use thiserror::Error;
+
+    /// The same key occurred two or more times
+    #[derive(Debug, PartialEq, Eq, Clone, Diagnostic, Error)]
+    #[error("duplicate key `{key}` {context}")]
+    pub struct DuplicateKeyError {
+        /// The key which occurred two or more times
+        pub(crate) key: SmolStr,
+        /// Information about where the duplicate key occurred (e.g., "in record literal")
+        pub(crate) context: &'static str,
+    }
+
+    impl DuplicateKeyError {
+        /// Get the key which occurred two or more times
+        pub fn key(&self) -> &str {
+            &self.key
+        }
+
+        /// Make a new error with an updated `context` field
+        pub(crate) fn with_context(self, context: &'static str) -> Self {
+            Self { context, ..self }
+        }
+    }
 }
 
 /// A new type wrapper around `Expr` that provides `Eq` and `Hash`

diff --git a/cedar-policy-core/src/ast/request.rs b/cedar-policy-core/src/ast/request.rs
@@ -213,16 +213,15 @@ pub struct Context {
     /// Context is serialized as a `RestrictedExpr`, for partly historical reasons.
     //
     // INVARIANT(ContextRecord): This must be a `Record`: either
-    // `PartialValue::Value(Value::Record)`, or
-    // `PartialValue::Residual(Expr::Record)`, or an appropriate unknown
+    // `PartialValue::Value(Value::Record)` or `PartialValue::Residual(Expr::Record)`
     #[serde(flatten)]
     context: PartialValueSerializedAsExpr,
 }
 
 impl Context {
     /// Create an empty `Context`
     //
-    // INVARIANT(ContextRecord): via invariant on `Self::from_pairs`
+    // INVARIANT(ContextRecord): due to use of `Value::empty_record`
     pub fn empty() -> Self {
         Self {
             context: PartialValue::Value(Value::empty_record(None)).into(),
@@ -240,7 +239,8 @@ impl Context {
         extensions: Extensions<'_>,
     ) -> Result<Self, ContextCreationError> {
         match expr.expr_kind() {
-            // INVARIANT(ContextRecord): guaranteed by the match case
+            // INVARIANT(ContextRecord): `RestrictedEvaluator::partial_interpret`
+            // always returns a record (or an error) given a record as input
             ExprKind::Record { .. } => {
                 let evaluator = RestrictedEvaluator::new(&extensions);
                 let pval = evaluator.partial_interpret(expr)?;
@@ -259,14 +259,17 @@ impl Context {
     ///
     /// `extensions` provides the `Extensions` which should be active for
     /// evaluating the `RestrictedExpr`.
-    //
-    // INVARIANT(ContextRecord): always constructs a record if it returns Ok
     pub fn from_pairs(
         pairs: impl IntoIterator<Item = (SmolStr, RestrictedExpr)>,
         extensions: Extensions<'_>,
     ) -> Result<Self, ContextCreationError> {
         // INVARIANT(ContextRecord): via invariant on `Self::from_expr`
-        Self::from_expr(RestrictedExpr::record(pairs)?.as_borrowed(), extensions)
+        match RestrictedExpr::record(pairs) {
+            Ok(record) => Self::from_expr(record.as_borrowed(), extensions),
+            Err(ExprConstructionError::DuplicateKey(err)) => {
+                Err(ExprConstructionError::DuplicateKey(err.with_context("in context")).into())
+            }
+        }
     }
 
     /// Create a `Context` from a string containing JSON (which must be a JSON
@@ -276,7 +279,7 @@ impl Context {
     ///
     /// For schema-based parsing, use `ContextJsonParser`.
     pub fn from_json_str(json: &str) -> Result<Self, ContextJsonDeserializationError> {
-        // INVARIANT `.from_json_str` always produces an expression of variant `Record`
+        // INVARIANT(ContextRecord): `.from_json_str` always produces an expression of variant `Record`
         ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available())
             .from_json_str(json)
     }
@@ -290,7 +293,7 @@ impl Context {
     pub fn from_json_value(
         json: serde_json::Value,
     ) -> Result<Self, ContextJsonDeserializationError> {
-        // INVARIANT `.from_json_value` always produces an expression of variant `Record`
+        // INVARIANT(ContextRecord): `.from_json_value` always produces an expression of variant `Record`
         ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available())
             .from_json_value(json)
     }
@@ -304,40 +307,73 @@ impl Context {
     pub fn from_json_file(
         json: impl std::io::Read,
     ) -> Result<Self, ContextJsonDeserializationError> {
-        // INVARIANT `.from_json_file` always produces an expression of variant `Record`
+        // INVARIANT(ContextRecord): `.from_json_file` always produces an expression of variant `Record`
         ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available())
             .from_json_file(json)
     }
 
-    /// Iterate over the (key, value) pairs in the `Context`; or return `None`
-    /// if the `Context` is purely unknown
+    /// Private helper function to implement `into_iter()` for `Context`.
+    /// Gets an iterator over the (key, value) pairs in the `Context`, cloning
+    /// only if necessary.
     //
-    // PANIC SAFETY: This is safe due to the invariant on `self.context`, `self.context` must always be a record
-    pub fn iter<'s>(&'s self) -> Option<Box<dyn Iterator<Item = (&SmolStr, PartialValue)> + 's>> {
+    // PANIC SAFETY: This is safe due to the invariant (ContextRecord) on `self.context`
+    fn into_values(self) -> Box<dyn Iterator<Item = (SmolStr, PartialValue)>> {
         // PANIC SAFETY invariant on `self.context` ensures that it is a record
         #[allow(clippy::panic)]
-        match self.context.as_ref() {
+        match self.context.into() {
             PartialValue::Value(Value {
                 value: ValueKind::Record(record),
                 ..
-            }) => Some(Box::new(
-                record
-                    .iter()
-                    .map(|(k, v)| (k, PartialValue::Value(v.clone()))),
-            )),
-            PartialValue::Residual(expr) => match expr.expr_kind() {
-                ExprKind::Record(map) => Some(Box::new(
-                    map.iter()
-                        .map(|(k, v)| (k, PartialValue::Residual(v.clone()))),
-                )),
-                ExprKind::Unknown(_) => None,
+            }) => Box::new(
+                Arc::unwrap_or_clone(record)
+                    .into_iter()
+                    .map(|(k, v)| (k, PartialValue::Value(v))),
+            ),
+            PartialValue::Residual(expr) => match expr.into_expr_kind() {
+                ExprKind::Record(map) => Box::new(
+                    Arc::unwrap_or_clone(map)
+                        .into_iter()
+                        .map(|(k, v)| (k, PartialValue::Residual(v))),
+                ),
                 kind => panic!("internal invariant violation: expected a record, got {kind:?}"),
             },
             v => panic!("internal invariant violation: expected a record, got {v:?}"),
         }
     }
 }
 
+/// Utilities for implementing `IntoIterator` for `Context`
+mod iter {
+    use super::*;
+
+    /// `IntoIter` iterator for `Context`
+    pub struct IntoIter(pub(super) Box<dyn Iterator<Item = (SmolStr, PartialValue)>>);
+
+    impl std::fmt::Debug for IntoIter {
+        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            write!(f, "IntoIter(<context>)")
+        }
+    }
+
+    impl Iterator for IntoIter {
+        type Item = (SmolStr, PartialValue);
+
+        fn next(&mut self) -> Option<Self::Item> {
+            self.0.next()
+        }
+    }
+}
+
+impl IntoIterator for Context {
+    type Item = (SmolStr, PartialValue);
+
+    type IntoIter = iter::IntoIter;
+
+    fn into_iter(self) -> Self::IntoIter {
+        iter::IntoIter(self.into_values())
+    }
+}
+
 impl AsRef<PartialValue> for Context {
     fn as_ref(&self) -> &PartialValue {
         &self.context
@@ -375,8 +411,8 @@ pub enum ContextCreationError {
     #[error(transparent)]
     #[diagnostic(transparent)]
     Evaluation(#[from] EvaluationError),
-    /// Error constructing the expression given for the `Context`.
-    /// Only returned by `Context::from_pairs()`
+    /// Error constructing a record for the `Context`.
+    /// Only returned by `Context::from_pairs()` and `Context::merge()`
     #[error(transparent)]
     #[diagnostic(transparent)]
     ExprConstruction(#[from] ExprConstructionError),

diff --git a/cedar-policy-core/src/ast/restricted_expr.rs b/cedar-policy-core/src/ast/restricted_expr.rs
@@ -650,7 +650,7 @@ pub enum RestrictedExprParseError {
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::ast::ExprConstructionError;
+    use crate::ast::expression_construction_errors;
     use crate::parser::err::{ParseError, ToASTError, ToASTErrorKind};
     use crate::parser::Loc;
     use std::str::FromStr;
@@ -664,7 +664,11 @@ mod test {
                 ("foo".into(), RestrictedExpr::val(37),),
                 ("foo".into(), RestrictedExpr::val("hello"),),
             ]),
-            Err(ExprConstructionError::DuplicateKeyInRecordLiteral { key: "foo".into() })
+            Err(expression_construction_errors::DuplicateKeyError {
+                key: "foo".into(),
+                context: "in record literal",
+            }
+            .into())
         );
 
         // duplicate key is an error when mapped to different values of same type
@@ -673,7 +677,11 @@ mod test {
                 ("foo".into(), RestrictedExpr::val(37),),
                 ("foo".into(), RestrictedExpr::val(101),),
             ]),
-            Err(ExprConstructionError::DuplicateKeyInRecordLiteral { key: "foo".into() })
+            Err(expression_construction_errors::DuplicateKeyError {
+                key: "foo".into(),
+                context: "in record literal",
+            }
+            .into())
         );
 
         // duplicate key is an error when mapped to the same value multiple times
@@ -682,7 +690,11 @@ mod test {
                 ("foo".into(), RestrictedExpr::val(37),),
                 ("foo".into(), RestrictedExpr::val(37),),
             ]),
-            Err(ExprConstructionError::DuplicateKeyInRecordLiteral { key: "foo".into() })
+            Err(expression_construction_errors::DuplicateKeyError {
+                key: "foo".into(),
+                context: "in record literal",
+            }
+            .into())
         );
 
         // duplicate key is an error even when other keys appear in between
@@ -694,7 +706,11 @@ mod test {
                 ("foo".into(), RestrictedExpr::val(37),),
                 ("eggs".into(), RestrictedExpr::val("spam"),),
             ]),
-            Err(ExprConstructionError::DuplicateKeyInRecordLiteral { key: "foo".into() })
+            Err(expression_construction_errors::DuplicateKeyError {
+                key: "foo".into(),
+                context: "in record literal",
+            }
+            .into())
         );
 
         // duplicate key is also an error when parsing from string
@@ -704,7 +720,11 @@ mod test {
             Err(RestrictedExprParseError::Parse(ParseErrors(vec![
                 ParseError::ToAST(ToASTError::new(
                     ToASTErrorKind::ExprConstructionError(
-                        ExprConstructionError::DuplicateKeyInRecordLiteral { key: "foo".into() }
+                        expression_construction_errors::DuplicateKeyError {
+                            key: "foo".into(),
+                            context: "in record literal",
+                        }
+                        .into()
                     ),
                     Loc::new(0..32, Arc::from(str))
                 ))

diff --git a/cedar-policy-core/src/entities/json/err.rs b/cedar-policy-core/src/entities/json/err.rs
@@ -122,14 +122,10 @@ pub enum JsonDeserializationError {
         /// argument type of the constructor we were looking for
         arg_type: Box<SchemaType>,
     },
-    /// The same key appears two or more times in a single record literal
-    #[error("{ctx}, duplicate key `{key}` in record literal")]
-    DuplicateKeyInRecordLiteral {
-        /// Context of this error
-        ctx: Box<JsonDeserializationErrorContext>,
-        /// The key that appeared two or more times
-        key: SmolStr,
-    },
+    /// The same key appears two or more times in a single record
+    #[error(transparent)]
+    #[diagnostic(transparent)]
+    DuplicateKey(DuplicateKey),
     /// Error when evaluating an entity attribute
     #[error(transparent)]
     #[diagnostic(transparent)]
@@ -238,6 +234,28 @@ pub enum JsonDeserializationError {
     Null(Box<JsonDeserializationErrorContext>),
 }
 
+impl JsonDeserializationError {
+    pub(crate) fn duplicate_key(
+        ctx: JsonDeserializationErrorContext,
+        key: impl Into<SmolStr>,
+    ) -> Self {
+        Self::DuplicateKey(DuplicateKey {
+            ctx: Box::new(ctx),
+            key: key.into(),
+        })
+    }
+}
+
+#[derive(Debug, Error, Diagnostic)]
+#[error("{}, duplicate key `{}` in record", .ctx, .key)]
+/// Error type for records having duplicate keys
+pub struct DuplicateKey {
+    /// Context of this error
+    ctx: Box<JsonDeserializationErrorContext>,
+    /// The key that appeared two or more times
+    key: SmolStr,
+}
+
 /// Errors thrown during serialization to JSON
 #[derive(Debug, Diagnostic, Error)]
 pub enum JsonSerializationError {

diff --git a/cedar-policy-core/src/entities/json/value.rs b/cedar-policy-core/src/entities/json/value.rs
@@ -18,8 +18,8 @@ use super::{
     JsonDeserializationError, JsonDeserializationErrorContext, JsonSerializationError, SchemaType,
 };
 use crate::ast::{
-    BorrowedRestrictedExpr, Eid, EntityUID, ExprConstructionError, ExprKind, Literal, Name,
-    RestrictedExpr, Unknown, Value, ValueKind,
+    expression_construction_errors, BorrowedRestrictedExpr, Eid, EntityUID, ExprConstructionError,
+    ExprKind, Literal, Name, RestrictedExpr, Unknown, Value, ValueKind,
 };
 use crate::entities::{
     schematype_of_restricted_expr, EntitySchemaConformanceError, EscapeKind, GetSchemaTypeError,
@@ -244,12 +244,9 @@ impl CedarValueJson {
                     .collect::<Result<Vec<_>, JsonDeserializationError>>()?,
             )
             .map_err(|e| match e {
-                ExprConstructionError::DuplicateKeyInRecordLiteral { key } => {
-                    JsonDeserializationError::DuplicateKeyInRecordLiteral {
-                        ctx: Box::new(ctx()),
-                        key,
-                    }
-                }
+                ExprConstructionError::DuplicateKey(
+                    expression_construction_errors::DuplicateKeyError { key, .. },
+                ) => JsonDeserializationError::duplicate_key(ctx(), key),
             })?),
             Self::EntityEscape { __entity: entity } => Ok(RestrictedExpr::val(
                 EntityUID::try_from(entity.clone()).map_err(|errs| {
@@ -583,12 +580,9 @@ impl<'e> ValueParser<'e> {
                     // duplicate keys; they're both maps), but we can still throw
                     // the error properly in the case that it somehow happens
                     RestrictedExpr::record(rexpr_pairs).map_err(|e| match e {
-                        ExprConstructionError::DuplicateKeyInRecordLiteral { key } => {
-                            JsonDeserializationError::DuplicateKeyInRecordLiteral {
-                                ctx: Box::new(ctx2()),
-                                key,
-                            }
-                        }
+                        ExprConstructionError::DuplicateKey(
+                            expression_construction_errors::DuplicateKeyError { key, .. },
+                        ) => JsonDeserializationError::duplicate_key(ctx2(), key),
                     })
                 }
                 val => {