Fix GUID conversion

[andrea-mgn]

GUIDs are not handled like drivers for other SQL dialects (postgres, mariaDB, ...)
Someone had already signaled an issue:
https://stackoverflow.com/questions/70119954/go-mssqldb-returns-incorrect-id-value
But seems it has not been fixed in the meantime.

I'm not sure which would be the best way to change it, but we'd need a driver behaving like other ones.

[aberezin777]

for guid type?

[shueybubbles]

It's why the driver has its own mssql.UniqueIdentifier type. I'm worried that changing the default behavior now could be a huge breaking change for other clients.

[shueybubbles]

I think we could take this, as long as we document the breaking change in the readme and CHANGELOG.md.
Take a look at comments from the earlier attempt in the old repo: denisenkom#505
The prior driver owner had requested a couple additional tests to verify round tripping and string conversion.

[shueybubbles]

also - see if frameworks like gorm have implemented any workarounds like reordering the slice already. They'll need to undo those workarounds if they pick up a version with this fix. Filing an issue against them could remind them to do so.

[shueybubbles]

another option is to make the behavior dependent on a connection string property, and new apps would set it, or a Connector property, etc.

[andrea-mgn]

@shueybubbles, thanks for your comments.
Yes, I think that adding a property on a connection string would be a viable option (would it be read in msdsn.Config?), but I don't know what would be the proper way to pass it to the functions in types.go. (probably adding a flag in tdsSession then passing it explicitly to every function, but maybe there's a better way).
Another thing I cannot fix is the tests involving nullable UUIDs. For some reason the conversion isn't used properly and byte order isn't correct, unlike non-nullable UUIDs, and I cannot find which is the difference. Could you please advise how to fix it?

[andrea-mgn]

@shueybubbles I've implemented the management of a new parameter in the connection string, named "guid conversion", to enabled the proper conversion of GUID; I've also duplicated involved tests so that they are executed both with and without this parameter.
I still have 2 problems, for which I'd need your help:

• I'm not sure what would be the better way to pass this flag to TVP.encode (currently PID fields in TestTVP tests is not converted correctly due to the lack of this part)
• I have no idea why nullable unique identifiers are not converted correctly when this parameter is enabled.

[andrea-mgn]

@shueybubbles, could you please help in fixing tests as I explained in the previous comment?

[shueybubbles]

Sorry, I've not had much time to work on these driver issues lately. I will try to make some time in the next couple weeks.

[andrea-mgn]

Sorry, I've not had much time to work on these driver issues lately. I will try to make some time in the next couple weeks.

ok, thank you

[andrea-mgn]

I updated the PR, now I've solved the handling of TVP.encode, but it still fails the one about nullable Uniqueidentifier.
I tried debugging, when enabling GuidConversion, for both parseRow and parseNbcRow, the result returned by the Reader (the same one for both fields) inside columnContent:
for p_id is [103 69 35 1 171 137 239 205 1 35 69 103 137 171 205 239], correct;
for p_idNull is [1 35 69 103 137 171 205 239 1 35 69 103 137 171 205 239], byte swapping didn't happened (or more likely it happened twice).
@bharathpalaksha , @stuartpa, @PeteBassettBet365, could you please help to understand how to fix it?

[andrea-mgn]

@shueybubbles, could you please help understanding why tests for nullable unique identifier don't pass, unlike the one for unique identifiers?

[shueybubbles]

@andrea-mgn I think it's because the switch statement in convertAssignRows treats a &UniqueIdentifier as []byte

sql.convertAssignRows (c:\Program Files\Go\src\database\sql\convert.go:243)
sql.(*Rows).Scan (c:\Program Files\Go\src\database\sql\sql.go:3354)
go-mssqldb.testTVP_WithTag (f:\git\go-mssqldb\tvp_go19_db_test.go:957)
go-mssqldb.TestTVP_WithTagAndGuidConversion (f:\git\go-mssqldb\tvp_go19_db_test.go:1126)
testing.tRunner (c:\Program Files\Go\src\testing\testing.go:1689)
testing.(*T).Run.gowrap1 (c:\Program Files\Go\src\testing\testing.go:1742)
runtime.goexit (c:\Program Files\Go\src\runtime\asm_amd64.s:1695)

It might need a case entry for UniqueIdentifier

case []byte:
  switch d := dest.(type) {
  case *string:
   if d == nil {
    return errNilPtr
   }
   *d = string(s)
   return nil
  case *any:
   if d == nil {
    return errNilPtr
   }
   *d = bytes.Clone(s)
   return nil
  case *[]byte:
   if d == nil {
    return errNilPtr
   }
   *d = bytes.Clone(s)
   return nil
  case *RawBytes:
   if d == nil {
    return errNilPtr
   }
   *d = s
   return nil
  }

---

In reply to: 2249651953

[andrea-mgn]

@shueybubbles, many thanks for your analysis, but that is a standard go library, hence I would not know how to fix it.
@aberezin777 FYI

In reply to: 2249651953

[shueybubbles]

oh you're right, I wasn't looking closely at the file paths.
It might be that to use your parameter with nullable guids, the app would have to use the NullUniqueIdentifier or []byte type instead of UniqueIdentifier

Considering the goal of your change is to enable apps to get non-SQL Server formatted guids, such apps are not likely to use the UniqueIdentifier type in the first place. Maybe the tests can exclude that type when covering the new parameter.

---

In reply to: 2550719670

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 88.60759% with 9 lines in your changes missing coverage. Please review.

Project coverage is 74.77%. Comparing base (dad23d2) to head (54eeb3e).

Files with missing lines	Patch %	Lines
types.go	89.06%	5 Missing and 2 partials ⚠️
mssql.go	0.00%	0 Missing and 1 partial ⚠️
token.go	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
+ Coverage   74.71%   74.77%   +0.06%     
==========================================
  Files          32       32              
  Lines        6410     6457      +47     
==========================================
+ Hits         4789     4828      +39     
- Misses       1333     1338       +5     
- Partials      288      291       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[andrea-mgn]

@shueybubbles, thank you.
I agree with you, so I've disabled tests for nullable identifiers in case of guidConversions.
It passes tests, and code coverage seems similar with the rest.

[shueybubbles]

thx. Please update the README with appropriate documentation for how to use this new parameter and what its limitations are.

---

In reply to: 2553394960

[andrea-mgn]

@shueybubbles, thanks. I've described briefly the new parameter in README.md, please check as I'm not a native speaker.