You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by akapps <gi...@git.apache.org> on 2017/08/15 07:51:50 UTC
[GitHub] groovy pull request #586: GROOVY-8288 executeBatch() should not be called wi...
GitHub user akapps opened a pull request:
https://github.com/apache/groovy/pull/586
GROOVY-8288 executeBatch() should not be called with batchCount == 0
This is a first proposition of resolution for [GROOVY-8288](https://issues.apache.org/jira/browse/GROOVY-8288).
There still are open questions (see my comments on changes).
Thank you for your feedback.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/akapps/groovy GROOVY-8288-execute-batch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/586.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #586
----
commit 7af055833f82606b12db5d66834ff1a4beaace2a
Author: Antoine Kapps <an...@orange.fr>
Date: 2017-08-15T07:42:26Z
GROOVY-8288 executeBatch() should not be called with batchCount == 0
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #586: GROOVY-8288 executeBatch() should not be called wi...
Posted by akapps <gi...@git.apache.org>.
Github user akapps commented on a diff in the pull request:
https://github.com/apache/groovy/pull/586#discussion_r133134481
--- Diff: subprojects/groovy-sql/src/main/java/groovy/sql/BatchingStatementWrapper.java ---
@@ -83,8 +83,15 @@ public void clearBatch() throws SQLException {
}
public int[] executeBatch() throws SQLException {
- int[] lastResult = delegate.executeBatch();
- processResult(lastResult);
+ if (batchCount > 0) {
+ int[] lastResult = delegate.executeBatch();
+ processResult(lastResult);
+ }
+ else if (results.isEmpty()) {
+ log.warning("Nothing has been added to batch");
+ // we let the JDBC provider decide how to handle an empty batch execution
+ return delegate.executeBatch();
+ }
--- End diff --
This "else if" is not really elegant for such a particular case...
I could have done `if (batchCount > 0 || results.isEmpty())` in the first condition, but I thought it made the intention unreadable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #586: GROOVY-8288 executeBatch() should not be called wi...
Posted by akapps <gi...@git.apache.org>.
Github user akapps commented on a diff in the pull request:
https://github.com/apache/groovy/pull/586#discussion_r133135254
--- Diff: subprojects/groovy-sql/src/test/groovy/groovy/sql/SqlBatchTest.groovy ---
@@ -119,6 +121,44 @@ class SqlBatchTest extends GroovyTestCase {
// FINE: Successfully executed batch with 1 command(s)
}
+ void testWithBatchHavingSizeSameSizeAsStatements() {
+ def numRows = sql.rows("SELECT * FROM PERSON").size()
+ assert numRows == 3
+ def myOthers = ['f4':'l4','f5':'l5','f6':'l6','f7':'l7']
+ def result = sql.withBatch(myOthers.size(), "insert into PERSON (id, firstname, lastname) values (?, ?, ?)") { ps ->
+ myOthers.eachWithIndex { k, v, index ->
+ def id = index + numRows + 1
+ ps.addBatch(id, k, v)
+ }
+ }
+ assert result == [1] * myOthers.size()
+ assert sql.rows("SELECT * FROM PERSON").size() == numRows + myOthers.size()
+ // end result the same as if no batching was in place but logging should show:
+ // FINE: Successfully executed batch with 4 command(s)
+ }
+
+ void testWithBatchNothingSpecified() {
+ def numRows = sql.rows("SELECT * FROM PERSON").size()
+ assert numRows == 3
+
+ def msg = GroovyAssert.shouldFail {
+ sql.withBatch(3, "insert into PERSON (id, firstname, lastname) values (?, ?, ?)") { ps ->
+ // Do nothing - not a good practice at all...
+ }
+ }
+ System.err.println(msg)
+ }
+
+ void testWithBatchNoPreparedStatementNothingSpecified() {
+ def numRows = sql.rows("SELECT * FROM PERSON").size()
+ assert numRows == 3
+
+ def result = sql.withBatch { ps ->
+ // Do nothing - not a good practice at all...
+ }
+ assert result == [] as int[]
--- End diff --
It is quite surprising at first sight that HSQL driver behaves differently if `executeBatch()` is called with nothing added to the batch, whether a preparedStatement is used or not.
I felt like it's not Groovy responsibility to "suppress" the exception for the preparedStatement case, and I wanted to test-specify those 2 behaviours as they are the current ones.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #586: GROOVY-8288 executeBatch() should not be called wi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/586
---