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.

Sign up for GitHub

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

Jump to bottom

feat: add priority_policy_explicit support #250

Merged
merged 10 commits into from
Mar 23, 2021
Merged

feat: add priority_policy_explicit support #250

merged 10 commits into from
Mar 23, 2021

Conversation

Zxilly
Copy link
Contributor

@Zxilly Zxilly commented Mar 20, 2021 •
edited by hsluoyz
Loading

Fix: #248

Zxilly added 2 commits March 20, 2021 20:55
@Zxilly
test: add unit tests for explicit priority model
368db62 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@Zxilly
feat: add priority_policy_explicit support
4c254d9 
#248
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@Zxilly Zxilly requested review from hsluoyz, nodece and keainye March 20, 2021 16:27
@Zxilly
perf: enhance batch action performance
69576e7 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@hsluoyz
Copy link
Member

hsluoyz commented Mar 20, 2021

@paul4156 please review.

@paul4156
Copy link

@Zxilly @hsluoyz
Swapped policies in priority_policy_explicit.csv

npm test has one failed test:

image

paul4156
paul4156 reviewed Mar 20, 2021
View reviewed changes
Copy link

@paul4156 paul4156 left a comment

Choose a reason for hiding this comment

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

I left some minor comments. Otherwise, the change looks good to me.
I am not sure if the broken test after swapping policies in example csv file is relevant or not.

Thank you very much!

src/model/model.ts Outdated Show resolved Hide resolved
src/util/util.ts Outdated Show resolved Hide resolved
src/util/util.ts Outdated Show resolved Hide resolved
@Zxilly
Copy link
Contributor Author

Zxilly commented Mar 21, 2021

@paul4156 I try to swap policies but failed to reproduce. Could you please provide more information?

image

Zxilly added 2 commits March 21, 2021 11:24
@Zxilly
style: small fix
9c5c54e 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@Zxilly
fix: change function argument
c851497 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
nodece
nodece reviewed Mar 21, 2021
View reviewed changes
src/model/model.ts Outdated Show resolved Hide resolved
nodece
nodece reviewed Mar 21, 2021
View reviewed changes
src/model/model.ts Outdated Show resolved Hide resolved
nodece
nodece reviewed Mar 21, 2021
View reviewed changes
src/model/model.ts Outdated Show resolved Hide resolved
nodece
nodece reviewed Mar 21, 2021
View reviewed changes
src/util/util.ts Outdated Show resolved Hide resolved
@paul4156
Copy link

@paul4156 I try to swap policies but failed to reproduce. Could you please provide more information?

image

I cannot reliably reproduce the failure either... Please ignore.

@nodece
Copy link
Member

nodece commented Mar 21, 2021

@Zxilly you also should add this feature to remove, update action, and add unit test for this changes.

@Zxilly
fix: fix incorrect implement
604316e 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@Zxilly
Copy link
Contributor Author

Zxilly commented Mar 21, 2021 •
edited
Loading

@nodece remove a policy will not break the priority.
I will later add it to update. My implement will be like

When checked priority, update will not operate policy directly but use removePolicy and addPolicy

But it seems not atomic

Zxilly added 2 commits March 21, 2021 16:26
@Zxilly
refactor: refactor priority index
c9b33e3 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@Zxilly
feat: add update priority unittest
e4f38c3 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@Zxilly Zxilly requested a review from nodece March 21, 2021 08:27
nodece
nodece reviewed Mar 21, 2021
View reviewed changes
src/model/model.ts Outdated Show resolved Hide resolved
@Zxilly
perf: priority performance enhanced
88d2ada 
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
nodece
nodece reviewed Mar 22, 2021
View reviewed changes
src/model/model.ts Outdated Show resolved Hide resolved
@Zxilly
refactor: refactor updatePolicy()
c4eda9c 
throw error when new rule and old rule didn't have same priority

Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>
@hsluoyz hsluoyz merged commit 763c18e into casbin:master Mar 23, 2021
github-actions bot pushed a commit that referenced this pull request Mar 23, 2021
@semantic-release-bot
chore(release): 5.6.0 [skip ci]
92f8d05 
# [5.6.0](v5.5.0...v5.6.0) (2021-03-23)

### Features

* add priority_policy_explicit support ([#250](#250)) ([763c18e](763c18e))
@github-actions
Copy link

🎉 This PR is included in version 5.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nodece nodece nodece left review comments

@paul4156 paul4156 paul4156 left review comments

@hsluoyz hsluoyz hsluoyz approved these changes

@keainye keainye Awaiting requested review from keainye

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] priority_policy_explicit does not respect priority
4 participants
@Zxilly @hsluoyz @paul4156 @nodece