-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
mysqlStmt Implements CheckNamedValue #1090
Conversation
Add CheckNamedValue for mysqlStmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Travis CI is currently failing, but that seems to be due to a broken master in golang/go
Looks like it is still occurring, can we merge this in first? |
We will after #1092 is merged (or the golang/go |
#1092 was merged. Kindly rebase your PR on the current master branch ( |
Updated |
Thanks! |
* Add CheckNamedValue for mysqlStmt * Update AUTHORS Co-authored-by: Zhixin Wen <[email protected]>
* Add CheckNamedValue for mysqlStmt * Update AUTHORS Co-authored-by: Zhixin Wen <[email protected]>
Description
uint support is broken in
v1.5.0
when working with instrument library (which is essentially a wrapper aroundgo-sql-driver/mydriver
). The issue is insql
, it would check if the wrapper implementsCheckNamedValue
. The wrapper does implementCheckNamedValue
, but it when it checks the underlyingmysqlStmt
does not implement this, so it just returnsdriver.ErrSkip
(which is the correct behavior to me).In
sql
, when it seesdriver.ErrSkip
, it would useccChecker.CheckNamedValue
to do the conversion. However, it would fail in:because nv.Value would be of type
uint64
(before 1.5.0, it would returnint64
), anddriver.IsValue
would return false.The easiest way to fix it is to add
CheckNamedValue
tomysqlStmt
. It also makes the driver complies to the general sql protocol.Checklist