Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic on format nil *fmt.Stringer type value #1200

Merged
merged 1 commit into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions bind.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,11 @@ func format(tz *time.Location, scale TimeUnit, v any) (string, error) {
}
return fmt.Sprintf("[%s]", val), nil
case fmt.Stringer:
if v := reflect.ValueOf(v); v.Kind() == reflect.Pointer &&
v.IsNil() &&
v.Type().Elem().Implements(reflect.TypeOf((*fmt.Stringer)(nil)).Elem()) {
return "NULL", nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a similar check using database/sql as a reference.
This implementation is also copied go-sql-driver/mysql.
Should I write a source code comment mentioning the reference source?

}
return quote(v.String()), nil
case column.OrderedMap:
values := make([]string, 0)
Expand Down
84 changes: 84 additions & 0 deletions tests/issues/1200_pr_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package issues

import (
"context"
"fmt"
"testing"

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

func Test1200(t *testing.T) {
var (
conn, err = clickhouse_tests.GetConnection("issues", clickhouse.Settings{
"max_execution_time": 60,
"allow_experimental_object_type": true,
}, nil, &clickhouse.Compression{
Method: clickhouse.CompressionLZ4,
})
)
ctx := context.Background()
require.NoError(t, err)
const ddl = "CREATE TABLE test_1200 (id UInt32, null_str Nullable(FixedString(5))) Engine MergeTree() ORDER BY tuple()"
require.NoError(t, conn.Exec(ctx, ddl))
defer func() {
conn.Exec(ctx, "DROP TABLE IF EXISTS test_1200")
}()

v := "value"

tests := []struct {
name string
value fmt.Stringer
want *string
}{
{
name: "fmt.Stringer implemented struct value",
value: Test1200NullStr{underlying: v},
want: &v,
},
{
name: "nil value",
value: nil,
want: nil,
},
{
name: "fmt.Stringer implemented struct pointer value",
value: &Test1200NullStr{underlying: v},
want: &v,
},
{
name: "fmt.Stringer implemented struct typed-nil value",
value: (*Test1200NullStr)(nil),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, panic this case.

--- FAIL: Test1200 (0.03s)
    --- FAIL: Test1200/fmt.Stringer_implemented_struct_typed-nil_value (0.00s)
panic: value method github.com/ClickHouse/clickhouse-go/v2/tests/issues.Test1200NullStr.String called using nil *Test1200NullStr pointer [recovered]
	panic: value method github.com/ClickHouse/clickhouse-go/v2/tests/issues.Test1200NullStr.String called using nil *Test1200NullStr pointer

goroutine 188 [running]:
testing.tRunner.func1.2({0x10084d7a0, 0xc0001ae200})
	/Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
	/Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1548 +0x397
panic({0x10084d7a0?, 0xc0001ae200?})
	/Users/zaneli/sdk/go1.21.1/src/runtime/panic.go:914 +0x21f
github.com/ClickHouse/clickhouse-go/v2/tests/issues.(*Test1200NullStr).String(0xc00008a958?)
	<autogenerated>:1 +0x25
github.com/ClickHouse/clickhouse-go/v2.format(0x1010c8680?, 0x60?, {0x100839d60?, 0x0?})
	/Users/zaneli/go/src/github.com/zaneli/clickhouse-go/bind.go:309 +0x62e
github.com/ClickHouse/clickhouse-go/v2.bindPositional(0x0?, {0x1009644b4, 0x32}, {0xc0000d6160, 0x2, 0x0?})
	/Users/zaneli/go/src/github.com/zaneli/clickhouse-go/bind.go:144 +0x2d4
github.com/ClickHouse/clickhouse-go/v2.bind(0x0?, {0x1009644b4, 0x32}, {0xc0000d6160?, 0x2, 0x2})
	/Users/zaneli/go/src/github.com/zaneli/clickhouse-go/bind.go:93 +0x235
github.com/ClickHouse/clickhouse-go/v2.bindQueryOrAppendParameters(0x0?, 0xc00008ad58?, {0x1009644b4?, 0x32?}, 0x0?, {0xc0000d6160?, 0x2?, 0x2?})
	/Users/zaneli/go/src/github.com/zaneli/clickhouse-go/query_parameters.go:59 +0x16e
github.com/ClickHouse/clickhouse-go/v2.(*connect).exec(0xc000512000, {0x100ac6c00, 0x1010c8680}, {0x1009644b4, 0x32}, {0xc0000d6160, 0x2, 0x2})
	/Users/zaneli/go/src/github.com/zaneli/clickhouse-go/conn_exec.go:30 +0x105
github.com/ClickHouse/clickhouse-go/v2.(*clickhouse).Exec(0x5ad?, {0x100ac6c00, 0x1010c8680}, {0x1009644b4, 0x32}, {0xc0000d6160, 0x2, 0x2})
	/Users/zaneli/go/src/github.com/zaneli/clickhouse-go/clickhouse.go:149 +0x99
github.com/ClickHouse/clickhouse-go/v2/tests/issues.Test1200.func2(0x1010c8680?)
	/Users/zaneli/go/src/github.com/zaneli/clickhouse-go/tests/issues/1200_pr_test.go:61 +0x126
testing.tRunner(0xc000622680, 0xc0003ee4c0)
	/Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 185
	/Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1648 +0x3ad

want: nil,
},
}
for i, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
id := i + 1
err = conn.Exec(ctx, "INSERT INTO test_1200 (id, null_str) VALUES (?, ?)", id, tt.value)
require.NoError(t, err)

var got *string
err = conn.QueryRow(ctx, "SELECT null_str FROM test_1200 WHERE id = ?", id).Scan(&got)
require.NoError(t, err)

if tt.want == nil {
require.Nil(t, got)
} else {
require.NotNil(t, got)
require.Equal(t, *tt.want, *got)
}
})
}
}

type Test1200NullStr struct {
underlying string
}

func (nc Test1200NullStr) String() string {
return nc.underlying
}
Loading