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

Add SchemaStore and policy validation #24

Merged
merged 11 commits into from
May 26, 2024

Conversation

Akamatsu21
Copy link
Contributor

Created the SchemaMemoryStore with get, update and delete operations
Implemented creating schema from file
Added API endpoints for the three operations
Added validation to policy create/update endpoints

@RazcoDev
Copy link

Hey @Akamatsu21 -
We're taking a look at the PR - but maybe it'll be worth to add some tests here :)
Thanks for the contribution !

@obsd obsd requested a review from omer9564 December 13, 2023 20:40
@obsd
Copy link

obsd commented Dec 17, 2023

Hey, @Akamatsu21, do you have an estimate of when you can add tests?

@Akamatsu21
Copy link
Contributor Author

Hey, @Akamatsu21, do you have an estimate of when you can add tests?

Hello and big apologies, I have been away for the past two weeks on a long holiday. I will have a look at adding some tests, you can expect them by the end of this week. Appreciate your patience on this!

@Akamatsu21
Copy link
Contributor Author

Hello @obsd @RazcoDev, I just pushed some unit tests, looking forward to your feedback.

@obsd
Copy link

obsd commented Jan 9, 2024

Thanks @Akamatsu21, I will ask someone to review :)

Copy link
Collaborator

@omer9564 omer9564 left a comment

Choose a reason for hiding this comment

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

Overall logic looks good

  • Some syntax comments
  • Some cosmetic comments
  • Some required changes on forward compatibility

src/routes/policies.rs Outdated Show resolved Hide resolved
src/routes/policies.rs Outdated Show resolved Hide resolved
src/routes/policies.rs Outdated Show resolved Hide resolved
src/routes/policies.rs Outdated Show resolved Hide resolved
src/routes/schema.rs Outdated Show resolved Hide resolved
src/services/schema/mod.rs Outdated Show resolved Hide resolved
tests/services/schema_tests.rs Outdated Show resolved Hide resolved
tests/services/schema_tests.rs Outdated Show resolved Hide resolved
src/services/policies/mod.rs Show resolved Hide resolved
tests/services/schema_tests.rs Outdated Show resolved Hide resolved
@omer9564
Copy link
Collaborator

We currently don't have CI to run the test so we run them manually, please attach a screenshot of the successfully running tests after your changes :)

@omer9564
Copy link
Collaborator

omer9564 commented Jan 15, 2024

Correct me if I'm wrong but the PR is missing the DataStore changes - validations

@Akamatsu21
Copy link
Contributor Author

Akamatsu21 commented Jan 29, 2024

Correct me if I'm wrong but the PR is missing the DataStore changes - validations

Hi @omer9564 thank you so much for the review, I will address your comments today. As mentioned in the discussion the data store validation is the next item on the list, but I wanted to check that the logic thus far has been correct before I attempt it. I am not expecting the PR to be merged before that's finished.

@omer9564
Copy link
Collaborator

omer9564 commented Feb 5, 2024

Hey @Akamatsu21 ,
Overall logic looks very good, besides what I wrote in the comments everything seems ok.
Please tag me once you finish with the data store validation and I will re-review everything :)

@Akamatsu21
Copy link
Contributor Author

Hi @omer9564 apologies you had to wait quite long for this fairly small update. I completed all the remaining tasks on my original list, please let me know if this is good to be merged. I am attaching a screenshot of passing tests, this was run on the RustRover IDE with the cargo version 1.76.0 (c84b36747 2024-01-18).

By the way, I merged the main branch into this one in order to make sure any tests added by other people still pass. I appreciate in hindsight that maybe a rebase would have made for a cleaner commit history, hope it's not a big issue.
cedar-agent-tests

@omer9564
Copy link
Collaborator

Hi @omer9564 apologies you had to wait quite long for this fairly small update. I completed all the remaining tasks on my original list, please let me know if this is good to be merged. I am attaching a screenshot of passing tests, this was run on the RustRover IDE with the cargo version 1.76.0 (c84b36747 2024-01-18).

By the way, I merged the main branch into this one in order to make sure any tests added by other people still pass. I appreciate in hindsight that maybe a rebase would have made for a cleaner commit history, hope it's not a big issue.

cedar-agent-tests

Thanks for the update, will take a look in the following days :)

@omer9564
Copy link
Collaborator

Sorry for the delay, had busy couple of weeks.
Will hopefully check this in the first week of April.

Copy link
Collaborator

@omer9564 omer9564 left a comment

Choose a reason for hiding this comment

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

Looks very good in general,

  • left a single comment on how the error handling of the policy store can be simplified with a reference to rust docs
  • left a single comment on a missing validation on schema mutation
    @Akamatsu21 Sorry for taking so long to re-review 😅

src/routes/schema.rs Show resolved Hide resolved
src/services/policies/mod.rs Show resolved Hide resolved
tests/services/schema_tests.rs Outdated Show resolved Hide resolved
src/routes/policies.rs Show resolved Hide resolved
@Akamatsu21
Copy link
Contributor Author

Looks very good in general,

  • left a single comment on how the error handling of the policy store can be simplified with a reference to rust docs
  • left a single comment on a missing validation on schema mutation
    @Akamatsu21 Sorry for taking so long to re-review 😅

No worries, and also sorry for taking a while. I should be able to respond faster now. I added the missing validation, but still think the unpacking of that error type is as good as it's going to be, unless we agree to just overhaul the return types

@omer9564
Copy link
Collaborator

omer9564 commented May 6, 2024

@Akamatsu21 I will try to push it next week :)

Copy link
Collaborator

@omer9564 omer9564 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@omer9564
Copy link
Collaborator

@Akamatsu21 Will merge & release a new version later this week

@omer9564 omer9564 merged commit 558b3ac into permitio:main May 26, 2024
@omer9564
Copy link
Collaborator

Fixes the discussion #23

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