From 506c2b6861080f64bcbbb4d909291d8a252dc32b Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Tue, 16 Dec 2025 18:52:17 -0800 Subject: [PATCH] Restore the behavior for protobuf timestamp/duration field values to reside outside RFC3339 range Related: https://github.com/google/cel-java/issues/890 PiperOrigin-RevId: 845523949 --- .../java/dev/cel/common/internal/BUILD.bazel | 1 + .../cel/common/internal/ProtoTimeUtils.java | 24 ++++--- .../java/dev/cel/common/internal/BUILD.bazel | 1 + .../common/internal/ProtoTimeUtilsTest.java | 72 +++++++++++++++++++ runtime/src/test/resources/wrappers.baseline | 7 +- .../dev/cel/testing/BaseInterpreterTest.java | 6 ++ 6 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java diff --git a/common/src/main/java/dev/cel/common/internal/BUILD.bazel b/common/src/main/java/dev/cel/common/internal/BUILD.bazel index 690b1cc75..ca8f6375e 100644 --- a/common/src/main/java/dev/cel/common/internal/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/internal/BUILD.bazel @@ -429,6 +429,7 @@ cel_android_library( deps = [ "//common/annotations", "@maven//:com_google_errorprone_error_prone_annotations", + "@maven//:com_google_guava_guava", "@maven_android//:com_google_guava_guava", "@maven_android//:com_google_protobuf_protobuf_javalite", ], diff --git a/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java b/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java index cc705fccd..36671842d 100644 --- a/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java +++ b/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java @@ -18,6 +18,7 @@ import static com.google.common.math.LongMath.checkedMultiply; import static com.google.common.math.LongMath.checkedSubtract; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.protobuf.Duration; @@ -49,12 +50,16 @@ public final class ProtoTimeUtils { // Timestamp for "0001-01-01T00:00:00Z" - private static final long TIMESTAMP_SECONDS_MIN = -62135596800L; - + @VisibleForTesting + static final long TIMESTAMP_SECONDS_MIN = -62135596800L; // Timestamp for "9999-12-31T23:59:59Z" - private static final long TIMESTAMP_SECONDS_MAX = 253402300799L; - private static final long DURATION_SECONDS_MIN = -315576000000L; - private static final long DURATION_SECONDS_MAX = 315576000000L; + @VisibleForTesting + static final long TIMESTAMP_SECONDS_MAX = 253402300799L; + @VisibleForTesting + static final long DURATION_SECONDS_MIN = -315576000000L; + @VisibleForTesting + static final long DURATION_SECONDS_MAX = 315576000000L; + private static final int MILLIS_PER_SECOND = 1000; private static final int NANOS_PER_SECOND = 1000000000; @@ -344,7 +349,8 @@ public static Timestamp parse(String value) throws ParseException { } } try { - return normalizedTimestamp(seconds, nanos); + Timestamp timestamp = normalizedTimestamp(seconds, nanos); + return checkValid(timestamp); } catch (IllegalArgumentException e) { ParseException ex = new ParseException( @@ -532,8 +538,7 @@ private static Timestamp normalizedTimestamp(long seconds, int nanos) { nanos = nanos + NANOS_PER_SECOND; // no overflow since nanos is negative (and we're adding) seconds = checkedSubtract(seconds, 1); } - Timestamp timestamp = Timestamp.newBuilder().setSeconds(seconds).setNanos(nanos).build(); - return checkValid(timestamp); + return Timestamp.newBuilder().setSeconds(seconds).setNanos(nanos).build(); } private static Duration normalizedDuration(long seconds, int nanos) { @@ -549,8 +554,7 @@ private static Duration normalizedDuration(long seconds, int nanos) { nanos -= NANOS_PER_SECOND; // no overflow since nanos is positive (and we're subtracting) seconds++; // no overflow since seconds is negative (and we're incrementing) } - Duration duration = Duration.newBuilder().setSeconds(seconds).setNanos(nanos).build(); - return checkValid(duration); + return Duration.newBuilder().setSeconds(seconds).setNanos(nanos).build(); } private static String formatNanos(int nanos) { diff --git a/common/src/test/java/dev/cel/common/internal/BUILD.bazel b/common/src/test/java/dev/cel/common/internal/BUILD.bazel index 295c48a6e..33127ce16 100644 --- a/common/src/test/java/dev/cel/common/internal/BUILD.bazel +++ b/common/src/test/java/dev/cel/common/internal/BUILD.bazel @@ -28,6 +28,7 @@ java_library( "//common/internal:errors", "//common/internal:proto_equality", "//common/internal:proto_message_factory", + "//common/internal:proto_time_utils", "//common/internal:well_known_proto", "//common/src/test/resources:default_instance_message_test_protos_java_proto", "//common/src/test/resources:service_conflicting_name_java_proto", diff --git a/common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java b/common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java new file mode 100644 index 000000000..31ee9a9eb --- /dev/null +++ b/common/src/test/java/dev/cel/common/internal/ProtoTimeUtilsTest.java @@ -0,0 +1,72 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://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 dev.cel.common.internal; + +import static com.google.common.truth.Truth.assertThat; +import static dev.cel.common.internal.ProtoTimeUtils.DURATION_SECONDS_MAX; +import static dev.cel.common.internal.ProtoTimeUtils.DURATION_SECONDS_MIN; + +import com.google.protobuf.Duration; +import com.google.protobuf.Timestamp; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; +import java.time.Instant; +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(TestParameterInjector.class) +public class ProtoTimeUtilsTest { + + @Test + public void toJavaInstant_overRfc3339Range() { + Timestamp ts = + Timestamp.newBuilder().setSeconds(ProtoTimeUtils.TIMESTAMP_SECONDS_MAX + 1).build(); + + Instant instant = ProtoTimeUtils.toJavaInstant(ts); + + assertThat(instant).isEqualTo(Instant.ofEpochSecond(ProtoTimeUtils.TIMESTAMP_SECONDS_MAX + 1)); + } + + @Test + public void toJavaInstant_underRfc3339Range() { + Timestamp ts = + Timestamp.newBuilder().setSeconds(ProtoTimeUtils.TIMESTAMP_SECONDS_MIN - 1).build(); + + Instant instant = ProtoTimeUtils.toJavaInstant(ts); + + assertThat(instant).isEqualTo(Instant.ofEpochSecond(ProtoTimeUtils.TIMESTAMP_SECONDS_MIN - 1)); + } + + @Test + public void toJavaDuration_overRfc3339Range() { + Duration d = Duration.newBuilder() + .setSeconds(DURATION_SECONDS_MAX + 1) + .build(); + + java.time.Duration duration = ProtoTimeUtils.toJavaDuration(d); + + assertThat(duration).isEqualTo(java.time.Duration.ofSeconds(DURATION_SECONDS_MAX + 1)); + } + + @Test + public void toJavaDuration_underRfc3339Range() { + Duration d = Duration.newBuilder() + .setSeconds(DURATION_SECONDS_MIN - 1) + .build(); + + java.time.Duration duration = ProtoTimeUtils.toJavaDuration(d); + + assertThat(duration).isEqualTo(java.time.Duration.ofSeconds(DURATION_SECONDS_MIN - 1)); + } +} diff --git a/runtime/src/test/resources/wrappers.baseline b/runtime/src/test/resources/wrappers.baseline index 4a188e484..fc5d93654 100644 --- a/runtime/src/test/resources/wrappers.baseline +++ b/runtime/src/test/resources/wrappers.baseline @@ -83,4 +83,9 @@ declare dyn_var { } =====> bindings: {dyn_var=NULL_VALUE} -result: NULL_VALUE \ No newline at end of file +result: NULL_VALUE + +Source: google.protobuf.Timestamp{ seconds: 253402300800 } +=====> +bindings: {} +result: +10000-01-01T00:00:00Z \ No newline at end of file diff --git a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java index 096114757..b69e4fb7e 100644 --- a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java +++ b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java @@ -2005,6 +2005,12 @@ public void wrappers() throws Exception { declareVariable("dyn_var", SimpleType.DYN); source = "dyn_var"; runTest(ImmutableMap.of("dyn_var", NullValue.NULL_VALUE)); + + clearAllDeclarations(); + // Currently allowed, but will be an error + // See https://github.com/google/cel-spec/pull/501 + source = "google.protobuf.Timestamp{ seconds: 253402300800 }"; + runTest(); } @Test