Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise APIs of PrincipalOrResourceConstraint #954

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

shaobo-he-aws
Copy link
Contributor

@shaobo-he-aws shaobo-he-aws commented Jun 4, 2024

Description of changes

  • Replace PrincipalOrResourceConstraint::iter_euids with PrincipalOrResourceConstraint::get_euid
    • The original function returns an iterator that works on at most one element. So, this PR replaces it with a function returning an Option so that we can pattern match the return value more conveniently.
  • Wrap Name and EntityUID with Arc
    • They are mostly read only but can contain lots of bytes

Issue #, if available

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)

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

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

…ResourceConstraint::get_euid`

The original function returns an iterator that works on at most
one element. So, this PR replaces it with a function returning
an `Option` so that we can pattern match the return value more
conveniently.

Signed-off-by: Shaobo He <[email protected]>
@shaobo-he-aws shaobo-he-aws changed the title Replace PrincipalOrResourceConstraint::iter_euids with ResourceConstraint::get_euid Replace PrincipalOrResourceConstraint::iter_euids with PrincipalOrResourceConstraint::get_euid Jun 4, 2024
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-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

@shaobo-he-aws shaobo-he-aws changed the title Replace PrincipalOrResourceConstraint::iter_euids with PrincipalOrResourceConstraint::get_euid Revise APIs of PrincipalOrResourceConstraint Jun 4, 2024
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 good. Name and EntityUID were already O(1) clone (they already contain Arcs internally I believe), but I'm willing to believe this Arc is still a good idea

cedar-policy-core/src/ast/policy.rs Outdated Show resolved Hide resolved
@andrewmwells-amazon
Copy link
Contributor

Looks like the downstream DRT will need to be fixed but the Java failure on Mac seems to be a glitch.

@shaobo-he-aws shaobo-he-aws merged commit 8cf0400 into main Jun 5, 2024
14 of 16 checks passed
@shaobo-he-aws shaobo-he-aws deleted the feature/shaobo/update-core-api branch June 5, 2024 16:29
shaobo-he-aws added a commit to cedar-policy/cedar-spec that referenced this pull request Jun 5, 2024
shaobo-he-aws added a commit to cedar-policy/cedar-spec that referenced this pull request Jun 5, 2024
aaronjeline pushed a commit to cedar-policy/cedar-spec that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants