From d9d502790631159bb976cfcf7a33e95559d4960c Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Fri, 28 Aug 2026 14:30:54 -0700 Subject: [PATCH] Update field access behavior to tolerate out of range timestamps. Preserves existing behavior of accepting an out of range value if it comes from proto. PiperOrigin-RevId: 972792517 --- common/legacy_value.h | 8 -- common/value.cc | 7 +- common/values/duration_value.h | 6 ++ common/values/struct_value_builder.cc | 22 ++--- common/values/timestamp_value.h | 6 ++ internal/message_equality.cc | 4 +- internal/well_known_types.cc | 6 +- internal/well_known_types_test.cc | 93 +++++++++++++++++-- runtime/internal/function_adapter.h | 4 +- runtime/standard/type_conversion_functions.cc | 6 +- 10 files changed, 123 insertions(+), 39 deletions(-) diff --git a/common/legacy_value.h b/common/legacy_value.h index e71eb85a7..8fea22ec0 100644 --- a/common/legacy_value.h +++ b/common/legacy_value.h @@ -121,14 +121,6 @@ inline MapValue CreateLegacyMapValue( return common_internal::LegacyMapValue(value); } -inline Value CreateDurationValue(absl::Duration value, bool unchecked = false) { - return DurationValue{value}; -} - -inline TimestampValue CreateTimestampValue(absl::Time value) { - return TimestampValue{value}; -} - Value LegacyValueToModernValueOrDie( google::protobuf::Arena* arena, const google::api::expr::runtime::CelValue& value, bool unchecked = false); diff --git a/common/value.cc b/common/value.cc index e749a16c6..a2ffea620 100644 --- a/common/value.cc +++ b/common/value.cc @@ -1146,10 +1146,13 @@ Value VistWellKnownTypeValue(float value) { return DoubleValue(value); } Value VistWellKnownTypeValue(double value) { return DoubleValue(value); } Value VistWellKnownTypeValue(absl::Duration value) { - return DurationValue(value); + // Tolerate out-of-range values. + return UnsafeDurationValue(value); } -Value VistWellKnownTypeValue(absl::Time value) { return TimestampValue(value); } +Value VistWellKnownTypeValue(absl::Time value) { + return UnsafeTimestampValue(value); +} struct OwningWellKnownTypesValueVisitor { google::protobuf::Arena* absl_nullable arena; diff --git a/common/values/duration_value.h b/common/values/duration_value.h index 1b2468b60..0c934f097 100644 --- a/common/values/duration_value.h +++ b/common/values/duration_value.h @@ -51,6 +51,12 @@ class DurationValue final : private common_internal::ValueMixin { public: static constexpr ValueKind kKind = ValueKind::kDuration; + // Constructs a `DurationValue` from an `absl::Duration`. + // + // DCHECK-fails if the value is not in the supported range. + // + // Prefer using `SafeDurationValue` or `UnsafeDurationValue` if the caller + // has already validated the value. explicit DurationValue(absl::Duration value) noexcept : DurationValue(absl::in_place, value) { ABSL_DCHECK_OK(internal::ValidateDuration(value)); diff --git a/common/values/struct_value_builder.cc b/common/values/struct_value_builder.cc index d5206acbf..f43369e33 100644 --- a/common/values/struct_value_builder.cc +++ b/common/values/struct_value_builder.cc @@ -254,8 +254,8 @@ absl::StatusOr> ProtoMessageFromValueImpl( if (auto duration_value = value.AsDuration(); duration_value) { CEL_RETURN_IF_ERROR( well_known_types->Duration().Initialize(message->GetDescriptor())); - CEL_RETURN_IF_ERROR(well_known_types->Duration().SetFromAbslDuration( - message, duration_value->NativeValue())); + well_known_types->Duration().UnsafeSetFromAbslDuration( + message, duration_value->NativeValue()); return std::nullopt; } return TypeConversionError(value.GetTypeName(), to_desc->full_name()); @@ -264,8 +264,8 @@ absl::StatusOr> ProtoMessageFromValueImpl( if (auto timestamp_value = value.AsTimestamp(); timestamp_value) { CEL_RETURN_IF_ERROR( well_known_types->Timestamp().Initialize(message->GetDescriptor())); - CEL_RETURN_IF_ERROR(well_known_types->Timestamp().SetFromAbslTime( - message, timestamp_value->NativeValue())); + well_known_types->Timestamp().UnsafeSetFromAbslTime( + message, timestamp_value->NativeValue()); return std::nullopt; } return TypeConversionError(value.GetTypeName(), to_desc->full_name()); @@ -1304,11 +1304,11 @@ class MessageValueBuilderImpl { if (auto duration_value = value.AsDuration(); duration_value) { CEL_RETURN_IF_ERROR(well_known_types_.Duration().Initialize( field->message_type())); - CEL_RETURN_IF_ERROR( - well_known_types_.Duration().SetFromAbslDuration( - reflection_->MutableMessage(message_, field, - message_factory_), - duration_value->NativeValue())); + + well_known_types_.Duration().UnsafeSetFromAbslDuration( + reflection_->MutableMessage(message_, field, + message_factory_), + duration_value->NativeValue()); return std::nullopt; } return TypeConversionError(value.GetTypeName(), @@ -1322,10 +1322,10 @@ class MessageValueBuilderImpl { if (auto timestamp_value = value.AsTimestamp(); timestamp_value) { CEL_RETURN_IF_ERROR(well_known_types_.Timestamp().Initialize( field->message_type())); - CEL_RETURN_IF_ERROR(well_known_types_.Timestamp().SetFromAbslTime( + well_known_types_.Timestamp().UnsafeSetFromAbslTime( reflection_->MutableMessage(message_, field, message_factory_), - timestamp_value->NativeValue())); + timestamp_value->NativeValue()); return std::nullopt; } return TypeConversionError(value.GetTypeName(), diff --git a/common/values/timestamp_value.h b/common/values/timestamp_value.h index acc202300..87eae0ed0 100644 --- a/common/values/timestamp_value.h +++ b/common/values/timestamp_value.h @@ -52,6 +52,12 @@ class TimestampValue final public: static constexpr ValueKind kKind = ValueKind::kTimestamp; + // Constructs a `TimestampValue` from an `absl::Time`. + // + // DCHECK-fails if the value is not in the supported range. + // + // Prefer using `SafeTimestampValue` or `UnsafeTimestampValue` if the caller + // has already validated the value. explicit TimestampValue(absl::Time value) noexcept : TimestampValue(absl::in_place, value) { ABSL_DCHECK_OK(internal::ValidateTimestamp(value)); diff --git a/internal/message_equality.cc b/internal/message_equality.cc index 33ef78089..34080e6b1 100644 --- a/internal/message_equality.cc +++ b/internal/message_equality.cc @@ -380,11 +380,11 @@ absl::StatusOr AsEquatableValue( case Descriptor::WELLKNOWNTYPE_DURATION: CEL_RETURN_IF_ERROR( reflection.duration_reflection.Initialize(descriptor)); - return reflection.duration_reflection.ToAbslDuration(message); + return reflection.duration_reflection.UnsafeToAbslDuration(message); case Descriptor::WELLKNOWNTYPE_TIMESTAMP: CEL_RETURN_IF_ERROR( reflection.timestamp_reflection.Initialize(descriptor)); - return reflection.timestamp_reflection.ToAbslTime(message); + return reflection.timestamp_reflection.UnsafeToAbslTime(message); case Descriptor::WELLKNOWNTYPE_ANY: return EquatableAny(message); default: diff --git a/internal/well_known_types.cc b/internal/well_known_types.cc index 02e50c3e3..175f978d4 100644 --- a/internal/well_known_types.cc +++ b/internal/well_known_types.cc @@ -2114,13 +2114,15 @@ absl::StatusOr AdaptFromMessage( case Descriptor::WELLKNOWNTYPE_ANY: // This is unreachable, as AdaptAny() above recursively unpacks. ABSL_UNREACHABLE(); + // Don't check that time values are in range on field access. Assume + // error will propagate if they are used in any arithmetic. case Descriptor::WELLKNOWNTYPE_DURATION: { CEL_ASSIGN_OR_RETURN(auto reflection, GetDurationReflection(descriptor)); - return reflection.ToAbslDuration(*to_adapt); + return reflection.UnsafeToAbslDuration(*to_adapt); } case Descriptor::WELLKNOWNTYPE_TIMESTAMP: { CEL_ASSIGN_OR_RETURN(auto reflection, GetTimestampReflection(descriptor)); - return reflection.ToAbslTime(*to_adapt); + return reflection.UnsafeToAbslTime(*to_adapt); } case Descriptor::WELLKNOWNTYPE_VALUE: { CEL_ASSIGN_OR_RETURN(auto reflection, GetValueReflection(descriptor)); diff --git a/internal/well_known_types_test.cc b/internal/well_known_types_test.cc index afc8ce396..5ffc06ee9 100644 --- a/internal/well_known_types_test.cc +++ b/internal/well_known_types_test.cc @@ -341,6 +341,44 @@ TEST_F(ReflectionTest, Duration_Dynamic) { StatusIs(absl::StatusCode::kInvalidArgument)); } +TEST_F(ReflectionTest, Duration_ToAbslDuration) { + auto* value = MakeDynamic(); + ASSERT_OK_AND_ASSIGN( + auto reflection, + GetDurationReflection(ABSL_DIE_IF_NULL(value->GetDescriptor()))); + + reflection.SetSeconds(value, 1); + reflection.SetNanos(value, 1); + EXPECT_THAT(reflection.ToAbslDuration(*value), + IsOkAndHolds(absl::Seconds(1) + absl::Nanoseconds(1))); + EXPECT_EQ(reflection.UnsafeToAbslDuration(*value), + absl::Seconds(1) + absl::Nanoseconds(1)); + + reflection.SetSeconds(value, 0x7fffffffffffffff); + reflection.SetNanos(value, 1); + EXPECT_THAT(reflection.ToAbslDuration(*value), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("invalid duration seconds: "))); + EXPECT_EQ(reflection.UnsafeToAbslDuration(*value), + absl::Seconds(0x7fffffffffffffff) + absl::Nanoseconds(1)); + + reflection.SetSeconds(value, 1); + reflection.SetNanos(value, 0x7fffffff); + EXPECT_THAT(reflection.ToAbslDuration(*value), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("invalid duration nanoseconds: "))); + EXPECT_EQ(reflection.UnsafeToAbslDuration(*value), + absl::Seconds(1) + absl::Nanoseconds(0x7fffffff)); + + reflection.SetSeconds(value, -1); + reflection.SetNanos(value, 1); + EXPECT_THAT(reflection.ToAbslDuration(*value), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("duration sign mismatch: "))); + EXPECT_EQ(reflection.UnsafeToAbslDuration(*value), + absl::Seconds(-1) + absl::Nanoseconds(1)); +} + TEST_F(ReflectionTest, Timestamp_Generated) { auto* value = MakeGenerated(); EXPECT_EQ(TimestampReflection::GetSeconds(*value), 0); @@ -389,6 +427,39 @@ TEST_F(ReflectionTest, Timestamp_Dynamic) { StatusIs(absl::StatusCode::kInvalidArgument)); } +TEST_F(ReflectionTest, Timestamp_ToAbslTime) { + auto* value = MakeDynamic(); + ASSERT_OK_AND_ASSIGN( + auto reflection, + GetTimestampReflection(ABSL_DIE_IF_NULL(value->GetDescriptor()))); + + reflection.SetSeconds(value, 1); + reflection.SetNanos(value, 1); + EXPECT_THAT(reflection.ToAbslTime(*value), + IsOkAndHolds(absl::UnixEpoch() + absl::Seconds(1) + + absl::Nanoseconds(1))); + EXPECT_EQ(reflection.UnsafeToAbslTime(*value), + absl::UnixEpoch() + absl::Seconds(1) + absl::Nanoseconds(1)); + + reflection.SetSeconds(value, 0x7fffffffffffffff); + reflection.SetNanos(value, 1); + EXPECT_THAT(reflection.ToAbslTime(*value), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("invalid timestamp seconds: "))); + EXPECT_EQ(reflection.UnsafeToAbslTime(*value), + absl::UnixEpoch() + absl::Seconds(0x7fffffffffffffff) + + absl::Nanoseconds(1)); + + reflection.SetSeconds(value, 1); + reflection.SetNanos(value, 0x7fffffff); + EXPECT_THAT(reflection.ToAbslTime(*value), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("invalid timestamp nanoseconds: "))); + EXPECT_EQ( + reflection.UnsafeToAbslTime(*value), + absl::UnixEpoch() + absl::Seconds(1) + absl::Nanoseconds(0x7fffffff)); +} + TEST_F(ReflectionTest, Value_Generated) { auto* value = MakeGenerated(); EXPECT_EQ(ValueReflection::GetKindCase(*value), @@ -698,16 +769,16 @@ TEST_F(AdaptFromMessageTest, Duration_SecondsOutOfRange) { auto message = DynamicParseTextProto( R"pb(seconds: 0x7fffffffffffffff nanos: 1)pb"); EXPECT_THAT(AdaptFromMessage(*message), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("invalid duration seconds: "))); + IsOkAndHolds(VariantWith( + absl::Seconds(0x7fffffffffffffff) + absl::Nanoseconds(1)))); } TEST_F(AdaptFromMessageTest, Duration_NanosOutOfRange) { auto message = DynamicParseTextProto( R"pb(seconds: 1 nanos: 0x7fffffff)pb"); EXPECT_THAT(AdaptFromMessage(*message), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("invalid duration nanoseconds: "))); + IsOkAndHolds(VariantWith( + absl::Seconds(1) + absl::Nanoseconds(0x7fffffff)))); } TEST_F(AdaptFromMessageTest, Duration_SignMismatch) { @@ -715,8 +786,8 @@ TEST_F(AdaptFromMessageTest, Duration_SignMismatch) { DynamicParseTextProto(R"pb(seconds: -1 nanos: 1)pb"); EXPECT_THAT(AdaptFromMessage(*message), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("duration sign mismatch: "))); + IsOkAndHolds(VariantWith(absl::Seconds(-1) + + absl::Nanoseconds(1)))); } TEST_F(AdaptFromMessageTest, Timestamp) { @@ -733,16 +804,18 @@ TEST_F(AdaptFromMessageTest, Timestamp_SecondsOutOfRange) { auto message = DynamicParseTextProto( R"pb(seconds: 0x7fffffffffffffff nanos: 1)pb"); EXPECT_THAT(AdaptFromMessage(*message), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("invalid timestamp seconds: "))); + IsOkAndHolds(VariantWith( + absl::UnixEpoch() + absl::Seconds(0x7fffffffffffffff) + + absl::Nanoseconds(1)))); } TEST_F(AdaptFromMessageTest, Timestamp_NanosOutOfRange) { auto message = DynamicParseTextProto( R"pb(seconds: 1 nanos: 0x7fffffff)pb"); EXPECT_THAT(AdaptFromMessage(*message), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("invalid timestamp nanoseconds: "))); + IsOkAndHolds( + VariantWith(absl::UnixEpoch() + absl::Seconds(1) + + absl::Nanoseconds(0x7fffffff)))); } TEST_F(AdaptFromMessageTest, Value_NullValue) { diff --git a/runtime/internal/function_adapter.h b/runtime/internal/function_adapter.h index 9b191e577..1ff497486 100644 --- a/runtime/internal/function_adapter.h +++ b/runtime/internal/function_adapter.h @@ -200,13 +200,13 @@ struct AdaptedToValueVisitor { absl::StatusOr operator()(absl::Time in) { // Type matching may have already occurred. It's too late to change up the // type and return an error. - return TimestampValue(in); + return UnsafeTimestampValue(in); } absl::StatusOr operator()(absl::Duration in) { // Type matching may have already occurred. It's too late to change up the // type and return an error. - return DurationValue(in); + return UnsafeDurationValue(in); } absl::StatusOr operator()(Value in) { return in; } diff --git a/runtime/standard/type_conversion_functions.cc b/runtime/standard/type_conversion_functions.cc index 76e95751b..2400c8fdf 100644 --- a/runtime/standard/type_conversion_functions.cc +++ b/runtime/standard/type_conversion_functions.cc @@ -403,14 +403,16 @@ absl::Status RegisterTimeConversionFunctions(FunctionRegistry& registry, CEL_RETURN_IF_ERROR( (UnaryFunctionAdapter::RegisterGlobalOverload( cel::builtin::kTimestamp, - [](absl::Time value) -> Value { return TimestampValue(value); }, + [](absl::Time value) -> Value { return UnsafeTimestampValue(value); }, registry))); // duration -> duration CEL_RETURN_IF_ERROR( (UnaryFunctionAdapter::RegisterGlobalOverload( cel::builtin::kDuration, - [](absl::Duration value) -> Value { return DurationValue(value); }, + [](absl::Duration value) -> Value { + return UnsafeDurationValue(value); + }, registry))); // timestamp() conversion from string.