feat(contrib/drivers/mssql): convert uniqueidentifier to uuid.UUID#4756
Open
NickWilde18 wants to merge 2 commits into
Open
feat(contrib/drivers/mssql): convert uniqueidentifier to uuid.UUID#4756NickWilde18 wants to merge 2 commits into
NickWilde18 wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds MSSQL-specific handling for SQL Server UNIQUEIDENTIFIER so result decoding returns uuid.UUID (rather than mojibake) by swapping the wire byte order via go-mssqldb’s UniqueIdentifier.Scan, mirroring existing UUID handling in other drivers.
Changes:
- Add
CheckLocalTypeForFieldoverride to mapuniqueidentifier→gdb.LocalTypeUUID(case-insensitive). - Add
ConvertValueForLocaloverride to decodeUNIQUEIDENTIFIERintouuid.UUID. - Add unit tests covering type mapping and conversion, and promote
github.com/google/uuidto a direct dependency for the MSSQL contrib module.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| contrib/drivers/mssql/mssql_convert.go | Introduces MSSQL-specific type detection and value conversion for uniqueidentifier to uuid.UUID. |
| contrib/drivers/mssql/mssql_z_unit_convert_test.go | Adds unit tests validating type mapping and correct wire-format UUID decoding behavior. |
| contrib/drivers/mssql/go.mod | Promotes github.com/google/uuid to a direct dependency for this driver module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SQL Server UNIQUEIDENTIFIER values arrive on the wire as 16 bytes whose first 8 follow the little-endian COM/Win32 GUID layout while the remaining 8 are big-endian. The MSSQL driver previously had no override for this column type, so gdb's default conversion path treated the raw bytes as a string and returned garbage characters (any caller doing `row["id"].String()` on a uniqueidentifier column would get mojibake). This change mirrors the existing pgsql `uuid` handling: add `CheckLocalTypeForField` and `ConvertValueForLocal` overrides that detect `uniqueidentifier` (case-insensitive), delegate the byte-order swap to `mssql.UniqueIdentifier.Scan`, and return a `uuid.UUID`. All other column types fall through to `Core` unchanged. Tests cover the wire-format byte swap, the string form (used after a server-side `CAST AS NVARCHAR(36)`), case-insensitive type matching, fallthrough behaviour, and an invalid-length error path.
98b133b to
8ecf492
Compare
Author
|
Thanks @copilot-pull-request-reviewer for the review. Both points addressed in 8ecf492:
Removed the now-unused |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
07e62d1 to
245b987
Compare
- Refer to go-mssqldb's UniqueIdentifier.Scan explicitly so the docs cannot be misread as a symbol on this package. - Drop the nvarchar(100) fallthrough assertion: it exercised gdb.Core's default path with a nil receiver, not the new uniqueidentifier hook, and its passing was coincidental to Core's current implementation.
245b987 to
69c947d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SQL Server
UNIQUEIDENTIFIERcolumns arrive on the wire as 16 bytes whose first 8 follow the little-endian COM/Win32 GUID layout while the remaining 8 are big-endian. The MSSQL driver previously had no override for this column type, sogdb's default conversion path treated the raw bytes as a string and returned garbage characters. Any caller doingrow[\"id\"].String()on auniqueidentifiercolumn would get mojibake.This PR mirrors the existing pgsql
uuidhandling: addCheckLocalTypeForFieldandConvertValueForLocaloverrides on the MSSQL driver that detectuniqueidentifier(case-insensitive), delegate the byte-order swap tomssql.UniqueIdentifier.Scan, and return auuid.UUID. All other column types fall through toCoreunchanged.Reproduction (before this PR)
```sql
CREATE TABLE t_uuid (id UNIQUEIDENTIFIER NOT NULL DEFAULT NEWID(), label NVARCHAR(50));
INSERT INTO t_uuid (label) VALUES ('hello');
```
```go
rows, _ := db.Model("t_uuid").Ctx(ctx).Fields("id, label").All()
for _, r := range rows {
fmt.Println(r["id"].String()) // garbage characters like "\ufffdԓ\ufffd?\"\ufffdB\ufffdGx\ufffd..."
}
```
After this PR the same call returns the canonical UUID string (e.g.
da93d4f6-223f-42b2-a647-789371ffa693).Tests
Test_CheckLocalTypeForField_UniqueIdentifier— verifiesuniqueidentifier(andUNIQUEIDENTIFIER) maps toLocalTypeUUID, and that unknown types still fall through to Core (nvarchar(100) → LocalTypeString).Test_ConvertValueForLocal_UniqueIdentifier— covers the wire-format byte swap, the string form (e.g. after a server-sideCAST AS NVARCHAR(36)), a sanity check againstmssql.UniqueIdentifier.Scandirectly, and the invalid-length error path.These unit tests do not require a running MSSQL instance — they exercise the driver hooks against fixture inputs. Existing DB-backed tests continue to use the CI's MSSQL container unchanged.
Notes
github.com/google/uuidis promoted from indirect to direct dependency incontrib/drivers/mssql/go.mod(already pulled in transitively viamicrosoft/go-mssqldb).