Skip to content
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

feat: Evaluate JSON Pointers in Policy Expressions #418

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

cstepanian
Copy link
Contributor

@cstepanian cstepanian commented Sep 17, 2024

Relates to #371

This PR replaces the previous preprocessor-based implementation of JSON Pointers with one that natively runs in the Policy Expression Executor.

It uses the ExprMutator visitor trait to evaluate JSON Pointers in two steps: first, pointers are looked up but the original pointer string is preserved. Then, the looked-up value is inserted. This structure is meant to allow for better error messages and user-facing explanations of why a Policy Expression was evaluated a given way.

@j-lanson j-lanson added product: hc Relates to the core "hc" binary type: enhancement New feature or request labels Sep 18, 2024
@j-lanson
Copy link
Collaborator

j-lanson commented Sep 18, 2024

Hey Cal,

Your solution right now is very reasonable, and I don't mind merging it as-is and making changes to it in a subsequent PR.

As for your questions about passing around the context, let me know if this is a possible solution.

The way I was imagining it, you have an Expr that represents the parsed program, which may contain JsonPointer variants. You have a function preprocess(expr: &mut Expr, context: &Value) which produces an Expr tree guaranteed to have no JsonPointer variants. You can then execute the program as normal without having to pass around &context.

It's like the JSON pointers are macros, but they get replaced once we have a program AST instead of before everything is tokenized.

@alilleybrinker
Copy link
Collaborator

Discussed, plan is to merge #458 and then rebase this on top and get it merged.

@cstepanian cstepanian force-pushed the cstepanian/eval_json_pointer branch 2 times, most recently from 25c106f to 0c5380a Compare October 28, 2024 19:47
This commit replaces the previous preprocessor-based implementation of
JSON Pointers with one that natively runs in the Policy Expression
`Executor`.

It uses the ExprMutator visitor trait to evaluate JSON Pointers in two
steps: first, pointers are looked up but the original pointer string is
preserved. Then, the looked-up value is inserted.  This structure is
meant to allow for better error messages and user-facing explanations of
why a Policy Expression was evaluated a given way.
@cstepanian
Copy link
Contributor Author

Re-implemented with late binding and using the ExprMutator visitor trait.

@j-lanson @alilleybrinker Requesting review again.

Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks very clean

@j-lanson j-lanson merged commit abfb40c into main Oct 28, 2024
18 checks passed
@cstepanian cstepanian deleted the cstepanian/eval_json_pointer branch October 28, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: hc Relates to the core "hc" binary type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants