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

Re-write Rust SDK #15

Merged
merged 16 commits into from
Sep 7, 2023
Merged

Re-write Rust SDK #15

merged 16 commits into from
Sep 7, 2023

Conversation

sheepduke
Copy link
Contributor

This PR

Re-writes the OpenFeature Rust SDK based on the spec.

This PR brings the following major improvements compared to existing code:

  • A singleton of OpenFeature is implemented as per spec 1.1.1. The application author should call get_client to get an instance. No more manual creation.

  • EvaluationDetails and ResolutionsDetails has been refined to use enum to store the reason and error code, per spec.

  • Evaluation functions is now strongly typed. Meaning the user should call get_int_value to get an i64 value. This is required by the spec 1.3.1.

  • Evaluation functions get_xx_value becomes async as suggested by the spec 1.4.11.

  • Meaningful doc strings for public types and traits. Content selectively copied from the spec.

See Follow-up tasks for what has not been done.

Related Issues

Fixes:

Notes

Why re-writing?

I tried to patch existing code but ended up re-writing it. The main reason was that too much code needed to be changed to make both spec and Rust compiler happy.

For example, the old resolution function takes a type parameter T: Clone that makes it non object safe, which breaks the requirements of singleton (lazy_static), async and more. The provider author can never put this T value in a cache because it has Clone constraint that implies Sized etc etc.

There are some other pain points and I cannot recall now. Still, the existing code inspired me in a lot of ways. Just I was a bit lazy to patch everything and re-writing would be easier.

Follow-up Tasks

  • Implement more evaluation functions get_xx_details.
  • Implement hooks as per [SDK] implement hooks #5.
  • Re-scan the spec and ensure everything is compliant.
  • Use Clippy to guarantee the code quality (by running some selective rules).
  • Address other issues in Rust SDK #9.

Signed-off-by: YUE Daian <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
@beeme1mr
Copy link
Member

beeme1mr commented Sep 6, 2023

Hey @sheepduke, could you please sign off your commits? It's a requirement from the CNCNF before we can merge.

To add your Signed-off-by line to every commit in this branch:

  1. Ensure you have a local copy of your branch by checking out the pull request locally via command line.
  2. In your local branch, run: git rebase HEAD~12 --signoff
  3. Force push your changes to overwrite the branch: git push --force-with-lease origin main

Copy link
Member

@beeme1mr beeme1mr 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! As a follow-up, please update the readme to match the new package name and structure.

renovate.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the renovate config?

Copy link
Member

Choose a reason for hiding this comment

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

Noting that this needs to be fixed before merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please kindly explain to me what does this file do? I googled it but did not see anywhere related. Plus no usage in the code...

Copy link
Member

@justinabrahms justinabrahms left a comment

Choose a reason for hiding this comment

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

I'm biased b/c I wrote this code, but you might like the pattern we used in the java-sdk where each test is annotated with a specification that it covers. It helps uncover which areas we're not covering yet.

https://github.com/open-feature/java-sdk/blob/main/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java#L69

renovate.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Noting that this needs to be fixed before merge.

src/api/api.rs Show resolved Hide resolved
src/api/api.rs Outdated Show resolved Hide resolved
src/evaluation/context.rs Show resolved Hide resolved
src/evaluation/details.rs Outdated Show resolved Hide resolved
src/evaluation/details.rs Outdated Show resolved Hide resolved
src/evaluation/field_value.rs Show resolved Hide resolved
sheepduke and others added 3 commits September 7, 2023 09:51
Co-authored-by: Justin Abrahms <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Co-authored-by: Justin Abrahms <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
@sheepduke
Copy link
Contributor Author

I'm biased b/c I wrote this code, but you might like the pattern we used in the java-sdk where each test is annotated with a specification that it covers. It helps uncover which areas we're not covering yet.

https://github.com/open-feature/java-sdk/blob/main/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java#L69

@justinabrahms My apologies. Your request changes have been overwritten by my force-update to resolve the signing issue. :-(

Could you please kindly re-request it, or let me know what have you changed?

I am rather curious that how did you add the specification to the tests.

Copy link
Member

@justinabrahms justinabrahms left a comment

Choose a reason for hiding this comment

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

The main thing we need to change is the renovate file being deleted.

As for spec tests, there's a python script in the repo which parses them and validates. Something for later, but you could probably make your life easier by commenting the relevant spec until we get something statically verifiable.

@sheepduke
Copy link
Contributor Author

The main thing we need to change is the renovate file being deleted.

As for spec tests, there's a python script in the repo which parses them and validates. Something for later, but you could probably make your life easier by commenting the relevant spec until we get something statically verifiable.

Thanks for sharing the info. I have added this JSON file back and created an issue to track it:
#16

I have re-requested your review. :-)

@justinabrahms
Copy link
Member

Ahh. Bit of confusion. I'll update the ticket. Renovate keeps our dependencies up to date. The json thing I mentioned was something else.

@justinabrahms justinabrahms merged commit 8cf5b19 into open-feature:main Sep 7, 2023
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.

4 participants