Repository: googleapis/java-spanner-jdbc
Pull request #324 (2021-01-17 12:54:56)

feat: support default ClientInfo properties
Adds support for default `ClientInfo` properties. These are not required by the JDBC standard, but many applications (including IntelliJ) will try to set these, and will log a warning if they are not set. This will prevent that warning from being logged.

As a next step, these `ClientInfo` property values should be set as `SessionLabel` values in Cloud Spanner. This makes it possible to track which sessions are being used by which (JDBC) application.

codecov[bot]: # [Codecov](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324?src=pr&el=h1) Report
> Merging [#324](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324?src=pr&el=desc) (7cba84e) into [master](https://codecov.io/gh/googleapis/java-spanner-jdbc/commit/e1cd90d8afbfa725a92186b85bd446413c8ed4bc?el=desc) (e1cd90d) will **increase** coverage by `0.36%`.
> The diff coverage is `89.65%`.

[![Impacted file tree graph](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324/graphs/tree.svg?width=650&height=150&src=pr&token=ODlczvbGrj)](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master     #324      +/-   ##
============================================
+ Coverage     70.93%   71.30%   +0.36%     
- Complexity     1105     1112       +7     
============================================
  Files            24       24              
  Lines          3423     3474      +51     
  Branches        526      531       +5     
============================================
+ Hits           2428     2477      +49     
  Misses          780      780              
- Partials        215      217       +2     
```


| [Impacted Files](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../com/google/cloud/spanner/jdbc/JdbcConnection.java](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324/diff?src=pr&el=tree#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvamRiYy9KZGJjQ29ubmVjdGlvbi5qYXZh) | `44.01% <ø> (ø)` | `42.00 <0.00> (ø)` | |
| [...gle/cloud/spanner/jdbc/AbstractJdbcConnection.java](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324/diff?src=pr&el=tree#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvamRiYy9BYnN0cmFjdEpkYmNDb25uZWN0aW9uLmphdmE=) | `67.81% <70.00%> (+3.92%)` | `24.00 <2.00> (+4.00)` | |
| [...oogle/cloud/spanner/jdbc/JdbcDatabaseMetaData.java](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324/diff?src=pr&el=tree#diff-c3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3NwYW5uZXIvamRiYy9KZGJjRGF0YWJhc2VNZXRhRGF0YS5qYXZh) | `95.68% <100.00%> (+0.20%)` | `183.00 <3.00> (+3.00)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324?src=pr&el=footer). Last update [e1cd90d...7cba84e](https://codecov.io/gh/googleapis/java-spanner-jdbc/pull/324?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pom.xml b/pom.xml
index ae04a08..9797798 100644
--- a/pom.xml
+++ b/pom.xml
@@ -71,7 +71,7 @@
       <dependency>
         <groupId>com.google.cloud</groupId>
         <artifactId>google-cloud-spanner-bom</artifactId>
-        <version>3.1.2</version>
+        <version>3.3.1</version>
         <type>pom</type>
         <scope>import</scope>
       </dependency>
diff --git a/samples/snippets/pom.xml b/samples/snippets/pom.xml
index 6cef2b5..69cbd88 100644
--- a/samples/snippets/pom.xml
+++ b/samples/snippets/pom.xml
@@ -30,7 +30,7 @@
       <dependency>
         <groupId>com.google.cloud</groupId>
         <artifactId>libraries-bom</artifactId>
-        <version>16.2.1</version>
+        <version>16.3.0</version>
         <type>pom</type>
         <scope>import</scope>
       </dependency>
diff --git a/src/main/java/com/google/cloud/spanner/jdbc/AbstractJdbcConnection.java b/src/main/java/com/google/cloud/spanner/jdbc/AbstractJdbcConnection.java
index aad5ad8..385a46f 100644
--- a/src/main/java/com/google/cloud/spanner/jdbc/AbstractJdbcConnection.java
+++ b/src/main/java/com/google/cloud/spanner/jdbc/AbstractJdbcConnection.java
@@ -45,19 +45,21 @@ abstract class AbstractJdbcConnection extends AbstractJdbcWrapper
   private static final String ABORT_UNSUPPORTED = "Abort is not supported";
   private static final String NETWORK_TIMEOUT_UNSUPPORTED = "Network timeout is not supported";
   static final String CLIENT_INFO_NOT_SUPPORTED =
-      "Cloud Spanner does not support any ClientInfo properties";
+      "Cloud Spanner does not support ClientInfo property %s";
 
   private final String connectionUrl;
   private final ConnectionOptions options;
   private final com.google.cloud.spanner.connection.Connection spanner;
+  private final Properties clientInfo;
 
   private SQLWarning firstWarning = null;
   private SQLWarning lastWarning = null;
 
-  AbstractJdbcConnection(String connectionUrl, ConnectionOptions options) {
+  AbstractJdbcConnection(String connectionUrl, ConnectionOptions options) throws SQLException {
     this.connectionUrl = connectionUrl;
     this.options = options;
     this.spanner = options.getConnection();
+    this.clientInfo = new Properties(JdbcDatabaseMetaData.getDefaultClientInfoProperties());
   }
 
   /** Return the corresponding {@link com.google.cloud.spanner.connection.Connection} */
@@ -168,8 +170,10 @@ abstract class AbstractJdbcConnection extends AbstractJdbcWrapper
 
   @Override
   public void setClientInfo(String name, String value) throws SQLClientInfoException {
+    Properties supported = null;
     try {
       checkClosed();
+      supported = JdbcDatabaseMetaData.getDefaultClientInfoProperties();
     } catch (SQLException e) {
       if (e instanceof JdbcSqlException) {
         throw JdbcSqlExceptionFactory.clientInfoException(
@@ -178,7 +182,23 @@ abstract class AbstractJdbcConnection extends AbstractJdbcWrapper
         throw JdbcSqlExceptionFactory.clientInfoException(e.getMessage(), Code.UNKNOWN);
       }
     }
-    pushWarning(new SQLWarning(CLIENT_INFO_NOT_SUPPORTED));
+    if (value == null) {
+      throw JdbcSqlExceptionFactory.clientInfoException(
+          "Null-value is not allowed for client info.", Code.INVALID_ARGUMENT);
+    }
+    if (value.length() > JdbcDatabaseMetaData.MAX_CLIENT_INFO_VALUE_LENGTH) {
+      throw JdbcSqlExceptionFactory.clientInfoException(
+          String.format(
+              "Max length of value is %d characters.",
+              JdbcDatabaseMetaData.MAX_CLIENT_INFO_VALUE_LENGTH),
+          Code.INVALID_ARGUMENT);
+    }
+    name = name.toUpperCase();
+    if (supported.containsKey(name)) {
+      clientInfo.setProperty(name, value);
+    } else {
+      pushWarning(new SQLWarning(String.format(CLIENT_INFO_NOT_SUPPORTED, name)));
+    }
   }
 
   @Override
@@ -193,19 +213,22 @@ abstract class AbstractJdbcConnection extends AbstractJdbcWrapper
         throw JdbcSqlExceptionFactory.clientInfoException(e.getMessage(), Code.UNKNOWN);
       }
     }
-    pushWarning(new SQLWarning(CLIENT_INFO_NOT_SUPPORTED));
+    clientInfo.clear();
+    for (String property : properties.stringPropertyNames()) {
+      setClientInfo(property, properties.getProperty(property));
+    }
   }
 
   @Override
   public String getClientInfo(String name) throws SQLException {
     checkClosed();
-    return null;
+    return clientInfo.getProperty(name.toUpperCase());
   }
 
   @Override
   public Properties getClientInfo() throws SQLException {
     checkClosed();
-    return null;
+    return (Properties) clientInfo.clone();
   }
 
   @Override
diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java
index 3d79c5e..6c23db4 100644
--- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java
+++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java
@@ -53,7 +53,7 @@ class JdbcConnection extends AbstractJdbcConnection {
 
   private Map<String, Class<?>> typeMap = new HashMap<>();
 
-  JdbcConnection(String connectionUrl, ConnectionOptions options) {
+  JdbcConnection(String connectionUrl, ConnectionOptions options) throws SQLException {
     super(connectionUrl, options);
   }
 
diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java
index 508a2c3..a5d79b1 100644
--- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java
+++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java
@@ -36,6 +36,7 @@ import java.sql.SQLException;
 import java.sql.Types;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Properties;
 import java.util.Scanner;
 
 /** {@link DatabaseMetaData} implementation for Cloud Spanner */
@@ -1495,16 +1496,68 @@ class JdbcDatabaseMetaData extends AbstractJdbcWrapper implements DatabaseMetaDa
     return false;
   }
 
-  @Override
-  public ResultSet getClientInfoProperties() throws SQLException {
+  /**
+   * The max length for client info values is 63 to make them fit in Cloud Spanner session labels.
+   */
+  static final int MAX_CLIENT_INFO_VALUE_LENGTH = 63;
+
+  static Properties getDefaultClientInfoProperties() throws SQLException {
+    Properties info = new Properties();
+    try (ResultSet rs = getDefaultClientInfo()) {
+      while (rs.next()) {
+        info.put(rs.getString("NAME"), rs.getString("DEFAULT_VALUE"));
+      }
+    }
+    return info;
+  }
+
+  private static ResultSet getDefaultClientInfo() throws SQLException {
     return JdbcResultSet.of(
         ResultSets.forRows(
             Type.struct(
                 StructField.of("NAME", Type.string()),
-                StructField.of("MAX_LEN", Type.string()),
+                StructField.of("MAX_LEN", Type.int64()),
                 StructField.of("DEFAULT_VALUE", Type.string()),
                 StructField.of("DESCRIPTION", Type.string())),
-            Collections.<Struct>emptyList()));
+            Arrays.asList(
+                Struct.newBuilder()
+                    .set("NAME")
+                    .to("APPLICATIONNAME")
+                    .set("MAX_LEN")
+                    .to(MAX_CLIENT_INFO_VALUE_LENGTH)
+                    .set("DEFAULT_VALUE")
+                    .to("")
+                    .set("DESCRIPTION")
+                    .to("The name of the application currently utilizing the connection.")
+                    .build(),
+                Struct.newBuilder()
+                    .set("NAME")
+                    .to("CLIENTHOSTNAME")
+                    .set("MAX_LEN")
+                    .to(MAX_CLIENT_INFO_VALUE_LENGTH)
+                    .set("DEFAULT_VALUE")
+                    .to("")
+                    .set("DESCRIPTION")
+                    .to(
+                        "The hostname of the computer the application using the connection is running on.")
+                    .build(),
+                Struct.newBuilder()
+                    .set("NAME")
+                    .to("CLIENTUSER")
+                    .set("MAX_LEN")
+                    .to(MAX_CLIENT_INFO_VALUE_LENGTH)
+                    .set("DEFAULT_VALUE")
+                    .to("")
+                    .set("DESCRIPTION")
+                    .to(
+                        "The name of the user that the application using the connection is performing work for. "
+                            + "This may not be the same as the user name that was used in establishing the connection.")
+                    .build())));
+  }
+
+  @Override
+  public ResultSet getClientInfoProperties() throws SQLException {
+    return getDefaultClientInfo();
   }
 
   @Override
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 3f10b0c..c4ca6b5 100644
--- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcResultSet.java
+++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcResultSet.java
@@ -547,7 +547,7 @@ class JdbcResultSet extends AbstractJdbcResultSet {
   @Override
   public JdbcResultSetMetaData getMetaData() throws SQLException {
     checkClosed();
-    if (isBeforeFirst()) {
+    if (isBeforeFirst() && !nextCalledForMetaData) {
       // do a call to next() on the underlying resultset to initialize metadata
       nextCalledForMetaData = true;
       nextCalledForMetaDataResult = spanner.next();


Test patch:
diff --git a/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionGeneratedSqlScriptTest.java b/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionGeneratedSqlScriptTest.java
index b835668..b818c63 100644
--- a/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionGeneratedSqlScriptTest.java
+++ b/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionGeneratedSqlScriptTest.java
@@ -19,12 +19,14 @@ package com.google.cloud.spanner.jdbc;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import com.google.cloud.spanner.SpannerExceptionFactory;
 import com.google.cloud.spanner.connection.AbstractSqlScriptVerifier.GenericConnection;
 import com.google.cloud.spanner.connection.AbstractSqlScriptVerifier.GenericConnectionProvider;
 import com.google.cloud.spanner.connection.ConnectionImplTest;
 import com.google.cloud.spanner.connection.ConnectionOptions;
 import com.google.cloud.spanner.connection.SqlScriptVerifier;
 import com.google.cloud.spanner.jdbc.JdbcSqlScriptVerifier.JdbcGenericConnection;
+import java.sql.SQLException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -46,13 +48,17 @@ public class JdbcConnectionGeneratedSqlScriptTest {
       com.google.cloud.spanner.connection.Connection spannerConnection =
           ConnectionImplTest.createConnection(options);
       when(options.getConnection()).thenReturn(spannerConnection);
-      JdbcConnection connection =
-          new JdbcConnection(
-              "jdbc:cloudspanner://localhost/projects/project/instances/instance/databases/database;credentialsUrl=url",
-              options);
-      JdbcGenericConnection res = JdbcGenericConnection.of(connection);
-      res.setStripCommentsBeforeExecute(true);
-      return res;
+      try {
+        JdbcConnection connection =
+            new JdbcConnection(
+                "jdbc:cloudspanner://localhost/projects/project/instances/instance/databases/database;credentialsUrl=url",
+                options);
+        JdbcGenericConnection res = JdbcGenericConnection.of(connection);
+        res.setStripCommentsBeforeExecute(true);
+        return res;
+      } catch (SQLException e) {
+        throw SpannerExceptionFactory.asSpannerException(e);
+      }
     }
   }
 
diff --git a/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java b/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java
index 82e26d0..27389b4 100644
--- a/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java
+++ b/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java
@@ -59,7 +59,7 @@ public class JdbcConnectionTest {
           Type.struct(StructField.of("", Type.int64())),
           Arrays.asList(Struct.newBuilder().set("").to(1L).build()));
 
-  private JdbcConnection createConnection(ConnectionOptions options) {
+  private JdbcConnection createConnection(ConnectionOptions options) throws SQLException {
     com.google.cloud.spanner.connection.Connection spannerConnection =
         ConnectionImplTest.createConnection(options);
     when(options.getConnection()).thenReturn(spannerConnection);
@@ -405,14 +405,24 @@ public class JdbcConnectionTest {
   }
 
   @Test
-  public void testSetClientInfo() throws SQLException {
+  public void getDefaultClientInfo() throws SQLException {
+    ConnectionOptions options = mock(ConnectionOptions.class);
+    try (JdbcConnection connection = createConnection(options)) {
+      Properties defaultProperties = connection.getClientInfo();
+      assertThat(defaultProperties.stringPropertyNames())
+          .containsExactly("APPLICATIONNAME", "CLIENTHOSTNAME", "CLIENTUSER");
+    }
+  }
+
+  @Test
+  public void testSetInvalidClientInfo() throws SQLException {
     ConnectionOptions options = mock(ConnectionOptions.class);
     try (JdbcConnection connection = createConnection(options)) {
       assertThat((Object) connection.getWarnings()).isNull();
       connection.setClientInfo("test", "foo");
       assertThat((Object) connection.getWarnings()).isNotNull();
       assertThat(connection.getWarnings().getMessage())
-          .isEqualTo(AbstractJdbcConnection.CLIENT_INFO_NOT_SUPPORTED);
+          .isEqualTo(String.format(AbstractJdbcConnection.CLIENT_INFO_NOT_SUPPORTED, "TEST"));
 
       connection.clearWarnings();
       assertThat((Object) connection.getWarnings()).isNull();
@@ -422,7 +432,38 @@ public class JdbcConnectionTest {
       connection.setClientInfo(props);
       assertThat((Object) connection.getWarnings()).isNotNull();
       assertThat(connection.getWarnings().getMessage())
-          .isEqualTo(AbstractJdbcConnection.CLIENT_INFO_NOT_SUPPORTED);
+          .isEqualTo(String.format(AbstractJdbcConnection.CLIENT_INFO_NOT_SUPPORTED, "TEST"));
+    }
+  }
+
+  @Test
+  public void testSetClientInfo() throws SQLException {
+    ConnectionOptions options = mock(ConnectionOptions.class);
+    try (JdbcConnection connection = createConnection(options)) {
+      try (ResultSet validProperties = connection.getMetaData().getClientInfoProperties()) {
+        while (validProperties.next()) {
+          assertThat((Object) connection.getWarnings()).isNull();
+          String name = validProperties.getString("NAME");
+
+          connection.setClientInfo(name, "new-client-info-value");
+          assertThat((Object) connection.getWarnings()).isNull();
+          assertThat(connection.getClientInfo(name)).isEqualTo("new-client-info-value");
+
+          Properties props = new Properties();
+          props.setProperty(name.toLowerCase(), "some-other-value");
+          connection.setClientInfo(props);
+          assertThat((Object) connection.getWarnings()).isNull();
+          assertThat(connection.getClientInfo(name)).isEqualTo("some-other-value");
+          assertThat(connection.getClientInfo().keySet()).hasSize(1);
+          for (String key : connection.getClientInfo().stringPropertyNames()) {
+            if (key.equals(name)) {
+              assertThat(connection.getClientInfo().getProperty(key)).isEqualTo("some-other-value");
+            } else {
+              assertThat(connection.getClientInfo().getProperty(key)).isEqualTo("");
+            }
+          }
+        }
+      }
     }
   }
 
diff --git a/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java b/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java
index b3697a0..ac8ab7b 100644
--- a/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java
+++ b/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java
@@ -322,6 +322,18 @@ public class JdbcDatabaseMetaDataTest {
     JdbcConnection connection = mock(JdbcConnection.class);
     DatabaseMetaData meta = new JdbcDatabaseMetaData(connection);
     try (ResultSet rs = meta.getClientInfoProperties()) {
+      assertThat(rs.next(), is(true));
+      assertThat(rs.getString("NAME"), is(equalTo("APPLICATIONNAME")));
+      assertThat(rs.getString("DEFAULT_VALUE"), is(equalTo("")));
+
+      assertThat(rs.next(), is(true));
+      assertThat(rs.getString("NAME"), is(equalTo("CLIENTHOSTNAME")));
+      assertThat(rs.getString("DEFAULT_VALUE"), is(equalTo("")));
+
+      assertThat(rs.next(), is(true));
+      assertThat(rs.getString("NAME"), is(equalTo("CLIENTUSER")));
+      assertThat(rs.getString("DEFAULT_VALUE"), is(equalTo("")));
+
       assertThat(rs.next(), is(false));
       ResultSetMetaData rsmd = rs.getMetaData();
       assertThat(rsmd.getColumnCount(), is(equalTo(4)));
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 9429288..2ad4a6d 100644
--- a/src/test/java/com/google/cloud/spanner/jdbc/JdbcResultSetTest.java
+++ b/src/test/java/com/google/cloud/spanner/jdbc/JdbcResultSetTest.java
@@ -25,6 +25,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 import com.google.cloud.ByteArray;
 import com.google.cloud.Date;
@@ -802,6 +803,32 @@ public class JdbcResultSetTest {
     assertNotNull(metadata);
   }
 
+  @Test
+  public void testGetMetaDataBeforeNext() throws SQLException {
+    ResultSet spannerResultSet = mock(ResultSet.class);
+    when(spannerResultSet.next()).thenReturn(true, false);
+
+    JdbcResultSet resultSet = JdbcResultSet.of(spannerResultSet);
+    assertNotNull(resultSet.getMetaData());
+    assertTrue(resultSet.next());
+    assertFalse(resultSet.next());
+  }
+
+  @Test
+  public void testGetMetaDataTwiceBeforeNext() throws SQLException {
+    ResultSet spannerResultSet = mock(ResultSet.class);
+    when(spannerResultSet.next()).thenReturn(true, false);
+
+    JdbcResultSet resultSet = JdbcResultSet.of(spannerResultSet);
+    assertNotNull(resultSet.getMetaData());
+    assertNotNull(resultSet.getMetaData());
+
+    // This would have returned false before the fix in
+    // https://github.com/googleapis/java-spanner-jdbc/pull/323
+    assertTrue(resultSet.next());
+    assertFalse(resultSet.next());
+  }
+
   @Test
   public void testFindColumn() throws SQLException {
     assertEquals(2, subject.findColumn(STRING_COL_NOT_NULL));