Skip to content

Commit 6faa5a0

Browse files
CEL Dev Teamkyessenov
CEL Dev Team
authored and
kyessenov
committed
Update CreateContainerBackedMap to use absl::StatusOr and fix ValueToCelValue
PiperOrigin-RevId: 367682028
1 parent 30af72e commit 6faa5a0

14 files changed

Lines changed: 98 additions & 63 deletions

‎eval/compiler/flat_expr_builder_test.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) {
604604
{CelValue::CreateStringView("var2"), CelValue::CreateBool(false)}};
605605

606606
std::unique_ptr<CelMap> map_value =
607-
CreateContainerBackedMap(absl::MakeSpan(map_pairs));
607+
CreateContainerBackedMap(absl::MakeSpan(map_pairs)).value();
608608
activation.InsertValue("bar", CelValue::CreateMap(map_value.get()));
609609
result_or = cel_expr->Evaluate(activation, &arena);
610610
ASSERT_OK(result_or);

‎eval/eval/create_struct_step.cc‎

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

33
#include <memory>
4+
#include <utility>
45

56
#include "google/api/expr/v1alpha1/syntax.pb.h"
67
#include "absl/status/status.h"
@@ -233,16 +234,17 @@ absl::Status CreateStructStepForMap::DoEvaluate(ExecutionFrame* frame,
233234
map_entries.push_back({args[2 * i], args[2 * i + 1]});
234235
}
235236

236-
auto cel_map =
237+
auto status_or_cel_map =
237238
CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
238239
map_entries.data(), map_entries.size()));
239-
240-
if (cel_map == nullptr) {
241-
*result = CreateErrorValue(frame->arena(), "Failed to create map");
242-
240+
if (!status_or_cel_map.ok()) {
241+
*result =
242+
CreateErrorValue(frame->arena(), status_or_cel_map.status().message());
243243
return absl::OkStatus();
244244
}
245245

246+
auto cel_map = std::move(*status_or_cel_map);
247+
246248
*result = CelValue::CreateMap(cel_map.get());
247249

248250
// Pass object ownership to Arena.

‎eval/eval/create_struct_step_test.cc‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -593,7 +593,8 @@ TEST_P(CreateCreateStructStepTest, TestSetStringMapField) {
593593

594594
auto cel_map =
595595
CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
596-
entries.data(), entries.size()));
596+
entries.data(), entries.size()))
597+
.value();
597598

598599
ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage(
599600
"string_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg,
@@ -620,7 +621,8 @@ TEST_P(CreateCreateStructStepTest, TestSetInt64MapField) {
620621

621622
auto cel_map =
622623
CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
623-
entries.data(), entries.size()));
624+
entries.data(), entries.size()))
625+
.value();
624626

625627
ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage(
626628
"int64_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg,
@@ -647,7 +649,8 @@ TEST_P(CreateCreateStructStepTest, TestSetUInt64MapField) {
647649

648650
auto cel_map =
649651
CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
650-
entries.data(), entries.size()));
652+
entries.data(), entries.size()))
653+
.value();
651654

652655
ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage(
653656
"uint64_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg,

‎eval/eval/select_step_test.cc‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,8 @@ TEST_P(SelectStepTest, MapPresenseIsFalseTest) {
147147
{CelValue::CreateString(&key1), CelValue::CreateInt64(1)}};
148148

149149
auto map_value = CreateContainerBackedMap(
150-
absl::Span<std::pair<CelValue, CelValue>>(key_values));
150+
absl::Span<std::pair<CelValue, CelValue>>(key_values))
151+
.value();
151152

152153
google::protobuf::Arena arena;
153154

@@ -165,7 +166,8 @@ TEST_P(SelectStepTest, MapPresenseIsTrueTest) {
165166
{CelValue::CreateString(&key1), CelValue::CreateInt64(1)}};
166167

167168
auto map_value = CreateContainerBackedMap(
168-
absl::Span<std::pair<CelValue, CelValue>>(key_values));
169+
absl::Span<std::pair<CelValue, CelValue>>(key_values))
170+
.value();
169171

170172
google::protobuf::Arena arena;
171173

@@ -185,7 +187,8 @@ TEST(SelectStepTest, MapPresenseIsTrueWithUnknownTest) {
185187
CelValue::CreateUnknownSet(&unknown_set)}};
186188

187189
auto map_value = CreateContainerBackedMap(
188-
absl::Span<std::pair<CelValue, CelValue>>(key_values));
190+
absl::Span<std::pair<CelValue, CelValue>>(key_values))
191+
.value();
189192

190193
google::protobuf::Arena arena;
191194

@@ -427,7 +430,8 @@ TEST_P(SelectStepTest, MapSimpleInt32Test) {
427430
{CelValue::CreateString(&key2), CelValue::CreateInt64(2)}};
428431

429432
auto map_value = CreateContainerBackedMap(
430-
absl::Span<std::pair<CelValue, CelValue>>(key_values));
433+
absl::Span<std::pair<CelValue, CelValue>>(key_values))
434+
.value();
431435

432436
google::protobuf::Arena arena;
433437

‎eval/public/containers/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ cc_library(
4848
deps = [
4949
"//eval/public:cel_value",
5050
"@com_google_absl//absl/container:node_hash_map",
51+
"@com_google_absl//absl/status",
52+
"@com_google_absl//absl/status:statusor",
5153
"@com_google_absl//absl/types:span",
5254
],
5355
)

‎eval/public/containers/container_backed_map_impl.cc‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include "eval/public/containers/container_backed_map_impl.h"
44

55
#include "absl/container/node_hash_map.h"
6+
#include "absl/status/status.h"
67
#include "absl/types/span.h"
78
#include "eval/public/cel_value.h"
89

@@ -102,14 +103,15 @@ class Equal {
102103
// type of key in STL map.
103104
class ContainerBackedMapImpl : public CelMap {
104105
public:
105-
static std::unique_ptr<CelMap> Create(
106+
static absl::StatusOr<std::unique_ptr<CelMap>> Create(
106107
absl::Span<std::pair<CelValue, CelValue>> key_values) {
107108
auto cel_map = absl::WrapUnique(new ContainerBackedMapImpl());
108109

109-
if (!cel_map->AddItems(key_values)) {
110-
return nullptr;
110+
auto status = cel_map->AddItems(key_values);
111+
if (!status.ok()) {
112+
return status;
111113
}
112-
return std::move(cel_map);
114+
return cel_map;
113115
}
114116

115117
// Map size.
@@ -141,17 +143,17 @@ class ContainerBackedMapImpl : public CelMap {
141143

142144
ContainerBackedMapImpl() = default;
143145

144-
bool AddItems(absl::Span<std::pair<CelValue, CelValue>> key_values) {
146+
absl::Status AddItems(absl::Span<std::pair<CelValue, CelValue>> key_values) {
145147
for (const auto& item : key_values) {
146148
auto result = values_map_.emplace(item.first, item.second);
147149

148150
// Failed to insert pair into map - addition failed.
149151
if (!result.second) {
150-
return false;
152+
return absl::InvalidArgumentError("duplicate map keys");
151153
}
152154
key_list_.Add(item.first);
153155
}
154-
return true;
156+
return absl::OkStatus();
155157
}
156158

157159
absl::node_hash_map<CelValue, CelValue, Hasher, Equal> values_map_;
@@ -160,7 +162,7 @@ class ContainerBackedMapImpl : public CelMap {
160162

161163
} // namespace
162164

163-
std::unique_ptr<CelMap> CreateContainerBackedMap(
165+
absl::StatusOr<std::unique_ptr<CelMap>> CreateContainerBackedMap(
164166
absl::Span<std::pair<CelValue, CelValue>> key_values) {
165167
return ContainerBackedMapImpl::Create(key_values);
166168
}

‎eval/public/containers/container_backed_map_impl.h‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,20 @@
11
#ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CONTAINERS_CONTAINER_BACKED_MAP_IMPL_H_
22
#define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CONTAINERS_CONTAINER_BACKED_MAP_IMPL_H_
33

4-
#include "eval/public/cel_value.h"
4+
#include <memory>
5+
#include <utility>
6+
7+
#include "absl/status/statusor.h"
58
#include "absl/types/span.h"
9+
#include "eval/public/cel_value.h"
610

711
namespace google {
812
namespace api {
913
namespace expr {
1014
namespace runtime {
1115

1216
// Template factory method creating container-backed CelMap.
13-
std::unique_ptr<CelMap> CreateContainerBackedMap(
17+
absl::StatusOr<std::unique_ptr<CelMap>> CreateContainerBackedMap(
1418
absl::Span<std::pair<CelValue, CelValue>> key_values);
1519

1620
} // namespace runtime

‎eval/public/containers/container_backed_map_impl_test.cc‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,18 @@ namespace runtime {
1616
namespace {
1717

1818
using testing::Eq;
19-
using testing::Not;
2019
using testing::IsNull;
20+
using testing::Not;
2121

2222
TEST(ContainerBackedMapImplTest, TestMapInt64) {
2323
std::vector<std::pair<CelValue, CelValue>> args = {
2424
{CelValue::CreateInt64(1), CelValue::CreateInt64(2)},
2525
{CelValue::CreateInt64(2), CelValue::CreateInt64(3)}};
2626

27-
auto cel_map = CreateContainerBackedMap(
28-
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()));
27+
auto cel_map =
28+
CreateContainerBackedMap(
29+
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()))
30+
.value();
2931

3032
ASSERT_THAT(cel_map, Not(IsNull()));
3133

@@ -56,8 +58,10 @@ TEST(ContainerBackedMapImplTest, TestMapUint64) {
5658
std::vector<std::pair<CelValue, CelValue>> args = {
5759
{CelValue::CreateUint64(1), CelValue::CreateInt64(2)},
5860
{CelValue::CreateUint64(2), CelValue::CreateInt64(3)}};
59-
auto cel_map = CreateContainerBackedMap(
60-
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()));
61+
auto cel_map =
62+
CreateContainerBackedMap(
63+
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()))
64+
.value();
6165

6266
ASSERT_THAT(cel_map, Not(IsNull()));
6367

@@ -92,8 +96,10 @@ TEST(ContainerBackedMapImplTest, TestMapString) {
9296
std::vector<std::pair<CelValue, CelValue>> args = {
9397
{CelValue::CreateString(&kKey1), CelValue::CreateInt64(2)},
9498
{CelValue::CreateString(&kKey2), CelValue::CreateInt64(3)}};
95-
auto cel_map = CreateContainerBackedMap(
96-
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()));
99+
auto cel_map =
100+
CreateContainerBackedMap(
101+
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()))
102+
.value();
97103

98104
ASSERT_THAT(cel_map, Not(IsNull()));
99105

‎eval/public/set_util_test.cc‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -331,19 +331,22 @@ TEST(CelValueLessThan, CelMapSameSize) {
331331
{CelValue::CreateInt64(1), CelValue::CreateInt64(2)},
332332
{CelValue::CreateInt64(3), CelValue::CreateInt64(6)}};
333333

334-
auto cel_map_backing_1 = CreateContainerBackedMap(absl::MakeSpan(values));
334+
auto cel_map_backing_1 =
335+
CreateContainerBackedMap(absl::MakeSpan(values)).value();
335336

336337
std::vector<std::pair<CelValue, CelValue>> values2{
337338
{CelValue::CreateInt64(1), CelValue::CreateInt64(2)},
338339
{CelValue::CreateInt64(4), CelValue::CreateInt64(6)}};
339340

340-
auto cel_map_backing_2 = CreateContainerBackedMap(absl::MakeSpan(values2));
341+
auto cel_map_backing_2 =
342+
CreateContainerBackedMap(absl::MakeSpan(values2)).value();
341343

342344
std::vector<std::pair<CelValue, CelValue>> values3{
343345
{CelValue::CreateInt64(1), CelValue::CreateInt64(2)},
344346
{CelValue::CreateInt64(3), CelValue::CreateInt64(8)}};
345347

346-
auto cel_map_backing_3 = CreateContainerBackedMap(absl::MakeSpan(values3));
348+
auto cel_map_backing_3 =
349+
CreateContainerBackedMap(absl::MakeSpan(values3)).value();
347350

348351
CelValue map1 = CelValue::CreateMap(cel_map_backing_1.get());
349352
CelValue map2 = CelValue::CreateMap(cel_map_backing_2.get());
@@ -359,14 +362,14 @@ TEST(CelValueLessThan, CelMapDifferentSizes) {
359362
{CelValue::CreateInt64(1), CelValue::CreateInt64(2)},
360363
{CelValue::CreateInt64(2), CelValue::CreateInt64(4)}};
361364

362-
auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values));
365+
auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)).value();
363366

364367
std::vector<std::pair<CelValue, CelValue>> values2{
365368
{CelValue::CreateInt64(1), CelValue::CreateInt64(2)},
366369
{CelValue::CreateInt64(2), CelValue::CreateInt64(4)},
367370
{CelValue::CreateInt64(3), CelValue::CreateInt64(6)}};
368371

369-
auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2));
372+
auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)).value();
370373

371374
EXPECT_TRUE(CelValueLessThan(CelValue::CreateMap(cel_map_1.get()),
372375
CelValue::CreateMap(cel_map_2.get())));
@@ -378,14 +381,14 @@ TEST(CelValueLessThan, CelMapEqual) {
378381
{CelValue::CreateInt64(2), CelValue::CreateInt64(4)},
379382
{CelValue::CreateInt64(3), CelValue::CreateInt64(6)}};
380383

381-
auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values));
384+
auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)).value();
382385

383386
std::vector<std::pair<CelValue, CelValue>> values2{
384387
{CelValue::CreateInt64(1), CelValue::CreateInt64(2)},
385388
{CelValue::CreateInt64(2), CelValue::CreateInt64(4)},
386389
{CelValue::CreateInt64(3), CelValue::CreateInt64(6)}};
387390

388-
auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2));
391+
auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)).value();
389392

390393
EXPECT_FALSE(CelValueLessThan(CelValue::CreateMap(cel_map_1.get()),
391394
CelValue::CreateMap(cel_map_2.get())));
@@ -418,7 +421,7 @@ TEST(CelValueLessThan, CelMapSupportProtoMapCompatible) {
418421
{CelValue::CreateStringView(kFields[1]), CelValue::CreateDouble(1.0)},
419422
{CelValue::CreateStringView(kFields[0]), CelValue::CreateBool(true)}};
420423

421-
auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values));
424+
auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)).value();
422425

423426
CelValue cel_map = CelValue::CreateMap(backing_map.get());
424427

@@ -451,7 +454,7 @@ TEST(CelValueLessThan, NestedMap) {
451454
std::vector<std::pair<CelValue, CelValue>> values{
452455
{CelValue::CreateStringView("field"), cel_list}};
453456

454-
auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values));
457+
auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)).value();
455458

456459
CelValue cel_map = CelValue::CreateMap(backing_map.get());
457460
CelValue proto_map = CelProtoWrapper::CreateMessage(&value_struct, &arena);

‎eval/public/structs/cel_proto_wrapper_test.cc‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -750,8 +750,10 @@ TEST_F(CelProtoWrapperTest, WrapStruct) {
750750
std::vector<std::pair<CelValue, CelValue>> args = {
751751
{CelValue::CreateString(CelValue::StringHolder(&kField1)),
752752
CelValue::CreateBool(true)}};
753-
auto cel_map = CreateContainerBackedMap(
754-
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()));
753+
auto cel_map =
754+
CreateContainerBackedMap(
755+
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()))
756+
.value();
755757
auto cel_value = CelValue::CreateMap(cel_map.get());
756758

757759
Value json;
@@ -768,8 +770,10 @@ TEST_F(CelProtoWrapperTest, WrapStruct) {
768770
TEST_F(CelProtoWrapperTest, WrapFailureStructBadKeyType) {
769771
std::vector<std::pair<CelValue, CelValue>> args = {
770772
{CelValue::CreateInt64(1L), CelValue::CreateBool(true)}};
771-
auto cel_map = CreateContainerBackedMap(
772-
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()));
773+
auto cel_map =
774+
CreateContainerBackedMap(
775+
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()))
776+
.value();
773777
auto cel_value = CelValue::CreateMap(cel_map.get());
774778

775779
Value json;
@@ -782,8 +786,10 @@ TEST_F(CelProtoWrapperTest, WrapFailureStructBadValueType) {
782786
std::vector<std::pair<CelValue, CelValue>> args = {
783787
{CelValue::CreateString(CelValue::StringHolder(&kField1)),
784788
CelProtoWrapper::CreateMessage(&bad_value, arena())}};
785-
auto cel_map = CreateContainerBackedMap(
786-
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()));
789+
auto cel_map =
790+
CreateContainerBackedMap(
791+
absl::Span<std::pair<CelValue, CelValue>>(args.data(), args.size()))
792+
.value();
787793
auto cel_value = CelValue::CreateMap(cel_map.get());
788794
Value json;
789795
ExpectNotWrapped(cel_value, json);

0 commit comments

Comments
 (0)