Repository: GoogleCloudPlatform/pgadapter (java)

Issue:
Errors are not propagated correctly in a DDL batch
A DDL batch that contains the following:
1. The first statement is executed correctly.
2. The second statement is executed, but contains an error.

The error from the second statement is not propagated correctly to the client by PGAdapter.

Pull request description:
fix: DDL batch errors halfway were not propagated
A DDL batch that executed at least one statement successfully and then failed for a later statement would be reported as successful. This was caused by an invalid extraction of the returned update counts from the SpannerBatchUpdateException. The exception contains a full list of update counts for all statements in the batch. The value is 1 for a statement that was (successfully) executed and 0 for all other statements. The extraction of the update counts assumed that the list would only contain an entry for the statements that were actually executed.

Fixes #443

Patch:
diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java b/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java
index 6bac0860..69bdc609 100644
--- a/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java
+++ b/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java
@@ -813,7 +813,8 @@ public class BackendConnection {
    * @param fromIndex The index of the statements array where the batch should start
    * @return The number of statements included in the batch.
    */
-  private int executeStatementsInBatch(int fromIndex) {
+  @VisibleForTesting
+  int executeStatementsInBatch(int fromIndex) {
     Preconditions.checkArgument(fromIndex < getStatementCount() - 1);
     Preconditions.checkArgument(
         canBeBatchedTogether(getStatementType(fromIndex), getStatementType(fromIndex + 1)));
@@ -837,8 +838,7 @@ public class BackendConnection {
         if (canBeBatchedTogether(batchType, statementType)) {
           // Send DDL statements to the DdlExecutor instead of executing them directly on the
           // connection, so we can support certain DDL constructs that are currently not supported
-          // by
-          // the backend, such as IF [NOT] EXISTS.
+          // by the backend, such as IF [NOT] EXISTS.
           if (batchType == StatementType.DDL) {
             statementResults.add(
                 ddlExecutor.execute(
@@ -850,8 +850,7 @@ public class BackendConnection {
           index++;
         } else {
           // End the batch here, as the statement type on this index can not be batched together
-          // with
-          // the other statements in the batch.
+          // with the other statements in the batch.
           break;
         }
       }
@@ -894,6 +893,7 @@ public class BackendConnection {
     int index = 0;
     int successfullyExecutedCount = 0;
     while (index < returnedUpdateCounts.length
+        && returnedUpdateCounts[index] == 1
         && successfullyExecutedCount < statementResults.size()) {
       if (!(statementResults.get(successfullyExecutedCount) instanceof NotExecuted)) {
         index++;


Test patch:
diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/statements/BackendConnectionTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/statements/BackendConnectionTest.java
index 651c153b..0ec73a20 100644
--- a/src/test/java/com/google/cloud/spanner/pgadapter/statements/BackendConnectionTest.java
+++ b/src/test/java/com/google/cloud/spanner/pgadapter/statements/BackendConnectionTest.java
@@ -33,6 +33,7 @@ import com.google.cloud.spanner.DatabaseId;
 import com.google.cloud.spanner.Dialect;
 import com.google.cloud.spanner.ErrorCode;
 import com.google.cloud.spanner.ResultSet;
+import com.google.cloud.spanner.SpannerBatchUpdateException;
 import com.google.cloud.spanner.SpannerException;
 import com.google.cloud.spanner.SpannerExceptionFactory;
 import com.google.cloud.spanner.Statement;
@@ -119,6 +120,34 @@ public class BackendConnectionTest {
             ImmutableList.of(NOT_EXECUTED, NOT_EXECUTED, NOT_EXECUTED), new long[] {}));
   }
 
+  @Test
+  public void testExecuteStatementsInBatch() {
+    Connection spannerConnection = mock(Connection.class);
+    SpannerBatchUpdateException expectedException = mock(SpannerBatchUpdateException.class);
+    when(expectedException.getMessage()).thenReturn("Invalid statement");
+    when(expectedException.getErrorCode()).thenReturn(ErrorCode.INVALID_ARGUMENT);
+    when(expectedException.getUpdateCounts()).thenReturn(new long[] {1L, 0L});
+    when(spannerConnection.runBatch()).thenThrow(expectedException);
+    BackendConnection backendConnection =
+        new BackendConnection(
+            DatabaseId.of("p", "i", "d"),
+            spannerConnection,
+            mock(OptionsMetadata.class),
+            ImmutableList.of());
+
+    backendConnection.execute(
+        PARSER.parse(Statement.of("CREATE TABLE \"Foo\" (id bigint primary key)")),
+        Statement.of("CREATE TABLE \"Foo\" (id bigint primary key)"));
+    backendConnection.execute(
+        PARSER.parse(Statement.of("CREATE TABLE bar (id bigint primary key, value text)")),
+        Statement.of("CREATE TABLE bar (id bigint primary key, value text)"));
+
+    SpannerBatchUpdateException batchUpdateException =
+        assertThrows(
+            SpannerBatchUpdateException.class, () -> backendConnection.executeStatementsInBatch(0));
+    assertSame(expectedException, batchUpdateException);
+  }
+
   @Test
   public void testCopyPropagatesNonSpannerException() {
     String sql = "COPY users FROM STDIN";