go
go copied to clipboard
golang
database/sql: sql.Null[T].Value method does not recognize T implementing driver.Valuer interface itself
Go version
1.22 / 1.23
Output of go env in your module/workspace:
GO111MODULE='on'
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/silvio-ginter/.cache/go-build'
GOENV='/home/silvio-ginter/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/silvio-ginter/go/pkg/mod'
GONOPROXY=''
GONOSUMDB='*'
GOOS='linux'
GOPATH='/home/silvio-ginter/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/silvio-ginter/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/silvio-ginter/workspace/playground/sql-null/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2644935471=/tmp/go-build -gno-record-gcc-switches'
What did you do?
I wanted to wrap a type, that itself implements database/sql.Scanner / database/sql/driver.Valuer interfaces, in sq.Null[T]. While scanning values using the wrapped type works flawlessly, the Value method on the type won't be invoked when written to the database. This appears unintuitive to me.
Go Playground example: https://go.dev/play/p/9Pnu9JEN-zd
package main
import (
"database/sql"
"database/sql/driver"
"fmt"
"io"
)
func main() {
var forValue, forScan sql.Null[myType]
forValue.Valid = true
forValue.V.ID = 42
forValue.V.Name = "foobar"
val, err := forValue.Value()
fmt.Println("value wrapped")
fmt.Printf("err: %v\n", err)
fmt.Printf("val: %[1]T(%[1]v)\n", val)
val, err = forValue.V.Value()
fmt.Println("\nvalue not wrapped")
fmt.Printf("err: %v\n", err)
fmt.Printf("val: %[1]T(%[1]s)\n", val)
err = forScan.Scan(val)
fmt.Println("\nvalid value scanned using sql.Null[T]")
fmt.Printf("err: %v\n", err)
fmt.Printf("val: %[1]T(%[1]v)\n", forScan)
}
type myType struct {
ID int `json:"id"`
Name string `json:"value"`
}
func (t myType) Value() (driver.Value, error) {
return []byte(fmt.Sprintf("%d|%s", t.ID, t.Name)), nil
}
func (t *myType) Scan(src any) (err error) {
switch v := src.(type) {
case []byte:
if _, err = fmt.Sscanf(string(v), "%d|%s", &t.ID, &t.Name); err == io.EOF {
err = nil
}
default:
err = fmt.Errorf("unsupported type: %T", src)
}
return err
}
What did you see happen?
The program outputs the following
value wrapped
err: <nil>
val: main.myType({42 foobar}) # <-- this doesn't work as the database driver will reject the value
value not wrapped
err: <nil>
val: []uint8(42|foobar)
valid value scanned using sql.Null[T]
err: <nil>
val: sql.Null[main.myType]({{42 foobar} true})
What did you expect to see?
But instead it should result in this
value wrapped
err: <nil>
val: []uint8(42|foobar)
value not wrapped
err: <nil>
val: []uint8(42|foobar)
valid value scanned using sql.Null[T]
err: <nil>
val: sql.Null[main.myType]({{42 foobar} true})
A very naive fix for sql.Null would look like this, but someone with deeper knowledge might come up with a better solution.
func (n Null[T]) Value() (driver.Value, error) {
if !n.Valid {
return nil, nil
}
if v, ok := any(n.V).(driver.Valuer); ok {
return v.Value()
}
return n.V, nil
}
Related Issues and Documentation
- database/sql: Inconsistent Behavior with Custom Null Types #66621 (closed)
- proposal: database/sql: add a common method to unwrap value of sql.Null* types #63633
- database/sql: panic inserting a nil pointer when underlying type implements driver.Valuer #8415 (closed)
- database/sql: (n *NullInt64) Scan identity #35228 (closed)
- database/sql: add configuration option to ignore null values in Scan #57066
- database/sql: the Null* types naively/prematurely set .Valid=true before converting and scanning data #45662
- proposal: database/sql: add configuration option to ignore null values in Scan #57099
- database/sql: convert null values to []byte as nil #2788 (closed)
- x/tools/go/types: panic in objectpath.find #48588 (closed)
- Proposal: database/sql: NULL column values should be parsed as zero values for floats and ints instead of trying to parse with strconv #22542 (closed)
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)
Thanks for filing this issue. I also ran into this recently when trying to use sql.Null[T] more widely.
One way to look at the issue is that there is an unfortunate asymmetry between Scan and Value when using Null[T]. If you implement a type X that implements arbitrarily complicated Scan and Value logic, a Null[X] is only usable as a Scan target (reading data from the DB) but not as a Value target (writing to the DB). Null[X] is only usable as a Value target if X is a simple type (int, string, etc -- one of the driver.Value types) that can be marshaled directly without calling X.Value.
This is such a shortcoming that it seems like essentially an API or implementation mistake.
Unfortunately, it will be hard to change this for backward compatibility reasons.
@cespare What's wrong with @sGy1980de's proposed fix? Maybe it's technically backwards-incompatible, but it seems like a bug fix to me.
@jba I agree it looks like a bug fix, and without thinking too hard about it it seems like the right fix.
I'm just commenting that you can construct some kind of contrived example that would be "broken" by the fix.
Perhaps the bug is clear-cut enough, and sql.Null is new enough, that we can just make the change.
@cespare Regarding backwards compatibility, I kind of have a hard time getting my head around in what situation this should break working code. Are there database drivers out there which support raw structs as values?
I thought it all boils down to the types, I have to expect as src for Scan.
@sGy1980de Here's an example:
type S string
func (s *S) Scan(src any) error {
switch src := src.(type) {
case string, []byte:
*s = S(src)
default:
return fmt.Errorf("unexpected type %T for S", src)
}
return nil
}
func (s S) Value() (driver.Value, error) {
panic("this method is buggy and bad")
}
So here we have kind of a pointless type that just wraps a string. It implement Scanner and Valuer, but the Value method is actually broken. However, if you use it only via sql.Null[S], it will currently "work", because the Value method is never called.
Here's a demo of what I mean: https://go.dev/play/p/pC0VhgSgm01
So there, the proposed fix would make the Value method get called and would break the program.
As I said, very contrived.
Who makes the call about whether this fix is acceptable? The database/sql owners are @bradfitz and @kardianos. Or should we make a proposal?
Otherwise I'm happy to send a CL if we think it's okay.
We don't need a proposal if there's not an API change. I would prefer to hear from at least one of the two owners before doing it.
@jba Since I'm on vacation for the next two weeks, please feel free to go ahead. Thanks for taking over.
Change https://go.dev/cl/620858 mentions this issue: database/sql: refactor Null[T].Value method, update doc for Null[T]
