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

mysqlStmt Implements CheckNamedValue #1090

Merged
merged 4 commits into from May 9, 2020

Conversation

zhixinwen
Copy link
Contributor

@zhixinwen zhixinwen commented May 2, 2020

Description

uint support is broken in v1.5.0 when working with instrument library (which is essentially a wrapper around go-sql-driver/mydriver). The issue is in sql, it would check if the wrapper implements CheckNamedValue. The wrapper does implement CheckNamedValue, but it when it checks the underlying mysqlStmtdoes not implement this, so it just returns driver.ErrSkip (which is the correct behavior to me).

In sql, when it sees driver.ErrSkip, it would use ccChecker.CheckNamedValue to do the conversion. However, it would fail in:

	nv.Value, err = c.cci.ColumnConverter(index).ConvertValue(arg)
	if err != nil {
		return err
	}
	if !driver.IsValue(nv.Value) {
		return fmt.Errorf("driver ColumnConverter error converted %T to unsupported type %T", arg, nv.Value)
	}

because nv.Value would be of type uint64 (before 1.5.0, it would return int64), and driver.IsValue would return false.

The easiest way to fix it is to add CheckNamedValue to mysqlStmt. It also makes the driver complies to the general sql protocol.

Checklist

  • Code compiles correctly
  • All tests passing
  • Added myself / the copyright holder to the AUTHORS file

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.

LGTM. Thanks!

Travis CI is currently failing, but that seems to be due to a broken master in golang/go

@zhixinwen zhixinwen closed this May 2, 2020
@zhixinwen zhixinwen reopened this May 2, 2020
@zhixinwen
Copy link
Contributor Author

Looks like it is still occurring, can we merge this in first?

@zhixinwen zhixinwen closed this May 3, 2020
@zhixinwen zhixinwen reopened this May 3, 2020
@julienschmidt
Copy link
Member

We will after #1092 is merged (or the golang/go master is fixed).
Sorry for the wait.

@julienschmidt
Copy link
Member

#1092 was merged. Kindly rebase your PR on the current master branch (git pull --rebase).
Afterwards it should pass the CI check.

@julienschmidt julienschmidt added this to the v1.6.0 milestone May 8, 2020
@zhixinwen
Copy link
Contributor Author

Updated

@julienschmidt julienschmidt merged commit 343c803 into go-sql-driver:master May 9, 2020
@julienschmidt
Copy link
Member

Thanks!

tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
* Add CheckNamedValue for mysqlStmt

* Update AUTHORS

Co-authored-by: Zhixin Wen <zwen@nuro.ai>
tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
* Add CheckNamedValue for mysqlStmt

* Update AUTHORS

Co-authored-by: Zhixin Wen <zwen@nuro.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants