-
Notifications
You must be signed in to change notification settings - Fork 107
Stateful isAuthorization method #1831
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
Conversation
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.
Seems like a good thing if it's 15x speedup.
Please update the CHANGELOG to specify that this is a change in the FFI/wasm. Also, fix the formatting issues.
70d0c06
to
2a81e21
Compare
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.
Looks good once CI passes.
} | ||
} | ||
|
||
impl StatefulAuthorizationCall { |
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.
This, AuthorizationCall
and PartialAuthorizationCall
are substantial similar. Wondering if there's any parsing logic we can share
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.
I went for the "rather have some duplication than break something existing" approach for this PR.
I'm not very familiar with the idiosyncrasies of Rust so I figured this'd be safer. I can give it a try if required.
6a84478
to
54b1b20
Compare
…licy#1829) Signed-off-by: Marc Bridner <[email protected]>
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 91.09% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 84.04% Status: PASSED ✅ Details
|
Looks good. Merging! |
Description of changes
Adds
stateful_is_authorized
,preparse_policy_set
andpreparse_schema
that can be used to pre-load an immutable schema/policy set.This way, we don't have to build a massive JSON struct on each authorization request.
In our tests where we perform approximately 50000 evaluations the execution time went from:
to
Or, in other words, a 15x performance increase.
Issue #, if available
#1829
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy
(e.g., addition of a new API).I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec
(choose one, and delete the other options):I confirm that
docs.cedarpolicy.com
(choose one, and delete the other options):