Decouple concrete evaluator from partial evaluator #1392
Conversation
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
Signed-off-by: Shaobo He <[email protected]>
There was a problem hiding this comment.
This does introduce some duplication between the partial-evaluation and concrete-evaluation code, and I'm vaguely worried that the two could get out of sync if we update one and forget to update the other, but nonetheless I'll approve this, it seems like a reasonable code organization
This is an invasive change to the core Cedar evaluation code, which makes me nervous, but we have tests and DRT to catch any problems, and can rely on that
| /// The language spec and formal model give a precise definition of how this is | ||
| /// computed. |
There was a problem hiding this comment.
this comment probably makes more sense on is_authorized_core(); we don't have a formal model for partial authorization at the moment
| &self, | ||
| q: Request, | ||
| pset: &PolicySet, | ||
| evaluator: impl Fn(&Policy) -> std::result::Result<Either<bool, Expr>, EvaluationError>, |
There was a problem hiding this comment.
| evaluator: impl Fn(&Policy) -> std::result::Result<Either<bool, Expr>, EvaluationError>, | |
| evaluate: impl Fn(&Policy) -> std::result::Result<Either<bool, Expr>, EvaluationError>, |
| #[allow(clippy::unreachable)] | ||
| expr => unreachable!("internal invariant violation: BorrowedRestrictedExpr somehow contained this expr case: {expr:?}"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'e> Evaluator<'e> { |
There was a problem hiding this comment.
Suggest moving this impl block (or at least its evaluate() function) to concrete.rs where the Evaluator struct is defined
| let long_op = match op { | ||
| BinaryOrd::Less => |x, y| x < y, | ||
| BinaryOrd::LessEq => |x, y| x <= y, | ||
| }; | ||
| let ext_op = match op { | ||
| BinaryOrd::Less => |x, y| x < y, | ||
| BinaryOrd::LessEq => |x, y| x <= y, | ||
| }; |
There was a problem hiding this comment.
do we need both of these?
There was a problem hiding this comment.
alternately should we just inline them? they are each only used one place
|
|
||
| /// Interpret an `Expr` into a `Value` in this evaluation environment. | ||
| /// | ||
| /// May return a residual expression, if the input expression is symbolic. |
There was a problem hiding this comment.
does this comment needs updating? This is the concrete evaluator, right?
But isn't it the goal of this PR? That being said, I admit the duplication could be reduced. The major obstacle that prevents us from reducing it is that the two evaluators still share the same request and entity store types. |
|
Yes, this PR is doing exactly as #209 requests. My nervousness is indeed about #209 itself. Before we had a PR, it was hard to make a compelling argument about duplication or maintainability. Now that we have a PR (thanks!) we can much more concretely (ha) evaluate whether the duplication is worth the benefits of #209. |
Description of changes
Issue #, if available
#209
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
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):I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):