Repository: googleapis/java-spanner-jdbc
Pull request #1323 (2023-08-15 09:39:31)

fix: session leak for invalid update
Close the underlying ResultSet if the result that was returned for an
executeUpdate call turned out to be of an unexpected type.


Patch:
diff --git a/.github/sync-repo-settings.yaml b/.github/sync-repo-settings.yaml
index 4879f02..a259dc0 100644
--- a/.github/sync-repo-settings.yaml
+++ b/.github/sync-repo-settings.yaml
@@ -39,6 +39,7 @@ branchProtectionRules:
     - OwlBot Post Processor
     - 'Kokoro - Test: Java GraalVM Native Image'
     - 'Kokoro - Test: Java 17 GraalVM Native Image'
+    - javadoc
 
 # Identifies the protection rule pattern. Name of the branch to be protected.
 # Defaults to `main`
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a1f93aa..3992709 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,12 @@
 # Changelog
 
+## [2.11.9](https://github.com/googleapis/java-spanner-jdbc/compare/v2.11.8...v2.11.9) (2023-08-21)
+
+
+### Dependencies
+
+* Update dependency com.google.cloud:google-cloud-spanner-bom to v6.45.3 ([#1325](https://github.com/googleapis/java-spanner-jdbc/issues/1325)) ([d96d278](https://github.com/googleapis/java-spanner-jdbc/commit/d96d278f58942c38b30dc0a4e1f9f92aefdc760b))
+
 ## [2.11.8](https://github.com/googleapis/java-spanner-jdbc/compare/v2.11.7...v2.11.8) (2023-08-15)
 
 
diff --git a/pom.xml b/pom.xml
index c46a840..4a33dea 100644
--- a/pom.xml
+++ b/pom.xml
@@ -4,7 +4,7 @@
   xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
   <modelVersion>4.0.0</modelVersion>
   <artifactId>google-cloud-spanner-jdbc</artifactId>
-  <version>2.11.9-SNAPSHOT</version><!-- {x-version-update:google-cloud-spanner-jdbc:current} -->
+  <version>2.11.10-SNAPSHOT</version><!-- {x-version-update:google-cloud-spanner-jdbc:current} -->
   <packaging>jar</packaging>
   <name>Google Cloud Spanner JDBC</name>
   <url>https://github.com/googleapis/java-spanner-jdbc</url>
@@ -62,7 +62,7 @@
       <dependency>
         <groupId>com.google.cloud</groupId>
         <artifactId>google-cloud-spanner-bom</artifactId>
-        <version>6.45.2</version>
+        <version>6.45.3</version>
         <type>pom</type>
         <scope>import</scope>
       </dependency>
diff --git a/samples/install-without-bom/pom.xml b/samples/install-without-bom/pom.xml
index 1922636..be63235 100644
--- a/samples/install-without-bom/pom.xml
+++ b/samples/install-without-bom/pom.xml
@@ -29,7 +29,7 @@
     <dependency>
       <groupId>com.google.cloud</groupId>
       <artifactId>google-cloud-spanner-jdbc</artifactId>
-      <version>2.11.7</version>
+      <version>2.11.9</version>
     </dependency>
     <!-- [END spanner-jdbc_install_without_bom] -->
 
diff --git a/samples/snapshot/pom.xml b/samples/snapshot/pom.xml
index 4abc54e..5cc1db0 100644
--- a/samples/snapshot/pom.xml
+++ b/samples/snapshot/pom.xml
@@ -28,7 +28,7 @@
     <dependency>
       <groupId>com.google.cloud</groupId>
       <artifactId>google-cloud-spanner-jdbc</artifactId>
-      <version>2.11.9-SNAPSHOT</version>
+      <version>2.11.10-SNAPSHOT</version>
     </dependency>
     <!-- {x-version-update-end} -->
 
diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcResultSet.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcResultSet.java
index f8ab452..94b4604 100644
--- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcResultSet.java
+++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcResultSet.java
@@ -16,12 +16,13 @@
 
 package com.google.cloud.spanner.jdbc;
 
-import static com.google.cloud.spanner.Type.Code.PG_NUMERIC;
-
+import com.google.cloud.spanner.ResultSets;
+import com.google.cloud.spanner.Struct;
 import com.google.cloud.spanner.Type;
 import com.google.cloud.spanner.Type.Code;
 import com.google.cloud.spanner.Value;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.io.Reader;
@@ -43,8 +44,11 @@ import java.sql.Statement;
 import java.sql.Time;
 import java.sql.Timestamp;
 import java.util.Calendar;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
+import javax.annotation.Nonnull;
 
 /** Implementation of {@link ResultSet} for Cloud Spanner */
 class JdbcResultSet extends AbstractJdbcResultSet {
@@ -60,6 +64,57 @@ class JdbcResultSet extends AbstractJdbcResultSet {
     return new JdbcResultSet(statement, resultSet);
   }
 
+  /**
+   * Creates a JDBC result set by copying the given Spanner {@link
+   * com.google.cloud.spanner.ResultSet}. This can be used for result sets that are known not to be
+   * too large. This type of result set should be preferred for results that are unlikely to be
+   * closed by the client application, such as the returned generated keys of an update statement.
+   * The copy will not hold on to a reference to a Cloud Spanner session or result stream. All the
+   * data in the given Spanner {@link com.google.cloud.spanner.ResultSet} have been consumed after
+   * calling this method. The {@link com.google.cloud.spanner.ResultSet} is not closed by this
+   * method.
+   */
+  static JdbcResultSet copyOf(@Nonnull com.google.cloud.spanner.ResultSet resultSet) {
+    Preconditions.checkNotNull(resultSet);
+    // Make the copy first. This ensures that ResultSet#next() has been called at least once, which
+    // is necessary to get the type of the result set.
+    ImmutableList<Struct> rows = ImmutableList.copyOf(new ResultSetIterator(resultSet));
+    return of(ResultSets.forRows(resultSet.getType(), rows));
+  }
+
+  /**
+   * {@link Iterator} implementation for {@link com.google.cloud.spanner.ResultSet}. This is used to
+   * create a copy of an existing result set without the need to iterate the rows more than once.
+   */
+  private static class ResultSetIterator implements Iterator<Struct> {
+    private final com.google.cloud.spanner.ResultSet resultSet;
+    private boolean calculatedHasNext = false;
+    private boolean hasNext = false;
+
+    ResultSetIterator(com.google.cloud.spanner.ResultSet resultSet) {
+      this.resultSet = resultSet;
+    }
+
+    @Override
+    public boolean hasNext() {
+      if (!calculatedHasNext) {
+        calculatedHasNext = true;
+        hasNext = resultSet.next();
+      }
+      return hasNext;
+    }
+
+    @Override
+    public Struct next() {
+      if (hasNext()) {
+        // Indicate that the next call to hasNext() must re-check whether there are more results.
+        calculatedHasNext = false;
+        return resultSet.getCurrentRowAsStruct();
+      }
+      throw new NoSuchElementException();
+    }
+  }
+
   private boolean closed = false;
   private final Statement statement;
   private boolean wasNull = false;
diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcStatement.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcStatement.java
index 3e7c86f..3385ef0 100644
--- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcStatement.java
+++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcStatement.java
@@ -92,8 +92,9 @@ class JdbcStatement extends AbstractJdbcStatement {
     StatementResult result = execute(statement);
     switch (result.getResultType()) {
       case RESULT_SET:
-        throw JdbcSqlExceptionFactory.of(
-            "The statement is not a non-returning DML or DDL statement", Code.INVALID_ARGUMENT);
+        // Close the result set as we are not going to return it to the user. This prevents the
+        // underlying session from potentially being leaked.
+        throw closeResultSetAndCreateInvalidQueryException(result);
       case UPDATE_COUNT:
         return result.getUpdateCount();
       case NO_RESULT:
@@ -104,6 +105,17 @@ class JdbcStatement extends AbstractJdbcStatement {
     }
   }
 
+  private SQLException closeResultSetAndCreateInvalidQueryException(StatementResult result) {
+    //noinspection finally
+    try {
+      result.getResultSet().close();
+    } finally {
+      //noinspection ReturnInsideFinallyBlock
+      return JdbcSqlExceptionFactory.of(
+          "The statement is not a non-returning DML or DDL statement", Code.INVALID_ARGUMENT);
+    }
+  }
+
   /**
    * Adds a THEN RETURN/RETURNING clause to the given statement if the following conditions are all
    * met:
diff --git a/versions.txt b/versions.txt
index f29b5a4..4a69a3a 100644
--- a/versions.txt
+++ b/versions.txt
@@ -1,4 +1,4 @@
 # Format:
 # module:released-version:current-version
 
-google-cloud-spanner-jdbc:2.11.8:2.11.9-SNAPSHOT
+google-cloud-spanner-jdbc:2.11.9:2.11.10-SNAPSHOT


Test patch:
diff --git a/src/test/java/com/google/cloud/spanner/jdbc/ExecuteMockServerTest.java b/src/test/java/com/google/cloud/spanner/jdbc/ExecuteMockServerTest.java
new file mode 100644
index 0000000..f74de66
--- /dev/null
+++ b/src/test/java/com/google/cloud/spanner/jdbc/ExecuteMockServerTest.java
@@ -0,0 +1,114 @@
+/*
+ * Copyright 2023 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.cloud.spanner.jdbc;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult;
+import com.google.cloud.spanner.SessionPoolOptions;
+import com.google.cloud.spanner.connection.AbstractMockServerTest;
+import com.google.cloud.spanner.connection.ConnectionOptions;
+import com.google.protobuf.ListValue;
+import com.google.protobuf.Value;
+import com.google.rpc.Code;
+import com.google.spanner.v1.ResultSetMetadata;
+import com.google.spanner.v1.StructType;
+import com.google.spanner.v1.StructType.Field;
+import com.google.spanner.v1.Type;
+import com.google.spanner.v1.TypeCode;
+import java.sql.Connection;
+import java.sql.SQLException;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Test class for verifying that the methods execute, executeQuery, and executeUpdate work as
+ * intended.
+ */
+@RunWith(JUnit4.class)
+public class ExecuteMockServerTest extends AbstractMockServerTest {
+  private static final String QUERY = "select * from my_table";
+
+  @Before
+  public void setupResults() {
+    super.setupResults();
+    com.google.spanner.v1.ResultSet resultSet =
+        com.google.spanner.v1.ResultSet.newBuilder()
+            .setMetadata(
+                ResultSetMetadata.newBuilder()
+                    .setRowType(
+                        StructType.newBuilder()
+                            .addFields(
+                                Field.newBuilder()
+                                    .setType(Type.newBuilder().setCode(TypeCode.INT64).build())
+                                    .setName("id")
+                                    .build())
+                            .addFields(
+                                Field.newBuilder()
+                                    .setType(Type.newBuilder().setCode(TypeCode.STRING).build())
+                                    .setName("value")
+                                    .build())
+                            .build())
+                    .build())
+            .addRows(
+                ListValue.newBuilder()
+                    .addValues(Value.newBuilder().setStringValue("1").build())
+                    .addValues(Value.newBuilder().setStringValue("One").build())
+                    .build())
+            .build();
+    mockSpanner.putStatementResult(
+        StatementResult.query(com.google.cloud.spanner.Statement.of(QUERY), resultSet));
+  }
+
+  private String createUrl() {
+    return String.format(
+        "jdbc:cloudspanner://localhost:%d/projects/%s/instances/%s/databases/%s?usePlainText=true",
+        getPort(), "proj", "inst", "db");
+  }
+
+  @Test
+  public void testInvalidExecuteUpdate_shouldNotLeakSession() throws SQLException {
+    int maxSessions = 1;
+    try (Connection connection =
+        new JdbcConnection(
+            createUrl(),
+            ConnectionOptions.newBuilder()
+                .setUri(createUrl().substring("jdbc:".length()))
+                .setSessionPoolOptions(
+                    SessionPoolOptions.newBuilder()
+                        .setMinSessions(1)
+                        .setMaxSessions(maxSessions)
+                        .setFailIfPoolExhausted()
+                        .build())
+                .build())) {
+
+      for (int i = 0; i < (maxSessions + 1); i++) {
+        SQLException exception =
+            assertThrows(
+                SQLException.class, () -> connection.createStatement().executeUpdate(QUERY));
+        assertTrue(exception instanceof JdbcSqlException);
+        JdbcSqlException jdbcSqlException = (JdbcSqlException) exception;
+        // This would be RESOURCE_EXHAUSTED if the query leaked a session.
+        assertEquals(Code.INVALID_ARGUMENT, jdbcSqlException.getCode());
+      }
+    }
+  }
+}
diff --git a/src/test/java/com/google/cloud/spanner/jdbc/JdbcResultSetTest.java b/src/test/java/com/google/cloud/spanner/jdbc/JdbcResultSetTest.java
index 024f6c2..476c404 100644
--- a/src/test/java/com/google/cloud/spanner/jdbc/JdbcResultSetTest.java
+++ b/src/test/java/com/google/cloud/spanner/jdbc/JdbcResultSetTest.java
@@ -37,6 +37,7 @@ import com.google.cloud.spanner.Type;
 import com.google.cloud.spanner.Type.StructField;
 import com.google.cloud.spanner.Value;
 import com.google.cloud.spanner.jdbc.JdbcSqlExceptionFactory.JdbcSqlExceptionImpl;
+import com.google.common.collect.ImmutableList;
 import com.google.rpc.Code;
 import java.io.IOException;
 import java.io.InputStream;
@@ -46,9 +47,11 @@ import java.math.RoundingMode;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
+import java.sql.ResultSetMetaData;
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.sql.Time;
+import java.sql.Types;
 import java.time.Instant;
 import java.time.LocalDate;
 import java.time.OffsetDateTime;
@@ -1753,4 +1756,57 @@ public class JdbcResultSetTest {
         offsetDateTime);
     assertFalse(subject.wasNull());
   }
+
+  @Test
+  public void testCopyOf() throws SQLException {
+    ResultSet original =
+        ResultSets.forRows(
+            Type.struct(StructField.of("id", Type.int64()), StructField.of("value", Type.string())),
+            ImmutableList.of(
+                Struct.newBuilder().set("id").to(1L).set("value").to("One").build(),
+                Struct.newBuilder().set("id").to(2L).set("value").to("Two").build()));
+    java.sql.ResultSet copy = JdbcResultSet.copyOf(original);
+    // The original result set has been fully consumed.
+    assertFalse(original.next());
+    // We can safely close the original result set and still use the copy.
+    original.close();
+
+    ResultSetMetaData metadata = copy.getMetaData();
+    assertEquals(2, metadata.getColumnCount());
+    assertEquals("id", metadata.getColumnName(1));
+    assertEquals("value", metadata.getColumnName(2));
+    assertEquals(Types.BIGINT, metadata.getColumnType(1));
+    assertEquals(Types.NVARCHAR, metadata.getColumnType(2));
+
+    assertTrue(copy.next());
+    assertEquals(1L, copy.getLong(1));
+    assertEquals("One", copy.getString(2));
+    assertTrue(copy.next());
+    assertEquals(2L, copy.getLong("id"));
+    assertEquals("Two", copy.getString("value"));
+    assertFalse(copy.next());
+  }
+
+  @Test
+  public void testCopyOfEmpty() throws SQLException {
+    ResultSet original =
+        ResultSets.forRows(
+            Type.struct(StructField.of("id", Type.int64()), StructField.of("value", Type.string())),
+            ImmutableList.of());
+    java.sql.ResultSet copy = JdbcResultSet.copyOf(original);
+    // The original result set has been fully consumed.
+    assertFalse(original.next());
+    // We can safely close the original result set and still use the copy.
+    original.close();
+
+    ResultSetMetaData metadata = copy.getMetaData();
+    assertEquals(2, metadata.getColumnCount());
+    assertEquals("id", metadata.getColumnName(1));
+    assertEquals("value", metadata.getColumnName(2));
+    assertEquals(Types.BIGINT, metadata.getColumnType(1));
+    assertEquals(Types.NVARCHAR, metadata.getColumnType(2));
+
+    // The copy should not contain any rows.
+    assertFalse(copy.next());
+  }
 }