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: convertAssign should be exposed as DefaultConvertAssign #24258

Closed
awreece opened this issue Mar 5, 2018 · 21 comments
Closed
Assignees
Milestone

Comments

@awreece
Copy link
Contributor

@awreece awreece commented Mar 5, 2018

It would be nice for there to be a generic convertAssign method exposed a la DefaultParameterConvert.

For an example use case, I want to make a custom wrapper for driver.Valuer and sql.Scanner that will store in the database null when my object is the zero value (this allows me to use the "zero value means empty" idiom in proto3 and "null means empty" in sql).

For the driver.Valuer side, this is simple: I check to see if the value is zero, and otherwise delegate to DefaultParameterConverter:

type protoFieldWrapper reflect.Value

//
// Value implements the database/sql/driver.Valuer interface for the wrapped value,
// returning null if the value is the zero value or a driver.Value otherwise.
//
func (pw protoFieldWrapper) Value() (driver.Value, error) {
	pwv := reflect.Value(pw)
	if reflect.DeepEqual(pwv.Interface(), reflect.Zero(pwv.Type()).Interface()) {
		return nil, nil
	}
	v := pwv.Interface()
	return driver.DefaultParameterConverter.ConvertValue(v)
}

Unfortunately, convertAssign is not exposed, making the corresponding in Scan much harder:

//
// Scan implements the database/sql.Scanner interface for the wrapped value,
// setting the value to its zero value if the src is null or scanning it otherwise.
//
func (pw protoFieldWrapper) Scan(src interface{}) error {
	pwv := reflect.Value(pw)
	if src == nil {
		pwv.Set(reflect.Zero(pwv.Type()))
		return nil
	}
	if !driver.IsValue(src) {
		return errors.Errorf("don't know how to scan %T", src)
	}

	t := reflect.TypeOf(src)
	if !t.ConvertibleTo(pwv.Type()) {
		return errors.Errorf("could not assign value of type %v to %v", t, pwv.Type())
	}
	pwv.Set(reflect.ValueOf(src).Convert(pwv.Type()))
	return nil
}

A savvy reader will note that the above code is not actually as robust as convertAssign in a myriad of ways:

  • If the value being scanned is a pointer, convertAssign will allocate a new object and scan into it, if possible.
  • If the value being scanned is a sql.Scanner, convertAssign will call .Scan().
  • etc.

#22544 is a related proposal, but it appears to be focused on performance/safety for driver, not improving the end user api here.

@gopherbot gopherbot added this to the Proposal milestone Mar 5, 2018
@gopherbot gopherbot added the Proposal label Mar 5, 2018
@awreece
Copy link
Contributor Author

@awreece awreece commented Mar 5, 2018

I don't really know the process for small proposals like this -- if there is interest in this for an upcoming minor version release, I'm happy to work on a PR.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 5, 2018

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 5, 2018

Related to #22544 , possibly a duplicate depending on how/if it gets resolved.

@kardianos kardianos self-assigned this Mar 5, 2018
@awreece
Copy link
Contributor Author

@awreece awreece commented Mar 5, 2018

Indeed, but it would be nice to not block this smaller-scope proposal on that larger one.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 8, 2018

As a counter proposal:

If a type's Scan method returned "database/sql/driver".ErrSkip rather then returning the error it could continue on to attempt to assign using reflection.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 16, 2018

@awreece Hi, what do you think of my proposal above?

@awreece
Copy link
Contributor Author

@awreece awreece commented Mar 16, 2018

I'm not quite sure how that achieves my use case:

type MyScanner struct {
     Val *int // Wraps some type
}

function (ms *MyScanner) Scan(src interface{}) error {
    if (can_custom_logic) {
         // Initialize ms.Val
         return nil
    }

    return ErrSkip
}

My goal would be to convertAssign into the wrapped value Val rather than to try to use reflection to assign to ms. Am I missing something?

@awreece
Copy link
Contributor Author

@awreece awreece commented Mar 16, 2018

Compare to the original proposal:

function (ms *MyScanner) Scan(src interface{}) error {
    if (can_custom_logic) {
         // Initialize ms.Val
         return nil
    }

    return driver.DefaultConvertAssign.Scan(ms.Val, src)
}
@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 16, 2018

What type of MyScanner.Val? Is it a struct or string or []byte?

@awreece
Copy link
Contributor Author

@awreece awreece commented Mar 16, 2018

In the example above it was an *int. In my real code it's an interface{}

@awreece
Copy link
Contributor Author

@awreece awreece commented Mar 22, 2018

I got bit by this again today.

The test case below fails. I believe it is becase I failed to mimic the behavior in convertAssign for byte slices
(https://golang.org/pkg/database/sql/#Rows.Scan):

If a dest argument has type *[]byte, Scan saves in that argument a copy of the corresponding data. The copy is owned by the caller and can be modified and held indefinitely. The copy can be avoided by using an argument of type *RawBytes instead; see the documentation for RawBytes for restrictions on its use.

package proto_test

import (
	"database/sql"
	"os"
	"reflect"
	"testing"

	_ "github.com/lib/pq"
)

func (w wrapper) Scan(src interface{}) error {
	w.t.Logf("in scan, src = %#v", src)

	srcBytes := src.([]byte)
	*w.dst = srcBytes

	w.t.Logf("after set, dst = %#v", *w.dst)
	return nil
}

type wrapper struct {
	dst *[]byte
	t   *testing.T
}

func SQLValue(t *testing.T, i *[]byte) wrapper {
	return wrapper{t: t, dst: i}
}

func TestWrapper(t *testing.T) {
	dsn, ok := os.LookupEnv("DATABASE_URL")
	if !ok {
		t.Skip("no DATABASE_URL")
	}

	db, err := sql.Open("postgres", dsn)
	if err != nil {
		t.Fatal("database error", err)
	}

	var want = []byte{0x41, 0x42, 0x43}
	var got []byte

	var wrapped = SQLValue(t, &got)
	if err := db.QueryRow(`select $1::bytea`, &want).Scan(wrapped); err != nil {
		t.Error("select", err)
	}
	if !reflect.DeepEqual(got, want) {
		t.Errorf("got = %#v, want = %#v", got, want)
	}
}

fails:

=== RUN   TestWrapper
--- FAIL: TestWrapper (0.01s)
	proto_test.go:13: in scan, src = []byte{0x41, 0x42, 0x43}
	proto_test.go:18: after set, dst = []byte{0x41, 0x42, 0x43}
	proto_test.go:53: got = []byte{0x20, 0x31, 0x0}, want = []byte{0x41, 0x42, 0x43}
FAIL

(the test case passes when I changed the scan method to use a copy of cloneBytes)

@awreece
Copy link
Contributor Author

@awreece awreece commented Mar 22, 2018

I filed a separate bug for the surprising behavior (#24492) but I think it is another illustration of the perils of not being able to call back to the standard library convertAssign :(

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 2, 2018

I'm fine exporting a new variable ("DefaultConvertAssign" or whatever) if @kardianos is.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 16, 2018

@kardianos, any thoughts?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 16, 2018

I would prefer not to do this so long as I can come up with a more comprehensive value scanning proposal.

I've been working on a potential Go2 interface would look like, the biggest change has to do with scanning values and results. Ideally this could be opted in by Go1 drivers, then Go2 drivers would simply remove the extra old methods. I should have an answer in a day or two.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2018

Change https://golang.org/cl/107995 mentions this issue: database/sql: add the driver.ValueScanner interface

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 19, 2018

@awreece I think I may be having a hard time understanding your final use case. Could you point me to some code about what you are doing or describe it here? Just a high level description would be fine.

@awreece
Copy link
Contributor Author

@awreece awreece commented Apr 19, 2018

I don't quite know what you mean by my "final use case" -- is it the memory corruption test cast? #24492 is a dedicated bug filed for that behavior and probably a better place to discuss that. In short, all of these are the same use case for me:

  • I use protobufs in go, which have the convention that "zero value means no value" (vs the database idiom of "null means no value).
  • I have a wrapper for protobuf fields that translates between these two idioms, converting zero values to null by implementing driver.Valuer and converting null to a zero values by implementing sql.Scanner. This wrapper looks something the initial example in this issue (#24258 (comment)).

The last test case is a reduced demonstration of one of the issues I've hit because I don't have access to the internal convertAssign function: my wrapper struct implements sql.Scanner and assumes it can assign values via reflect.Set. This works for every driver.Value type except []byte, which instead produces memory corruption.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 20, 2018

Two questions:

  • In the protoFieldWrapper, can you write if bb; ok := src.([]byte); ok {copy(dest, src)} ? Am I missing something?
  • I'm assuming you don't want to copy/paste the convertAssign for your own use (it is stand alone code for the most part). Is it just DRY or or some other reason I haven't realized?
@awreece
Copy link
Contributor Author

@awreece awreece commented Apr 20, 2018

Q1) Absolutely I could just do copy(dest, src) -- that is my current workaround for this issue :). That is an additional special case I have to maintain and feels like a brittle solution to fundamentally surprising behavior. What if the code changes are there are new cases I need to add?

My comment reads something like:

The comment on rows.Scan seems to indicate that unless RawBytes is used, the src []byte will be copied and owned by the caller. Unfortunately, this is not the case: sql.Scanner is the unique case where src []bytes are not copied. Since we are sql.Scanner, we must manually copy the slice before returning it to avoid memory corruption.

Q2) I do not want to copy/paste convertAssign -- it is under active development and I have no reliable way to maintain a fork! :) I would much rather work with the open source project than fork it and lose the collaborative benefits.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 20, 2018

I've sent a CL for #24492 .

I don't want the extra burden of maintaining convertAssign as a public API. I'm going to ask you either continue using the type assert or copy the one function out of database/sql and check on it every couple of years to see if you need to update it.

@kardianos kardianos closed this Apr 20, 2018
@golang golang locked and limited conversation to collaborators Apr 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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