-
Notifications
You must be signed in to change notification settings - Fork 117
expose json schema with resolved types functionality in cli #2090
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
expose json schema with resolved types functionality in cli #2090
Conversation
a7ae8d9 to
fa9b56b
Compare
fa9b56b to
e2d596b
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 86.76% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 86.18% Status: PASSED ✅ Details
|
cedar-policy/src/api.rs
Outdated
| pub fn schema_str_to_resolved_fragment( | ||
| schema_str: &str, | ||
| ) -> Result< | ||
| ( | ||
| json_schema::Fragment<cedar_policy_core::ast::InternalName>, | ||
| Vec<miette::Report>, | ||
| ), | ||
| Vec<miette::Report>, | ||
| > { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of returning warnings and errors as miette::Report, in this cedar-policy function can we return them as SchemaError and SchemaWarning, which are already public types? (Or whatever other specific error types are appropriate here.) This will also allow us to get rid of the code in CLI that detects parse errors just by looking for specific strings like "parse" in the error message. And the code for converting into miette::Reports can live in the ffi module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap SchemaError and SchemaWarning in an enum for the Err type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it only SchemaError in the Err case? the warnings being in the Ok pair?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah my bad, there's two errors in the Err case though, which was why I didn't do it this way. There's CedarSchemaParseErrors from converting the&str to Fragment<RawName> and SchemaError from converting from raw to internal. Should I wrap those two in an enum for this function? Didn't love the idea of creating a new public enum just for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CedarSchemaError is an existing public enum covering this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think)
cedar-policy/src/api.rs
Outdated
| schema_str: &str, | ||
| ) -> Result< | ||
| ( | ||
| json_schema::Fragment<cedar_policy_core::ast::InternalName>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it ok that this function in cedar_policy::api is leaking cedar_policy_core::ast::InternalName? Should i make it return something else, like maybe serde_json::Value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also leaking json_schema::Fragment, if I'm not mistaken.
serde_json::Value sounds good to me -- I think both the CLI and FFI don't need anything more detailed than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it's definitely leaking that too
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.25% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 86.17% Status: PASSED ✅ Details
|
john-h-kastner-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. just two comment tweaks
cedar-policy-cli/src/lib.rs
Outdated
| JsonToCedar, | ||
| /// Cedar schema syntax -> JSON | ||
| CedarToJson, | ||
| /// Cedar schema syntax -> JSON with all types resolved to entity or common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Cedar schema syntax -> JSON with all types resolved to entity or common | |
| /// Cedar schema syntax -> JSON with all types resolved to entity or common | |
| /// | |
| /// In contrast to `cedar-to-json`, this option requires that every type | |
| /// referenced in the schema is also defined. |
cedar-policy/src/api.rs
Outdated
| /// This is primarily meant to be used when working with schemas programmatically, | ||
| /// for example when creating a schema building UI. | ||
| /// | ||
| /// Returns `Ok((json_value`, warnings)) on success, or Err(error) on failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Returns `Ok((json_value`, warnings)) on success, or Err(error) on failure. | |
| /// Returns `Ok((json_value, warnings))` on success, or `Err(error)` on failure. |
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.25% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 86.17% Status: PASSED ✅ Details
|
Description of changes
Refactored the ffi functionality a little bit to move some of it to cedar-policy so it can be reused by both the ffi module and the cli. Added cli functionality to convert cedar schemas to json with resolved types.
Issue #, if available
n/a
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy(e.g., changes to the signature of an existing API).cedar-policy(e.g., addition of a new API).cedar-policy.cedar-policy-core,cedar-validator, etc.)I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):cedar-spec, and how you have tested that your updates are correct.)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):cedar-docs. PRs should be targeted at astaging-X.Ybranch, notmain.)