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

[yareid]

Description

This addresses #739 by copying across more recent changes to ConvertValue() from the reference implementation in database/sql/driver here. The important change is the use of callValuerValue which checks for nil before calling a Valuer implemented with value receiver. This was added to database/sql/driver for go 1.8 in this commit

This PR leaves the only difference from the reference implementation being the handling of uint64 with their high bit set.

The test added to driver_go18_test.go will fail for go 1.9 and 1.10 without this change. The code is not exercised for go 1.8 as it pre-dates #690. The test cannot pass for go 1.7 as that pre-dates the relevant changes in database/sql - which is why the test is added to driver_go18_test.go.

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary
• Added myself / the copyright holder to the AUTHORS file

[tdewolff]

This could be simplified, as far as I can see, to the following diff below, at line https://github.com/go-sql-driver/mysql/pull/760/files#diff-9a45f603848db9e865d7a2a24011038cL140

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 {
+       if valuer, ok := v.(driver.Valuer); ok {
+               rv := reflect.ValueOf(valuer)
+               if rv.Kind() != reflect.Ptr || !rv.IsNil() {
                        return valuer.Value()
                }
        }

Firstly, v != nil is already handled above since driver.IsValue(v) returns true. So if the interface itself is not nil, the value of the interface still could be nil and panic on valuer.Value(). Pointer types are then handled in the switch rv.Kind(), which recursively calls ConvertValue again.

[julienschmidt]

Please also describe what changes where made / why it was copied and how it should be synchronized with database/sql

[yareid]

@julienschmidt - I've added additional comments - let me know if those work for you.

@tdewolff - I prefer to more closely mirror the form of these functions in database/sql/driver. It makes the comparison easier which should make synchronising any future changes easier.

[methane]

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

[yareid]

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

[methane]

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

[julienschmidt]

Thanks, LGTM now.

[yareid]

I've modified the test as requested by @methane and checked it still passes/fails as expected with/without the code changes.