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

Implement #745 for api::SchemaError #876

Merged
merged 12 commits into from
May 28, 2024
Merged

Implement #745 for api::SchemaError #876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e50aa54
Implement #745 for `api::SchemaError`
shaobo-he-aws May 15, 2024
ce96e52
review feedback
shaobo-he-aws May 22, 2024
23b6d42
Merge remote-tracking branch 'origin/main' into chore/shaobo/745
shaobo-he-aws May 22, 2024
a7d8f8e
minor fix
shaobo-he-aws May 22, 2024
8183716
hide implementation details
shaobo-he-aws May 22, 2024
3d39049
Merge remote-tracking branch 'origin/main' into chore/shaobo/745
shaobo-he-aws May 24, 2024
7d4182a
review feedback
shaobo-he-aws May 24, 2024
94f93e6
Merge remote-tracking branch 'origin/main' into chore/shaobo/745
shaobo-he-aws May 24, 2024
4d17db9
updates
shaobo-he-aws May 24, 2024
2a2d3f1
new design
shaobo-he-aws May 24, 2024
ad41bb0
Merge remote-tracking branch 'origin/main' into chore/shaobo/745
shaobo-he-aws May 28, 2024
cb5ebae
review feedback
shaobo-he-aws May 28, 2024
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
431 changes: 276 additions & 155 deletions cedar-policy-validator/src/err.rs
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion cedar-policy-validator/src/human_schema/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,8 +1742,9 @@ mod common_type_references {
use cool_asserts::assert_matches;

use crate::{
schema_error::SchemaError,
types::{AttributeType, EntityRecordKind, Type},
SchemaError, SchemaFragment, ValidatorSchema,
SchemaFragment, ValidatorSchema,
};

#[test]
Expand Down
2 changes: 2 additions & 0 deletions cedar-policy-validator/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ mod test {
};
use itertools::Itertools;

use crate::schema_error::Result;

#[test]
fn top_level_validate() -> Result<()> {
let mut set = PolicySet::new();
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy-validator/src/rbac.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ mod test {

use super::*;
use crate::{
err::*,
err::schema_error::*,
schema_file_format::{NamespaceDefinition, *},
validation_errors::{UnrecognizedEntityType, UnspecifiedEntity},
ValidationMode, ValidationWarning, Validator,
Expand Down
93 changes: 54 additions & 39 deletions cedar-policy-validator/src/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ use cedar_policy_core::{
};
use serde::{Deserialize, Serialize};
use serde_with::serde_as;
use smol_str::ToSmolStr;

use super::NamespaceDefinition;
use crate::{
err::schema_error::*,
err::*,
human_schema::SchemaWarning,
types::{Attributes, EntityRecordKind, OpenTag, Type},
Expand Down Expand Up @@ -251,7 +253,7 @@ impl ValidatorSchema {
match type_defs.entry(name) {
Entry::Vacant(v) => v.insert(ty),
Entry::Occupied(o) => {
return Err(SchemaError::DuplicateCommonType(o.key().to_string()));
return Err(DuplicateCommonTypeError(o.key().clone()).into());
}
};
}
Expand All @@ -260,7 +262,7 @@ impl ValidatorSchema {
match entity_type_fragments.entry(name) {
Entry::Vacant(v) => v.insert(entity_type),
Entry::Occupied(o) => {
return Err(SchemaError::DuplicateEntityType(o.key().to_string()))
return Err(DuplicateEntityTypeError(o.key().clone()).into())
}
};
}
Expand All @@ -269,7 +271,7 @@ impl ValidatorSchema {
match action_fragments.entry(action_euid) {
Entry::Vacant(v) => v.insert(action),
Entry::Occupied(o) => {
return Err(SchemaError::DuplicateAction(o.key().to_string()))
return Err(DuplicateActionError(o.key().to_smolstr()).into())
}
};
}
Expand Down Expand Up @@ -306,9 +308,9 @@ impl ValidatorSchema {
let (attributes, open_attributes) = Self::record_attributes_or_none(
entity_type.attributes.resolve_type_defs(&type_defs)?,
)
.ok_or(SchemaError::ContextOrShapeNotRecord(
.ok_or(SchemaError::from(ContextOrShapeNotRecordError(
ContextOrShape::EntityTypeShape(name.clone()),
))?;
)))?;
Ok((
name.clone(),
ValidatorEntityType {
Expand Down Expand Up @@ -336,9 +338,9 @@ impl ValidatorSchema {
let descendants = action_children.remove(&name).unwrap_or_default();
let (context, open_context_attributes) =
Self::record_attributes_or_none(action.context.resolve_type_defs(&type_defs)?)
.ok_or(SchemaError::ContextOrShapeNotRecord(
.ok_or(SchemaError::from(ContextOrShapeNotRecordError(
ContextOrShape::ActionContext(name.clone()),
))?;
)))?;
Ok((
name.clone(),
ValidatorActionId {
Expand All @@ -358,7 +360,8 @@ impl ValidatorSchema {

// We constructed entity types and actions with child maps, but we need
// transitively closed descendants.
compute_tc(&mut entity_types, false)?;
compute_tc(&mut entity_types, false)
.map_err(|e| EntityTypeTransitiveClosureError::from(Box::new(e)))?;
// Pass `true` here so that we also check that the action hierarchy does
// not contain cycles.
compute_tc(&mut action_ids, true)?;
Expand Down Expand Up @@ -398,7 +401,6 @@ impl ValidatorSchema {
// any undeclared entity types which appeared in a `memberOf` list.
let mut undeclared_e = undeclared_parent_entities
.into_iter()
.map(|n| n.to_string())
.collect::<HashSet<_>>();
// Looking at entity types, we need to check entity references in
// attribute types. We already know that all elements of the
Expand All @@ -418,7 +420,7 @@ impl ValidatorSchema {
// Undeclared actions in a `memberOf` list.
let undeclared_a = undeclared_parent_actions
.into_iter()
.map(|n| n.to_string())
.map(|n| n.to_smolstr())
.collect::<HashSet<_>>();
// For actions, we check entity references in the context attribute
// types and `appliesTo` lists. See the `entity_types` loop for why the
Expand All @@ -430,7 +432,7 @@ impl ValidatorSchema {
match p_entity {
EntityType::Specified(p_entity) => {
if !entity_types.contains_key(&p_entity) {
undeclared_e.insert(p_entity.to_string());
undeclared_e.insert(p_entity.clone());
}
}
EntityType::Unspecified => (),
Expand All @@ -441,18 +443,18 @@ impl ValidatorSchema {
match r_entity {
EntityType::Specified(r_entity) => {
if !entity_types.contains_key(&r_entity) {
undeclared_e.insert(r_entity.to_string());
undeclared_e.insert(r_entity.clone());
}
}
EntityType::Unspecified => (),
}
}
}
if !undeclared_e.is_empty() {
return Err(SchemaError::UndeclaredEntityTypes(undeclared_e));
return Err(UndeclaredEntityTypesError(undeclared_e).into());
}
if !undeclared_a.is_empty() {
return Err(SchemaError::UndeclaredActions(undeclared_a));
return Err(UndeclaredActionsError(undeclared_a).into());
}

Ok(())
Expand All @@ -474,13 +476,13 @@ impl ValidatorSchema {
fn check_undeclared_in_type(
ty: &Type,
entity_types: &HashMap<Name, ValidatorEntityType>,
undeclared_types: &mut HashSet<String>,
undeclared_types: &mut HashSet<Name>,
) {
match ty {
Type::EntityOrRecord(EntityRecordKind::Entity(lub)) => {
for name in lub.iter() {
if !entity_types.contains_key(name) {
undeclared_types.insert(name.to_string());
undeclared_types.insert(name.clone());
}
}
}
Expand Down Expand Up @@ -793,9 +795,9 @@ impl<'a> CommonTypeResolver<'a> {
match ty {
SchemaType::TypeDef { type_name } => resolve_table
.get(&type_name)
.ok_or(SchemaError::UndeclaredCommonTypes(HashSet::from_iter(
std::iter::once(type_name.to_string()),
)))
.ok_or(SchemaError::UndeclaredCommonTypes(
UndeclaredCommonTypesError(HashSet::from_iter(std::iter::once(type_name))),
))
.cloned(),
SchemaType::Type(SchemaTypeVariant::Set { element }) => {
Ok(SchemaType::Type(SchemaTypeVariant::Set {
Expand Down Expand Up @@ -828,9 +830,9 @@ impl<'a> CommonTypeResolver<'a> {

// Resolve common type references
fn resolve(&self, extensions: Extensions) -> Result<HashMap<Name, Type>> {
let sorted_names = self
.topo_sort()
.map_err(SchemaError::CycleInCommonTypeReferences)?;
let sorted_names = self.topo_sort().map_err(|n| {
SchemaError::CycleInCommonTypeReferences(CycleInCommonTypeReferencesError(n))
})?;

let mut resolve_table = HashMap::new();
let mut tys = HashMap::new();
Expand Down Expand Up @@ -1016,7 +1018,7 @@ mod test {
let schema: Result<ValidatorSchema> = schema_file.try_into();
match schema {
Ok(_) => panic!("from_schema_file should have failed"),
Err(SchemaError::UndeclaredEntityTypes(v)) => {
Err(SchemaError::UndeclaredEntityTypes(UndeclaredEntityTypesError(v))) => {
assert_eq!(v.len(), 3)
}
_ => panic!("Unexpected error from from_schema_file"),
Expand All @@ -1039,8 +1041,8 @@ mod test {
let schema: Result<ValidatorSchema> = schema_file.try_into();
match schema {
Ok(_) => panic!("try_into should have failed"),
Err(SchemaError::UndeclaredEntityTypes(v)) => {
assert_eq!(v, HashSet::from(["Bar::Group".to_string()]))
Err(SchemaError::UndeclaredEntityTypes(UndeclaredEntityTypesError(v))) => {
assert_eq!(v, HashSet::from(["Bar::Group".parse().unwrap()]))
}
_ => panic!("Unexpected error from try_into"),
}
Expand All @@ -1064,10 +1066,10 @@ mod test {
let schema: Result<ValidatorSchema> = schema_file.try_into();
match schema {
Ok(_) => panic!("try_into should have failed"),
Err(SchemaError::UndeclaredEntityTypes(v)) => {
Err(SchemaError::UndeclaredEntityTypes(UndeclaredEntityTypesError(v))) => {
assert_eq!(
v,
HashSet::from(["Bar::Photo".to_string(), "Bar::User".to_string()])
HashSet::from(["Bar::Photo".parse().unwrap(), "Bar::User".parse().unwrap()])
)
}
_ => panic!("Unexpected error from try_into"),
Expand Down Expand Up @@ -1104,7 +1106,9 @@ mod test {
let schema: Result<ValidatorSchema> = schema_file.try_into();
match schema {
Ok(_) => panic!("from_schema_file should have failed"),
Err(SchemaError::UndeclaredActions(v)) => assert_eq!(v.len(), 1),
Err(SchemaError::UndeclaredActions(UndeclaredActionsError(v))) => {
assert_eq!(v.len(), 1)
}
_ => panic!("Unexpected error from from_schema_file"),
}
}
Expand All @@ -1126,7 +1130,7 @@ mod test {
let schema: Result<ValidatorSchema> = schema_file.try_into();
assert_matches!(
schema,
Err(SchemaError::CycleInActionHierarchy(euid)) => {
Err(SchemaError::CycleInActionHierarchy(CycleInActionHierarchyError(euid))) => {
assert_eq!(euid, r#"Action::"view_photo""#.parse().unwrap());
}
)
Expand Down Expand Up @@ -1266,8 +1270,8 @@ mod test {

let schema: Result<ValidatorSchema> = schema_json.try_into();
match schema {
Err(SchemaError::UndeclaredEntityTypes(tys)) => {
assert_eq!(tys, HashSet::from(["C::D::Foo".to_string()]))
Err(SchemaError::UndeclaredEntityTypes(UndeclaredEntityTypesError(v))) => {
assert_eq!(v, HashSet::from(["C::D::Foo".parse().unwrap()]))
}
_ => panic!("Schema construction should have failed due to undeclared entity type."),
}
Expand Down Expand Up @@ -1325,7 +1329,10 @@ mod test {
.expect("Expected valid schema");

let schema: Result<ValidatorSchema> = schema_json.try_into();
assert!(matches!(schema, Err(SchemaError::ActionEntityTypeDeclared)));
assert!(matches!(
schema,
Err(SchemaError::ActionEntityTypeDeclared(_))
));
}

#[test]
Expand Down Expand Up @@ -1372,7 +1379,9 @@ mod test {
Extensions::all_available(),
);
match schema {
Err(SchemaError::UnsupportedFeature(UnsupportedFeature::ActionAttributes(actions))) => {
Err(SchemaError::UnsupportedFeature(UnsupportedFeatureError(
UnsupportedFeature::ActionAttributes(actions),
))) => {
assert_eq!(
actions.into_iter().collect::<HashSet<_>>(),
HashSet::from([
Expand Down Expand Up @@ -1833,7 +1842,11 @@ mod test {
);

match schema {
Err(SchemaError::DuplicateCommonType(s)) if s.contains("A::MyLong") => (),
Err(SchemaError::DuplicateCommonType(DuplicateCommonTypeError(s)))
if s == "A::MyLong".parse().unwrap() =>
{
()
}
_ => panic!("should have errored because schema fragments have duplicate types"),
};
}
Expand Down Expand Up @@ -2189,8 +2202,8 @@ mod test {
}
);
let schema = ValidatorSchema::from_json_value(src, Extensions::all_available());
assert_matches!(schema, Err(SchemaError::UndeclaredCommonTypes(types)) =>
assert_eq!(types, HashSet::from(["Demo::id".to_string()])));
assert_matches!(schema, Err(SchemaError::UndeclaredCommonTypes(UndeclaredCommonTypesError(types))) =>
assert_eq!(types, HashSet::from(["Demo::id".parse().unwrap()])));
}

#[test]
Expand Down Expand Up @@ -2223,8 +2236,8 @@ mod test {
}
);
let schema = ValidatorSchema::from_json_value(src, Extensions::all_available());
assert_matches!(schema, Err(SchemaError::UndeclaredCommonTypes(types)) =>
assert_eq!(types, HashSet::from(["Demo::id".to_string()])));
assert_matches!(schema, Err(SchemaError::UndeclaredCommonTypes(UndeclaredCommonTypesError(types))) =>
assert_eq!(types, HashSet::from(["Demo::id".parse().unwrap()])));
}

#[test]
Expand Down Expand Up @@ -2326,7 +2339,9 @@ mod test_resolver {
use cool_asserts::assert_matches;

use super::CommonTypeResolver;
use crate::{types::Type, SchemaError, SchemaFragment, ValidatorSchemaFragment};
use crate::{
err::schema_error::SchemaError, types::Type, SchemaFragment, ValidatorSchemaFragment,
};

fn resolve(schema: SchemaFragment) -> Result<HashMap<Name, Type>, SchemaError> {
let schema: ValidatorSchemaFragment = schema.try_into().unwrap();
Expand Down
Loading
Loading