Finalize v4.0 JSON FFI #1014
Finalize v4.0 JSON FFI #1014
Conversation
|
Marking as a draft for now since this PR will break the Java bindings & I want to make sure things are fixable before I commit to the current design. Still happy for comments now though. |
|
Marking this PR as ready for review 🎉 The PR to fix the downstream Java break is here cedar-policy/cedar-java#173, and I'll get up a PR to fix the downstream Wasm break later this week. |
| let policies = set | ||
| .into_iter() | ||
| .map(|policy| policy.parse(None)) | ||
| .filter_map(|r| r.map_err(|e| errs.push(e)).ok()) |
There was a problem hiding this comment.
Would it be more idiomatic here to use something like .partition instead of mutating a vec?
There was a problem hiding this comment.
Possibly 🙂 The examples here https://doc.rust-lang.org/rust-by-example/error/iter_result.html show how to do this in a couple different ways (incl. using partition), but I think the filter_map + push approach is the most readable.
| @@ -373,7 +373,7 @@ impl From<PolicyId> for ast::PolicyID { | |||
| /// Identifier for a Template slot | |||
| #[repr(transparent)] | |||
| #[allow(clippy::module_name_repetitions)] | |||
| #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord, Hash, RefCast)] | |||
| #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord, Hash, RefCast, Serialize, Deserialize)] | |||
There was a problem hiding this comment.
deny_unknown_fields?
Does repr(transparent) imply serde(transparent)?
There was a problem hiding this comment.
I'll make sure denny_unknown_fields gets added as part of #1041 (assuming this PR is merged first)
There was a problem hiding this comment.
then my question is just, should we also have serde(transparent)
There was a problem hiding this comment.
Ah yes, didn't answer that part. From a quick google search: I'm not sure. Slot ids are definitely de/serializing like I'd expect (?principal and ?resource), so I don't think adding serde(transparent) will change anything, but maybe I should add it to be safe?
There was a problem hiding this comment.
Not sure either. As long as we have tests to catch regressions, I think we're fine without it
| .clone() | ||
| .map_or(String::new(), |id| format!(" with id `{id}`")); | ||
| match self { | ||
| Self::Human(str) => crate::Policy::parse(id.map(|id| id.to_string()), str) |
There was a problem hiding this comment.
I wonder if we should have a crate::Policy constructor that takes a PolicyId instead of a String here, to avoid unparsing and reparsing. Doesn't have to be in this PR.
Description of changes
This PR completes the remaining changes required by #757, and should be an accurate reflection of the JSON FFI to be used by Cedar v4.0 (aside from #854, which I'll address in a future PR).
The primary change in this PR is a new definition of
PolicySetto be used by both authorization and validation that supports static policies, templates, and template links. I also updated the currentSchematype to be an untagged enum to make the JSON prettier. The rest of the diff is changes to testing code.Issue #, if available
Resolves #757
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).I confirm that this PR (choose one, and delete the other options):
(Changes are covered by the current entry "Finalized the
ffimodule which was preview-released in 3.2.0. This involved a few additional API breaking changes inffi. See #757.")I confirm that
cedar-spec(choose one, and delete the other options):(This PR won't break DRT, but it will break the Java bindings and possibly others downstream)