From 2d61bc410b7c680169129725bcc11069c2390505 Mon Sep 17 00:00:00 2001
From: Thiago Nunes <thiagotnunes@google.com>
Date: Wed, 7 Apr 2021 09:02:50 +1000
Subject: [PATCH] fix: local connection checker ignores exceptions (#1036)

* fix: local connection checker ignores exceptions

The local connection checker should only check for unavailable
exceptions when warning the user about the spanner emulator
configuration. It should ignore all other kinds of errors.

* tests: refactors local connection checker tests
---
 .../connection/LocalConnectionChecker.java    |  9 +--
 .../LocalConnectionCheckerTest.java           | 55 +++++++++++++++----
 2 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/LocalConnectionChecker.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/LocalConnectionChecker.java
index c35390e447..4271a8c2f0 100644
--- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/LocalConnectionChecker.java
+++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/LocalConnectionChecker.java
@@ -19,13 +19,11 @@
 import com.google.api.gax.core.NoCredentialsProvider;
 import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
 import com.google.api.gax.rpc.UnavailableException;
-import com.google.api.gax.rpc.UnimplementedException;
 import com.google.cloud.spanner.ErrorCode;
 import com.google.cloud.spanner.SpannerExceptionFactory;
 import com.google.cloud.spanner.admin.instance.v1.stub.GrpcInstanceAdminStub;
 import com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStubSettings;
 import com.google.spanner.admin.instance.v1.ListInstanceConfigsRequest;
-import java.io.IOException;
 import org.threeten.bp.Duration;
 
 /**
@@ -95,10 +93,9 @@ void checkLocalConnection(ConnectionOptions options) {
                   emulatorHost);
         }
         throw SpannerExceptionFactory.newSpannerException(ErrorCode.UNAVAILABLE, msg);
-      } catch (UnimplementedException e) {
-        // Ignore, this is probably a local mock server.
-      } catch (IOException e) {
-        // Ignore, this method is not checking whether valid credentials have been set.
+      } catch (Throwable t) {
+        // Ignore, any other exceptions should also be thrown when connecting to the remote
+        // server and should not be treated here.
       }
     }
   }
diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/LocalConnectionCheckerTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/LocalConnectionCheckerTest.java
index fd2901a986..daf42b55af 100644
--- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/LocalConnectionCheckerTest.java
+++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/LocalConnectionCheckerTest.java
@@ -17,39 +17,50 @@
 
 import static com.google.cloud.spanner.MockSpannerTestUtil.SELECT1;
 import static com.google.cloud.spanner.MockSpannerTestUtil.SELECT1_RESULTSET;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 import com.google.cloud.spanner.MockSpannerServiceImpl;
 import com.google.cloud.spanner.ResultSet;
+import com.google.cloud.spanner.SpannerException;
 import io.grpc.Server;
 import io.grpc.netty.shaded.io.grpc.netty.NettyServerBuilder;
 import java.net.InetSocketAddress;
-import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
 public class LocalConnectionCheckerTest {
 
-  private MockSpannerServiceImpl mockSpanner;
-  private Server server;
-  private InetSocketAddress address;
+  private static MockSpannerServiceImpl mockSpanner;
+  private static Server server;
+  private LocalConnectionChecker connectionChecker;
 
-  @Before
-  public void setUp() throws Exception {
+  @BeforeClass
+  public static void beforeClass() throws Exception {
     mockSpanner = new MockSpannerServiceImpl();
     mockSpanner.setAbortProbability(0.0D); // We don't want any unpredictable aborted transactions.
-    address = new InetSocketAddress("localhost", 0);
+
+    final InetSocketAddress address = new InetSocketAddress("localhost", 0);
     server = NettyServerBuilder.forAddress(address).addService(mockSpanner).build();
     server.start();
   }
 
-  @After
-  public void tearDown() throws Exception {
+  @AfterClass
+  public static void afterClass() throws Exception {
     server.shutdown();
     server.awaitTermination();
   }
 
+  @Before
+  public void setUp() {
+    mockSpanner.reset();
+    connectionChecker = new LocalConnectionChecker();
+  }
+
   @Test
-  public void localConnectionCheckerWorksWithMockSpanner() {
+  public void testMockSpanner() {
     final String uri =
         String.format(
             "cloudspanner://localhost:%d/projects/proj/instances/inst/databases/db?usePlainText=true",
@@ -63,4 +74,28 @@ public void localConnectionCheckerWorksWithMockSpanner() {
       while (resultSet.next()) {}
     }
   }
+
+  @Test
+  public void testNoRunningEmulator() {
+    final int port = server.getPort() - 1;
+    final String uri =
+        String.format(
+            "cloudspanner://localhost:%d/projects/proj/instances/inst/databases/db?usePlainText=true",
+            port);
+    final ConnectionOptions connectionOptions = ConnectionOptions.newBuilder().setUri(uri).build();
+
+    try {
+      connectionChecker.checkLocalConnection(connectionOptions);
+      fail("Unavailable exception expected");
+    } catch (SpannerException e) {
+      assertEquals(
+          "UNAVAILABLE: The connection string '"
+              + uri
+              + "' contains host 'localhost:"
+              + port
+              + "', but no running emulator or other server could be found at that address.\n"
+              + "Please check the connection string and/or that the emulator is running.",
+          e.getMessage());
+    }
+  }
 }