Skip to content

Update ConvertValue() to match the database/sql/driver implementation except for uint64 #760

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

Merged
merged 3 commits into from
Mar 8, 2018
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()
}