From a7d0fbb529ff71b45d6ddbbad8fc3be43e7c966f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 25 Aug 2023 14:05:13 +0200 Subject: [PATCH] fix: session leak for invalid update (#1323) * deps: update dependency com.google.cloud:google-cloud-spanner-bom to v6.45.2 (#1318) * 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. --------- Co-authored-by: Mend Renovate --- .../cloud/spanner/jdbc/JdbcStatement.java | 16 ++- .../spanner/jdbc/ExecuteMockServerTest.java | 114 ++++++++++++++++++ 2 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 src/test/java/com/google/cloud/spanner/jdbc/ExecuteMockServerTest.java 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 3e7c86f9..3385ef01 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 @@ public long executeLargeUpdate(String sql) throws SQLException { 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 @@ public long executeLargeUpdate(String sql) throws SQLException { } } + 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/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 00000000..f74de663 --- /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()); + } + } + } +}