fix: allow zero durations to be set for connections (#3916)

SET statements that set a duration, e.g. max_commit_delay or statement_timeout,
did not allow setting a zero duration. This is however sometimes useful, as the
server default for max_commit_delay is not zero. This means that setting this to
zero triggers a different behavior than not setting a value, as when no value is
included in a commit request, Spanner will choose a reasonable value for the
commit delay. By explicitly setting it to zero, Spanner will always also use a
zero commit delay.

Author: olavloite

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java

@@ -248,7 +248,8 @@ public Duration convert(String value) {
             } else {
               duration = Duration.ofMillis(Long.parseLong(value.trim()));
             }
-            if (duration.isZero()) {
+            // Converters should return null for invalid values.
+            if (duration.isNegative()) {
               return null;
             }
             return duration;

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

@@ -361,7 +361,7 @@ public StatementResult statementShowReturnCommitStats() {
 
   @Override
   public StatementResult statementSetMaxCommitDelay(Duration duration) {
-    getConnection().setMaxCommitDelay(duration == null || duration.isZero() ? null : duration);
+    getConnection().setMaxCommitDelay(duration);
     return noResult(SET_MAX_COMMIT_DELAY);
   }
 

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractConnectionImplTest.java

@@ -276,13 +276,14 @@ public void testSetStatementTimeout() {
         assertThat(connection.hasStatementTimeout(), is(false));
         boolean gotException = false;
         try {
-          log("@EXPECT EXCEPTION INVALID_ARGUMENT");
+          //          log("@EXPECT EXCEPTION INVALID_ARGUMENT");
           log(String.format("SET STATEMENT_TIMEOUT='0%s';", getTimeUnitAbbreviation(unit)));
-          connection.setStatementTimeout(0L, unit);
+          connection.clearStatementTimeout();
+          //          connection.setStatementTimeout(0L, unit);
         } catch (IllegalArgumentException e) {
           gotException = true;
         }
-        assertThat(gotException, is(true));
+        assertThat(gotException, is(false));
         log(
             String.format(
                 "@EXPECT RESULT_SET 'STATEMENT_TIMEOUT',%s",

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/DurationConverterTest.java

@@ -42,7 +42,7 @@ public void testConvert() throws CompileException {
     DurationConverter converter = new DurationConverter(allowedValues);
     assertThat(converter.convert("'100ms'"), is(equalTo(Duration.ofMillis(100L))));
     assertThat(converter.convert("100"), is(equalTo(Duration.ofMillis(100))));
-    assertThat(converter.convert("'0ms'"), is(nullValue()));
+    assertThat(converter.convert("'0ms'"), is(Duration.ZERO));
     assertThat(converter.convert("'-100ms'"), is(nullValue()));
     assertThat(
         converter.convert("'315576000000000ms'"), is(equalTo(Duration.ofSeconds(315576000000L))));

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/MaxCommitDelayTest.java

@@ -83,7 +83,20 @@ public void testNoMaxCommitDelayByDefault() {
       for (boolean autocommit : new boolean[] {true, false}) {
         connection.setAutocommit(autocommit);
         executeCommit(connection);
-        assertMaxCommitDelay(Duration.getDefaultInstance());
+        assertMaxCommitDelay(Duration.getDefaultInstance(), false);
+        mockSpanner.clearRequests();
+      }
+    }
+  }
+
+  @Test
+  public void testZeroMaxCommitDelay() {
+    try (Connection connection = createConnection()) {
+      for (boolean autocommit : new boolean[] {true, false}) {
+        connection.setAutocommit(autocommit);
+        connection.setMaxCommitDelay(java.time.Duration.ZERO);
+        executeCommit(connection);
+        assertMaxCommitDelay(Duration.getDefaultInstance(), true);
         mockSpanner.clearRequests();
       }
     }
@@ -95,7 +108,19 @@ public void testMaxCommitDelayInConnectionString() {
       for (boolean autocommit : new boolean[] {true, false}) {
         connection.setAutocommit(autocommit);
         executeCommit(connection);
-        assertMaxCommitDelay(Duration.newBuilder().setSeconds(1).build());
+        assertMaxCommitDelay(Duration.newBuilder().setSeconds(1).build(), true);
+        mockSpanner.clearRequests();
+      }
+    }
+  }
+
+  @Test
+  public void testZeroMaxCommitDelayInConnectionString() {
+    try (Connection connection = createConnection(";maxCommitDelay=0")) {
+      for (boolean autocommit : new boolean[] {true, false}) {
+        connection.setAutocommit(autocommit);
+        executeCommit(connection);
+        assertMaxCommitDelay(Duration.getDefaultInstance(), true);
         mockSpanner.clearRequests();
       }
     }
@@ -121,20 +146,31 @@ public void testSetMaxCommitDelay() {
               () -> {
                 executeCommit(connection);
                 assertMaxCommitDelay(
-                    Duration.newBuilder()
-                        .setNanos((int) TimeUnit.MILLISECONDS.toNanos(40))
-                        .build());
+                    Duration.newBuilder().setNanos((int) TimeUnit.MILLISECONDS.toNanos(40)).build(),
+                    true);
                 mockSpanner.clearRequests();
               });
 
           if (useSql) {
+            // This is translated to Duration.ZERO.
             connection.execute(
                 Statement.of(String.format("set %smax_commit_delay=null", getVariablePrefix())));
           } else {
             connection.setMaxCommitDelay(null);
           }
           executeCommit(connection);
-          assertMaxCommitDelay(Duration.getDefaultInstance());
+          // The SQL statement set max_commit_delay=null is translated to Duration.ZERO.
+          assertMaxCommitDelay(Duration.getDefaultInstance(), useSql);
+          mockSpanner.clearRequests();
+
+          if (useSql) {
+            connection.execute(
+                Statement.of(String.format("set %smax_commit_delay=0", getVariablePrefix())));
+          } else {
+            connection.setMaxCommitDelay(java.time.Duration.ZERO);
+          }
+          executeCommit(connection);
+          assertMaxCommitDelay(Duration.getDefaultInstance(), true);
           mockSpanner.clearRequests();
         }
       }
@@ -150,10 +186,11 @@ void executeCommit(Connection connection) {
     }
   }
 
-  private void assertMaxCommitDelay(Duration expected) {
+  private void assertMaxCommitDelay(Duration expected, boolean hasMaxCommitDelay) {
     List<CommitRequest> requests = mockSpanner.getRequestsOfType(CommitRequest.class);
     assertEquals(1, requests.size());
     CommitRequest request = requests.get(0);
     assertEquals(expected, request.getMaxCommitDelay());
+    assertEquals(hasMaxCommitDelay, request.hasMaxCommitDelay());
   }
 }

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/PgDurationConverterTest.java

@@ -45,7 +45,7 @@ public void testConvert() throws CompileException {
 
     assertEquals(
         Duration.ofNanos((int) TimeUnit.MILLISECONDS.toNanos(100L)), converter.convert("'100ms'"));
-    assertNull(converter.convert("'0ms'"));
+    assertEquals(Duration.ZERO, converter.convert("'0ms'"));
     assertNull(converter.convert("'-100ms'"));
     assertEquals(Duration.ofSeconds(315576000000L), converter.convert("'315576000000000ms'"));
     assertEquals(Duration.ofSeconds(1L), converter.convert("'1s'"));