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
Labels
Projects
Milestone

Comments

@dolmen
Copy link

@dolmen dolmen commented Nov 19, 2019

The internal convertAssignRows function 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
@gopherbot gopherbot added the Proposal label Nov 19, 2019
@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

@rsc rsc moved this from Incoming to Active in Proposals Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 11, 2019

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

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Dec 12, 2019

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

This comment has been minimized.

Copy link

@tgulacsi tgulacsi commented Dec 12, 2019

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

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Dec 14, 2019

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

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Dec 17, 2019

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 Dec 17, 2019
@dolmen

This comment has been minimized.

Copy link
Author

@dolmen 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 Jan 8, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@rsc 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 Jan 15, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 22, 2020

No change in consensus, so declined.

@rsc rsc closed this Jan 22, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.