Suggest obvious fix when action id includes type

cedar-policy-validator/src/diagnostics.rs

@@ -19,6 +19,7 @@
 
 use miette::Diagnostic;
 use thiserror::Error;
+use validation_errors::UnrecognizedActionIdHelp;
 
 use std::collections::BTreeSet;
 
@@ -195,13 +196,13 @@ impl ValidationError {
 
         policy_id: PolicyID,
         actual_action_id: String,
-        suggested_action_id: Option<String>,
+        hint: Option<UnrecognizedActionIdHelp>,
     ) -> Self {
         validation_errors::UnrecognizedActionId {
             source_loc,
             policy_id,
             actual_action_id,
-            suggested_action_id,
+            hint,
         }
         .into()
     }

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

@@ -26,10 +26,12 @@ use cedar_policy_core::parser::Loc;
 
 use std::collections::BTreeSet;
 
-use cedar_policy_core::ast::{EntityType, EntityUID, Expr, ExprKind, PolicyID, Var};
+use cedar_policy_core::ast::{Eid, EntityType, EntityUID, Expr, ExprKind, PolicyID, Var};
 use cedar_policy_core::parser::join_with_conjunction;
 
+use crate::fuzzy_match::fuzzy_search;
 use crate::types::{EntityLUB, EntityRecordKind, RequestEnv, Type};
+use crate::ValidatorSchema;
 use itertools::Itertools;
 use smol_str::SmolStr;
 
@@ -68,21 +70,59 @@ pub struct UnrecognizedActionId {
     pub source_loc: Option<Loc>,
     /// Policy ID where the error occurred
     pub policy_id: PolicyID,
-    /// Action Id seen in the policy.
+    /// Action Id seen in the policy
     pub actual_action_id: String,
-    /// An action id from the schema that the user might reasonably have
-    /// intended to write.
-    pub suggested_action_id: Option<String>,
+    /// Hint for resolving the error
+    pub hint: Option<UnrecognizedActionIdHelp>,
 }
 
 impl Diagnostic for UnrecognizedActionId {
     impl_diagnostic_from_source_loc_opt_field!(source_loc);
 
     fn help<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> {
-        match &self.suggested_action_id {
-            Some(s) => Some(Box::new(format!("did you mean `{s}`?"))),
-            None => None,
-        }
+        self.hint
+            .as_ref()
+            .map(|help| Box::new(help) as Box<dyn std::fmt::Display>)
+    }
+}
+
+/// Help for resolving an unrecognized action id error
+#[derive(Debug, Clone, Error, Hash, Eq, PartialEq)]
+pub enum UnrecognizedActionIdHelp {
+    /// Draw attention to action id including action type (e.g., `Action::"Action::view"`)
+    #[error("did you intend to include the type in action `{0}`?")]
+    AvoidActionTypeInActionId(String),
+    /// Suggest an alternative action
+    #[error("did you mean `{0}`?")]
+    SuggestAlternative(String),
+}
+
+/// Determine the help to offer in the presence of an unrecognized action id error.
+pub fn unrecognized_action_id_help(
+    euid: &EntityUID,
+    schema: &ValidatorSchema,
+) -> Option<UnrecognizedActionIdHelp> {
+    // Check if the user has included the type (i.e., `Action::`) in the action id
+    let eid_str: &str = euid.eid().as_ref();
+    let eid_with_type = format!("Action::{}", eid_str);
+    let eid_with_type_and_quotes = format!("Action::\"{}\"", eid_str);
+    let maybe_id_with_type = schema.known_action_ids().find(|euid| {
+        let eid = <Eid as AsRef<str>>::as_ref(euid.eid());
+        eid.contains(&eid_with_type) || eid.contains(&eid_with_type_and_quotes)
+    });
+    if let Some(id) = maybe_id_with_type {
+        // In that case, let the user know about it
+        Some(UnrecognizedActionIdHelp::AvoidActionTypeInActionId(
+            id.to_string(),
+        ))
+    } else {
+        // Otherwise, suggest using another id
+        let euids_strs = schema
+            .known_action_ids()
+            .map(ToString::to_string)
+            .collect::<Vec<_>>();
+        fuzzy_search(euid.eid().as_ref(), &euids_strs)
+            .map(UnrecognizedActionIdHelp::SuggestAlternative)
     }
 }
 

cedar-policy-validator/src/lib.rs

@@ -212,6 +212,7 @@ mod test {
     use std::{collections::HashMap, sync::Arc};
 
     use crate::types::Type;
+    use crate::validation_errors::UnrecognizedActionIdHelp;
     use crate::Result;
 
     use super::*;
@@ -290,7 +291,9 @@ mod test {
             Some(Loc::new(45..60, Arc::from(policy_a_src))),
             PolicyID::from_string("pola"),
             "Action::\"actin\"".to_string(),
-            Some("Action::\"action\"".to_string()),
+            Some(UnrecognizedActionIdHelp::SuggestAlternative(
+                "Action::\"action\"".to_string(),
+            )),
         );
 
         assert!(!result.validation_passed());

cedar-policy-validator/src/rbac.rs

@@ -28,6 +28,7 @@ use std::{collections::HashSet, sync::Arc};
 
 use crate::{
     expr_iterator::{policy_entity_type_names, policy_entity_uids},
+    validation_errors::unrecognized_action_id_help,
     ValidationError,
 };
 
@@ -76,19 +77,14 @@ impl Validator {
     ) -> impl Iterator<Item = ValidationError> + 'a {
         // Valid action id names that will be used to generate suggestions if an
         // action id is not found
-        let known_action_ids = self
-            .schema
-            .known_action_ids()
-            .map(ToString::to_string)
-            .collect::<Vec<_>>();
         policy_entity_uids(template).filter_map(move |euid| {
             let entity_type = euid.entity_type();
             if entity_type.is_action() && !self.schema.is_known_action_id(euid) {
                 Some(ValidationError::unrecognized_action_id(
                     euid.loc().cloned(),
                     template.id().clone(),
                     euid.to_string(),
-                    fuzzy_search(euid.eid().as_ref(), known_action_ids.as_slice()),
+                    unrecognized_action_id_help(euid, &self.schema),
                 ))
             } else {
                 None
@@ -727,6 +723,71 @@ mod test {
         assert_eq!(notes.len(), 1, "{:?}", notes);
     }
 
+    #[test]
+    fn validate_action_id_with_action_type() {
+        let schema_file = json_schema::NamespaceDefinition::new(
+            [],
+            [(
+                "Action::view".into(),
+                json_schema::ActionType {
+                    applies_to: None,
+                    member_of: None,
+                    attributes: None,
+                },
+            )],
+        );
+        let singleton_schema = schema_file.try_into().unwrap();
+
+        let src = r#"permit(principal, action == Action::"view", resource);"#;
+        let policy = parse_policy_or_template(None, src).unwrap();
+        let validate = Validator::new(singleton_schema);
+        let notes: Vec<ValidationError> = validate.validate_action_ids(&policy).collect();
+        expect_err(
+            src,
+            &Report::new(notes.first().unwrap().clone()),
+            &ExpectedErrorMessageBuilder::error(
+                r#"for policy `policy0`, unrecognized action `Action::"view"`"#,
+            )
+            .exactly_one_underline(r#"Action::"view""#)
+            .help(r#"did you intend to include the type in action `Action::"Action::view"`?"#)
+            .build(),
+        );
+        assert_eq!(notes.len(), 1, "{:?}", notes);
+    }
+
+    #[test]
+    fn validate_action_id_with_action_type_namespace() {
+        let schema_src = r#"
+        {
+            "foo::foo::bar::baz": {
+                "entityTypes": {},
+                "actions": {
+                    "Action::view": {}
+                }
+            }
+        }"#;
+
+        let schema_fragment: json_schema::Fragment<RawName> =
+            serde_json::from_str(schema_src).expect("Parse Error");
+        let schema = schema_fragment.try_into().unwrap();
+
+        let src = r#"permit(principal, action == Action::"view", resource);"#;
+        let policy = parse_policy_or_template(None, src).unwrap();
+        let validate = Validator::new(schema);
+        let notes: Vec<ValidationError> = validate.validate_action_ids(&policy).collect();
+        expect_err(
+            src,
+            &Report::new(notes.first().unwrap().clone()),
+            &ExpectedErrorMessageBuilder::error(
+                r#"for policy `policy0`, unrecognized action `Action::"view"`"#,
+            )
+            .exactly_one_underline(r#"Action::"view""#)
+            .help(r#"did you intend to include the type in action `foo::foo::bar::baz::Action::"Action::view"`?"#)
+            .build(),
+        );
+        assert_eq!(notes.len(), 1, "{:?}", notes);
+    }
+
     #[test]
     fn validate_namespaced_action_id_in_schema() {
         let descriptors = json_schema::Fragment::from_json_str(

cedar-policy/CHANGELOG.md

@@ -23,6 +23,11 @@ Cedar Language Version: TBD
   Cedar request. To use this API you must enable the `entity-manifest` feature flag.
 - Added public APIs to get language and SDK version numbers (#1219).
 
+### Changed
+
+- The validator provides a more specific hint when an action ID cannot be found
+  and the same action ID with `Action::` has been defined.
+
 ### Fixed
 
 - The formatter will now consistently add a trailing newline. (resolving #1217)