v2.25.0 breaks sql.Scanner implementation

[IlyushaZ]

Observed

I am using stdlib interface to interact with Clickhouse. I have a table with Nullable(String) column which stores JSON objects. To unmarshal JSON when retrieving data from table I am using custom type that implements sql.Scanner interface. After upgrading from v2.23.0 to v2.26.0 I noticed that my program started failing because of incorrect data type passed to Scan method.

I assume that is because *stdRows now populates dest with string instead of *string in case when column with type Nullable(String) has a non-null value in it. That behavior was changed in v2.25.0 (in this PR in particular) so that the driver could work with standard library.

However this update has broken the code that had already been written assuming that *string is passed as an argument to sql.Scanner’s Scan method. It’s a common practice to use pointers to deal with nullable columns.

There's example of code that reproduces the issue below. It will work as I am expecting only using version older than v2.25.0.

Expected behaviour

JSON object will be unmarshalled successfully.

Code example

package main

import (
	"database/sql/driver"
	"encoding/json"
	"fmt"

	"github.com/ClickHouse/clickhouse-go/v2"
)

type SomeStruct struct {
	SomeField string `json:"some_field"`
}

// SomeStructs implements sql.Scanner and driver.Valuer interfaces.
// We want to save slice as a JSON object to clickhouse. We're using Nullable(String) for this purpose.
type SomeStructs []SomeStruct

func (ss *SomeStructs) Scan(src any) error {
	sp, ok := src.(*string)
	if !ok {
		return fmt.Errorf("expected *string, got %T", src)
	}

	return json.Unmarshal([]byte(*sp), ss)
}

func (ss SomeStructs) Value() (driver.Value, error) {
	if ss == nil {
		return nil, nil
	}

	marshalled, err := json.Marshal(ss)
	if err != nil {
		return nil, err
	}

	return string(marshalled), nil
}

func main() {
	conn := clickhouse.OpenDB(&clickhouse.Options{
		Addr: []string{"127.0.0.1:9000"},
		Auth: clickhouse.Auth{
			Database: "default",
			Username: "default",
			Password: "",
		},
	})

	// MergeTree engine requires non-nullable column to be sorting key.
	// It doesn't matter in our case, we're only interested in json_field.
	_, err := conn.Exec(`
		create table if not exists test_table (
			id UInt64,
			json_field Nullable(String)
		) engine = MergeTree ORDER BY id
	`)
	if err != nil {
		panic(fmt.Sprintf("can't create table: %v", err))
	}

	toInsert := SomeStructs{
		{SomeField: "value1"},
		{SomeField: "value2"},
	}
	_, err = conn.Exec("insert into test_table (id, json_field) values (?, ?)", 1, toInsert)
	if err != nil {
		panic(fmt.Sprintf("can't insert data: %v", err))
	}

	var fromDB SomeStructs

	row := conn.QueryRow("select json_field from test_table order by id desc limit 1")
	if err := row.Scan(&fromDB); err != nil {
		panic(fmt.Sprintf("can't scan data from table: %v", err))
	}

	// this code will not be reached starting with [email protected]
	fmt.Printf("selected data: %v\n", fromDB)
}

Error log

panic: can't scan data from table: sql: Scan error on column index 0, name "json_field": expected *string, got string

goroutine 1 [running]:
main.main()
        /Users/ilyaz/click-example/main.go:76 +0x260
exit status 2

Environment

• clickhouse-go version: v2.26.0
• Interface: database/sql compatible driver
• Go version: 1.22.2
• Operating system: MacOS as well as Linux
• ClickHouse version: 23.8.12.13
• Is it a ClickHouse Cloud? No
• ClickHouse Server non-default settings, if any:
• CREATE TABLE statements for tables involved: presented in the example above
• Sample data for all these tables, use clickhouse-obfuscator if necessary

[jkaflik]

Hi @IlyushaZ

Thanks for submitting. In fact, the fix provided in #1306 is kinda hacky, and it will have to be reverted. I am looking for an alternative way to get previous bug fixed.

[jkaflik]

Hi @IlyushaZ

At the moment, I haven't come up with any solution that will enable us to pass a pointer to database/sql to make it work compatible with Scan function.

I was looking into other database/sql drivers and none of these has been returning a pointer to the data type via driver.Value. Eventually, nil is returned but cast to any type.

Indeed, your code can work, but if we don't expect a pointer, but value:

package issues

import (
	"database/sql/driver"
	"encoding/json"
	"fmt"
	"testing"

	clickhouse_tests "github.com/ClickHouse/clickhouse-go/v2/tests"
	"github.com/stretchr/testify/require"
)

type SomeStruct struct {
	SomeField string `json:"some_field"`
}

// SomeStructs implements sql.Scanner and driver.Valuer interfaces.
// We want to save slice as a JSON object to clickhouse. We're using Nullable(String) for this purpose.
type SomeStructs []SomeStruct

func (ss *SomeStructs) Scan(src any) error {
	if src == nil {
		*ss = nil
		return nil
	}

	sp, ok := src.(string)
	if !ok {
		return fmt.Errorf("expected *string, got %T", src)
	}

	return json.Unmarshal([]byte(sp), ss)
}

func (ss SomeStructs) Value() (driver.Value, error) {
	if ss == nil {
		return nil, nil
	}

	marshalled, err := json.Marshal(ss)
	if err != nil {
		return nil, err
	}

	return string(marshalled), nil
}

func Test1359(t *testing.T) {
	testEnv, err := clickhouse_tests.GetTestEnvironment("issues")
	require.NoError(t, err)
	conn, err := clickhouse_tests.TestDatabaseSQLClientWithDefaultSettings(testEnv)
	require.NoError(t, err)

	_, err = conn.Exec(`CREATE TABLE test_1359 (
			  NStr Nullable(String)
		) Engine MergeTree() ORDER BY tuple()`)
	require.NoError(t, err)
	t.Cleanup(func() {
		_, _ = conn.Exec("DROP TABLE test_1359")
	})

	toInsert := SomeStructs{
		{SomeField: "value1"},
		{SomeField: "value2"},
	}
	_, err = conn.Exec("insert into test_1359 (NStr) values (?)", toInsert)
	require.NoError(t, err)

	var fromDB SomeStructs

	row := conn.QueryRow("select NStr from test_1359 limit 1")
	require.NoError(t, row.Scan(&fromDB))
}

Happy to hear proposals thought. I will update changelog with a breaking change note.

[jkaflik]

Feel free to continue the discussion on this issue. I will close it for now.