code review 5630052: database/sql: treat pointers as nullable types like enc...

database/sql: treat pointers as nullable types like encoding/json

- convert from nil pointers to the nil interface{}
- dereference non-nil pointers
- convert from nil interface{}s to nil pointers
- allocate pointers for non-nil interface{}s
- tests for all of the above

[awpritchard, 2012-02-04 11:42:12]

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

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

[bradfitz, 2012-02-04 20:21:35]

If we allow this for parameters then I fear people will expect it to work
for Scan too, and then it becomes allocation heavy.

I'm torn.

Russ?


On Sat, Feb 4, 2012 at 3:42 AM, <awpritchard@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> database/sql: treat pointers as nullable types like encoding/json
>
> - convert from nil pointers to the nil interface{}
> - dereference non-nil pointers
> - convert from nil interface{}s to nil pointers
> - allocate pointers for non-nil interface{}s
> - tests for all of the above
>
> Please review this at
http://codereview.appspot.com/**5630052/<http://codereview.appspot.com/5630052/>
>
> Affected files:
>  M src/pkg/database/sql/convert.**go
>  M src/pkg/database/sql/convert_**test.go
>  M src/pkg/database/sql/driver/**types.go
>  M src/pkg/database/sql/driver/**types_test.go
>
>
> Index: src/pkg/database/sql/convert.**go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/**convert.go
> +++ b/src/pkg/database/sql/**convert.go
> @@ -102,6 +102,14 @@
>        }
>
>        switch dv.Kind() {
> +       case reflect.Ptr:
> +               if src == nil {
> +                       dv.Set(reflect.Zero(dv.Type())**)
> +                       return nil
> +               } else {
> +                       dv.Set(reflect.New(dv.Type().**Elem()))
> +                       return convertAssign(dv.Interface(), src)
> +               }
>        case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32,
> reflect.Int64:
>                s := asString(src)
>                i64, err := strconv.ParseInt(s, 10, dv.Type().Bits())
> Index: src/pkg/database/sql/convert_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/**convert_test.go
> +++ b/src/pkg/database/sql/**convert_test.go
> @@ -13,6 +13,7 @@
>  )
>
>  var someTime = time.Unix(123, 0)
> +var answer int64 = 42
>
>  type conversionTest struct {
>        s, d interface{} // source and destination
> @@ -26,6 +27,8 @@
>        wanttime time.Time
>        wantbool bool // used if d is of type *bool
>        wanterr  string
> +       wantptr  *int64 // if non-nil, *d's pointed value must be equal to
> *wantptr
> +       wantnil  bool   // if true, *d must be *int64(nil)
>  }
>
>  // Target variables for scanning into.
> @@ -41,6 +44,7 @@
>        scanf32    float32
>        scanf64    float64
>        scantime   time.Time
> +       scanptr    *int64
>  )
>
>  var conversionTests = []conversionTest{
> @@ -95,6 +99,14 @@
>        {s: float64(1.5), d: &scanf32, wantf32: float32(1.5)},
>        {s: "1.5", d: &scanf32, wantf32: float32(1.5)},
>        {s: "1.5", d: &scanf64, wantf64: float64(1.5)},
> +
> +       // Pointers
> +       {s: interface{}(nil), d: &scanptr, wantnil: true},
> +       {s: int64(42), d: &scanptr, wantptr: &answer},
> +}
> +
> +func intPtrValue(intptr interface{}) interface{} {
> +       return reflect.Indirect(reflect.**Indirect(reflect.ValueOf(**
> intptr))).Int()
>  }
>
>  func intValue(intptr interface{}) int64 {
> @@ -152,6 +164,16 @@
>                if !ct.wanttime.IsZero() &&
!ct.wanttime.Equal(timeValue(**ct.d))
> {
>                        errf("want time %v, got %v", ct.wanttime,
> timeValue(ct.d))
>                }
> +               if ct.wantnil && *ct.d.(**int64) != nil {
> +                       errf("want nil, got %v", intPtrValue(ct.d))
> +               }
> +               if ct.wantptr != nil {
> +                       if *ct.d.(**int64) == nil {
> +                               errf("want pointer to %v, got nil",
> *ct.wantptr)
> +                       } else if *ct.wantptr != intPtrValue(ct.d) {
> +                               errf("want pointer to %v, got %v",
> *ct.wantptr, intPtrValue(ct.d))
> +                       }
> +               }
>        }
>  }
>
> Index: src/pkg/database/sql/driver/**types.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/driver/**types.go
> +++ b/src/pkg/database/sql/driver/**types.go
> @@ -248,6 +248,13 @@
>
>        rv := reflect.ValueOf(v)
>        switch rv.Kind() {
> +       case reflect.Ptr:
> +               // indirect pointers
> +               if rv.IsNil() {
> +                       return nil, nil
> +               } else {
> +                       return defaultConverter{}.**
> ConvertValue(rv.Elem().**Interface())
> +               }
>        case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32,
> reflect.Int64:
>                return rv.Int(), nil
>        case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32:
> Index: src/pkg/database/sql/driver/**types_test.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/driver/**types_test.go
> +++ b/src/pkg/database/sql/driver/**types_test.go
> @@ -18,6 +18,7 @@
>  }
>
>  var now = time.Now()
> +var answer int64 = 42
>
>  var valueConverterTests = []valueConverterTest{
>        {Bool, "true", true, ""},
> @@ -37,6 +38,9 @@
>        {c: Bool, in: "foo", err: "sql/driver: couldn't convert \"foo\"
> into type bool"},
>        {c: Bool, in: 2, err: "sql/driver: couldn't convert 2 into type
> bool"},
>        {DefaultParameterConverter, now, now, ""},
> +       {DefaultParameterConverter, (*int64)(nil), nil, ""},
> +       {DefaultParameterConverter, &answer, answer, ""},
> +       {DefaultParameterConverter, &now, now, ""},
>  }
>
>  func TestValueConverters(t *testing.T) {
>
>
>

[awpritchard, 2012-02-04 20:30:46]

This patch does add the same behavior for Scan; I've been using it in a
database-driven web app for a few weeks now.  I coded it up because interaction
with encoding/json was awkward, requiring either an extra struct or a map and
some copying boilerplate, to ferry data from database/sql's struct{Int int64,
Valid bool} world to json's *int64.

Would it be better to have encoding/json recognize database/sql's Null* types
and treat them similarly to how it treats pointers?  In that case, it would be
odd to have encoding/json importing database/sql, though.

On 2012/02/04 20:21:35, bradfitz wrote:
> If we allow this for parameters then I fear people will expect it to work
> for Scan too, and then it becomes allocation heavy.
> 
> I'm torn.
> 
> Russ?
>

[rsc, 2012-02-05 02:56:15]

This only affects places where people want to use NullInt64 and so on, right?
What if instead we dropped those types in favor of *int64?
It seems like the allocations would only happen in the case where
you are trying to distinguish null from value.

Russ

[rog, 2012-02-06 11:15:51]

On 5 February 2012 02:56, Russ Cox <rsc@golang.org> wrote:
> This only affects places where people want to use NullInt64 and so on, right?
> What if instead we dropped those types in favor of *int64?
> It seems like the allocations would only happen in the case where
> you are trying to distinguish null from value.

+1

to be fair, that might be quite turn out to be a problem
in some cases, but presumably the caller could use ScannerInto
to save the allocation then.

[bradfitz, 2012-02-06 19:07:52]

Okay, we've decided to allow this, as well as keeping NullString for people
who care about allocations (and as an example of how to use the ScannerInto
interface).  I'm happy as long as both Scan and parameters both accept
pointers.

Could you re-sync this patch against tip and mail it out again?  I think I
broke the clean merge with my *interface{} change.

Thanks!


On Sat, Feb 4, 2012 at 12:30 PM, <awpritchard@gmail.com> wrote:

> This patch does add the same behavior for Scan; I've been using it in a
> database-driven web app for a few weeks now.  I coded it up because
> interaction with encoding/json was awkward, requiring either an extra
> struct or a map and some copying boilerplate, to ferry data from
> database/sql's struct{Int int64, Valid bool} world to json's *int64.
>
> Would it be better to have encoding/json recognize database/sql's Null*
> types and treat them similarly to how it treats pointers?  In that case,
> it would be odd to have encoding/json importing database/sql, though.
>
>
> On 2012/02/04 20:21:35, bradfitz wrote:
>
>> If we allow this for parameters then I fear people will expect it to
>>
> work
>
>> for Scan too, and then it becomes allocation heavy.
>>
>
>  I'm torn.
>>
>
>  Russ?
>>
>
>
>
http://codereview.appspot.com/**5630052/<http://codereview.appspot.com/5630052/>
>

[awpritchard, 2012-02-06 19:44:15]

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

Please take another look.

[bradfitz, 2012-02-08 06:14:02]

LGTM

Thanks!


On Mon, Feb 6, 2012 at 11:44 AM, <awpritchard@gmail.com> wrote:

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

[bradfitz, 2012-02-08 06:14:21]

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

database/sql: treat pointers as nullable types like encoding/json

- convert from nil pointers to the nil interface{}
- dereference non-nil pointers
- convert from nil interface{}s to nil pointers
- allocate pointers for non-nil interface{}s
- tests for all of the above

R=golang-dev, bradfitz, rsc, rogpeppe
CC=golang-dev
http://codereview.appspot.com/5630052

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[ikedam, 2022-01-15 04:24:18]

Hi,

I believe this feature works perfect (and really useful! Thanks!).
https://go.dev/play/p/oN8MLbd_T_L

But somehow it doesn't look documented anywhere.

I plan to add following descriptions to
https://github.com/golang/go/wiki/SQLInterface ("Dealing with NULL" section):

----
You can also pass pointer types. Be careful for performance issues as it
requires extra memory allocations.

```go
var name *string
err := db.QueryRowContext(ctx, "SELECT name FROM names WHERE id = $1",
id).Scan(&name)
```
----

That's a Wiki and provides no reviewing system, and I want this reviewed before
publish.
Let me know if this feature should not be documented for some reason.