Improve schema parsing error on cycle in action hierarchy (#436)

Fixes #416
cedar-policy · Nov 16, 2023 · c9bdef1 · c9bdef1
1 parent 288877d
commit c9bdef1
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 20 deletions.
diff --git a/cedar-policy-validator/src/err.rs b/cedar-policy-validator/src/err.rs
@@ -64,8 +64,8 @@ pub enum SchemaError {
     #[error("duplicate common type `{0}`")]
     DuplicateCommonType(String),
     /// Cycle in the schema's action hierarchy.
-    #[error("cycle in action hierarchy")]
-    CycleInActionHierarchy,
+    #[error("cycle in action hierarchy containing `{0}`")]
+    CycleInActionHierarchy(EntityUID),
     /// Parse errors occurring while parsing an entity type.
     #[error("parse error in entity type: {}", Self::format_parse_errs(.0))]
     ParseEntityType(ParseErrors),
@@ -110,7 +110,9 @@ impl From<transitive_closure::TcError<EntityUID>> for SchemaError {
             transitive_closure::TcError::MissingTcEdge { .. } => {
                 SchemaError::ActionTransitiveClosure(Box::new(e))
             }
-            transitive_closure::TcError::HasCycle { .. } => SchemaError::CycleInActionHierarchy,
+            transitive_closure::TcError::HasCycle { vertex_with_loop } => {
+                SchemaError::CycleInActionHierarchy(vertex_with_loop)
+            }
         }
     }
 }

diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs
@@ -609,6 +609,7 @@ mod test {
 
     use cedar_policy_core::ast::RestrictedExpr;
     use cedar_policy_core::parser::err::{ParseError, ToASTError};
+    use cool_asserts::assert_matches;
     use serde_json::json;
 
     use super::*;
@@ -856,11 +857,12 @@ mod test {
         });
         let schema_file: NamespaceDefinition = serde_json::from_value(src).expect("Parse Error");
         let schema: Result<ValidatorSchema> = schema_file.try_into();
-        match schema {
-            Ok(_) => panic!("from_schema_file should have failed"),
-            Err(SchemaError::CycleInActionHierarchy) => (), // expected result
-            e => panic!("Unexpected error from from_schema_file: {:?}", e),
-        }
+        assert_matches!(
+            schema,
+            Err(SchemaError::CycleInActionHierarchy(euid)) => {
+                assert_eq!(euid, r#"Action::"view_photo""#.parse().unwrap());
+            }
+        )
     }
 
     // Slightly more complex cycle in action hierarchy
@@ -887,14 +889,11 @@ mod test {
         });
         let schema_file: NamespaceDefinition = serde_json::from_value(src).expect("Parse Error");
         let schema: Result<ValidatorSchema> = schema_file.try_into();
-        match schema {
-            Ok(x) => {
-                println!("{:?}", x);
-                panic!("from_schema_file should have failed");
-            }
-            Err(SchemaError::CycleInActionHierarchy) => (), // expected result
-            e => panic!("Unexpected error from from_schema_file: {:?}", e),
-        }
+        assert_matches!(
+            schema,
+            // The exact action reported as being in the cycle isn't deterministic.
+            Err(SchemaError::CycleInActionHierarchy(_)),
+        )
     }
 
     #[test]

diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md
@@ -72,6 +72,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `ValidationError::location`. (#405)
 - `ValidationWarningKind` is now `non_exhaustive`, allowing future warnings to
   be added without a breaking change. (#404)
+- Improve schema parsing error messages when a cycle exists in the action
+  hierarchy to includes an action which is part of the cycle (#436, resolving #416).
 
 ### Fixed
 

diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs
@@ -1177,8 +1177,8 @@ pub enum SchemaError {
     #[error("duplicate common type `{0}`")]
     DuplicateCommonType(String),
     /// Cycle in the schema's action hierarchy.
-    #[error("cycle in action hierarchy")]
-    CycleInActionHierarchy,
+    #[error("cycle in action hierarchy containing `{0}`")]
+    CycleInActionHierarchy(EntityUid),
     /// Parse errors occurring while parsing an entity type.
     #[error("parse error in entity type: {0}")]
     ParseEntityType(ParseErrors),
@@ -1280,8 +1280,8 @@ impl From<cedar_policy_validator::SchemaError> for SchemaError {
             cedar_policy_validator::SchemaError::DuplicateCommonType(c) => {
                 Self::DuplicateCommonType(c)
             }
-            cedar_policy_validator::SchemaError::CycleInActionHierarchy => {
-                Self::CycleInActionHierarchy
+            cedar_policy_validator::SchemaError::CycleInActionHierarchy(e) => {
+                Self::CycleInActionHierarchy(EntityUid(e))
             }
             cedar_policy_validator::SchemaError::ParseEntityType(e) => Self::ParseEntityType(e),
             cedar_policy_validator::SchemaError::ParseNamespace(e) => Self::ParseNamespace(e),