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

Improve error messages for request validation errors #1357

Merged
merged 4 commits into from
Dec 11, 2024
+359 −54
Merged

Improve error messages for request validation errors #1357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa40504
improve some request-validation error messages
cdisselkoen Dec 5, 2024
2e4b897
improve help text
cdisselkoen Dec 5, 2024
5294a33
don't print large contexts in error message
cdisselkoen Dec 10, 2024
519e207
bounded display for Value
cdisselkoen Dec 10, 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
16 changes: 13 additions & 3 deletions cedar-policy-core/src/ast/request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,10 +427,21 @@ impl Context {
.from_json_file(json)
}

/// Get the number of keys in this `Context`.
pub fn num_keys(&self) -> usize {
match self {
Context::Value(record) => record.len(),
Context::RestrictedResidual(record) => record.len(),
}
}

/// Private helper function to implement `into_iter()` for `Context`.
/// Gets an iterator over the (key, value) pairs in the `Context`, cloning
/// only if necessary.
fn into_values(self) -> Box<dyn Iterator<Item = (SmolStr, RestrictedExpr)>> {
///
/// Note that some error messages rely on this function returning keys in
/// sorted order, or else the error message will not be fully deterministic.
fn into_pairs(self) -> Box<dyn Iterator<Item = (SmolStr, RestrictedExpr)>> {
match self {
Context::Value(record) => Box::new(
Arc::unwrap_or_clone(record)
Expand Down Expand Up @@ -507,11 +518,10 @@ mod iter {

impl IntoIterator for Context {
type Item = (SmolStr, RestrictedExpr);

type IntoIter = iter::IntoIter;

fn into_iter(self) -> Self::IntoIter {
iter::IntoIter(self.into_values())
iter::IntoIter(self.into_pairs())
}
}

Expand Down
149 changes: 108 additions & 41 deletions cedar-policy-core/src/ast/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use crate::ast::*;
use crate::parser::Loc;
use std::collections::{BTreeMap, BTreeSet, HashSet};
use std::str::FromStr;
use std::sync::Arc;

use itertools::Itertools;
Expand Down Expand Up @@ -157,6 +158,19 @@ impl Value {
pub fn eq_and_same_source_loc(&self, other: &Self) -> bool {
self == other && self.source_loc() == other.source_loc()
}

/// Alternate `Display` impl, that truncates large sets/records (including recursively).
///
/// `n`: the maximum number of set elements, or record key-value pairs, that
/// will be shown before eliding the rest with `..`.
/// `None` means no bound.
pub fn bounded_display(
&self,
f: &mut std::fmt::Formatter<'_>,
n: Option<usize>,
) -> std::fmt::Result {
self.value.bounded_display(f, n)
}
}

impl ValueKind {
Expand Down Expand Up @@ -204,6 +218,99 @@ impl ValueKind {
_ => None,
}
}

/// Alternate `Display` impl, that truncates large sets/records (including recursively).
///
/// `n`: the maximum number of set elements, or record key-value pairs, that
/// will be shown before eliding the rest with `..`.
/// `None` means no bound.
pub fn bounded_display(
&self,
f: &mut std::fmt::Formatter<'_>,
n: Option<usize>,
) -> std::fmt::Result {
match self {
Self::Lit(lit) => write!(f, "{lit}"),
Self::Set(Set {
fast,
authoritative,
}) => {
write!(f, "[")?;
let truncated = n.map(|n| authoritative.len() > n).unwrap_or(false);
if let Some(rc) = fast {
// sort the elements, because we want the Display output to be
// deterministic, particularly for tests which check equality
// of error messages
let elements = match n {
Some(n) => Box::new(rc.as_ref().iter().sorted_unstable().take(n))
as Box<dyn Iterator<Item = &Literal>>,
None => Box::new(rc.as_ref().iter().sorted_unstable())
as Box<dyn Iterator<Item = &Literal>>,
};
for (i, item) in elements.enumerate() {
write!(f, "{item}")?;
if i < authoritative.len() - 1 {
write!(f, ", ")?;
}
}
} else {
// don't need to sort the elements in this case because BTreeSet iterates
// in a deterministic order already
let elements = match n {
Some(n) => Box::new(authoritative.as_ref().iter().take(n))
as Box<dyn Iterator<Item = &Value>>,
None => Box::new(authoritative.as_ref().iter())
as Box<dyn Iterator<Item = &Value>>,
};
for (i, item) in elements.enumerate() {
item.bounded_display(f, n)?;
if i < authoritative.len() - 1 {
write!(f, ", ")?;
}
}
}
if truncated {
write!(f, ".. ")?;
}
write!(f, "]")?;
Ok(())
}
Self::Record(record) => {
write!(f, "{{")?;
let truncated = n.map(|n| record.len() > n).unwrap_or(false);
// no need to sort the elements because BTreeMap iterates in a
// deterministic order already
let elements = match n {
Some(n) => Box::new(record.as_ref().iter().take(n))
as Box<dyn Iterator<Item = (&SmolStr, &Value)>>,
None => Box::new(record.as_ref().iter())
as Box<dyn Iterator<Item = (&SmolStr, &Value)>>,
};
for (i, (k, v)) in elements.enumerate() {
match UnreservedId::from_str(k) {
Ok(k) => {
// we can omit the quotes around the key, it's a valid identifier and not a reserved keyword
write!(f, "{k}: ")?;
}
Err(_) => {
// put quotes around the key
write!(f, "\"{k}\": ")?;
}
}
v.bounded_display(f, n)?;
if i < record.len() - 1 {
write!(f, ", ")?;
}
}
if truncated {
write!(f, ".. ")?;
}
write!(f, "}}")?;
Ok(())
}
Self::ExtensionValue(ev) => write!(f, "{}", RestrictedExpr::from(ev.as_ref().clone())),
}
}
}

#[derive(Debug, Error)]
Expand Down Expand Up @@ -478,47 +585,7 @@ impl std::fmt::Display for Value {

impl std::fmt::Display for ValueKind {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::Lit(lit) => write!(f, "{}", lit),
Self::Set(Set {
fast,
authoritative,
}) => {
match authoritative.len() {
0 => write!(f, "[]"),
n @ 1..=5 => {
write!(f, "[")?;
if let Some(rc) = fast {
// sort the elements, because we want the Display output to be
// deterministic, particularly for tests which check equality
// of error messages
for (i, item) in rc.as_ref().iter().sorted_unstable().enumerate() {
write!(f, "{item}")?;
if i < n - 1 {
write!(f, ", ")?;
}
}
} else {
// don't need to sort the elements in this case because BTreeSet iterates
// in a deterministic order already
for (i, item) in authoritative.as_ref().iter().enumerate() {
write!(f, "{item}")?;
if i < n - 1 {
write!(f, ", ")?;
}
}
}
write!(f, "]")?;
Ok(())
}
n => write!(f, "<set with {} elements>", n),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this case for the same reason we applied #887? Similarly should consider adding a case like this for the record branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A goal for the policy pretty-printer is to produce valid, parseable syntax. Is a similar goal reasonable here, that the value pretty-printer should produce syntax that is parseable as a Cedar expression?

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fair point.

But, for request validation errors in particular, I don't think it's a reasonable goal. It isn't necessary to have InvalidContextError send back the entire context when the context is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the request-validation error message to truncate after 5 key-value pairs in the Context record. Left the Display impl for Value as printing valid Cedar syntax. Is that a good compromise between the competing goals here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it doesn't guard against large sets/records located recursively inside the Context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to new bounded_display() method on Value and ValueKind, which shares code with their Display impls. Addresses bounding the display of large sets and records, including recursively; doesn't address large literals or extension values (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good enough for now. To me it feels more likely that the context record will be very large in the normal operation of an application than very large individual strings. If this turns about to be wrong we can truncate these as well.

}
}
Self::Record(record) => {
write!(f, "<first-class record with {} fields>", record.len())
}
Self::ExtensionValue(ev) => write!(f, "{}", RestrictedExpr::from(ev.as_ref().clone())),
}
self.bounded_display(f, None)
}
}

Expand Down
Loading
Loading