Added Config for custom driver (specifically libSQL)

[ytsruh]

• Do only one thing
• Non breaking API changes
• Tested

What did this pull request do?

It adds a Config struct that can be used the same way the Postgres version can be. This allows a user to add their own driver instead of using the pre-defined one.

User Case Description

To use this with libSQL or Turso compatible SQLite

[haaawk]

LGTM

[asilvadesigns]

👀

[xEricL]

Looking forward to this! 😃

[xEricL]

Just to confirm, I tested the changes in this PR and everything works flawlessly with Turso. Nice work @ytsruh!

[haaawk]

What is needed to merge this?
@jinzhu Could you have a look please?

[ncruces]

I'll be blunt: this is a bad idea.

The API is fine, although I'm partial to just adding a func OpenConn(conn gorm.ConnPool) and be done with it.

The reason why this is a bad idea is the intended purpose of using alternative drivers. I cannot stress this enough: using multiple implementations of SQLite in the same process is a really, really bad idea. If you, by accident, open the same database with different drivers, the most likely result is database corruption ¹.

As long as this package imports github.com/mattn/go-sqlite3 we shouldn't encourage using it with any other drivers.

You can easily make a copy of this, and publish your own Turso/libSQL Gorm driver. Same as was done by @glebarez for modernc.org/sqlite.

If we want to encourage alternative drivers, this package should stop importing github.com/mattn/go-sqlite3 and simply assume the driver is registered elsewhere.

Footnotes

1. the POSIX locking mechanisms SQLite depends on require global variables and if you bundle multiple versions of the library, you'll have multiple copies of those global variables. ↩

[ytsruh]

Thanks a lot, really valuable and I appreciate the feedback. This is my first time playing with SQLite since school so wasn't aware of this.

Given your suggestion would be a breaking change & the maintainers have requested no breaking changes - best course of action is that I split into a new package and publish myself, as you suggested.

If watchers could add a 'thumbs up' to this comment then I'll package this up (I'll replace github.com/mattn/go-sqlite3 package with the Turso!libsql one so it doesn't need to be imported) and publish as a separate Go package on pkg.go.dev

Thanks a lot

[haaawk]

I'll be blunt: this is a bad idea.

The API is fine, although I'm partial to just adding a func OpenConn(conn gorm.ConnPool) and be done with it.

The reason why this is a bad idea is the intended purpose of using alternative drivers. I cannot stress this enough: using multiple implementations of SQLite in the same process is a really, really bad idea. If you, by accident, open the same database with different drivers, the most likely result is database corruption 1.

As long as this package imports github.com/mattn/go-sqlite3 we shouldn't encourage using it with any other drivers.

You can easily make a copy of this, and publish your own Turso/libSQL Gorm driver. Same as was done by @glebarez for modernc.org/sqlite.

If we want to encourage alternative drivers, this package should stop importing github.com/mattn/go-sqlite3 and simply assume the driver is registered elsewhere.

Footnotes

1. the POSIX locking mechanisms SQLite depends on require global variables and if you bundle multiple versions of the library, you'll have multiple copies of those global variables. ↩

I'm not sure this is a problem at all. Neither github.com/tursodatabase/libsql-client-go nor github.com/tursodatabase/go-libsql imports
github.com/mattn/go-sqlite3 or modernc.org/sqlite. Turso uses http to access remote instance of sqlite.

[haaawk]

If you decide to do your own "clone" @ytsruh you can take inspiration in https://github.com/ncruces/go-sqlite3/blob/main/gormlite/sqlite.go

[ncruces]

I'm not sure this is a problem at all. Neither github.com/tursodatabase/libsql-client-go nor github.com/tursodatabase/go-libsql imports github.com/mattn/go-sqlite3 or modernc.org/sqlite. Turso uses http to access remote instance of sqlite.

Then maybe it isn't a problem for Turso, because it only accesses remote databases through HTTP.

But this answer shows exactly why this is a problem: github.com/tursodatabase/go-libsql has a "libsql" driver, with CGO bindings to libSQL.

The fact that Turso doesn't import mattn or modernc is irrelevant. If your users import them (and GORM does), they'll have 3 drivers in their apps: "sqlite3" (mattn), "sqlite" (modernc) and "libsql".

And if they use those drivers to modify the same databases, they'll pretty quickly corrupt them.

[ncruces]

If you decide to do your own "clone" @ytsruh you can take inspiration in https://github.com/ncruces/go-sqlite3/blob/main/gormlite/sqlite.go

It's very similar to this one, as is @glebarez. The only real difference is the error handling.

The situation with locking is very unfortunate, but people working on SQLite bindings and drivers need to put making it hard for users to corrupt data by accident at the very top.

[ytsruh]

Hmm I would rather get some feedback from the maintainer. The idea to use the config struct originally came from Jinzhu (in another thread). I would rather it be part of the original package but I dont want any breaking changes.

I'll give it another week and see if any feedback comes. If not, I'll clone my own and create some docs.

[ytsruh]

Hi all, I've decided to go solo/indie on this one. Package should now be available at https://github.com/ytsruh/gorm-libsql

go get -u github.com/ytsruh/gorm-libsql

I'd still prefer this to be part of the gorm library so if it is merged in then I willa archive my package and point any users back to Gorm. Until that happens, its useable (with docs in the readme).

Thanks everybody for your support/feedback on this

[haaawk]

Thanks for doing this @ytsruh !!!

[xEricL]

Hi all, I've decided to go solo/indie on this one. Package should now be available at https://github.com/ytsruh/gorm-libsql

go get -u github.com/ytsruh/gorm-libsql

I'd still prefer this to be part of the gorm library so if it is merged in then I willa archive my package and point any users back to Gorm. Until that happens, its useable (with docs in the readme).

Thanks everybody for your support/feedback on this

@ytsruh When I run go get -u github.com/ytsruh/gorm-libsql I get the error

go: downloading github.com/ytsruh/gorm-libsql v0.1.1
go: github.com/ytsruh/gorm-libsql@upgrade (v0.1.1) requires github.com/ytsruh/[email protected]: parsing go.mod:
        module declares its path as: gorm.io/driver/sqlite
                but was required as: github.com/ytsruh/gorm-libsql

Also, in your README.md, your import uses "https://":

sqlite "https://github.com/ytsruh/gorm-libsql"

[ytsruh]

Sorry about that! (Both of them) Both fixed now...

... https://pkg.go.dev/github.com/ytsruh/gorm-libsql

[jinzhu]

Sorry for the delay. This PR looks good, so I'll merge it first. If you want your new driver to be added to the GORM documentation, you can submit a PR to gorm.io.

[ncruces]

This PR is just depressing.

Where does libsql-client-go even return an error that's compatible with error_translator.go?

@glebarez has its own error translation, gormlite does the same.

This just does... nothing?

It's like #161: it just ignores the issues, and acts like they don't exist.

Do the extensive Gorm unit tests even pass? No one knows, no one cares, apparently.

[avinassh]

TIL about GORM tests. May be they should be part of this repo's CI? In any case, those can be worked and fixed.

[ncruces]

They are part of GORM's CI.

This GORM driver is tested with the mattn SQLite driver on GORM's CI.

If you want to use another SQLite driver with this GORM driver, you should run tests, like:

• @glebarez: https://github.com/glebarez/sqlite/blob/master/.github/workflows/gorm-tests.yml
• gormlite: https://github.com/ncruces/go-sqlite3/blob/main/gormlite/test.sh

[xEricL]

@jinzhu what are your thoughts on @ncruces's concerns?