From e4e08e97fe2d1120789b1c88d69d09774be18fc7 Mon Sep 17 00:00:00 2001 From: tania <44262898+taniabogatsch@users.noreply.github.com> Date: Thu, 21 Mar 2024 21:44:52 +0100 Subject: [PATCH] Bump Go version, unify errors and other refactoring (#187) * started unifying errors * nits * trying to fix linter run failure by bumping the go version * %w instead of %s * fix more than one error-wrapping directive * run gofumpt, bump go version everywhere * fix freebsd run * trigger tests * battling CI * revert to 1.21 * revert conn changes * revert comment changes * nit --- .github/workflows/golangci-lint.yml | 4 +- .github/workflows/tests.yaml | 8 ++-- appender.go | 3 +- appender_test.go | 21 ++++++---- connection.go | 14 +++---- duckdb.go | 64 ++++++++++++----------------- duckdb_test.go | 11 ----- errors.go | 30 ++++++++++++++ errors_test.go | 28 +++++++++++++ go.mod | 2 +- go.sum | 5 +++ rows.go | 1 - statement.go | 6 +-- 13 files changed, 120 insertions(+), 77 deletions(-) create mode 100644 errors.go create mode 100644 errors_test.go diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index 742252b2..fd63b922 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -10,12 +10,12 @@ jobs: name: lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: actions/setup-go@v4 with: go-version: '1.21' cache: false - name: golangci-lint - uses: golangci/golangci-lint-action@v3 + uses: golangci/golangci-lint-action@v4 with: version: latest \ No newline at end of file diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 0cded2b5..515ab3ed 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -13,22 +13,22 @@ jobs: strategy: matrix: os: [ubuntu-latest, macos-12] - go: ["1.18"] + go: ["1.21"] fail-fast: false steps: - - uses: actions/setup-go@v2 + - uses: actions/setup-go@v4 with: go-version: ${{ matrix.go }} - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: "Run Tests" run: make test freebsd_amd64: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: vmactions/freebsd-vm@v1 with: usesh: true diff --git a/appender.go b/appender.go index d4eca539..d9e98213 100644 --- a/appender.go +++ b/appender.go @@ -107,7 +107,7 @@ func NewAppenderFromConn(driverConn driver.Conn, schema, table string) (*Appende defer C.free(unsafe.Pointer(cTable)) var appender C.duckdb_appender - state := C.duckdb_appender_create(*c.con, cSchema, cTable, &appender) + state := C.duckdb_appender_create(c.duckdbCon, cSchema, cTable, &appender) if state == C.DuckDBError { // We'll destroy the error message when destroying the appender. @@ -242,7 +242,6 @@ func initPrimitive[T any](duckdbType C.duckdb_type) colInfo { } func (a *Appender) initColInfos(logicalType C.duckdb_logical_type, colIdx int) (colInfo, error) { - duckdbType := C.duckdb_get_type_id(logicalType) switch duckdbType { diff --git a/appender_test.go b/appender_test.go index d511dfd8..6378ec23 100644 --- a/appender_test.go +++ b/appender_test.go @@ -5,11 +5,11 @@ import ( "database/sql" "database/sql/driver" "fmt" - "github.com/google/uuid" "math/rand" "testing" "time" + "github.com/google/uuid" "github.com/mitchellh/mapstructure" "github.com/stretchr/testify/require" ) @@ -369,8 +369,10 @@ func TestAppenderNested(t *testing.T) { rows[i].wrappedStruct = wrappedStruct{"wrapped", simpleStruct{1, "foo"}} rows[i].doubleWrappedStruct = doubleWrappedStruct{ "so much nesting", - wrappedStruct{"wrapped", - simpleStruct{1, "foo"}}, + wrappedStruct{ + "wrapped", + simpleStruct{1, "foo"}, + }, } rows[i].structList = []simpleStruct{{1, "a"}, {2, "b"}, {3, "c"}} rows[i].structWithList.L = []int32{6, 7, 8} @@ -569,8 +571,10 @@ func TestAppenderNestedNullStruct(t *testing.T) { err := appender.AppendRow(doubleWrappedStruct{ "so much nesting", - wrappedStruct{"wrapped", - simpleStruct{1, "foo"}}, + wrappedStruct{ + "wrapped", + simpleStruct{1, "foo"}, + }, }) require.NoError(t, err) @@ -580,8 +584,10 @@ func TestAppenderNestedNullStruct(t *testing.T) { err = appender.AppendRow(doubleWrappedStruct{ "now we are done nesting NULLs", - wrappedStruct{"unwrap", - simpleStruct{21, "bar"}}, + wrappedStruct{ + "unwrap", + simpleStruct{21, "bar"}, + }, }) require.NoError(t, err) @@ -1016,7 +1022,6 @@ func TestAppenderUint8SliceTinyInt(t *testing.T) { } func TestAppenderUnsupportedType(t *testing.T) { - connector, err := NewConnector("", nil) require.NoError(t, err) diff --git a/connection.go b/connection.go index bcb80a8c..63ae13d7 100644 --- a/connection.go +++ b/connection.go @@ -16,9 +16,9 @@ import ( ) type conn struct { - con *C.duckdb_connection - closed bool - tx bool + duckdbCon C.duckdb_connection + closed bool + tx bool } func (c *conn) CheckNamedValue(nv *driver.NamedValue) error { @@ -146,7 +146,7 @@ func (c *conn) Close() error { } c.closed = true - C.duckdb_disconnect(c.con) + C.duckdb_disconnect(&c.duckdbCon) return nil } @@ -156,7 +156,7 @@ func (c *conn) prepareStmt(cmd string) (*stmt, error) { defer C.free(unsafe.Pointer(cmdstr)) var s C.duckdb_prepared_statement - if state := C.duckdb_prepare(*c.con, cmdstr, &s); state == C.DuckDBError { + if state := C.duckdb_prepare(c.duckdbCon, cmdstr, &s); state == C.DuckDBError { dbErr := C.GoString(C.duckdb_prepare_error(s)) C.duckdb_destroy_prepare(&s) return nil, errors.New(dbErr) @@ -170,7 +170,7 @@ func (c *conn) extractStmts(query string) (C.duckdb_extracted_statements, C.idx_ defer C.free(unsafe.Pointer(cquery)) var stmts C.duckdb_extracted_statements - stmtsCount := C.duckdb_extract_statements(*c.con, cquery, &stmts) + stmtsCount := C.duckdb_extract_statements(c.duckdbCon, cquery, &stmts) if stmtsCount == 0 { err := C.GoString(C.duckdb_extract_statements_error(stmts)) C.duckdb_destroy_extracted(&stmts) @@ -185,7 +185,7 @@ func (c *conn) extractStmts(query string) (C.duckdb_extracted_statements, C.idx_ func (c *conn) prepareExtractedStmt(extractedStmts C.duckdb_extracted_statements, index C.idx_t) (*stmt, error) { var s C.duckdb_prepared_statement - if state := C.duckdb_prepare_extracted_statement(*c.con, extractedStmts, index, &s); state == C.DuckDBError { + if state := C.duckdb_prepare_extracted_statement(c.duckdbCon, extractedStmts, index, &s); state == C.DuckDBError { dbErr := C.GoString(C.duckdb_prepare_error(s)) C.duckdb_destroy_prepare(&s) return nil, errors.New(dbErr) diff --git a/duckdb.go b/duckdb.go index 53e7f321..bbbce698 100644 --- a/duckdb.go +++ b/duckdb.go @@ -14,7 +14,6 @@ import ( "context" "database/sql" "database/sql/driver" - "errors" "fmt" "net/url" "strings" @@ -28,11 +27,11 @@ func init() { type Driver struct{} func (d Driver) Open(dsn string) (driver.Conn, error) { - connector, err := d.OpenConnector(dsn) + c, err := d.OpenConnector(dsn) if err != nil { return nil, err } - return connector.Connect(context.Background()) + return c.Connect(context.Background()) } func (Driver) OpenConnector(dsn string) (driver.Connector, error) { @@ -42,14 +41,14 @@ func (Driver) OpenConnector(dsn string) (driver.Connector, error) { } // NewConnector opens a new Connector for a DuckDB database. -// The user must close the returned Connector, if it is not passed to the sql.OpenDB function. +// The user must close the Connector, if it is not passed to the sql.OpenDB function. // Otherwise, sql.DB closes the Connector when calling sql.DB.Close(). func NewConnector(dsn string, connInitFn func(execer driver.ExecerContext) error) (*Connector, error) { var db C.duckdb_database parsedDSN, err := url.Parse(dsn) if err != nil { - return nil, fmt.Errorf("%w: %s", errParseDSN, err.Error()) + return nil, getError(errParseDSN, err) } config, err := prepareConfig(parsedDSN) @@ -58,14 +57,14 @@ func NewConnector(dsn string, connInitFn func(execer driver.ExecerContext) error } defer C.duckdb_destroy_config(&config) - connStr := C.CString(extractConnectionString(dsn)) + connStr := C.CString(getConnString(dsn)) defer C.free(unsafe.Pointer(connStr)) - var errOpenMsg *C.char - defer C.duckdb_free(unsafe.Pointer(errOpenMsg)) + var outError *C.char + defer C.duckdb_free(unsafe.Pointer(outError)) - if state := C.duckdb_open_ext(connStr, &db, config, &errOpenMsg); state == C.DuckDBError { - return nil, fmt.Errorf("%w: %s", errOpen, C.GoString(errOpenMsg)) + if state := C.duckdb_open_ext(connStr, &db, config, &outError); state == C.DuckDBError { + return nil, getError(errOpen, getDuckDBError(outError)) } return &Connector{ @@ -79,26 +78,25 @@ type Connector struct { connInitFn func(execer driver.ExecerContext) error } -func (c *Connector) Driver() driver.Driver { +func (*Connector) Driver() driver.Driver { return Driver{} } func (c *Connector) Connect(context.Context) (driver.Conn, error) { - var con C.duckdb_connection - if state := C.duckdb_connect(c.db, &con); state == C.DuckDBError { - return nil, errOpen + var duckdbCon C.duckdb_connection + if state := C.duckdb_connect(c.db, &duckdbCon); state == C.DuckDBError { + return nil, getError(errConnect, nil) } - conn := &conn{con: &con} + con := &conn{duckdbCon: duckdbCon} - // Call the connection init function if defined if c.connInitFn != nil { - if err := c.connInitFn(conn); err != nil { + if err := c.connInitFn(con); err != nil { return nil, err } } - return conn, nil + return con, nil } func (c *Connector) Close() error { @@ -107,27 +105,26 @@ func (c *Connector) Close() error { return nil } -func extractConnectionString(dsn string) string { - var queryIndex = strings.Index(dsn, "?") - if queryIndex < 0 { - queryIndex = len(dsn) +func getConnString(dsn string) string { + idx := strings.Index(dsn, "?") + if idx < 0 { + idx = len(dsn) } - - return dsn[0:queryIndex] + return dsn[0:idx] } func prepareConfig(parsedDSN *url.URL) (C.duckdb_config, error) { var config C.duckdb_config if state := C.duckdb_create_config(&config); state == C.DuckDBError { C.duckdb_destroy_config(&config) - return nil, errCreateConfig + return nil, getError(errCreateConfig, nil) } - if err := setConfig(config, "duckdb_api", "go"); err != nil { + if err := setConfigOption(config, "duckdb_api", "go"); err != nil { return nil, err } - // early-out + // Early-out, if the DSN does not contain configuration options. if len(parsedDSN.RawQuery) == 0 { return config, nil } @@ -136,7 +133,7 @@ func prepareConfig(parsedDSN *url.URL) (C.duckdb_config, error) { if len(v) == 0 { continue } - if err := setConfig(config, k, v[0]); err != nil { + if err := setConfigOption(config, k, v[0]); err != nil { return nil, err } } @@ -144,7 +141,7 @@ func prepareConfig(parsedDSN *url.URL) (C.duckdb_config, error) { return config, nil } -func setConfig(config C.duckdb_config, name string, option string) error { +func setConfigOption(config C.duckdb_config, name string, option string) error { cName := C.CString(name) defer C.free(unsafe.Pointer(cName)) @@ -154,15 +151,8 @@ func setConfig(config C.duckdb_config, name string, option string) error { state := C.duckdb_set_config(config, cName, cOption) if state == C.DuckDBError { C.duckdb_destroy_config(&config) - return fmt.Errorf("%w: %s=%s is not a global config option or does not exist", errSetConfig, name, option) + return getError(errSetConfig, fmt.Errorf("%s=%s", name, option)) } return nil } - -var ( - errOpen = errors.New("could not open database") - errParseDSN = errors.New("could not parse DSN for database") - errCreateConfig = errors.New("could not create config for database") - errSetConfig = errors.New("could not set config option for database") -) diff --git a/duckdb_test.go b/duckdb_test.go index 888e194c..6bcbdabd 100644 --- a/duckdb_test.go +++ b/duckdb_test.go @@ -51,16 +51,6 @@ func TestOpen(t *testing.T) { require.NoError(t, res.Scan(&species)) require.Equal(t, "Gopher", species) }) - - t.Run("with invalid config", func(t *testing.T) { - _, err := sql.Open("duckdb", "?threads=NaN") - require.Contains(t, err.Error(), "is not a global config option or does not exist") - }) - - t.Run("with connection-local config", func(t *testing.T) { - _, err := sql.Open("duckdb", "?schema=main") - require.Contains(t, err.Error(), "is not a global config option or does not exist") - }) } func TestConnectorBootQueries(t *testing.T) { @@ -677,7 +667,6 @@ func TestBlob(t *testing.T) { } func TestTimestampTZ(t *testing.T) { - t.Parallel() db := openDB(t) defer db.Close() diff --git a/errors.go b/errors.go new file mode 100644 index 00000000..88bb474e --- /dev/null +++ b/errors.go @@ -0,0 +1,30 @@ +package duckdb + +import "C" +import ( + "errors" + "fmt" +) + +func getError(errDriver error, err error) error { + if err == nil { + return fmt.Errorf("%s: %w", driverErrMsg, errDriver) + } + return fmt.Errorf("%s: %w: %s", driverErrMsg, errDriver, err.Error()) +} + +func getDuckDBError(err *C.char) error { + return errors.New(C.GoString(err)) +} + +const driverErrMsg = "database/sql/driver" + +var ( + errParseDSN = errors.New("could not parse DSN for database") + errOpen = errors.New("could not open database") + errSetConfig = errors.New("could not set invalid or local option for global database config") + + // Errors not covered in tests. + errConnect = errors.New("could not connect to database") + errCreateConfig = errors.New("could not create config for database") +) diff --git a/errors_test.go b/errors_test.go new file mode 100644 index 00000000..d0971da5 --- /dev/null +++ b/errors_test.go @@ -0,0 +1,28 @@ +package duckdb + +import ( + "database/sql" + "testing" + + "github.com/stretchr/testify/require" +) + +var testErrOpenMap = map[string]string{ + errParseDSN.Error(): ":mem ory:", + errOpen.Error(): "?readonly", + errSetConfig.Error(): "?threads=NaN", +} + +func TestErrOpen(t *testing.T) { + for errMsg, dsn := range testErrOpenMap { + t.Run(errMsg, func(t *testing.T) { + _, err := sql.Open("duckdb", dsn) + require.Contains(t, err.Error(), errMsg) + }) + } + + t.Run("local config option", func(t *testing.T) { + _, err := sql.Open("duckdb", "?schema=main") + require.Contains(t, err.Error(), errSetConfig.Error()) + }) +} diff --git a/go.mod b/go.mod index 5cbb0b56..0b1eceda 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/marcboeker/go-duckdb -go 1.18 +go 1.21 require ( github.com/apache/arrow/go/v14 v14.0.2 diff --git a/go.sum b/go.sum index 92bb61f8..58903a2d 100644 --- a/go.sum +++ b/go.sum @@ -17,6 +17,7 @@ github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORN github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= +github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/mitchellh/mapstructure v1.5.0 h1:jeMsZIYE/09sWLaz43PL7Gy6RuMjD2eJVyuac5Z2hdY= github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= github.com/pierrec/lz4/v4 v4.1.18 h1:xaKrnTkyoqfh1YItXl56+6KJNVYWlEEPuAQW9xsplYQ= @@ -26,12 +27,15 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ= +github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0= github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0= github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA= golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/mod v0.13.0 h1:I/DsJXRlw/8l/0c24sM9yb0T4z9liZTduXvdAWYiysY= golang.org/x/mod v0.13.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/sync v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ= +golang.org/x/sync v0.4.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE= golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= @@ -40,6 +44,7 @@ golang.org/x/tools v0.14.0/go.mod h1:uYBEerGOWcJyEORxN+Ek8+TT266gXkNlHdJBwexUsBg golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 h1:H2TDz8ibqkAF6YGhCdN3jS9O0/s90v0rJh3X/OLHEUk= golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2/go.mod h1:K8+ghG5WaK9qNqU5K3HdILfMLy1f3aNYFI/wnl100a8= gonum.org/v1/gonum v0.12.0 h1:xKuo6hzt+gMav00meVPUlXwSdoEJP46BR+wdxQEFK2o= +gonum.org/v1/gonum v0.12.0/go.mod h1:73TDxJfAAHeA8Mk9mf8NlIppyhQNo5GLTcYeqgo2lvY= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/rows.go b/rows.go index 5217380e..35159304 100644 --- a/rows.go +++ b/rows.go @@ -95,7 +95,6 @@ func scanValue(vector C.duckdb_vector, rowIdx C.idx_t) (any, error) { } func scan(vector C.duckdb_vector, rowIdx C.idx_t) (any, error) { - // FIXME: implement support for these types: // DUCKDB_TYPE_UHUGEINT // DUCKDB_TYPE_UNION diff --git a/statement.go b/statement.go index 7bf51ff6..99e85c78 100644 --- a/statement.go +++ b/statement.go @@ -210,7 +210,7 @@ func (s *stmt) execute(ctx context.Context, args []driver.NamedValue) (*C.duckdb go func() { select { case <-ctx.Done(): - C.duckdb_interrupt(*s.c.con) + C.duckdb_interrupt(s.c.duckdbCon) close(bgDoneCh) return case <-mainDoneCh: @@ -249,6 +249,4 @@ func argsToNamedArgs(values []driver.Value) []driver.NamedValue { return args } -var ( - errCouldNotBind = errors.New("could not bind parameter") -) +var errCouldNotBind = errors.New("could not bind parameter")