Support for Scan into pointer to RedisScan

[bferullo]

Currently, calling redis.Scan with a destination that is a pointer to a type that implements redis.RedisScan results in the following error:
redigo.Scan: cannot assign to dest 0: cannot convert from Redis simple string to *data.Foo

Other flavors of Scan (e.g. ScanStruct) seem happy to reflect.New a target if necessary. Can the same be extended to Scan?

Example code:

// scan_test.go
package data

import (
	"fmt"
	"strconv"
	"testing"

	"github.com/gomodule/redigo/redis"
	"github.com/stretchr/testify/assert"
)

type Foo struct{ Bar int64 }

func (f *Foo) RedisScan(src interface{}) error {
	switch s := src.(type) {
	case []byte:
		f.Bar, _ = strconv.ParseInt(string(s), 10, 64)
	case string:
		f.Bar, _ = strconv.ParseInt(string(s), 10, 64)
	default:
		return fmt.Errorf("invalid type %T", src)
	}
	return nil
}

func TestUnit_Scan(t *testing.T) {
	src := []interface{}{[]byte("1234"), nil}
	var foo *Foo
	_, err := redis.Scan(src, &foo)
	assert.NoError(t, err)
	assert.Equal(t, &Foo{Bar: 1234}, foo)
}

go test result:

--- FAIL: TestUnit_Scan (0.00s)
    sandbox\scan_test.go:30: 
        	Error Trace:	scan_test.go:30
        	Error:      	Received unexpected error:
        	            	redigo.Scan: cannot assign to dest 0: cannot convert from Redis simple string to *data.Foo
        	Test:       	TestUnit_Scan
    sandbox\scan_test.go:31: 
        	Error Trace:	scan_test.go:31
        	Error:      	Not equal: 
        	            	expected: &data.Foo{Bar:1234}
        	            	actual  : (*data.Foo)(nil)
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,2 @@
        	            	-(*data.Foo)({
        	            	- Bar: (int64) 1234
        	            	-})
        	            	+(*data.Foo)(<nil>)
        	            	 
        	Test:       	TestUnit_Scan
FAIL
FAIL	sandbox	0.332s
Error: Tests failed.

[stevenh]

The correct way to do this is actually:

	var foo Foo
	_, err := redis.Scan(src, &foo)

[bferullo]

@stevenh sorry, my example probably wasn't sufficient for the problem I was trying to solve when I wrote this issue -- I was ultimately trying to scan into a pointer field of a struct and hoping to save some work with temporary variables or shims. An example of that structure would be something along these lines:

type InnerStruct struct {
	Foo int64
}

func (f *InnerStruct) RedisScan(src interface{}) error {
	switch s := src.(type) {
	case []byte:
		f.Foo, _ = strconv.ParseInt(string(s), 10, 64)
	case string:
		f.Foo, _ = strconv.ParseInt(string(s), 10, 64)
	default:
		return fmt.Errorf("invalid type %T", src)
	}
	return nil
}

type OuterStruct struct {
	Inner *InnerStruct
}

func TestUnit_Scan(t *testing.T) {
	src := []interface{}{[]byte("1234"), nil}
	var foo OuterStruct
	_, err := redis.Scan(src, &foo.Inner) // ERROR
	assert.NoError(t, err)
	assert.Equal(t, OuterStruct{Inner: &InnerStruct{Foo: 1234}}, foo)
}

To get around this, I made a little shim:

func scanToPointer(t interface{}) interface{} {
	return &scanShim{t}
}

type scanShim struct{ T interface{} }

func (s *scanShim) RedisScan(src interface{}) error {
	if dest := reflect.ValueOf(s.T).Elem(); dest.Type().Kind() == reflect.Ptr && dest.CanInterface() {
		if src == nil || src.([]byte) == nil {
			// if our source is nil, leave the target unchanged (and presumably nil)
			return nil
		}
		if dest.IsNil() {
			dest.Set(reflect.New(dest.Type().Elem()))
		}
		if scanner, ok := dest.Interface().(redis.Scanner); ok {
			return scanner.RedisScan(src)
		}
	}
	return fmt.Errorf("type (%T) invalid for scanToPointer", s.T)
}

Used like this:

_, err := redis.Scan(src, scanToPointer(&foo.Inner))

(which works)

[stevenh]

This is what I would use in that scenario:

type InnerStruct struct {
        Foo int64
}

func (f *InnerStruct) RedisScan(src interface{}) (err error) {
        switch s := src.(type) {
        case []byte:
                f.Foo, err = strconv.ParseInt(string(s), 10, 64) // <-- Added error check
        case string:
                f.Foo, err = strconv.ParseInt(s, 10, 64) // <-- Added error check
        default:
                return fmt.Errorf("invalid type %T", src)
        }
        return err // <-- Added error check
}

type OuterStruct struct {
        Inner *InnerStruct
}

func TestUnit_Scan(t *testing.T) {
        src := []interface{}{[]byte("1234"), nil}
        foo := OuterStruct{&InnerStruct{}} // <-- Initialise the struct
        _, err := redis.Scan(src, foo.Inner)
        assert.NoError(t, err)
        assert.Equal(t, OuterStruct{Inner: &InnerStruct{Foo: 1234}}, foo)
}

[bferullo]

(Thanks for taking the time, by the way)

That's another reasonable solution, but leaves foo.Inner as an allocated field (but zero-valued) when the input from redis is null. We might be getting too far into a weird scenario here, but what I was trying to do was preserve the meaningfulness of a nil field, like:

	src2 := []interface{}{[]byte(nil), nil}
	var bar OuterStruct
	_, err = redis.Scan(src2, &bar.Inner)
	assert.NoError(t, err)
	assert.Equal(t, OuterStruct{}, bar)

That is, using the scanShim (or a built-in feature that did the same thing), bar.Inner would remain nil on nil input from redis.

[stevenh]

Hi @bferullo could you test the above PR in your situation to make sure it addresses your issue?

[bferullo]

Totally does, thanks!