From 30af72e187dbb80ca23a5e9d3d4edc6638c741fc Mon Sep 17 00:00:00 2001 From: tswadell Date: Wed, 7 Apr 2021 15:22:31 -0400 Subject: [PATCH 01/12] Default enable .size() to return the number of unicode codepoints. PiperOrigin-RevId: 367272711 --- eval/public/cel_options.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/eval/public/cel_options.h b/eval/public/cel_options.h index 4dbe48d03..952059ae3 100644 --- a/eval/public/cel_options.h +++ b/eval/public/cel_options.h @@ -29,8 +29,7 @@ struct InterpreterOptions { // Enable functions which return the string.size() as the number of unicode // codepoints. - // Starting on 4/7/2021 this will default to 'true' - bool enable_string_size_as_unicode_codepoints = false; + bool enable_string_size_as_unicode_codepoints = true; // Enable short-circuiting of the logical operator evaluation. If enabled, // AND, OR, and TERNARY do not evaluate the entire expression once the the From 6faa5a0ba698c18e189ff28f605c06b7da0c2d69 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Fri, 9 Apr 2021 15:28:16 -0400 Subject: [PATCH 02/12] Update CreateContainerBackedMap to use absl::StatusOr and fix ValueToCelValue PiperOrigin-RevId: 367682028 --- eval/compiler/flat_expr_builder_test.cc | 2 +- eval/eval/create_struct_step.cc | 12 ++++++----- eval/eval/create_struct_step_test.cc | 9 +++++--- eval/eval/select_step_test.cc | 12 +++++++---- eval/public/containers/BUILD | 2 ++ .../containers/container_backed_map_impl.cc | 18 +++++++++------- .../containers/container_backed_map_impl.h | 8 +++++-- .../container_backed_map_impl_test.cc | 20 +++++++++++------- eval/public/set_util_test.cc | 21 +++++++++++-------- eval/public/structs/cel_proto_wrapper_test.cc | 18 ++++++++++------ eval/public/transform_utility.cc | 11 +++++----- .../unknown_function_result_set_test.cc | 18 ++++++++-------- eval/public/value_export_util_test.cc | 6 ++++-- eval/tests/unknowns_end_to_end_test.cc | 4 ++-- 14 files changed, 98 insertions(+), 63 deletions(-) diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index 0a1fcf258..72588672d 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -604,7 +604,7 @@ TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) { {CelValue::CreateStringView("var2"), CelValue::CreateBool(false)}}; std::unique_ptr map_value = - CreateContainerBackedMap(absl::MakeSpan(map_pairs)); + CreateContainerBackedMap(absl::MakeSpan(map_pairs)).value(); activation.InsertValue("bar", CelValue::CreateMap(map_value.get())); result_or = cel_expr->Evaluate(activation, &arena); ASSERT_OK(result_or); diff --git a/eval/eval/create_struct_step.cc b/eval/eval/create_struct_step.cc index fd9c79dec..13243e14d 100644 --- a/eval/eval/create_struct_step.cc +++ b/eval/eval/create_struct_step.cc @@ -1,6 +1,7 @@ #include "eval/eval/create_struct_step.h" #include +#include #include "google/api/expr/v1alpha1/syntax.pb.h" #include "absl/status/status.h" @@ -233,16 +234,17 @@ absl::Status CreateStructStepForMap::DoEvaluate(ExecutionFrame* frame, map_entries.push_back({args[2 * i], args[2 * i + 1]}); } - auto cel_map = + auto status_or_cel_map = CreateContainerBackedMap(absl::Span>( map_entries.data(), map_entries.size())); - - if (cel_map == nullptr) { - *result = CreateErrorValue(frame->arena(), "Failed to create map"); - + if (!status_or_cel_map.ok()) { + *result = + CreateErrorValue(frame->arena(), status_or_cel_map.status().message()); return absl::OkStatus(); } + auto cel_map = std::move(*status_or_cel_map); + *result = CelValue::CreateMap(cel_map.get()); // Pass object ownership to Arena. diff --git a/eval/eval/create_struct_step_test.cc b/eval/eval/create_struct_step_test.cc index 7586b1b0b..655f4fe76 100644 --- a/eval/eval/create_struct_step_test.cc +++ b/eval/eval/create_struct_step_test.cc @@ -593,7 +593,8 @@ TEST_P(CreateCreateStructStepTest, TestSetStringMapField) { auto cel_map = CreateContainerBackedMap(absl::Span>( - entries.data(), entries.size())); + entries.data(), entries.size())) + .value(); ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage( "string_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg, @@ -620,7 +621,8 @@ TEST_P(CreateCreateStructStepTest, TestSetInt64MapField) { auto cel_map = CreateContainerBackedMap(absl::Span>( - entries.data(), entries.size())); + entries.data(), entries.size())) + .value(); ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage( "int64_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg, @@ -647,7 +649,8 @@ TEST_P(CreateCreateStructStepTest, TestSetUInt64MapField) { auto cel_map = CreateContainerBackedMap(absl::Span>( - entries.data(), entries.size())); + entries.data(), entries.size())) + .value(); ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage( "uint64_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg, diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index ba6f0954b..329b0d746 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -147,7 +147,8 @@ TEST_P(SelectStepTest, MapPresenseIsFalseTest) { {CelValue::CreateString(&key1), CelValue::CreateInt64(1)}}; auto map_value = CreateContainerBackedMap( - absl::Span>(key_values)); + absl::Span>(key_values)) + .value(); google::protobuf::Arena arena; @@ -165,7 +166,8 @@ TEST_P(SelectStepTest, MapPresenseIsTrueTest) { {CelValue::CreateString(&key1), CelValue::CreateInt64(1)}}; auto map_value = CreateContainerBackedMap( - absl::Span>(key_values)); + absl::Span>(key_values)) + .value(); google::protobuf::Arena arena; @@ -185,7 +187,8 @@ TEST(SelectStepTest, MapPresenseIsTrueWithUnknownTest) { CelValue::CreateUnknownSet(&unknown_set)}}; auto map_value = CreateContainerBackedMap( - absl::Span>(key_values)); + absl::Span>(key_values)) + .value(); google::protobuf::Arena arena; @@ -427,7 +430,8 @@ TEST_P(SelectStepTest, MapSimpleInt32Test) { {CelValue::CreateString(&key2), CelValue::CreateInt64(2)}}; auto map_value = CreateContainerBackedMap( - absl::Span>(key_values)); + absl::Span>(key_values)) + .value(); google::protobuf::Arena arena; diff --git a/eval/public/containers/BUILD b/eval/public/containers/BUILD index cbe409d2e..d19572bfb 100644 --- a/eval/public/containers/BUILD +++ b/eval/public/containers/BUILD @@ -48,6 +48,8 @@ cc_library( deps = [ "//eval/public:cel_value", "@com_google_absl//absl/container:node_hash_map", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/types:span", ], ) diff --git a/eval/public/containers/container_backed_map_impl.cc b/eval/public/containers/container_backed_map_impl.cc index 5e05abbde..d948ed97e 100644 --- a/eval/public/containers/container_backed_map_impl.cc +++ b/eval/public/containers/container_backed_map_impl.cc @@ -3,6 +3,7 @@ #include "eval/public/containers/container_backed_map_impl.h" #include "absl/container/node_hash_map.h" +#include "absl/status/status.h" #include "absl/types/span.h" #include "eval/public/cel_value.h" @@ -102,14 +103,15 @@ class Equal { // type of key in STL map. class ContainerBackedMapImpl : public CelMap { public: - static std::unique_ptr Create( + static absl::StatusOr> Create( absl::Span> key_values) { auto cel_map = absl::WrapUnique(new ContainerBackedMapImpl()); - if (!cel_map->AddItems(key_values)) { - return nullptr; + auto status = cel_map->AddItems(key_values); + if (!status.ok()) { + return status; } - return std::move(cel_map); + return cel_map; } // Map size. @@ -141,17 +143,17 @@ class ContainerBackedMapImpl : public CelMap { ContainerBackedMapImpl() = default; - bool AddItems(absl::Span> key_values) { + absl::Status AddItems(absl::Span> key_values) { for (const auto& item : key_values) { auto result = values_map_.emplace(item.first, item.second); // Failed to insert pair into map - addition failed. if (!result.second) { - return false; + return absl::InvalidArgumentError("duplicate map keys"); } key_list_.Add(item.first); } - return true; + return absl::OkStatus(); } absl::node_hash_map values_map_; @@ -160,7 +162,7 @@ class ContainerBackedMapImpl : public CelMap { } // namespace -std::unique_ptr CreateContainerBackedMap( +absl::StatusOr> CreateContainerBackedMap( absl::Span> key_values) { return ContainerBackedMapImpl::Create(key_values); } diff --git a/eval/public/containers/container_backed_map_impl.h b/eval/public/containers/container_backed_map_impl.h index c89fd3d21..8865352e0 100644 --- a/eval/public/containers/container_backed_map_impl.h +++ b/eval/public/containers/container_backed_map_impl.h @@ -1,8 +1,12 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CONTAINERS_CONTAINER_BACKED_MAP_IMPL_H_ #define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CONTAINERS_CONTAINER_BACKED_MAP_IMPL_H_ -#include "eval/public/cel_value.h" +#include +#include + +#include "absl/status/statusor.h" #include "absl/types/span.h" +#include "eval/public/cel_value.h" namespace google { namespace api { @@ -10,7 +14,7 @@ namespace expr { namespace runtime { // Template factory method creating container-backed CelMap. -std::unique_ptr CreateContainerBackedMap( +absl::StatusOr> CreateContainerBackedMap( absl::Span> key_values); } // namespace runtime diff --git a/eval/public/containers/container_backed_map_impl_test.cc b/eval/public/containers/container_backed_map_impl_test.cc index d017ca893..c9e3af927 100644 --- a/eval/public/containers/container_backed_map_impl_test.cc +++ b/eval/public/containers/container_backed_map_impl_test.cc @@ -16,16 +16,18 @@ namespace runtime { namespace { using testing::Eq; -using testing::Not; using testing::IsNull; +using testing::Not; TEST(ContainerBackedMapImplTest, TestMapInt64) { std::vector> args = { {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(3)}}; - auto cel_map = CreateContainerBackedMap( - absl::Span>(args.data(), args.size())); + auto cel_map = + CreateContainerBackedMap( + absl::Span>(args.data(), args.size())) + .value(); ASSERT_THAT(cel_map, Not(IsNull())); @@ -56,8 +58,10 @@ TEST(ContainerBackedMapImplTest, TestMapUint64) { std::vector> args = { {CelValue::CreateUint64(1), CelValue::CreateInt64(2)}, {CelValue::CreateUint64(2), CelValue::CreateInt64(3)}}; - auto cel_map = CreateContainerBackedMap( - absl::Span>(args.data(), args.size())); + auto cel_map = + CreateContainerBackedMap( + absl::Span>(args.data(), args.size())) + .value(); ASSERT_THAT(cel_map, Not(IsNull())); @@ -92,8 +96,10 @@ TEST(ContainerBackedMapImplTest, TestMapString) { std::vector> args = { {CelValue::CreateString(&kKey1), CelValue::CreateInt64(2)}, {CelValue::CreateString(&kKey2), CelValue::CreateInt64(3)}}; - auto cel_map = CreateContainerBackedMap( - absl::Span>(args.data(), args.size())); + auto cel_map = + CreateContainerBackedMap( + absl::Span>(args.data(), args.size())) + .value(); ASSERT_THAT(cel_map, Not(IsNull())); diff --git a/eval/public/set_util_test.cc b/eval/public/set_util_test.cc index 0845ac86a..2a39821f7 100644 --- a/eval/public/set_util_test.cc +++ b/eval/public/set_util_test.cc @@ -331,19 +331,22 @@ TEST(CelValueLessThan, CelMapSameSize) { {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(6)}}; - auto cel_map_backing_1 = CreateContainerBackedMap(absl::MakeSpan(values)); + auto cel_map_backing_1 = + CreateContainerBackedMap(absl::MakeSpan(values)).value(); std::vector> values2{ {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(4), CelValue::CreateInt64(6)}}; - auto cel_map_backing_2 = CreateContainerBackedMap(absl::MakeSpan(values2)); + auto cel_map_backing_2 = + CreateContainerBackedMap(absl::MakeSpan(values2)).value(); std::vector> values3{ {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(8)}}; - auto cel_map_backing_3 = CreateContainerBackedMap(absl::MakeSpan(values3)); + auto cel_map_backing_3 = + CreateContainerBackedMap(absl::MakeSpan(values3)).value(); CelValue map1 = CelValue::CreateMap(cel_map_backing_1.get()); CelValue map2 = CelValue::CreateMap(cel_map_backing_2.get()); @@ -359,14 +362,14 @@ TEST(CelValueLessThan, CelMapDifferentSizes) { {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}}; - auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)); + auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)).value(); std::vector> values2{ {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(6)}}; - auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)); + auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)).value(); EXPECT_TRUE(CelValueLessThan(CelValue::CreateMap(cel_map_1.get()), CelValue::CreateMap(cel_map_2.get()))); @@ -378,14 +381,14 @@ TEST(CelValueLessThan, CelMapEqual) { {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(6)}}; - auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)); + auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)).value(); std::vector> values2{ {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(6)}}; - auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)); + auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)).value(); EXPECT_FALSE(CelValueLessThan(CelValue::CreateMap(cel_map_1.get()), CelValue::CreateMap(cel_map_2.get()))); @@ -418,7 +421,7 @@ TEST(CelValueLessThan, CelMapSupportProtoMapCompatible) { {CelValue::CreateStringView(kFields[1]), CelValue::CreateDouble(1.0)}, {CelValue::CreateStringView(kFields[0]), CelValue::CreateBool(true)}}; - auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)); + auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)).value(); CelValue cel_map = CelValue::CreateMap(backing_map.get()); @@ -451,7 +454,7 @@ TEST(CelValueLessThan, NestedMap) { std::vector> values{ {CelValue::CreateStringView("field"), cel_list}}; - auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)); + auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)).value(); CelValue cel_map = CelValue::CreateMap(backing_map.get()); CelValue proto_map = CelProtoWrapper::CreateMessage(&value_struct, &arena); diff --git a/eval/public/structs/cel_proto_wrapper_test.cc b/eval/public/structs/cel_proto_wrapper_test.cc index a93f28b61..2ac55345c 100644 --- a/eval/public/structs/cel_proto_wrapper_test.cc +++ b/eval/public/structs/cel_proto_wrapper_test.cc @@ -750,8 +750,10 @@ TEST_F(CelProtoWrapperTest, WrapStruct) { std::vector> args = { {CelValue::CreateString(CelValue::StringHolder(&kField1)), CelValue::CreateBool(true)}}; - auto cel_map = CreateContainerBackedMap( - absl::Span>(args.data(), args.size())); + auto cel_map = + CreateContainerBackedMap( + absl::Span>(args.data(), args.size())) + .value(); auto cel_value = CelValue::CreateMap(cel_map.get()); Value json; @@ -768,8 +770,10 @@ TEST_F(CelProtoWrapperTest, WrapStruct) { TEST_F(CelProtoWrapperTest, WrapFailureStructBadKeyType) { std::vector> args = { {CelValue::CreateInt64(1L), CelValue::CreateBool(true)}}; - auto cel_map = CreateContainerBackedMap( - absl::Span>(args.data(), args.size())); + auto cel_map = + CreateContainerBackedMap( + absl::Span>(args.data(), args.size())) + .value(); auto cel_value = CelValue::CreateMap(cel_map.get()); Value json; @@ -782,8 +786,10 @@ TEST_F(CelProtoWrapperTest, WrapFailureStructBadValueType) { std::vector> args = { {CelValue::CreateString(CelValue::StringHolder(&kField1)), CelProtoWrapper::CreateMessage(&bad_value, arena())}}; - auto cel_map = CreateContainerBackedMap( - absl::Span>(args.data(), args.size())); + auto cel_map = + CreateContainerBackedMap( + absl::Span>(args.data(), args.size())) + .value(); auto cel_value = CelValue::CreateMap(cel_map.get()); Value json; ExpectNotWrapped(cel_value, json); diff --git a/eval/public/transform_utility.cc b/eval/public/transform_utility.cc index 7081170c6..4509868cb 100644 --- a/eval/public/transform_utility.cc +++ b/eval/public/transform_utility.cc @@ -146,12 +146,13 @@ absl::StatusOr ValueToCelValue(const Value& value, ASSIGN_OR_RETURN(auto map_value, ValueToCelValue(entry.value(), arena)); key_values.push_back(std::pair(map_key, map_value)); } - auto cel_map = + ASSIGN_OR_RETURN( + auto cel_map, CreateContainerBackedMap(absl::Span>( - key_values.data(), key_values.size())) - .release(); - arena->Own(cel_map); - return CelValue::CreateMap(cel_map); + key_values.data(), key_values.size()))); + auto* cel_map_ptr = cel_map.release(); + arena->Own(cel_map_ptr); + return CelValue::CreateMap(cel_map_ptr); } case Value::kNullValue: return CelValue::CreateNull(); diff --git a/eval/public/unknown_function_result_set_test.cc b/eval/public/unknown_function_result_set_test.cc index 022ec19d0..c8e6a65fc 100644 --- a/eval/public/unknown_function_result_set_test.cc +++ b/eval/public/unknown_function_result_set_test.cc @@ -183,8 +183,8 @@ TEST(UnknownFunctionResult, MapsEqual) { {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}}; - auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)); - auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values)); + auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)).value(); + auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values)).value(); CelFunctionDescriptor desc("OneMap", false, {CelValue::Type::kMap}); @@ -202,14 +202,14 @@ TEST(UnknownFunctionResult, MapsDifferentSizes) { {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}}; - auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)); + auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)).value(); std::vector> values2{ {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(6)}}; - auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)); + auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)).value(); CelFunctionDescriptor desc("OneMap", false, {CelValue::Type::kMap}); @@ -229,21 +229,21 @@ TEST(UnknownFunctionResult, MapsDifferentElements) { {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(6)}}; - auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)); + auto cel_map_1 = CreateContainerBackedMap(absl::MakeSpan(values)).value(); std::vector> values2{ {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}, {CelValue::CreateInt64(4), CelValue::CreateInt64(8)}}; - auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)); + auto cel_map_2 = CreateContainerBackedMap(absl::MakeSpan(values2)).value(); std::vector> values3{ {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}, {CelValue::CreateInt64(2), CelValue::CreateInt64(4)}, {CelValue::CreateInt64(3), CelValue::CreateInt64(5)}}; - auto cel_map_3 = CreateContainerBackedMap(absl::MakeSpan(values3)); + auto cel_map_3 = CreateContainerBackedMap(absl::MakeSpan(values3)).value(); CelFunctionDescriptor desc("OneMap", false, {CelValue::Type::kMap}); @@ -387,7 +387,7 @@ TEST(UnknownFunctionResult, ProtoStructTreatedAsMap) { {CelValue::CreateStringView(kFields[1]), CelValue::CreateDouble(1.0)}, {CelValue::CreateStringView(kFields[0]), CelValue::CreateBool(true)}}; - auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)); + auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)).value(); CelValue cel_map = CelValue::CreateMap(backing_map.get()); @@ -455,7 +455,7 @@ TEST(UnknownFunctionResult, NestedProtoTypes) { std::vector> values{ {CelValue::CreateStringView("field"), cel_list}}; - auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)); + auto backing_map = CreateContainerBackedMap(absl::MakeSpan(values)).value(); CelValue cel_map = CelValue::CreateMap(backing_map.get()); CelValue proto_map = CelProtoWrapper::CreateMessage(&value_struct, &arena); diff --git a/eval/public/value_export_util_test.cc b/eval/public/value_export_util_test.cc index 86ebfa297..f88d4270d 100644 --- a/eval/public/value_export_util_test.cc +++ b/eval/public/value_export_util_test.cc @@ -298,7 +298,8 @@ TEST(ValueExportUtilTest, ConvertCelMapWithStringKey) { {CelValue::CreateString(&key2), CelValue::CreateString(&value2)}); auto cel_map = CreateContainerBackedMap( - absl::Span>(map_entries)); + absl::Span>(map_entries)) + .value(); CelValue cel_value = CelValue::CreateMap(cel_map.get()); EXPECT_OK(ExportAsProtoValue(cel_value, &value)); @@ -325,7 +326,8 @@ TEST(ValueExportUtilTest, ConvertCelMapWithInt64Key) { {CelValue::CreateInt64(key2), CelValue::CreateString(&value2)}); auto cel_map = CreateContainerBackedMap( - absl::Span>(map_entries)); + absl::Span>(map_entries)) + .value(); CelValue cel_value = CelValue::CreateMap(cel_map.get()); EXPECT_OK(ExportAsProtoValue(cel_value, &value)); diff --git a/eval/tests/unknowns_end_to_end_test.cc b/eval/tests/unknowns_end_to_end_test.cc index e709669b2..c698ac1a6 100644 --- a/eval/tests/unknowns_end_to_end_test.cc +++ b/eval/tests/unknowns_end_to_end_test.cc @@ -742,7 +742,7 @@ TEST(UnknownsIterAttrTest, IterAttributeTrailMapKeyTypes) { {CelValue::CreateError(&error), CelValue::CreateBool(false)}); backing.push_back({CelValue::CreateBool(true), CelValue::CreateBool(false)}); - auto map_impl = CreateContainerBackedMap(absl::MakeSpan(backing)); + auto map_impl = CreateContainerBackedMap(absl::MakeSpan(backing)).value(); options.unknown_processing = UnknownProcessingOptions::kAttributeAndFunction; auto builder = CreateCelExpressionBuilder(options); @@ -784,7 +784,7 @@ TEST(UnknownsIterAttrTest, IterAttributeTrailMapKeyTypesShortcutted) { {CelValue::CreateError(&error), CelValue::CreateBool(false)}); backing.push_back({CelValue::CreateBool(true), CelValue::CreateBool(false)}); - auto map_impl = CreateContainerBackedMap(absl::MakeSpan(backing)); + auto map_impl = CreateContainerBackedMap(absl::MakeSpan(backing)).value(); options.unknown_processing = UnknownProcessingOptions::kAttributeAndFunction; auto builder = CreateCelExpressionBuilder(options); From 683c2443f76c40fabb0594dba8ece2dcad3bcbed Mon Sep 17 00:00:00 2001 From: tswadell Date: Tue, 13 Apr 2021 12:59:54 -0400 Subject: [PATCH 03/12] Add microbenchmarks for has() macro. PiperOrigin-RevId: 368237226 --- eval/tests/BUILD | 1 + eval/tests/benchmark_test.cc | 86 ++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/eval/tests/BUILD b/eval/tests/BUILD index 53f0206cb..ca597d6f8 100644 --- a/eval/tests/BUILD +++ b/eval/tests/BUILD @@ -24,6 +24,7 @@ cc_test( "//eval/public:cel_expression", "//eval/public:cel_value", "//eval/public/containers:container_backed_list_impl", + "//eval/public/containers:container_backed_map_impl", "//eval/public/structs:cel_proto_wrapper", "@com_github_google_benchmark//:benchmark", "@com_github_google_benchmark//:benchmark_main", diff --git a/eval/tests/benchmark_test.cc b/eval/tests/benchmark_test.cc index 70db54653..1d6772ec7 100644 --- a/eval/tests/benchmark_test.cc +++ b/eval/tests/benchmark_test.cc @@ -13,6 +13,7 @@ #include "eval/public/cel_expression.h" #include "eval/public/cel_value.h" #include "eval/public/containers/container_backed_list_impl.h" +#include "eval/public/containers/container_backed_map_impl.h" #include "eval/public/structs/cel_proto_wrapper.h" #include "eval/tests/request_context.pb.h" #include "base/status_macros.h" @@ -827,6 +828,91 @@ void BM_Comprehension(benchmark::State& state) { BENCHMARK(BM_Comprehension)->Range(1, 1 << 20); +// has(request.path) && !has(request.ip) +constexpr char kHas[] = R"( +call_expr: < + function: "_&&_" + args: < + select_expr: < + operand: < + ident_expr: < + name: "request" + > + > + field: "path" + test_only: true + > + > + args: < + call_expr: < + function: "!_" + args: < + select_expr: < + operand: < + ident_expr: < + name: "request" + > + > + field: "ip" + test_only: true + > + > + > + > +>)"; + +void BM_HasMap(benchmark::State& state) { + google::protobuf::Arena arena; + Expr expr; + Activation activation; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kHas, &expr)); + auto builder = CreateCelExpressionBuilder(); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + auto expr_plan = builder->CreateExpression(&expr, nullptr); + + std::vector> map_pairs{ + {CelValue::CreateStringView("path"), CelValue::CreateStringView("path")}}; + auto cel_map = + CreateContainerBackedMap(absl::Span>( + map_pairs.data(), map_pairs.size())); + + activation.InsertValue("request", CelValue::CreateMap((*cel_map).get())); + ASSERT_OK(expr_plan.status()); + for (auto _ : state) { + auto result = expr_plan.value()->Evaluate(activation, &arena); + ASSERT_OK(result.status()); + ASSERT_TRUE(result->IsBool()); + ASSERT_TRUE(result->BoolOrDie()); + } +} + +BENCHMARK(BM_HasMap); + +void BM_HasProto(benchmark::State& state) { + google::protobuf::Arena arena; + Expr expr; + Activation activation; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kHas, &expr)); + auto builder = CreateCelExpressionBuilder(); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + auto expr_plan = builder->CreateExpression(&expr, nullptr); + + RequestContext request; + request.set_path(kPath); + request.set_token(kToken); + activation.InsertValue("request", + CelProtoWrapper::CreateMessage(&request, &arena)); + ASSERT_OK(expr_plan.status()); + for (auto _ : state) { + auto result = expr_plan.value()->Evaluate(activation, &arena); + ASSERT_OK(result.status()); + ASSERT_TRUE(result->IsBool()); + ASSERT_TRUE(result->BoolOrDie()); + } +} + +BENCHMARK(BM_HasProto); + // Sum a square with a nested comprehension constexpr char kNestedListSum[] = R"( id: 1 From ac181d3d5afb4732ed60a3bccfcba2270eada5ad Mon Sep 17 00:00:00 2001 From: tswadell Date: Wed, 14 Apr 2021 20:56:13 -0400 Subject: [PATCH 04/12] Internal tests PiperOrigin-RevId: 368545070 --- eval/tests/benchmark_test.cc | 117 +++++++++++++++++++++++++++++++ eval/tests/request_context.proto | 1 + 2 files changed, 118 insertions(+) diff --git a/eval/tests/benchmark_test.cc b/eval/tests/benchmark_test.cc index 1d6772ec7..786e5e764 100644 --- a/eval/tests/benchmark_test.cc +++ b/eval/tests/benchmark_test.cc @@ -913,6 +913,123 @@ void BM_HasProto(benchmark::State& state) { BENCHMARK(BM_HasProto); +// has(request.headers.create_time) && !has(request.headers.update_time) +constexpr char kHasProtoMap[] = R"( +call_expr: < + function: "_&&_" + args: < + select_expr: < + operand: < + select_expr: < + operand: < + ident_expr: < + name: "request" + > + > + field: "headers" + > + > + field: "create_time" + test_only: true + > + > + args: < + call_expr: < + function: "!_" + args: < + select_expr: < + operand: < + select_expr: < + operand: < + ident_expr: < + name: "request" + > + > + field: "headers" + > + > + field: "update_time" + test_only: true + > + > + > + > +>)"; + +void BM_HasProtoMap(benchmark::State& state) { + google::protobuf::Arena arena; + Expr expr; + Activation activation; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kHasProtoMap, &expr)); + auto builder = CreateCelExpressionBuilder(); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + auto expr_plan = builder->CreateExpression(&expr, nullptr); + + RequestContext request; + request.mutable_headers()->insert({"create_time", "2021-01-01"}); + activation.InsertValue("request", + CelProtoWrapper::CreateMessage(&request, &arena)); + ASSERT_OK(expr_plan.status()); + for (auto _ : state) { + auto result = expr_plan.value()->Evaluate(activation, &arena); + ASSERT_OK(result.status()); + ASSERT_TRUE(result->IsBool()); + ASSERT_TRUE(result->BoolOrDie()); + } +} + +BENCHMARK(BM_HasProtoMap); + +// has(request.headers.create_time) && !has(request.headers.update_time) +constexpr char kReadProtoMap[] = R"( +call_expr: < + function: "_==_" + args: < + select_expr: < + operand: < + select_expr: < + operand: < + ident_expr: < + name: "request" + > + > + field: "headers" + > + > + field: "create_time" + > + > + args: < + const_expr: < + string_value: "2021-01-01" + > + > +>)"; + +void BM_ReadProtoMap(benchmark::State& state) { + google::protobuf::Arena arena; + Expr expr; + Activation activation; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kReadProtoMap, &expr)); + auto builder = CreateCelExpressionBuilder(); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + auto expr_plan = builder->CreateExpression(&expr, nullptr); + + RequestContext request; + request.mutable_headers()->insert({"create_time", "2021-01-01"}); + activation.InsertValue("request", + CelProtoWrapper::CreateMessage(&request, &arena)); + ASSERT_OK(expr_plan.status()); + for (auto _ : state) { + auto result = expr_plan.value()->Evaluate(activation, &arena); + ASSERT_OK(result.status()); + ASSERT_TRUE(result->IsBool()); + ASSERT_TRUE(result->BoolOrDie()); + } +} + +BENCHMARK(BM_ReadProtoMap); + // Sum a square with a nested comprehension constexpr char kNestedListSum[] = R"( id: 1 diff --git a/eval/tests/request_context.proto b/eval/tests/request_context.proto index 9e771cfed..2e307d3b1 100644 --- a/eval/tests/request_context.proto +++ b/eval/tests/request_context.proto @@ -9,4 +9,5 @@ message RequestContext { string ip = 1; string path = 2; string token = 3; + map headers = 4; } From d20e1ef3b080ce402d93fa4155b771ecb1e4daea Mon Sep 17 00:00:00 2001 From: tswadell Date: Fri, 16 Apr 2021 19:02:43 -0400 Subject: [PATCH 05/12] Lower the size of the input expression to 100k codepoints Also, introduce `ParserOptions` to encapsulate limit and behavior settings. PiperOrigin-RevId: 368935535 --- parser/BUILD | 8 ++++++++ parser/options.h | 35 +++++++++++++++++++++++++++++++++++ parser/parser.cc | 29 ++++++++++++++++------------- parser/parser.h | 13 ++++--------- parser/parser_test.cc | 15 ++++++++++++++- 5 files changed, 77 insertions(+), 23 deletions(-) create mode 100644 parser/options.h diff --git a/parser/BUILD b/parser/BUILD index 23e382c8b..ad165b92e 100644 --- a/parser/BUILD +++ b/parser/BUILD @@ -25,9 +25,11 @@ cc_library( deps = [ ":cel_cc_parser", ":macro", + ":options", ":source_factory", ":visitor", "@antlr4_runtimes//:cpp", + "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", @@ -108,10 +110,16 @@ cc_library( ], ) +cc_library( + name = "options", + hdrs = ["options.h"], +) + cc_test( name = "parser_test", srcs = ["parser_test.cc"], deps = [ + ":options", ":parser", ":source_factory", "//testutil:expr_printer", diff --git a/parser/options.h b/parser/options.h new file mode 100644 index 000000000..8726030f0 --- /dev/null +++ b/parser/options.h @@ -0,0 +1,35 @@ +#ifndef THIRD_PARTY_CEL_CPP_PARSER_OPTIONS_H_ +#define THIRD_PARTY_CEL_CPP_PARSER_OPTIONS_H_ + +namespace google { +namespace api { +namespace expr { +namespace parser { + +constexpr int kDefaultErrorRecoveryLimit = 30; +constexpr int kDefaultMaxRecursionDepth = 250; +constexpr int kExpressionSizeCodepointLimit = 100'000; + +// Options for configuring the limits and features of the parser. +struct ParserOptions { + // Limit of the number of error recovery attempts made by the ANTLR parser + // when processing an input. This limit, when reached, will halt further + // parsing of the expression. + int error_recovery_limit = kDefaultErrorRecoveryLimit; + + // Limit on the amount of recusive parse instructions permitted when building + // the abstract syntax tree for the expression. This prevents pathological + // inputs from causing stack overflows. + int max_recursion_depth = kDefaultMaxRecursionDepth; + + // Limit on the number of codepoints in the input string which the parser will + // attempt to parse. + int expression_size_codepoint_limit = kExpressionSizeCodepointLimit; +}; + +} // namespace parser +} // namespace expr +} // namespace api +} // namespace google + +#endif // THIRD_PARTY_CEL_CPP_PARSER_OPTIONS_H_ diff --git a/parser/parser.cc b/parser/parser.cc index 9fb31e118..52b09e43a 100644 --- a/parser/parser.cc +++ b/parser/parser.cc @@ -1,11 +1,13 @@ #include "parser/parser.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_format.h" #include "absl/strings/str_replace.h" #include "absl/types/optional.h" #include "parser/cel_grammar.inc/cel_grammar/CelLexer.h" #include "parser/cel_grammar.inc/cel_grammar/CelParser.h" +#include "parser/options.h" #include "parser/source_factory.h" #include "parser/visitor.h" #include "antlr4-runtime.h" @@ -126,19 +128,15 @@ class RecoveryLimitErrorStrategy : public antlr4::DefaultErrorStrategy { absl::StatusOr Parse(const std::string& expression, const std::string& description, - int max_recursion_depth, - int error_recovery_limit) { - return ParseWithMacros(expression, Macro::AllMacros(), description, - max_recursion_depth, error_recovery_limit); + const ParserOptions& options) { + return ParseWithMacros(expression, Macro::AllMacros(), description, options); } absl::StatusOr ParseWithMacros(const std::string& expression, const std::vector& macros, const std::string& description, - int max_recursion_depth, - int error_recovery_limit) { - auto result = EnrichedParse(expression, macros, description, - max_recursion_depth, error_recovery_limit); + const ParserOptions& options) { + auto result = EnrichedParse(expression, macros, description, options); if (result.ok()) { return result->parsed_expr(); } @@ -147,14 +145,19 @@ absl::StatusOr ParseWithMacros(const std::string& expression, absl::StatusOr EnrichedParse( const std::string& expression, const std::vector& macros, - const std::string& description, int max_recursion_depth, - int error_recovery_limit) { + const std::string& description, const ParserOptions& options) { ANTLRInputStream input(expression); + if (input.size() > options.expression_size_codepoint_limit) { + return absl::InvalidArgumentError(absl::StrCat( + "expression size exceeds codepoint limit.", " input size: ", + input.size(), ", limit: ", options.expression_size_codepoint_limit)); + } CelLexer lexer(&input); CommonTokenStream tokens(&lexer); CelParser parser(&tokens); - ExprRecursionListener listener(max_recursion_depth); - ParserVisitor visitor(description, expression, max_recursion_depth, macros); + ExprRecursionListener listener(options.max_recursion_depth); + ParserVisitor visitor(description, expression, options.max_recursion_depth, + macros); lexer.removeErrorListeners(); parser.removeErrorListeners(); @@ -165,7 +168,7 @@ absl::StatusOr EnrichedParse( // Limit the number of error recovery attempts to prevent bad expressions // from consuming lots of cpu / memory. std::shared_ptr error_strategy( - new RecoveryLimitErrorStrategy(error_recovery_limit)); + new RecoveryLimitErrorStrategy(options.error_recovery_limit)); parser.setErrorHandler(error_strategy); CelParser::StartContext* root; diff --git a/parser/parser.h b/parser/parser.h index 70edd5848..ab628d81b 100644 --- a/parser/parser.h +++ b/parser/parser.h @@ -5,6 +5,7 @@ #include "absl/status/statusor.h" #include "absl/types/optional.h" #include "parser/macro.h" +#include "parser/options.h" #include "parser/source_factory.h" namespace google { @@ -12,9 +13,6 @@ namespace api { namespace expr { namespace parser { -constexpr int kDefaultErrorRecoveryLimit = 30; -constexpr int kDefaultMaxRecursionDepth = 250; - class VerboseParsedExpr { public: VerboseParsedExpr(google::api::expr::v1alpha1::ParsedExpr parsed_expr, @@ -37,19 +35,16 @@ class VerboseParsedExpr { absl::StatusOr EnrichedParse( const std::string& expression, const std::vector& macros, const std::string& description = "", - int max_recursion_depth = kDefaultMaxRecursionDepth, - int error_recovery_limit = kDefaultErrorRecoveryLimit); + const ParserOptions& options = ParserOptions()); absl::StatusOr Parse( const std::string& expression, const std::string& description = "", - int max_recursion_depth = kDefaultMaxRecursionDepth, - int error_recovery_limit = kDefaultErrorRecoveryLimit); + const ParserOptions& options = ParserOptions()); absl::StatusOr ParseWithMacros( const std::string& expression, const std::vector& macros, const std::string& description = "", - int max_recursion_depth = kDefaultMaxRecursionDepth, - int error_recovery_limit = kDefaultErrorRecoveryLimit); + const ParserOptions& options = ParserOptions()); } // namespace parser } // namespace expr diff --git a/parser/parser_test.cc b/parser/parser_test.cc index 1b9c3ca2c..9635f0d9d 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -11,6 +11,7 @@ #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" #include "absl/types/optional.h" +#include "parser/options.h" #include "parser/source_factory.h" #include "testutil/expr_printer.h" @@ -998,7 +999,9 @@ TEST(ExpressionTest, TsanOom) { } TEST(ExpressionTest, ErrorRecoveryLimits) { - auto result = Parse("......", "", kDefaultMaxRecursionDepth, 1); + ParserOptions options; + options.error_recovery_limit = 1; + auto result = Parse("......", "", options); EXPECT_FALSE(result.ok()); EXPECT_EQ(result.status().message(), "ERROR: :1:2: Syntax error: missing IDENTIFIER at '.'\n" @@ -1009,6 +1012,16 @@ TEST(ExpressionTest, ErrorRecoveryLimits) { " | ..^"); } +TEST(ExpressionTest, ExpressionSizeLimit) { + ParserOptions options; + options.expression_size_codepoint_limit = 10; + auto result = Parse("...............", "", options); + EXPECT_FALSE(result.ok()); + EXPECT_EQ( + result.status().message(), + "expression size exceeds codepoint limit. input size: 15, limit: 10"); +} + INSTANTIATE_TEST_SUITE_P(CelParserTest, ExpressionTest, testing::ValuesIn(test_cases)); From 78cf511e2acb8ca6408d8b23af23caa702196bf4 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Thu, 29 Apr 2021 16:40:39 -0400 Subject: [PATCH 06/12] Internal change. PiperOrigin-RevId: 371194131 --- eval/public/containers/BUILD | 1 + eval/public/containers/container_backed_map_impl.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/eval/public/containers/BUILD b/eval/public/containers/BUILD index d19572bfb..f0c52114f 100644 --- a/eval/public/containers/BUILD +++ b/eval/public/containers/BUILD @@ -48,6 +48,7 @@ cc_library( deps = [ "//eval/public:cel_value", "@com_google_absl//absl/container:node_hash_map", + "@com_google_absl//absl/hash", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/types:span", diff --git a/eval/public/containers/container_backed_map_impl.cc b/eval/public/containers/container_backed_map_impl.cc index d948ed97e..2128b71d6 100644 --- a/eval/public/containers/container_backed_map_impl.cc +++ b/eval/public/containers/container_backed_map_impl.cc @@ -3,6 +3,7 @@ #include "eval/public/containers/container_backed_map_impl.h" #include "absl/container/node_hash_map.h" +#include "absl/hash/hash.h" #include "absl/status/status.h" #include "absl/types/span.h" #include "eval/public/cel_value.h" From bdf9939de08e2fa1cbcae5ad9ddb75ab258ee226 Mon Sep 17 00:00:00 2001 From: tswadell Date: Mon, 17 May 2021 17:26:54 -0400 Subject: [PATCH 07/12] Documentation change PiperOrigin-RevId: 374280421 --- eval/tests/benchmark_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eval/tests/benchmark_test.cc b/eval/tests/benchmark_test.cc index 786e5e764..737abf5bb 100644 --- a/eval/tests/benchmark_test.cc +++ b/eval/tests/benchmark_test.cc @@ -980,7 +980,7 @@ void BM_HasProtoMap(benchmark::State& state) { BENCHMARK(BM_HasProtoMap); -// has(request.headers.create_time) && !has(request.headers.update_time) +// request.headers.create_time == "2021-01-01" constexpr char kReadProtoMap[] = R"( call_expr: < function: "_==_" From e972c4b6dd3b3b09c18f42de2c2067acde696ca5 Mon Sep 17 00:00:00 2001 From: tswadell Date: Mon, 17 May 2021 19:37:02 -0400 Subject: [PATCH 08/12] Introduce new `Has` method for presence testing on `CelMap` values. PiperOrigin-RevId: 374306406 --- conformance/BUILD | 2 - eval/eval/BUILD | 1 + eval/eval/select_step.cc | 19 +- eval/eval/select_step_test.cc | 39 ++- eval/public/BUILD | 1 + eval/public/builtin_func_registrar.cc | 312 ++++++------------ eval/public/builtin_func_test.cc | 74 ++++- eval/public/cel_value.h | 32 +- eval/public/containers/BUILD | 4 + .../containers/container_backed_map_impl.cc | 11 +- .../containers/field_backed_map_impl.cc | 244 +++++++++----- .../public/containers/field_backed_map_impl.h | 20 ++ .../containers/field_backed_map_impl_test.cc | 154 ++++++++- eval/public/structs/BUILD | 1 + eval/public/structs/cel_proto_wrapper.cc | 20 +- eval/public/structs/cel_proto_wrapper_test.cc | 33 +- eval/testutil/test_message.proto | 2 + tools/BUILD | 4 + tools/flatbuffers_backed_impl.cc | 64 +++- tools/flatbuffers_backed_impl.h | 15 +- tools/flatbuffers_backed_impl_test.cc | 104 +++++- 21 files changed, 796 insertions(+), 360 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 8592cec0c..ca49c2744 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -85,8 +85,6 @@ cc_binary( # TODO(issues/114): Ensure the 'in' operator is a logical OR of element equality results. "--skip_test=comparisons/in_list_literal/elem_in_mixed_type_list_error", "--skip_test=comparisons/in_map_literal/key_in_mixed_key_type_map_error", - # TODO(issues/115): The 'in' operator fails with maps containing boolean keys. - "--skip_test=fields/in/singleton", # TODO(issues/97): Parse-only qualified variable lookup "x.y" wtih binding "x.y" or "y" within container "x" fails "--skip_test=fields/qualified_identifier_resolution/qualified_ident,map_field_select,ident_with_longest_prefix_check,qualified_identifier_resolution_unchecked", "--skip_test=namespace/qualified/self_eval_qualified_lookup", diff --git a/eval/eval/BUILD b/eval/eval/BUILD index a2cb71025..68cdf8f4b 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -419,6 +419,7 @@ cc_test( "//eval/public/structs:cel_proto_wrapper", "//eval/testutil:test_message_cc_proto", "//testutil:util", + "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", "@com_google_googletest//:gtest_main", diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index cc83b4167..71de4ea08 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -1,5 +1,6 @@ #include "eval/eval/select_step.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "eval/eval/evaluator_core.h" @@ -155,7 +156,6 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { } absl::Status status = CreateValueFromField(msg, frame->arena(), &result); - if (status.ok()) { frame->value_stack().PopAndPush(result, result_trail); } @@ -178,15 +178,23 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { return absl::OkStatus(); } - auto lookup_result = (*cel_map)[CelValue::CreateString(&field_)]; - - // Test only Select expression. + CelValue field_name = CelValue::CreateString(&field_); if (test_field_presence_) { - result = CelValue::CreateBool(lookup_result.has_value()); + // Field presence only supports string keys containing valid identifier + // characters. + auto presence = cel_map->Has(field_name); + if (!presence.ok()) { + CelValue error_value = + CreateErrorValue(frame->arena(), presence.status()); + frame->value_stack().PopAndPush(error_value); + return absl::OkStatus(); + } + result = CelValue::CreateBool(*presence); frame->value_stack().PopAndPush(result); return absl::OkStatus(); } + auto lookup_result = (*cel_map)[field_name]; if (frame->enable_unknowns()) { result_trail = trail.Step(&field_, frame->arena()); if (frame->attribute_utility().CheckForUnknown(result_trail, false)) { @@ -205,7 +213,6 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { result = CreateNoSuchKeyError(frame->arena(), field_); } frame->value_stack().PopAndPush(result, result_trail); - return absl::OkStatus(); } case CelValue::Type::kUnknownSet: { diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index 329b0d746..6c89175af 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -3,6 +3,7 @@ #include "google/api/expr/v1alpha1/syntax.pb.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "eval/eval/ident_step.h" #include "eval/public/cel_attribute.h" @@ -32,7 +33,6 @@ absl::StatusOr RunExpression(const CelValue target, absl::string_view unknown_path, bool enable_unknowns) { ExecutionPath path; - Expr dummy_expr; auto select = dummy_expr.mutable_select_expr(); @@ -179,6 +179,43 @@ TEST_P(SelectStepTest, MapPresenseIsTrueTest) { EXPECT_EQ(result.BoolOrDie(), true); } +TEST(SelectStepTest, MapPresenseIsErrorTest) { + TestMessage message; + google::protobuf::Arena arena; + + Expr select_expr; + auto select = select_expr.mutable_select_expr(); + select->set_field("1"); + select->set_test_only(true); + Expr* expr1 = select->mutable_operand(); + auto select_map = expr1->mutable_select_expr(); + select_map->set_field("int32_int32_map"); + Expr* expr0 = select_map->mutable_operand(); + auto ident = expr0->mutable_ident_expr(); + ident->set_name("target"); + + auto step0_status = CreateIdentStep(ident, expr0->id()); + auto step1_status = CreateSelectStep(select_map, expr1->id(), ""); + auto step2_status = CreateSelectStep(select, select_expr.id(), ""); + ASSERT_OK(step0_status); + ASSERT_OK(step1_status); + ASSERT_OK(step2_status); + + ExecutionPath path; + path.push_back(std::move(step0_status.value())); + path.push_back(std::move(step1_status.value())); + path.push_back(std::move(step2_status.value())); + CelExpressionFlatImpl cel_expr(&select_expr, std::move(path), 0, {}, false); + Activation activation; + activation.InsertValue("target", + CelProtoWrapper::CreateMessage(&message, &arena)); + auto status = cel_expr.Evaluate(activation, &arena); + CelValue result = status.value(); + + EXPECT_TRUE(result.IsError()); + EXPECT_EQ(result.ErrorOrDie()->code(), absl::StatusCode::kInvalidArgument); +} + TEST(SelectStepTest, MapPresenseIsTrueWithUnknownTest) { UnknownSet unknown_set; std::string key1 = "key1"; diff --git a/eval/public/BUILD b/eval/public/BUILD index d3a25becd..f2144960d 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -25,6 +25,7 @@ cc_library( deps = [ ":cel_value_internal", "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", "@com_google_absl//absl/time", diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index fd0e574b8..b0c2c5660 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -3,6 +3,7 @@ #include #include #include +#include #include "absl/numeric/int128.h" #include "absl/status/status.h" @@ -10,6 +11,7 @@ #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_replace.h" +#include "absl/strings/string_view.h" #include "absl/time/time.h" #include "eval/public/cel_builtins.h" #include "eval/public/cel_function_adapter.h" @@ -321,11 +323,8 @@ absl::Status RegisterComparisonFunctionsForType(CelFunctionRegistry* registry) { if (!status.ok()) return status; // Greater than or Equal - status = FunctionAdapter::CreateAndRegister( + return FunctionAdapter::CreateAndRegister( builtin::kGreaterOrEqual, false, GreaterThanOrEqual, registry); - if (!status.ok()) return status; - - return absl::OkStatus(); } // Template functions providing arithmetic operations @@ -638,7 +637,7 @@ CelValue GetTimeBreakdownPart( auto status = FindTimeBreakdown(timestamp, tz, &breakdown); if (!status.ok()) { - return CreateErrorValue(arena, status.message()); + return CreateErrorValue(arena, status); } return extractor_func(breakdown); @@ -815,9 +814,95 @@ absl::Status RegisterComparisonFunctions(CelFunctionRegistry* registry, status = RegisterEqualityFunctionsForType(registry); if (!status.ok()) return status; - status = RegisterEqualityFunctionsForType(registry); - if (!status.ok()) return status; + return RegisterEqualityFunctionsForType(registry); +} + +absl::Status RegisterSetMembershipFunctions(CelFunctionRegistry* registry, + const InterpreterOptions& options) { + constexpr std::array in_operators = { + builtin::kIn, // @in for map and list types. + builtin::kInFunction, // deprecated in() -- for backwards compat + builtin::kInDeprecated, // deprecated _in_ -- for backwards compat + }; + + if (options.enable_list_contains) { + for (absl::string_view op : in_operators) { + auto status = + FunctionAdapter::CreateAndRegister( + op, false, In, registry); + if (!status.ok()) return status; + status = FunctionAdapter::CreateAndRegister( + op, false, In, registry); + if (!status.ok()) return status; + status = FunctionAdapter::CreateAndRegister( + op, false, In, registry); + if (!status.ok()) return status; + status = FunctionAdapter::CreateAndRegister( + op, false, In, registry); + if (!status.ok()) return status; + status = FunctionAdapter:: + CreateAndRegister(op, false, In, registry); + if (!status.ok()) return status; + status = FunctionAdapter:: + CreateAndRegister(op, false, In, registry); + if (!status.ok()) return status; + } + } + + auto boolKeyInSet = [](Arena* arena, bool key, + const CelMap* cel_map) -> CelValue { + const auto& result = cel_map->Has(CelValue::CreateBool(key)); + if (!result.ok()) { + return CreateErrorValue(arena, result.status()); + } + return CelValue::CreateBool(*result); + }; + auto intKeyInSet = [](Arena* arena, int64_t key, + const CelMap* cel_map) -> CelValue { + const auto& result = cel_map->Has(CelValue::CreateInt64(key)); + if (!result.ok()) { + return CreateErrorValue(arena, result.status()); + } + return CelValue::CreateBool(*result); + }; + auto stringKeyInSet = [](Arena* arena, CelValue::StringHolder key, + const CelMap* cel_map) -> CelValue { + const auto& result = cel_map->Has(CelValue::CreateString(key)); + if (!result.ok()) { + return CreateErrorValue(arena, result.status()); + } + return CelValue::CreateBool(*result); + }; + auto uintKeyInSet = [](Arena* arena, uint64_t key, + const CelMap* cel_map) -> CelValue { + const auto& result = cel_map->Has(CelValue::CreateUint64(key)); + if (!result.ok()) { + return CreateErrorValue(arena, result.status()); + } + return CelValue::CreateBool(*result); + }; + + for (auto op : in_operators) { + auto status = + FunctionAdapter::CreateAndRegister(op, false, + stringKeyInSet, + registry); + if (!status.ok()) return status; + + status = FunctionAdapter::CreateAndRegister( + op, false, boolKeyInSet, registry); + if (!status.ok()) return status; + + status = FunctionAdapter::CreateAndRegister( + op, false, intKeyInSet, registry); + if (!status.ok()) return status; + status = + FunctionAdapter::CreateAndRegister( + op, false, uintKeyInSet, registry); + if (!status.ok()) return status; + } return absl::OkStatus(); } @@ -853,13 +938,9 @@ absl::Status RegisterStringFunctions(CelFunctionRegistry* registry, registry); if (!status.ok()) return status; - status = - FunctionAdapter:: - CreateAndRegister(builtin::kStringStartsWith, true, StringStartsWith, - registry); - if (!status.ok()) return status; - - return absl::OkStatus(); + return FunctionAdapter:: + CreateAndRegister(builtin::kStringStartsWith, true, StringStartsWith, + registry); } absl::Status RegisterTimestampFunctions(CelFunctionRegistry* registry, @@ -1016,15 +1097,12 @@ absl::Status RegisterTimestampFunctions(CelFunctionRegistry* registry, registry); if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( + return FunctionAdapter::CreateAndRegister( builtin::kMilliseconds, true, [](Arena* arena, absl::Time ts) -> CelValue { return GetMilliseconds(arena, ts, ""); }, registry); - if (!status.ok()) return status; - - return absl::OkStatus(); } absl::Status RegisterBytesConversionFunctions(CelFunctionRegistry* registry, @@ -1040,15 +1118,12 @@ absl::Status RegisterBytesConversionFunctions(CelFunctionRegistry* registry, if (!status.ok()) return status; // string -> bytes - status = FunctionAdapter::CreateAndRegister( + return FunctionAdapter::CreateAndRegister( builtin::kBytes, false, [](Arena* arena, CelValue::StringHolder value) -> CelValue { return CelValue::CreateBytesView(value.value()); }, registry); - if (!status.ok()) return status; - - return absl::OkStatus(); } absl::Status RegisterDoubleConversionFunctions(CelFunctionRegistry* registry, @@ -1080,12 +1155,9 @@ absl::Status RegisterDoubleConversionFunctions(CelFunctionRegistry* registry, if (!status.ok()) return status; // uint -> double - status = FunctionAdapter::CreateAndRegister( + return FunctionAdapter::CreateAndRegister( builtin::kDouble, false, [](Arena*, uint64_t v) { return static_cast(v); }, registry); - if (!status.ok()) return status; - - return absl::OkStatus(); } absl::Status RegisterIntConversionFunctions(CelFunctionRegistry* registry, @@ -1139,7 +1211,7 @@ absl::Status RegisterIntConversionFunctions(CelFunctionRegistry* registry, if (!status.ok()) return status; // uint -> int - status = FunctionAdapter::CreateAndRegister( + return FunctionAdapter::CreateAndRegister( builtin::kInt, false, [](Arena* arena, uint64_t v) { if (v > static_cast(kIntMax)) { @@ -1149,9 +1221,6 @@ absl::Status RegisterIntConversionFunctions(CelFunctionRegistry* registry, return CelValue::CreateInt64(static_cast(v)); }, registry); - if (!status.ok()) return status; - - return absl::OkStatus(); } absl::Status RegisterStringConversionFunctions( @@ -1231,7 +1300,7 @@ absl::Status RegisterStringConversionFunctions( if (!status.ok()) return status; // timestamp -> string - status = FunctionAdapter::CreateAndRegister( + return FunctionAdapter::CreateAndRegister( builtin::kString, false, [](Arena* arena, absl::Time value) -> CelValue { auto encode = google::api::expr::internal::EncodeTimeToString(value); @@ -1243,9 +1312,6 @@ absl::Status RegisterStringConversionFunctions( Arena::Create(arena, encode.value()))); }, registry); - if (!status.ok()) return status; - - return absl::OkStatus(); } absl::Status RegisterUintConversionFunctions(CelFunctionRegistry* registry, @@ -1301,11 +1367,8 @@ absl::Status RegisterUintConversionFunctions(CelFunctionRegistry* registry, if (!status.ok()) return status; // uint -> uint - status = FunctionAdapter::CreateAndRegister( + return FunctionAdapter::CreateAndRegister( builtin::kUint, false, [](Arena*, uint64_t v) { return v; }, registry); - if (!status.ok()) return status; - - return absl::OkStatus(); } absl::Status RegisterConversionFunctions(CelFunctionRegistry* registry, @@ -1338,10 +1401,7 @@ absl::Status RegisterConversionFunctions(CelFunctionRegistry* registry, builtin::kTimestamp, false, CreateTimestampFromString, registry); if (!status.ok()) return status; - status = RegisterUintConversionFunctions(registry, options); - if (!status.ok()) return status; - - return absl::OkStatus(); + return RegisterUintConversionFunctions(registry, options); } } // namespace @@ -1452,76 +1512,6 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, builtin::kSize, false, list_size_func, registry); if (!status.ok()) return status; - // List in operator: @in - if (options.enable_list_contains) { - status = FunctionAdapter::CreateAndRegister( - builtin::kIn, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kIn, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kIn, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kIn, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter:: - CreateAndRegister(builtin::kIn, false, In, - registry); - if (!status.ok()) return status; - status = FunctionAdapter:: - CreateAndRegister(builtin::kIn, false, In, - registry); - if (!status.ok()) return status; - - // List in operator: _in_ (deprecated) - // Bindings preserved for backward compatibility with stored expressions. - status = FunctionAdapter::CreateAndRegister( - builtin::kInDeprecated, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kInDeprecated, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kInDeprecated, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kInDeprecated, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter:: - CreateAndRegister(builtin::kInDeprecated, false, - In, registry); - if (!status.ok()) return status; - status = FunctionAdapter:: - CreateAndRegister(builtin::kInDeprecated, false, - In, registry); - if (!status.ok()) return status; - - // List in() function (deprecated) - // Bindings preserved for backward compatibility with stored expressions. - status = FunctionAdapter::CreateAndRegister( - builtin::kInFunction, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kInFunction, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kInFunction, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( - builtin::kInFunction, false, In, registry); - if (!status.ok()) return status; - status = FunctionAdapter:: - CreateAndRegister(builtin::kInFunction, false, - In, registry); - if (!status.ok()) return status; - status = FunctionAdapter:: - CreateAndRegister(builtin::kInFunction, false, - In, registry); - if (!status.ok()) return status; - } - // Map size auto map_size_func = [](Arena*, const CelMap* cel_map) -> int64_t { return (*cel_map).size(); @@ -1534,86 +1524,8 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, builtin::kSize, false, map_size_func, registry); if (!status.ok()) return status; - // Map in operator: @in - status = FunctionAdapter:: - CreateAndRegister( - builtin::kIn, false, - [](Arena*, CelValue::StringHolder key, - const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateString(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter::CreateAndRegister( - builtin::kIn, false, - [](Arena*, int64_t key, const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateInt64(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter::CreateAndRegister( - builtin::kIn, false, - [](Arena*, uint64_t key, const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateUint64(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - // Map in operators: _in_ (deprecated). - // Bindings preserved for backward compatibility with stored expressions. - status = FunctionAdapter::CreateAndRegister( - builtin::kInDeprecated, false, - [](Arena*, int64_t key, const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateInt64(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter::CreateAndRegister( - builtin::kInDeprecated, false, - [](Arena*, uint64_t key, const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateUint64(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter:: - CreateAndRegister( - builtin::kInDeprecated, false, - [](Arena*, CelValue::StringHolder key, - const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateString(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - // Map in() function (deprecated) - status = FunctionAdapter:: - CreateAndRegister( - builtin::kInFunction, false, - [](Arena*, CelValue::StringHolder key, - const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateString(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter::CreateAndRegister( - builtin::kInFunction, false, - [](Arena*, int64_t key, const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateInt64(key)].has_value(); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter::CreateAndRegister( - builtin::kInFunction, false, - [](Arena*, uint64_t key, const CelMap* cel_map) -> bool { - return (*cel_map)[CelValue::CreateUint64(key)].has_value(); - }, - registry); + // Register set membership tests with the 'in' operator and its variants. + status = RegisterSetMembershipFunctions(registry, options); if (!status.ok()) return status; // basic Arithmetic functions for numeric types @@ -1785,16 +1697,12 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, registry); if (!status.ok()) return status; - status = - FunctionAdapter::CreateAndRegister( - builtin::kType, false, - [](Arena*, CelValue value) -> CelValue::CelTypeHolder { - return value.ObtainCelType().CelTypeOrDie(); - }, - registry); - if (!status.ok()) return status; - - return absl::OkStatus(); + return FunctionAdapter::CreateAndRegister( + builtin::kType, false, + [](Arena*, CelValue value) -> CelValue::CelTypeHolder { + return value.ObtainCelType().CelTypeOrDie(); + }, + registry); } } // namespace runtime diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index 5cff08eda..fcd8c799a 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -1071,6 +1071,23 @@ class FakeList : public CelList { std::vector values_; }; +class FakeErrorMap : public CelMap { + public: + FakeErrorMap() {} + + int size() const override { return 0; } + + absl::StatusOr Has(const CelValue& key) const override { + return absl::InvalidArgumentError("bad key type"); + } + + absl::optional operator[](CelValue key) const override { + return absl::nullopt; + } + + const CelList* ListKeys() const override { return nullptr; } +}; + template class FakeMap : public CelMap { public: @@ -1108,6 +1125,18 @@ class FakeMap : public CelMap { std::function(CelValue)> get_cel_value_; }; +class FakeBoolMap : public FakeMap { + public: + explicit FakeBoolMap(const std::map& data) + : FakeMap(data, CelValue::CreateBool, + [](CelValue v) -> absl::optional { + if (!v.IsBool()) { + return absl::nullopt; + } + return v.BoolOrDie(); + }) {} +}; + class FakeInt64Map : public FakeMap { public: explicit FakeInt64Map(const std::map& data) @@ -1146,18 +1175,6 @@ class FakeStringMap : public FakeMap { }) {} }; -class FakeBoolMap : public FakeMap { - public: - explicit FakeBoolMap(const std::map& data) - : FakeMap(data, CelValue::CreateBool, - [](CelValue v) -> absl::optional { - if (!v.IsBool()) { - return absl::nullopt; - } - return v.BoolOrDie(); - }) {} -}; - // Test list index access function TEST_F(BuiltinsTest, ListIndex) { constexpr int64_t kValues[] = {3, 4, 5, 6}; @@ -1534,6 +1551,39 @@ TEST_F(BuiltinsTest, TestBytesListIn) { TestInList(&cel_list, CelValue::CreateBytes(&v2), false); } +TEST_F(BuiltinsTest, TestMapInError) { + Arena arena; + FakeErrorMap cel_map; + std::vector kValues = { + CelValue::CreateBool(true), + CelValue::CreateInt64(1), + CelValue::CreateStringView("hello"), + CelValue::CreateUint64(2), + }; + for (auto key : kValues) { + CelValue result_value; + ASSERT_NO_FATAL_FAILURE(PerformRun( + builtin::kIn, {}, {key, CelValue::CreateMap(&cel_map)}, &result_value)); + + EXPECT_TRUE(result_value.IsError()); + EXPECT_EQ(result_value.ErrorOrDie()->message(), "bad key type"); + EXPECT_EQ(result_value.ErrorOrDie()->code(), + absl::StatusCode::kInvalidArgument); + } +} + +TEST_F(BuiltinsTest, TestBoolMapIn) { + constexpr bool kValues[] = {true, true}; + std::map data; + for (auto value : kValues) { + data[value] = CelValue::CreateBool(value); + } + FakeBoolMap cel_map(data); + TestInMap(&cel_map, CelValue::CreateBool(true), true); + TestInMap(&cel_map, CelValue::CreateBool(false), false); + TestInMap(&cel_map, CelValue::CreateUint64(3), false); +} + TEST_F(BuiltinsTest, TestInt64MapIn) { constexpr int64_t kValues[] = {3, -4, 5, -6}; std::map data; diff --git a/eval/public/cel_value.h b/eval/public/cel_value.h index 6b0fa400b..c8e260cf3 100644 --- a/eval/public/cel_value.h +++ b/eval/public/cel_value.h @@ -21,6 +21,7 @@ #include "google/protobuf/message.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/time/time.h" #include "absl/types/optional.h" @@ -423,12 +424,31 @@ class CelList { // CelMap is a base class for map accessors. class CelMap { public: - // Map lookup. If value found, - // returns CelValue in return type. + // Map lookup. If value found, returns CelValue in return type. // Per Protobuffer specification, acceptable key types are - // int64_t,uint64,string. + // bool, int64_t, uint64_t, string. + // TODO(issues/122): Make this method const correct. virtual absl::optional operator[](CelValue key) const = 0; + // Return whether the key is present within the map. + // + // Typically, key resolution will be a simple boolean result; however, there + // are scenarios where the conversion of the input key to the underlying + // key-type held by the map may result in an IllegalArgument error. + // + // Evaluators are responsible for handling non-OK results by propagating the + // error, as appropriate, up the evaluation stack either as a `StatusOr` or + // as a `CelError` value, depending on the context. + virtual absl::StatusOr Has(const CelValue &key) const { + // This implementation preserves the prior behavior for any derived classes + // which have not overridden the method. Note, it is possible that this + // implementation will return 'true' when the key lookup fails with an + // error. All core-CEL implementations override this method to provide + // semantics consistent with the CEL spec. + auto lookup_result = (*this)[key]; + return lookup_result.has_value(); + } + // Map size virtual int size() const = 0; // Default empty check. Can be overridden in subclass for performance. @@ -451,6 +471,12 @@ CelValue CreateErrorValue( absl::StatusCode error_code = absl::StatusCode::kUnknown, int position = -1); +// Utility method for generating a CelValue from an absl::Status. +inline CelValue CreateErrorValue(google::protobuf::Arena *arena, + const absl::Status &status) { + return CreateErrorValue(arena, status.message(), status.code()); +} + CelValue CreateNoMatchingOverloadError(google::protobuf::Arena *arena); CelValue CreateNoMatchingOverloadError(google::protobuf::Arena *arena, absl::string_view fn); diff --git a/eval/public/containers/BUILD b/eval/public/containers/BUILD index f0c52114f..7db32ba16 100644 --- a/eval/public/containers/BUILD +++ b/eval/public/containers/BUILD @@ -51,6 +51,7 @@ cc_library( "@com_google_absl//absl/hash", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/types:optional", "@com_google_absl//absl/types:span", ], ) @@ -81,6 +82,8 @@ cc_library( deps = [ ":field_access", "//eval/public:cel_value", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_protobuf//:protobuf", ], @@ -124,6 +127,7 @@ cc_test( deps = [ ":field_backed_map_impl", "//eval/testutil:test_message_cc_proto", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_googletest//:gtest_main", ], diff --git a/eval/public/containers/container_backed_map_impl.cc b/eval/public/containers/container_backed_map_impl.cc index 2128b71d6..37754ec8e 100644 --- a/eval/public/containers/container_backed_map_impl.cc +++ b/eval/public/containers/container_backed_map_impl.cc @@ -1,10 +1,10 @@ - - #include "eval/public/containers/container_backed_map_impl.h" #include "absl/container/node_hash_map.h" #include "absl/hash/hash.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/types/optional.h" #include "absl/types/span.h" #include "eval/public/cel_value.h" @@ -107,7 +107,6 @@ class ContainerBackedMapImpl : public CelMap { static absl::StatusOr> Create( absl::Span> key_values) { auto cel_map = absl::WrapUnique(new ContainerBackedMapImpl()); - auto status = cel_map->AddItems(key_values); if (!status.ok()) { return status; @@ -122,11 +121,15 @@ class ContainerBackedMapImpl : public CelMap { absl::optional operator[](CelValue cel_key) const override { auto item = values_map_.find(cel_key); if (item == values_map_.end()) { - return {}; + return absl::nullopt; } return item->second; } + absl::StatusOr Has(const CelValue& cel_key) const override { + return values_map_.contains(cel_key); + } + const CelList* ListKeys() const override { return &key_list_; } private: diff --git a/eval/public/containers/field_backed_map_impl.cc b/eval/public/containers/field_backed_map_impl.cc index 76c45be61..2dca6ffec 100644 --- a/eval/public/containers/field_backed_map_impl.cc +++ b/eval/public/containers/field_backed_map_impl.cc @@ -1,6 +1,13 @@ #include "eval/public/containers/field_backed_map_impl.h" +#include + +#include "google/protobuf/descriptor.h" #include "google/protobuf/map_field.h" +#include "google/protobuf/message.h" +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/strings/str_cat.h" #include "eval/public/cel_value.h" #include "eval/public/containers/field_access.h" @@ -17,12 +24,6 @@ namespace expr { // of macros usage. class CelMapReflectionFriend { public: - static bool ContainsMapKey(const Reflection* reflection, - const Message& message, - const FieldDescriptor* field, const MapKey& key) { - return reflection->ContainsMapKey(message, field, key); - } - static bool LookupMapValue(const Reflection* reflection, const Message& message, const FieldDescriptor* field, const MapKey& key, @@ -88,7 +89,7 @@ class KeyList : public CelList { auto status = CreateValueFromSingleField(entry, key_desc, arena_, &key); if (!status.ok()) { - return CreateErrorValue(arena_, status.message()); + return CreateErrorValue(arena_, status); } return key; } @@ -100,6 +101,32 @@ class KeyList : public CelList { google::protobuf::Arena* arena_; }; +bool MatchesMapKeyType(const FieldDescriptor* key_desc, const CelValue& key) { + switch (key_desc->cpp_type()) { + case google::protobuf::FieldDescriptor::CPPTYPE_BOOL: + return key.IsBool(); + case google::protobuf::FieldDescriptor::CPPTYPE_INT32: + // fall through + case google::protobuf::FieldDescriptor::CPPTYPE_INT64: + return key.IsInt64(); + case google::protobuf::FieldDescriptor::CPPTYPE_UINT32: + // fall through + case google::protobuf::FieldDescriptor::CPPTYPE_UINT64: + return key.IsUint64(); + case google::protobuf::FieldDescriptor::CPPTYPE_STRING: + return key.IsString(); + default: + return false; + } +} + +absl::Status InvalidMapKeyType(absl::string_view key_type, + const CelValue& key) { + return absl::InvalidArgumentError(absl::StrCat("invalid map key type.", + " wanted: ", key_type, + " got: ", key.DebugString())); +} + } // namespace FieldBackedMapImpl::FieldBackedMapImpl( @@ -107,6 +134,8 @@ FieldBackedMapImpl::FieldBackedMapImpl( google::protobuf::Arena* arena) : message_(message), descriptor_(descriptor), + key_desc_(descriptor_->message_type()->FindFieldByNumber(kKeyTag)), + value_desc_(descriptor_->message_type()->FindFieldByNumber(kValueTag)), reflection_(message_->GetReflection()), arena_(arena), key_list_(absl::make_unique(message, descriptor, arena)) {} @@ -117,119 +146,174 @@ int FieldBackedMapImpl::size() const { const CelList* FieldBackedMapImpl::ListKeys() const { return key_list_.get(); } +absl::StatusOr FieldBackedMapImpl::Has(const CelValue& key) const { +#ifdef GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND + MapValueConstRef value_ref; + return LookupMapValue(key, &value_ref); +#else // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND + return LegacyHasMapValue(key); +#endif // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND +} + absl::optional FieldBackedMapImpl::operator[](CelValue key) const { #ifdef GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND - // Fast implementation. - google::protobuf::MapKey inner_key; - switch (key.type()) { - case CelValue::Type::kBool: { - inner_key.SetBoolValue(key.BoolOrDie()); - break; - } - case CelValue::Type::kInt64: { - inner_key.SetInt64Value(key.Int64OrDie()); - break; - } - case CelValue::Type::kUint64: { - inner_key.SetUInt64Value(key.Uint64OrDie()); - break; - } - case CelValue::Type::kString: { - auto str = key.StringOrDie().value(); - inner_key.SetStringValue(std::string(str.begin(), str.end())); - break; - } - default: { - return {}; - } - } + // Fast implementation which uses a friend method to do a hash-based key + // lookup. MapValueConstRef value_ref; - // Look the value up - if (!google::protobuf::expr::CelMapReflectionFriend::LookupMapValue( - reflection_, *message_, descriptor_, inner_key, &value_ref)) { - return {}; + auto lookup_result = LookupMapValue(key, &value_ref); + if (!lookup_result.ok()) { + return CreateErrorValue(arena_, lookup_result.status()); + } + if (!*lookup_result) { + return absl::nullopt; } // Get value descriptor treating it as a repeated field. // All values in protobuf map have the same type. - // The map is not empty, because LookuMapValue returned true. - const Message* entry = - &reflection_->GetRepeatedMessage(*message_, descriptor_, 0); - if (entry == nullptr) { - return {}; - } - const Descriptor* entry_descriptor = entry->GetDescriptor(); - const FieldDescriptor* value_desc = - entry_descriptor->FindFieldByNumber(kValueTag); - + // The map is not empty, because LookupMapValue returned true. CelValue result = CelValue::CreateNull(); - auto status = CreateValueFromMapValue(message_, value_desc, &value_ref, - arena_, &result); + const auto& status = CreateValueFromMapValue(message_, value_desc_, + &value_ref, arena_, &result); if (!status.ok()) { - return CreateErrorValue(arena_, status.message()); + return CreateErrorValue(arena_, status); } return result; + #else // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND - // Slow implementation. - CelValue result = CelValue::CreateNull(); - CelValue inner_key = CelValue::CreateNull(); + // Default proto implementation, does not use fast-path key lookup. + return LegacyLookupMapValue(key); +#endif // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND +} +absl::StatusOr FieldBackedMapImpl::LookupMapValue( + const CelValue& key, MapValueConstRef* value_ref) const { +#ifdef GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND + + if (!MatchesMapKeyType(key_desc_, key)) { + return InvalidMapKeyType(key_desc_->cpp_type_name(), key); + } + + google::protobuf::MapKey proto_key; + switch (key_desc_->cpp_type()) { + case google::protobuf::FieldDescriptor::CPPTYPE_BOOL: { + bool key_value; + key.GetValue(&key_value); + proto_key.SetBoolValue(key_value); + } break; + case google::protobuf::FieldDescriptor::CPPTYPE_INT32: { + int64_t key_value; + key.GetValue(&key_value); + if (key_value > std::numeric_limits::max() || + key_value < std::numeric_limits::lowest()) { + return absl::OutOfRangeError("integer overlow"); + } + proto_key.SetInt32Value(key_value); + } break; + case google::protobuf::FieldDescriptor::CPPTYPE_INT64: { + int64_t key_value; + key.GetValue(&key_value); + proto_key.SetInt64Value(key_value); + } break; + case google::protobuf::FieldDescriptor::CPPTYPE_STRING: { + CelValue::StringHolder key_value; + key.GetValue(&key_value); + auto str = key_value.value(); + proto_key.SetStringValue(std::string(str.begin(), str.end())); + } break; + case google::protobuf::FieldDescriptor::CPPTYPE_UINT32: { + uint64_t key_value; + key.GetValue(&key_value); + if (key_value > std::numeric_limits::max()) { + return absl::OutOfRangeError("unsigned integer overlow"); + } + proto_key.SetUInt32Value(key_value); + } break; + case google::protobuf::FieldDescriptor::CPPTYPE_UINT64: { + uint64_t key_value; + key.GetValue(&key_value); + proto_key.SetUInt64Value(key_value); + } break; + default: + return InvalidMapKeyType(key_desc_->cpp_type_name(), key); + } + // Look the value up + return google::protobuf::expr::CelMapReflectionFriend::LookupMapValue( + reflection_, *message_, descriptor_, proto_key, value_ref); +#else // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND + return absl::UnimplementedError("fast-path key lookup not implemented"); +#endif // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND +} + +absl::StatusOr FieldBackedMapImpl::LegacyHasMapValue( + const CelValue& key) const { + auto lookup_result = LegacyLookupMapValue(key); + if (!lookup_result.has_value()) { + return false; + } + auto result = *lookup_result; + if (result.IsError()) { + return *(result.ErrorOrDie()); + } + return true; +} + +absl::optional FieldBackedMapImpl::LegacyLookupMapValue( + const CelValue& key) const { + // Ensure that the key matches the key type. + if (!MatchesMapKeyType(key_desc_, key)) { + return CreateErrorValue(arena_, + InvalidMapKeyType(key_desc_->cpp_type_name(), key)); + } + + CelValue proto_key = CelValue::CreateNull(); int map_size = size(); for (int i = 0; i < map_size; i++) { const Message* entry = &reflection_->GetRepeatedMessage(*message_, descriptor_, i); - if (entry == nullptr) continue; - const Descriptor* entry_descriptor = entry->GetDescriptor(); // Key Tag == 1 - const FieldDescriptor* key_desc = - entry_descriptor->FindFieldByNumber(kKeyTag); - auto status = - CreateValueFromSingleField(entry, key_desc, arena_, &inner_key); + CreateValueFromSingleField(entry, key_desc_, arena_, &proto_key); if (!status.ok()) { - return CreateErrorValue(arena_, status.ToString()); - } - - if (key.type() != inner_key.type()) { - continue; + return CreateErrorValue(arena_, status); } bool match = false; - switch (key.type()) { - case CelValue::Type::kBool: - match = key.BoolOrDie() == inner_key.BoolOrDie(); + switch (key_desc_->cpp_type()) { + case google::protobuf::FieldDescriptor::CPPTYPE_BOOL: + match = key.BoolOrDie() == proto_key.BoolOrDie(); break; - case CelValue::Type::kInt64: - match = key.Int64OrDie() == inner_key.Int64OrDie(); + case google::protobuf::FieldDescriptor::CPPTYPE_INT32: + // fall through + case google::protobuf::FieldDescriptor::CPPTYPE_INT64: + match = key.Int64OrDie() == proto_key.Int64OrDie(); break; - case CelValue::Type::kUint64: - match = key.Uint64OrDie() == inner_key.Uint64OrDie(); + case google::protobuf::FieldDescriptor::CPPTYPE_UINT32: + // fall through + case google::protobuf::FieldDescriptor::CPPTYPE_UINT64: + match = key.Uint64OrDie() == proto_key.Uint64OrDie(); break; - case CelValue::Type::kString: - match = key.StringOrDie() == inner_key.StringOrDie(); + case google::protobuf::FieldDescriptor::CPPTYPE_STRING: + match = key.StringOrDie() == proto_key.StringOrDie(); break; default: - match = false; + // this would normally indicate a bad key type, which should not be + // possible based on the earlier test. + break; } if (match) { - const FieldDescriptor* value_desc = - entry_descriptor->FindFieldByNumber(kValueTag); - + CelValue result = CelValue::CreateNull(); auto status = - CreateValueFromSingleField(entry, value_desc, arena_, &result); + CreateValueFromSingleField(entry, value_desc_, arena_, &result); if (!status.ok()) { - return CreateErrorValue(arena_, status.message()); + return CreateErrorValue(arena_, status); } - return result; } } - return {}; -#endif // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND } } // namespace runtime diff --git a/eval/public/containers/field_backed_map_impl.h b/eval/public/containers/field_backed_map_impl.h index 2f1d7ad47..1ceb51185 100644 --- a/eval/public/containers/field_backed_map_impl.h +++ b/eval/public/containers/field_backed_map_impl.h @@ -1,6 +1,9 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CONTAINERS_FIELD_BACKED_MAP_IMPL_H_ #define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CONTAINERS_FIELD_BACKED_MAP_IMPL_H_ +#include "google/protobuf/descriptor.h" +#include "google/protobuf/message.h" +#include "absl/status/statusor.h" #include "eval/public/cel_value.h" namespace google { @@ -26,11 +29,28 @@ class FieldBackedMapImpl : public CelMap { // Map element access operator. absl::optional operator[](CelValue key) const override; + // Presence test function. + absl::StatusOr Has(const CelValue& key) const override; + const CelList* ListKeys() const override; + protected: + // These methods are exposed as protected methods for testing purposes since + // whether one or the other is used depends on build time flags, but each + // should be tested accordingly. + + absl::StatusOr LookupMapValue( + const CelValue& key, google::protobuf::MapValueConstRef* value_ref) const; + + absl::StatusOr LegacyHasMapValue(const CelValue& key) const; + + absl::optional LegacyLookupMapValue(const CelValue& key) const; + private: const google::protobuf::Message* message_; const google::protobuf::FieldDescriptor* descriptor_; + const google::protobuf::FieldDescriptor* key_desc_; + const google::protobuf::FieldDescriptor* value_desc_; const google::protobuf::Reflection* reflection_; google::protobuf::Arena* arena_; std::unique_ptr key_list_; diff --git a/eval/public/containers/field_backed_map_impl_test.cc b/eval/public/containers/field_backed_map_impl_test.cc index 59f75bf8e..518c593fe 100644 --- a/eval/public/containers/field_backed_map_impl_test.cc +++ b/eval/public/containers/field_backed_map_impl_test.cc @@ -1,7 +1,10 @@ #include "eval/public/containers/field_backed_map_impl.h" +#include + #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "eval/testutil/test_message.pb.h" @@ -14,28 +17,116 @@ namespace { using testing::Eq; using testing::UnorderedPointwise; +class FieldBackedMapTestImpl : public FieldBackedMapImpl { + public: + FieldBackedMapTestImpl(const google::protobuf::Message* message, + const google::protobuf::FieldDescriptor* descriptor, + google::protobuf::Arena* arena) + : FieldBackedMapImpl(message, descriptor, arena) {} + + using FieldBackedMapImpl::LegacyHasMapValue; + using FieldBackedMapImpl::LegacyLookupMapValue; +}; + // Helper method. Creates simple pipeline containing Select step and runs it. -std::unique_ptr CreateMap(const TestMessage* message, - const std::string& field, - google::protobuf::Arena* arena) { +std::unique_ptr CreateMap(const TestMessage* message, + const std::string& field, + google::protobuf::Arena* arena) { const google::protobuf::FieldDescriptor* field_desc = message->GetDescriptor()->FindFieldByName(field); - return absl::make_unique(message, field_desc, arena); + return absl::make_unique(message, field_desc, arena); } -TEST(FieldBackedMapImplTest, IntKeyTest) { +TEST(FieldBackedMapImplTest, BadKeyTypeTest) { TestMessage message; - auto field_map = message.mutable_int64_int32_map(); + google::protobuf::Arena arena; + constexpr std::array map_types = { + "int64_int32_map", "uint64_int32_map", "string_int32_map", + "bool_int32_map", "int32_int32_map", "uint32_uint32_map", + }; + + for (auto map_type : map_types) { + auto cel_map = CreateMap(&message, std::string(map_type), &arena); + // Look up a boolean key. This should result in an error for both the + // presence test and the value lookup. + auto result = cel_map->Has(CelValue::CreateNull()); + EXPECT_FALSE(result.ok()); + EXPECT_THAT(result.status().code(), Eq(absl::StatusCode::kInvalidArgument)); + + result = cel_map->LegacyHasMapValue(CelValue::CreateNull()); + EXPECT_FALSE(result.ok()); + EXPECT_THAT(result.status().code(), Eq(absl::StatusCode::kInvalidArgument)); + + auto lookup = (*cel_map)[CelValue::CreateNull()]; + EXPECT_TRUE(lookup.has_value()); + EXPECT_TRUE(lookup->IsError()); + EXPECT_THAT(lookup->ErrorOrDie()->code(), + Eq(absl::StatusCode::kInvalidArgument)); + + lookup = cel_map->LegacyLookupMapValue(CelValue::CreateNull()); + EXPECT_TRUE(lookup.has_value()); + EXPECT_TRUE(lookup->IsError()); + EXPECT_THAT(lookup->ErrorOrDie()->code(), + Eq(absl::StatusCode::kInvalidArgument)); + } +} + +TEST(FieldBackedMapImplTest, Int32KeyTest) { + TestMessage message; + auto field_map = message.mutable_int32_int32_map(); (*field_map)[0] = 1; (*field_map)[1] = 2; google::protobuf::Arena arena; + auto cel_map = CreateMap(&message, "int32_int32_map", &arena); + + EXPECT_EQ((*cel_map)[CelValue::CreateInt64(0)]->Int64OrDie(), 1); + EXPECT_EQ((*cel_map)[CelValue::CreateInt64(1)]->Int64OrDie(), 2); + EXPECT_TRUE(cel_map->Has(CelValue::CreateInt64(1)).value_or(false)); + EXPECT_TRUE( + cel_map->LegacyHasMapValue(CelValue::CreateInt64(1)).value_or(false)); + + // Look up nonexistent key + EXPECT_EQ((*cel_map)[CelValue::CreateInt64(3)].has_value(), false); + EXPECT_FALSE(cel_map->Has(CelValue::CreateInt64(3)).value_or(true)); + EXPECT_FALSE( + cel_map->LegacyHasMapValue(CelValue::CreateInt64(3)).value_or(true)); +} + +TEST(FieldBackedMapImplTest, Int32KeyOutOfRangeTest) { + TestMessage message; + google::protobuf::Arena arena; + auto cel_map = CreateMap(&message, "int32_int32_map", &arena); + + // Look up keys out of int32_t range + auto result = cel_map->Has( + CelValue::CreateInt64(std::numeric_limits::max() + 1L)); + EXPECT_FALSE(result.ok()); + EXPECT_THAT(result.status().code(), Eq(absl::StatusCode::kOutOfRange)); + + result = cel_map->Has( + CelValue::CreateInt64(std::numeric_limits::lowest() - 1L)); + EXPECT_FALSE(result.ok()); + EXPECT_THAT(result.status().code(), Eq(absl::StatusCode::kOutOfRange)); +} + +TEST(FieldBackedMapImplTest, Int64KeyTest) { + TestMessage message; + auto field_map = message.mutable_int64_int32_map(); + (*field_map)[0] = 1; + (*field_map)[1] = 2; + google::protobuf::Arena arena; auto cel_map = CreateMap(&message, "int64_int32_map", &arena); EXPECT_EQ((*cel_map)[CelValue::CreateInt64(0)]->Int64OrDie(), 1); EXPECT_EQ((*cel_map)[CelValue::CreateInt64(1)]->Int64OrDie(), 2); + EXPECT_TRUE(cel_map->Has(CelValue::CreateInt64(1)).value_or(false)); + EXPECT_EQ( + cel_map->LegacyLookupMapValue(CelValue::CreateInt64(1))->Int64OrDie(), 2); + EXPECT_TRUE( + cel_map->LegacyHasMapValue(CelValue::CreateInt64(1)).value_or(false)); // Look up nonexistent key EXPECT_EQ((*cel_map)[CelValue::CreateInt64(3)].has_value(), false); @@ -47,10 +138,12 @@ TEST(FieldBackedMapImplTest, BoolKeyTest) { (*field_map)[false] = 1; google::protobuf::Arena arena; - auto cel_map = CreateMap(&message, "bool_int32_map", &arena); EXPECT_EQ((*cel_map)[CelValue::CreateBool(false)]->Int64OrDie(), 1); + EXPECT_TRUE(cel_map->Has(CelValue::CreateBool(false)).value_or(false)); + EXPECT_TRUE( + cel_map->LegacyHasMapValue(CelValue::CreateBool(false)).value_or(false)); // Look up nonexistent key EXPECT_EQ((*cel_map)[CelValue::CreateBool(true)].has_value(), false); @@ -58,18 +151,52 @@ TEST(FieldBackedMapImplTest, BoolKeyTest) { EXPECT_EQ((*cel_map)[CelValue::CreateBool(true)]->Int64OrDie(), 2); } -TEST(FieldBackedMapImplTest, UintKeyTest) { +TEST(FieldBackedMapImplTest, Uint32KeyTest) { + TestMessage message; + auto field_map = message.mutable_uint32_uint32_map(); + (*field_map)[0] = 1u; + (*field_map)[1] = 2u; + + google::protobuf::Arena arena; + auto cel_map = CreateMap(&message, "uint32_uint32_map", &arena); + + EXPECT_EQ((*cel_map)[CelValue::CreateUint64(0)]->Uint64OrDie(), 1UL); + EXPECT_EQ((*cel_map)[CelValue::CreateUint64(1)]->Uint64OrDie(), 2UL); + EXPECT_TRUE(cel_map->Has(CelValue::CreateUint64(1)).value_or(false)); + EXPECT_TRUE( + cel_map->LegacyHasMapValue(CelValue::CreateUint64(1)).value_or(false)); + + // Look up nonexistent key + EXPECT_EQ((*cel_map)[CelValue::CreateUint64(3)].has_value(), false); + EXPECT_EQ(cel_map->Has(CelValue::CreateUint64(3)).value_or(true), false); +} + +TEST(FieldBackedMapImplTest, Uint32KeyOutOfRangeTest) { + TestMessage message; + google::protobuf::Arena arena; + auto cel_map = CreateMap(&message, "uint32_uint32_map", &arena); + + // Look up keys out of uint32_t range + auto result = cel_map->Has( + CelValue::CreateUint64(std::numeric_limits::max() + 1UL)); + EXPECT_FALSE(result.ok()); + EXPECT_THAT(result.status().code(), Eq(absl::StatusCode::kOutOfRange)); +} + +TEST(FieldBackedMapImplTest, Uint64KeyTest) { TestMessage message; auto field_map = message.mutable_uint64_int32_map(); (*field_map)[0] = 1; (*field_map)[1] = 2; google::protobuf::Arena arena; - auto cel_map = CreateMap(&message, "uint64_int32_map", &arena); EXPECT_EQ((*cel_map)[CelValue::CreateUint64(0)]->Int64OrDie(), 1); EXPECT_EQ((*cel_map)[CelValue::CreateUint64(1)]->Int64OrDie(), 2); + EXPECT_TRUE(cel_map->Has(CelValue::CreateUint64(1)).value_or(false)); + EXPECT_TRUE( + cel_map->LegacyHasMapValue(CelValue::CreateUint64(1)).value_or(false)); // Look up nonexistent key EXPECT_EQ((*cel_map)[CelValue::CreateUint64(3)].has_value(), false); @@ -82,7 +209,6 @@ TEST(FieldBackedMapImplTest, StringKeyTest) { (*field_map)["test1"] = 2; google::protobuf::Arena arena; - auto cel_map = CreateMap(&message, "string_int32_map", &arena); std::string test0 = "test0"; @@ -91,6 +217,9 @@ TEST(FieldBackedMapImplTest, StringKeyTest) { EXPECT_EQ((*cel_map)[CelValue::CreateString(&test0)]->Int64OrDie(), 1); EXPECT_EQ((*cel_map)[CelValue::CreateString(&test1)]->Int64OrDie(), 2); + EXPECT_TRUE(cel_map->Has(CelValue::CreateString(&test1)).value_or(false)); + EXPECT_TRUE(cel_map->LegacyHasMapValue(CelValue::CreateString(&test1)) + .value_or(false)); // Look up nonexistent key EXPECT_EQ((*cel_map)[CelValue::CreateString(&test_notfound)].has_value(), @@ -99,9 +228,7 @@ TEST(FieldBackedMapImplTest, StringKeyTest) { TEST(FieldBackedMapImplTest, EmptySizeTest) { TestMessage message; - google::protobuf::Arena arena; - auto cel_map = CreateMap(&message, "string_int32_map", &arena); EXPECT_EQ(cel_map->size(), 0); } @@ -114,7 +241,6 @@ TEST(FieldBackedMapImplTest, RepeatedAddTest) { (*field_map)["test0"] = 3; google::protobuf::Arena arena; - auto cel_map = CreateMap(&message, "string_int32_map", &arena); EXPECT_EQ(cel_map->size(), 2); @@ -131,9 +257,7 @@ TEST(FieldBackedMapImplTest, KeyListTest) { } google::protobuf::Arena arena; - auto cel_map = CreateMap(&message, "string_int32_map", &arena); - const CelList* key_list = cel_map->ListKeys(); EXPECT_EQ(key_list->size(), 100); diff --git a/eval/public/structs/BUILD b/eval/public/structs/BUILD index 3afa8d9c2..22d66fc53 100644 --- a/eval/public/structs/BUILD +++ b/eval/public/structs/BUILD @@ -15,6 +15,7 @@ cc_library( "//eval/testutil:test_message_cc_proto", "//internal:proto_util", "@com_google_absl//absl/container:flat_hash_map", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_absl//absl/synchronization", "@com_google_protobuf//:protobuf", diff --git a/eval/public/structs/cel_proto_wrapper.cc b/eval/public/structs/cel_proto_wrapper.cc index b121ccf59..2a7dfc81f 100644 --- a/eval/public/structs/cel_proto_wrapper.cc +++ b/eval/public/structs/cel_proto_wrapper.cc @@ -11,6 +11,7 @@ #include "google/protobuf/wrappers.pb.h" #include "google/protobuf/message.h" #include "absl/container/flat_hash_map.h" +#include "absl/status/status.h" #include "absl/strings/escaping.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" @@ -93,15 +94,30 @@ class DynamicMap : public CelMap { DynamicMap(const Struct* values, Arena* arena) : arena_(arena), values_(values), key_list_(values) {} + absl::StatusOr Has(const CelValue& key) const override { + CelValue::StringHolder str_key; + if (!key.GetValue(&str_key)) { + // Not a string key. + return absl::InvalidArgumentError(absl::StrCat( + "invalid map key type. wanted: string, got: ", key.DebugString())); + } + + return values_->fields().contains(std::string(str_key.value())); + } + absl::optional operator[](CelValue key) const override { CelValue::StringHolder str_key; if (!key.GetValue(&str_key)) { - return {}; // Not a string key + // Not a string key. + return CreateErrorValue(arena_, + absl::InvalidArgumentError(absl::StrCat( + "invalid map key type. wanted: string, got: ", + key.DebugString()))); } auto it = values_->fields().find(std::string(str_key.value())); if (it == values_->fields().end()) { - return {}; + return absl::nullopt; } return ValueFromMessage(&it->second, arena_); diff --git a/eval/public/structs/cel_proto_wrapper_test.cc b/eval/public/structs/cel_proto_wrapper_test.cc index 2ac55345c..08ac4b528 100644 --- a/eval/public/structs/cel_proto_wrapper_test.cc +++ b/eval/public/structs/cel_proto_wrapper_test.cc @@ -250,21 +250,39 @@ TEST_F(CelProtoWrapperTest, UnwrapValueStruct) { const CelMap* cel_map = value.MapOrDie(); - auto lookup1 = (*cel_map)[CelValue::CreateString(&kFields[0])]; + CelValue field1 = CelValue::CreateString(&kFields[0]); + auto field1_presence = cel_map->Has(field1); + ASSERT_OK(field1_presence); + EXPECT_TRUE(*field1_presence); + auto lookup1 = (*cel_map)[field1]; ASSERT_TRUE(lookup1.has_value()); ASSERT_TRUE(lookup1.value().IsBool()); EXPECT_EQ(lookup1.value().BoolOrDie(), true); - auto lookup2 = (*cel_map)[CelValue::CreateString(&kFields[1])]; + CelValue field2 = CelValue::CreateString(&kFields[1]); + auto field2_presence = cel_map->Has(field2); + ASSERT_OK(field2_presence); + EXPECT_TRUE(*field2_presence); + auto lookup2 = (*cel_map)[field2]; ASSERT_TRUE(lookup2.has_value()); ASSERT_TRUE(lookup2.value().IsDouble()); EXPECT_DOUBLE_EQ(lookup2.value().DoubleOrDie(), 1.0); - auto lookup3 = (*cel_map)[CelValue::CreateString(&kFields[2])]; + CelValue field3 = CelValue::CreateString(&kFields[2]); + auto field3_presence = cel_map->Has(field3); + ASSERT_OK(field3_presence); + EXPECT_TRUE(*field3_presence); + auto lookup3 = (*cel_map)[field3]; ASSERT_TRUE(lookup3.has_value()); ASSERT_TRUE(lookup3.value().IsString()); EXPECT_EQ(lookup3.value().StringOrDie().value(), "test"); + std::string missing = "missing_field"; + CelValue missing_field = CelValue::CreateString(&missing); + auto missing_field_presence = cel_map->Has(missing_field); + ASSERT_OK(missing_field_presence); + EXPECT_FALSE(*missing_field_presence); + const CelList* key_list = cel_map->ListKeys(); ASSERT_EQ(key_list->size(), kFields.size()); @@ -306,6 +324,15 @@ TEST_F(CelProtoWrapperTest, UnwrapDynamicStruct) { ASSERT_TRUE(v.IsBool()); EXPECT_EQ(v.BoolOrDie(), true); } + { + auto presence = cel_map->Has(CelValue::CreateBool(true)); + ASSERT_FALSE(presence.ok()); + EXPECT_EQ(presence.status().code(), absl::StatusCode::kInvalidArgument); + auto lookup = (*cel_map)[CelValue::CreateBool(true)]; + ASSERT_TRUE(lookup.has_value()); + auto v = lookup.value(); + ASSERT_TRUE(v.IsError()); + } } TEST_F(CelProtoWrapperTest, UnwrapDynamicValueStruct) { diff --git a/eval/testutil/test_message.proto b/eval/testutil/test_message.proto index 38a08b428..513fe7815 100644 --- a/eval/testutil/test_message.proto +++ b/eval/testutil/test_message.proto @@ -65,6 +65,8 @@ message TestMessage { map uint64_int32_map = 202; map string_int32_map = 203; map bool_int32_map = 204; + map int32_int32_map = 205; + map uint32_uint32_map = 206; // Well-known types. google.protobuf.Any any_value = 300; diff --git a/tools/BUILD b/tools/BUILD index 7729b01b5..ce509f96c 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -18,6 +18,10 @@ cc_library( deps = [ "//eval/public:cel_value", "@com_github_google_flatbuffers//:flatbuffers", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings", + "@com_google_absl//absl/types:optional", ], ) diff --git a/tools/flatbuffers_backed_impl.cc b/tools/flatbuffers_backed_impl.cc index ffeb06044..2fa2f99e2 100644 --- a/tools/flatbuffers_backed_impl.cc +++ b/tools/flatbuffers_backed_impl.cc @@ -2,6 +2,11 @@ #include +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/strings/str_cat.h" +#include "absl/types/optional.h" +#include "eval/public/cel_value.h" #include "flatbuffers/flatbuffers.h" namespace google { @@ -80,12 +85,29 @@ class ObjectStringIndexedMapImpl : public CelMap { schema_(schema), object_(object), index_(index) { - keys_.parent_ = this; + keys_.parent = this; } + int size() const override { return list_ ? list_->size() : 0; } + + absl::StatusOr Has(const CelValue& key) const override { + auto lookup_result = (*this)[key]; + if (!lookup_result.has_value()) { + return false; + } + auto result = *lookup_result; + if (result.IsError()) { + return *(result.ErrorOrDie()); + } + return true; + } + absl::optional operator[](CelValue cel_key) const override { if (!cel_key.IsString()) { - return {}; + return CreateErrorValue(arena_, + absl::InvalidArgumentError(absl::StrCat( + "invalid map key type. wanted: string, got: ", + cel_key.DebugString()))); } const absl::string_view key = cel_key.StringOrDie().value(); const auto it = std::lower_bound( @@ -105,23 +127,24 @@ class ObjectStringIndexedMapImpl : public CelMap { arena_, **it, schema_, object_, arena_)); } } - return {}; + return absl::nullopt; } + const CelList* ListKeys() const override { return &keys_; } private: struct KeyList : public CelList { - int size() const override { return parent_->size(); } + int size() const override { return parent->size(); } CelValue operator[](int index) const override { - auto value = flatbuffers::GetFieldS(*(parent_->list_->Get(index)), - parent_->index_); + auto value = + flatbuffers::GetFieldS(*(parent->list_->Get(index)), parent->index_); if (value == nullptr) { return CelValue::CreateStringView(absl::string_view()); } return CelValue::CreateStringView( absl::string_view(value->c_str(), value->size())); } - ObjectStringIndexedMapImpl* parent_; + ObjectStringIndexedMapImpl* parent; }; google::protobuf::Arena* arena_; const flatbuffers::Vector>* list_; @@ -143,14 +166,29 @@ const reflection::Field* findStringKeyField(const reflection::Object& object) { } // namespace +absl::StatusOr FlatBuffersMapImpl::Has(const CelValue& key) const { + auto lookup_result = (*this)[key]; + if (!lookup_result.has_value()) { + return false; + } + auto result = *lookup_result; + if (result.IsError()) { + return *(result.ErrorOrDie()); + } + return true; +} + absl::optional FlatBuffersMapImpl::operator[]( CelValue cel_key) const { if (!cel_key.IsString()) { - return {}; + return CreateErrorValue( + arena_, absl::InvalidArgumentError( + absl::StrCat("invalid map key type. wanted: string, got: ", + cel_key.DebugString()))); } - auto field = keys_.fields_->LookupByKey(cel_key.StringOrDie().value().data()); + auto field = keys_.fields->LookupByKey(cel_key.StringOrDie().value().data()); if (field == nullptr) { - return {}; + return absl::nullopt; } switch (field->type()->base_type()) { case reflection::Byte: @@ -285,15 +323,15 @@ absl::optional FlatBuffersMapImpl::operator[]( } default: // Unsupported vector base types - return {}; + return absl::nullopt; } break; } default: // Unsupported types: enums, unions, arrays - return {}; + return absl::nullopt; } - return {}; + return absl::nullopt; } const CelMap* CreateFlatBuffersBackedObject(const uint8_t* flatbuf, diff --git a/tools/flatbuffers_backed_impl.h b/tools/flatbuffers_backed_impl.h index 2fe9b9b02..86374a0be 100644 --- a/tools/flatbuffers_backed_impl.h +++ b/tools/flatbuffers_backed_impl.h @@ -15,21 +15,26 @@ class FlatBuffersMapImpl : public CelMap { const reflection::Schema& schema, const reflection::Object& object, google::protobuf::Arena* arena) : arena_(arena), table_(table), schema_(schema) { - keys_.fields_ = object.fields(); + keys_.fields = object.fields(); } - int size() const override { return keys_.fields_->size(); } + + int size() const override { return keys_.fields->size(); } + + absl::StatusOr Has(const CelValue& key) const override; + absl::optional operator[](CelValue cel_key) const override; + const CelList* ListKeys() const override { return &keys_; } private: struct FieldList : public CelList { - int size() const override { return fields_->size(); } + int size() const override { return fields->size(); } CelValue operator[](int index) const override { - auto name = fields_->Get(index)->name(); + auto name = fields->Get(index)->name(); return CelValue::CreateStringView( absl::string_view(name->c_str(), name->size())); } - const flatbuffers::Vector>* fields_; + const flatbuffers::Vector>* fields; }; FieldList keys_; google::protobuf::Arena* arena_; diff --git a/tools/flatbuffers_backed_impl_test.cc b/tools/flatbuffers_backed_impl_test.cc index e2328f332..b743c56c8 100644 --- a/tools/flatbuffers_backed_impl_test.cc +++ b/tools/flatbuffers_backed_impl_test.cc @@ -178,11 +178,17 @@ TEST_F(FlatBuffersTest, PrimitiveFields) { EXPECT_TRUE(f.value().IsString()); EXPECT_EQ("test", f.value().StringOrDie().value()); } - // missing field + // bad field type { - auto f = value[CelValue::CreateInt64(1)]; - EXPECT_FALSE(f.has_value()); + CelValue bad_field = CelValue::CreateInt64(1); + auto f = value[bad_field]; + EXPECT_TRUE(f.has_value()); + EXPECT_TRUE(f->IsError()); + auto presence = value.Has(bad_field); + EXPECT_FALSE(presence.ok()); + EXPECT_EQ(presence.status().code(), absl::StatusCode::kInvalidArgument); } + // missing field { auto f = value[CelValue::CreateStringView(kUnknownField)]; EXPECT_FALSE(f.has_value()); @@ -228,23 +234,48 @@ TEST_F(FlatBuffersTest, ObjectField) { f_int: 16 } })"); - auto f = value[CelValue::CreateStringView(kObjField)]; + CelValue field = CelValue::CreateStringView(kObjField); + auto presence = value.Has(field); + EXPECT_OK(presence); + EXPECT_TRUE(*presence); + auto f = value[field]; EXPECT_TRUE(f.has_value()); EXPECT_TRUE(f.value().IsMap()); const CelMap& m = *f.value().MapOrDie(); EXPECT_EQ(2, m.size()); { - auto mf = m[CelValue::CreateStringView(kStringField)]; + auto obj_field = CelValue::CreateStringView(kStringField); + auto member_presence = m.Has(obj_field); + EXPECT_OK(member_presence); + EXPECT_TRUE(*member_presence); + auto mf = m[obj_field]; EXPECT_TRUE(mf.has_value()); EXPECT_TRUE(mf.value().IsString()); EXPECT_EQ("entry", mf.value().StringOrDie().value()); } { - auto mf = m[CelValue::CreateStringView(kIntField)]; + auto obj_field = CelValue::CreateStringView(kIntField); + auto member_presence = m.Has(obj_field); + EXPECT_OK(member_presence); + EXPECT_TRUE(*member_presence); + auto mf = m[obj_field]; EXPECT_TRUE(mf.has_value()); EXPECT_TRUE(mf.value().IsInt64()); EXPECT_EQ(16, mf.value().Int64OrDie()); } + { + std::string undefined = "f_undefined"; + CelValue undefined_field = CelValue::CreateStringView(undefined); + auto presence = m.Has(undefined_field); + EXPECT_OK(presence); + EXPECT_FALSE(*presence); + auto v = m[undefined_field]; + EXPECT_FALSE(v.has_value()); + + presence = m.Has(CelValue::CreateBool(false)); + EXPECT_FALSE(presence.ok()); + EXPECT_EQ(presence.status().code(), absl::StatusCode::kInvalidArgument); + } } TEST_F(FlatBuffersTest, ObjectFieldDefault) { @@ -389,13 +420,21 @@ TEST_F(FlatBuffersTest, ObjectVectorField) { const CelMap& m = *l[0].MapOrDie(); EXPECT_EQ(2, m.size()); { - auto mf = m[CelValue::CreateStringView(kStringField)]; + CelValue field = CelValue::CreateStringView(kStringField); + auto presence = m.Has(field); + EXPECT_OK(presence); + EXPECT_TRUE(*presence); + auto mf = m[field]; EXPECT_TRUE(mf.has_value()); EXPECT_TRUE(mf.value().IsString()); EXPECT_EQ("entry", mf.value().StringOrDie().value()); } { - auto mf = m[CelValue::CreateStringView(kIntField)]; + CelValue field = CelValue::CreateStringView(kIntField); + auto presence = m.Has(field); + EXPECT_OK(presence); + EXPECT_TRUE(*presence); + auto mf = m[field]; EXPECT_TRUE(mf.has_value()); EXPECT_TRUE(mf.value().IsInt64()); EXPECT_EQ(16, mf.value().Int64OrDie()); @@ -406,17 +445,36 @@ TEST_F(FlatBuffersTest, ObjectVectorField) { const CelMap& m = *l[1].MapOrDie(); EXPECT_EQ(2, m.size()); { - auto mf = m[CelValue::CreateStringView(kStringField)]; + CelValue field = CelValue::CreateStringView(kStringField); + auto presence = m.Has(field); + EXPECT_OK(presence); + // Note, the presence checks on flat buffers seem to only apply to whether + // the field is defined. + EXPECT_TRUE(*presence); + auto mf = m[field]; EXPECT_TRUE(mf.has_value()); EXPECT_TRUE(mf.value().IsString()); EXPECT_EQ("", mf.value().StringOrDie().value()); } { - auto mf = m[CelValue::CreateStringView(kIntField)]; + CelValue field = CelValue::CreateStringView(kIntField); + auto presence = m.Has(field); + EXPECT_OK(presence); + EXPECT_TRUE(*presence); + auto mf = m[field]; EXPECT_TRUE(mf.has_value()); EXPECT_TRUE(mf.value().IsInt64()); EXPECT_EQ(32, mf.value().Int64OrDie()); } + { + std::string undefined = "f_undefined"; + CelValue field = CelValue::CreateStringView(undefined); + auto presence = m.Has(field); + EXPECT_OK(presence); + EXPECT_FALSE(*presence); + auto mf = m[field]; + EXPECT_FALSE(mf.has_value()); + } } } @@ -522,17 +580,39 @@ TEST_F(FlatBuffersTest, IndexedObjectVectorFieldDefaults) { } ] })"); - auto f = value[CelValue::CreateStringView(kIndexedField)]; + CelValue field = CelValue::CreateStringView(kIndexedField); + auto presence = value.Has(field); + EXPECT_OK(presence); + EXPECT_TRUE(*presence); + auto f = value[field]; EXPECT_TRUE(f.has_value()); EXPECT_TRUE(f.value().IsMap()); const CelMap& m = *f.value().MapOrDie(); + EXPECT_EQ(1, m.size()); const CelList& l = *m.ListKeys(); EXPECT_EQ(1, l.size()); EXPECT_TRUE(l[0].IsString()); EXPECT_EQ("", l[0].StringOrDie().value()); - auto v = m[CelValue::CreateStringView(absl::string_view())]; + + CelValue map_field = CelValue::CreateStringView(absl::string_view()); + presence = m.Has(map_field); + EXPECT_OK(presence); + EXPECT_TRUE(*presence); + auto v = m[map_field]; EXPECT_TRUE(v.has_value()); + + std::string undefined = "f_undefined"; + CelValue undefined_field = CelValue::CreateStringView(undefined); + presence = m.Has(undefined_field); + EXPECT_OK(presence); + EXPECT_FALSE(*presence); + v = m[undefined_field]; + EXPECT_FALSE(v.has_value()); + + presence = m.Has(CelValue::CreateBool(false)); + EXPECT_FALSE(presence.ok()); + EXPECT_EQ(presence.status().code(), absl::StatusCode::kInvalidArgument); } } // namespace From 16b9285b724967e092d3487a520edb5493d13988 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Wed, 19 May 2021 14:39:20 -0400 Subject: [PATCH 09/12] Separate ValueStack from evaluator core. Rename to EvaluatorStack. PiperOrigin-RevId: 374693139 --- eval/eval/BUILD | 26 ++++++ eval/eval/evaluator_core.cc | 13 +-- eval/eval/evaluator_core.h | 125 ++-------------------------- eval/eval/evaluator_core_test.cc | 46 ---------- eval/eval/evaluator_stack.cc | 22 +++++ eval/eval/evaluator_stack.h | 134 ++++++++++++++++++++++++++++++ eval/eval/evaluator_stack_test.cc | 78 +++++++++++++++++ 7 files changed, 266 insertions(+), 178 deletions(-) create mode 100644 eval/eval/evaluator_stack.cc create mode 100644 eval/eval/evaluator_stack.h create mode 100644 eval/eval/evaluator_stack_test.cc diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 68cdf8f4b..073134033 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -17,6 +17,7 @@ cc_library( deps = [ ":attribute_trail", ":attribute_utility", + ":evaluator_stack", "//base:status_macros", "//eval/public:activation", "//eval/public:cel_attribute", @@ -33,6 +34,31 @@ cc_library( ], ) +cc_library( + name = "evaluator_stack", + srcs = [ + "evaluator_stack.cc", + ], + hdrs = [ + "evaluator_stack.h", + ], + deps = [ + ":attribute_trail", + "//eval/public:cel_value", + ], +) + +cc_test( + name = "evaluator_stack_test", + srcs = [ + "evaluator_stack_test.cc", + ], + deps = [ + ":evaluator_stack", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "expression_step_base", hdrs = [ diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index fc68104ba..54d5672d8 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -34,17 +34,6 @@ absl::Status CheckIterAccess(CelExpressionFlatEvaluationState* state, } // namespace -void ValueStack::Clear() { - for (auto& v : stack_) { - v = CelValue(); - } - for (auto& attr : attribute_stack_) { - attr = AttributeTrail(); - } - - current_size_ = 0; -} - CelExpressionFlatEvaluationState::CelExpressionFlatEvaluationState( size_t value_stack_size, const std::set& iter_variable_names, google::protobuf::Arena* arena) @@ -171,7 +160,7 @@ absl::StatusOr CelExpressionFlatImpl::Trace( enable_unknowns_, enable_unknown_function_results_, enable_missing_attribute_errors_); - ValueStack* stack = &frame.value_stack(); + EvaluatorStack* stack = &frame.value_stack(); size_t initial_stack_size = stack->size(); const ExpressionStep* expr; while ((expr = frame.Next()) != nullptr) { diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index b9d05c583..55c999b98 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -20,6 +20,7 @@ #include "absl/types/span.h" #include "eval/eval/attribute_trail.h" #include "eval/eval/attribute_utility.h" +#include "eval/eval/evaluator_stack.h" #include "eval/public/activation.h" #include "eval/public/cel_attribute.h" #include "eval/public/cel_expression.h" @@ -42,7 +43,7 @@ class ExpressionStep { virtual ~ExpressionStep() {} // Performs actual evaluation. - // Values are passed between Expression objects via ValueStack, which is + // Values are passed between Expression objects via EvaluatorStack, which is // supplied with context. // Also, Expression gets values supplied by caller though Activation // interface. @@ -64,122 +65,6 @@ class ExpressionStep { using ExecutionPath = std::vector>; -// CelValue stack. -// Implementation is based on vector to allow passing parameters from -// stack as Span<>. -class ValueStack { - public: - explicit ValueStack(size_t max_size) : current_size_(0) { - stack_.resize(max_size); - attribute_stack_.resize(max_size); - } - - // Return the current stack size. - size_t size() const { return current_size_; } - - // Return the maximum size of the stack. - size_t max_size() const { return stack_.size(); } - - // Returns true if stack is empty. - bool empty() const { return current_size_ == 0; } - - // Attributes stack size. - size_t attribute_size() const { return current_size_; } - - // Check that stack has enough elements. - bool HasEnough(size_t size) const { return current_size_ >= size; } - - // Dumps the entire stack state as is. - void Clear(); - - // Gets the last size elements of the stack. - // Checking that stack has enough elements is caller's responsibility. - // Please note that calls to Push may invalidate returned Span object. - absl::Span GetSpan(size_t size) const { - if (!HasEnough(size)) { - GOOGLE_LOG(ERROR) << "Requested span size (" << size - << ") exceeds current stack size: " << current_size_; - } - return absl::Span(stack_.data() + current_size_ - size, - size); - } - - // Gets the last size attribute trails of the stack. - // Checking that stack has enough elements is caller's responsibility. - // Please note that calls to Push may invalidate returned Span object. - absl::Span GetAttributeSpan(size_t size) const { - return absl::Span( - attribute_stack_.data() + current_size_ - size, size); - } - - // Peeks the last element of the stack. - // Checking that stack is not empty is caller's responsibility. - const CelValue& Peek() const { - if (empty()) { - GOOGLE_LOG(ERROR) << "Peeking on empty ValueStack"; - } - return stack_[current_size_ - 1]; - } - - // Peeks the last element of the attribute stack. - // Checking that stack is not empty is caller's responsibility. - const AttributeTrail& PeekAttribute() const { - if (empty()) { - GOOGLE_LOG(ERROR) << "Peeking on empty ValueStack"; - } - return attribute_stack_[current_size_ - 1]; - } - - // Clears the last size elements of the stack. - // Checking that stack has enough elements is caller's responsibility. - void Pop(size_t size) { - if (!HasEnough(size)) { - GOOGLE_LOG(ERROR) << "Trying to pop more elements (" << size - << ") than the current stack size: " << current_size_; - } - current_size_ -= size; - } - - // Put element on the top of the stack. - void Push(const CelValue& value) { Push(value, AttributeTrail()); } - - void Push(const CelValue& value, AttributeTrail attribute) { - if (current_size_ >= stack_.size()) { - GOOGLE_LOG(ERROR) << "No room to push more elements on to ValueStack"; - } - stack_[current_size_] = value; - attribute_stack_[current_size_] = attribute; - current_size_++; - } - - // Replace element on the top of the stack. - // Checking that stack is not empty is caller's responsibility. - void PopAndPush(const CelValue& value) { - PopAndPush(value, AttributeTrail()); - } - - // Replace element on the top of the stack. - // Checking that stack is not empty is caller's responsibility. - void PopAndPush(const CelValue& value, AttributeTrail attribute) { - if (empty()) { - GOOGLE_LOG(ERROR) << "Cannot PopAndPush on empty stack."; - } - stack_[current_size_ - 1] = value; - attribute_stack_[current_size_ - 1] = attribute; - } - - // Preallocate stack. - void Reserve(size_t size) { - stack_.reserve(size); - attribute_stack_.reserve(size); - } - - private: - std::vector stack_; - std::vector attribute_stack_; - size_t current_size_; -}; - class CelExpressionFlatEvaluationState : public CelEvaluationState { public: CelExpressionFlatEvaluationState( @@ -196,7 +81,7 @@ class CelExpressionFlatEvaluationState : public CelEvaluationState { void Reset(); - ValueStack& value_stack() { return value_stack_; } + EvaluatorStack& value_stack() { return value_stack_; } std::vector& iter_stack() { return iter_stack_; } @@ -207,7 +92,7 @@ class CelExpressionFlatEvaluationState : public CelEvaluationState { google::protobuf::Arena* arena() { return arena_; } private: - ValueStack value_stack_; + EvaluatorStack value_stack_; std::set iter_variable_names_; std::vector iter_stack_; google::protobuf::Arena* arena_; @@ -254,7 +139,7 @@ class ExecutionFrame { return absl::OkStatus(); } - ValueStack& value_stack() { return state_->value_stack(); } + EvaluatorStack& value_stack() { return state_->value_stack(); } bool enable_unknowns() const { return enable_unknowns_; } bool enable_unknown_function_results() const { return enable_unknown_function_results_; diff --git a/eval/eval/evaluator_core_test.cc b/eval/eval/evaluator_core_test.cc index 5d2d7d6cb..b922bb888 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -53,52 +53,6 @@ class FakeIncrementExpressionStep : public ExpressionStep { bool ComesFromAst() const override { return true; } }; -// Test Value Stack Push/Pop operation -TEST(EvaluatorCoreTest, ValueStackPushPop) { - google::protobuf::Arena arena; - google::api::expr::v1alpha1::Expr expr; - expr.mutable_ident_expr()->set_name("name"); - CelAttribute attribute(expr, {}); - ValueStack stack(10); - stack.Push(CelValue::CreateInt64(1)); - stack.Push(CelValue::CreateInt64(2), AttributeTrail()); - stack.Push(CelValue::CreateInt64(3), AttributeTrail(expr, &arena)); - - ASSERT_EQ(stack.Peek().Int64OrDie(), 3); - ASSERT_THAT(stack.PeekAttribute().attribute(), NotNull()); - ASSERT_EQ(*stack.PeekAttribute().attribute(), attribute); - - stack.Pop(1); - - ASSERT_EQ(stack.Peek().Int64OrDie(), 2); - ASSERT_EQ(stack.PeekAttribute().attribute(), nullptr); - - stack.Pop(1); - - ASSERT_EQ(stack.Peek().Int64OrDie(), 1); - ASSERT_EQ(stack.PeekAttribute().attribute(), nullptr); -} - -// Test that inner stacks within value stack retain the equality of their sizes. -TEST(EvaluatorCoreTest, ValueStackBalanced) { - ValueStack stack(10); - ASSERT_EQ(stack.size(), stack.attribute_size()); - - stack.Push(CelValue::CreateInt64(1)); - ASSERT_EQ(stack.size(), stack.attribute_size()); - stack.Push(CelValue::CreateInt64(2), AttributeTrail()); - stack.Push(CelValue::CreateInt64(3), AttributeTrail()); - ASSERT_EQ(stack.size(), stack.attribute_size()); - - stack.PopAndPush(CelValue::CreateInt64(4), AttributeTrail()); - ASSERT_EQ(stack.size(), stack.attribute_size()); - stack.PopAndPush(CelValue::CreateInt64(5)); - ASSERT_EQ(stack.size(), stack.attribute_size()); - - stack.Pop(3); - ASSERT_EQ(stack.size(), stack.attribute_size()); -} - TEST(EvaluatorCoreTest, ExecutionFrameNext) { ExecutionPath path; auto const_step = absl::make_unique(); diff --git a/eval/eval/evaluator_stack.cc b/eval/eval/evaluator_stack.cc new file mode 100644 index 000000000..34907cb97 --- /dev/null +++ b/eval/eval/evaluator_stack.cc @@ -0,0 +1,22 @@ +#include "eval/eval/evaluator_stack.h" + +namespace google { +namespace api { +namespace expr { +namespace runtime { + +void EvaluatorStack::Clear() { + for (auto& v : stack_) { + v = CelValue(); + } + for (auto& attr : attribute_stack_) { + attr = AttributeTrail(); + } + + current_size_ = 0; +} + +} // namespace runtime +} // namespace expr +} // namespace api +} // namespace google diff --git a/eval/eval/evaluator_stack.h b/eval/eval/evaluator_stack.h new file mode 100644 index 000000000..4521acb53 --- /dev/null +++ b/eval/eval/evaluator_stack.h @@ -0,0 +1,134 @@ +#ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_EVALUATOR_STACK_H_ +#define THIRD_PARTY_CEL_CPP_EVAL_EVAL_EVALUATOR_STACK_H_ + +#include + +#include "eval/eval/attribute_trail.h" +#include "eval/public/cel_value.h" + +namespace google { +namespace api { +namespace expr { +namespace runtime { + +// CelValue stack. +// Implementation is based on vector to allow passing parameters from +// stack as Span<>. +class EvaluatorStack { + public: + explicit EvaluatorStack(size_t max_size) : current_size_(0) { + stack_.resize(max_size); + attribute_stack_.resize(max_size); + } + + // Return the current stack size. + size_t size() const { return current_size_; } + + // Return the maximum size of the stack. + size_t max_size() const { return stack_.size(); } + + // Returns true if stack is empty. + bool empty() const { return current_size_ == 0; } + + // Attributes stack size. + size_t attribute_size() const { return current_size_; } + + // Check that stack has enough elements. + bool HasEnough(size_t size) const { return current_size_ >= size; } + + // Dumps the entire stack state as is. + void Clear(); + + // Gets the last size elements of the stack. + // Checking that stack has enough elements is caller's responsibility. + // Please note that calls to Push may invalidate returned Span object. + absl::Span GetSpan(size_t size) const { + if (!HasEnough(size)) { + GOOGLE_LOG(ERROR) << "Requested span size (" << size + << ") exceeds current stack size: " << current_size_; + } + return absl::Span(stack_.data() + current_size_ - size, + size); + } + + // Gets the last size attribute trails of the stack. + // Checking that stack has enough elements is caller's responsibility. + // Please note that calls to Push may invalidate returned Span object. + absl::Span GetAttributeSpan(size_t size) const { + return absl::Span( + attribute_stack_.data() + current_size_ - size, size); + } + + // Peeks the last element of the stack. + // Checking that stack is not empty is caller's responsibility. + const CelValue& Peek() const { + if (empty()) { + GOOGLE_LOG(ERROR) << "Peeking on empty EvaluatorStack"; + } + return stack_[current_size_ - 1]; + } + + // Peeks the last element of the attribute stack. + // Checking that stack is not empty is caller's responsibility. + const AttributeTrail& PeekAttribute() const { + if (empty()) { + GOOGLE_LOG(ERROR) << "Peeking on empty EvaluatorStack"; + } + return attribute_stack_[current_size_ - 1]; + } + + // Clears the last size elements of the stack. + // Checking that stack has enough elements is caller's responsibility. + void Pop(size_t size) { + if (!HasEnough(size)) { + GOOGLE_LOG(ERROR) << "Trying to pop more elements (" << size + << ") than the current stack size: " << current_size_; + } + current_size_ -= size; + } + + // Put element on the top of the stack. + void Push(const CelValue& value) { Push(value, AttributeTrail()); } + + void Push(const CelValue& value, AttributeTrail attribute) { + if (current_size_ >= stack_.size()) { + GOOGLE_LOG(ERROR) << "No room to push more elements on to EvaluatorStack"; + } + stack_[current_size_] = value; + attribute_stack_[current_size_] = attribute; + current_size_++; + } + + // Replace element on the top of the stack. + // Checking that stack is not empty is caller's responsibility. + void PopAndPush(const CelValue& value) { + PopAndPush(value, AttributeTrail()); + } + + // Replace element on the top of the stack. + // Checking that stack is not empty is caller's responsibility. + void PopAndPush(const CelValue& value, AttributeTrail attribute) { + if (empty()) { + GOOGLE_LOG(ERROR) << "Cannot PopAndPush on empty stack."; + } + stack_[current_size_ - 1] = value; + attribute_stack_[current_size_ - 1] = attribute; + } + + // Preallocate stack. + void Reserve(size_t size) { + stack_.reserve(size); + attribute_stack_.reserve(size); + } + + private: + std::vector stack_; + std::vector attribute_stack_; + size_t current_size_; +}; + +} // namespace runtime +} // namespace expr +} // namespace api +} // namespace google +#endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_EVALUATOR_STACK_H_ diff --git a/eval/eval/evaluator_stack_test.cc b/eval/eval/evaluator_stack_test.cc new file mode 100644 index 000000000..4f2970224 --- /dev/null +++ b/eval/eval/evaluator_stack_test.cc @@ -0,0 +1,78 @@ +#include "eval/eval/evaluator_stack.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace google { +namespace api { +namespace expr { +namespace runtime { +namespace { + +using testing::NotNull; + +// Test Value Stack Push/Pop operation +TEST(EvaluatorStackTest, StackPushPop) { + google::protobuf::Arena arena; + google::api::expr::v1alpha1::Expr expr; + expr.mutable_ident_expr()->set_name("name"); + CelAttribute attribute(expr, {}); + EvaluatorStack stack(10); + stack.Push(CelValue::CreateInt64(1)); + stack.Push(CelValue::CreateInt64(2), AttributeTrail()); + stack.Push(CelValue::CreateInt64(3), AttributeTrail(expr, &arena)); + + ASSERT_EQ(stack.Peek().Int64OrDie(), 3); + ASSERT_THAT(stack.PeekAttribute().attribute(), NotNull()); + ASSERT_EQ(*stack.PeekAttribute().attribute(), attribute); + + stack.Pop(1); + + ASSERT_EQ(stack.Peek().Int64OrDie(), 2); + ASSERT_EQ(stack.PeekAttribute().attribute(), nullptr); + + stack.Pop(1); + + ASSERT_EQ(stack.Peek().Int64OrDie(), 1); + ASSERT_EQ(stack.PeekAttribute().attribute(), nullptr); +} + +// Test that inner stacks within value stack retain the equality of their sizes. +TEST(EvaluatorStackTest, StackBalanced) { + EvaluatorStack stack(10); + ASSERT_EQ(stack.size(), stack.attribute_size()); + + stack.Push(CelValue::CreateInt64(1)); + ASSERT_EQ(stack.size(), stack.attribute_size()); + stack.Push(CelValue::CreateInt64(2), AttributeTrail()); + stack.Push(CelValue::CreateInt64(3), AttributeTrail()); + ASSERT_EQ(stack.size(), stack.attribute_size()); + + stack.PopAndPush(CelValue::CreateInt64(4), AttributeTrail()); + ASSERT_EQ(stack.size(), stack.attribute_size()); + stack.PopAndPush(CelValue::CreateInt64(5)); + ASSERT_EQ(stack.size(), stack.attribute_size()); + + stack.Pop(3); + ASSERT_EQ(stack.size(), stack.attribute_size()); +} + +TEST(EvaluatorStackTest, Clear) { + EvaluatorStack stack(10); + ASSERT_EQ(stack.size(), stack.attribute_size()); + + stack.Push(CelValue::CreateInt64(1)); + stack.Push(CelValue::CreateInt64(2), AttributeTrail()); + stack.Push(CelValue::CreateInt64(3), AttributeTrail()); + ASSERT_EQ(stack.size(), 3); + + stack.Clear(); + ASSERT_EQ(stack.size(), 0); + ASSERT_TRUE(stack.empty()); +} + +} // namespace +} // namespace runtime +} // namespace expr +} // namespace api +} // namespace google From 61aeb51a2d8ae19738684fe18046fe965562d560 Mon Sep 17 00:00:00 2001 From: kuat Date: Mon, 24 May 2021 12:29:14 -0400 Subject: [PATCH 10/12] Fix constant suffix. "l" maybe 32 bits, "ll" is 64 bits. Windows compiler does not like this initialization. PiperOrigin-RevId: 375491910 --- eval/public/structs/cel_proto_wrapper.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eval/public/structs/cel_proto_wrapper.cc b/eval/public/structs/cel_proto_wrapper.cc index 2a7dfc81f..d9eaeaa37 100644 --- a/eval/public/structs/cel_proto_wrapper.cc +++ b/eval/public/structs/cel_proto_wrapper.cc @@ -50,7 +50,7 @@ using google::protobuf::UInt64Value; using google::protobuf::Value; // kMaxIntJSON is defined as the Number.MAX_SAFE_INTEGER value per EcmaScript 6. -constexpr int64_t kMaxIntJSON = (1l << 53) - 1; +constexpr int64_t kMaxIntJSON = (1ll << 53) - 1; // kMinIntJSON is defined as the Number.MIN_SAFE_INTEGER value per EcmaScript 6. constexpr int64_t kMinIntJSON = -kMaxIntJSON; From 4f3bddffeb9e887121dac45459149a91cf58c1bc Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Thu, 3 Jun 2021 13:25:27 -0400 Subject: [PATCH 11/12] Delete debug_string library. Prefer the CelValue.DebugString() method. PiperOrigin-RevId: 377318595 --- eval/public/testing/BUILD | 37 ------ eval/public/testing/debug_string.cc | 151 --------------------- eval/public/testing/debug_string.h | 26 ---- eval/public/testing/debug_string_test.cc | 162 ----------------------- eval/public/testing/matchers.cc | 5 +- eval/public/testing/matchers.h | 1 - 6 files changed, 2 insertions(+), 380 deletions(-) delete mode 100644 eval/public/testing/debug_string.cc delete mode 100644 eval/public/testing/debug_string.h delete mode 100644 eval/public/testing/debug_string_test.cc diff --git a/eval/public/testing/BUILD b/eval/public/testing/BUILD index 61f75a421..785ad714a 100644 --- a/eval/public/testing/BUILD +++ b/eval/public/testing/BUILD @@ -5,48 +5,11 @@ package( licenses(["notice"]) # Apache 2.0 -cc_library( - name = "debug_string", - srcs = ["debug_string.cc"], - hdrs = ["debug_string.h"], - deps = [ - "//eval/public:cel_attribute", - "//eval/public:cel_value", - "//eval/public:unknown_function_result_set", - "//eval/public:unknown_set", - "@com_google_absl//absl/strings", - "@com_google_absl//absl/strings:str_format", - "@com_google_absl//absl/time", - "@com_google_protobuf//:protobuf", - ], -) - -cc_test( - name = "debug_string_test", - srcs = ["debug_string_test.cc"], - deps = [ - ":debug_string", - "//eval/public:cel_attribute", - "//eval/public:cel_function", - "//eval/public:cel_value", - "//eval/public:unknown_attribute_set", - "//eval/public:unknown_function_result_set", - "//eval/public:unknown_set", - "//eval/public/structs:cel_proto_wrapper", - "//eval/testutil:test_message_cc_proto", - "@com_google_absl//absl/status", - "@com_google_absl//absl/time", - "@com_google_googletest//:gtest_main", - "@com_google_protobuf//:protobuf", - ], -) - cc_library( name = "matchers", srcs = ["matchers.cc"], hdrs = ["matchers.h"], deps = [ - ":debug_string", "//eval/public:cel_value", "//eval/public:set_util", "//eval/public:unknown_set", diff --git a/eval/public/testing/debug_string.cc b/eval/public/testing/debug_string.cc deleted file mode 100644 index 5496e11c0..000000000 --- a/eval/public/testing/debug_string.cc +++ /dev/null @@ -1,151 +0,0 @@ -#include "eval/public/testing/debug_string.h" - -#include "google/protobuf/message.h" -#include "absl/strings/escaping.h" -#include "absl/strings/str_cat.h" -#include "absl/strings/str_format.h" -#include "absl/strings/str_join.h" -#include "absl/strings/substitute.h" -#include "absl/time/time.h" -#include "eval/public/cel_attribute.h" -#include "eval/public/unknown_function_result_set.h" -#include "eval/public/unknown_set.h" - -namespace google { -namespace api { -namespace expr { -namespace runtime { -namespace test { - -namespace { - -// Forward declare -- depends on value string visitor. -std::string AttributeString(const CelAttribute* attr); - -std::string FunctionCallString(const UnknownFunctionResult* fn); - -struct ValueStringVisitor { - std::string operator()(int64_t arg) { return absl::StrFormat("%d", arg); } - - std::string operator()(uint64_t arg) { return absl::StrFormat("%d", arg); } - - std::string operator()(bool arg) { return (arg) ? "true" : "false"; } - - std::string operator()(double arg) { return absl::StrFormat("%f", arg); } - - std::string operator()(const google::protobuf::Message* arg) { - if (arg == nullptr) { - return "NULL"; - } - return arg->DebugString(); - } - std::string operator()(CelValue::StringHolder arg) { - return absl::StrFormat("'%s'", arg.value()); - } - - std::string operator()(CelValue::BytesHolder arg) { - return absl::StrFormat("0x%s", absl::BytesToHexString(arg.value())); - } - - std::string operator()(absl::Time arg) { return absl::FormatTime(arg); } - - std::string operator()(absl::Duration arg) { - return absl::FormatDuration(arg); - } - - std::string operator()(const CelList* arg) { - std::vector elements; - elements.reserve(arg->size()); - for (int i = 0; i < arg->size(); i++) { - elements.push_back(DebugString(arg->operator[](i))); - } - return absl::StrCat("[", absl::StrJoin(elements, ", "), "]"); - } - - std::string operator()(const CelMap* arg) { - const CelList* keys = arg->ListKeys(); - std::vector elements; - elements.reserve(keys->size()); - for (int i = 0; i < keys->size(); i++) { - elements.push_back( - absl::Substitute("$0:$1", DebugString((*keys)[i]), - DebugString(arg->operator[]((*keys)[i]).value()))); - } - return absl::Substitute("{$0}", absl::StrJoin(elements, ", ")); - } - - std::string operator()(CelValue::CelTypeHolder arg) { - return absl::StrFormat("'%s'", arg.value()); - } - - std::string operator()(const CelError* arg) { return arg->ToString(); } - - std::string operator()(const UnknownSet* arg) { - std::vector attrs; - attrs.reserve(arg->unknown_attributes().attributes().size()); - for (const auto* attr : arg->unknown_attributes().attributes()) { - attrs.push_back(AttributeString(attr)); - } - std::vector fns; - fns.reserve( - arg->unknown_function_results().unknown_function_results().size()); - for (const auto* fn : - arg->unknown_function_results().unknown_function_results()) { - fns.push_back(FunctionCallString(fn)); - } - return absl::Substitute("{attributes:[$0], functions:[$1]}", - absl::StrJoin(attrs, ", "), - absl::StrJoin(fns, ", ")); - } -}; - -std::string AttributeString(const CelAttribute* attr) { - // qualification = - std::string output(attr->variable().ident_expr().name()); - for (const auto& q : attr->qualifier_path()) { - absl::StrAppend(&output, ".", q.Visit(ValueStringVisitor())); - } - return output; -} - -std::string FunctionCallString(const UnknownFunctionResult* fn) { - std::vector args; - std::string call; - args.reserve(fn->arguments().size()); - if (fn->descriptor().receiver_style()) { - if (fn->arguments().empty()) { - absl::StrAppend(&call, "."); - } else { - absl::StrAppend(&call, DebugString(fn->arguments()[0]), "."); - } - absl::StrAppend(&call, fn->descriptor().name()); - for (size_t i = 1; i < fn->arguments().size(); i++) { - args.push_back(DebugString(fn->arguments()[i])); - } - } else { - absl::StrAppend(&call, fn->descriptor().name()); - for (size_t i = 0; i < fn->arguments().size(); i++) { - args.push_back(DebugString(fn->arguments()[i])); - } - } - return absl::Substitute("$0($1)", call, absl::StrJoin(args, ", ")); -} - -} // namespace - -// String rerpesentation of the underlying value. -std::string DebugValueString(const CelValue& value) { - return value.Visit(ValueStringVisitor()); -} - -// String representation of the cel value. -std::string DebugString(const CelValue& value) { - return absl::Substitute("<$0,$1>", CelValue::TypeName(value.type()), - DebugValueString(value)); -} - -} // namespace test -} // namespace runtime -} // namespace expr -} // namespace api -} // namespace google diff --git a/eval/public/testing/debug_string.h b/eval/public/testing/debug_string.h deleted file mode 100644 index 2060fc777..000000000 --- a/eval/public/testing/debug_string.h +++ /dev/null @@ -1,26 +0,0 @@ -#ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_TESTING_DEBUG_STRING_H_ -#define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_TESTING_DEBUG_STRING_H_ - -#include "eval/public/cel_value.h" - -namespace google { -namespace api { -namespace expr { -namespace runtime { -namespace test { - -// String rerpesentation of the underlying value. -std::string DebugValueString(const CelValue& value); - -// String representation of the cel value. This should only be used for -// informational purposes and the exact format may change. In particular, -// ordering is not guaranteed for some container types. -std::string DebugString(const CelValue& value); - -} // namespace test -} // namespace runtime -} // namespace expr -} // namespace api -} // namespace google - -#endif // THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_TESTING_DEBUG_STRING_H_ diff --git a/eval/public/testing/debug_string_test.cc b/eval/public/testing/debug_string_test.cc deleted file mode 100644 index 5ff9dd497..000000000 --- a/eval/public/testing/debug_string_test.cc +++ /dev/null @@ -1,162 +0,0 @@ -#include "eval/public/testing/debug_string.h" - -#include "google/protobuf/struct.pb.h" -#include "google/protobuf/arena.h" -#include "gmock/gmock.h" -#include "gtest/gtest.h" -#include "absl/status/status.h" -#include "absl/time/time.h" -#include "eval/public/cel_attribute.h" -#include "eval/public/cel_function.h" -#include "eval/public/cel_value.h" -#include "eval/public/structs/cel_proto_wrapper.h" -#include "eval/public/unknown_attribute_set.h" -#include "eval/public/unknown_function_result_set.h" -#include "eval/public/unknown_set.h" -#include "eval/testutil/test_message.pb.h" - -namespace google { -namespace api { -namespace expr { -namespace runtime { -namespace test { -namespace { -using testing::AnyOf; -using testing::HasSubstr; - -TEST(DebugString, Primitives) { - constexpr char data[] = { - '\x01', '\x01', '\x00', '\xc0', '\xff', - }; - std::string bytestring(data, 5); - CelError error = absl::InternalError("error"); - - EXPECT_EQ(DebugString(CelValue::CreateStringView("hello world")), - ""); - EXPECT_EQ(DebugString(CelValue::CreateBytes(&bytestring)), - ""); - EXPECT_EQ(DebugString(CelValue::CreateBool(false)), ""); - EXPECT_EQ(DebugString(CelValue::CreateDouble(1.5)), ""); - EXPECT_EQ(DebugString(CelValue::CreateDuration(absl::Seconds(2))), - ""); - EXPECT_THAT(DebugString(CelValue::CreateTimestamp(absl::FromUnixSeconds(1))), - AnyOf(HasSubstr(""); - EXPECT_EQ(DebugString(CelValue::CreateInt64(-1)), - ""); // no transform - - EXPECT_EQ(DebugString(CelValue::CreateUint64(1)), - ""); // no transform -} - -TEST(DebugString, Messages) { - google::protobuf::Arena arena; - TestMessage message; - message.add_int64_list(1); - message.add_int64_list(2); - - EXPECT_EQ(DebugString(CelValue::CreateNull()), ""); - EXPECT_EQ(DebugString(CelProtoWrapper::CreateMessage(&message, &arena)), - ""); -} - -TEST(DebugString, Lists) { - google::protobuf::Arena arena; - - protobuf::ListValue list_msg; - list_msg.add_values()->set_bool_value(true); - list_msg.add_values()->set_bool_value(false); - - // converted to a list - EXPECT_EQ(DebugString(CelProtoWrapper::CreateMessage(&list_msg, &arena)), - ", ]>"); -} - -TEST(DebugString, Maps) { - google::protobuf::Arena arena; - - // Converted to a map on CelValue::Create. - protobuf::Struct struct_msg; - (*struct_msg.mutable_fields())["field1"].set_bool_value(true); - (*struct_msg.mutable_fields())["field2"].set_bool_value(false); - - // Ordering isn't guaranteed for the backing map for a converted struct. - EXPECT_THAT(DebugString(CelProtoWrapper::CreateMessage(&struct_msg, &arena)), - AnyOf(":, " - ":" - "}>", - ":, " - ":" - "}>")); -} - -TEST(DebugString, UnknownSet) { - google::protobuf::Arena arena; - google::api::expr::v1alpha1::Expr ident; - ident.mutable_ident_expr()->set_name("var"); - CelAttribute attr( - ident, - {CelAttributeQualifier::Create(CelValue::CreateInt64(1)), - CelAttributeQualifier::Create(CelValue::CreateStringView("field"))}); - UnknownFunctionResult function_result( - CelFunctionDescriptor("IntFn", false, {CelValue::Type::kInt64}), 1, - {CelValue::CreateInt64(1)}); - UnknownSet unknown_set(UnknownAttributeSet({&attr}), - UnknownFunctionResultSet(&function_result)); - EXPECT_EQ(DebugString(CelValue::CreateUnknownSet(&unknown_set)), - ")]}>"); // no transform -} - -TEST(DebugString, Type) { - constexpr char data[] = { - '\x01', '\x01', '\x00', '\xc0', '\xff', - }; - std::string bytestring(data, 5); - EXPECT_EQ( - DebugString(CelValue::CreateStringView("hello world").ObtainCelType()), - ""); - EXPECT_EQ(DebugString(CelValue::CreateBytes(&bytestring).ObtainCelType()), - ""); - EXPECT_EQ(DebugString(CelValue::CreateBool(false).ObtainCelType()), - ""); - EXPECT_EQ(DebugString(CelValue::CreateDouble(1.5).ObtainCelType()), - ""); - EXPECT_EQ( - DebugString(CelValue::CreateDuration(absl::Seconds(2)).ObtainCelType()), - ""); - EXPECT_EQ( - DebugString( - CelValue::CreateTimestamp(absl::FromUnixSeconds(1)).ObtainCelType()), - ""); - EXPECT_EQ(DebugString(CelValue::CreateInt64(-1).ObtainCelType()), - ""); - - EXPECT_EQ(DebugString(CelValue::CreateUint64(1).ObtainCelType()), - ""); - - google::protobuf::Arena arena; - - protobuf::ListValue list_msg; - EXPECT_EQ( - DebugString( - CelProtoWrapper::CreateMessage(&list_msg, &arena).ObtainCelType()), - ""); - // Converted to a map on CelValue::Create. - protobuf::Struct struct_msg; - EXPECT_EQ( - DebugString( - CelProtoWrapper::CreateMessage(&struct_msg, &arena).ObtainCelType()), - ""); -} - -} // namespace -} // namespace test -} // namespace runtime -} // namespace expr -} // namespace api -} // namespace google diff --git a/eval/public/testing/matchers.cc b/eval/public/testing/matchers.cc index 930df8d77..18eb8b480 100644 --- a/eval/public/testing/matchers.cc +++ b/eval/public/testing/matchers.cc @@ -4,7 +4,6 @@ #include "gtest/gtest.h" #include "absl/strings/string_view.h" #include "eval/public/set_util.h" -#include "eval/public/testing/debug_string.h" namespace google { namespace api { @@ -12,7 +11,7 @@ namespace expr { namespace runtime { void PrintTo(const CelValue& value, std::ostream* os) { - *os << test::DebugString(value); + *os << value.DebugString(); } namespace test { @@ -31,7 +30,7 @@ class CelValueEqualImpl : public MatcherInterface { } void DescribeTo(std::ostream* os) const override { - *os << DebugString(value_); + *os << value_.DebugString(); } private: diff --git a/eval/public/testing/matchers.h b/eval/public/testing/matchers.h index d2ef85677..1b59fb8bb 100644 --- a/eval/public/testing/matchers.h +++ b/eval/public/testing/matchers.h @@ -10,7 +10,6 @@ #include "absl/time/time.h" #include "eval/public/cel_value.h" #include "eval/public/set_util.h" -#include "eval/public/testing/debug_string.h" #include "eval/public/unknown_set.h" namespace google { From d414f873ff7bcbb0e36dd0f8ea04c172540677d6 Mon Sep 17 00:00:00 2001 From: kuat Date: Thu, 10 Jun 2021 15:10:19 -0400 Subject: [PATCH 12/12] OSS export. PiperOrigin-RevId: 378705509 --- eval/public/structs/BUILD | 1 + eval/public/structs/cel_proto_wrapper_test.cc | 1 + tools/BUILD | 1 + tools/flatbuffers_backed_impl_test.cc | 1 + 4 files changed, 4 insertions(+) diff --git a/eval/public/structs/BUILD b/eval/public/structs/BUILD index 22d66fc53..b7eaf7ca6 100644 --- a/eval/public/structs/BUILD +++ b/eval/public/structs/BUILD @@ -30,6 +30,7 @@ cc_test( ], deps = [ ":cel_proto_wrapper", + "//base:status_macros", "//eval/public:cel_value", "//eval/public/containers:container_backed_list_impl", "//eval/public/containers:container_backed_map_impl", diff --git a/eval/public/structs/cel_proto_wrapper_test.cc b/eval/public/structs/cel_proto_wrapper_test.cc index 08ac4b528..f5c4877a3 100644 --- a/eval/public/structs/cel_proto_wrapper_test.cc +++ b/eval/public/structs/cel_proto_wrapper_test.cc @@ -21,6 +21,7 @@ #include "eval/testutil/test_message.pb.h" #include "internal/proto_util.h" #include "testutil/util.h" +#include "base/status_macros.h" namespace google { namespace api { diff --git a/tools/BUILD b/tools/BUILD index ce509f96c..f2a447146 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -54,6 +54,7 @@ cc_test( deps = [ ":flatbuffers_backed_impl", ":flatbuffers_test_cc", + "//base:status_macros", "@com_github_google_flatbuffers//:flatbuffers", "@com_google_googletest//:gtest_main", ], diff --git a/tools/flatbuffers_backed_impl_test.cc b/tools/flatbuffers_backed_impl_test.cc index b743c56c8..3fb7459fb 100644 --- a/tools/flatbuffers_backed_impl_test.cc +++ b/tools/flatbuffers_backed_impl_test.cc @@ -4,6 +4,7 @@ #include "gtest/gtest.h" #include "flatbuffers/idl.h" #include "flatbuffers/reflection.h" +#include "base/status_macros.h" namespace google { namespace api {