Fix ArithmeticException in toJavaDuration.

temporal-sdk/src/main/java/io/temporal/internal/common/ProtobufTimeUtils.java

@@ -25,28 +25,62 @@
 import com.google.protobuf.util.Timestamps;
 import java.time.Duration;
 import java.time.Instant;
+import java.util.Objects;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
 public class ProtobufTimeUtils {
+
+  static final long MAX_SECONDS = 315_576_000_000L;
+  static final long MIN_SECONDS = -315_576_000_000L;
+  static final int MAX_NANOS = 999_999_999;
+  static final int MIN_NANOS = -999_999_999;
+  static final int MILLIS_PER_NANO = 1_000_000;
+
+  /**
+   * Converts a Protobuf Duration to a Java Duration with millisecond precision. Null inputs are
+   * treated as zero.
+   */
   @Nonnull
-  public static Duration toJavaDuration(com.google.protobuf.Duration d) {
-    // TODO we should refactor an implicit conversion of empty values into ZERO and rename the
-    // current method into toJavaDurationSafe, toJavaDurationOrDefault or something like that
-    if (d == null) {
+  public static Duration toJavaDuration(@Nullable com.google.protobuf.Duration d) {
+    if (Objects.isNull(d)) {
       return Duration.ZERO;
     }
 
-    return Duration.ofMillis(Durations.toMillis(d));
+    // NB: Durations.toMillis is tempting, but it can throw ArithmeticException if
+    // Durations.isValid would return false, e.g. if the proto contains a number of
+    // seconds outside [MIN_SECONDS,MAX_SECONDS].  We don't want to throw an exception
+    // under any circumstances, so we clip it to the range using saturating arithmetic,
+    // since that's probably the best we can do to reflect the user's intent.
+    //
+    // Additionally, it's worth noting that Durations.toMillis truncates toward zero,
+    // just as we do here.  This behavior is correctly documented in the JavaDoc, but
+    // the exact wording is confusing: one can easily misread the phrase
+    // "rounded ... to the nearest millisecond" as a reference to "round to nearest"
+    // a.k.a. grade school rounding, where a fractional value of half or more gets
+    // rounded away from zero.  However, a careful reading of the JavaDoc shows that
+    // the doc author does not mean "nearest" in that way.
+    //
+    final long rawSeconds = d.getSeconds();
+    final int rawNanos = d.getNanos();
+    final long saturatedSeconds = Math.min(MAX_SECONDS, Math.max(MIN_SECONDS, rawSeconds));
+    final int saturatedNanos = Math.min(MAX_NANOS, Math.max(MIN_NANOS, rawNanos));
+    final int roundedNanos = (saturatedNanos / MILLIS_PER_NANO) * MILLIS_PER_NANO;
+    return Duration.ofSeconds(saturatedSeconds, roundedNanos);
   }
 
-  public static com.google.protobuf.Duration toProtoDuration(Duration d) {
-    // TODO we should refactor an implicit conversion of empty values into ZERO and rename the
-    // current method into toJavaDurationSafe, toJavaDurationOrDefault or something like that
-    if (d == null) {
+  /**
+   * Converts a Java Duration to a Protobuf Duration with millisecond precision. Null inputs are
+   * treated as zero.
+   */
+  @Nonnull
+  public static com.google.protobuf.Duration toProtoDuration(@Nullable Duration d) {
+    if (Objects.isNull(d)) {
       return Durations.ZERO;
     }
 
+    // NB: the overflow behavior is not documented, but Duration.toMillis seems to
+    // use saturating arithmetic, which is what we want.
     return Durations.fromMillis(d.toMillis());
   }
 

temporal-sdk/src/test/java/io/temporal/internal/common/ProtobufTimeUtilsTest.java

@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2022 Temporal Technologies, Inc. All Rights Reserved.
+ *
+ * Copyright (C) 2012-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Modifications copyright (C) 2017 Uber Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this material 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 io.temporal.internal.common;
+
+import static io.temporal.internal.common.ProtobufTimeUtils.MAX_SECONDS;
+import static io.temporal.internal.common.ProtobufTimeUtils.MIN_SECONDS;
+import static org.junit.Assert.assertEquals;
+
+import java.time.Duration;
+import java.util.Arrays;
+import java.util.Collection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class ProtobufTimeUtilsTest {
+
+  @Parameters
+  public static Collection<Object[]> data() {
+    return Arrays.asList(
+        // Values with integral milliseconds
+        new Object[] {0L, 0, 0L, 0},
+        new Object[] {0L, 1_000_000, 0L, 1_000_000},
+        new Object[] {0L, 500_000_000, 0L, 500_000_000},
+        new Object[] {0L, 999_000_000, 0L, 999_000_000},
+        new Object[] {123L, 456_000_000, 123L, 456_000_000},
+        new Object[] {0L, -1_000_000, 0L, -1_000_000},
+        new Object[] {0L, -500_000_000, 0L, -500_000_000},
+        new Object[] {0L, -999_000_000, 0L, -999_000_000},
+        new Object[] {-123L, -456_000_000, -123L, -456_000_000},
+
+        // Values with fractional milliseconds
+        new Object[] {0L, 123_000_001, 0L, 123_000_000},
+        new Object[] {0L, 123_100_000, 0L, 123_000_000},
+        new Object[] {0L, 123_499_999, 0L, 123_000_000},
+        new Object[] {0L, 123_500_000, 0L, 123_000_000},
+        new Object[] {0L, 123_999_999, 0L, 123_000_000},
+        new Object[] {0L, -123_000_001, 0L, -123_000_000},
+        new Object[] {0L, -123_100_000, 0L, -123_000_000},
+        new Object[] {0L, -123_499_999, 0L, -123_000_000},
+        new Object[] {0L, -123_500_000, 0L, -123_000_000},
+        new Object[] {0L, -123_999_999, 0L, -123_000_000},
+
+        // Extremely large values
+        new Object[] {MAX_SECONDS, 0, MAX_SECONDS, 0},
+        new Object[] {MAX_SECONDS, 999_000_000, MAX_SECONDS, 999_000_000},
+        new Object[] {MAX_SECONDS, 999_999_999, MAX_SECONDS, 999_000_000},
+        new Object[] {Long.MAX_VALUE, 0, MAX_SECONDS, 0},
+        new Object[] {Long.MAX_VALUE, 999_000_000, MAX_SECONDS, 999_000_000},
+        new Object[] {Long.MAX_VALUE, 999_999_999, MAX_SECONDS, 999_000_000},
+        new Object[] {MIN_SECONDS, 0, MIN_SECONDS, 0},
+        new Object[] {MIN_SECONDS, -999_000_000, MIN_SECONDS, -999_000_000},
+        new Object[] {MIN_SECONDS, -999_999_999, MIN_SECONDS, -999_000_000},
+        new Object[] {Long.MIN_VALUE, 0, MIN_SECONDS, 0},
+        new Object[] {Long.MIN_VALUE, -999_000_000, MIN_SECONDS, -999_000_000},
+        new Object[] {Long.MIN_VALUE, -999_999_999, MIN_SECONDS, -999_000_000});
+  }
+
+  private final long inputSeconds;
+  private final int inputNanos;
+  private final long outputSeconds;
+  private final int outputNanos;
+
+  public ProtobufTimeUtilsTest(
+      long inputSeconds, int inputNanos, long outputSeconds, int outputNanos) {
+    this.inputSeconds = inputSeconds;
+    this.inputNanos = inputNanos;
+    this.outputSeconds = outputSeconds;
+    this.outputNanos = outputNanos;
+  }
+
+  @Test
+  public void toJavaDuration() {
+    final Duration actual = ProtobufTimeUtils.toJavaDuration(makeProto(inputSeconds, inputNanos));
+    final Duration expect = makeJava(outputSeconds, outputNanos);
+    assertEquals(expect, actual);
+  }
+
+  @Test
+  public void toProtoDuration() {
+    final com.google.protobuf.Duration actual =
+        ProtobufTimeUtils.toProtoDuration(makeJava(inputSeconds, inputNanos));
+    final com.google.protobuf.Duration expect = makeProto(outputSeconds, outputNanos);
+    assertEquals(expect, actual);
+  }
+
+  private static com.google.protobuf.Duration makeProto(long seconds, int nanos) {
+    return com.google.protobuf.Duration.newBuilder().setSeconds(seconds).setNanos(nanos).build();
+  }
+
+  private static Duration makeJava(long seconds, int nanos) {
+    final long saturatedSeconds = Math.min(MAX_SECONDS, Math.max(MIN_SECONDS, seconds));
+    return Duration.ofSeconds(saturatedSeconds, nanos);
+  }
+}