code review 5577054: database/sql: convert SQL null values to []byte as nil.

database/sql: convert SQL null values to []byte as nil.

Also allow string values to scan into []byte.
Fixes issue 2788.

[coopernurse, 2012-01-26 17:33:14]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-01-26 21:22:29]

http://codereview.appspot.com/5577054/diff/1002/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

http://codereview.appspot.com/5577054/diff/1002/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:904: if !ok || *b == nil {
I'd put this in its own if statement following this one, just so you could put a
comment in it and explain it.  it's a little subtle.

if *b == nil {
    // If the []byte is now nil (for a NULL value),
    // don't fall through to below which would
    // turn it into a non-nil 0-length byte slice
    continue
}

http://codereview.appspot.com/5577054/diff/1002/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:970: // TODO(coopernurse): We do the same []byte
clone in Rows.Scan()
oh, good catch!  This code used to matter when only Row.Scan was doing the
copies.  But once Rows.Scan got the copying behavior, this code does seem
redundant.

We do want to do a pass over dest at some point (either here, or before
r.rows.Scan on line 952 above) to make sure there are no *RawBytes in Row.Scan,
but we don't need the copying anymore.

Want to fix that in this CL too?

http://codereview.appspot.com/5577054/diff/1002/src/pkg/database/sql/sql_test.go
File src/pkg/database/sql/sql_test.go (right):

http://codereview.appspot.com/5577054/diff/1002/src/pkg/database/sql/sql_test...
src/pkg/database/sql/sql_test.go:366: exec(t, db,
"CREATE|t|id=int32,name=string")
don't you want name=nullstring here?

in fakedb_test.go:

        case "string":
                return driver.NotNull{driver.String}
        case "nullstring":
                return driver.Null{driver.String}

http://codereview.appspot.com/5577054/diff/1002/src/pkg/database/sql/sql_test...
src/pkg/database/sql/sql_test.go:370: if name != nil {
remove this.  this is testing the language, not sql.  the language has tests
elsewhere.

http://codereview.appspot.com/5577054/diff/1002/src/pkg/database/sql/sql_test...
src/pkg/database/sql/sql_test.go:388: t.Fatalf("name []byte should be bob, got:
%s", string(name))
use %q here instead of %s.  makes it easier to see empty strings and corrupt
strings.  and if a test is failing, one of those is very likely.

[coopernurse, 2012-01-26 22:29:29]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-01-26 22:36:59]

http://codereview.appspot.com/5577054/diff/7001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

http://codereview.appspot.com/5577054/diff/7001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:975: for _, dp := range dest {
can you move this loop up before "defer r.rows.Close()"?  I want this error
message to be consistently returned and never shadowed by an error from lines
956 or 960.

(I do want the error from line 952 coming first, though).

Move the comment up too, but update it a bit:

// TODO(bradfitz): for now we need to defensively clone all
// []byte that the driver returned (not permitting *RawBytes in Rows.Scan), ...

[coopernurse, 2012-01-26 22:46:07]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-01-26 23:12:41]

LGTM
thanks!

On Thu, Jan 26, 2012 at 2:46 PM, <jamespcooper@gmail.com> wrote:

> Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5577054/<http://codereview.appspot.com/5577054/>
>

[bradfitz, 2012-01-26 23:12:50]

*** Submitted as http://code.google.com/p/go/source/detail?r=a1b77ddd6056 ***

    database/sql: convert SQL null values to []byte as nil.

Also allow string values to scan into []byte.
Fixes issue 2788.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/5577054

Committer: Brad Fitzpatrick <bradfitz@golang.org>