Fix NPE issue when batch execution is called without any queries

[T45K]

Motivation: (Please describe the problem you are trying to solve, or the new feature you are trying to add.)

we're using this R2DBC client with jOOQ. NPE occurs when we give empty list to DSLContext#batch

val queries = emptyList()
dslContext.batch(queries).awaitLast()

this is because StringBuilder in MySqlBatchingBatch is not initialized until add method is called. so, execute method will refer to not-initialized builder when add method is not called.

Modification: (Please describe the changes you have made to the codebase, including any new files, modified files, or deleted files.)

modify execute to return Flux#empty when builder is not initialized (i.e., add method is not called). send empty string to client without NPE

Result: (Please describe the expected outcome of your changes, and any potential side effects or drawbacks. If possible, please also include any relevant testing results.)

it now does not throw NPE but returns empty flux instead.

[mirromutth]

The io.r2dbc.spi.Batch spec shows a specification like following:

Batch objects are run by calling the execute() method after adding one or more SQL statements to a Batch.

So we cannot return an empty Flux. Here are some suggestions:

• Execute an empty query. Then server may return an error (e.g. MySQL and MariaDB), or do nothing (e.g. TiDB)
• Emit an exception that should not be a NPE, and it cannot be thrown directly.
  • In r2dbc-mariadb MariadbBatch.java#L92, it will emit a NoSuchElementException due to iterator.next() without checking
  • Note: the empty checking should be deferred.

However, it seems like a code smell coming from MySqlBatchingBatch.builder, could you please add a @Nullable to the builder property? Then, we can see a null-check warning at MySqlBatchingBatch.getSql(), so we can add a null-check to the getSql() instead of execute(), which should make sense.

[T45K]

thank you for confirming my PR!

The io.r2dbc.spi.Batch spec shows a specification like following:

Batch objects are run by calling the execute() method after adding one or more SQL statements to a Batch.

sorry, i didn't know the spi spec :pray:

Execute an empty query. Then server may return an error (e.g. MySQL and MariaDB), or do nothing (e.g. TiDB)

i think this is more reasonable. i'd like to rewrite in this way.

---

(question)

i think we can make builder not-null by initializing it when MySqlBatchingBatch is constructed.

-     private StringBuilder builder;
+     private final StringBuilder builder = new StringBuilder();

and then, requireBuilder can be rewritten like

    private StringBuilder requireBuilder() {
-      if (builder == null) {
-          return (builder = new StringBuilder());
-      }

+        if (builder.length() == 0) {
+            return builder;
+        }

        return builder.append(';');
    }

this looks better for me. what do you think?

[mirromutth]

i think we can make builder not-null by initializing it when MySqlBatchingBatch is constructed.

Hmmm, if we check only it is empty or not, consider following code:

batch1.add(";").add("SELECT 1").execute()
batch2.add("").add("SELECT 1").execute()
batch3.add("SELECT 1").execute()

There Batchs will execute same query SELECT 1, but first and second Batchs should be ;SELECT 1, third should be SELECT 1.

Maybe it would be simpler to refactor the entire builder into a List. Note that statement joins should be deferred.

[T45K]

i see. i'll update this PR later