Skip to content

Conversation

vladcebo
Copy link
Contributor

@vladcebo vladcebo commented Mar 28, 2025

Introduces unknown_entities method to the PartialResponse.

Description of changes

Introduces a method uknown_entities on the PartialResponse.
During partial evaluation, knowing unknown entity ids can help clients to fetch the missing entities
and run the evaluation again.

This is achievable using the existing code via:

    let unknowns = partial_response
        .all_residuals()
        .fold(HashSet::new(), |mut unknowns, p| {
            unknowns.extend(p.unknown_entities().into_iter());
            unknowns
        });

Unfortunately this is slow for large policy sets due to Policy::from_ast relying on pretty printing.

so calling that in a loop doing entity discovery is just very expensive.

Issue #, if available

N/A

Checklist for requesting a review

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

  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).

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

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.

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.

@vladcebo vladcebo force-pushed the partial-response-unknown-entities branch 4 times, most recently from 12461ea to 53cf049 Compare March 28, 2025 12:00
@vladcebo
Copy link
Contributor Author

vladcebo commented Mar 28, 2025

Looking more at the API, there are many other methods that map the inner result viaPolicy::from_ast.

For example, say I'm interested in the policy ids that are definitely satisfied:

    let satisfied: HashSet<PolicyId> = cedar_response
        .definitely_satisfied()
        .map(|policy| policy.id().clone())
        .collect();

Well, you if you have lots of satisfied policies then there's lots of extra work in pretty printing that stuff.
Maybe can think of a more generic approach to expose that information in a cheaper way?

@adpaco-aws
Copy link
Contributor

Hi @vladcebo , thanks for the PR!

Maybe can think of a more generic approach to expose that information in a cheaper way?

Yes, it sounds like at the very least we should test the assumption that pretty-printing is cheaper than building the est with est::Policy::from(ast.clone()). I'm also not sure if these interfaces will remain the same with Type-Aware Partial Evaluation. @shaobo-he-aws any thoughts?

@shaobo-he-aws
Copy link
Contributor

cedar-policy/rfcs#95 should be an independent implementation in parallel to the existing PE. Besides, there's no explicit unknown in TPE. So, we should be fine here.

Copy link

github-actions bot commented Apr 3, 2025

Coverage Report

Head Commit: 53cf0490e051d45da55a651e5a7ab987f2f0611f

Base Commit: 5dd810be8cf74cf71500f4c2723751a66ff51f34

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 65.22%

Status: FAILED ❌

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/ast/policy.rs 🟢 12/13 92.31% 601
cedar-policy/src/api.rs 🔴 3/10 30.00% 1063-1069

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.38%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3052/4464 68.37% --
cedar-policy-cli 🔴 566/976 57.99% --
cedar-policy-core 🟢 11668/13912 83.87% --
cedar-policy-formatter 🟢 914/1043 87.63% --
cedar-policy-validator 🟢 8513/9944 85.61% --
cedar-wasm 🔴 0/29 0.00% --

Introduces unknown_entities method to
the PartialResponse.

Signed-off-by: Vlad <[email protected]>
@vladcebo vladcebo force-pushed the partial-response-unknown-entities branch from 53cf049 to c9ac0c8 Compare April 3, 2025 06:22
@vladcebo
Copy link
Contributor Author

vladcebo commented Apr 3, 2025

Added a test for coverage 👍

Copy link

github-actions bot commented Apr 3, 2025

Coverage Report

Head Commit: c9ac0c89b26403f537c1cb03e929c4f46ef9394c

Base Commit: 62d5394e600774f672af26ebd3c51a4a8c5d3129

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 95.24%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/ast/policy.rs 🟢 12/13 92.31% 601
cedar-policy/src/api.rs 🟢 8/8 100.00%

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.59%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3071/4467 68.75% 68.66%
cedar-policy-cli 🔴 573/976 58.71% 58.71%
cedar-policy-core 🟢 11710/13911 84.18% 84.17%
cedar-policy-formatter 🟢 914/1043 87.63% 87.63%
cedar-policy-validator 🟢 8509/9940 85.60% 85.60%
cedar-wasm 🔴 0/29 0.00% 0.00%

@adpaco-aws
Copy link
Contributor

Thanks for the contribution, @vladcebo ! I'll merge it now.

@adpaco-aws adpaco-aws merged commit 5f0189e into cedar-policy:main Apr 4, 2025
20 checks passed
jv-garcia pushed a commit to jv-garcia/cedar that referenced this pull request May 6, 2025
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