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

fix: batch execute with comments results in error #470

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

crystall-bitquill
Copy link
Contributor

@crystall-bitquill crystall-bitquill commented Oct 5, 2023

Summary

Batch execute with comments results in error

Description

Batch execute with comments resulted in errors about being unable to issue statements that produce result sets. The isNonResultSetProducingQuery method was missing the None query return type.

Addresses #464

Additional Reviewers


/**
* Test fix for Issue #464. Executing a batch statement should ignore any comments.
*

Choose a reason for hiding this comment

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

NIT: I'd suggest adding an extra line like
"Verifying that isNonResultSetProducingQuery() properly handles queries that return queryReturnType == QueryReturnType.NONE

bstmt.addBatch("-- Comment 6");

try {
bstmt.executeBatch();

Choose a reason for hiding this comment

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

is addBatch() that returns NONE or executeBatch() or both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The executeBatch() method triggers the query type check, addBatch() will not.

@benkanlu
Copy link

Yes, that did seem to fix the problems with .batchUpdate() not handling comments correctly. Thank you. I would suggest also opening a PR to the Oracle MySQL repo (https://github.com/mysql/mysql-connector-j) so that your fix will eventually get applied there. But, I guess if no additional work is going to be done on this driver for AWS, no point. Thank you, regardless!

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.

4 participants