-
Notifications
You must be signed in to change notification settings - Fork 80
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 serialization of schemas #949
Conversation
This fixes existing issue #500. I think we should consider some of the questions mentioned in #500 before merging this (because changing the behavior later might be difficult or painful for folks relying on it). Specifically, it would be great to leave default-valued fields implicit, rather than explicitly writing the value of every default-valued field, which I think this implementation will do (although I could be wrong). |
@@ -1382,6 +1383,18 @@ impl Schema { | |||
)) | |||
} | |||
|
|||
/// Serialize the `Schema` to a JSON value. | |||
pub fn to_json_str(&self) -> Result<String, schema_error::SchemaError> { | |||
serde_json::to_string(&self.0) |
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.
Unfortunately this will not give the public JSON schema format. IIRC the Serialize
impl for cedar_policy_validator::ValidatorSchema
is only to serialize the schema to send it to the Lean.
This function would need to first convert back to a cedar_policy_validator::SchemaFragment
and then serialize. We don't have any way to do that conversion atm. Perhaps a better solution would to take an approach similar to the LosslessPolicy
struct we use when serializing policies.
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.
Actually, along these lines, we should also have at least one test for each of the new public methods, just to sanity-check that we're getting the results we expect. (We can also add the JSON serialization to the roundtrip PBT targets, but that would obviously be a separate PR to cedar-spec
.)
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.
Oh I see, this explains the differences I was seeing in the output JSON compared to the tutorial.
Given the scope of changes needed to make this work, closing this PR for now. Updated #500 |
Description of changes
As far as I can tell, the json format for schemas is stable. So, this PR exposes methods to print and pretty-print json strings from schemas. I ran into this while writing my first cedar code in rust.
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.)