Skip to content

Implement NamedValueChecker for mysqlConn #690

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 1 commit into from
Nov 16, 2017
Merged

Implement NamedValueChecker for mysqlConn #690

merged 1 commit into from
Nov 16, 2017

Conversation

pushrax
Copy link
Contributor

@pushrax pushrax commented Oct 20, 2017 •
edited
Loading

Fixes #342

Calls to Exec for non-prepared statements with uint64 arguments having high-bits set are currently broken. The typical "uint64 values with high bit set are not supported" error is returned.

_, err := db.Exec("INSERT INTO test (id) VALUES (?)", uint64(0xffffffffffffffff))

Now that the NamedValueChecker interface is in Go, this can be solved by implementing it for mysqlConn. The implementation is based on defaultCheckNamedValue.

I also included changes from golang/go@d7c0de9

cc @shuhaowu @daniellaniyo @hkdsun @fw42

@pushrax
Copy link
Contributor Author

pushrax commented Oct 20, 2017 •
edited
Loading

This breaks the build for Go 1.7 and earlier, so I moved the implementation into the go1.8 file.

@pushrax pushrax force-pushed the named-value-checker branch from 0ce6231 to 5e3001f Compare October 21, 2017 04:44
@fw42
Copy link

fw42 commented Oct 23, 2017

This breaks the build for Go 1.7 and earlier, so I moved the implementation into the go1.8 file.

I assume that means that your binary will only include the fix to this issue if you compile with a newer version of Go? Compiling it with older Go versions will give you an unfixed version (same as before)?

fw42
fw42 reviewed Oct 23, 2017
View reviewed changes
statement.go
@@ -157,6 +157,16 @@ func (c converter) ConvertValue(v interface{}) (driver.Value, error) {
return int64(u64), nil
case reflect.Float32, reflect.Float64:
return rv.Float(), nil
case reflect.Bool:
Copy link

Choose a reason for hiding this comment

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

The changes in this file affect all versions (not just 1.8). Can you explain what this does? In particular what does this do to 1.7 and lower?

Copy link

Choose a reason for hiding this comment

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

Oh nvm I just saw that this is from golang/go@d7c0de9

fw42
fw42 approved these changes Oct 23, 2017
View reviewed changes
Copy link

@fw42 fw42 left a comment

Choose a reason for hiding this comment

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

LGTM as far as I can tell 👍

@pushrax
Copy link
Contributor Author

pushrax commented Oct 23, 2017

Compiling it with older Go versions will give you an unfixed version (same as before)?

Right, the API for making the fix possible is not available in older Go versions.

@julienschmidt
Copy link
Member

julienschmidt commented Oct 27, 2017 •
edited
Loading

The general approach looks fine to me 👍 .
Bonus points, if you implement further tests (e.g. for string #623), as the NamedValueChecker is used also for other values than uint64 with high bit set.

@julienschmidt julienschmidt added this to the v1.4 milestone Oct 27, 2017
@pushrax pushrax force-pushed the named-value-checker branch 2 times, most recently from 8f556d3 to 4214b36 Compare October 30, 2017 20:02
@pushrax
Copy link
Contributor Author

pushrax commented Nov 9, 2017

@julienschmidt ping

julienschmidt
julienschmidt approved these changes Nov 12, 2017
View reviewed changes
Copy link
Member

@julienschmidt julienschmidt left a comment

Choose a reason for hiding this comment

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

Seems good.
I'd like to merge #623 first though. The author of that PR is responsive again.

@julienschmidt
Copy link
Member

#623 is merged. This PR now needs a rebase on (or merge with) the current master.

@pushrax
Implement NamedValueChecker for mysqlConn
e281d84 
* Also add conversions for additional types in ConvertValue
  ref golang/go@d7c0de9
@pushrax pushrax force-pushed the named-value-checker branch from 4214b36 to e281d84 Compare November 16, 2017 03:33
@pushrax
Copy link
Contributor Author

pushrax commented Nov 16, 2017

Done.

@julienschmidt julienschmidt merged commit 78d399c into go-sql-driver:master Nov 16, 2017
@julienschmidt
Copy link
Member

Thanks for contributing!

@busyfree busyfree mentioned this pull request Nov 16, 2017
Closed
@randomjunk randomjunk mentioned this pull request Nov 16, 2017
Closed
randomjunk added a commit to randomjunk/mysql that referenced this pull request Nov 16, 2017
@randomjunk
Fix Valuers by returning driver.ErrSkip if couldn't convert type inte…
875d541 
…rnally

Added a test that Valuer types are handled correctly. This test fails without the fix.

CheckNamedValue code was included recently and breaks existing applications:
go-sql-driver#690

Reported in:
go-sql-driver#708
randomjunk added a commit to randomjunk/mysql that referenced this pull request Nov 16, 2017
@randomjunk
Fix Valuers by returning driver.ErrSkip if couldn't convert type inte…
57372b7 
…rnally

Added a test that Valuer types are handled correctly. This test fails without the fix.

CheckNamedValue code was included recently and breaks existing applications:
go-sql-driver#690

Reported in:
go-sql-driver#708
@randomjunk randomjunk mentioned this pull request Nov 16, 2017
Merged
4 tasks
@yareid yareid mentioned this pull request Jan 23, 2018
Closed
@yareid yareid mentioned this pull request Feb 2, 2018
Closed
5 tasks
@yareid yareid mentioned this pull request Mar 1, 2018
Merged
5 tasks
bLamarche413 pushed a commit to bLamarche413/mysql that referenced this pull request Mar 23, 2018
@pushrax
Implement NamedValueChecker for mysqlConn (go-sql-driver#690)
f9c6a2c 
* Also add conversions for additional types in ConvertValue
  ref golang/go@d7c0de9
@shuhaowu shuhaowu mentioned this pull request Jun 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienschmidt julienschmidt julienschmidt approved these changes

@fw42 fw42 fw42 approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
v1.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@pushrax @fw42 @julienschmidt