Skip to content

Conversation

john-h-kastner-aws
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws commented Sep 25, 2024

Description of changes

Adds support for annotations like @foo as 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):

  • A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing API).
  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).
  • A bug fix or other functionality change requiring a patch to cedar-policy.
  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)
  • A change (breaking or otherwise) that only impacts unreleased or experimental code.

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).
  • Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.
  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)
  • Requires updates, but I do not plan to make them in the near future. (Make sure that your changes are hidden behind a feature flag to mark them as experimental.)
  • I'm not sure how my change impacts 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):

  • Does not require updates because my change does not impact the Cedar language specification.
  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-docs. PRs should be targeted at a staging-X.Y branch, not main.)
  • I'm not sure how my change impacts the documentation. (Post your PR anyways, and we'll discuss in the comments.)

Signed-off-by: John Kastner <[email protected]>
Signed-off-by: John Kastner <[email protected]>
Signed-off-by: John Kastner <[email protected]>
Signed-off-by: John Kastner <[email protected]>
Signed-off-by: John Kastner <[email protected]>
@john-h-kastner-aws john-h-kastner-aws force-pushed the feat/jkastner/annot_no_value branch from 9fa05b4 to 5f1eb88 Compare September 26, 2024 17:39
Signed-off-by: John Kastner <[email protected]>
Signed-off-by: John Kastner <[email protected]>
Copy link
Contributor

@shaobo-he-aws shaobo-he-aws left a comment

Choose a reason for hiding this comment

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

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.

@john-h-kastner-aws
Copy link
Contributor Author

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.

Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

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

Leaving a high-level comment, will leave a detailed review in a bit

Comment on lines 1170 to 1174
/// 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>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@john-h-kastner-aws john-h-kastner-aws Sep 30, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@john-h-kastner-aws john-h-kastner-aws Sep 30, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

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

Looks great.

@john-h-kastner-aws john-h-kastner-aws merged commit 6afaef8 into main Oct 1, 2024
19 checks passed
@shaobo-he-aws shaobo-he-aws deleted the feat/jkastner/annot_no_value branch October 1, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Key-only policy annotation
3 participants