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

Apply the Cedar schema terminology #1114

Merged
merged 17 commits into from
Aug 6, 2024

Conversation

shaobo-he-aws
Copy link
Contributor

@shaobo-he-aws shaobo-he-aws commented Aug 5, 2024

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):

  • A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing API).

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).

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

  • 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.)

Signed-off-by: Shaobo He <shaobohe@amazon.com>
@shaobo-he-aws shaobo-he-aws linked an issue Aug 5, 2024 that may be closed by this pull request
2 tasks
shaobo-he-aws added a commit to cedar-policy/cedar-spec that referenced this pull request Aug 5, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
shaobo-he-aws added a commit to cedar-policy/cedar-java that referenced this pull request Aug 5, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
shaobo-he-aws added a commit to cedar-policy/cedar-examples that referenced this pull request Aug 5, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
@shaobo-he-aws
Copy link
Contributor Author

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

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

I like the proposed renaming.

Will this extend to renaming the policy format?

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

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

Looks great

cedar-policy-cli/src/lib.rs Show resolved Hide resolved
cedar-policy-cli/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

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

cedar-policy/src/api/err.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/cedar_schema/parser.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/err.rs Outdated Show resolved Hide resolved
@cdisselkoen cdisselkoen mentioned this pull request Aug 6, 2024
3 tasks
@shaobo-he-aws
Copy link
Contributor Author

shaobo-he-aws commented Aug 6, 2024

I like the proposed renaming.

Will this extend to renaming the policy format?

Good idea. Craig just did it.

@shaobo-he-aws
Copy link
Contributor Author

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,

  • Schema::from_str_cedar to Schema::from_cedarschema_str
  • Schema::from_file_cedar to Schema::from_cedarschema_file
  • SchemaFragment::as_cedar to SchemaFragment::to_cedarschema

Signed-off-by: Shaobo He <shaobohe@amazon.com>
cedar-policy-validator/src/json_schema.rs Outdated Show resolved Hide resolved
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
cedar-policy/src/api.rs Show resolved Hide resolved
shaobo-he-aws and others added 6 commits August 6, 2024 11:05
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Co-authored-by: Kesha Hietala <khieta@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
…hub.com:cedar-policy/cedar into 842-unify-human-vs-natural-vs-cedar-terminology
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

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

Looks great

cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
cedar-policy/src/api.rs Outdated Show resolved Hide resolved
cedar-policy/src/api.rs Outdated Show resolved Hide resolved
cedar-policy/src/api.rs Outdated Show resolved Hide resolved
cedar-policy/src/api.rs Outdated Show resolved Hide resolved
shaobo-he-aws and others added 3 commits August 6, 2024 11:54
Co-authored-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

Looks good, aside from a small typo.

cedar-policy-validator/src/schema.rs Outdated Show resolved Hide resolved
Comment on lines +1266 to +1268
cedar_policy_validator::json_schema::Fragment::from_cedarschema_str(
src,
Extensions::all_available(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Was about to comment here that the lossless schema should be the input src, but then realized that this is just an issue with the implementation, so I opened an issue instead #1118

Co-authored-by: Kesha Hietala <khieta@amazon.com>
@shaobo-he-aws shaobo-he-aws merged commit 3a38ac9 into main Aug 6, 2024
12 of 19 checks passed
@shaobo-he-aws shaobo-he-aws deleted the 842-unify-human-vs-natural-vs-cedar-terminology branch August 6, 2024 19:33
shaobo-he-aws added a commit to cedar-policy/cedar-spec that referenced this pull request Aug 6, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Co-authored-by: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com>
shaobo-he-aws added a commit to cedar-policy/cedar-java that referenced this pull request Aug 7, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
shaobo-he-aws added a commit to cedar-policy/cedar-examples that referenced this pull request Aug 7, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
shaobo-he-aws added a commit that referenced this pull request Aug 7, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
shaobo-he-aws added a commit that referenced this pull request Aug 8, 2024
Signed-off-by: Shaobo He <shaobohe@amazon.com>
@khieta khieta mentioned this pull request Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify "human" vs "natural" vs "Cedar" terminology
4 participants