Internal data structures for Generalized templates #1732

alanwang67 · 2025-07-31T19:21:25Z

Description of changes

Introduce internal data structures for generalized templates. The main changes are having a policy now store a map from slots to types. The differences in the changes between Allow principal resource slots in condition and this PR are here.

Issue #, if available

Checklist for requesting a review

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

A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing API).
A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).
A bug fix or other functionality change requiring a patch to cedar-policy.
A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)
A change (breaking or otherwise) that only impacts unreleased or experimental code.

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).
Does not update the CHANGELOG because my change does not significantly impact released code.

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.
Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)
Requires updates, but I do not plan to make them in the near future. (Make sure that your changes are hidden behind a feature flag to mark them as experimental.)
I'm not sure how my change impacts 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):

Does not require updates because my change does not impact the Cedar language specification.
Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-docs. PRs should be targeted at a staging-X.Y branch, not main.)
I'm not sure how my change impacts the documentation. (Post your PR anyways, and we'll discuss in the comments.)

Signed-off-by: Alan Wang <[email protected]>

cedar-policy-core/src/ast/generalized_slots_annotation.rs

cedar-policy-core/src/ast/policy.rs

cdisselkoen · 2025-08-01T17:41:00Z

cedar-policy-core/src/ast/policy.rs

+    },
+
+    /// Value provided for a slot is not of the type annotated
+    #[error("value provided `{value}` for `{slot}` is not of type annotated `{ty}`")]


Suggested change

#[error("value provided `{value}` for `{slot}` is not of type annotated `{ty}`")]

#[error("value provided `{value}` for `{slot}` is not of declared type `{ty}`")]

cedar-policy-core/src/ast/policy.rs

cdisselkoen · 2025-08-01T17:43:30Z

cedar-policy-core/src/ast/policy.rs

    /// Values of the slots
    values: SlotEnv,
+    /// Generalized values of the slots
+    generalized_values: GeneralizedSlotEnv,


Would it be simpler to store the values of all slots in a GeneralizedSlotEnv? EntityUIDs could be stored as a RestrictedExpr

I spent some more time thinking about this and I think one of the consequences would be that now every time we have code involving templates we would have to handle the regular templates and the generalized case differently since the values of their slots appear in different fields and we would also have to enforce the invariant that only one of these HashMaps can have values in it. I was considering making the default value field of type HashMap<SlotId, RestrictedExpr> however, that would cause issues with the EST format.

One alternative solution in terms of making it easier for user's of the API not needing to supply values in two HashMaps and the potentially confusing error messages is to have the API handle the translation to our internal data structures.

If I understand correctly, you think it would be better to have a single map for internal data structures, two maps for the EST, and handle the conversion during translation. That sounds fine to me.

Can you update this comment thread based on our out-of-band discussion?

After implementing a version of this PR that used a single HashMap to store all slot values, there were issues involved with need to introduce many unwrap calls to translate between RestrictedExpr and EntityUID. Therefore, the choice ultimately made is to internally represent values of templates using two HashMap's and for user convenience at the API level to expose a single HashMap from SlotId to RestrictedExpr. Internally, we will do this conversion to use two HashMap's and so therefore we have to maintain the invariant that in value there are only bindings for SlotId's ?principal and ?resource and in generalized_values there are only bindings for generalized slots.

Signed-off-by: Alan Wang <[email protected]>

john-h-kastner-aws

Can you say a little more about the relation between this and #1722. And I guess lay out more the implementation plan so I know what to expect coming in future PRs.

Signed-off-by: Alan Wang <[email protected]>

alanwang67 · 2025-08-01T20:38:08Z

Can you say a little more about the relation between this and #1722. And I guess lay out more the implementation plan so I know what to expect coming in future PRs.

The changes in #1722 only allow the current ?principal and ?resource slots to appear in the condition of the template. The changes there more or less don't touch any of the other stuff with user defined slots. The other parts of the PR are here. The changes there are to the typechecker (the typechecker struct stores a map from slot id's to validator types and uses that when typechecking slots), changes to the parser, EST, and evaluator (whenever we evaluate we carry around another the generalized_values map).

Signed-off-by: Alan Wang <[email protected]>

This reverts commit 48ad73c.

This reverts commit d83ee15.

This reverts commit 5913651.

…EST format"" This reverts commit 4b4ed2a.

This reverts commit 92f3c36.

This reverts commit d83ee15.

This reverts commit 5913651.

This reverts commit 8e725ec.

…ing error messages

cedar-policy-core/src/ast/policy.rs

cdisselkoen · 2025-08-11T14:38:12Z

cedar-policy-core/src/ast/policy.rs

    /// Values of the slots
    values: SlotEnv,
+    /// Generalized values of the slots
+    generalized_values: GeneralizedSlotEnv,


Can you update this comment thread based on our out-of-band discussion?

Signed-off-by: Alan Wang <[email protected]>

john-h-kastner-aws

a couple questions, but looks good

john-h-kastner-aws · 2025-08-12T17:16:58Z

cedar-policy-core/src/ast/generalized_slots_declaration.rs

+
+/// Struct storing the pairs of SlotId's and their corresponding type
+#[derive(Clone, Eq, PartialEq, PartialOrd, Ord, Debug, Hash, Serialize, Deserialize)]
+pub struct GeneralizedSlotsDeclaration(BTreeMap<SlotId, JSONSchemaType<RawName>>);


On a first pass it's not clear why we need this and ValidatorGeneralizedSlotsDeclaration. Probably worth a comment explaining this design

Gotcha, yeah will include a comment. The reason why we need both is that JSONSchemaType<RawName> can't be used by the typechecker and needs to be converted to a validator type by combing information from the Schema.

john-h-kastner-aws · 2025-08-12T20:18:31Z

cedar-policy-core/src/validator/schema.rs

    }

+    /// Construct a [`Type`] from [`json_schema::Type<RawName>`]
+    pub fn json_schema_type_to_validator_type(


Does this function not already exist somewhere else? I feel like it must

I think the other place where a similar conversion occurs is the function from_schema_fragments in cedar-policy-core/src/validator/schema.rs but it's basically inlined within it.

alanwang67 added 10 commits July 29, 2025 09:59

slots in the condition of template

3149180

Signed-off-by: Alan Wang <[email protected]>

fixed + added testcases

4934e51

Signed-off-by: Alan Wang <[email protected]>

improved comments

e3a96b5

Signed-off-by: Alan Wang <[email protected]>

fmt

b0e6683

Signed-off-by: Alan Wang <[email protected]>

added has_slot for est & refactored

41804ab

Signed-off-by: Alan Wang <[email protected]>

added missing match arm

4a7e2f5

Signed-off-by: Alan Wang <[email protected]>

added linking with slots in the condition for the EST data structure

d2967de

Signed-off-by: Alan Wang <[email protected]>

first step

a0436e7

Signed-off-by: Alan Wang <[email protected]>

adding a new field to Policy

85e8dd5

Signed-off-by: Alan Wang <[email protected]>

removed repeated line

0b23d86

Signed-off-by: Alan Wang <[email protected]>

alanwang67 mentioned this pull request Aug 1, 2025

Generalized templates changes to EST, typechecker, parser, and evalutator #1738

Open

12 tasks

cdisselkoen self-requested a review August 1, 2025 17:16

cdisselkoen reviewed Aug 1, 2025

View reviewed changes

cdisselkoen requested review from adpaco-aws and john-h-kastner-aws August 1, 2025 17:47

alanwang67 added 2 commits August 1, 2025 14:20

updates to naming

3f40da2

Signed-off-by: Alan Wang <[email protected]>

more naming changes

e35cac5

Signed-off-by: Alan Wang <[email protected]>

john-h-kastner-aws reviewed Aug 1, 2025

View reviewed changes

changes

48ad73c

Signed-off-by: Alan Wang <[email protected]>

alanwang67 added 10 commits August 7, 2025 15:08

use only one field for values of slots

8e725ec

Signed-off-by: Alan Wang <[email protected]>

updates to use restricted expr as type for slots for EST format

5913651

Signed-off-by: Alan Wang <[email protected]>

Revert "changes"

d83ee15

This reverts commit 48ad73c.

Revert "Revert "changes""

92f3c36

This reverts commit d83ee15.

Revert "updates to use restricted expr as type for slots for EST format"

4b4ed2a

This reverts commit 5913651.

Revert "Revert "updates to use restricted expr as type for slots for …

905db6c

…EST format"" This reverts commit 4b4ed2a.

Revert "Revert "Revert "changes"""

15c1804

This reverts commit 92f3c36.

Revert "Revert "changes""

613d171

This reverts commit d83ee15.

Revert "updates to use restricted expr as type for slots for EST format"

bb0b339

This reverts commit 5913651.

Revert "use only one field for values of slots"

bd272da

This reverts commit 8e725ec.

validate incorrect entity types in generalized slots + removed confus…

c86dd88

…ing error messages

cdisselkoen approved these changes Aug 11, 2025

View reviewed changes

provided better comments clarifying invariants

a28f3e8

Signed-off-by: Alan Wang <[email protected]>

john-h-kastner-aws approved these changes Aug 12, 2025

View reviewed changes

	#[error("value provided `{value}` for `{slot}` is not of type annotated `{ty}`")]
	#[error("value provided `{value}` for `{slot}` is not of declared type `{ty}`")]

Internal data structures for Generalized templates #1732

Are you sure you want to change the base?

Internal data structures for Generalized templates #1732

Uh oh!

Conversation

alanwang67 commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Issue #, if available

Checklist for requesting a review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

alanwang67 commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alanwang67 commented Jul 31, 2025 •

edited

Loading

alanwang67 commented Aug 1, 2025 •

edited

Loading