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

Native Go SQLite implementation #150

Closed
TJM opened this issue Mar 19, 2022 · 15 comments · Fixed by #158
Closed

Native Go SQLite implementation #150

TJM opened this issue Mar 19, 2022 · 15 comments · Fixed by #158

Comments

@TJM
Copy link

TJM commented Mar 19, 2022

I propose that sqlite should be supported, perhaps using this implementation: "github.com/glebarez/sqlite"

REF: https://gorm.io/docs/connecting_to_the_database.html#SQLite

@casbin-bot
Copy link
Member

@tangyang9464 @closetool @sagilio

@hsluoyz
Copy link
Member

hsluoyz commented Mar 20, 2022

@Abingcbc

@hsluoyz
Copy link
Member

hsluoyz commented Apr 16, 2022

@TJM can you make a PR to support it?

@TJM
Copy link
Author

TJM commented Apr 19, 2022

@hsluoyz I tried, but failed. :( I use sqlite for local development. I have tried with both the 'c' implementation and this golang implementation. They both work fine for my application, which uses gorm, but I get an error about "id" fields when I try to use it for casbin. The funny part is that it works fine the first time you start, but the second time you start the app (where the database has already been populated) and it tries to create the enforcer, this error comes up:

.../vendor/github.com/casbin/gorm-adapter/v3/adapter.go:373 SQL logic error: duplicate column name: id (1)
[0.017ms] [rows:0] ALTER TABLE `casbin_rule` ADD `id` integer

.../middleware.createEnforcer() failed to initialize casbin gorm adapter: SQL logic error: duplicate column name: id (1) 
exit status 1

SO, it doesn't do any good to add it to the (internal?) supported databases. There is apparently some other problem relating to "id" column. Honestly, I don't think that this code should have any direct support for databases. It should just take the gorm.DB and use whatever gorm supports. :)

... that way I could reduce the amount of imported code for the other N databases that I have no intention of using :)

~tommy

@hsluoyz
Copy link
Member

hsluoyz commented Apr 21, 2022

@tangyang9464

@tangyang9464
Copy link
Member

@TJM I found the bug was because of this line of code.

if err := a.db.AutoMigrate(t); err != nil {

See AutoMigrate bug. Since glebarez/sqlite is too backward and has not been updated, this PR cannot be implemented at present.

Regarding your second question, I don't quite understand what it means. If you have a better idea, you can open a new PR

@glebarez
Copy link

@TJM I found the bug was because of this line of code.

if err := a.db.AutoMigrate(t); err != nil {

See AutoMigrate bug. Since glebarez/sqlite is too backward and has not been updated, this PR cannot be implemented at present.
Regarding your second question, I don't quite understand what it means. If you have a better idea, you can open a new PR

Actually it is not falling backwards.
Here's the diff between fork and the original repo glebarez/sqlite@master...go-gorm:master
The only thing is deps, which are already up to date in fork.

@glebarez
Copy link

@TJM does your case fail with both CGo and pure-Go implementations? Or is it just Pure-Go fails ?

@tangyang9464
Copy link
Member

tangyang9464 commented Apr 21, 2022

@TJM I found the bug was because of this line of code.

if err := a.db.AutoMigrate(t); err != nil {

See AutoMigrate bug. Since glebarez/sqlite is too backward and has not been updated, this PR cannot be implemented at present.
Regarding your second question, I don't quite understand what it means. If you have a better idea, you can open a new PR

Actually it is not falling backwards. Here's the diff between fork and the original repo glebarez/sqlite@master...go-gorm:master The only thing is deps, which are already up to date in fork.

@glebarez Yes, you are right, I ignore that. If so, when this bug is resolved I'd be gald to implement our functionality based on it.

@tangyang9464
Copy link
Member

@TJM does your case fail with both CGo and pure-Go implementations? Or is it just Pure-Go fails ?

@glebarez I tested it. Fail with both CGo and pure-Go implementations.

@glebarez
Copy link

@TJM does your case fail with both CGo and pure-Go implementations? Or is it just Pure-Go fails ?

@glebarez I tested it. Fail with both CGo and pure-Go implementations.

created an issue go-gorm/gorm#5282 to the upstream.

@glebarez
Copy link

I fixed this issue in https://github.com/glebarez/sqlite
Please do go get -u github.com/glebarez/sqlite@v1.4.3
and try to reproduce.

@TJM
Copy link
Author

TJM commented Apr 21, 2022

You guys rock! Thanks!

I am passing it a glebarez/sqlite *gorm.DB, and it is working great!

@tangyang9464
Copy link
Member

I fixed this issue in https://github.com/glebarez/sqlite Please do go get -u github.com/glebarez/sqlite@v1.4.3 and try to reproduce.

@glebarez thanks. It works.

@github-actions
Copy link

🎉 This issue has been resolved in version 3.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants