fix: Implement sql.Valuer with pointer receiver

[abdusco]

This allows interpreting *uuid.UUID(nil) values as NULL on SQL databases.

Closes: #107

[charlievieth]

This is a breaking change since uuid.UUID values will no longer implement the database/sql/driver.Valuer interface.

[abdusco]

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

[codecov-commenter]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (39d7b1b) compared to base (e1079f3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #108   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          411       414    +3     
=========================================
+ Hits           411       414    +3     

Impacted Files	Coverage Δ
sql.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[charlievieth]

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

[dylan-bourque]

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.

[cameracker]

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!