go-structured-query icon indicating copy to clipboard operation
go-structured-query copied to clipboard

data mapping for pointer fields will overwrite every item in the slice

Open bokwoon95 opened this issue 3 years ago • 3 comments

sq's multi-row struct mapping works by repeatedly binding data to a single variable, then appending that variable to a slice. This works if the variable is a value type, since it will be copied by value every append: subsequent modifications to that variable will not affect what has already been appended into the slice. However if there were any fields that are pointer types, subsequent modifications to those fields will overwrite them for everything that has already been appended to the slice (since they were copied by reference).

There are workarounds for this (manually copying data before append), but it's not very user friendly as it requires understanding the inner workings of the library in order to fully grok.

I'm honestly not sure if there's a way around it, since the current way is the only way to have a fully 'generic' mapper function that works for any data type. By encapsulating stateful modifications in a closure function (or a pointer receiver method), the function for all mapper functions can have the same type of func (*sq.Row).

Even Go generics may not be sufficient here as they do not permit type parameters on methods. The following code below is ideal, but will not be achievable with Go's generics:

func (q SelectQuery) Get[Item any](db *DB, mapper func(*Row) Item) (Item, error)
func (q SelectQuery) GetSlice[Item any](db *DB, mapper func(*Row) Item) ([]Item, error)
func (q SelectQuery) GetAccumulate[Item, Items any](db *DB, initial Items, mapper func(*Row) Item, accumulator func(Item, Items) Items) (Items, error)

The document states that 'a suitably parameterized top-level function' will have to be written. Perhaps that may be the workaround.

func Get[Item any](db *DB, q Query, mapper func(*Row) Item) (Item, error)
func GetSlice[Item any](db *DB, q Query, mapper func(*Row) Item) ([]Item, error)

However the invocation will be so clunky compared to regular Selectx that I am not so sure that it is a direct upgrade:

// using Selectx
var user User
var user []User
err := sq.
    From(u).
    Where(u.Name.EqString("bob")).
    Selectx(user.RowMapper, func() { users = append(users, user) }).
    Fetch(db)
if err != nil {
}
fmt.Println(users)

// using GetSlice
users, err := sq.GetSlice(db,
    sq.From(u).
        Where(u.NAME.EqString("bob")),
    User{}.RowMapper,
if err != nil {
}
fmt.Println(users)
)

The only value Get brings over Selectx is that the mapper function can return a new value everytime rather than doing some side effect on a pointer receiver. My main issue with this is that it solves the problem of overwriting pointers fields but with an entirely different way of doing things (converting the query from a method to a top level function).

bokwoon95 avatar Oct 09 '20 19:10 bokwoon95

Why not use this use-case as an argument for permitting type parameters on methods?

mrg0lden avatar Nov 23 '20 20:11 mrg0lden

Nah I don't think it's possible to convince the Go team to budge on this, I think they have some performance issues with it. Besides once generics roll out on Feb 2022, more people will be trying it out and if type parameters on methods prove to be a paint point for many people it might get added eventually.

bokwoon95 avatar Nov 26 '20 09:11 bokwoon95

@bokwoon95 fair enough, I guess. Let's hope it doesn't get delayed.

mrg0lden avatar Nov 26 '20 12:11 mrg0lden