database/sql: panic inserting a nil pointer when underlying type implements driver.Valuer

[a13xb]

*What does 'go version' print?*

go version go1.3 darwin/amd64

*What steps reproduce the problem?*

See attached sample. It needs Postgres to run, but another DB should work just as well,
there shouldn't be driver-dependent code involved.

*What happened?*

Panic in case 6 in the sample program.

{{{
panic: value method main.MyType.Value called using nil *MyType pointer

goroutine 16 [running]:
runtime.panic(0x1c1980, 0xc208000960)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/runtime/panic.c:279 +0xf5
main.(*MyType).Value(0x0, 0x0, 0x0, 0x0, 0x0)
    <autogenerated>:1 +0x97
database/sql/driver.defaultConverter.ConvertValue(0x2053a0, 0x0, 0x0, 0x0, 0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/driver/types.go:219 +0x101
database/sql.driverArgs(0x0, 0xc208095f20, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/convert.go:35 +0x1e1
database/sql.(*DB).exec(0xc208054100, 0x28a1f0, 0x1b, 0xc208095f20, 0x1, 0x1, 0x0, 0x0,
0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/sql.go:884 +0x1ec
database/sql.(*DB).Exec(0xc208054100, 0x28a1f0, 0x1b, 0xc208095f20, 0x1, 0x1, 0x0, 0x0,
0x0, 0x0)
    /usr/local/Cellar/go/1.3/libexec/src/pkg/database/sql/sql.go:866 +0xd4
main.main()
    /Users/alex/g/main.go:60 +0x6b5
}}}

*What should have happened instead?*

Behaviour of Exec() when handling nil pointers of values that implement sql.Valuer
should be consistent with primitive values. I can insert a pointer to string, and if
it's nil, it will translate to SQL NULL just fine.

Attachments:

1. main.go (996 bytes)

[griesemer]

Comment 1:

Labels changed: added repo-main.

[rsc]

Even attempting to do this would require package reflect in database/sql, which is a step backward. If you want to use *MyType with this package, then put the Valuer method on *MyType.

[a13xb]

@rsc

Well... database/sql/driver already uses reflect package. In fact, in the very same function that cause the panic (defaultConverter.ConvertValue), so I'm not sure what you are implying.

Also, here is another related problem I've run into:

When I implement Valuer on *MyType instead of MyType, it (obviously) stops working on on non-pointer values. Because you cannot use MyType and *MyType as Valuer at the same time, you cannot have a nullable Valuer fields in frameworks like gorp, where I don't directly control per-field binding code. Well, theoretically you can, but then they all have to be pointers, even non-nullable fields, which is a nuisance, and is not consistent with basic types. E.g. I can have a mix of time.Time and *time.Time fields, but not MyType and *MyType columns.

So my current workaround is to only use standard types in database code, and avoid any uses of Valuer.

[danilobuerger]

@rsc why is this issue closed? @a13xb makes an excellent point. If its implemented on *MyType i can no longer use MyType.

[ianlancetaylor]

Yes, I think @rsc missed something here. The database/sql/driver package already relies on the reflect pkacage. If the value implements Valuer, we need to check whether it is a pointer, whether it is nil, and whether the Value method is implemented on a value rather than on a pointer. This all seems doable. Maybe something like

if rv.Kind() == reflect.Ptr && rv.IsNil && rv.Type().Elem().Implements(reflect.TypeOf(Valuer)) {
    return nil, nil
}

Reopening in case somebody wants to implement this, with a test.

[ConnorFoody]

@ianlancetaylor I think you may have it backwards in your snippet.

It seems like you want the code to check if a pointer to the var takes the Value interface, then try to get that pointer. I believe all the other combos of implemented on a pointer or not / value a pointer or not work with the existing code.

I got this working, with tests, on my machine:

ptrType := reflect.PtrTo(reflect.TypeOf(v))
if ptrType.Implements(reflect.TypeOf((*Valuer)(nil)).Elem()) {

    uPtr := unsafe.Pointer(&v)
    tPtr := reflect.NewAt(ptrType, uPtr)

    return defaultConverter{}.ConvertValue(tPtr.Elem().Interface())
}

I had to use unsafe to create the pointer because it wouldn't let let me do something like:

ptr, ok := (interface{}(&val)).(Valuer)

However, I am a go noobie so I may be missing something. What do you think?

[ianlancetaylor]

@ConnorFoody OK, that is a different perspective. You are suggesting that people should always write a pointer method, and the database/sql package should invent a pointer if necessary.