-
Notifications
You must be signed in to change notification settings - Fork 160
symcc: add SymSchema for amortized SymEntities across request envs #2467
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ use super::tags::SymTags; | |
| use super::term::{Term, TermPrim, TermVar}; | ||
| use super::term_type::TermType; | ||
| use super::type_abbrevs::*; | ||
| use cedar_policy::{RequestEnv, Schema}; | ||
| use cedar_policy_core::validator::ValidatorSchema; | ||
| use cedar_policy_core::validator::{ | ||
| types::{Attributes, OpenTag, Type}, | ||
|
|
@@ -173,7 +174,7 @@ pub struct SymEnv { | |
| /// Symbolic request | ||
| pub request: SymRequest, | ||
| /// Symbolic entities | ||
| pub entities: SymEntities, | ||
| pub entities: Arc<SymEntities>, | ||
| } | ||
|
|
||
| impl SymEnv { | ||
|
|
@@ -403,7 +404,7 @@ impl SymEnv { | |
| pub fn of_env(ty_env: &Environment<'_>) -> Result<Self, CompileError> { | ||
| Ok(SymEnv { | ||
| request: SymRequest::of_request_type(&ty_env.req_ty)?, | ||
| entities: SymEntities::of_schema(ty_env.schema)?, | ||
| entities: Arc::new(SymEntities::of_schema(ty_env.schema)?), | ||
| }) | ||
| } | ||
| } | ||
|
|
@@ -419,6 +420,51 @@ impl SymEnv { | |
| // `RequestType` that are only used in `Cedar/SymCC/Env.lean`, so we define | ||
| // them here. | ||
|
|
||
| /// A [`Schema`] paired with its precomputed [`SymEntities`]. | ||
| /// | ||
| /// Building [`SymEntities`] from a schema is expensive and depends only on the | ||
| /// schema, not on any particular request environment. [`SymSchema`] computes it | ||
| /// once so it can be reused across many request environments via | ||
| /// [`SymSchema::sym_env`], which is significantly cheaper than rebuilding the | ||
| /// symbolic entities per environment (as [`SymEnv::new`] does). | ||
| #[derive(Debug, Clone)] | ||
| pub struct SymSchema { | ||
| schema: Schema, | ||
| entities: Arc<SymEntities>, | ||
| } | ||
|
|
||
| impl SymSchema { | ||
| /// Builds the symbolic entities for `schema` once, up front. | ||
| pub fn new(schema: &Schema) -> crate::err::Result<Self> { | ||
| Ok(Self { | ||
| schema: schema.clone(), | ||
| entities: Arc::new(SymEntities::of_schema(schema.as_ref())?), | ||
| }) | ||
| } | ||
|
|
||
| /// The [`Schema`] this symbolic schema was built from. | ||
| pub fn schema(&self) -> &Schema { | ||
| &self.schema | ||
| } | ||
|
|
||
| /// Returns a [`SymEnv`] for `req_env`, reusing the precomputed [`SymEntities`]. | ||
| /// | ||
| /// Only the (cheap) symbolic request is built per call; the (expensive) | ||
| /// symbolic entities are shared via a reference-counted clone. The resulting | ||
| /// [`SymEnv`] is identical to one built by [`SymEnv::new`] for the same | ||
| /// schema and request environment. | ||
| // INVARIANT: must produce the same SymEnv as SymEnv::of_env / SymEnv::new. | ||
| // Guarded by test sym_schema_sym_env_matches_sym_env_new. | ||
| pub fn sym_env(&self, req_env: &RequestEnv) -> crate::err::Result<SymEnv> { | ||
| let env = Environment::from_request_env(req_env, self.schema.as_ref()) | ||
| .ok_or_else(|| crate::err::Error::ActionNotInSchema(req_env.action().to_string()))?; | ||
| Ok(SymEnv { | ||
| request: SymRequest::of_request_type(&env.req_ty)?, | ||
| entities: self.entities.clone(), | ||
| }) | ||
| } | ||
|
Comment on lines
+458
to
+465
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a pre-existing issue in SymEnv::new (same ok_or_else(ActionNotInSchema) on line 56). SymSchema::sym_env matches the existing behavior. Should I change both?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think changing both makes sense, no need to block this PR though. |
||
| } | ||
|
|
||
| // TODO: test this | ||
|
|
||
| // Convenience functions | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that
SymSchemais the best name here since the schema isn't symbolic, the entities are. WouldCompiledSchemabe better? I don't know that it actually makes more sense, but we have "compiled" the schema intoSymEntities. Any thoughts @lianah ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
CompiledSchemasounds good. Also, it's similar toCompiledPolicywhich follows a similar pattern and contains the type checked policy + the symbolic representation of the policy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Liana's agrees, @tomlikestorock , can you do the rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sure, no problem