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 missing implementation for casbin@0.3.0 and make unique key constraint works #15

Merged
merged 4 commits into from
Mar 2, 2020

Conversation

GopherJ
Copy link
Member

@GopherJ GopherJ commented Mar 2, 2020

Hello, this PR make unique key works for policy rules, before it cannot work because we do have nullable fields.

Now we are adding empty string to solve this issue.

Besides, it adds implementation of add_policies and remove_policies using diesel transaction.

@GopherJ
Copy link
Member Author

GopherJ commented Mar 2, 2020

@hsluoyz @omid could you review?

@GopherJ GopherJ changed the title add missing implementation for casbin@0.3.0 and make unique key works add missing implementation for casbin@0.3.0 and make unique key constraint works Mar 2, 2020
@hsluoyz
Copy link
Member

hsluoyz commented Mar 2, 2020

@GopherJ why don't use the built-in services provided by Travis CI? Like how we did it in Xorm adapter: https://github.com/casbin/xorm-adapter/blob/79a2aa54a016320eb29cf90090f642183827750b/.travis.yml#L14-L16

@GopherJ
Copy link
Member Author

GopherJ commented Mar 2, 2020

hello @hsluoyz we are using it, but in dev I would like to have some scripts so I added these two scripts

@hsluoyz
Copy link
Member

hsluoyz commented Mar 2, 2020

Got it.

@hsluoyz hsluoyz merged commit a612f82 into casbin-rs:master Mar 2, 2020
.collect::<Vec<NewCasbinRule>>();

conn.transaction::<_, DieselError, _>(|| {
diesel::insert_into(casbin_rules)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the transaction here? I think Diesel will generate one query.

Copy link
Member Author

@GopherJ GopherJ Mar 2, 2020

Choose a reason for hiding this comment

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

I was also thinking about this. But I'm not really sure about it. I'm not sure if it's one query or multiple. If it's one query we can then remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not 100% sure in every case 🙈

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