cedar-policy / cedar

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

Apply the Cedar schema terminology #1114

Closed shaobo-he-aws closed 3 months ago

shaobo-he-aws commented 3 months ago

Description of changes

Issue #, if available

842

Checklist for requesting a review

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

I confirm that this PR (choose one, and delete the other options):

I confirm that cedar-spec (choose one, and delete the other options):

shaobo-he-aws commented 3 months ago

All APIs and comments should be updated. The remaining work is to rename tests, which I will do once a preliminary review is done.

shaobo-he-aws commented 3 months ago

I like the proposed renaming.

Will this extend to renaming the policy format?

Good idea. Craig just did it.

shaobo-he-aws commented 3 months ago

Overall, the changes look good. The new names are more consistent.

But I'd prefer more symmetry between the public (aka defined in api.rs) PolicySet and Schema interfaces. For PolicySet we provide:

  • a FromStr impl (which parses the Cedar syntax)
  • from_json_str
  • from_json_value
  • from_json_file
  • to_json

I think we should provide the same sort of interface for Schema and SchemaFragment. So:

  • the FromStr impl should parse the Cedar syntax, and we should have a separate from_json_str to parse the JSON syntax
  • from_file should be renamed from_json_file
  • from_file_cedar should be renamed to from_file
  • maybe rename as_cedar to to_cedar

How about using the file suffix (cedarschema vs json) to name these APIs? For instance,