Add support for annotations without explicit value#1231
Add support for annotations without explicit value#1231john-h-kastner-aws merged 14 commits intomainfrom
Conversation
Signed-off-by: John Kastner <jkastner@amazon.com>
Signed-off-by: John Kastner <jkastner@amazon.com>
Signed-off-by: John Kastner <jkastner@amazon.com>
9fa05b4 to
5f1eb88
Compare
shaobo-he-aws
left a comment
There was a problem hiding this comment.
LGTM.
Though I'd like to propose a new terminology. Can we call what's after @ the annotation name and what's inside () the annotation argument.
I don't particularly want to pull a change in terminology into this PR. |
cdisselkoen
left a comment
There was a problem hiding this comment.
Leaving a high-level comment, will leave a detailed review in a bit
cedar-policy-core/src/ast/policy.rs
Outdated
| /// Annotation value. `None` for annotations without a value, i.e., `@foo`. | ||
| /// An annotation without a value should be treated as equivalent to the | ||
| /// value being `""`. This interpretation is implemented by the `AsRef<str>` | ||
| /// impl an `val` method below. | ||
| raw_val: Option<SmolStr>, |
There was a problem hiding this comment.
Why Option and explicitly saying that None and Some("") are equivalent? This seems like a footgun for later. Wouldn't it be better to just have a SmolStr here so that there's only one representation?
There was a problem hiding this comment.
The reason is that conversion to EST, which is currently written to go through this AST struct, even when converting directly from CST, should be lossless (mostly? not sure our exact standard here), so this struct needed to know if it was originally present.
There was a problem hiding this comment.
Personally I would propose that we pick one or the other representation to be canonical and output that EST. So either we decide that empty annotation values are always omitted, or always explicit "". I believe "lossless" already drops extraneous parens, is this any different?
There was a problem hiding this comment.
A related decision: In the EST I updated annotations so that null is an allowed value, equivalent to "". Thoughts? I think we would eventually need to allow that in a future world where we differentiate no-value and empty-string, but it's not necessary for now.
There was a problem hiding this comment.
I could see an argument either way there. Some sugar in the EST can be helpful, but the feature request was probably primarily for sugar in the Cedar policy format, and it's totally reasonable to require explicitness in the EST.
There was a problem hiding this comment.
Made some of the cst-to-ast code generic so that est-to-ast can still reuse the same id and string validation checks without passing through the ast structs to avoid this issue. New impl preserves the absent value on cst<->est, but converts it to "" on ast<->est
Signed-off-by: John Kastner <jkastner@amazon.com>
Description of changes
Adds support for annotations like
@fooas sugar for@foo("").Will require a small DRT update to account for the change in data structures. Will also require a small change to the docs to note that this syntax is also allowed.
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy(e.g., changes to the signature of an existing API).cedar-policy(e.g., addition of a new API).cedar-policy.cedar-policy-core,cedar-validator, etc.)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.)