Rename nontrivial_residual_policies to residual_policies; residual_policies to policies - #2418
Conversation
Signed-off-by: John Kastner <jkastner@amazon.com>
17bb9dd to
d172fd4
Compare
Signed-off-by: John Kastner <jkastner@amazon.com>
|
DRT build failure is expected due to API change. semver-checks failure is warning that adding It's also worth calling out that the behavior change in |
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 93.55% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.94% Status: PASSED ✅ Details
|
There was a problem hiding this comment.
Pull request overview
This PR refines the experimental TPE (“type-aware partial evaluation”) residual-policy APIs to reduce naming confusion: TpeResponse::residual_policies() now yields only non-trivial residuals, the old nontrivial_residual_policies() is deprecated, and a new TpeResponse::policies() exposes the original “all residuals” behavior. It also renames several cedar-policy-core tpe::response::Response accessors to better reflect semantics (true_*, policies), updates CLI output accordingly, and documents an invariant for query request wrappers.
Changes:
- Adjust TPE residual policy iterators: non-trivial residuals via
residual_policies(), all residuals via newpolicies(), deprecatenontrivial_residual_policies(). - Rename cedar-policy-core TPE
Responseiterators/accessors (satisfied_*→true_*,residual_policies→policies) and update tests/callers. - Update CLI TPE command output and add CHANGELOG entry for the API behavior change.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cedar-policy/src/api/tpe.rs |
Updates public TPE response API (residual_policies semantics, adds policies, deprecates alias) and adjusts permission-query implementation. |
cedar-policy/CHANGELOG.md |
Documents the updated TPE residual policy API behavior and new iterator name. |
cedar-policy-core/src/tpe/response.rs |
Renames core TPE response accessors and changes get_residual return type; adds new policies() iterator. |
cedar-policy-core/src/tpe.rs |
Updates tests to match the new core TPE response accessor names and types. |
cedar-policy-cli/src/command/tpe.rs |
Updates CLI output to print all residuals using ans.policies(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 95.24% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.94% Status: PASSED ✅ Details
|
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 95.24% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.94% Status: PASSED ✅ Details
|
luxas
left a comment
luxas
left a comment
There was a problem hiding this comment.
Thanks for the followup, looks nice 👍
| /// Call [`Policy::to_pst()`] on each result to get a [`crate::pst::Policy`] | ||
| /// for structured inspection. Residual expressions may contain | ||
| /// [`crate::pst::Expr::ResidualError`] nodes indicating subexpressions that | ||
| /// would error at runtime; use [`crate::pst::Expr::has_error()`] to check. |
There was a problem hiding this comment.
would certainly or could possibly error? I guess that those that did concretely error already should not be shown here, but those that certainly will error should be here, as we don't know yet which exact error it will be, and thus it is non-trivial.
There was a problem hiding this comment.
hmmm, the whole has_error function is questionably useful. It says only that there is some subexpression which, if evaluated, will evaluate to an error. But, the subexpression might never be evaluated, or some other expression might error regardless of what has_error returns.
I think it's only useful when you want to represent a residual as cedar policy text since error residuals aren't directly representable
There was a problem hiding this comment.
Updated comment here and on has_error to be clearer
There was a problem hiding this comment.
yeah, it feels like this is one of the things that will get easier to reason about once I add the possible_bool_outcomes after my vacation 👍
Co-authored-by: Lucas Käldström <luxas@users.noreply.github.com>
Signed-off-by: John Kastner <jkastner@amazon.com>
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 95.18% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.94% Status: PASSED ✅ Details
|
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 95.18% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.94% Status: PASSED ✅ Details
|
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 95.18% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.94% Status: PASSED ✅ Details
|
| } | ||
|
|
||
| #[track_caller] | ||
| pub(super) fn assert_resource_get_attr(residual: &Residual, expected_attr: &str) { |
There was a problem hiding this comment.
nit: I felt it was quite nice to use assert_snapshot and interpret_typed_str_to_str in #2323. I'm thinking to convert most TPE tests to that style down the road.
There was a problem hiding this comment.
Yeah, insta is really nice. I'd be happy to review PRs adopting it where applicable.
| }); | ||
| }); | ||
| // (User::"aaron" in (resource["readers"])) || (User::"aaron" in (resource["editors"])) | ||
| assert_matches!(residuals.get_residual(policy2.id()).unwrap().get_residual().as_ref(), Residual::Partial { kind: ResidualKind::Or{ left, right }, .. } => { |
There was a problem hiding this comment.
yeah this was for sure a pain to read, thanks for making it better!
Description of changes
nontrivial_residual_policiesresidual_policiesto return only non-trivial residual policiespoliciesmatching original behavior ofresidual_policies, pluspolicy_setto get these policy collected into aPolicySetget_policyto lookup a partially evaluated policy by id, which may be useful when using one of the policy id iteratorsNot attached to these exact names if there are any strong opinions, but I do think the current combination of
nontrivial_residual_policiesandresidual_policiesis bad.Also contain an unrelated change to document some slightly subtle invariant on
query_resourceandquery_principal.Issue #, if available
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.)