Closed abdusco closed 1 year ago
This is a breaking change since uuid.UUID
values will no longer implement the database/sql/driver.Valuer
interface.
breaking change
How so? What needs to be changed by the users?
package main
import (
"database/sql/driver"
"fmt"
)
type UUIDP struct {
}
func (u *UUIDP) Value() (driver.Value, error) {
return "pointer", nil
}
type UUIDNP struct {
}
func (u UUIDNP) Value() (driver.Value, error) {
return "non pointer", nil
}
func main() {
// regular
var u UUIDP
fmt.Println(u.Value())
var up *UUIDP
// calling .Value on pointer implementation works ok.
fmt.Println(up.Value()) // works
if _, ok := ((any)(up)).(driver.Valuer); ok { // implements
fmt.Printf("%T implements driver.Valuer\n", up)
}
// non-pointer
var np UUIDNP
fmt.Println(np.Value())
var npp *UUIDNP
if _, ok := ((any)(npp)).(driver.Valuer); ok { // implements
fmt.Printf("%T implements driver.Valuer\n", npp)
}
// calling the implementation on a non-pointer method panics
fmt.Println(npp.Value())
}
run it on playground: https://go.dev/play/p/6B2INaIbb0h
Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:
Coverage data is based on head (
39d7b1b
) compared to base (e1079f3
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
breaking change
How so? What needs to be changed by the users?
This will break all code bases that expect a UUID value (not pointer) to implement database/sql/driver.Valuer
. Add var _ driver.Valuer = UUID{}
to your change and it will not compile. Basically, all code bases that pass UUID's as values will need to change their code to pass them as pointers (failures will sometimes occur at compile time, but will more often occur at runtime due to interface{}
, which is delightfully bad). This is subtle and has to do with the how interfaces in Go work, which can be a bit weird.
Additionally, you can check this using the actual sql
package:
package main
import (
"database/sql"
"github.com/gofrs/uuid"
_ "github.com/mattn/go-sqlite3"
)
func main() {
db, err := sql.Open("sqlite3", ":memory:")
if err != nil {
panic(err)
}
defer db.Close()
_, err = db.Exec(`CREATE TABLE uuids (id INTEGER PRIMARY KEY, uuid TEXT NOT NULL);`)
if err != nil {
panic(err)
}
u, err := uuid.NewV4()
if err != nil {
panic(err)
}
_, err = db.Exec(`INSERT INTO uuids (uuid) VALUES (?);`, u)
if err != nil {
panic(err)
}
var found uuid.UUID
err = db.QueryRow(`SELECT (uuid) FROM uuids WHERE uuid = ?`, u).Scan(&found)
if err != nil {
panic(err)
}
if found != u {
panic("WAT")
}
println("Ok")
}
With your proposed change the program will error with:
panic: sql: converting argument $1 type: unsupported type uuid.UUID, a array
goroutine 1 [running]:
main.main()
/usr/home/go/src/github.com/gofrs/uuid/sqlite.go:28 +0x2bc
In addition to this being a breaking change as @charlievieth called out, NullUUID
exists to cover nullable database columns following the established pattern of database/sql.NullString
, etc. This change would introduce ambiguity around how consumers are supposed to use the library.
Hi @abdusco really appreciate that you identified this gap and took the time to submit a fix.
As others have stated, this is a breaking change to the library and as is I don't believe we can accept this donation. There's another PR that address a similar need (#113). I'll give that PR a couple of days before I merge it, would love to hear your thoughts over there.
Thanks again!
This allows interpreting
*uuid.UUID(nil)
values asNULL
on SQL databases.Closes: https://github.com/gofrs/uuid/issues/107