Repository: GoogleCloudPlatform/pgadapter
Pull request #397 (2022-10-14 12:16:29)

test: add batch tests for node-postgres
Add batch tests for node-postgres.

codecov[bot]: # [Codecov](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/397?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) Report
> Merging [#397](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/397?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) (68b2e70) into [postgresql-dialect](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/commit/bec1657a43500c1f61126c744ad5b95cdce4c2e7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) (bec1657) will **decrease** coverage by `0.01%`.
> The diff coverage is `80.00%`.

> :exclamation: Current head 68b2e70 differs from pull request most recent head ff01c8b. Consider uploading reports for the commit ff01c8b to get more accurate results

```diff
@@                   Coverage Diff                    @@
##             postgresql-dialect     #397      +/-   ##
========================================================
- Coverage                 86.31%   86.29%   -0.02%     
- Complexity                 1906     1907       +1     
========================================================
  Files                       122      122              
  Lines                      6182     6181       -1     
  Branches                    845      844       -1     
========================================================
- Hits                       5336     5334       -2     
+ Misses                      605      604       -1     
- Partials                    241      243       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| all_tests | `86.29% <80.00%> (-0.02%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) | Coverage Δ | |
|---|---|---|
| [.../spanner/pgadapter/statements/CopyToStatement.java](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/397/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvcGdhZGFwdGVyL3N0YXRlbWVudHMvQ29weVRvU3RhdGVtZW50LmphdmE=) | `95.16% <66.66%> (-0.50%)` | :arrow_down: |
| [...spanner/pgadapter/wireoutput/CopyDataResponse.java](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/397/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvcGdhZGFwdGVyL3dpcmVvdXRwdXQvQ29weURhdGFSZXNwb25zZS5qYXZh) | `89.58% <81.81%> (-4.04%)` | :arrow_down: |
| [...oogle/cloud/spanner/pgadapter/utils/Converter.java](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/397/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvcGdhZGFwdGVyL3V0aWxzL0NvbnZlcnRlci5qYXZh) | `90.69% <83.33%> (-1.41%)` | :arrow_down: |
| [...om/google/cloud/spanner/pgadapter/ProxyServer.java](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/397/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvcGdhZGFwdGVyL1Byb3h5U2VydmVyLmphdmE=) | `80.95% <0.00%> (+1.19%)` | :arrow_up: |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform)


Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index c9800fad..ab1ccad2 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -41,11 +41,10 @@ jobs:
       with:
         python-version: '3.9'
     - run: python --version
-    - uses: actions/checkout@v3
-    - uses: actions/setup-python@v4
-      with:
-        python-version: '3.9'
     - run: pip install -r ./src/test/python/requirements.txt
+    - uses: actions/setup-node@v3
+      with:
+        node-version: 16
     - run: .ci/run-with-credentials.sh units
 
   lint:
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 90d7c331..6bac0860 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
@@ -235,7 +235,7 @@ public class BackendConnection {
           result.setException(spannerException);
         }
         throw spannerException;
-      } catch (Exception exception) {
+      } catch (Throwable exception) {
         result.setException(exception);
         throw exception;
       }
@@ -612,6 +612,10 @@ public class BackendConnection {
     if (isSync && hasOnlyDmlStatementsAfter(index)) {
       return;
     }
+    // Do not start a transaction if a batch is already active.
+    if (spannerConnection.isDdlBatchActive() || spannerConnection.isDmlBatchActive()) {
+      return;
+    }
 
     // We need to start an implicit transaction.
     // Check if a read-only transaction suffices.


Test patch:
diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/ITNodePostgresTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/ITNodePostgresTest.java
index 21e46603..20ef0172 100644
--- a/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/ITNodePostgresTest.java
+++ b/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/ITNodePostgresTest.java
@@ -407,6 +407,22 @@ public class ITNodePostgresTest implements IntegrationTest {
     }
   }
 
+  @Test
+  public void testDmlBatch() throws Exception {
+    String output = runTest("testDmlBatch", getHost(), testEnv.getServer().getLocalPort());
+    assertEquals("\n\nexecuted dml batch\n", output);
+
+    DatabaseClient client = testEnv.getSpanner().getDatabaseClient(database.getId());
+    try (ResultSet resultSet =
+        client.singleUse().executeQuery(Statement.of("SELECT * FROM users ORDER BY name"))) {
+      assertTrue(resultSet.next());
+      assertEquals("bar", resultSet.getString("name"));
+      assertTrue(resultSet.next());
+      assertEquals("foo", resultSet.getString("name"));
+      assertFalse(resultSet.next());
+    }
+  }
+
   static String runTest(String testName, String host, int port)
       throws IOException, InterruptedException {
     return NodeJSTest.runTest(
diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/NodePostgresMockServerTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/NodePostgresMockServerTest.java
index d534bb28..9c09c710 100644
--- a/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/NodePostgresMockServerTest.java
+++ b/src/test/java/com/google/cloud/spanner/pgadapter/nodejs/NodePostgresMockServerTest.java
@@ -31,8 +31,10 @@ import com.google.cloud.spanner.pgadapter.wireprotocol.ControlMessage.PreparedTy
 import com.google.cloud.spanner.pgadapter.wireprotocol.DescribeMessage;
 import com.google.cloud.spanner.pgadapter.wireprotocol.ParseMessage;
 import com.google.common.collect.ImmutableList;
+import com.google.spanner.admin.database.v1.UpdateDatabaseDdlRequest;
 import com.google.spanner.v1.BeginTransactionRequest;
 import com.google.spanner.v1.CommitRequest;
+import com.google.spanner.v1.ExecuteBatchDmlRequest;
 import com.google.spanner.v1.ExecuteSqlRequest;
 import com.google.spanner.v1.RollbackRequest;
 import com.google.spanner.v1.TypeCode;
@@ -598,6 +600,72 @@ public class NodePostgresMockServerTest extends AbstractMockServerTest {
     assertEquals("\n\nFinished copy operation\n", output);
   }
 
+  @Test
+  public void testDmlBatch() throws Exception {
+    String sql = "INSERT INTO users(name) VALUES($1)";
+    String describeParamsSql = "select $1 from (select name=$1 from users) p";
+    mockSpanner.putStatementResult(
+        StatementResult.query(
+            Statement.of(describeParamsSql),
+            com.google.spanner.v1.ResultSet.newBuilder()
+                .setMetadata(createMetadata(ImmutableList.of(TypeCode.STRING)))
+                .build()));
+    mockSpanner.putStatementResult(
+        StatementResult.update(Statement.newBuilder(sql).bind("p1").to("foo").build(), 1L));
+    mockSpanner.putStatementResult(
+        StatementResult.update(Statement.newBuilder(sql).bind("p1").to("bar").build(), 1L));
+
+    String output = runTest("testDmlBatch", getHost(), pgServer.getLocalPort());
+
+    assertEquals("\n\nexecuted dml batch\n", output);
+
+    List<ExecuteSqlRequest> executeSqlRequests =
+        mockSpanner.getRequestsOfType(ExecuteSqlRequest.class).stream()
+            .filter(
+                request ->
+                    request.getSql().equals(sql) || request.getSql().equals(describeParamsSql))
+            .collect(Collectors.toList());
+    assertEquals(1, executeSqlRequests.size());
+    ExecuteSqlRequest describeRequest = executeSqlRequests.get(0);
+    assertEquals(describeParamsSql, describeRequest.getSql());
+    assertFalse(describeRequest.hasTransaction());
+
+    List<ExecuteBatchDmlRequest> batchDmlRequests =
+        mockSpanner.getRequestsOfType(ExecuteBatchDmlRequest.class);
+    assertEquals(1, batchDmlRequests.size());
+    ExecuteBatchDmlRequest request = batchDmlRequests.get(0);
+    assertTrue(request.getTransaction().hasBegin());
+    assertTrue(request.getTransaction().getBegin().hasReadWrite());
+    assertEquals(2, request.getStatementsCount());
+    String[] expectedValues = new String[] {"foo", "bar"};
+    for (int i = 0; i < request.getStatementsCount(); i++) {
+      assertEquals(sql, request.getStatements(i).getSql());
+      assertEquals(1, request.getStatements(i).getParamTypesCount());
+      assertEquals(
+          expectedValues[i],
+          request.getStatements(i).getParams().getFieldsMap().get("p1").getStringValue());
+    }
+    assertEquals(1, mockSpanner.countRequestsOfType(CommitRequest.class));
+  }
+
+  @Test
+  public void testDdlBatch() throws Exception {
+    addDdlResponseToSpannerAdmin();
+
+    String output = runTest("testDdlBatch", getHost(), pgServer.getLocalPort());
+
+    assertEquals("\n\nexecuted ddl batch\n", output);
+    assertEquals(1, mockDatabaseAdmin.getRequests().size());
+    assertEquals(UpdateDatabaseDdlRequest.class, mockDatabaseAdmin.getRequests().get(0).getClass());
+    UpdateDatabaseDdlRequest request =
+        (UpdateDatabaseDdlRequest) mockDatabaseAdmin.getRequests().get(0);
+    assertEquals(2, request.getStatementsCount());
+    assertEquals(
+        "create table my_table1 (id bigint primary key, value varchar)", request.getStatements(0));
+    assertEquals(
+        "create table my_table2 (id bigint primary key, value varchar)", request.getStatements(1));
+  }
+
   static String runTest(String testName, String host, int port)
       throws IOException, InterruptedException {
     return NodeJSTest.runTest("node-postgres", testName, host, port, "db");
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 fee0f46d..651c153b 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
@@ -484,4 +484,26 @@ public class BackendConnectionTest {
 
     verify(connection).execute(statement);
   }
+
+  @Test
+  public void testDoNotStartTransactionInBatch() {
+    Connection connection = mock(Connection.class);
+    when(connection.isDmlBatchActive()).thenReturn(true);
+    Statement statement = Statement.of("insert into foo values (1)");
+    ParsedStatement parsedStatement =
+        AbstractStatementParser.getInstance(Dialect.POSTGRESQL).parse(statement);
+
+    BackendConnection backendConnection =
+        new BackendConnection(
+            DatabaseId.of("p", "i", "d"),
+            connection,
+            mock(OptionsMetadata.class),
+            EMPTY_LOCAL_STATEMENTS);
+
+    backendConnection.execute(parsedStatement, statement);
+    backendConnection.flush();
+
+    verify(connection).execute(statement);
+    verify(connection, never()).beginTransaction();
+  }
 }
diff --git a/src/test/nodejs/node-postgres/src/index.ts b/src/test/nodejs/node-postgres/src/index.ts
index bd930bbf..cc756310 100644
--- a/src/test/nodejs/node-postgres/src/index.ts
+++ b/src/test/nodejs/node-postgres/src/index.ts
@@ -261,6 +261,30 @@ async function testCopyFrom(client) {
   }
 }
 
+async function testDmlBatch(client) {
+  try {
+    const queryText = 'INSERT INTO users(name) VALUES($1)';
+    await client.query('start batch dml');
+    await client.query(queryText, ['foo']);
+    await client.query(queryText, ['bar']);
+    await client.query('run batch');
+    console.log('executed dml batch');
+  } catch (e) {
+    console.error(`Dml batch error: ${e}`);
+  }
+}
+
+async function testDdlBatch(client) {
+  try {
+    await client.query('start batch ddl');
+    await client.query('create table my_table1 (id bigint primary key, value varchar)');
+    await client.query('create table my_table2 (id bigint primary key, value varchar)');
+    await client.query('run batch');
+    console.log('executed ddl batch');
+  } catch (e) {
+    console.error(`Dml batch error: ${e}`);
+  }
+}
 
 require('yargs')
 .demand(4)
@@ -348,6 +372,18 @@ require('yargs')
     {},
     opts => runTest(opts.host, opts.port, opts.database, testCopyFrom)
 )
+.command(
+    'testDmlBatch <host> <port> <database>',
+    'Tests DML batch',
+    {},
+    opts => runTest(opts.host, opts.port, opts.database, testDmlBatch)
+)
+.command(
+    'testDdlBatch <host> <port> <database>',
+    'Tests DDL batch',
+    {},
+    opts => runTest(opts.host, opts.port, opts.database, testDdlBatch)
+)
 .wrap(120)
 .recommendCommands()
 .strict()