Skip to content

Fix TIME value sign byte #140

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 28, 2013
Merged

Fix TIME value sign byte #140

merged 4 commits into from
Oct 28, 2013

Conversation

xiam
Copy link
Contributor

@xiam xiam commented Oct 27, 2013

Attempts to fix #139

@xiam
This commit attempts to fix #139 by
ed7d907 
using an empty string ("") instead of the 0 byte []byte('0') when
formatting positive TIME fields.
@julienschmidt
Copy link
Member

Thanks for the PR.
Interesting, that nobody noticed until now... this is in there for a while.
A test would be nice.

@julienschmidt
Copy link
Member

I mean a test in driver_test.go 😉

@xiam
Copy link
Contributor Author

xiam commented Oct 28, 2013

Of course Julien :-),

I've added a test and ran it against the master branch. The message looks funny:

--- FAIL: TestTime (0.04 seconds)
        driver_test.go:546: time values differ: got "12:34:56", expecting "12:34:56".
FAIL
exit status 1
FAIL    github.com/go-sql-driver/mysql  2.951s

Damn invisible chars!

The same test succeeds with the proposed fix:

PASS
ok      github.com/xiam/gomysql 3.219s

@xiam
Copy link
Contributor Author

xiam commented Oct 28, 2013

I'm still curious why this happens only when using "?" arguments in queries.

@arnehormann
Copy link
Member

First off: Thank you for the Pull Request!

The bug only happens when you use "?" because that (or prepared statements) trigger the binary protocol, otherwise the text protocol is used. In the text protocol, the TIME fields are sent and returned as strings. See readRow in packets.go.

LGTM 👍

julienschmidt
julienschmidt reviewed Oct 28, 2013
View reviewed changes
driver_test.go
@@ -514,6 +514,45 @@ func TestDateTime(t *testing.T) {
}
}

// This tests for https://github.com/go-sql-driver/mysql/pull/140
Copy link
Member

Choose a reason for hiding this comment

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

The bug report was #139
140 is the fix ;)

julienschmidt
julienschmidt reviewed Oct 28, 2013
View reviewed changes
driver_test.go
for _, sTime := range sTimes {
dbt.db.Exec("DROP TABLE IF EXISTS test")
dbt.mustExec("CREATE TABLE test (id INT, time_field " + sTime.fieldType + ")")
dbt.mustExec("TRUNCATE TABLE test")
Copy link
Member

Choose a reason for hiding this comment

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

A new table is always empty 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duh! yeah. Initially the CREATE statement was outside the loop and it made sense once, however it is of no use anymore.

@julienschmidt
Copy link
Member

LGTM

julienschmidt added a commit that referenced this pull request Oct 28, 2013
@julienschmidt
Merge pull request #140 from xiam/master
ae73333 
Fix TIME value sign byte
@julienschmidt julienschmidt merged commit ae73333 into go-sql-driver:master Oct 28, 2013
@julienschmidt julienschmidt mentioned this pull request Oct 28, 2013
Closed
julienschmidt added a commit that referenced this pull request Oct 30, 2013
@julienschmidt
AUTHORS: Add Carlos Nieto
1429b99 
Forgot to add him in #140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

Bad extra byte (0x00) at the beginning of TIME values.
3 participants
@xiam @julienschmidt @arnehormann