Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
Aaron Hopkins <go-sql-driver at die.net>
Achille Roussel <achille.roussel at gmail.com>
Alexey Palazhchenko <alexey.palazhchenko at gmail.com>
Andrew Reid <andrew.reid at tixtrack.com>
Arne Hormann <arnehormann at gmail.com>
Asta Xie <xiemengjun at gmail.com>
Bulat Gaifullin <gaifullinbf at gmail.com>
Expand Down
14 changes: 14 additions & 0 deletions driver_go18_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -796,3 +796,17 @@ func TestRowsColumnTypes(t *testing.T) {
})
}
}

func TestValuerWithValueReceiverGivenNilValue(t *testing.T) {
runTests(t, dsn, func(dbt *DBTest) {
dbt.mustExec("CREATE TABLE test (value VARCHAR(255))")

defer func() {
if r := recover(); r != nil {
dbt.Errorf("Failed to check typed nil before calling valuer with value receiver")
}
}()
Copy link
Member

Choose a reason for hiding this comment

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

Is this recover really necessary?
I don't like it.

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 can leave the test to panic on failure instead - like below. If you prefer that I will alter the PR.

func TestValuerWithValueReceiverGivenNilValue(t *testing.T) {
	runTests(t, dsn, func(dbt *DBTest) {
		dbt.mustExec("CREATE TABLE test (value VARCHAR(255))")
		dbt.db.Exec("INSERT INTO test VALUES (?)", (*testValuer)(nil))
		// the insert will panic if typed nil is not handled before calling a Valuer with a value receiver
	})
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please. It is much informative than manual recovery:

$ go test .
--- FAIL: TestPanic (0.00s)
panic: runtime error: index out of range [recovered]
	panic: runtime error: index out of range

goroutine 5 [running]:
testing.tRunner.func1(0xc4200900f0)
	/usr/local/go/src/testing/testing.go:711 +0x2d2
panic(0x1110920, 0x11d96f0)
	/usr/local/go/src/runtime/panic.go:491 +0x283
_/Users/inada-n/tmp/go-panic-test.TestPanic(0xc4200900f0)
	/Users/inada-n/tmp/go-panic-test/panic_test.go:9 +0x1b
testing.tRunner(0xc4200900f0, 0x113cb30)
	/usr/local/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:789 +0x2de
FAIL	_/Users/inada-n/tmp/go-panic-test	0.009s


dbt.db.Exec("INSERT INTO test VALUES (?)", (*testValuer)(nil))
})
}
41 changes: 37 additions & 4 deletions statement.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,25 @@ func (stmt *mysqlStmt) query(args []driver.Value) (*binaryRows, error) {

type converter struct{}

// ConvertValue mirrors the reference/default converter in database/sql/driver
// with _one_ exception. We support uint64 with their high bit and the default
// implementation does not. This function should be kept in sync with
// database/sql/driver defaultConverter.ConvertValue() except for that
// deliberate difference.
func (c converter) ConvertValue(v interface{}) (driver.Value, error) {
if driver.IsValue(v) {
return v, nil
}

if v != nil {
if valuer, ok := v.(driver.Valuer); ok {
return valuer.Value()
if vr, ok := v.(driver.Valuer); ok {
sv, err := callValuerValue(vr)
if err != nil {
return nil, err
}
if !driver.IsValue(sv) {
return nil, fmt.Errorf("non-Value type %T returned from Value", sv)
}
return sv, nil
}

rv := reflect.ValueOf(v)
Expand All @@ -149,8 +159,9 @@ func (c converter) ConvertValue(v interface{}) (driver.Value, error) {
// indirect pointers
if rv.IsNil() {
return nil, nil
} else {
return c.ConvertValue(rv.Elem().Interface())
}
return c.ConvertValue(rv.Elem().Interface())
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return rv.Int(), nil
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32:
Expand All @@ -176,3 +187,25 @@ func (c converter) ConvertValue(v interface{}) (driver.Value, error) {
}
return nil, fmt.Errorf("unsupported type %T, a %s", v, rv.Kind())
}

var valuerReflectType = reflect.TypeOf((*driver.Valuer)(nil)).Elem()

// callValuerValue returns vr.Value(), with one exception:
// If vr.Value is an auto-generated method on a pointer type and the
// pointer is nil, it would panic at runtime in the panicwrap
// method. Treat it like nil instead.
//
// This is so people can implement driver.Value on value types and
// still use nil pointers to those types to mean nil/NULL, just like
// string/*string.
//
// This is an exact copy of the same-named unexported function from the
// database/sql package.
func callValuerValue(vr driver.Valuer) (v driver.Value, err error) {
if rv := reflect.ValueOf(vr); rv.Kind() == reflect.Ptr &&
rv.IsNil() &&
rv.Type().Elem().Implements(valuerReflectType) {
return nil, nil
}
return vr.Value()
}