Repository: GoogleCloudPlatform/pgadapter
Pull request #279 (2022-07-12 13:55:31)

fix: prepared statement could return error 'This ResultSet is closed'
Executing a query using a server side prepared statement in a read/write transaction
would return the error "This ResultSet is closed". This happened because the result
set that was used to describe the statement would be closed before the actual result
was sent.

Fixes #278

codecov[bot]: # [Codecov](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) Report
> Merging [#279](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) (01f1563) into [postgresql-dialect](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/commit/fe56a537933417446540bc434505fdafdfce53ac?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) (fe56a53) will **increase** coverage by `0.00%`.
> The diff coverage is `96.29%`.

```diff
@@                  Coverage Diff                  @@
##             postgresql-dialect     #279   +/-   ##
=====================================================
  Coverage                 82.29%   82.29%           
- Complexity                 1277     1278    +1     
=====================================================
  Files                       102      102           
  Lines                      4410     4411    +1     
  Branches                    548      546    -2     
=====================================================
+ Hits                       3629     3630    +1     
  Misses                      598      598           
  Partials                    183      183           
```

| Flag | Coverage Δ | |
|---|---|---|
| all_tests | `82.29% <96.29%> (+<0.01%)` | :arrow_up: |

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/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform) | Coverage Δ | |
|---|---|---|
| [...pter/statements/IntermediatePreparedStatement.java](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvcGdhZGFwdGVyL3N0YXRlbWVudHMvSW50ZXJtZWRpYXRlUHJlcGFyZWRTdGF0ZW1lbnQuamF2YQ==) | `84.26% <95.45%> (-0.27%)` | :arrow_down: |
| [.../pgadapter/metadata/DescribeStatementMetadata.java](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvcGdhZGFwdGVyL21ldGFkYXRhL0Rlc2NyaWJlU3RhdGVtZW50TWV0YWRhdGEuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...panner/pgadapter/wireprotocol/DescribeMessage.java](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvcGdhZGFwdGVyL3dpcmVwcm90b2NvbC9EZXNjcmliZU1lc3NhZ2UuamF2YQ==) | `84.37% <100.00%> (+0.24%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform). Last update [fe56a53...01f1563](https://codecov.io/gh/GoogleCloudPlatform/pgadapter/pull/279?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleCloudPlatform).


Patch:
diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/metadata/DescribeStatementMetadata.java b/src/main/java/com/google/cloud/spanner/pgadapter/metadata/DescribeStatementMetadata.java
index 67ba5d0f..badb818f 100644
--- a/src/main/java/com/google/cloud/spanner/pgadapter/metadata/DescribeStatementMetadata.java
+++ b/src/main/java/com/google/cloud/spanner/pgadapter/metadata/DescribeStatementMetadata.java
@@ -20,7 +20,8 @@ import com.google.cloud.spanner.ResultSet;
 
 /** Simple POJO to hold describe metadata specific to prepared statements. */
 @InternalApi
-public class DescribeStatementMetadata extends DescribeMetadata<Tuple<int[], ResultSet>> {
+public class DescribeStatementMetadata extends DescribeMetadata<Tuple<int[], ResultSet>>
+    implements AutoCloseable {
 
   public DescribeStatementMetadata(int[] parameters, ResultSet resultMetaData) {
     this.metadata = Tuple.of(parameters, resultMetaData);
@@ -33,4 +34,11 @@ public class DescribeStatementMetadata extends DescribeMetadata<Tuple<int[], Res
   public ResultSet getResultSet() {
     return metadata.y();
   }
+
+  @Override
+  public void close() {
+    if (getResultSet() != null) {
+      getResultSet().close();
+    }
+  }
 }
diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePreparedStatement.java b/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePreparedStatement.java
index 138d1b48..6d92bb6b 100644
--- a/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePreparedStatement.java
+++ b/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePreparedStatement.java
@@ -136,60 +136,53 @@ public class IntermediatePreparedStatement extends IntermediateStatement {
     Set<String> parameters = extractParameters(this.parsedStatement.getSqlWithoutComments());
 
     ResultSet columnsResultSet = null;
-    try {
-      if (this.parsedStatement.isQuery()) {
-        Statement statement = Statement.of(this.parsedStatement.getSqlWithoutComments());
-        columnsResultSet = connection.analyzeQuery(statement, QueryAnalyzeMode.PLAN);
-      }
+    if (this.parsedStatement.isQuery()) {
+      Statement statement = Statement.of(this.parsedStatement.getSqlWithoutComments());
+      columnsResultSet = connection.analyzeQuery(statement, QueryAnalyzeMode.PLAN);
+    }
 
-      boolean describeFailed = false;
-      if (parameters.isEmpty()) {
-        ensureParameterLength(0);
-      } else if (parameters.size() != this.parameterDataTypes.length
-          || Arrays.stream(this.parameterDataTypes).anyMatch(p -> p == 0)) {
-        // Note: We are only asking the backend to parse the types if there is at least one
-        // parameter with unspecified type. Otherwise, we will rely on the types given in PARSE.
-
-        // Transform the statement into a select statement that selects the parameters, and then
-        // extract the types from the result set metadata.
-        Statement selectParamsStatement = transformToSelectParams(parameters);
-        if (selectParamsStatement == null) {
-          // The transformation failed. Just rely on the types given in the PARSE message. If the
-          // transformation failed because the statement was malformed, the backend will catch that
-          // at a later stage.
+    boolean describeFailed = false;
+    if (parameters.isEmpty()) {
+      ensureParameterLength(0);
+    } else if (parameters.size() != this.parameterDataTypes.length
+        || Arrays.stream(this.parameterDataTypes).anyMatch(p -> p == 0)) {
+      // Note: We are only asking the backend to parse the types if there is at least one
+      // parameter with unspecified type. Otherwise, we will rely on the types given in PARSE.
+
+      // Transform the statement into a select statement that selects the parameters, and then
+      // extract the types from the result set metadata.
+      Statement selectParamsStatement = transformToSelectParams(parameters);
+      if (selectParamsStatement == null) {
+        // The transformation failed. Just rely on the types given in the PARSE message. If the
+        // transformation failed because the statement was malformed, the backend will catch that
+        // at a later stage.
+        describeFailed = true;
+        ensureParameterLength(parameters.size());
+      } else {
+        try (ResultSet paramsResultSet =
+            connection.analyzeQuery(selectParamsStatement, QueryAnalyzeMode.PLAN)) {
+          extractParameterTypes(paramsResultSet);
+        } catch (SpannerException exception) {
+          // Ignore here and rely on the types given in PARSE.
           describeFailed = true;
           ensureParameterLength(parameters.size());
-        } else {
-          try (ResultSet paramsResultSet =
-              connection.analyzeQuery(selectParamsStatement, QueryAnalyzeMode.PLAN)) {
-            extractParameterTypes(paramsResultSet);
-          } catch (SpannerException exception) {
-            // Ignore here and rely on the types given in PARSE.
-            describeFailed = true;
-            ensureParameterLength(parameters.size());
-          }
         }
       }
-      if (columnsResultSet != null) {
-        return new DescribeStatementMetadata(this.parameterDataTypes, columnsResultSet);
-      }
+    }
+    if (columnsResultSet != null) {
+      return new DescribeStatementMetadata(this.parameterDataTypes, columnsResultSet);
+    }
 
-      if (this.parsedStatement.isUpdate()
-          && (describeFailed || !Strings.isNullOrEmpty(this.name))) {
-        // Let the backend analyze the statement if it is a named prepared statement or if the query
-        // that was used to determine the parameter types failed, so we can return a reasonable
-        // error message if the statement is invalid. If it is the unnamed statement or getting the
-        // param types succeeded, we will let the following EXECUTE message handle that, instead of
-        // sending the statement twice to the backend.
-        connection.analyzeUpdate(
-            Statement.of(this.parsedStatement.getSqlWithoutComments()), QueryAnalyzeMode.PLAN);
-      }
-      return new DescribeStatementMetadata(this.parameterDataTypes, null);
-    } finally {
-      if (columnsResultSet != null) {
-        columnsResultSet.close();
-      }
+    if (this.parsedStatement.isUpdate() && (describeFailed || !Strings.isNullOrEmpty(this.name))) {
+      // Let the backend analyze the statement if it is a named prepared statement or if the query
+      // that was used to determine the parameter types failed, so we can return a reasonable
+      // error message if the statement is invalid. If it is the unnamed statement or getting the
+      // param types succeeded, we will let the following EXECUTE message handle that, instead of
+      // sending the statement twice to the backend.
+      connection.analyzeUpdate(
+          Statement.of(this.parsedStatement.getSqlWithoutComments()), QueryAnalyzeMode.PLAN);
     }
+    return new DescribeStatementMetadata(this.parameterDataTypes, null);
   }
 
   /**
diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/DescribeMessage.java b/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/DescribeMessage.java
index 08e3b85f..08c15da2 100644
--- a/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/DescribeMessage.java
+++ b/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/DescribeMessage.java
@@ -166,8 +166,8 @@ public class DescribeMessage extends AbstractQueryProtocolMessage {
    * @throws Exception if sending the message back to the client causes an error.
    */
   public void handleDescribeStatement() throws Exception {
-    try {
-      DescribeStatementMetadata metadata = (DescribeStatementMetadata) this.statement.describe();
+    try (DescribeStatementMetadata metadata =
+        (DescribeStatementMetadata) this.statement.describe()) {
       new ParameterDescriptionResponse(this.outputStream, metadata.getParameters()).send(false);
       if (metadata.getResultSet() != null) {
         new RowDescriptionResponse(


Test patch:
diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcMockServerTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcMockServerTest.java
index 3e84c0c8..f268235f 100644
--- a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcMockServerTest.java
+++ b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcMockServerTest.java
@@ -65,6 +65,7 @@ import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import org.postgresql.PGConnection;
 import org.postgresql.PGStatement;
 import org.postgresql.jdbc.PgStatement;
 
@@ -276,6 +277,27 @@ public class JdbcMockServerTest extends AbstractMockServerTest {
     }
   }
 
+  @Test
+  public void testMultipleQueriesInTransaction() throws SQLException {
+    String sql = "SELECT 1";
+
+    try (Connection connection = DriverManager.getConnection(createUrl())) {
+      // Use a read/write transaction to execute two queries.
+      connection.setAutoCommit(false);
+      // Force the use of prepared statements.
+      connection.unwrap(PGConnection.class).setPrepareThreshold(-1);
+      for (int i = 0; i < 2; i++) {
+        // https://github.com/GoogleCloudPlatform/pgadapter/issues/278
+        // This would return `ERROR: FAILED_PRECONDITION: This ResultSet is closed`
+        try (ResultSet resultSet = connection.createStatement().executeQuery(sql)) {
+          assertTrue(resultSet.next());
+          assertEquals(1L, resultSet.getLong(1));
+          assertFalse(resultSet.next());
+        }
+      }
+    }
+  }
+
   @Test
   public void testQueryWithNonExistingTable() throws SQLException {
     String sql = "select * from non_existing_table where id=$1";