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

Surface locations from validation errors, improve TS interfaces #824

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

Swolebrain
Copy link
Contributor

Description of changes

Issue #, if available

806

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).

I confirm that this PR (choose one, and delete the other options):

  • Does not update the CHANGELOG because my change does not significantly impact released code.

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

@aaronjeline aaronjeline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. My only comment is on two changes:
You changed a pub enums variant name, and all the changes from untagged to tagged(type), doesn't this make this a breaking change?

@Swolebrain
Copy link
Contributor Author

Looks good. My only comment is on two changes: You changed a pub enums variant name, and all the changes from untagged to tagged(type), doesn't this make this a breaking change?

Totally, but cedar-wasm is basically unreleased. People re using it by building directly from source, but it's not officially really released. I don't have strong opinions as to whether it's okay to do this though.

Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conflicts with #800 which will be making more changes, but I think this PR is good on its own and we can take it and then revise #800 accordingly.

Signed-off-by: Victor Moreno <morevct@amazon.com>
@aaronjeline aaronjeline merged commit 3ab9330 into main Apr 26, 2024
14 of 16 checks passed
@aaronjeline aaronjeline deleted the wasm-interface branch April 26, 2024 17:59
@cdisselkoen
Copy link
Contributor

cdisselkoen commented Apr 29, 2024

Heads-up: #800 does want to make breaking changes to some of the choices in this PR. Reviews of #800 are welcome and encouraged.

  • Errors can have multiple source locations, at least in the fullness of time. (Cedar doesn't use this much currently, but miette supports it, and it seems like a good future-proofing measure to support it in the FFI now.) Propagate miette error info through FFI #800 proposes to rename sourceLocation to sourceLocations and make it a JSON array.
  • Because there are many fields (up to 7) associated with each error in Propagate miette error info through FFI #800, Propagate miette error info through FFI #800 proposes to rename the main error message from error to message.
  • Propagate miette error info through FFI #800 also introduces one more level of nesting for errors; validation errors are proposed to be { policyId: ..., error: { message: ..., sourceLocations: [...], ...}. This could be flattened, but I thought it was valuable to have a consistent structure for errors which could be mirrored on the TS side, and the contents of error in this struct (DetailedError in the Rust in Propagate miette error info through FFI #800) are used in many other places now, and there is not always a policyId associated with all of these errors (this struct is used for other kinds of errors as well, not just errors on policies). I'm open to removing the nesting if yall think the flatter structure is more important than a consistent DetailedError type.
  • type: "error" renamed to type: "failure". I think this is more clear, since the "success" case can still contain policy evaluation errors. The top-level Failure is only for failures to parse or call the authorizer entirely.

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.

3 participants