Improve error messages for request validation errors #1357
Conversation
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
cedar-policy-core/src/ast/value.rs
Outdated
| write!(f, "<first-class record with {} fields>", record.len()) | ||
| write!(f, "{{")?; | ||
| for (i, (k, v)) in record.as_ref().iter().enumerate() { |
There was a problem hiding this comment.
Nit: This looks nicer in the general case, but it's shown as `{}` in the case with 0 fields. In that case, I'm not sure the help message is so useful (might take a while to realize it's an empty context).
| } | ||
| n => write!(f, "<set with {} elements>", n), |
There was a problem hiding this comment.
Should we keep this case for the same reason we applied #887? Similarly should consider adding a case like this for the record branch.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I guess it doesn't guard against large sets/records located recursively inside the Context
There was a problem hiding this comment.
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 (?)
There was a problem hiding this comment.
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.
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
| } | ||
| n => write!(f, "<set with {} elements>", n), |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| /// Context does not comply with the shape specified for the request action | ||
| #[derive(Debug, Error, Diagnostic)] | ||
| #[error("context `{context}` is not valid for `{action}`")] | ||
| #[error("context `{}` is not valid for `{action}`", pretty_print(&.context))] |
There was a problem hiding this comment.
this might be a good place to use the new thiserror syntax #[error(fmt = path::to::myfmt)] to avoid allocating a string here
There was a problem hiding this comment.
Performance on the error path is not critical, but seems reasonable to make a followup issue for this. #1371
Description of changes
Makes the improvements suggested in #998.
Issue #, if available
Fixes #998.
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy.I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):