Skip to content

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.

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 khieta requested a review from john-h-kastner-aws July 9, 2024 17:05
@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
@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