Skip to content

Conversation

@tooilxui
Copy link

@tooilxui tooilxui commented Jun 7, 2022

this RP contains huiyu's contribution in #179, and add test for #178

@tooilxui
Copy link
Author

tooilxui commented Jun 7, 2022

@alexbrainman please review this PR, if there is any suggestion please let me know.

@alexbrainman
Copy link
Owner

@tooilxui your new test fails

=== RUN   TestMSSQLIssue178
    mssql_test.go:1977: people name missmatch, ?? != 張三
--- FAIL: TestMSSQLIssue178 (0.20s)
FAIL

on my Linux computer

go version devel go1.19-a11a885cb5 Mon Apr 18 23:57:00 2022 +0000 linux/amd64

Alex

@tooilxui
Copy link
Author

@alexbrainman I had change column type to nvarchar and test passed.

$ go test -run ^TestMSSQLIssue178$ github.com/alexbrainman/odbc
ok      github.com/alexbrainman/odbc    0.617s

go version:

$ go version
go version go1.18.3 windows/amd64

sql server info:

Microsoft SQL Server 2022 (CTP2.0) - 16.0.600.9 (X64) 
	May 20 2022 13:29:42 
	Copyright (C) 2022 Microsoft Corporation
	Developer Edition (64-bit) on Linux (Ubuntu 20.04.4 LTS) <X64>
COLLATION: SQL_Latin1_General_CP1_CI_AS

@alexbrainman
Copy link
Owner

@tooilxui but now your new test (TestMSSQLIssue178) passes even if I revert your fix (df72402).

Alex

@tooilxui
Copy link
Author

tooilxui commented Jun 11, 2022

@alexbrainman I had try another way to test, but in GBK test still failed, could you help me figure it out? (I think BIG5 case is correct)

@alexbrainman
Copy link
Owner

@tooilxui sorry but I don't have time to debug this. And I don't speak Chinese. And I am unfamiliar with language specific features in MS SQL. So I don't know whether you test is correct or not.

Alex

Copy link
Author

@tooilxui tooilxui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexbrainman test of big5 and gbk are both work now. please review again.

// query inserted poem and compare with testCase.poem
var rowPoem string
if err = db.QueryRow(
"select cast(poem as nvarchar(max)) from dbo.temp",
Copy link
Author

@tooilxui tooilxui Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexbrainman
It seems that sql server automatically detects the client locale and converts the query value to the client charset (like following picture), so I decided to convert the query value to nvarchar to ensure the value is in unicode encoding. The test compares inserted value (testCase.poem) and query value are both in unicode.

I checked every single byte of query value, and characters not compatible with big5 (yellow part) are all turns to 3F (question mark / ?). my client locale is zh_tw.big5.
image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexbrainman I found this article on stackflow which explains how client charset works on sqlserver. just match with my guess.

@tooilxui
Copy link
Author

@alexbrainman hello,do you have time to take a look?

@alexbrainman
Copy link
Owner

Your test succeeds if I revert the fix:

a@spot:~/src/github.com/alexbrainman/odbc$ git rev-parse HEAD
5b9fdf0f0dfabd0cc9739156689ae18ec283ba75
a@spot:~/src/github.com/alexbrainman/odbc$ git diff
diff --git a/param.go b/param.go
index 53f84d8..a4cff66 100644
--- a/param.go
+++ b/param.go
@@ -70,9 +70,9 @@ func (p *Parameter) BindValue(h api.SQLHSTMT, idx int, v driver.Value, conn *Con
                                sqltype = api.SQL_WLONGVARCHAR
                        case p.isDescribed:
                                sqltype = p.SQLType
-                               if p.Size != 0 {
-                                       size = p.Size
-                               }
+                               //if p.Size != 0 {
+                               //      size = p.Size
+                               //}
                        case size <= 1:
                                sqltype = api.SQL_WVARCHAR
                        default:
a@spot:~/src/github.com/alexbrainman/odbc$ go test -v -mssrv=localhost -msdb=test -msuser=sa -mspass=Passw0rd -run=TestMSSQLIssue178
=== RUN   TestMSSQLIssue178
--- PASS: TestMSSQLIssue178 (11.85s)
PASS
ok      github.com/alexbrainman/odbc    11.855s
a@spot:~/src/github.com/alexbrainman/odbc$

Your test should fail instead.

Also you test cannot be run second time. If I run your test second time, this happens:

a@spot:~/src/github.com/alexbrainman/odbc$ go test -v -mssrv=localhost -msdb=test -msuser=sa -mspass=Passw0rd -run=TestMSSQLIssue178
=== RUN   TestMSSQLIssue178
    mssql_test.go:1978: SQLDriverConnect: {42000} [FreeTDS][SQL Server]Login failed for user 'sa'.
        {42000} [FreeTDS][SQL Server]Cannot open database "test" requested by the login. The login failed.
        {08001} [FreeTDS][SQL Server]Unable to connect to data source
--- FAIL: TestMSSQLIssue178 (0.51s)
FAIL
exit status 1
FAIL    github.com/alexbrainman/odbc    0.516s
a@spot:~/src/github.com/alexbrainman/odbc$

Alex

@tooilxui
Copy link
Author

tooilxui commented Aug 6, 2022

@alexbrainman

  • that fix only need and work when client locale not set to UTF8, try change locale to zh_TW.BIG5 or others will see right result.
  • in that test, will drop database which named "test" and create again with different "colleges" in each test cases. sqlserver don't allow you drop database you're using now, so the flag -msdb=test should not set to test.

@alexbrainman
Copy link
Owner

  • that fix only need and work when client locale not set to UTF8, try change locale to zh_TW.BIG5 or others will see right result.

Isn't your test supposed to be responsible for setting everything to demonstrate the bug in existing code?

  • in that test, will drop database which named "test" and create again with different "colleges" in each test cases. sqlserver don't allow you drop database you're using now, so the flag -msdb=test should not set to test.

I run this Makefile command

odbc/Makefile

Line 19 in 9c9a2e6

test-mssql:
to test this package. If I need more commands to test your change, then we need to add these commands. But, please, address top problem first, before making test more complicated.

Thank you.

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants