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: allow rows.Scan to create value, not just assign #20549

Closed
220kts opened this issue Jun 1, 2017 · 10 comments
Closed

proposal: database/sql: allow rows.Scan to create value, not just assign #20549

220kts opened this issue Jun 1, 2017 · 10 comments

Comments

@220kts
Copy link

@220kts 220kts commented Jun 1, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8.1 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

See attached file

main.txt

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

What did you see instead?

Would it be possible for the Scan() function to allocate nil pointers instead of returning an error? Alternatively, should it be documented what the intended behavior is if the client passes the address of a pointer variable. I suspect a lot of people pass the address of pointer variables to get around the nil pointer error (we do). Allocating pointers would make Scan() behave similar to json.Unmarshal(), which is quite practical in non-trivial applications where the Scan() destination is often a pointer inside a struct.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jun 2, 2017

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jun 2, 2017

Could you provide an example please?

@220kts
Copy link
Author

@220kts 220kts commented Jun 2, 2017

@kardianos the file attachment in my original post has more detail. The TLDR version:

type SomeData struct {
    prop1 int
    prop2 *int
}

myData := new(SomeData)

// Error
err = db.QueryRow('SELECT 1, 2').Scan(&myData.prop1, myData.prop2)

// No error, note address of operator on pointer variable.
err = db.QueryRow('SELECT 1, 2').Scan(&myData.prop1, &myData.prop2)

I suspect a lot of people use the latter implementation to avoid returning the nil pointer error but I think a more practical solution might be for convertAssign to allocate nil pointers instead of returning an error. The attachment in my original post also has some code for types that implement the Scanner interface.

@kardianos kardianos changed the title database/sql - should Scan() allocate pointers? proposal: database/sql: allow rows.Scan to create value, not just assign Jun 2, 2017
@gopherbot gopherbot added this to the Proposal milestone Jun 2, 2017
@gopherbot gopherbot added the Proposal label Jun 2, 2017
@kardianos
Copy link
Contributor

@kardianos kardianos commented Jun 2, 2017

Thank you for your clarification. I'm unsure if that's what we want to do or not. The first step would be to see how involved this change would be and what the runtime cost would be to do this.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 5, 2017

If you have

func f(x *int) { ... }

and call

var y *int = nil
f(y)

there is just no way for f to do anything to cause the definition of y in the caller's context to become non-nil. f doesn't know where the argument came from nor does it have any way to get back to the "source" and change it. You'd have to write f(**int) and call f(&y).

It seems like that's what you're asking Scan to do here. Am I missing something?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jun 5, 2017

@rsc , yes you are correct. There is nothing we can do here in this case. I was not thinking correctly when I posted my last comment.

@kardianos kardianos closed this Jun 5, 2017
@220kts
Copy link
Author

@220kts 220kts commented Jun 5, 2017

@rsc, @kardianos, Thanks for the clarification. Could anything be added to the documentation to clarify behavior when dest is a pointer type? We end up with inconsistent client side code when developers randomly mix Scan(i) and Scan(&i) when i is of type *int.

I admit I don't understand lower level implementation well but perhaps something like "If dest is a nil-pointer, Scan returns sql.errNilPtr. If dest is a pointer to a pointer, Scan attempts to indirect dest."

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jun 5, 2017

@220kts The issue is a bit more fundamental then that.
Take: type User struct { ID *int } , if you pass in rows.Scan(user.ID), it is the same as passing in rows.Scan(0) (nil pointer). We could create it and assign it, but we could never "return" it because you don't have the address to use it after we assign it. Does that make sense?

@220kts
Copy link
Author

@220kts 220kts commented Jun 5, 2017

@kardianos Yes, thanks. The part we struggle with - due to limited understanding, we're application developers - is inconsistent client side code. Often our developers know in a certain context user.ID is not nil so they write rows.Scan(user.ID) and it works. In other context user.ID may be nil so they write rows.Scan(&user.ID) and it also works.

This discussion clarified the issue for me personally but perhaps some additional verbiage or an example with pointer types in the documentation might be useful for other developers as well?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jun 5, 2017

I don't think there is anything to really add on database/sql side. Though I could be wrong.

I think this might be just better understanding points. It is tempting to use a pointer as a NULLABLE value in Go. I've done it myself. However, when at all possible I actually try to avoid using pointers to facilitate database NULL values. Database NULL and Go nil arguably convey different concepts: Go nil / pointers point to another value and remove the value from the current struct. The database NULL says the value is not be present.

@golang golang locked and limited conversation to collaborators Jun 5, 2018
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
5 participants
You can’t perform that action at this time.