diff --git a/runtime/BUILD b/runtime/BUILD index d8ba3eb7d..980767bbe 100644 --- a/runtime/BUILD +++ b/runtime/BUILD @@ -580,6 +580,11 @@ cc_library( "//base:function_adapter", "//common:type", "//common:value", + "//eval/compiler:cel_expression_builder_flat_impl", + "//eval/compiler:flat_expr_builder", + "//eval/public:cel_expression", + "//eval/public:cel_function_registry", + "//eval/public:cel_type_registry", "//internal:casts", "//internal:number", "//internal:status_macros", @@ -612,6 +617,12 @@ cc_test( "//common:kind", "//common:value", "//common:value_testing", + "//eval/public:activation", + "//eval/public:builtin_func_registrar", + "//eval/public:cel_expr_builder_factory", + "//eval/public:cel_expression", + "//eval/public:cel_options", + "//eval/public:cel_value", "//extensions/protobuf:runtime_adapter", "//internal:testing", "//internal:testing_descriptor_pool", @@ -620,6 +631,7 @@ cc_test( "@com_google_absl//absl/status", "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/types:optional", "@com_google_absl//absl/types:span", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_protobuf//:protobuf", diff --git a/runtime/optional_types.cc b/runtime/optional_types.cc index 91e5b0543..fa8ccd0d4 100644 --- a/runtime/optional_types.cc +++ b/runtime/optional_types.cc @@ -29,6 +29,11 @@ #include "base/function_adapter.h" #include "common/type.h" #include "common/value.h" +#include "eval/compiler/cel_expression_builder_flat_impl.h" +#include "eval/compiler/flat_expr_builder.h" +#include "eval/public/cel_expression.h" +#include "eval/public/cel_function_registry.h" +#include "eval/public/cel_type_registry.h" #include "internal/casts.h" #include "internal/number.h" #include "internal/status_macros.h" @@ -104,20 +109,40 @@ absl::StatusOr SelectOptionalFieldStruct( return OptionalValue::Of(std::move(field), arena); } +// Finds `key` in `map`. With kCheckHas, the key must also pass MapValue::Has, +// since legacy maps may return values for absent keys. +template +absl::StatusOr> FindMapValue( + const MapValue& map, const Value& key, + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, + google::protobuf::MessageFactory* absl_nonnull message_factory, + google::protobuf::Arena* absl_nonnull arena) { + if constexpr (kCheckHas) { + CEL_ASSIGN_OR_RETURN(Value has, + map.Has(key, descriptor_pool, message_factory, arena)); + if (auto has_bool = has.AsBool(); has_bool && !has_bool->NativeValue()) { + return absl::nullopt; + } + } + return map.Find(key, descriptor_pool, message_factory, arena); +} + +template absl::StatusOr SelectOptionalFieldMap( const MapValue& map, const StringValue& key, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) { absl::optional value; - CEL_ASSIGN_OR_RETURN(value, - map.Find(key, descriptor_pool, message_factory, arena)); + CEL_ASSIGN_OR_RETURN(value, FindMapValue(map, key, descriptor_pool, + message_factory, arena)); if (value) { return OptionalValue::Of(std::move(*value), arena); } return OptionalValue::None(); } +template absl::StatusOr SelectOptionalField( const OpaqueValue& opaque_value, const StringValue& key, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, @@ -129,8 +154,8 @@ absl::StatusOr SelectOptionalField( } auto container = optional_value->Value(); if (auto map_value = container.AsMap(); map_value) { - return SelectOptionalFieldMap(*map_value, key, descriptor_pool, - message_factory, arena); + return SelectOptionalFieldMap(*map_value, key, descriptor_pool, + message_factory, arena); } if (auto struct_value = container.AsStruct(); struct_value) { return SelectOptionalFieldStruct(*struct_value, key, descriptor_pool, @@ -141,6 +166,7 @@ absl::StatusOr SelectOptionalField( runtime_internal::CreateNoMatchingOverloadError("_[?_]"), arena); } +template absl::StatusOr MapOptIndexOptionalValue( const MapValue& map, const Value& key, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, @@ -151,32 +177,34 @@ absl::StatusOr MapOptIndexOptionalValue( // Try int/uint. auto number = internal::Number::FromDouble(double_key->NativeValue()); if (number.LosslessConvertibleToInt()) { - CEL_ASSIGN_OR_RETURN(value, - map.Find(IntValue{number.AsInt()}, descriptor_pool, - message_factory, arena)); + CEL_ASSIGN_OR_RETURN(value, FindMapValue( + map, IntValue{number.AsInt()}, + descriptor_pool, message_factory, arena)); if (value) { return OptionalValue::Of(std::move(*value), arena); } } if (number.LosslessConvertibleToUint()) { - CEL_ASSIGN_OR_RETURN(value, - map.Find(UintValue{number.AsUint()}, descriptor_pool, - message_factory, arena)); + CEL_ASSIGN_OR_RETURN(value, FindMapValue( + map, UintValue{number.AsUint()}, + descriptor_pool, message_factory, arena)); if (value) { return OptionalValue::Of(std::move(*value), arena); } } } else { - CEL_ASSIGN_OR_RETURN( - value, map.Find(key, descriptor_pool, message_factory, arena)); + CEL_ASSIGN_OR_RETURN(value, + FindMapValue(map, key, descriptor_pool, + message_factory, arena)); if (value) { return OptionalValue::Of(std::move(*value), arena); } if (auto int_key = key.AsInt(); int_key && int_key->NativeValue() >= 0) { CEL_ASSIGN_OR_RETURN( value, - map.Find(UintValue{static_cast(int_key->NativeValue())}, - descriptor_pool, message_factory, arena)); + FindMapValue( + map, UintValue{static_cast(int_key->NativeValue())}, + descriptor_pool, message_factory, arena)); if (value) { return OptionalValue::Of(std::move(*value), arena); } @@ -186,8 +214,9 @@ absl::StatusOr MapOptIndexOptionalValue( static_cast(std::numeric_limits::max())) { CEL_ASSIGN_OR_RETURN( value, - map.Find(IntValue{static_cast(uint_key->NativeValue())}, - descriptor_pool, message_factory, arena)); + FindMapValue( + map, IntValue{static_cast(uint_key->NativeValue())}, + descriptor_pool, message_factory, arena)); if (value) { return OptionalValue::Of(std::move(*value), arena); } @@ -211,6 +240,7 @@ absl::StatusOr ListOptIndexOptionalInt( return OptionalValue::Of(std::move(element), arena); } +template absl::StatusOr OptionalOptIndexOptionalValue( const OpaqueValue& opaque_value, const Value& key, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, @@ -222,8 +252,8 @@ absl::StatusOr OptionalOptIndexOptionalValue( } auto container = optional_value->Value(); if (auto map_value = container.AsMap(); map_value) { - return MapOptIndexOptionalValue(*map_value, key, descriptor_pool, - message_factory, arena); + return MapOptIndexOptionalValue( + *map_value, key, descriptor_pool, message_factory, arena); } if (auto list_value = container.AsList(); list_value) { if (auto int_value = key.AsInt(); int_value) { @@ -293,6 +323,7 @@ absl::StatusOr ListUnwrapOpt( return std::move(*builder).Build(); } +template absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry, const RuntimeOptions& options) { if (!options.enable_qualified_type_identifiers) { @@ -335,17 +366,17 @@ absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry, BinaryFunctionAdapter, MapValue, StringValue>::CreateDescriptor("_?._", false), BinaryFunctionAdapter, MapValue, StringValue>:: - WrapFunction(&SelectOptionalFieldMap))); + WrapFunction(&SelectOptionalFieldMap))); CEL_RETURN_IF_ERROR(registry.Register( BinaryFunctionAdapter, OpaqueValue, StringValue>::CreateDescriptor("_?._", false), - BinaryFunctionAdapter, OpaqueValue, - StringValue>::WrapFunction(&SelectOptionalField))); + BinaryFunctionAdapter, OpaqueValue, StringValue>:: + WrapFunction(&SelectOptionalField))); CEL_RETURN_IF_ERROR(registry.Register( BinaryFunctionAdapter, MapValue, Value>::CreateDescriptor("_[?_]", false), - BinaryFunctionAdapter, MapValue, - Value>::WrapFunction(&MapOptIndexOptionalValue))); + BinaryFunctionAdapter, MapValue, Value>:: + WrapFunction(&MapOptIndexOptionalValue))); CEL_RETURN_IF_ERROR(registry.Register( BinaryFunctionAdapter, ListValue, int64_t>::CreateDescriptor("_[?_]", false), @@ -355,7 +386,7 @@ absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry, BinaryFunctionAdapter, OpaqueValue, Value>::CreateDescriptor("_[?_]", false), BinaryFunctionAdapter, OpaqueValue, Value>:: - WrapFunction(&OptionalOptIndexOptionalValue))); + WrapFunction(&OptionalOptIndexOptionalValue))); CEL_RETURN_IF_ERROR(registry.Register( UnaryFunctionAdapter, ListValue>::CreateDescriptor( "optional.unwrap", false), @@ -384,11 +415,26 @@ absl::Status RegisterOptionalTypeFunctions(FunctionRegistry& registry, absl::Status EnableOptionalTypes(RuntimeBuilder& builder) { auto& runtime = cel::internal::down_cast( runtime_internal::RuntimeFriendAccess::GetMutableRuntime(builder)); - CEL_RETURN_IF_ERROR(RegisterOptionalTypeFunctions( + CEL_RETURN_IF_ERROR(RegisterOptionalTypeFunctions( builder.function_registry(), runtime.expr_builder().options())); CEL_RETURN_IF_ERROR(builder.type_registry().RegisterType(OptionalType())); runtime.expr_builder().enable_optional_types(); return absl::OkStatus(); } +absl::Status EnableOptionalTypes( + google::api::expr::runtime::CelExpressionBuilder& builder) { + google::api::expr::runtime::FlatExprBuilder& expr_builder = + cel::internal::down_cast< + google::api::expr::runtime::CelExpressionBuilderFlatImpl&>(builder) + .flat_expr_builder(); + CEL_RETURN_IF_ERROR(RegisterOptionalTypeFunctions( + builder.GetRegistry()->InternalGetRegistry(), expr_builder.options())); + CEL_RETURN_IF_ERROR( + builder.GetTypeRegistry()->InternalGetModernRegistry().RegisterType( + OptionalType())); + expr_builder.enable_optional_types(); + return absl::OkStatus(); +} + } // namespace cel::extensions diff --git a/runtime/optional_types.h b/runtime/optional_types.h index 7c8087175..b49473528 100644 --- a/runtime/optional_types.h +++ b/runtime/optional_types.h @@ -16,6 +16,7 @@ #define THIRD_PARTY_CEL_CPP_RUNTIME_OPTIONAL_TYPES_H_ #include "absl/status/status.h" +#include "eval/public/cel_expression.h" #include "runtime/runtime_builder.h" namespace cel::extensions { @@ -147,6 +148,14 @@ namespace cel::extensions { // msg.?elements.optFlatMap(e, e[?0]) // return the first element if present. absl::Status EnableOptionalTypes(RuntimeBuilder& builder); +// Legacy CelExpressionBuilder version of EnableOptionalTypes. The builder must +// be created with InterpreterOptions.enable_qualified_type_identifiers. +// +// On maps, `m.?k` and `m[?k]` also check `has(m.k)`, since legacy CelMap +// implementations may return values for absent keys. +absl::Status EnableOptionalTypes( + google::api::expr::runtime::CelExpressionBuilder& builder); + } // namespace cel::extensions #endif // THIRD_PARTY_CEL_CPP_RUNTIME_OPTIONAL_TYPES_H_ diff --git a/runtime/optional_types_test.cc b/runtime/optional_types_test.cc index 578b6d5bc..8147a8eb9 100644 --- a/runtime/optional_types_test.cc +++ b/runtime/optional_types_test.cc @@ -25,11 +25,18 @@ #include "absl/status/status.h" #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" +#include "absl/types/optional.h" #include "absl/types/span.h" #include "common/function_descriptor.h" #include "common/kind.h" #include "common/value.h" #include "common/value_testing.h" +#include "eval/public/activation.h" +#include "eval/public/builtin_func_registrar.h" +#include "eval/public/cel_expr_builder_factory.h" +#include "eval/public/cel_expression.h" +#include "eval/public/cel_options.h" +#include "eval/public/cel_value.h" #include "extensions/protobuf/runtime_adapter.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" @@ -57,6 +64,7 @@ using ::cel::test::OptionalValueIsEmpty; using ::cel::expr::ParsedExpr; using ::google::api::expr::parser::Parse; using ::google::api::expr::parser::ParserOptions; +using ::google::api::expr::runtime::CelValue; using ::testing::ElementsAre; using ::testing::HasSubstr; using ::testing::TestWithParam; @@ -419,5 +427,66 @@ TEST(OptionalTypesTest, CreateStruct_KeyTypeConversionError) { HasSubstr("type conversion error"))); } +// A legacy map that only has key "a", but returns a value for any key. +class DefaultingCelMap : public google::api::expr::runtime::CelMap { + public: + absl::optional operator[](CelValue key) const override { + return CelValue::CreateInt64(IsA(key) ? 1 : 0); + } + absl::StatusOr Has(const CelValue& key) const override { + return IsA(key); + } + int size() const override { return 1; } + absl::StatusOr ListKeys() + const override { + return absl::UnimplementedError("ListKeys"); + } + + private: + static bool IsA(const CelValue& key) { + return key.IsString() && key.StringOrDie().value() == "a"; + } +}; + +using LegacyOptionalTypesTest = TestWithParam; + +TEST_P(LegacyOptionalTypesTest, EvaluatesToTrue) { + google::api::expr::runtime::InterpreterOptions options; + options.enable_qualified_type_identifiers = true; + std::unique_ptr builder = + google::api::expr::runtime::CreateCelExpressionBuilder(options); + ASSERT_THAT(google::api::expr::runtime::RegisterBuiltinFunctions( + builder->GetRegistry(), options), + IsOk()); + ASSERT_THAT(EnableOptionalTypes(*builder), IsOk()); + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, + Parse(GetParam(), "", + ParserOptions{.enable_optional_syntax = true})); + ASSERT_OK_AND_ASSIGN(auto expression, builder->CreateExpression( + &expr.expr(), &expr.source_info())); + DefaultingCelMap map; + google::api::expr::runtime::Activation activation; + activation.InsertValue("m", CelValue::CreateMap(&map)); + google::protobuf::Arena arena; + ASSERT_OK_AND_ASSIGN(CelValue result, + expression->Evaluate(activation, &arena)); + EXPECT_TRUE(result.IsBool() && result.BoolOrDie()) << result.DebugString(); +} + +INSTANTIATE_TEST_SUITE_P(LegacyOptionalTypesTest, LegacyOptionalTypesTest, + testing::Values("m.?a.orValue(-1) == 1", + "m.?b.orValue(-1) == -1", + "m[?'b'].orValue(-1) == -1", + "m.?b.or(m.?a).orValue(-1) == 1", + "type(m.?a) == optional_type")); + +TEST(LegacyOptionalTypes, RequiresQualifiedTypeIdentifiers) { + std::unique_ptr builder = + google::api::expr::runtime::CreateCelExpressionBuilder( + google::api::expr::runtime::InterpreterOptions()); + EXPECT_THAT(EnableOptionalTypes(*builder), + StatusIs(absl::StatusCode::kFailedPrecondition)); +} + } // namespace } // namespace cel::extensions