Skip to content

Commit

Permalink
Delete SourceLocation struct (#906)
Browse files Browse the repository at this point in the history 
Signed-off-by: John Kastner <[email protected]>
  • Loading branch information
@john-h-kastner-aws
john-h-kastner-aws authored May 23, 2024
1 parent 766b550 commit 1bedb6c
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 147 deletions.
10 changes: 5 additions & 5 deletions cedar-policy-validator/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,13 @@ mod test {
assert!(!result.validation_passed());
assert!(result
.validation_errors()
.any(|x| x.error_kind() == principal_err.error_kind()));
.any(|x| x.kind() == principal_err.kind()));
assert!(result
.validation_errors()
.any(|x| x.error_kind() == resource_err.error_kind()));
.any(|x| x.kind() == resource_err.kind()));
assert!(result
.validation_errors()
.any(|x| x.error_kind() == action_err.error_kind()));
.any(|x| x.kind() == action_err.kind()));

Ok(())
}
Expand Down Expand Up @@ -447,7 +447,7 @@ mod test {
);
assert!(result
.validation_errors()
.any(|x| x.error_kind() == invalid_action_err.error_kind()));
.any(|x| x.kind() == invalid_action_err.kind()));

Ok(())
}
Expand Down Expand Up @@ -490,7 +490,7 @@ mod test {
assert_eq!(
result
.validation_errors()
.map(|err| err.error_kind())
.map(|err| err.kind())
.collect::<Vec<_>>(),
vec![
&TypeError::expected_type(
Expand Down
14 changes: 7 additions & 7 deletions cedar-policy-validator/src/rbac.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ mod test {
let validate = Validator::new(schema);
let notes: Vec<ValidationErrorKind> = validate
.validate_action_ids(&policy)
.map(|e| e.into_location_and_error_kind().1)
.map(|e| e.kind)
.collect();
assert_eq!(notes, vec![], "Did not expect any invalid action.");
Ok(())
Expand Down Expand Up @@ -894,7 +894,7 @@ mod test {
let validate = Validator::new(schema);
let notes: Vec<ValidationErrorKind> = validate
.validate_entity_types(&policy)
.map(|e| e.into_location_and_error_kind().1)
.map(|e| e.kind)
.collect();

assert_eq!(notes, vec![], "Did not expect any invalid action.");
Expand Down Expand Up @@ -1128,7 +1128,7 @@ mod test {
validator
.validate_policy(policy, ValidationMode::default())
.0
.map(|e| { e.into_location_and_error_kind().1 })
.map(|e| { e.kind })
.collect::<Vec<ValidationErrorKind>>(),
expected.into_iter().map(|e| e.kind).collect::<Vec<_>>(),
"Unexpected validation errors."
Expand All @@ -1141,7 +1141,7 @@ mod test {
validator
.validate_policy(policy, ValidationMode::default())
.1
.map(|w| { w.to_kind_and_location().1 })
.map(|w| { w.kind })
.collect::<Vec<ValidationWarningKind>>(),
vec![ValidationWarningKind::ImpossiblePolicy],
"Unexpected validation warnings."
Expand Down Expand Up @@ -1519,7 +1519,7 @@ mod test {
);
let notes: Vec<ValidationErrorKind> = validate
.validate_entity_types(&policy)
.map(|e| e.into_location_and_error_kind().1)
.map(|e| e.kind)
.collect();
assert_eq!(1, notes.len());
assert_matches!(notes.first(),
Expand All @@ -1541,7 +1541,7 @@ mod test {
);
let notes: Vec<ValidationErrorKind> = validate
.validate_entity_types(&policy)
.map(|e| e.into_location_and_error_kind().1)
.map(|e| e.kind)
.collect();
assert_eq!(1, notes.len());
assert_matches!(notes.first(),
Expand Down Expand Up @@ -1573,7 +1573,7 @@ mod test {
);
let notes: Vec<ValidationErrorKind> = validate
.validate_entity_types(&policy)
.map(|e| e.into_location_and_error_kind().1)
.map(|e| e.kind)
.collect();

println!("{:?}", notes);
Expand Down
32 changes: 9 additions & 23 deletions cedar-policy-validator/src/str_checks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use cedar_policy_core::ast::{Pattern, Template};

use crate::expr_iterator::expr_text;
use crate::expr_iterator::TextKind;
use crate::{SourceLocation, ValidationWarning, ValidationWarningKind};
use crate::{ValidationWarning, ValidationWarningKind};
use unicode_security::GeneralSecurityProfile;
use unicode_security::MixedScript;

Expand All @@ -43,8 +43,9 @@ pub fn confusable_string_checks<'a>(

if let Some(kind) = warning {
warnings.push(ValidationWarning {
location: SourceLocation::new(policy.id().clone(), loc.cloned()),
kind,
policy_id: policy.id().clone(),
source_loc: loc.cloned(),
})
}
}
Expand Down Expand Up @@ -139,7 +140,6 @@ mod test {
assert_eq!(warnings.len(), 1);
let warning = &warnings[0];
let kind = warning.kind().clone();
let location = warning.location();
assert_eq!(
kind,
ValidationWarningKind::MixedScriptIdentifier(r#"say_һello"#.to_string())
Expand All @@ -148,10 +148,8 @@ mod test {
format!("{warning}"),
"for policy `test`, identifier `say_һello` contains mixed scripts"
);
assert_eq!(
location,
&SourceLocation::new(PolicyID::from_string("test"), None)
);
assert_eq!(warning.loc(), None);
assert_eq!(warning.policy_id(), &PolicyID::from_string("test"));
}

#[test]
Expand Down Expand Up @@ -184,7 +182,6 @@ mod test {
assert_eq!(warnings.len(), 1);
let warning = &warnings[0];
let kind = warning.kind().clone();
let location = warning.location();
assert_eq!(
kind,
ValidationWarningKind::MixedScriptString(r#"*_һello"#.to_string())
Expand All @@ -193,13 +190,8 @@ mod test {
format!("{warning}"),
"for policy `test`, string `\"*_һello\"` contains mixed scripts"
);
assert_eq!(
location,
&SourceLocation::new(
PolicyID::from_string("test"),
Some(Loc::new(64..94, Arc::from(src)))
),
);
assert_eq!(warning.loc(), Some(&Loc::new(64..94, Arc::from(src))));
assert_eq!(warning.policy_id(), &PolicyID::from_string("test"));
}

#[test]
Expand All @@ -218,18 +210,12 @@ mod test {
assert_eq!(warnings.len(), 1);
let warning = &warnings[0];
let kind = warning.kind().clone();
let location = warning.location();
assert_eq!(
kind,
ValidationWarningKind::BidiCharsInString(r#"user‮ ⁦&& principal.is_admin⁩ ⁦"#.to_string())
);
assert_eq!(format!("{warning}"), "for policy `test`, string `\"user‮ ⁦&& principal.is_admin⁩ ⁦\"` contains BIDI control characters");
assert_eq!(
location,
&SourceLocation::new(
PolicyID::from_string("test"),
Some(Loc::new(90..131, Arc::from(src)))
)
);
assert_eq!(warning.loc(), Some(&Loc::new(90..131, Arc::from(src))));
assert_eq!(warning.policy_id(), &PolicyID::from_string("test"));
}
}
90 changes: 38 additions & 52 deletions cedar-policy-validator/src/validation_result.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,48 +77,54 @@ impl ValidationResult {
/// and provides details specific to that kind of problem. The error also records
/// where the problem was encountered.
#[derive(Clone, Debug, Error, Eq, PartialEq)]
#[error("for policy `{}`, {}", location.policy_id(), kind)]
#[error("for policy `{policy_id}`, {kind}")]
pub struct ValidationError {
location: SourceLocation,
kind: ValidationErrorKind,
pub(crate) policy_id: PolicyID,
pub(crate) source_loc: Option<Loc>,
pub(crate) kind: ValidationErrorKind,
}

impl ValidationError {
pub(crate) fn with_policy_id(id: PolicyID, source_loc: Option<Loc>, err: TypeError) -> Self {
pub(crate) fn with_policy_id(
policy_id: PolicyID,
source_loc: Option<Loc>,
err: TypeError,
) -> Self {
let (kind, error_loc) = err.kind_and_location();
let source_loc = error_loc.or(source_loc);
Self {
policy_id,
source_loc,
kind,
location: SourceLocation::new(id, source_loc),
}
}

/// Deconstruct this into its component source location and error kind.
pub fn into_location_and_error_kind(self) -> (SourceLocation, ValidationErrorKind) {
(self.location, self.kind)
}

/// Extract details about the exact issue detected by the validator.
pub fn error_kind(&self) -> &ValidationErrorKind {
pub fn kind(&self) -> &ValidationErrorKind {
&self.kind
}

/// Extract the policy id of the policy where the validator found the issue.
pub fn policy_id(&self) -> &PolicyID {
&self.policy_id
}

/// Extract the location where the validator found the issue.
pub fn location(&self) -> &SourceLocation {
&self.location
pub fn loc(&self) -> Option<&Loc> {
self.source_loc.as_ref()
}
}

// custom impl of `Diagnostic`: source location and source code are from
// .location, everything else forwarded to .error_kind
impl Diagnostic for ValidationError {
fn labels(&self) -> Option<Box<dyn Iterator<Item = miette::LabeledSpan> + '_>> {
let label = miette::LabeledSpan::underline(self.location.source_loc.as_ref()?.span);
let label = miette::LabeledSpan::underline(self.source_loc.as_ref()?.span);
Some(Box::new(std::iter::once(label)))
}

fn source_code(&self) -> Option<&dyn miette::SourceCode> {
Some(&self.location.source_loc.as_ref()?.src)
Some(&self.source_loc.as_ref()?.src)
}

fn code(&self) -> Option<Box<dyn std::fmt::Display + '_>> {
Expand Down Expand Up @@ -146,74 +152,54 @@ impl Diagnostic for ValidationError {
}
}

/// Represents a location in Cedar policy source.
#[derive(Debug, Clone, Hash, Eq, PartialEq)]
pub struct SourceLocation {
policy_id: PolicyID,
source_loc: Option<Loc>,
}

impl SourceLocation {
pub(crate) fn new(policy_id: PolicyID, source_loc: Option<Loc>) -> Self {
Self {
policy_id,
source_loc,
}
}

/// Get the `PolicyId` for the policy at this source location.
pub fn policy_id(&self) -> &PolicyID {
&self.policy_id
}

pub fn source_loc(&self) -> Option<&Loc> {
self.source_loc.as_ref()
}
}

/// The structure for validation warnings.
#[derive(Hash, Eq, PartialEq, Error, Debug, Clone)]
#[error("for policy `{}`, {}", location.policy_id(), kind)]
#[error("for policy `{policy_id}`, {kind}")]
pub struct ValidationWarning {
pub(crate) location: SourceLocation,
pub(crate) policy_id: PolicyID,
pub(crate) source_loc: Option<Loc>,
pub(crate) kind: ValidationWarningKind,
}

impl ValidationWarning {
pub(crate) fn with_policy_id(
id: PolicyID,
policy_id: PolicyID,
source_loc: Option<Loc>,
kind: ValidationWarningKind,
) -> Self {
Self {
kind,
location: SourceLocation::new(id, source_loc),
policy_id,
source_loc,
}
}

pub fn location(&self) -> &SourceLocation {
&self.location
/// Extract the policy id of the policy where the validator found the issue.
pub fn policy_id(&self) -> &PolicyID {
&self.policy_id
}

pub fn kind(&self) -> &ValidationWarningKind {
&self.kind
/// Extract the location where the validator found the issue.
pub fn loc(&self) -> Option<&Loc> {
self.source_loc.as_ref()
}

pub fn to_kind_and_location(self) -> (SourceLocation, ValidationWarningKind) {
(self.location, self.kind)
/// Extract details about the exact issue detected by the validator.
pub fn kind(&self) -> &ValidationWarningKind {
&self.kind
}
}

// custom impl of `Diagnostic`: source location and source code are from
// .location, everything else forwarded to .kind
impl Diagnostic for ValidationWarning {
fn labels(&self) -> Option<Box<dyn Iterator<Item = miette::LabeledSpan> + '_>> {
let label = miette::LabeledSpan::underline(self.location.source_loc.as_ref()?.span);
let label = miette::LabeledSpan::underline(self.source_loc.as_ref()?.span);
Some(Box::new(std::iter::once(label)))
}

fn source_code(&self) -> Option<&dyn miette::SourceCode> {
Some(&self.location.source_loc.as_ref()?.src)
Some(&self.source_loc.as_ref()?.src)
}

fn code(&self) -> Option<Box<dyn std::fmt::Display + '_>> {
Expand Down
Loading

0 comments on commit 1bedb6c

Please sign in to comment.