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

Allow Introspection on RestrictedExpression #1151

Open
2 tasks
ShiromMakkad opened this issue Aug 27, 2024 · 7 comments
Open
2 tasks

Allow Introspection on RestrictedExpression #1151

ShiromMakkad opened this issue Aug 27, 2024 · 7 comments
Labels
clarification-needed This issue needs further clarification before we can decide how to proceed papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request

Comments

@ShiromMakkad
Copy link

ShiromMakkad commented Aug 27, 2024

Category

User level API features/changes

Describe the feature you'd like to request

For testing purposes, I'd like to access the inner value of a restricted expression. For example,

assert_eq!(RestrictedExpression::from_decimal("16.0").into_inner(), "16.0".to_string());

Describe alternatives you've considered

It would be fine if that's not possible if we could just compare equality between RestrictedExpressions like so:

fn some_function_that_parses_data_into_cedar_context(value: T) -> HashMap<String, RestrictedExpression>;

let expr_map = some_function_that...(my_data);
assert_eq!(res.get("my_number").unwrap(), RestrictedExpression::from_decimal("16.0"));

I haven't dug around the Cedar codebase but I think you can just #[derive(PartialEq, Eq)].

Additional context

For now we're doing:

fn compare_expr(v1: &RestrictedExpression, v2: &RestrictedExpression) {
    assert_eq!(format!("{v1:?}"), format!("{v2:?}"));
}

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

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change
@ShiromMakkad ShiromMakkad added feature-request This issue requets a substantial new feature pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. labels Aug 27, 2024
@aaronjeline aaronjeline added papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request backlog and removed pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. labels Sep 3, 2024
@shaobo-he-aws shaobo-he-aws added clarification-needed This issue needs further clarification before we can decide how to proceed and removed backlog labels Oct 1, 2024
@shaobo-he-aws
Copy link
Contributor

@ShiromMakkad could you clarify why introspection is needed? I think for the alternative, you can evaluate restricted expressions into values and do the equality testing.

@ShiromMakkad
Copy link
Author

It's for convenience in unit testing. It could be done like I showed above or assert_ne!(RestrictedExpression::from_decimal("16.0"), RestrictedExpression::from_decimal("15.0"));

@cdisselkoen
Copy link
Contributor

Deriving equality on RestrictedExpression sounds perfectly reasonable to me. We'll have cases like, the restricted expression [1, 2, 3] will be != the restricted expression [3, 2, 1] even though, after evaluating, those produce equal Cedar values. But presumably that's the expected behavior when considering equality of expressions.

@cdisselkoen
Copy link
Contributor

more to the point of @ShiromMakkad 's above example, derived equality on RestrictedExpression would find that from_decimal("16.0") != from_decimal("16.00"). Is this your expected behavior @ShiromMakkad?

@ShiromMakkad
Copy link
Author

I think it's fine in the current version to go for a more restrictive result where "16.0" != "16.00". But if decimal("16.0") == decimal("16.00") are equal inside a cedar policy, it would be nice to have a way to evaluate that. I think that could also be a separate function though. Both interpretations are useful. For my unit tests, it wouldn't matter either way (the option you suggested would work just fine).

@cdisselkoen
Copy link
Contributor

The second interpretation (where decimal("16.0") and decimal("16.00") are equal) is what you would get if you called eval_expression on each expression, and then compared equality on the result.

@ShiromMakkad
Copy link
Author

Ok, then I'd prefer the first way where "16.0" != "16.00"

@john-h-kastner-aws john-h-kastner-aws removed the feature-request This issue requets a substantial new feature label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification-needed This issue needs further clarification before we can decide how to proceed papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request
Projects
None yet
Development

No branches or pull requests

5 participants