Skip to content

Commit 474f1af

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
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: 974144329
1 parent 2d4e7a2 commit 474f1af

10 files changed

Lines changed: 123 additions & 39 deletions

‎common/legacy_value.h‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -121,14 +121,6 @@ inline MapValue CreateLegacyMapValue(
121121
return common_internal::LegacyMapValue(value);
122122
}
123123

124-
inline Value CreateDurationValue(absl::Duration value, bool unchecked = false) {
125-
return DurationValue{value};
126-
}
127-
128-
inline TimestampValue CreateTimestampValue(absl::Time value) {
129-
return TimestampValue{value};
130-
}
131-
132124
Value LegacyValueToModernValueOrDie(
133125
google::protobuf::Arena* arena, const google::api::expr::runtime::CelValue& value,
134126
bool unchecked = false);

‎common/value.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1146,10 +1146,13 @@ Value VistWellKnownTypeValue(float value) { return DoubleValue(value); }
11461146
Value VistWellKnownTypeValue(double value) { return DoubleValue(value); }
11471147

11481148
Value VistWellKnownTypeValue(absl::Duration value) {
1149-
return DurationValue(value);
1149+
// Tolerate out-of-range values.
1150+
return UnsafeDurationValue(value);
11501151
}
11511152

1152-
Value VistWellKnownTypeValue(absl::Time value) { return TimestampValue(value); }
1153+
Value VistWellKnownTypeValue(absl::Time value) {
1154+
return UnsafeTimestampValue(value);
1155+
}
11531156

11541157
struct OwningWellKnownTypesValueVisitor {
11551158
google::protobuf::Arena* absl_nullable arena;

‎common/values/duration_value.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,12 @@ class DurationValue final : private common_internal::ValueMixin<DurationValue> {
5151
public:
5252
static constexpr ValueKind kKind = ValueKind::kDuration;
5353

54+
// Constructs a `DurationValue` from an `absl::Duration`.
55+
//
56+
// DCHECK-fails if the value is not in the supported range.
57+
//
58+
// Prefer using `SafeDurationValue` or `UnsafeDurationValue` if the caller
59+
// has already validated the value.
5460
explicit DurationValue(absl::Duration value) noexcept
5561
: DurationValue(absl::in_place, value) {
5662
ABSL_DCHECK_OK(internal::ValidateDuration(value));

‎common/values/struct_value_builder.cc‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,8 @@ absl::StatusOr<absl::optional<ErrorValue>> ProtoMessageFromValueImpl(
254254
if (auto duration_value = value.AsDuration(); duration_value) {
255255
CEL_RETURN_IF_ERROR(
256256
well_known_types->Duration().Initialize(message->GetDescriptor()));
257-
CEL_RETURN_IF_ERROR(well_known_types->Duration().SetFromAbslDuration(
258-
message, duration_value->NativeValue()));
257+
well_known_types->Duration().UnsafeSetFromAbslDuration(
258+
message, duration_value->NativeValue());
259259
return std::nullopt;
260260
}
261261
return TypeConversionError(value.GetTypeName(), to_desc->full_name());
@@ -264,8 +264,8 @@ absl::StatusOr<absl::optional<ErrorValue>> ProtoMessageFromValueImpl(
264264
if (auto timestamp_value = value.AsTimestamp(); timestamp_value) {
265265
CEL_RETURN_IF_ERROR(
266266
well_known_types->Timestamp().Initialize(message->GetDescriptor()));
267-
CEL_RETURN_IF_ERROR(well_known_types->Timestamp().SetFromAbslTime(
268-
message, timestamp_value->NativeValue()));
267+
well_known_types->Timestamp().UnsafeSetFromAbslTime(
268+
message, timestamp_value->NativeValue());
269269
return std::nullopt;
270270
}
271271
return TypeConversionError(value.GetTypeName(), to_desc->full_name());
@@ -1304,11 +1304,11 @@ class MessageValueBuilderImpl {
13041304
if (auto duration_value = value.AsDuration(); duration_value) {
13051305
CEL_RETURN_IF_ERROR(well_known_types_.Duration().Initialize(
13061306
field->message_type()));
1307-
CEL_RETURN_IF_ERROR(
1308-
well_known_types_.Duration().SetFromAbslDuration(
1309-
reflection_->MutableMessage(message_, field,
1310-
message_factory_),
1311-
duration_value->NativeValue()));
1307+
1308+
well_known_types_.Duration().UnsafeSetFromAbslDuration(
1309+
reflection_->MutableMessage(message_, field,
1310+
message_factory_),
1311+
duration_value->NativeValue());
13121312
return std::nullopt;
13131313
}
13141314
return TypeConversionError(value.GetTypeName(),
@@ -1322,10 +1322,10 @@ class MessageValueBuilderImpl {
13221322
if (auto timestamp_value = value.AsTimestamp(); timestamp_value) {
13231323
CEL_RETURN_IF_ERROR(well_known_types_.Timestamp().Initialize(
13241324
field->message_type()));
1325-
CEL_RETURN_IF_ERROR(well_known_types_.Timestamp().SetFromAbslTime(
1325+
well_known_types_.Timestamp().UnsafeSetFromAbslTime(
13261326
reflection_->MutableMessage(message_, field,
13271327
message_factory_),
1328-
timestamp_value->NativeValue()));
1328+
timestamp_value->NativeValue());
13291329
return std::nullopt;
13301330
}
13311331
return TypeConversionError(value.GetTypeName(),

‎common/values/timestamp_value.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,12 @@ class TimestampValue final
5252
public:
5353
static constexpr ValueKind kKind = ValueKind::kTimestamp;
5454

55+
// Constructs a `TimestampValue` from an `absl::Time`.
56+
//
57+
// DCHECK-fails if the value is not in the supported range.
58+
//
59+
// Prefer using `SafeTimestampValue` or `UnsafeTimestampValue` if the caller
60+
// has already validated the value.
5561
explicit TimestampValue(absl::Time value) noexcept
5662
: TimestampValue(absl::in_place, value) {
5763
ABSL_DCHECK_OK(internal::ValidateTimestamp(value));

‎internal/message_equality.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -380,11 +380,11 @@ absl::StatusOr<EquatableValue> AsEquatableValue(
380380
case Descriptor::WELLKNOWNTYPE_DURATION:
381381
CEL_RETURN_IF_ERROR(
382382
reflection.duration_reflection.Initialize(descriptor));
383-
return reflection.duration_reflection.ToAbslDuration(message);
383+
return reflection.duration_reflection.UnsafeToAbslDuration(message);
384384
case Descriptor::WELLKNOWNTYPE_TIMESTAMP:
385385
CEL_RETURN_IF_ERROR(
386386
reflection.timestamp_reflection.Initialize(descriptor));
387-
return reflection.timestamp_reflection.ToAbslTime(message);
387+
return reflection.timestamp_reflection.UnsafeToAbslTime(message);
388388
case Descriptor::WELLKNOWNTYPE_ANY:
389389
return EquatableAny(message);
390390
default:

‎internal/well_known_types.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2114,13 +2114,15 @@ absl::StatusOr<well_known_types::Value> AdaptFromMessage(
21142114
case Descriptor::WELLKNOWNTYPE_ANY:
21152115
// This is unreachable, as AdaptAny() above recursively unpacks.
21162116
ABSL_UNREACHABLE();
2117+
// Don't check that time values are in range on field access. Assume
2118+
// error will propagate if they are used in any arithmetic.
21172119
case Descriptor::WELLKNOWNTYPE_DURATION: {
21182120
CEL_ASSIGN_OR_RETURN(auto reflection, GetDurationReflection(descriptor));
2119-
return reflection.ToAbslDuration(*to_adapt);
2121+
return reflection.UnsafeToAbslDuration(*to_adapt);
21202122
}
21212123
case Descriptor::WELLKNOWNTYPE_TIMESTAMP: {
21222124
CEL_ASSIGN_OR_RETURN(auto reflection, GetTimestampReflection(descriptor));
2123-
return reflection.ToAbslTime(*to_adapt);
2125+
return reflection.UnsafeToAbslTime(*to_adapt);
21242126
}
21252127
case Descriptor::WELLKNOWNTYPE_VALUE: {
21262128
CEL_ASSIGN_OR_RETURN(auto reflection, GetValueReflection(descriptor));

‎internal/well_known_types_test.cc‎

Lines changed: 83 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,44 @@ TEST_F(ReflectionTest, Duration_Dynamic) {
341341
StatusIs(absl::StatusCode::kInvalidArgument));
342342
}
343343

344+
TEST_F(ReflectionTest, Duration_ToAbslDuration) {
345+
auto* value = MakeDynamic<google::protobuf::Duration>();
346+
ASSERT_OK_AND_ASSIGN(
347+
auto reflection,
348+
GetDurationReflection(ABSL_DIE_IF_NULL(value->GetDescriptor())));
349+
350+
reflection.SetSeconds(value, 1);
351+
reflection.SetNanos(value, 1);
352+
EXPECT_THAT(reflection.ToAbslDuration(*value),
353+
IsOkAndHolds(absl::Seconds(1) + absl::Nanoseconds(1)));
354+
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
355+
absl::Seconds(1) + absl::Nanoseconds(1));
356+
357+
reflection.SetSeconds(value, 0x7fffffffffffffff);
358+
reflection.SetNanos(value, 1);
359+
EXPECT_THAT(reflection.ToAbslDuration(*value),
360+
StatusIs(absl::StatusCode::kInvalidArgument,
361+
HasSubstr("invalid duration seconds: ")));
362+
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
363+
absl::Seconds(0x7fffffffffffffff) + absl::Nanoseconds(1));
364+
365+
reflection.SetSeconds(value, 1);
366+
reflection.SetNanos(value, 0x7fffffff);
367+
EXPECT_THAT(reflection.ToAbslDuration(*value),
368+
StatusIs(absl::StatusCode::kInvalidArgument,
369+
HasSubstr("invalid duration nanoseconds: ")));
370+
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
371+
absl::Seconds(1) + absl::Nanoseconds(0x7fffffff));
372+
373+
reflection.SetSeconds(value, -1);
374+
reflection.SetNanos(value, 1);
375+
EXPECT_THAT(reflection.ToAbslDuration(*value),
376+
StatusIs(absl::StatusCode::kInvalidArgument,
377+
HasSubstr("duration sign mismatch: ")));
378+
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
379+
absl::Seconds(-1) + absl::Nanoseconds(1));
380+
}
381+
344382
TEST_F(ReflectionTest, Timestamp_Generated) {
345383
auto* value = MakeGenerated<google::protobuf::Timestamp>();
346384
EXPECT_EQ(TimestampReflection::GetSeconds(*value), 0);
@@ -389,6 +427,39 @@ TEST_F(ReflectionTest, Timestamp_Dynamic) {
389427
StatusIs(absl::StatusCode::kInvalidArgument));
390428
}
391429

430+
TEST_F(ReflectionTest, Timestamp_ToAbslTime) {
431+
auto* value = MakeDynamic<google::protobuf::Timestamp>();
432+
ASSERT_OK_AND_ASSIGN(
433+
auto reflection,
434+
GetTimestampReflection(ABSL_DIE_IF_NULL(value->GetDescriptor())));
435+
436+
reflection.SetSeconds(value, 1);
437+
reflection.SetNanos(value, 1);
438+
EXPECT_THAT(reflection.ToAbslTime(*value),
439+
IsOkAndHolds(absl::UnixEpoch() + absl::Seconds(1) +
440+
absl::Nanoseconds(1)));
441+
EXPECT_EQ(reflection.UnsafeToAbslTime(*value),
442+
absl::UnixEpoch() + absl::Seconds(1) + absl::Nanoseconds(1));
443+
444+
reflection.SetSeconds(value, 0x7fffffffffffffff);
445+
reflection.SetNanos(value, 1);
446+
EXPECT_THAT(reflection.ToAbslTime(*value),
447+
StatusIs(absl::StatusCode::kInvalidArgument,
448+
HasSubstr("invalid timestamp seconds: ")));
449+
EXPECT_EQ(reflection.UnsafeToAbslTime(*value),
450+
absl::UnixEpoch() + absl::Seconds(0x7fffffffffffffff) +
451+
absl::Nanoseconds(1));
452+
453+
reflection.SetSeconds(value, 1);
454+
reflection.SetNanos(value, 0x7fffffff);
455+
EXPECT_THAT(reflection.ToAbslTime(*value),
456+
StatusIs(absl::StatusCode::kInvalidArgument,
457+
HasSubstr("invalid timestamp nanoseconds: ")));
458+
EXPECT_EQ(
459+
reflection.UnsafeToAbslTime(*value),
460+
absl::UnixEpoch() + absl::Seconds(1) + absl::Nanoseconds(0x7fffffff));
461+
}
462+
392463
TEST_F(ReflectionTest, Value_Generated) {
393464
auto* value = MakeGenerated<google::protobuf::Value>();
394465
EXPECT_EQ(ValueReflection::GetKindCase(*value),
@@ -698,25 +769,25 @@ TEST_F(AdaptFromMessageTest, Duration_SecondsOutOfRange) {
698769
auto message = DynamicParseTextProto<google::protobuf::Duration>(
699770
R"pb(seconds: 0x7fffffffffffffff nanos: 1)pb");
700771
EXPECT_THAT(AdaptFromMessage(*message),
701-
StatusIs(absl::StatusCode::kInvalidArgument,
702-
HasSubstr("invalid duration seconds: ")));
772+
IsOkAndHolds(VariantWith<absl::Duration>(
773+
absl::Seconds(0x7fffffffffffffff) + absl::Nanoseconds(1))));
703774
}
704775

705776
TEST_F(AdaptFromMessageTest, Duration_NanosOutOfRange) {
706777
auto message = DynamicParseTextProto<google::protobuf::Duration>(
707778
R"pb(seconds: 1 nanos: 0x7fffffff)pb");
708779
EXPECT_THAT(AdaptFromMessage(*message),
709-
StatusIs(absl::StatusCode::kInvalidArgument,
710-
HasSubstr("invalid duration nanoseconds: ")));
780+
IsOkAndHolds(VariantWith<absl::Duration>(
781+
absl::Seconds(1) + absl::Nanoseconds(0x7fffffff))));
711782
}
712783

713784
TEST_F(AdaptFromMessageTest, Duration_SignMismatch) {
714785
auto message =
715786
DynamicParseTextProto<google::protobuf::Duration>(R"pb(seconds: -1
716787
nanos: 1)pb");
717788
EXPECT_THAT(AdaptFromMessage(*message),
718-
StatusIs(absl::StatusCode::kInvalidArgument,
719-
HasSubstr("duration sign mismatch: ")));
789+
IsOkAndHolds(VariantWith<absl::Duration>(absl::Seconds(-1) +
790+
absl::Nanoseconds(1))));
720791
}
721792

722793
TEST_F(AdaptFromMessageTest, Timestamp) {
@@ -733,16 +804,18 @@ TEST_F(AdaptFromMessageTest, Timestamp_SecondsOutOfRange) {
733804
auto message = DynamicParseTextProto<google::protobuf::Timestamp>(
734805
R"pb(seconds: 0x7fffffffffffffff nanos: 1)pb");
735806
EXPECT_THAT(AdaptFromMessage(*message),
736-
StatusIs(absl::StatusCode::kInvalidArgument,
737-
HasSubstr("invalid timestamp seconds: ")));
807+
IsOkAndHolds(VariantWith<absl::Time>(
808+
absl::UnixEpoch() + absl::Seconds(0x7fffffffffffffff) +
809+
absl::Nanoseconds(1))));
738810
}
739811

740812
TEST_F(AdaptFromMessageTest, Timestamp_NanosOutOfRange) {
741813
auto message = DynamicParseTextProto<google::protobuf::Timestamp>(
742814
R"pb(seconds: 1 nanos: 0x7fffffff)pb");
743815
EXPECT_THAT(AdaptFromMessage(*message),
744-
StatusIs(absl::StatusCode::kInvalidArgument,
745-
HasSubstr("invalid timestamp nanoseconds: ")));
816+
IsOkAndHolds(
817+
VariantWith<absl::Time>(absl::UnixEpoch() + absl::Seconds(1) +
818+
absl::Nanoseconds(0x7fffffff))));
746819
}
747820

748821
TEST_F(AdaptFromMessageTest, Value_NullValue) {

‎runtime/internal/function_adapter.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,13 +200,13 @@ struct AdaptedToValueVisitor {
200200
absl::StatusOr<Value> operator()(absl::Time in) {
201201
// Type matching may have already occurred. It's too late to change up the
202202
// type and return an error.
203-
return TimestampValue(in);
203+
return UnsafeTimestampValue(in);
204204
}
205205

206206
absl::StatusOr<Value> operator()(absl::Duration in) {
207207
// Type matching may have already occurred. It's too late to change up the
208208
// type and return an error.
209-
return DurationValue(in);
209+
return UnsafeDurationValue(in);
210210
}
211211

212212
absl::StatusOr<Value> operator()(Value in) { return in; }

‎runtime/standard/type_conversion_functions.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -403,14 +403,16 @@ absl::Status RegisterTimeConversionFunctions(FunctionRegistry& registry,
403403
CEL_RETURN_IF_ERROR(
404404
(UnaryFunctionAdapter<Value, absl::Time>::RegisterGlobalOverload(
405405
cel::builtin::kTimestamp,
406-
[](absl::Time value) -> Value { return TimestampValue(value); },
406+
[](absl::Time value) -> Value { return UnsafeTimestampValue(value); },
407407
registry)));
408408

409409
// duration -> duration
410410
CEL_RETURN_IF_ERROR(
411411
(UnaryFunctionAdapter<Value, absl::Duration>::RegisterGlobalOverload(
412412
cel::builtin::kDuration,
413-
[](absl::Duration value) -> Value { return DurationValue(value); },
413+
[](absl::Duration value) -> Value {
414+
return UnsafeDurationValue(value);
415+
},
414416
registry)));
415417

416418
// timestamp() conversion from string.

0 commit comments

Comments
 (0)