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.

Sign up for GitHub

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

Jump to bottom

proposal: database/sql: export convertAssign as DefaultConvertAssign #62146

Open
moznion opened this issue Aug 18, 2023 · 6 comments
Open

proposal: database/sql: export convertAssign as DefaultConvertAssign #62146

moznion opened this issue Aug 18, 2023 · 6 comments
Labels
Proposal
Milestone
Proposal

Comments

@moznion
Copy link

moznion commented Aug 18, 2023 •
edited

As discussed in #24258 and #35697, but I think it would be nicer to have the exposed sql.convertAssign() function.
In the previous suggestion, @kardianos said at #24258 (comment):

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.

and @rsc also said as well at #35697 (comment), i.e. they suggested doing copy the sql.convertAssign() to the user's project.
At the date of the first suggestion, Apr 20, 2018, copying was a good solution because that function hadn't accessed the unexposed value and functions; ref: https://github.com/golang/go/blob/da24c95ce09668a0d977c208e8e610a21b98b019/src/database/sql/convert.go
However, that function touches the unexported items in the current implementation; for example,

go/src/database/sql/convert.go

Lines 319 to 347 in 9ac6b00

case driver.Rows:
switch d := dest.(type) {
case *Rows:
if d == nil {
return errNilPtr
}
if rows == nil {
return errors.New("invalid context to convert cursor rows, missing parent *Rows")
}
rows.closemu.Lock()
*d = Rows{
dc: rows.dc,
releaseConn: func(error) {},
rowsi: s,
}
// Chain the cancel function.
parentCancel := rows.cancel
rows.cancel = func() {
// When Rows.cancel is called, the closemu will be locked as well.
// So we can access rs.lasterr.
d.close(rows.lasterr)
if parentCancel != nil {
parentCancel()
}
}
rows.closemu.Unlock()
return nil
}
}

I suppose this implies users have not been able to copy this function simply so copying would be no longer an effective way to satisfy the demand. Can we have a chance to reboot exposure for this sql.convertAssign() function?
@gopherbot gopherbot added this to the Proposal milestone Aug 18, 2023
@moznion
Copy link
Author

moznion commented Aug 18, 2023

FYI: I would like to use that function in my project and we've tried to copy'n'paste that, but we faced such issue in copying.
moznion/go-optional#32

@seankhliao seankhliao changed the title proposal: database/sql: [reopen] convertAssign should be exposed as DefaultConvertAssign proposal: database/sql: export convertAssign as DefaultConvertAssign Aug 18, 2023
@ianlancetaylor
Copy link
Contributor

CC @kardianos

@ianlancetaylor
Copy link
Contributor

What are the types that you want to pass to the proposed function?

@moznion
Copy link
Author

moznion commented Aug 19, 2023

@ianlancetaylor actually, the type would be any; like https://github.com/moznion/go-optional/blob/c95f59ccbec4a6dab276f0ad80349cf989f1907e/sql_driver.go#L15-L19

@ianlancetaylor
Copy link
Contributor

I mean, what would the dynamic type be? What type would be stored in the interface?

What I'm trying to get at is: when would people want to use this? Do you have an example of code that is currently copying it? Thanks.

@moznion
Copy link
Author

moznion commented Aug 20, 2023 •
edited

Actually, this is my use case but I'm developing the optional type library for golang and it would be nicer to implement database/sql/driver.Valuer and database/sql.Scanner to map the database nullable values to that optional types. ref: moznion/go-optional#27.
And then, a contributor suggested me it would be good to use sql.convertAssign on Scan to map the value passed by database to the Golang's type. The following patch is what we did, we tried to copy that function but it was hard for us as I mentioned: https://github.com/moznion/go-optional/pull/32/files
Now this implementation was reverted because of imperfect copied code due to the unexposed values, but if I could get a chance I would like to use the officially exposed function instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
3 participants
@moznion @ianlancetaylor @gopherbot