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

WIP: fix: revert batch executions that silently accept select statements #983

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ public void handleResultRows(Query fromQuery, Field[] fields, List<byte[][]> tup
// If SELECT, then handleCommandStatus call would just be missing
resultIndex++;
if (!expectGeneratedKeys) {
// No rows expected -> just ignore rows
return;
handleError(new PSQLException(GT.tr("A result was returned when none was expected."),
PSQLState.TOO_MANY_RESULTS));
}
if (generatedKeys == null) {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,25 +214,24 @@ public void testClearPreparedEmptyBatch() throws Exception {
}

@Test
public void testSelectInBatch() throws Exception {
public void testSelectThrowsException() throws Exception {
Statement stmt = con.createStatement();

stmt.addBatch("UPDATE testbatch SET col1 = col1 + 1 WHERE pk = 1");
stmt.addBatch("SELECT col1 FROM testbatch WHERE pk = 1");
stmt.addBatch("UPDATE testbatch SET col1 = col1 + 2 WHERE pk = 1");

// There's no reason to Assert.fail
int[] updateCounts = stmt.executeBatch();

Assert.assertTrue("First update should succeed, thus updateCount should be 1 or SUCCESS_NO_INFO"
+ ", actual value: " + updateCounts[0],
updateCounts[0] == 1 || updateCounts[0] == Statement.SUCCESS_NO_INFO);
Assert.assertTrue("For SELECT, number of modified rows should be either 0 or SUCCESS_NO_INFO"
+ ", actual value: " + updateCounts[1],
updateCounts[1] == 0 || updateCounts[1] == Statement.SUCCESS_NO_INFO);
Assert.assertTrue("Second update should succeed, thus updateCount should be 1 or SUCCESS_NO_INFO"
+ ", actual value: " + updateCounts[2],
updateCounts[2] == 1 || updateCounts[2] == Statement.SUCCESS_NO_INFO);
try {
stmt.executeBatch();
Assert.fail("Should raise a BatchUpdateException because of the SELECT");
Copy link
Member

Choose a reason for hiding this comment

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

Please add explicit reference that clarifies why "select should raise BatchUpdateException"

} catch (BatchUpdateException e) {
int[] updateCounts = e.getUpdateCounts();
// There are 3 batches
Assert.assertEquals(3, updateCounts.length);
Assert.assertEquals(Statement.EXECUTE_FAILED, updateCounts[0]);
Copy link
Member

Choose a reason for hiding this comment

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

All the contents of resulting array should be verified, not just the first element

} catch (SQLException e) {
Assert.fail("Should throw a BatchUpdateException instead of a generic SQLException: " + e);
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid +e pattern, and add reference to the spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this was just a quick copy/revert of the method. I need to verify the correct behavior.

}

stmt.close();
}
Expand Down