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

@@ -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

.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

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 {

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)
 

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)

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,15 +133,15 @@ 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
 		}
 	}
 
 	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")
-)

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()

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")
+)