Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: database/sql: expose convertAssign functionality in an Any type #35697

Closed
dolmen opened this issue Nov 19, 2019 · 10 comments
Closed

proposal: database/sql: expose convertAssign functionality in an Any type #35697

dolmen opened this issue Nov 19, 2019 · 10 comments

Comments

@dolmen
Copy link
Contributor

dolmen commented Nov 19, 2019

The internal convertAssignRows function (from convert.go) does much magic. Access to this magic is useful for libraries that want to expose utilities (types, functions) that wrap any destination value accepted by Rows.Scan and expose an sql.Scanner by using sql.Scanners as russian dolls. This could be used for example to implement struct scanning.

However that magic is hard to reproduce and maintain with full compatibility on the long term for external users of database/sql.

I propose to add a function sql.Any that exposes most of the convertAssignRows magic by allowing to wrap any Rows.Scan-compatible value as an sql.Scanner.

type any func(v interface{}) error

func (a any) Scan(v interface{}) error {
    return (func(interface{} error)(a)(v)
}

func Any(dest interface{}) interface{} {
    switch d := dest.(type) {
    case *string:
        return any(func(src interface{}) error {
            switch s := src.(type) {
            case string:
                *d = s
                return nil
            case []byte:
                *d = string(s)
                return nil
            default:
		sv = reflect.ValueOf(src)
		switch sv.Kind() {
		case reflect.Bool,
			reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
			reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64,
			reflect.Float32, reflect.Float64:
			*d = asString(src)
			return nil
		...
		}
            }
        })
    case *[]byte:
        // ...
    case *time.Time:
        // ...
    // ...
    case *interface{}:
        return any(func(src interface{}) error {
            *d = src
            return nil
        })
    }
    if _, ok := dest.(Scanner); ok {
        return dest
    }

    // switch on reflect.TypeOf(dest).Elem().Kind()
    // ...

    panic(fmt.Errorf("unsupported Scan target type %T", dest))
}

Notes:

  • yes, Any is a bad name (not enough explicit)
  • I may not have fully understood the full convertAssignRows magic and what conversions would be not be possible if the converter doesn't have access to both the source and destinations types at the same time. I want to be lightened about those edge cases.
@gopherbot gopherbot added this to the Proposal milestone Nov 19, 2019
@rsc rsc added this to Incoming in Proposals (old) Nov 27, 2019
@rsc
Copy link
Contributor

rsc commented Nov 27, 2019

/cc @kardianos

@rsc rsc moved this from Incoming to Active in Proposals (old) Dec 4, 2019
@rsc
Copy link
Contributor

rsc commented Dec 11, 2019

@kardianos, any opinions on this one way or another?

@kardianos
Copy link
Contributor

The converAssign function is certainly useful for other purposes in drivers, or possibly libraries. For instance, the MS SQL driver has a copy of the convert assign function here. It is used to scan Output variables back out from the result.

@tgulacsi, are you aware of other use cases for someone copying convertAssign?

To date, I've classified this as "a little copying is fine", especially as the code doesn't change often. I think if we gathered sufficient real world examples of this copied in the wild, we should consider exporting it in some way.

@tgulacsi
Copy link
Contributor

No, I don't use a copy of convertAssign, but it may be useful, as it does some nice tricks.

But I don't understand what is this Any type for - you do know the type of the requested column, don't you? If not, use and interface{}...

Maybe a stripped-down Convert, which can convert between various string, []byte and numeric types?

@kardianos
Copy link
Contributor

@dolmen Can you point me to code that currently copies and pastes this function today?

@kardianos
Copy link
Contributor

Unless I can see a few more places this is already used by copy/paste, I'm inclined to reject this. This code can generally be safely copy and pasted. Setting to likely decline to declined if no additional examples are found.

@kardianos kardianos moved this from Active to Likely Decline in Proposals (old) Dec 17, 2019
@dolmen
Copy link
Contributor Author

dolmen commented Jan 3, 2020

@kardianos I wrote this AppendScanner function where convertAssign functionality would be helpful.

import (
	"database/sql"
	"fmt"
	"log"
	"reflect"
)

type appendScanner struct {
	elemZero reflect.Value
	scan     func(target reflect.Value, v interface{}) error
	slice    reflect.Value
}

func (as *appendScanner) Scan(v interface{}) error {
	newTarget := reflect.Append(as.slice.Elem(), as.elemZero)
	newElem := newTarget.Index(newTarget.Len() - 1).Addr()
	if err := as.scan(newElem.Elem(), v); err != nil {
		return err
	}
	as.slice.Elem().Set(newTarget)
	return nil
}

// AppendScanner returns an sql.Scanner that appends the value from the database to the slice.
func AppendScanner(slice interface{}) sql.Scanner {
	t := reflect.TypeOf(slice)
	if t.Kind() != reflect.Ptr || t.Elem().Kind() != reflect.Slice || reflect.ValueOf(slice).IsNil() {
		log.Println(t.Kind())
		log.Println(t.Elem().Kind())
		panic("target should be a pointer to a slice variable")
	}
	elemType := t.Elem().Elem()
	var scan func(target reflect.Value, v interface{}) error
	if reflect.PtrTo(elemType).Implements(scannerType) || reflect.PtrTo(reflect.PtrTo(elemType)).Implements(scannerType) {
		scan = func(target reflect.Value, v interface{}) error {
			return target.Interface().(sql.Scanner).Scan(v)
		}
	} else {
		switch reflect.Zero(elemType).Interface().(type) {
		case int, *int, int64, *int64, int32, *int32:
			scan = func(target reflect.Value, v interface{}) error {
				target.SetInt(v.(int64))
				return nil
			}
		case uint, *uint, uint64, *uint64, uint32, *uint32:
			scan = func(target reflect.Value, v interface{}) error {
				target.SetUint(uint64(v.(int64)))
				return nil
			}
		case string, *string:
			scan = func(target reflect.Value, v interface{}) error {
				switch v := v.(type) {
				case string:
					target.SetString(v)
				case []byte:
					target.SetString(string(v))
				default:
					return fmt.Errorf("unexpected type %T to scan into %s", v, target.Type())
				}
				return nil
			}
		case []byte:
			scan = func(target reflect.Value, v interface{}) error {
				target.SetBytes(v.([]byte))
				return nil
			}
		default:
			// TODO add support for any driver.Value type
			panic("slice elements must implement sql.Scanner")
		}
	}
	// If pointer, wrap the scan to allocate the target value if not NULL
	if reflect.Zero(elemType).Kind() == reflect.Ptr {
		innerScan := scan
		scan = func(target reflect.Value, v interface{}) error {
			if v == nil {
				return nil
			}
			vv := reflect.New(target.Type().Elem())
			if err := innerScan(vv.Elem(), v); err != nil {
				return err
			}
			target.Set(vv)
			return nil
		}
	}
	return &appendScanner{
		elemZero: reflect.Zero(elemType),
		// isElemPtr: elemType.Kind() == reflect.Ptr,
		scan:  scan,
		slice: reflect.ValueOf(slice),
	}
}

And here are two testcases:

func ExampleAppendScanner() {
	ctx := context.Background()
	db, err := sql.Open("sqlite3", ":memory:")
	if err != nil {
		log.Printf("Open: %v", err)
		return
	}
	defer db.Close()

	// Use AppendScanner to scan multiple columns values into a slice
	var v1 []int64
	appender := sqlutil.AppendScanner(&v1)
	if err := db.QueryRowContext(ctx, `SELECT 1, 2, 3`).
		Scan(appender, appender, appender); err != nil {
		log.Fatal(err)
	}
	fmt.Println(v1)

	// Use AppendScanner to scan value from multiple into a slice
	var v2 []string
	appender = sqlutil.AppendScanner(&v2)
	rows, err := db.QueryContext(ctx, ``+
		`   SELECT 'a'`+
		` UNION ALL`+
		`   SELECT 'b'`+
		` UNION ALL`+
		`   SELECT 'c'`,
	)
	if err != nil {
		log.Fatal(err)
	}
	defer rows.Close()
	for rows.Next() {
		if err = rows.Scan(appender); err != nil {
			log.Fatal(err)
		}
	}
	if err = rows.Err(); err != nil {
		log.Fatal(err)
	}
	fmt.Println(v2)

	// Output:
	// [1 2 3]
	// [a b c]
}

@rsc rsc moved this from Likely Decline to Active in Proposals (old) Jan 8, 2020
@rsc
Copy link
Contributor

rsc commented Jan 8, 2020

@kardianos said:

To date, I've classified this as "a little copying is fine", especially as the code doesn't change often. I think if we gathered sufficient real world examples of this copied in the wild, we should consider exporting it in some way.

The reply from @dolmen shows one copy, but I don't think we have evidence that the copying is very common. Are there more examples?

(@kardianos, I moved the project classification back to Active because we only mark Likely Decline when we publish proposal minutes.)

@rsc
Copy link
Contributor

rsc commented Jan 15, 2020

Based on the lack of evidence that copying is very common, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jan 15, 2020
@rsc
Copy link
Contributor

rsc commented Jan 22, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Jan 22, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jan 22, 2020
@golang golang locked and limited conversation to collaborators Jan 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants