RFC 62 implementation #1327
RFC 62 implementation #1327
Conversation
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
| @@ -998,6 +1002,74 @@ impl Node<Option<cst::Add>> { | |||
| fn to_expr(&self) -> Result<ast::Expr> { | |||
| self.to_expr_or_special()?.into_expr() | |||
| } | |||
|
|
|||
| // Peel the grammar onion until we see valid RHS | |||
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
shaobo-he-aws
requested review from
cdisselkoen,
adpaco-aws,
andrewmwells-amazon and
john-h-kastner-aws
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
There was a problem hiding this comment.
Code looks good.
Two test case requests:
- Some more invalid examples in the
cst_to_ast.rstests. There are a few different possibleErrreturn paths that might not be tested. Some of them might be covered by existinghastest, but it'd be good to be sure. (You mention there's some dead code, so maybe just annotate any unreachable error cases) - A validator test check that
principal has foo.bar.baz && principal.foo.bar.bazvalidates as expected for optional attributefoo,barandbaz
|
Also, we should add a formatter test case. |
|
and while I'm suggesting more tests cases, we should have a test for conversion to EST |
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
| Either::Left(attr) => nonempty![attr], | ||
| Either::Right(ids) => ids.map(|id| id.to_smolstr()), | ||
| }; | ||
| let (first, rest) = attrs.split_first(); |
There was a problem hiding this comment.
This wouldn't be trivial since the types are all different, but I wonder if there's some nice way to make this code generic enough to be shared here and in construct_exprs_extended_has in the CST to AST conversion. I think we'd just need some sort of ExprBuilder<T> trait providing has_attr(T,SmolStr)->T, get_attr(T, SmolStr)->T and and(T, T) -> T. IDK if that'd be worthwhile, but maybe it's an idea that could help share code elsewhere.
There was a problem hiding this comment.
Seems to me like a good idea for a followup; this PR is large enough already and I think it would be good to review that trait change on its own
There was a problem hiding this comment.
Yes, it's a good idea but worth a separate PR.
There was a problem hiding this comment.
it would be very cool if cst->{ast,est} could share all of their code, but that sounds like a lot of work
| Either::Left(attr) => nonempty![attr], | ||
| Either::Right(ids) => ids.map(|id| id.to_smolstr()), | ||
| }; | ||
| let (first, rest) = attrs.split_first(); |
There was a problem hiding this comment.
Seems to me like a good idea for a followup; this PR is large enough already and I think it would be good to review that trait change on its own
| // An example from RFC | ||
| permit ( | ||
| principal is User, | ||
| action == Action::"preview", | ||
| resource == Movie::"Blockbuster" | ||
| ) | ||
| when | ||
| { | ||
| // extended has | ||
| principal | ||
| has | ||
| // contactInfo | ||
| contactInfo. | ||
| // address | ||
| address | ||
| . | ||
| // zip | ||
| zip && | ||
| // we are safe to access all attributes | ||
| principal.contactInfo | ||
| .address | ||
| .zip == "90210" | ||
| }; |
There was a problem hiding this comment.
This is not very pretty, should we open an issue to improve this formatting? Or I guess the culprit is the comments; what does the formatter produce for this example without comments?
There was a problem hiding this comment.
I'll add another policy without comments.
There was a problem hiding this comment.
Will also open an issue after the PR is merged.
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Description of changes
Issue #, if available
#1329
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy(e.g., addition of a new API).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):cedar-docs. PRs should be targeted at astaging-X.Ybranch, notmain.)