extend the 'has' relation in EST and JSON policy format#2154
extend the 'has' relation in EST and JSON policy format#2154victornicolet wants to merge 5 commits intomainfrom
Conversation
…or in conversions Signed-off-by: Victor Nicolet <victornl@amazon.com>
Signed-off-by: Victor Nicolet <victornl@amazon.com>
|
Looks like this breaks cedar-wasm because it exposes the EST. |
cdisselkoen
left a comment
There was a problem hiding this comment.
Looks good, modulo the TODOs and the issue of cedar-wasm breakage
cedar-policy-core/src/est/expr.rs
Outdated
| Simple { | ||
| /// Left-hand argument | ||
| left: Arc<Expr>, | ||
| /// Atribute tested |
There was a problem hiding this comment.
| /// Atribute tested | |
| /// Attribute tested |
cedar-policy-core/src/est/expr.rs
Outdated
| // Extended has - serialize | ||
| let expr = Expr::ExprNoExt(ExprNoExt::HasAttr(HasAttrRepr::Extended { | ||
| left: Arc::new(Expr::ExprNoExt(ExprNoExt::Var(ast::Var::Context))), | ||
| attr: nonempty!["user".into(), "profile".into(), "email".into()], | ||
| })); | ||
| let json = serde_json::to_value(&expr).unwrap(); | ||
| assert_eq!( | ||
| json, | ||
| serde_json::json!({"has": {"left": {"Var": "context"}, "attr": ["user", "profile", "email"]}}) | ||
| ); |
There was a problem hiding this comment.
Might be useful to have a test for an Extended with only one attr. (Obviously that could also be represented with Simple, but IIUC, in this PR we will allow the Extended representation with a singleton set as a legal JSON)
There was a problem hiding this comment.
Yes, good point. I'll also add a test that shows deserializing "has": { ... "attr": []} fails (empty list).
Signed-off-by: Victor Nicolet <victornl@amazon.com>
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 57.58% Status: FAILED ❌ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 86.27% Status: PASSED ✅ Details
|
|
This results in a change in the exported types in export type ExprNoExt = ... | { has: { left: Expr; attr: SmolStr }} | ...Now: export type ExprNoExt = ... | { has: HasAttrRepr } | ...
export type HasAttrRepr = { left: Expr; attr: SmolStr } | { left: Expr; attr: NonEmpty<SmolStr> };Expressions that typechecked before still typecheck after the change. |
|
Nice, way to go making the TS export a nonbreaking change |
Signed-off-by: Victor Nicolet <victornl@amazon.com>
36ac084 to
1aa74bd
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 93.94% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 86.30% Status: PASSED ✅ Details
|
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 93.94% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 86.30% Status: PASSED ✅ Details
|
cedar-policy-core/src/est/expr.rs
Outdated
| Arc::unwrap_or_clone(left).try_into_ast(id)?, | ||
| attr, | ||
| )), | ||
| // Should the try_into_ast use a builder anyway? |
There was a problem hiding this comment.
I believe try_into_ast could written entirely in terms of a generic ExprBuilder if that's what you're asking. For the moment, we only convert to AST so it doesn't matter, but we might want that for a future public-syntax tree.
cedar-policy-core/src/est/expr.rs
Outdated
| write!(f, " has \"{}\"", attr.head.escape_debug())?; | ||
| } | ||
| for attr in attr.tail.iter() { | ||
| // TODO: validation, we shouldn't have non-idents here |
There was a problem hiding this comment.
I assume this is what you mean here, but it's worth commenting that principal has "foo".bar doesn't parse
There was a problem hiding this comment.
Yes that's what I meant, I can add the comment.
Signed-off-by: Victor Nicolet <victornl@amazon.com>
169a5a2 to
8af7e33
Compare
Description of changes
This add a new variant for the
hasoperator in the EST, which translates to an extendedhasin JSON:This is backwards compatible, in the sense that:
hasi.e.{"has": {"left": <EXPR>, "attr": "attribute"}}still results in the same expression as before,hasoperator in the Cedar language is still desugared to a conjunction ofhasand.when converting to JSON.We may want to validate that in the extended has, each element in the list is a valid identifier to match the extended has of the Cedar policy language.
The new EST results in changes in the exported types in
cedar-wasm/pkg/*/cedar_wasm.d.ts. We have a new type:+ export type HasAttrRepr = { left: Expr; attr: SmolStr } | { left: Expr; attr: NonEmpty<SmolStr> };And the type of expressions uses this type:
export type ExprNoExt = { Value: CedarValueJson } | { Var: Var } | { Slot: string } | { "!": { arg: Expr } } | { neg: { arg: Expr } } | { "==": { left: Expr; right: Expr } } | { "!=": { left: Expr; right: Expr } } | { in: { left: Expr; right: Expr } } | { "<": { left: Expr; right: Expr } } | { "<=": { left: Expr; right: Expr } } | { ">": { left: Expr; right: Expr } } | { ">=": { left: Expr; right: Expr } } | { "&&": { left: Expr; right: Expr } } | { "||": { left: Expr; right: Expr } } | { "+": { left: Expr; right: Expr } } | { "-": { left: Expr; right: Expr } } | { "*": { left: Expr; right: Expr } } | { contains: { left: Expr; right: Expr } } | { containsAll: { left: Expr; right: Expr } } | { containsAny: { left: Expr; right: Expr } } | { isEmpty: { arg: Expr } } | { getTag: { left: Expr; right: Expr } } | { hasTag: { left: Expr; right: Expr } } | { ".": { left: Expr; attr: SmolStr } } + | { has: HasAttrRepr } - | { has: { left: Expr; attr: SmolStr } } | { like: { left: Expr; pattern: PatternElem[] } } | { is: { left: Expr; entity_type: SmolStr; in?: Expr } } | { "if-then-else": { if: Expr; then: Expr; else: Expr } } | { Set: Expr[] } | { Record: Record<string, Expr> };Typescript that was previously type-checking should typecheck with the new types.
Issue #, if available
#1889 only the EST extension part. Does not touch the AST.
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):cedar-spec, and how you have tested that your updates are correct.)cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):cedar-docs. PRs should be targeted at astaging-X.Ybranch, notmain.)