rqlite / gorqlite

A Go client for rqlite, the distributed database built on SQLite
MIT License
149 stars 35 forks source link

Add a compatibility layer with database/sql in the standard library #35

Closed kkoreilly closed 10 months ago

kkoreilly commented 10 months ago

I understand the reasons stated in the readme on why a database/sql driver was not made. However, although directly using a gorqlite-specific API provides a better experience, an additional database/sql driver would significantly broaden the reach of gorqlite by allowing it to be used with other libraries like GORM. In fact, the main reason I wrote this PR is so that I can write a GORM driver for gorqlite.

This PR introduces a simple wrapper library, stdlib, which implements database/sql/driver interfaces. The implementations are trivial wrappers of already existing functions in the main gorqlite package, and no functionality in the main package is lost. This structure is based on jackc/pgx, which is a popular Go PostgreSQL driver.

There are only two changes to the main package in this PR:

  1. The addition of QueryResult.Slice, which is necessary because database/sql/driver.Rows.Next needs to copy the values into a destination slice of driver.Value objects. It is based on QueryResult.Map, and has effectively the same API and code, just with a slice instead of a map.
  2. A simple if statement in Connection.rqliteApiCall which ensures that a nil request body reader is passed for a nil request body. This is necessary because browsers are very strict about having empty bodies on GET requests, so an empty but non-nil buffer results in all gorqlite HTTP GET requests failing in browsers.
kkoreilly commented 10 months ago

Of course. I agree that testing is very important, and I will add tests for all of the new functions and improve the documentation in the README.

kkoreilly commented 10 months ago

I have addressed everything you mentioned. I do not know why the tests are failing; if you look at the history, they were passing before and then stopped passing when I changed the README, which shouldn't have any effect. Looking at the history of the main repository, it seems like the tests have randomly failed a couple of times recently, so there may be some problem making them not deterministic.

otoolep commented 10 months ago

I can dig into the test failure, but it's failing in code you made a change to, right? By its nature of such a failure, it just happened to come out when the README change ran.

=== NAME  TestQueryOne
    query_test.go:41: failed during dropping table: there were 1 statement errors
    query_test.go:44: caught error: there were 1 statement errors
--- FAIL: TestQueryOne (0.01s)
    --- PASS: TestQueryOne/Normal_case (0.00s)
    --- PASS: TestQueryOne/Invalid_Query (0.00s)
    --- FAIL: TestQueryOne/Map_before_next (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
        panic: runtime error: index out of range [0] with length 0

goroutine 24 [running]:
testing.tRunner.func1.2({0x712840, 0xc000296f30})
        /usr/local/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1529 +0x39f
panic({0x712840, 0xc000296f30})
        /usr/local/go/src/runtime/panic.go:884 +0x213
github.com/rqlite/gorqlite.(*QueryResult).Map(0xc000155e80)
        /home/circleci/rqlite/src/github.com/rqlite/gorqlite/query.go:370 +0x3e5
github.com/rqlite/gorqlite_test.TestQueryOne.func4(0xc0001036c0)
        /home/circleci/rqlite/src/github.com/rqlite/gorqlite/query_test.go:199 +0x1f2
testing.tRunner(0xc0001036c0, 0x7617c0)
        /usr/local/go/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:1629 +0x3ea
FAIL    github.com/rqlite/gorqlite      0.023s
=== RUN   TestDisableClusterDiscovery
--- PASS: TestDisableClusterDiscovery (0.00s)
=== RUN   TestEnableClusterDiscovery
--- PASS: TestEnableClusterDiscovery (0.00s)
PASS
ok      github.com/rqlite/gorqlite/integration  0.006s
=== RUN   TestTable
    sql_test.go:68: trying Exec CREATE TABLE
=== RUN   TestTable/Exec_INSERT
=== RUN   TestTable/Query_SELECT
=== RUN   TestTable/Invalid_Query
--- PASS: TestTable (0.02s)
    --- PASS: TestTable/Exec_INSERT (0.01s)
    --- PASS: TestTable/Query_SELECT (0.00s)
    --- PASS: TestTable/Invalid_Query (0.00s)
PASS

Is it possible something you changed has introduced a race into the code?

otoolep commented 10 months ago

I'll run testing on master (looks like it hasn't run in a while, but I know of no failures until now) and check master code.

otoolep commented 10 months ago

I ran testing on master 20+ times, and there was no failure. Check out the results at https://app.circleci.com/pipelines/github/rqlite/gorqlite?branch=master

This implies the failure is due to a change by this PR. I suggest you try to reproduce the failure locally. You'll need to execute the following, in a loop (script it?):

See if you can bring out the failure. That's what I would do. You could do the same with the master branch if you suspect this issue has been here all along.

otoolep commented 10 months ago

Looking at the history of the main repository, it seems like the tests have randomly failed a couple of times recently,

Can you show me where you're seeing that? It may have, but those breakages were most likely addressed if you look at the commit history.

kkoreilly commented 10 months ago

I fixed the tests. The new stdlib tests were using the same table as the main tests, so they were creating, deleting, modifying, and reading the table at the same time. I changed the default table name for the stdlib tests to gorqlite_test_stdlib and added the GORQLITE_TEST_TABLE_STDLIB env variable for changing it.

otoolep commented 10 months ago

Looks good -- thanks for your contribution.

KiddoV commented 4 months ago

How do we use this with GORM? Can I have a sample code? Any limitation when using this with GORM?

kkoreilly commented 4 months ago

Yes, I meant to link to the GORM driver at some point. It is currently here: https://github.com/goki/rqlite, but I might move it somewhere else soon. It is fully functional.

KiddoV commented 4 months ago

Nice! Thank you!