Change (*UUID) Scan to accept type UUID argument #58
Conversation
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 98.91% 98.92% +0.01%
==========================================
Files 4 4
Lines 276 279 +3
==========================================
+ Hits 273 276 +3
Misses 2 2
Partials 1 1
Continue to review full report at Codecov.
|
nanjj
force-pushed
the
sqlscan
branch
2 times, most recently
from
6131f16 to
1784f78
Compare
|
Hello, and thank you for sending this PR! I'm a little bit confused about it though, so please help me understand. The https://golang.org/pkg/database/sql/#Scanner documentation lists the possible types of |
|
Yes, I am using jinzhu/gorm, see below structs: AID is NullUUID type (in order to make it nullabe in db), which refer A.UUID( which is UUID since it's primary, which can not be null in db). gorm does not know how to covert UUID to NullUUID so it calls sql scan, which the UUID value passed in NullUUID. |
There was a problem hiding this comment.
It looks like gorm overloads the sql.Scanner interface somewhat suspiciously, but I think we could help with this nevertheless. Doing the check doesn't hurt.
The commit message explains what the problem is, but it doesn't describe the fix. Please start the commit message with a note along the lines of Change (*NullUUID).Scan to accept arguments of type UUID., then, continue with the existing explanation. Also, please drop the backticks from the commit message.
I have one little concern about the code too, which I have left in a comment. Once these are addressed, I would be inclined to merge the PR, but I would also like at least another pair of eyes on it before I do.
sql.go
Outdated
| @@ -74,6 +74,11 @@ func (u *NullUUID) Scan(src interface{}) error { | |||
| return nil | |||
| } | |||
|
|
|||
| if id, ok := src.(UUID); ok { | |||
There was a problem hiding this comment.
On the code path at line 84, we delegate to UnmarshalText or UnmarshalBinary, which validate the UUID. Here, we do no such thing, and we take it for granted that if src is of type UUID, then it represents a valid UUID.
In the context of your ORM and its usage, is there ever a world in which this src is not a valid UUID?
There was a problem hiding this comment.
In any case, I think we should leave a comment here, which describes why we are doing this. It should mention that certain ORMs use sql.Scanner to convert Go values to other Go values.
nanjj
force-pushed
the
sqlscan
branch
2 times, most recently
from
d35b69d to
8719eca
Compare
nanjj
changed the title
|
@acln0 yes, |
sql.go
Outdated
| @@ -38,6 +38,10 @@ func (u UUID) Value() (driver.Value, error) { | |||
| // a longer byte slice or a string will be handled by UnmarshalText. | |||
| func (u *UUID) Scan(src interface{}) error { | |||
| switch src := src.(type) { | |||
| case UUID: // support gorm convert from UUID to NullUUID | |||
| u = &src | |||
There was a problem hiding this comment.
This is almost certainly not right. It changes the pointer u's value, but not the UUID it points to. *u = src.
sql_test.go
Outdated
| @@ -193,6 +194,21 @@ func testNullUUIDScanValid(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func testNullUUIDScanUUID(t *testing.T) { | |||
| s := UUID{} | |||
There was a problem hiding this comment.
Let's not use the zero value here. It could lead to confusion. Please use codecTestUUID instead.
Type UUID can not used as primary key, which is not NULL in table. Type NullUUID can be used as reference to the primary key in another table, which may be NULL. If we use jinzhu/gorm to implement the usage scenario, jinzhu/gorm convert UUID to NullUUID via NullUUID Scan call. Without the fix below error may be found: uuid: cannot convert uuid.UUID to UUID. This fix changes (*UUID) scan to accept UUID argument.
Type UUID can not used as primary key, which is not NULL in table. Type
NullUUID can be used as reference to the primary key in another table,
which may be NULL. If we use jinzhu/gorm to implement the usage
scenario, jinzhu/gorm convert UUID to NullUUID via NullUUID Scan
call. Without the fix below error may be found: uuid: cannot convert
uuid.UUID to UUID.
This fix changes (*UUID) scan to accept UUID argument.