ConvertValue() panics when given a nil pointer to a type that implements driver.Valuer with a value receiver

[yareid]

Issue description

Given a type that implements driver.Valuer using a value receiver. (e.g. this one). Then passing a nil pointer to that type as an argument to db.Exec will cause mysql.converter.ConvertValue() to panic.

This is a regression. Previously this situation would result in a NULL argument.

Analysis

There is a lengthy discussion of the same issue (resolved some time ago) in database/sql here with their fix here.

I think the problem is the test here added in #710. The interface is not nil and the type assertion succeeds, resulting in a call to Value() with a nil receiver which is not okay because Value() was implemented on the type with a value receiver not a pointer receiver.

Example code

Add the following failing case to driver_test.go TestValuerWithValidation() here.

if _, err := dbt.db.Exec("INSERT INTO testValuer VALUES (?)", (*testValuerWithValidation)(nil)); err != nil {
	dbt.Errorf("Failed to check typed nil")
}

Error log

--- FAIL: TestValuerWithValidation (0.03s)
panic: value method github.com/go-sql-driver/mysql.testValuerWithValidation.Value called using nil *testValuerWithValidation pointer [recovered]
	panic: value method github.com/go-sql-driver/mysql.testValuerWithValidation.Value called using nil *testValuerWithValidation pointer

goroutine 836 [running]:
testing.tRunner.func1(0xc42011c3c0)
	/usr/local/Cellar/go/1.9.2/libexec/src/testing/testing.go:711 +0x2d2
panic(0x1270900, 0xc4202df190)
	/usr/local/Cellar/go/1.9.2/libexec/src/runtime/panic.go:491 +0x283
github.com/go-sql-driver/mysql.(*testValuerWithValidation).Value(0x0, 0x126c480, 0x0, 0x240a298, 0x0)
	<autogenerated>:1 +0x7c
github.com/go-sql-driver/mysql.converter.ConvertValue(0x126c480, 0x0, 0x14b2458, 0x0, 0x9, 0x1000)
	/Users/AndrewReid/Code/go/src/github.com/go-sql-driver/mysql/statement.go:142 +0x8d4
github.com/go-sql-driver/mysql.(*mysqlConn).CheckNamedValue(0xc4201f35c0, 0xc4203277d0, 0x1280801, 0x20a0028)
	/Users/AndrewReid/Code/go/src/github.com/go-sql-driver/mysql/connection_go18.go:200 +0x3c
database/sql/driver.(NamedValueChecker).CheckNamedValue-fm(0xc4203277d0, 0x0, 0x0)
	/usr/local/Cellar/go/1.9.2/libexec/src/database/sql/convert.go:180 +0x39

Configuration

Driver version (or git SHA): 2cc627a

Go version: go1.9.2 darwin/amd64

Server version: MySQL 5.7.19

Server OS: OSX 10.11.6 and also Ubuntu Trusty (Travis-CI)

[methane]

Why do you think it is bug?
nil is valid value.

type MyType struct {
    v string
};
func (m *MyType) Value() (driver.Value, error) {
    if m == nil {  // *MyType supports nil pointer for default value
        return "UNDEFINED", nil
    }
    return m.v, nil
}

If custom type doesn't support nil, it's callers responsibility to avoid passing nil.
Am I wrong?

[yareid]

Why do you think it is a bug?

The argument that was accepted for fixing this issue in database/sql was that the handling of non-builtin types should mirror the handling for builtins. If (*string)(nil) converts to NULL it is inconsistent to panic on (*mytype)(nil).

Also I would add:

• This is a recent change in driver behaviour that appears to be unintentional.
• This appears to be an unintentional difference from defaultConverter.ConvertValue() (The only intentional difference appears to be handling of large uint64 values)

I'm not convinced I fully understand the flow from NamedValueChecker to ColumnConverter to DefaultValueConverter and how this varies between prepared and non prepared statements. But it seems likely that #690 and subsequent patches #708, #709 and #710 moved handling for my case from database/sql to go-sql-driver/mysql and that this is a side effect of that change that should be resolved so that the only difference in behaviour from the default database/sql ConvertValue() is that intended for uint64.

If custom type doesn't support nil, it's callers responsibility to avoid passing nil.

True, the type does not natively support nil, and as such Value() has a value receiver not a pointer receiver. But I definitely do want to store a NULLable column of that type. A generic client based solution intercepting all calls to db.Exec (or db.Query etc) and implementing my own ColumnConverter is doable. But given that this appears to be a regression from the database/sql default Converter I would rather fix it here.

If the correct solution is to more fully align the default converter with the go-sql-driver converter then I can propose a PR that does that. I am wary however. It would feel more correct to me to only handle the unit64 exception case in go-sql-driver and fall back to the default converter in database/sql for all other cases. I don't know if that is reasonable/possible.

[yareid]

I've fixed this two different ways

99f906a is a more conservative change. It merely updates (copy and paste) ConvertValue to match that provided by database/sql except for the case of uint64 with their high bit set. This is probably the more acceptable approach.

e167565 is a more ambitious change. It modifies ConvertValue to return ErrSkip except in cases needed to preserve handling of uint64 values with their high bit set. As a result the default converter provided by database/sql is used for nearly all conversions. The conversion test cases needed to replaced for this approach.

@methane or @julienschmidt - I'd appreciate some feedback on whether you'd accept a PR following either of these approaches. If so I will prepare one. Thanks.

[methane]

If (*string)(nil) converts to NULL it is inconsistent to panic on (*mytype)(nil).

I don't think so because *string doesn't have ConvertValue().

True, the type does not natively support nil, and as such Value() has a value receiver not a pointer receiver. But I definitely do want to store a NULLable column of that type.

But you can pass untyped nil for it?

[methane]

I feel typed nil is:

• Valid value which methods support nil receiver, or
• Invalid value created by Go's pitfall (implicit type conversion)

For former case, we should call method.
For later case, we should panic to people notice their bug.
So I don't think current behavior is bad.

What is rational for using typed nil instead of untyped nil when the type doesn't support nil receiver?

[yareid]

What is rational for using typed nil instead of untyped nil when the type doesn't support nil receiver?

We are using a 3rd party package that defines the type and the Valuer. In our code we have a struct that amongst other things has a field that is a pointer to that type. It is a pointer primarily because for us, not setting a value is valid.

Our struct may be persisted to MySQL. When the field in question is nil, we model it in MySQL as NULL. We pass the struct fields to db.Exec which accepts them as args ...interface{} encapsulating them all in interface{} values.

For any pointer that is nil whose type does not implement driver.Valuer this intuitively stores NULL. For any pointer that is nil whose type happens to implement driver.Valuer with a value receiver that now/newly panics!

So I don't think current behaviour is bad.

If the above argument doesn't work for you then I think the next easiest argument that the current behaviour bad is that it is not deliberate:

1. The panic is recently-changed behaviour. And there is no evidence the change was deliberate.
2. The panic is a difference in behaviour from the reference ConvertValue() behaviour provided by database/sql. Again there is no evidence this difference is deliberate. The simplicity of 99f906a shows the current difference.

[methane]

We are using a 3rd party package that defines the type and the Valuer. In our code we have a struct that amongst other things has a field that is a pointer to that type. It is a pointer primarily because for us, not setting a value is valid.

Why don't you ask the library to support nil receiver?
If the package is not aimed for used as pointer, you can wrap that type.

type YourCustomType ThirdPartyType

func (t *YourCustomType) Value() (driver.Value(), error) {
    if t == nil {
        return nil, nil
    }
    return ThirdPartyType(*t).Value()
}

And there is no evidence the change was deliberate.

I'm not interested in difference is deliberate or not.
We fix many thing, and many wrong behavior will be fixed by intentionally or unintentionally.
So I'm interested in what is correct behavior.

The panic is a difference in behaviour from the reference ConvertValue() behaviour provided by database/sql.

I'm sorry for missing it. I'm not good at English and your article is too long to read carefully.
Now I feel you're right, but I have no time for thinking about it for now.

[methane]

e167565 is a more ambitious change. It modifies ConvertValue to return ErrSkip except in cases needed to preserve handling of uint64 values with their high bit set. As a result the default converter provided by database/sql is used for nearly all conversions. The conversion test cases needed to replaced for this approach.

OK, I like this idea.

[yareid]

I'm sorry for missing it. I'm not good at English and your article is too long to read carefully.

Thanks for your effort. I appreciate it. I've been using go-sql-driver/mysql for two years now. This is the first time I have encountered any problems at all when upgrading - which is quite remarkable.

I have raised #746 for review.