cedar-policy / cedar

Implementation of the Cedar Policy Language
https://www.cedarpolicy.com
Apache License 2.0
775 stars 64 forks source link

Refactor `Context` to enforce its invariant statically #541

Closed aaronjeline closed 5 days ago

aaronjeline commented 6 months ago

Category

Internal refactors/changes

Describe the feature you'd like to request

cedar_policy_core::ast::request::Context looks like:

/// `Context` field of a `Request`
#[derive(Debug, Clone, PartialEq, Serialize)]
pub struct Context {
    /// Context is internally represented as a `PartialValue`
    ///
    /// Context is serialized as a `RestrictedExpr`, for partly historical reasons.
    //
    // INVARIANT(ContextRecord): This must be a `Record`: either
    // `PartialValue::Value(Value::Record)`, or
    // `PartialValue::Residual(Expr::Record)`, or an appropriate unknown
    #[serde(flatten)]
    context: PartialValueSerializedAsExpr,
}

We should enforce this invariant statically if possible

Describe alternatives you've considered

Status quo, enforce it dynamically.

Additional context

No response

Is this something that you'd be interested in working on?

shaobo-he-aws commented 2 weeks ago

I'd like to prioritize this issue as it's been shown to help multiple PRs.

khieta commented 2 weeks ago

One PR this would have helped is #1027. (Note that the PR also updates the ContextRecord invariant to accurately reflect that a Context can never be purely unknown).

I think the right solution here is to store the context contents as a hash map (or similar data structure) instead of a record expression.