String conversion functions for timestamp and duration.

The functions `string(timestamp)` and `string(duration)` will produce formatted strings which match those found in string-encoded protobuf values for `google.protobuf.Timestamp` and `google.protobuf.Duration` respectively. PiperOrigin-RevId: 359302330
google · Mar 31, 2021 · c190825 · c190825
1 parent 2977932
commit c190825
Show file tree

Hide file tree

Showing 17 changed files with 253 additions and 46 deletions.
diff --git a/conformance/BUILD b/conformance/BUILD
@@ -83,9 +83,6 @@ cc_binary(
             "--pipe",
             # TODO(issues/93): Deprecate Duration.getMilliseconds.
             "--skip_test=timestamps/duration_converters/get_milliseconds",
-            # TODO(issues/94): Missing timestamp conversion functions (type / string)
-            "--skip_test=timestamps/timestamp_conversions/toString_timestamp",
-            "--skip_test=timestamps/duration_conversions/toString_duration",
             # TODO(issues/81): Conversion functions for int(), uint() which can be
             # uncommented when the spec changes to truncation rather than rounding.
             "--skip_test=conversions/int/double_nearest,double_nearest_neg,double_half_away_neg,double_half_away_pos",
@@ -161,6 +158,7 @@ sh_test(
         "$(location @com_google_cel_spec//tests/simple:simple_test)",
         "--server=$(location :server)",
         "--skip_check",
+        "--skip_test=dynamic/list/var",
         "--pipe",
     ] + ["$(location " + test + ")" for test in DASHBOARD_TESTS],
     data = [

diff --git a/conformance/server.cc b/conformance/server.cc
@@ -181,7 +181,7 @@ int RunServer(bool optimize) {
   // Implementation of a simple pipe protocol:
   // INPUT LINE 1: parse/check/eval
   // INPUT LINE 2: JSON of the corresponding request protobuf
-  // OUTPUT LINE 1: JSON of the coressponding response protobuf
+  // OUTPUT LINE 1: JSON of the corresponding response protobuf
   while (true) {
     std::string cmd, input, output;
     std::getline(std::cin, cmd);

diff --git a/eval/public/BUILD b/eval/public/BUILD
@@ -182,9 +182,11 @@ cc_library(
         ":cel_options",
         "//base:unilib",
         "//eval/public/containers:container_backed_list_impl",
+        "//internal:proto_util",
         "@com_google_absl//absl/numeric:int128",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/time",
         "@com_google_protobuf//:protobuf",
         "@com_googlesource_code_re2//:re2",
     ],
@@ -490,8 +492,10 @@ cc_test(
         ":cel_builtins",
         ":cel_expr_builder_factory",
         ":cel_function_registry",
+        ":cel_value",
         "//base:status_macros",
         "//eval/public/structs:cel_proto_wrapper",
+        "//internal:proto_util",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
         "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto",

diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc
@@ -11,11 +11,13 @@
 #include "absl/strings/numbers.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_replace.h"
+#include "absl/time/time.h"
 #include "eval/public/cel_builtins.h"
 #include "eval/public/cel_function_adapter.h"
 #include "eval/public/cel_function_registry.h"
 #include "eval/public/cel_options.h"
 #include "eval/public/containers/container_backed_list_impl.h"
+#include "internal/proto_util.h"
 #include "re2/re2.h"
 #include "base/unilib.h"
 
@@ -1200,6 +1202,38 @@ absl::Status RegisterStringConversionFunctions(
       registry);
   if (!status.ok()) return status;
 
+  // duration -> string
+  status = FunctionAdapter<CelValue, absl::Duration>::CreateAndRegister(
+      builtin::kString, false,
+      [](Arena* arena, absl::Duration value) -> CelValue {
+        google::protobuf::Duration d;
+        auto status = google::api::expr::internal::EncodeDuration(value, &d);
+        if (!status.ok()) {
+          return CreateErrorValue(arena, status.message(), status.code());
+        }
+        return CelValue::CreateString(
+            CelValue::StringHolder(Arena::Create<std::string>(
+                arena, google::protobuf::util::TimeUtil::ToString(d))));
+      },
+      registry);
+  if (!status.ok()) return status;
+
+  // timestamp -> string
+  status = FunctionAdapter<CelValue, absl::Time>::CreateAndRegister(
+      builtin::kString, false,
+      [](Arena* arena, absl::Time value) -> CelValue {
+        google::protobuf::Timestamp ts;
+        auto status = google::api::expr::internal::EncodeTime(value, &ts);
+        if (!status.ok()) {
+          return CreateErrorValue(arena, status.message(), status.code());
+        }
+        return CelValue::CreateString(
+            CelValue::StringHolder(Arena::Create<std::string>(
+                arena, google::protobuf::util::TimeUtil::ToString(ts))));
+      },
+      registry);
+  if (!status.ok()) return status;
+
   return absl::OkStatus();
 }
 

diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc
@@ -9,7 +9,9 @@
 #include "eval/public/cel_builtins.h"
 #include "eval/public/cel_expr_builder_factory.h"
 #include "eval/public/cel_function_registry.h"
+#include "eval/public/cel_value.h"
 #include "eval/public/structs/cel_proto_wrapper.h"
+#include "internal/proto_util.h"
 #include "base/status_macros.h"
 
 namespace google {
@@ -28,6 +30,9 @@ using google::api::expr::v1alpha1::SourceInfo;
 using google::protobuf::Arena;
 using google::protobuf::util::TimeUtil;
 
+using ::google::api::expr::internal::MakeGoogleApiDurationMax;
+using ::google::api::expr::internal::MakeGoogleApiDurationMin;
+using ::google::api::expr::internal::MakeGoogleApiTimeMin;
 using testing::Eq;
 
 class BuiltinsTest : public ::testing::Test {
@@ -132,6 +137,18 @@ class BuiltinsTest : public ::testing::Test {
         << operation << " for " << CelValue::TypeName(ref.type());
   }
 
+  // Helper method. Looks up in registry and tests Type conversions.
+  void TestTypeConverts(absl::string_view operation, const CelValue& ref,
+                        CelValue::StringHolder result) {
+    CelValue result_value;
+
+    ASSERT_NO_FATAL_FAILURE(PerformRun(operation, {}, {ref}, &result_value));
+
+    ASSERT_EQ(result_value.IsString(), true);
+    ASSERT_EQ(result_value.StringOrDie().value(), result.value())
+        << operation << " for " << CelValue::TypeName(ref.type());
+  }
+
   void TestTypeConverts(absl::string_view operation, const CelValue& ref,
                         double result) {
     CelValue result_value;
@@ -565,6 +582,10 @@ TEST_F(BuiltinsTest, TestDurationFunctions) {
   TestFunctions(builtin::kMilliseconds, CelProtoWrapper::CreateDuration(&ref),
                 11L);
 
+  std::string result = "93541.011s";
+  TestTypeConverts(builtin::kString, CelProtoWrapper::CreateDuration(&ref),
+                   CelValue::StringHolder(&result));
+
   ref.set_seconds(-93541L);
   ref.set_nanos(-11000000L);
 
@@ -575,6 +596,16 @@ TEST_F(BuiltinsTest, TestDurationFunctions) {
                 -93541L);
   TestFunctions(builtin::kMilliseconds, CelProtoWrapper::CreateDuration(&ref),
                 -11L);
+
+  result = "-93541.011s";
+  TestTypeConverts(builtin::kString, CelProtoWrapper::CreateDuration(&ref),
+                   CelValue::StringHolder(&result));
+
+  absl::Duration d = MakeGoogleApiDurationMin() + absl::Seconds(-1);
+  TestTypeConversionError(builtin::kString, CelValue::CreateDuration(d));
+
+  d = MakeGoogleApiDurationMax() + absl::Seconds(1);
+  TestTypeConversionError(builtin::kString, CelValue::CreateDuration(d));
 }
 
 // Test functions for Timestamp
@@ -598,11 +629,19 @@ TEST_F(BuiltinsTest, TestTimestampFunctions) {
   TestFunctions(builtin::kMilliseconds, CelProtoWrapper::CreateTimestamp(&ref),
                 11L);
 
+  std::string result = "1970-01-01T00:00:01.011Z";
+  TestTypeConverts(builtin::kString, CelProtoWrapper::CreateTimestamp(&ref),
+                   CelValue::StringHolder(&result));
+
   ref.set_seconds(259200L);
   ref.set_nanos(0L);
   TestFunctions(builtin::kDayOfWeek, CelProtoWrapper::CreateTimestamp(&ref),
                 0L);
 
+  result = "1970-01-04T00:00:00Z";
+  TestTypeConverts(builtin::kString, CelProtoWrapper::CreateTimestamp(&ref),
+                   CelValue::StringHolder(&result));
+
   // Test timestamp functions w/ IANA timezone
   ref.set_seconds(1L);
   ref.set_nanos(11000000L);
@@ -702,6 +741,10 @@ TEST_F(BuiltinsTest, TestTimestampFunctions) {
   TestFunctions(builtin::kSeconds, CelProtoWrapper::CreateTimestamp(&ref), 59L);
   TestFunctions(builtin::kDayOfWeek, CelProtoWrapper::CreateTimestamp(&ref),
                 3L);
+
+  TestTypeConversionError(
+      builtin::kString,
+      CelValue::CreateTimestamp(MakeGoogleApiTimeMin() + absl::Seconds(-1)));
 }
 
 TEST_F(BuiltinsTest, TestBytesConversions_bytes) {

diff --git a/eval/public/containers/BUILD b/eval/public/containers/BUILD
@@ -121,9 +121,22 @@ cc_test(
     ],
     deps = [
         ":field_backed_map_impl",
-        "//eval/eval:evaluator_core",
         "//eval/testutil:test_message_cc_proto",
         "@com_google_absl//absl/strings",
         "@com_google_googletest//:gtest_main",
     ],
 )
+
+cc_test(
+    name = "field_access_test",
+    srcs = ["field_access_test.cc"],
+    deps = [
+        ":field_access",
+        "//internal:proto_util",
+        "@com_google_absl//absl/status",
+        "@com_google_absl//absl/time",
+        "@com_google_cel_spec//proto/test/v1/proto3:test_all_types_cc_proto",
+        "@com_google_googletest//:gtest_main",
+        "@com_google_protobuf//:protobuf",
+    ],
+)
diff --git a/eval/public/containers/field_access.cc b/eval/public/containers/field_access.cc
@@ -471,7 +471,10 @@ class FieldSetter {
       return false;
     }
     google::protobuf::Duration duration;
-    google::api::expr::internal::EncodeDuration(d, &duration);
+    auto status = google::api::expr::internal::EncodeDuration(d, &duration);
+    if (!status.ok()) {
+      return false;
+    }
     static_cast<const Derived*>(this)->SetMessage(&duration);
     return true;
   }
@@ -483,7 +486,10 @@ class FieldSetter {
       return false;
     }
     google::protobuf::Timestamp timestamp;
-    google::api::expr::internal::EncodeTime(t, &timestamp);
+    auto status = google::api::expr::internal::EncodeTime(t, &timestamp);
+    if (!status.ok()) {
+      return false;
+    }
     static_cast<const Derived*>(this)->SetMessage(&timestamp);
     return true;
   }

diff --git a/eval/public/containers/field_access_test.cc b/eval/public/containers/field_access_test.cc
@@ -0,0 +1,86 @@
+#include "eval/public/containers/field_access.h"
+
+#include "google/protobuf/message.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "absl/status/status.h"
+#include "absl/time/time.h"
+#include "internal/proto_util.h"
+#include "proto/test/v1/proto3/test_all_types.pb.h"
+
+namespace google {
+namespace api {
+namespace expr {
+namespace runtime {
+
+namespace {
+
+using google::api::expr::internal::MakeGoogleApiDurationMax;
+using google::api::expr::internal::MakeGoogleApiTimeMax;
+using google::protobuf::FieldDescriptor;
+using test::v1::proto3::TestAllTypes;
+
+TEST(FieldAccessTest, SetDuration) {
+  TestAllTypes msg;
+  const FieldDescriptor* field =
+      TestAllTypes::descriptor()->FindFieldByName("single_duration");
+  auto status = SetValueToSingleField(
+      CelValue::CreateDuration(MakeGoogleApiDurationMax()), field, &msg);
+  EXPECT_TRUE(status.ok());
+}
+
+TEST(FieldAccessTest, SetDurationBadDuration) {
+  TestAllTypes msg;
+  const FieldDescriptor* field =
+      TestAllTypes::descriptor()->FindFieldByName("single_duration");
+  auto status = SetValueToSingleField(
+      CelValue::CreateDuration(MakeGoogleApiDurationMax() + absl::Seconds(1)),
+      field, &msg);
+  EXPECT_FALSE(status.ok());
+  EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument);
+}
+
+TEST(FieldAccessTest, SetDurationBadInputType) {
+  TestAllTypes msg;
+  const FieldDescriptor* field =
+      TestAllTypes::descriptor()->FindFieldByName("single_duration");
+  auto status = SetValueToSingleField(CelValue::CreateInt64(1), field, &msg);
+  EXPECT_FALSE(status.ok());
+  EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument);
+}
+
+TEST(FieldAccessTest, SetTimestamp) {
+  TestAllTypes msg;
+  const FieldDescriptor* field =
+      TestAllTypes::descriptor()->FindFieldByName("single_timestamp");
+  auto status = SetValueToSingleField(
+      CelValue::CreateTimestamp(MakeGoogleApiTimeMax()), field, &msg);
+  EXPECT_TRUE(status.ok());
+}
+
+TEST(FieldAccessTest, SetTimestampBadTime) {
+  TestAllTypes msg;
+  const FieldDescriptor* field =
+      TestAllTypes::descriptor()->FindFieldByName("single_timestamp");
+  auto status = SetValueToSingleField(
+      CelValue::CreateTimestamp(MakeGoogleApiTimeMax() + absl::Seconds(1)),
+      field, &msg);
+  EXPECT_FALSE(status.ok());
+  EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument);
+}
+
+TEST(FieldAccessTest, SetTimestampBadInputType) {
+  TestAllTypes msg;
+  const FieldDescriptor* field =
+      TestAllTypes::descriptor()->FindFieldByName("single_timestamp");
+  auto status = SetValueToSingleField(CelValue::CreateInt64(1), field, &msg);
+  EXPECT_FALSE(status.ok());
+  EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument);
+}
+
+}  // namespace
+
+}  // namespace runtime
+}  // namespace expr
+}  // namespace api
+}  // namespace google
diff --git a/eval/public/containers/field_backed_map_impl_test.cc b/eval/public/containers/field_backed_map_impl_test.cc
@@ -103,10 +103,6 @@ TEST(FieldBackedMapImplTest, EmptySizeTest) {
   google::protobuf::Arena arena;
 
   auto cel_map = CreateMap(&message, "string_int32_map", &arena);
-
-  std::string test0 = "test0";
-  std::string test1 = "test1";
-
   EXPECT_EQ(cel_map->size(), 0);
 }
 

diff --git a/eval/public/structs/cel_proto_wrapper_test.cc b/eval/public/structs/cel_proto_wrapper_test.cc
@@ -80,7 +80,8 @@ TEST(CelProtoWrapperTest, TestDuration) {
   //  CelValue value = CelValue::CreateString("test");
   EXPECT_TRUE(value.IsDuration());
   Duration out;
-  expr::internal::EncodeDuration(value.DurationOrDie(), &out);
+  auto status = expr::internal::EncodeDuration(value.DurationOrDie(), &out);
+  EXPECT_TRUE(status.ok());
   EXPECT_THAT(out, testutil::EqualsProto(msg_duration));
 }
 
@@ -102,7 +103,8 @@ TEST(CelProtoWrapperTest, TestTimestamp) {
   //  CelValue value = CelValue::CreateString("test");
   EXPECT_TRUE(value.IsTimestamp());
   Timestamp out;
-  expr::internal::EncodeTime(value.TimestampOrDie(), &out);
+  auto status = expr::internal::EncodeTime(value.TimestampOrDie(), &out);
+  EXPECT_TRUE(status.ok());
   EXPECT_THAT(out, testutil::EqualsProto(msg_timestamp));
 }
 

diff --git a/eval/public/transform_utility.cc b/eval/public/transform_utility.cc
@@ -44,13 +44,21 @@ absl::Status CelValueToValue(const CelValue& value, Value* result) {
       break;
     case CelValue::Type::kDuration: {
       google::protobuf::Duration duration;
-      expr::internal::EncodeDuration(value.DurationOrDie(), &duration);
+      auto status =
+          expr::internal::EncodeDuration(value.DurationOrDie(), &duration);
+      if (!status.ok()) {
+        return status;
+      }
       result->mutable_object_value()->PackFrom(duration);
       break;
     }
     case CelValue::Type::kTimestamp: {
       google::protobuf::Timestamp timestamp;
-      expr::internal::EncodeTime(value.TimestampOrDie(), &timestamp);
+      auto status =
+          expr::internal::EncodeTime(value.TimestampOrDie(), &timestamp);
+      if (!status.ok()) {
+        return status;
+      }
       result->mutable_object_value()->PackFrom(timestamp);
       break;
     }