Correctly merge schema errors (#1474)

Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>

cedar-policy-validator/src/lib.rs

@@ -52,6 +52,7 @@ pub use str_checks::confusable_string_checks;
 pub mod cedar_schema;
 pub mod typecheck;
 use typecheck::Typechecker;
+mod partition_nonempty;
 pub mod types;
 
 /// Used to select how a policy will be validated.

cedar-policy-validator/src/partition_nonempty.rs

@@ -0,0 +1,27 @@
+use itertools::Itertools;
+use nonempty::NonEmpty;
+
+/// A trait for partitioning a collection of `Result`s into a collection of `Ok` values or a `NonEmpty` of `Err` values.
+pub(crate) trait PartitionNonEmpty<T, E> {
+    fn partition_nonempty<C>(self) -> std::result::Result<C, NonEmpty<E>>
+    where
+        C: Default + Extend<T>;
+}
+
+impl<I, T, E> PartitionNonEmpty<T, E> for I
+where
+    I: Iterator<Item = Result<T, E>>,
+{
+    fn partition_nonempty<C>(self) -> Result<C, NonEmpty<E>>
+    where
+        C: Default + Extend<T>,
+    {
+        let (oks, errs): (_, Vec<_>) = self.partition_result();
+
+        if let Some(errs) = NonEmpty::from_vec(errs) {
+            Err(errs)
+        } else {
+            Ok(oks)
+        }
+    }
+}

cedar-policy-validator/src/schema.rs

@@ -27,7 +27,6 @@ use cedar_policy_core::{
     parser::Loc,
     transitive_closure::compute_tc,
 };
-use itertools::Itertools;
 use namespace_def::EntityTypeFragment;
 use nonempty::NonEmpty;
 use serde::{Deserialize, Serialize};
@@ -40,6 +39,7 @@ use std::sync::Arc;
 use crate::{
     cedar_schema::SchemaWarning,
     json_schema,
+    partition_nonempty::PartitionNonEmpty,
     types::{Attributes, EntityRecordKind, OpenTag, Type},
 };
 
@@ -124,7 +124,7 @@ impl ValidatorSchemaFragment<ConditionalName, ConditionalName> {
                         extensions,
                     )
                 })
-                .collect::<Result<Vec<_>>>()?,
+                .partition_nonempty()?,
         ))
     }
 
@@ -138,16 +138,12 @@ impl ValidatorSchemaFragment<ConditionalName, ConditionalName> {
         self,
         all_defs: &AllDefs,
     ) -> Result<ValidatorSchemaFragment<InternalName, EntityType>> {
-        let (nsdefs, errs) = self
-            .0
+        self.0
             .into_iter()
             .map(|ns_def| ns_def.fully_qualify_type_references(all_defs))
-            .partition_result::<Vec<ValidatorNamespaceDef<InternalName, EntityType>>, Vec<SchemaError>, _, _>();
-        if let Some(errs) = NonEmpty::from_vec(errs) {
-            Err(SchemaError::join_nonempty(errs))
-        } else {
-            Ok(ValidatorSchemaFragment(nsdefs))
-        }
+            .partition_nonempty()
+            .map(ValidatorSchemaFragment)
+            .map_err(SchemaError::join_nonempty)
     }
 }
 
@@ -463,13 +459,10 @@ impl ValidatorSchema {
         // come later.)
         // This produces an intermediate form of schema fragment,
         // `ValidatorSchemaFragment<InternalName, EntityType>`.
-        let (fragments, errs) = fragments
+        let fragments: Vec<_> = fragments
             .into_iter()
             .map(|frag| frag.fully_qualify_type_references(&all_defs))
-            .partition_result::<Vec<ValidatorSchemaFragment<InternalName, EntityType>>, Vec<SchemaError>, _, _>();
-        if let Some(errs) = NonEmpty::from_vec(errs) {
-            return Err(SchemaError::join_nonempty(errs));
-        }
+            .partition_nonempty()?;
 
         // Now that all references are fully-qualified, we can build the aggregate
         // maps for common types, entity types, and actions, checking that nothing
@@ -528,7 +521,6 @@ impl ValidatorSchema {
                     .insert(name.clone());
             }
         }
-
         let mut entity_types = entity_type_fragments
             .into_iter()
             .map(|(name, entity_type)| -> Result<_> {
@@ -582,7 +574,7 @@ impl ValidatorSchema {
                     }
                 }
             })
-            .collect::<Result<HashMap<_, _>>>()?;
+            .partition_nonempty()?;
 
         let mut action_children = HashMap::new();
         for (euid, action) in action_fragments.iter() {
@@ -619,7 +611,7 @@ impl ValidatorSchema {
                     },
                 ))
             })
-            .collect::<Result<HashMap<_, _>>>()?;
+            .partition_nonempty()?;
 
         // We constructed entity types and actions with child maps, but we need
         // transitively closed descendants.
@@ -1345,7 +1337,7 @@ impl<'a> CommonTypeResolver<'a> {
                     attributes: BTreeMap::from_iter(
                         attributes
                             .into_iter()
-                            .map(|(attr, attr_ty)| {
+                            .map(|(attr, attr_ty)| -> Result<_> {
                                 Ok((
                                     attr,
                                     json_schema::TypeOfAttribute {
@@ -1355,7 +1347,7 @@ impl<'a> CommonTypeResolver<'a> {
                                     },
                                 ))
                             })
-                            .collect::<Result<Vec<(_, _)>>>()?,
+                            .partition_nonempty::<Vec<_>>()?,
                     ),
                     additional_attributes,
                 }),
@@ -1584,6 +1576,85 @@ pub(crate) mod test {
         }
     }
 
+    #[test]
+    fn test_from_schema_file_missing_parent_action() {
+        let src = json!({
+            "": {
+                "entityTypes": {
+                    "Test": {}
+                },
+                "actions": {
+                    "doTests": {
+                        "memberOf": [
+                            { "type": "Action", "id": "test1" },
+                            { "type": "Action", "id": "test2" }
+                        ]
+                    }
+                }
+            }
+        });
+        match ValidatorSchema::from_json_value(src, Extensions::all_available()) {
+            Err(SchemaError::ActionNotDefined(missing)) => {
+                assert_eq!(missing.0.len(), 2);
+            }
+            _ => panic!("Expected ActionNotDefined due to unknown actions in memberOf."),
+        }
+    }
+
+    #[test]
+    fn test_from_schema_file_undefined_types_in_common() {
+        let src = json!({
+            "": {
+                "commonTypes": {
+                    "My1": {"type": "What"},
+                    "My2": {"type": "Ev"},
+                    "My3": {"type": "Er"}
+                },
+                "entityTypes": {
+                    "Test": {}
+                },
+                "actions": {},
+            }
+        });
+        match ValidatorSchema::from_json_value(src, Extensions::all_available()) {
+            Err(SchemaError::TypeNotDefined(missing)) => {
+                assert_eq!(missing.undefined_types.len(), 3);
+            }
+            x => panic!(
+                "Expected TypeNotDefined due to unknown types in commonTypes, found: {:?}",
+                x
+            ),
+        }
+    }
+
+    #[test]
+    fn test_from_schema_file_undefined_entities_in_one_action() {
+        let src = json!({
+            "": {
+                "entityTypes": {
+                    "Test": {}
+                },
+                "actions": {
+                    "doTests": {
+                        "appliesTo": {
+                            "principalTypes": ["Usr", "Group"],
+                            "resourceTypes": ["Phoot"]
+                        }
+                    }
+                }
+            }
+        });
+        match ValidatorSchema::from_json_value(src, Extensions::all_available()) {
+            Err(SchemaError::TypeNotDefined(missing)) => {
+                assert_eq!(missing.undefined_types.len(), 3);
+            }
+            x => panic!(
+                "Expected TypeNotDefined due to unknown entities in appliesTo, found: {:?}",
+                x
+            ),
+        }
+    }
+
     // Undefined entity types "Grop", "Usr", "Phoot"
     #[test]
     fn test_from_schema_file_undefined_entities() {
@@ -2623,7 +2694,7 @@ pub(crate) mod test {
                 view_photo_uid,
                 [],
                 HashSet::new(),
-                HashSet::from([view_uid.clone(), read_uid.clone()]),
+                HashSet::from([view_uid, read_uid.clone()]),
                 [],
             )
         );

cedar-policy-validator/src/schema/action.rs

@@ -20,14 +20,13 @@ use cedar_policy_core::{
     ast::{self, EntityType, EntityUID, PartialValueSerializedAsExpr},
     transitive_closure::TCNode,
 };
-use itertools::Itertools;
-use nonempty::NonEmpty;
 use serde::Serialize;
 use smol_str::SmolStr;
 use std::collections::{BTreeMap, HashSet};
 
 use super::internal_name_to_entity_type;
 use crate::{
+    partition_nonempty::PartitionNonEmpty,
     schema::{AllDefs, SchemaError},
     types::{Attributes, Type},
     ConditionalName,
@@ -239,33 +238,31 @@ impl ValidatorApplySpec<ConditionalName> {
         self,
         all_defs: &AllDefs,
     ) -> Result<ValidatorApplySpec<ast::EntityType>, crate::schema::SchemaError> {
-        let (principal_apply_spec, principal_errs) = self
+        let principal_apply_spec = self
             .principal_apply_spec
             .into_iter()
             .map(|cname| {
                 let internal_name = cname.resolve(all_defs)?;
                 internal_name_to_entity_type(internal_name).map_err(Into::into)
             })
-            .partition_result::<_, Vec<SchemaError>, _, _>();
-        let (resource_apply_spec, resource_errs) = self
+            .partition_nonempty();
+        let resource_apply_spec = self
             .resource_apply_spec
             .into_iter()
             .map(|cname| {
                 let internal_name = cname.resolve(all_defs)?;
                 internal_name_to_entity_type(internal_name).map_err(Into::into)
             })
-            .partition_result::<_, Vec<SchemaError>, _, _>();
-        match (
-            NonEmpty::from_vec(principal_errs),
-            NonEmpty::from_vec(resource_errs),
-        ) {
-            (None, None) => Ok(ValidatorApplySpec {
+            .partition_nonempty();
+
+        match (principal_apply_spec, resource_apply_spec) {
+            (Ok(principal_apply_spec), Ok(resource_apply_spec)) => Ok(ValidatorApplySpec {
                 principal_apply_spec,
                 resource_apply_spec,
             }),
-            (Some(principal_errs), None) => Err(SchemaError::join_nonempty(principal_errs)),
-            (None, Some(resource_errs)) => Err(SchemaError::join_nonempty(resource_errs)),
-            (Some(principal_errs), Some(resource_errs)) => {
+            (Ok(_), Err(errs)) => Err(SchemaError::join_nonempty(errs)),
+            (Err(resource_errs), Ok(_)) => Err(SchemaError::join_nonempty(resource_errs)),
+            (Err(principal_errs), Err(resource_errs)) => {
                 let mut errs = principal_errs;
                 errs.extend(resource_errs);
                 Err(SchemaError::join_nonempty(errs))

cedar-policy-validator/src/schema/err.rs

@@ -310,6 +310,24 @@ impl SchemaError {
     }
 }
 
+impl From<NonEmpty<SchemaError>> for SchemaError {
+    fn from(errs: NonEmpty<SchemaError>) -> Self {
+        Self::join_nonempty(errs)
+    }
+}
+
+impl From<NonEmpty<schema_errors::ActionNotDefinedError>> for SchemaError {
+    fn from(errs: NonEmpty<schema_errors::ActionNotDefinedError>) -> Self {
+        Self::ActionNotDefined(schema_errors::ActionNotDefinedError::join_nonempty(errs))
+    }
+}
+
+impl From<NonEmpty<schema_errors::TypeNotDefinedError>> for SchemaError {
+    fn from(errs: NonEmpty<schema_errors::TypeNotDefinedError>) -> Self {
+        Self::TypeNotDefined(schema_errors::TypeNotDefinedError::join_nonempty(errs))
+    }
+}
+
 /// Convenience alias
 pub type Result<T> = std::result::Result<T, SchemaError>;
 
@@ -431,6 +449,12 @@ pub mod schema_errors {
         }
     }
 
+    impl From<NonEmpty<TypeNotDefinedError>> for TypeNotDefinedError {
+        fn from(value: NonEmpty<TypeNotDefinedError>) -> Self {
+            Self::join_nonempty(value)
+        }
+    }
+
     /// Action resolution error
     //
     // CAUTION: this type is publicly exported in `cedar-policy`.
@@ -452,6 +476,12 @@ pub mod schema_errors {
         }
     }
 
+    impl From<NonEmpty<ActionNotDefinedError>> for ActionNotDefinedError {
+        fn from(value: NonEmpty<ActionNotDefinedError>) -> Self {
+            Self::join_nonempty(value)
+        }
+    }
+
     impl Display for ActionNotDefinedError {
         fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
             if self.0.len() == 1 {