Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix return error value when login fails #8

Merged
merged 1 commit into from
Jul 8, 2019
Merged

Fix return error value when login fails #8

merged 1 commit into from
Jul 8, 2019

Conversation

grethy
Copy link
Contributor

@grethy grethy commented Jul 8, 2019

At login failure the returned error is always nil and this creates a connection at sql.Open that is in an invalid state instead of reporting the sybase error that happened during the connection.

After the fix it returns the correct error value and the sql.Open call properly reports the error message

At login failure the returned error is always nil and this creates a connection that is in an invalid state.
Fixed the return so that it refers to the correct error value
@thda thda merged commit 573470d into thda:master Jul 8, 2019
@thda
Copy link
Owner

thda commented Jul 8, 2019

Thanks, looks good to me. Merged. Cheers.

@thda
Copy link
Owner

thda commented Jul 8, 2019

Needs a test case. If you have time to do it prs are welcome. Otherwise I will do it

@grethy
Copy link
Contributor Author

grethy commented Jul 9, 2019

Ups, so after some more testing i realized that my description was wrong and it still does not die on the sql.Open (as it should be, since sql.Open does not actually create a connection, i was just not thinking), however the db.Ping() will now report the correct error instead of the old message that comes from the database/sql module when it does not know whats wrong.

Old:
Ping error: driver: bad connection

New:
Ping error: Msg: 4002, Level: 14, State: 1
Server: MYSERVER, Line: 0:
Login failed.

and that helps diagnose login issues. Like in my case the server charset was iso_1 and the default utf-8 setting did not work, but the server errorlog only contained a line about a failed login attempt.

As it is, the TestBadConnect test in driver_test.go captures this and was capturing the previous one too, since its only checking if err contains any message or not. Shall I change it so that it checks that the first line starts with "Msg: 4002" ?

@thda
Copy link
Owner

thda commented Jul 9, 2019

Hi,

yes, if you can search for "Login failed" it would be great!

Thanks
Thomas

@thda
Copy link
Owner

thda commented Jul 9, 2019

By the way using SetErrorhandler as mentionned in the readme will allow you to print all the error messages.
You should have another message than "Login failed" sent by the server right before, and a more explanatory one.

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