Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add APIs to get schema annotations #1389

Merged
merged 9 commits into from
Jan 13, 2025
Merged

Add APIs to get schema annotations #1389

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bb06d89
Add APIs to get schema annotations
shaobo-he-aws Dec 20, 2024
4708c47
added tests
shaobo-he-aws Dec 21, 2024
b3a02d5
Update cedar-policy/src/api.rs
shaobo-he-aws Dec 30, 2024
5eb2a23
renaming
shaobo-he-aws Dec 31, 2024
1e2e136
Apply suggestions from code review
shaobo-he-aws Jan 2, 2025
649ef49
Apply suggestions from code review
shaobo-he-aws Jan 8, 2025
a418e4a
Merge remote-tracking branch 'origin/main' into feature/shaobo/annota…
shaobo-he-aws Jan 8, 2025
eb70160
Merge remote-tracking branch 'origin/main' into feature/shaobo/annota…
shaobo-he-aws Jan 8, 2025
0010302
added APIs to get individual annotation
shaobo-he-aws Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions cedar-policy/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use cedar_policy_validator::entity_manifest;
pub use cedar_policy_validator::entity_manifest::{
AccessTrie, EntityManifest, EntityRoot, Fields, RootAccessTrie,
};
use cedar_policy_validator::json_schema;
use cedar_policy_validator::typecheck::{PolicyCheck, Typechecker};
pub use id::*;

Expand Down Expand Up @@ -1361,7 +1362,77 @@ pub struct SchemaFragment {
lossless: cedar_policy_validator::json_schema::Fragment<cedar_policy_validator::RawName>,
}

fn annotations_to_pairs(annotations: &est::Annotations) -> impl Iterator<Item = (&str, &str)> {
annotations
.0
.iter()
.map(|(key, value)| (key.as_ref(), value.as_ref().map_or("", |a| a.as_ref())))
}

impl SchemaFragment {
/// Get annotations of a non-empty namespace.
/// We do not allow namespace-level annotations on the empty namespace
/// Returns `None` if `namespace` is not found in the [`SchemaFragment`]
pub fn namespace_annotations(
&self,
namespace: EntityNamespace,
) -> Option<impl Iterator<Item = (&str, &str)>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having all of these APIs return Iterator<Item = (&str, &str)> is good, but I wonder if we need an additional API to get the value of an annotation with a user-specified key. For instance if I want to specifically look up the value of the foo annotation on a particular namespace. As is, I'd be forced to iterate through the entire iterator looking for the key foo. We could solve this either by adding a separate API namespace_annotation(&self, namespace, key) or perhaps by returning a map structure (HashMap or BTreeMap) instead of an iterator in this API, so that users could do their own lookups.

Same for the other new annotation-getting APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think that having an iterator is a generic. That is, users can collect them into a HashMap or BTreeMap given their preferences (e.g., performance vs order). I also think it may be a good idea to let user collect the iterator and get the value by key, instead of having APIs. Maybe I'm too paranoid about breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm concerned that having the user collect the iterator in order to get the value by key is

  1. a little complicated, compared to having a direct method for it -- so, creates a suboptimal user experience
  2. potentially poor performance, as getting any value by key at all requires collecting the entire iterator

If we're worried about breaking changes, we don't have to return a map structure, we can just implement our own getter, something like:

pub fn namespace_annotation(
    &self,
    namespace: EntityNamespace,
    annotation_key: &str,
) -> Option<&str> { ... }

that gets a particular annotation value given a key. I don't think adding a method like this is overly committing to any particular implementation details

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I can do it.

self.lossless
.0
.get(&Some(namespace.0))
.map(|ns_def| annotations_to_pairs(&ns_def.annotations))
}

/// Get annotations of a common type declaration
/// Returns `None` if `namespace` is not found in the [`SchemaFragment`] or
/// `ty` is not a valid common type ID or `ty` is not found in the
/// corresponding namespace definition
Comment on lines +1457 to +1459
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overloading the None case here isn't great. Maybe we could publicly expose an Id data type so that error can be reported sooner? Though, using that in other places in the API would probably be a breaking change, so perhaps it's better to stick with this signature for now

pub fn common_type_annotations(
&self,
namespace: Option<EntityNamespace>,
ty: &str,
) -> Option<impl Iterator<Item = (&str, &str)>> {
let ns_def = self.lossless.0.get(&namespace.map(|n| n.0))?;
let ty = json_schema::CommonTypeId::new(ast::UnreservedId::from_normalized_str(ty).ok()?)
.ok()?;
ns_def
.common_types
.get(&ty)
.map(|ty| annotations_to_pairs(&ty.annotations))
}

/// Get annotations of an entity type declaration
/// Returns `None` if `namespace` is not found in the [`SchemaFragment`] or
/// `ty` is not a valid entity type name or `ty` is not found in the
/// corresponding namespace definition
pub fn entity_type_annotations(
&self,
namespace: Option<EntityNamespace>,
ty: &str,
) -> Option<impl Iterator<Item = (&str, &str)>> {
let ns_def = self.lossless.0.get(&namespace.map(|n| n.0))?;
let ty = ast::UnreservedId::from_normalized_str(ty).ok()?;
ns_def
.entity_types
.get(&ty)
.map(|ty| annotations_to_pairs(&ty.annotations))
}

/// Get annotations of an action declaration
/// Returns `None` if `namespace` is not found in the [`SchemaFragment`] or
/// `id` is not found in the corresponding namespace definition
pub fn action_annotations(
&self,
namespace: Option<EntityNamespace>,
id: EntityId,
) -> Option<impl Iterator<Item = (&str, &str)>> {
let ns_def = self.lossless.0.get(&namespace.map(|n| n.0))?;
ns_def
.actions
.get(id.as_ref())
.map(|a| annotations_to_pairs(&a.annotations))
}

/// Extract namespaces defined in this [`SchemaFragment`].
///
/// `None` indicates the empty namespace.
Expand Down
163 changes: 163 additions & 0 deletions cedar-policy/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6531,3 +6531,166 @@ mod reserved_keywords_in_policies {
}
}
}

mod schema_annotations {
use std::collections::BTreeMap;

use super::SchemaFragment;

#[track_caller]
fn example_schema() -> SchemaFragment {
SchemaFragment::from_cedarschema_str(
r#"
@a("a")
@b
entity A1,A2 {};
@c("c")
@d
type T = Long;
@e("e")
@f
action a1, a2 appliesTo { principal: [A1], resource: [A2] };

@m("m")
@n
namespace N {
@a("a")
@b
entity A1,A2 {};
@c("c")
@d
type T = Long;
@e("e")
@f
action a1, a2 appliesTo { principal: [N::A1], resource: [A2] };
}
"#,
)
.expect("should be a valid schema fragment")
.0
}

#[test]
fn namespace_annotations() {
let schema = example_schema();
let annotations = BTreeMap::from_iter(
schema
.namespace_annotations("N".parse().expect("should be a valid name"))
.expect("should get annotations"),
);
assert_eq!(annotations, BTreeMap::from_iter([("m", "m"), ("n", "")]));
}

#[test]
fn entity_type_annotations() {
let schema = example_schema();
let annotations = BTreeMap::from_iter([("a", "a"), ("b", "")]);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.entity_type_annotations(None, "A1")
.expect("should get annotations")
)
);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.entity_type_annotations(None, "A2")
.expect("should get annotations")
)
);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.entity_type_annotations(
Some("N".parse().expect("should be a valid name")),
"A1"
)
.expect("should get annotations")
)
);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.entity_type_annotations(
Some("N".parse().expect("should be a valid name")),
"A2"
)
.expect("should get annotations")
)
);
}

#[test]
fn common_type_annotations() {
let schema = example_schema();
let annotations = BTreeMap::from_iter([("c", "c"), ("d", "")]);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.common_type_annotations(None, "T")
.expect("should get annotations")
)
);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.common_type_annotations(
Some("N".parse().expect("should be a valid name")),
"T"
)
.expect("should get annotations")
)
);
}

#[test]
fn action_type_annotations() {
let schema = example_schema();
let annotations = BTreeMap::from_iter([("e", "e"), ("f", "")]);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.action_annotations(None, "a1".parse().unwrap(),)
.expect("should get annotations")
)
);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.action_annotations(None, "a2".parse().unwrap(),)
.expect("should get annotations")
)
);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.action_annotations(
Some("N".parse().expect("should be a valid name")),
"a1".parse().unwrap(),
)
.expect("should get annotations")
)
);
assert_eq!(
annotations,
BTreeMap::from_iter(
schema
.action_annotations(
Some("N".parse().expect("should be a valid name")),
"a2".parse().unwrap(),
)
.expect("should get annotations")
)
);
}
}
Loading