Handle NULL values in UniqueIdentifier.Scan()

[ngehrsitz]

As mentioned in denisenkom#56 (comment) the current implementation does not handle NULL values gracefully when scanning UUIDs.
In this PR I propose the simple solution to treat such values like an all zeros UUID.

[codecov-commenter]

Codecov Report

Merging #163 (34e68bc) into main (c902b67) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   74.33%   74.59%   +0.26%     
==========================================
  Files          31       32       +1     
  Lines        6253     6298      +45     
==========================================
+ Hits         4648     4698      +50     
+ Misses       1326     1322       -4     
+ Partials      279      278       -1     

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

... and 2 files with indirect coverage changes

[shueybubbles]

Isn't there some concern that a behavior change like this should be a breaking change instead of a silent change? If some apps start thinking they are now getting valid guids when they are not, it could cause issues.
Maybe this mode should be configurable in the connection string or by some global flag and be off by default. #Closed

[shueybubbles]

Here's an idea - maybe the driver should allow apps to provide their own data type/scanner implementation for some types. The lack of interoperability between mssql types and other Go packages, and the lack of type definitions provided by the core sql/driver package, are hard problems for a driver to solve in isolation. If an app wants to use google/uuid which has a nullable format, why not allow the app to replacemssql.UniqueIdentifier with that type as the registered encoder/decoder for mssql.typeGuid? #Closed

[ngehrsitz]

@microsoft-github-policy-service agree

[ngehrsitz]

Isn't there some concern that a behavior change like this should be a breaking change instead of a silent change? If some apps start thinking they are now getting valid guids when they are not, it could cause issues. Maybe this mode should be configurable in the connection string or by some global flag and be off by default.

I do agree that a breaking change poses the risk of breaking implementations that rely on erroring out on NULL uuids. But instead of coming up with some complicated configuration mechanism I would just define a different type like NullableUniqueIdentifier.

[ngehrsitz]

Here's an idea - maybe the driver should allow apps to provide their own data type/scanner implementation for some types. The lack of interoperability between mssql types and other Go packages, and the lack of type definitions provided by the core sql/driver package, are hard problems for a driver to solve in isolation. If an app wants to use google/uuid which has a nullable format, why not allow the app to replacemssql.UniqueIdentifier with that type as the registered encoder/decoder for mssql.typeGuid?

I am actually converting in my code to a google/uuid afterward so you do a have a point there. The main reason why you can´t just use any sql.Scanner implementation is that the byte order when reading from the DB is different:

go-mssqldb/uniqueidentifier.go

Lines 26 to 33 in c902b67

	var raw UniqueIdentifier
	copy(raw[:], vt)
	reverse(raw[0:4])
	reverse(raw[4:6])
	reverse(raw[6:8])
	*u = raw

So we would have to reverse that before handing the data to the Scan() method. Where would you do that then?
From my limited understanding you would have to add that reversing logic somewhere to

go-mssqldb/convert.go

Line 31 in c902b67

func convertAssign(dest, src interface{}) error {

when you invoke

go-mssqldb/convert.go

Lines 162 to 164 in c902b67

	if scanner, ok := dest.(sql.Scanner); ok {
	return scanner.Scan(src)
	}

But then how do you differentiate between Scanners that handle UUIDs which need to have their input reordered and everything else?
The only solution to the byte reordering challenge I can think of is creating a wrapper like this:

type UUIDWrapper[X sql.Scanner] struct {
	uuid X
}

func (u UUIDWrapper[X]) Scan(v interface{}) error {
	reverse := func(b []byte) {
		for i, j := 0, len(b)-1; i < j; i, j = i+1, j-1 {
			b[i], b[j] = b[j], b[i]
		}
	}

	switch vt := v.(type) {
	case [16]byte:
		var raw [16]byte

		copy(raw[:], vt[:])

		reverse(raw[0:4])
		reverse(raw[4:6])
		reverse(raw[6:8])

		return u.uuid.Scan(v)
	default:
		return u.uuid.Scan(v)
	}
}

But that requires upgrading the package to 1.18 to get generics support...

[shueybubbles]

a NullableUniqueIdentifier type sounds fine to me. Any app that wants to work correctly with the driver version having this fix is going to need to change its code anyway.

re: my suggestion about letting the app replace the typeGuid reader, I meant perhaps allowing decodeGuid to be replaced by an app-provided function. It would know it needs to reverse the bytes for consumption by the scanner. Or provide some boolean flag the app could set so the driver reverses the bytes. I guess there'd be corresponding changes needed for inserts/writes which could be harder to do because the driver doesn't know the input is meant to be a guid.

Without something defined for guids in sql package this problem is going to linger. Also, for new features like Always Encrypted to work, an app can't rely on the driver to guess which type to use for inserts based on the Go type. We need a way for the app to specify the SQL Server type explicitly for encrypted columns.
#Closed

[ngehrsitz]

I think the whole deserialization logic needs a bigger overhaul than what´s possible as part of this PR. The way in which it could work is that you pass down the declared data type from the database table into the deserialization logic. Then the logic has all the information and can apply needed data transformations like byte reordering for the uuid type, before handing control to external deserialization functions.
To address this issue for now I force-pushed the new NullableUniqueIdentifier type here, which borrows everything from UniqueIdentifier, but can also handle the nil case.

[shueybubbles]

(scanErr)

an error message would be more helpful than nil

[ngehrsitz]

7ae8aa0

[shueybubbles]

?

[shueybubbles]

ullableUniqueIdentifie

It might be more idiomatic with the nullable types from the sql package (NullInt64, NullString, etc) to use the same pattern.

type NullUniqueIdentifier struct
{
    UniqueIdentifier UniqueIdentifier
    Valid bool
}

[ngehrsitz]

eab3da4

[shueybubbles]

what about a Valid bool instead of relying on an array of zeroes? The empty guid might be a valid non-null value for some applications.

[ngehrsitz]

ccd7616

[shueybubbles]

update the README to show an example usage. Also , add usage of this type to some of the cross-cutting tests like bulk copy.

[ngehrsitz]

update the README to show an example usage. Also , add usage of this type to some of the cross-cutting tests like bulk copy.

I added it to 667bccf. Any other tests you had in mind?
Where in the READMEshould this go? There is currently no mention of UniqueIdentifier and it doesn´t really fit in Parameter Types...

[shueybubbles]

update the README to show an example usage. Also , add usage of this type to some of the cross-cutting tests like bulk copy.

I added it to 667bccf. Any other tests you had in mind? Where in the READMEshould this go? There is currently no mention of UniqueIdentifier and it doesn´t really fit in Parameter Types...

I'd list it in Features

[ngehrsitz]

update the README to show an example usage. Also , add usage of this type to some of the cross-cutting tests like bulk copy.

I added it to 667bccf. Any other tests you had in mind? Where in the READMEshould this go? There is currently no mention of UniqueIdentifier and it doesn´t really fit in Parameter Types...

I'd list it in Features

85f7832 Any remaining blockers to get this merged?

[shueybubbles]

turn n.UUID.Value()

Won't Value/String/Marshal etc need to handle !Valid?
Pls add tests for these. If there are existing functional tests that cover String() and Value() implementations of other types let's add a case to those too.

[ngehrsitz]

I wasn´t entirely sure what !Valid for String should be 34e68bc

[shueybubbles]

does this type need to implement String() ?
I don't see the core nullable types implementing it
https://cs.opensource.google/go/go/+/master:src/database/sql/sql.go?q=nullint64&ss=go%2Fgo

[shueybubbles]

b []byte

i think json would use a literal "null" string

[ngehrsitz]

45eb78e should handle that case

[ngehrsitz]

@shueybubbles Can you take another look?

[shueybubbles]