Skip to content

Add Entities::len and Entities::is_empty #1562

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 8, 2025
Merged

Add Entities::len and Entities::is_empty #1562

merged 6 commits into from
Apr 8, 2025

Conversation

adpaco-aws
Copy link
Contributor

@adpaco-aws adpaco-aws commented Apr 4, 2025 •
edited
Loading

Description of changes

Add the Entities::len method requested in #1523. This new API returns the number of Entitys in the Entities.
Also add the Entities::is_empty method as recommended by clippy.

The PR uses both len and is_empty in some existing unit tests and includes new tests specific to them.

Resolves #1523

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

@adpaco-aws
Add Entities::num_of_entities
c61cc69 
Signed-off-by: Adrian Palacios <[email protected]>
@adpaco-aws
Update changelog
ebda477 
Signed-off-by: Adrian Palacios <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 4, 2025

Coverage Report

Head Commit: ebda477b513820536ce485414badb7c979c629bb

Base Commit: 5f0189efedfad5d8b21005c4f7823d59b609a574

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/entities.rs 🟢 3/3 100.00%
cedar-policy/src/api.rs 🟢 3/3 100.00%

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.60%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3074/4470 68.77% --
cedar-policy-cli 🔴 573/976 58.71% --
cedar-policy-core 🟢 11713/13914 84.18% --
cedar-policy-formatter 🟢 914/1043 87.63% --
cedar-policy-validator 🟢 8509/9940 85.60% --
cedar-wasm 🔴 0/29 0.00% --

john-h-kastner-aws
john-h-kastner-aws reviewed Apr 7, 2025
View reviewed changes
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.

Complaining about the name

@john-h-kastner-aws
Copy link
Contributor

It's unclear to me if num_of_entities (or the underlying len method) should be specialized for the partial-evaluation case.

What would .iter().count() do? IMO these should give the same number unless we have a good reason.

cdisselkoen
cdisselkoen reviewed Apr 7, 2025
View reviewed changes
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 to me, modulo John's comments, which I endorse.

@adpaco-aws
Rename method as len
badd15d 
Signed-off-by: Adrian Palacios <[email protected]>
@adpaco-aws
Add is_empty too
c02061a 
Signed-off-by: Adrian Palacios <[email protected]>
@adpaco-aws
Update changelog
98ee0a2 
Signed-off-by: Adrian Palacios <[email protected]>
@adpaco-aws
Copy link
Contributor Author

What would .iter().count() do? IMO these should give the same number unless we have a good reason.

Ok, I'll leave it as is.

@adpaco-aws adpaco-aws changed the title Add Entities::num_of_entities Add Entities::len and Entities::is_empty Apr 8, 2025
john-h-kastner-aws
john-h-kastner-aws approved these changes Apr 8, 2025
View reviewed changes
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2025

Coverage Report

Head Commit: c02061a4c44a485c3d1a463f79688561517d8e39

Base Commit: 5f0189efedfad5d8b21005c4f7823d59b609a574

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/entities.rs 🟢 6/6 100.00%
cedar-policy/src/api.rs 🟢 6/6 100.00%

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.60%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3077/4473 68.79% --
cedar-policy-cli 🔴 573/976 58.71% --
cedar-policy-core 🟢 11716/13917 84.18% --
cedar-policy-formatter 🟢 914/1043 87.63% --
cedar-policy-validator 🟢 8509/9940 85.60% --
cedar-wasm 🔴 0/29 0.00% --

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2025

Coverage Report

Head Commit: 98ee0a2a3fccf557ba261e855f7ab11000aedfd6

Base Commit: 5f0189efedfad5d8b21005c4f7823d59b609a574

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/entities.rs 🟢 6/6 100.00%
cedar-policy/src/api.rs 🟢 6/6 100.00%

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.60%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3077/4473 68.79% --
cedar-policy-cli 🔴 573/976 58.71% --
cedar-policy-core 🟢 11716/13917 84.18% --
cedar-policy-formatter 🟢 914/1043 87.63% --
cedar-policy-validator 🟢 8509/9940 85.60% --
cedar-wasm 🔴 0/29 0.00% --

@adpaco-aws
Implement John's suggestions
7c31d6e 
Signed-off-by: Adrian Palacios <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2025

Coverage Report

Head Commit: 7c31d6e6c55cb3754d7e35f3986fd2122a71a4e8

Base Commit: 5f0189efedfad5d8b21005c4f7823d59b609a574

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/entities.rs 🟢 6/6 100.00%
cedar-policy/src/api.rs 🟢 6/6 100.00%

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.60%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3077/4473 68.79% --
cedar-policy-cli 🔴 573/976 58.71% --
cedar-policy-core 🟢 11716/13917 84.18% --
cedar-policy-formatter 🟢 914/1043 87.63% --
cedar-policy-validator 🟢 8509/9940 85.60% --
cedar-wasm 🔴 0/29 0.00% --

@adpaco-aws adpaco-aws merged commit 243edc2 into cedar-policy:main Apr 8, 2025
20 checks passed
@adpaco-aws adpaco-aws mentioned this pull request Apr 25, 2025
Merged
4 tasks
jv-garcia pushed a commit to jv-garcia/cedar that referenced this pull request May 6, 2025
@adpaco-aws @juanvgarcia
Add Entities::len and Entities::is_empty (cedar-policy#1562)
609f544 
Signed-off-by: Adrian Palacios <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cdisselkoen cdisselkoen cdisselkoen left review comments

@chaluli chaluli chaluli left review comments

@john-h-kastner-aws john-h-kastner-aws john-h-kastner-aws approved these changes

@shaobo-he-aws shaobo-he-aws Awaiting requested review from shaobo-he-aws

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Addition of Entities::num_of_entities function similar to PolicySet::num_of_policies

5 participants

@adpaco-aws @john-h-kastner-aws @cdisselkoen @shaobo-he-aws @chaluli