-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix detection of multi-statements in ComPrepare
#359
Conversation
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.
Change make sense; just a couple things I wanted to understand before approving.
go/mysql/query_test.go
Outdated
sql := "select * from test_table where id = ?" | ||
sql := "select ?; select ?" | ||
//sql := "select * from test_table where id = ?" |
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.
Looks like this is just a debugging/testing change? Should this test fail when multiple statements are included? Seems like it's still passing though?
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.
Oops yeah, this was a debugging change.
This test should fail when there are multiple statments; it doesn't currently because the check is in handleNextCommand
.
I'll move this test case to a different testing suite, and check for an error.
go/mysql/query_test.go
Outdated
@@ -1114,3 +1115,36 @@ func TestExecuteQueries(t *testing.T) { | |||
} | |||
}) | |||
} | |||
|
|||
func TestComPrepareMultiple(t *testing.T) { |
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.
Maybe rename to TestComStmtPrepareWithTrailingNewline
or something? That's a little more descriptive of what you're testing here (there aren't actually multiple statements, right?) and more consistent with the naming of the existing test TestComStmtPrepare
.
Speaking of multiple statements... do we have a test in here that tests the "prepare fails for multiple statements" case? It seems like that would be useful if we don't have it.
c.StatementID++ | ||
prepare := &PrepareData{ | ||
StatementID: c.StatementID, | ||
PrepareStmt: queries[0], | ||
} | ||
|
||
statement, err = sqlparser.ParseWithOptions(ctx, query, parserOptions) |
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.
Looks like we will still execute this code even if we send back the error response packet about not being able to prepare multiple statements? Should we return from this function earlier to avoid letting this execute in that case?
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.
Looks good! 🚢
Currently, preparing multi-statements is not supported; so we can't prepare a query like
select ?; select ?;
.However, the check for this condition just looked for any characters after the first
;
, which meant that queries likeselect ?; \n
would incorrectly throw an error.This was made apparent using the Prisma ORM, which runs the query:
The above query ends in a newline character.
The fix is to use
SplitStatementToPieces()
, which trims these white space characters, and check if there's exactly one piece; this was taken from the vitessio repo: https://github.com/vitessio/vitess/blob/main/go/mysql/conn.go#L1204fixes dolthub/dolt#8157