fix: return errors from BatchCreateSession to dialect detection (#1760)

Any error that was returned by BatchCreateSessions should also be set as the result of the auto-detect dialect query, as the query will never be able to return a result for the dialect when session creation fails. Fixes #1759
googleapis · Mar 24, 2022 · 6550a9d · 6550a9d
1 parent 6d64104
commit 6550a9d
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 0 deletions.
diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
@@ -2210,6 +2210,9 @@ private void handleCreateSessionsFailure(SpannerException e, int count) {
           break;
         }
       }
+      if (!dialect.isDone()) {
+        dialect.setException(e);
+      }
       if (isDatabaseOrInstanceNotFound(e)) {
         setResourceNotFoundException((ResourceNotFoundException) e);
         poolMaintainer.close();

diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
@@ -2261,6 +2261,46 @@ public void testGetDialectPostgreSQLPreloaded() {
     }
   }
 
+  @Test
+  public void testGetDialect_FailsDirectlyIfDatabaseNotFound() {
+    mockSpanner.setBatchCreateSessionsExecutionTime(
+        SimulatedExecutionTime.stickyDatabaseNotFoundException("invalid-database"));
+    DatabaseClient client =
+        spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+
+    SpannerException exception = assertThrows(SpannerException.class, client::getDialect);
+    assertEquals(ErrorCode.NOT_FOUND, exception.getErrorCode());
+    assertTrue(
+        exception
+            .getMessage()
+            .contains(
+                "NOT_FOUND: Database not found: Database with id invalid-database not found"));
+  }
+
+  @Test
+  public void testGetDialectDefaultPreloaded_FailsDirectlyIfDatabaseNotFound() {
+    mockSpanner.setBatchCreateSessionsExecutionTime(
+        SimulatedExecutionTime.stickyDatabaseNotFoundException("invalid-database"));
+    try (Spanner spanner =
+        this.spanner
+            .getOptions()
+            .toBuilder()
+            .setSessionPoolOption(
+                SessionPoolOptions.newBuilder().setAutoDetectDialect(true).build())
+            .build()
+            .getService()) {
+      DatabaseClient client =
+          spanner.getDatabaseClient(DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+      SpannerException exception = assertThrows(SpannerException.class, client::getDialect);
+      assertEquals(ErrorCode.NOT_FOUND, exception.getErrorCode());
+      assertTrue(
+          exception
+              .getMessage()
+              .contains(
+                  "NOT_FOUND: Database not found: Database with id invalid-database not found"));
+    }
+  }
+
   @Test
   public void testUntypedNullParameters() {
     Statement statement =

diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionTest.java
@@ -19,13 +19,17 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import com.google.api.core.ApiFuture;
 import com.google.api.core.ApiFutures;
 import com.google.cloud.spanner.AbortedException;
+import com.google.cloud.spanner.Dialect;
 import com.google.cloud.spanner.ErrorCode;
+import com.google.cloud.spanner.MockSpannerServiceImpl;
+import com.google.cloud.spanner.MockSpannerServiceImpl.SimulatedExecutionTime;
 import com.google.cloud.spanner.ResultSet;
 import com.google.cloud.spanner.SpannerException;
 import com.google.cloud.spanner.SpannerOptions;
@@ -43,6 +47,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import javax.annotation.Nonnull;
+import org.junit.After;
 import org.junit.AfterClass;
 import org.junit.Test;
 import org.junit.experimental.runners.Enclosed;
@@ -465,4 +470,48 @@ public void testShowSetRPCPriority() {
       }
     }
   }
+
+  public static class DialectDetectionTest extends AbstractMockServerTest {
+    protected String getBaseUrl() {
+      return super.getBaseUrl() + ";minSessions=1";
+    }
+
+    @After
+    public void reset() {
+      // Reset dialect to default.
+      mockSpanner.putStatementResult(
+          MockSpannerServiceImpl.StatementResult.detectDialectResult(Dialect.GOOGLE_STANDARD_SQL));
+      mockSpanner.reset();
+      mockSpanner.removeAllExecutionTimes();
+      // Close all open Spanner instances to ensure that each test run gets a fresh instance.
+      SpannerPool.closeSpannerPool();
+    }
+
+    @Test
+    public void testDefaultGetDialect() {
+      try (Connection connection = createConnection()) {
+        assertEquals(Dialect.GOOGLE_STANDARD_SQL, connection.getDialect());
+      }
+    }
+
+    @Test
+    public void testPostgreSQLGetDialect() {
+      mockSpanner.putStatementResult(
+          MockSpannerServiceImpl.StatementResult.detectDialectResult(Dialect.POSTGRESQL));
+      try (Connection connection = createConnection()) {
+        assertEquals(Dialect.POSTGRESQL, connection.getDialect());
+      }
+    }
+
+    @Test
+    public void testGetDialect_DatabaseNotFound() throws Exception {
+      mockSpanner.setBatchCreateSessionsExecutionTime(
+          SimulatedExecutionTime.stickyDatabaseNotFoundException("invalid-database"));
+      try (Connection connection = createConnection()) {
+        SpannerException exception = assertThrows(SpannerException.class, connection::getDialect);
+        assertEquals(ErrorCode.NOT_FOUND, exception.getErrorCode());
+        assertTrue(exception.getMessage().contains("Database with id invalid-database not found"));
+      }
+    }
+  }
 }