Skip to content

Commit e972c4b

Browse files
TristonianJoneskyessenov
authored andcommitted
Introduce new Has method for presence testing on CelMap values.
PiperOrigin-RevId: 374306406
1 parent bdf9939 commit e972c4b

21 files changed

Lines changed: 796 additions & 360 deletions

conformance/BUILD

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,6 @@ cc_binary(
8585
# TODO(issues/114): Ensure the 'in' operator is a logical OR of element equality results.
8686
"--skip_test=comparisons/in_list_literal/elem_in_mixed_type_list_error",
8787
"--skip_test=comparisons/in_map_literal/key_in_mixed_key_type_map_error",
88-
# TODO(issues/115): The 'in' operator fails with maps containing boolean keys.
89-
"--skip_test=fields/in/singleton",
9088
# TODO(issues/97): Parse-only qualified variable lookup "x.y" wtih binding "x.y" or "y" within container "x" fails
9189
"--skip_test=fields/qualified_identifier_resolution/qualified_ident,map_field_select,ident_with_longest_prefix_check,qualified_identifier_resolution_unchecked",
9290
"--skip_test=namespace/qualified/self_eval_qualified_lookup",

eval/eval/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,7 @@ cc_test(
419419
"//eval/public/structs:cel_proto_wrapper",
420420
"//eval/testutil:test_message_cc_proto",
421421
"//testutil:util",
422+
"@com_google_absl//absl/status",
422423
"@com_google_absl//absl/status:statusor",
423424
"@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto",
424425
"@com_google_googletest//:gtest_main",

eval/eval/select_step.cc

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "eval/eval/select_step.h"
22

3+
#include "absl/status/status.h"
34
#include "absl/status/statusor.h"
45
#include "absl/strings/str_cat.h"
56
#include "eval/eval/evaluator_core.h"
@@ -155,7 +156,6 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const {
155156
}
156157

157158
absl::Status status = CreateValueFromField(msg, frame->arena(), &result);
158-
159159
if (status.ok()) {
160160
frame->value_stack().PopAndPush(result, result_trail);
161161
}
@@ -178,15 +178,23 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const {
178178
return absl::OkStatus();
179179
}
180180

181-
auto lookup_result = (*cel_map)[CelValue::CreateString(&field_)];
182-
183-
// Test only Select expression.
181+
CelValue field_name = CelValue::CreateString(&field_);
184182
if (test_field_presence_) {
185-
result = CelValue::CreateBool(lookup_result.has_value());
183+
// Field presence only supports string keys containing valid identifier
184+
// characters.
185+
auto presence = cel_map->Has(field_name);
186+
if (!presence.ok()) {
187+
CelValue error_value =
188+
CreateErrorValue(frame->arena(), presence.status());
189+
frame->value_stack().PopAndPush(error_value);
190+
return absl::OkStatus();
191+
}
192+
result = CelValue::CreateBool(*presence);
186193
frame->value_stack().PopAndPush(result);
187194
return absl::OkStatus();
188195
}
189196

197+
auto lookup_result = (*cel_map)[field_name];
190198
if (frame->enable_unknowns()) {
191199
result_trail = trail.Step(&field_, frame->arena());
192200
if (frame->attribute_utility().CheckForUnknown(result_trail, false)) {
@@ -205,7 +213,6 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const {
205213
result = CreateNoSuchKeyError(frame->arena(), field_);
206214
}
207215
frame->value_stack().PopAndPush(result, result_trail);
208-
209216
return absl::OkStatus();
210217
}
211218
case CelValue::Type::kUnknownSet: {

eval/eval/select_step_test.cc

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include "google/api/expr/v1alpha1/syntax.pb.h"
44
#include "gmock/gmock.h"
55
#include "gtest/gtest.h"
6+
#include "absl/status/status.h"
67
#include "absl/status/statusor.h"
78
#include "eval/eval/ident_step.h"
89
#include "eval/public/cel_attribute.h"
@@ -32,7 +33,6 @@ absl::StatusOr<CelValue> RunExpression(const CelValue target,
3233
absl::string_view unknown_path,
3334
bool enable_unknowns) {
3435
ExecutionPath path;
35-
3636
Expr dummy_expr;
3737

3838
auto select = dummy_expr.mutable_select_expr();
@@ -179,6 +179,43 @@ TEST_P(SelectStepTest, MapPresenseIsTrueTest) {
179179
EXPECT_EQ(result.BoolOrDie(), true);
180180
}
181181

182+
TEST(SelectStepTest, MapPresenseIsErrorTest) {
183+
TestMessage message;
184+
google::protobuf::Arena arena;
185+
186+
Expr select_expr;
187+
auto select = select_expr.mutable_select_expr();
188+
select->set_field("1");
189+
select->set_test_only(true);
190+
Expr* expr1 = select->mutable_operand();
191+
auto select_map = expr1->mutable_select_expr();
192+
select_map->set_field("int32_int32_map");
193+
Expr* expr0 = select_map->mutable_operand();
194+
auto ident = expr0->mutable_ident_expr();
195+
ident->set_name("target");
196+
197+
auto step0_status = CreateIdentStep(ident, expr0->id());
198+
auto step1_status = CreateSelectStep(select_map, expr1->id(), "");
199+
auto step2_status = CreateSelectStep(select, select_expr.id(), "");
200+
ASSERT_OK(step0_status);
201+
ASSERT_OK(step1_status);
202+
ASSERT_OK(step2_status);
203+
204+
ExecutionPath path;
205+
path.push_back(std::move(step0_status.value()));
206+
path.push_back(std::move(step1_status.value()));
207+
path.push_back(std::move(step2_status.value()));
208+
CelExpressionFlatImpl cel_expr(&select_expr, std::move(path), 0, {}, false);
209+
Activation activation;
210+
activation.InsertValue("target",
211+
CelProtoWrapper::CreateMessage(&message, &arena));
212+
auto status = cel_expr.Evaluate(activation, &arena);
213+
CelValue result = status.value();
214+
215+
EXPECT_TRUE(result.IsError());
216+
EXPECT_EQ(result.ErrorOrDie()->code(), absl::StatusCode::kInvalidArgument);
217+
}
218+
182219
TEST(SelectStepTest, MapPresenseIsTrueWithUnknownTest) {
183220
UnknownSet unknown_set;
184221
std::string key1 = "key1";

eval/public/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ cc_library(
2525
deps = [
2626
":cel_value_internal",
2727
"@com_google_absl//absl/status",
28+
"@com_google_absl//absl/status:statusor",
2829
"@com_google_absl//absl/strings",
2930
"@com_google_absl//absl/strings:str_format",
3031
"@com_google_absl//absl/time",

0 commit comments

Comments
 (0)