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

Deny unknown fields in FFI. (#815) #1041

Merged
merged 5 commits into from
Jul 9, 2024

Conversation

andrewmwells-amazon
Copy link
Contributor

@andrewmwells-amazon andrewmwells-amazon commented Jul 5, 2024

Description of changes

Deny unknown fields in FFI.

Issue #, if available

Resolves #815

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

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

Signed-off-by: Andrew Wells <anmwells@amazon.com>
Signed-off-by: Andrew Wells <anmwells@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.

I think you'll want to add #[serde(deny_unknown_fields)] to several more types here (e.g., PartialAuthorizationCall and TemplateLink).

You might as well just add this annotation to every struct defined in ffi/. Some of these structs are expected to be output rather than input (e.g., Response), but it doesn't hurt to error on unknown fields anyways.

@khieta khieta mentioned this pull request Jul 9, 2024
3 tasks
@khieta
Copy link
Contributor

khieta commented Jul 9, 2024

Made the changes requested above (with @andrewmwells-amazon's permission) and merged with the new changes in #1014. Read for re-review!

@khieta
Copy link
Contributor

khieta commented Jul 9, 2024

Fyi cedar-drt failure is not due to this PR. cedar-java PR failure is caused by this PR -- looks like the Java wasn't generating completely well-formed JSON, despite passing all prior test cases. That's why it's useful to make this change 😄

Edit: fix for Java in cedar-java#178

@khieta khieta merged commit 30eb00f into main Jul 9, 2024
15 of 18 checks passed
@khieta khieta deleted the andrewmwells/deny_unknown_fields_ffi_815 branch July 9, 2024 19:44
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.

FFI functions should error, or at least warn, on unexpected fields
4 participants