prepareStatementWithTrailingAddShouldFail is not enabled
Michael-A-McMahon opened this issue · 5 comments
This TCK test method is missing the @test annotation
I'd like to check about this before making a code change:
We it be totally incorrect for a driver to throw IllegalStateException from Statement.execute() in this test?
It seems reasonable to do so, as the execute() method is specified to throw IllegalStateException if all parameters have not been set.
But I'm not opposed to having Oracle R2DBC conform to the test either. I can code it to execute the statement with all previously added binds, emit a Result for each, and then emit onError with R2dbcTransientException. It just feels a little bit awkward, because Oracle R2DBC and JDBC already perform a client side check to ensure that all parameters are set; We don't rely on the database to perform that check.
So I'm just looking for a definitive specification of what the driver should do in this case. It seems like this TCK test and the JavaDoc are not consistent.
That test boils down to the driver/database interaction. After calling Statement.add(), a new binding set should be prepared. That would lead to running the statement twice, once with the binding applied and once without bound values. The first statement would succeed and the second execution would fail. What we want to verify is that the statement isn't run statement only once and we want to verify that it does not run twice with the same parameters.
I initially added the test case without @Test to not break 0.8.x drivers as the refinement happened in a maintenance release. It makes sense to enable the test case for 0.9.
Likely, where the error signal originates (in the driver or in the database) doesn't make much of a difference, so changing to verifyError() would make sense.
Thanks, I understand it better now: An error that occurs from one set of binds should not effect the statement's execution of previously add()'d binds.
Have we considered what should happen when an error occurs before the last set of binds in the batch?
Let's say I execute a statement like this:
statement.bind(0, 0).add()
.bind(0, 1).add()
.bind(0, 2).execute();What happens if the execution of bind(0, 1) results in an error from the database? Should the bind(0, 2) execution still occur, or should all executions following an error also result in an error? This question applies to Statement.add() and to the io.r2dbc.spi.Batch type as well.
Have we considered what should happen when an error occurs before the last set of binds in the batch?
Input validation may happen in the driver, the database or both. I think upon early validation failures in the driver, it makes sense to not run anything and fail early.
If the execution fails in the database
execution of bind(0, 1) results in an error from the database
then conceptually this is the same as canceling the subscription. The publisher should stop producing items at the earliest convenience. Whether the subsequent query gets ran depends on the driver and database implementation. What could potentially happen is a database with a protocol where all bindings are sent at once and then the driver has no longer control over what happens within the database.
In Postgres and SQL Server, drivers are iterating over the bindings and if one statement encounters an error signal or sees a cancel during the execution the driver stops iterating over the bindings and in consequence, the next binding isn't sent to the database.
This leads to maybe consistency. The only thing that makes sense to ensure consistency is running this within a transaction that is being rolled back.
In any case, I'm going to enable the TCK test case for 0.9.