Skip to content

Commit 1e2eaa5

Browse files
jckingcopybara-github
jcking
authored and
copybara-github
committed
Fix handling of invalid values with optional elements/entries/fields
PiperOrigin-RevId: 736133580
1 parent df68462 commit 1e2eaa5

File tree

4 files changed

+120
-12
lines changed

4 files changed

+120
-12
lines changed

‎eval/eval/create_list_step.cc‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ absl::Status CreateListStep::Evaluate(ExecutionFrame* frame) const {
6363

6464
cel::Value result;
6565
for (const auto& arg : args) {
66-
if (arg->Is<cel::ErrorValue>()) {
66+
if (arg.IsError()) {
6767
result = arg;
6868
frame->value_stack().Pop(list_size_);
6969
frame->value_stack().Push(std::move(result));
@@ -95,8 +95,10 @@ absl::Status CreateListStep::Evaluate(ExecutionFrame* frame) const {
9595
}
9696
CEL_RETURN_IF_ERROR(builder->Add(optional_arg->Value()));
9797
} else {
98-
return cel::TypeConversionError(arg.GetTypeName(), "optional_type")
99-
.NativeValue();
98+
frame->value_stack().PopAndPush(
99+
list_size_,
100+
cel::TypeConversionError(arg.GetTypeName(), "optional_type"));
101+
return absl::OkStatus();
100102
}
101103
} else {
102104
CEL_RETURN_IF_ERROR(builder->Add(arg));
@@ -180,8 +182,9 @@ class CreateListDirectStep : public DirectExpressionStep {
180182
CEL_RETURN_IF_ERROR(builder->Add(optional_arg->Value()));
181183
continue;
182184
}
183-
return cel::TypeConversionError(result.GetTypeName(), "optional_type")
184-
.NativeValue();
185+
result =
186+
cel::TypeConversionError(result.GetTypeName(), "optional_type");
187+
return absl::OkStatus();
185188
}
186189

187190
// Otherwise just add.

‎eval/eval/create_map_step.cc‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,7 @@ absl::StatusOr<Value> CreateStructStepForMap::DoEvaluate(
9898
}
9999
} else {
100100
return cel::TypeConversionError(map_value.DebugString(),
101-
"optional_type")
102-
.NativeValue();
101+
"optional_type");
103102
}
104103
} else {
105104
auto key_status = builder->Put(std::move(map_key), std::move(map_value));
@@ -159,7 +158,7 @@ absl::Status DirectCreateMapStep::Evaluate(
159158
int map_value_index = map_key_index + 1;
160159
CEL_RETURN_IF_ERROR(deps_[map_key_index]->Evaluate(frame, key, tmp_attr));
161160

162-
if (InstanceOf<ErrorValue>(key)) {
161+
if (key.IsError()) {
163162
result = key;
164163
return absl::OkStatus();
165164
}
@@ -175,7 +174,7 @@ absl::Status DirectCreateMapStep::Evaluate(
175174
CEL_RETURN_IF_ERROR(
176175
deps_[map_value_index]->Evaluate(frame, value, tmp_attr));
177176

178-
if (InstanceOf<ErrorValue>(value)) {
177+
if (value.IsError()) {
179178
result = value;
180179
return absl::OkStatus();
181180
}
@@ -209,8 +208,8 @@ absl::Status DirectCreateMapStep::Evaluate(
209208
}
210209
continue;
211210
}
212-
return cel::TypeConversionError(value.DebugString(), "optional_type")
213-
.NativeValue();
211+
result = cel::TypeConversionError(value.DebugString(), "optional_type");
212+
return absl::OkStatus();
214213
}
215214

216215
CEL_RETURN_IF_ERROR(cel::CheckMapKey(key));

‎eval/eval/create_struct_step.cc‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@ absl::StatusOr<Value> CreateStructStepForStruct::DoEvaluate(
102102
}
103103
CEL_RETURN_IF_ERROR(
104104
builder->SetFieldByName(entry, optional_arg->Value()));
105+
} else {
106+
return cel::TypeConversionError(arg.DebugString(), "optional_type");
105107
}
106108
} else {
107109
CEL_RETURN_IF_ERROR(builder->SetFieldByName(entry, std::move(arg)));
@@ -206,8 +208,12 @@ absl::Status DirectCreateStructStep::Evaluate(ExecutionFrameBase& frame,
206208
result = cel::ErrorValue(std::move(status));
207209
return absl::OkStatus();
208210
}
211+
continue;
212+
} else {
213+
result = cel::TypeConversionError(field_value.DebugString(),
214+
"optional_type");
215+
return absl::OkStatus();
209216
}
210-
continue;
211217
}
212218

213219
auto status =

‎runtime/optional_types_test.cc‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,5 +357,105 @@ TEST(OptionalTypesTest, ErrorShortCircuiting) {
357357
HasSubstr("divide by zero")));
358358
}
359359

360+
TEST(OptionalTypesTest, CreateList_TypeConversionError) {
361+
RuntimeOptions opts{.enable_qualified_type_identifiers = true};
362+
google::protobuf::Arena arena;
363+
364+
ASSERT_OK_AND_ASSIGN(
365+
auto builder,
366+
CreateStandardRuntimeBuilder(internal::GetTestingDescriptorPool(), opts));
367+
368+
ASSERT_THAT(EnableOptionalTypes(builder), IsOk());
369+
ASSERT_THAT(
370+
EnableReferenceResolver(builder, ReferenceResolverEnabled::kAlways),
371+
IsOk());
372+
373+
ASSERT_OK_AND_ASSIGN(auto runtime, std::move(builder).Build());
374+
375+
ASSERT_OK_AND_ASSIGN(ParsedExpr expr,
376+
Parse("[?foo]", "<input>",
377+
ParserOptions{.enable_optional_syntax = true}));
378+
379+
ASSERT_OK_AND_ASSIGN(std::unique_ptr<Program> program,
380+
ProtobufRuntimeAdapter::CreateProgram(*runtime, expr));
381+
382+
Activation activation;
383+
activation.InsertOrAssignValue("foo", IntValue(1));
384+
385+
ASSERT_OK_AND_ASSIGN(Value result, program->Evaluate(&arena, activation));
386+
387+
ASSERT_TRUE(result.IsError()) << result.DebugString();
388+
EXPECT_THAT(result.GetError().ToStatus(),
389+
StatusIs(absl::StatusCode::kInvalidArgument,
390+
HasSubstr("type conversion error")));
391+
}
392+
393+
TEST(OptionalTypesTest, CreateMap_TypeConversionError) {
394+
RuntimeOptions opts{.enable_qualified_type_identifiers = true};
395+
google::protobuf::Arena arena;
396+
397+
ASSERT_OK_AND_ASSIGN(
398+
auto builder,
399+
CreateStandardRuntimeBuilder(internal::GetTestingDescriptorPool(), opts));
400+
401+
ASSERT_THAT(EnableOptionalTypes(builder), IsOk());
402+
ASSERT_THAT(
403+
EnableReferenceResolver(builder, ReferenceResolverEnabled::kAlways),
404+
IsOk());
405+
406+
ASSERT_OK_AND_ASSIGN(auto runtime, std::move(builder).Build());
407+
408+
ASSERT_OK_AND_ASSIGN(ParsedExpr expr,
409+
Parse("{?1: bar}", "<input>",
410+
ParserOptions{.enable_optional_syntax = true}));
411+
412+
ASSERT_OK_AND_ASSIGN(std::unique_ptr<Program> program,
413+
ProtobufRuntimeAdapter::CreateProgram(*runtime, expr));
414+
415+
Activation activation;
416+
activation.InsertOrAssignValue("foo", IntValue(1));
417+
418+
ASSERT_OK_AND_ASSIGN(Value result, program->Evaluate(&arena, activation));
419+
420+
ASSERT_TRUE(result.IsError()) << result.DebugString();
421+
EXPECT_THAT(result.GetError().ToStatus(),
422+
StatusIs(absl::StatusCode::kInvalidArgument,
423+
HasSubstr("type conversion error")));
424+
}
425+
426+
TEST(OptionalTypesTest, CreateStruct_KeyTypeConversionError) {
427+
RuntimeOptions opts{.enable_qualified_type_identifiers = true};
428+
google::protobuf::Arena arena;
429+
430+
ASSERT_OK_AND_ASSIGN(
431+
auto builder,
432+
CreateStandardRuntimeBuilder(internal::GetTestingDescriptorPool(), opts));
433+
434+
ASSERT_THAT(EnableOptionalTypes(builder), IsOk());
435+
ASSERT_THAT(
436+
EnableReferenceResolver(builder, ReferenceResolverEnabled::kAlways),
437+
IsOk());
438+
439+
ASSERT_OK_AND_ASSIGN(auto runtime, std::move(builder).Build());
440+
441+
ASSERT_OK_AND_ASSIGN(
442+
ParsedExpr expr,
443+
Parse("cel.expr.conformance.proto2.TestAllTypes{?single_int32: foo}",
444+
"<input>", ParserOptions{.enable_optional_syntax = true}));
445+
446+
ASSERT_OK_AND_ASSIGN(std::unique_ptr<Program> program,
447+
ProtobufRuntimeAdapter::CreateProgram(*runtime, expr));
448+
449+
Activation activation;
450+
activation.InsertOrAssignValue("foo", IntValue(1));
451+
452+
ASSERT_OK_AND_ASSIGN(Value result, program->Evaluate(&arena, activation));
453+
454+
ASSERT_TRUE(result.IsError()) << result.DebugString();
455+
EXPECT_THAT(result.GetError().ToStatus(),
456+
StatusIs(absl::StatusCode::kInvalidArgument,
457+
HasSubstr("type conversion error")));
458+
}
459+
360460
} // namespace
361461
} // namespace cel::extensions

0 commit comments

Comments
 (0)