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

Ensure Prepared Statements are Used Where Necessary #52

Closed
fjordan opened this issue Aug 2, 2018 · 5 comments
Closed

Ensure Prepared Statements are Used Where Necessary #52

fjordan opened this issue Aug 2, 2018 · 5 comments

Comments

@fjordan
Copy link
Contributor

fjordan commented Aug 2, 2018

To ensure MySQL's plain text interface isn't used, we need to make sure this uses prepared statements. A test should be added that fails if non-prepared statements are used to query MySQL.

More context can be gathered from #44 (comment)

@fw42
Copy link
Contributor

fw42 commented Aug 2, 2018

Having a test that fails as soon as you write any kind of non-prepared statement seems a bit overkill to me. Why are non-prepared statements bad in general?

What I was getting at with my comment in #44 (comment) is that this one particular query supposedly has a good reason why it's using a prepared statement (and that reason should be captured by a test). But I imagine the test itself doesn't need to know anything about prepared vs. non-prepared statements (that's an implementation detail, the same bug could probably be solved in some other way), i.e. test the interface, not the implementation.

@fjordan
Copy link
Contributor Author

fjordan commented Aug 2, 2018

Go's MySQL driver has two protocols: 1. Plain text and 2. Binary. The binary protocol is only used with prepared statements, and allows us to know the actual type of the data being transmitted versus just being []uint8s everywhere (unless we know the schema ahead of time and can scan into the specific type).

As far as I can tell, in addition to the performance gain, we need to know the type of the data being transmitted. Are there places we shouldn't be using prepared statements? This project may warrant strictly enforcing that given its purpose, although we do query for things like read_only that obviously don't require type information.

You're right in that we should probably just test in specific places for prepared statements instead of trying to test that any interaction with MySQL is using a prepared statement. I don't think our current interface with MySQL would enable that as we'd have to wrap the driver in our own interface.

@fjordan fjordan changed the title Ensure Prepared Statements are Used Ensure Prepared Statements are Used Where Necessary Aug 2, 2018
@shuhaowu
Copy link
Contributor

shuhaowu commented Aug 3, 2018

We use multistatements to execute binlog updates. Multistatement is only usable over the text interface and not the prepared interface.

@shuhaowu
Copy link
Contributor

shuhaowu commented Nov 1, 2019

Is this still a problem?

@fjordan
Copy link
Contributor Author

fjordan commented Nov 4, 2019

No, I don't think it's worth tracking as an issue right now. If we change the implementation to use non-prepared statements to query data and we cannot correctly handle receiving an []interface{} then we should have plenty of tests that fail.

@fjordan fjordan closed this as completed Nov 4, 2019
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

No branches or pull requests

3 participants