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

add support for driver.NamedValueChecker on driver connection #887

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

dvilaverde
Copy link
Contributor

This PR is to address issue #886, which prevents a client from using custom value types, specifically uint64 in prepared statements.

Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

And just for #886, I'm not sure to handle the large unit64 value, simply skipping the checker can work well. (will the driver write legal payload?)

driver/driver_options_test.go Show resolved Hide resolved
driver/driver.go Show resolved Hide resolved
@dvilaverde
Copy link
Contributor Author

dvilaverde commented Jun 11, 2024

And just for #886, I'm not sure to handle the large unit64 value, simply skipping the checker can work well. (will the driver write legal payload?)

Based on the documentation calling ErrSkip will cause the DefaultParameterConverter to be used instead. The driver will write a legal payload, but I'm going to update the tests for this.


// the NamedValueChecker will return ErrSkip for types that are NOT uint64, so make
// sure those make it to the server ok first.
int32Stmt, err := conn.Prepare("select ?")
Copy link
Contributor Author

@dvilaverde dvilaverde Jun 11, 2024

Choose a reason for hiding this comment

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

lines 149-159 tests that the nameValueChecker falls back to the driver.DefaultParameterConverter if ErrSkip is returned.


conn.Close()
}

func TestDriverOptions_namedValueChecker(t *testing.T) {
AddNamedValueChecker(func(nv *sqlDriver.NamedValue) error {
rv := reflect.ValueOf(nv.Value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also checked it can be written as

		_, ok := nv.Value.(uint64)
		if !ok {
			return sqlDriver.ErrSkip
		}

		return nil

Maybe it's faster to use golang's built-in type cast rather than reflection in your application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yea thanks. This is much better.

Unrelated question, how often do you, or what is the criteria to, create a tag for a new version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As a user, our project always uses "master" branch 😂 So generally when other contributors ask for a release I ping @atercattus to release it.

@lance6716 lance6716 merged commit c607c3c into go-mysql-org:master Jun 12, 2024
13 checks passed
@dvilaverde dvilaverde deleted the named_value_checker branch June 23, 2024 11:25
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