diff --git a/common/descriptor_pool_type_introspector.cc b/common/descriptor_pool_type_introspector.cc index fa9ed2a0e..980b2d25d 100644 --- a/common/descriptor_pool_type_introspector.cc +++ b/common/descriptor_pool_type_introspector.cc @@ -44,8 +44,7 @@ FindStructTypeFieldByNameDirectly( if (descriptor == nullptr) { return std::nullopt; } - const google::protobuf::FieldDescriptor* absl_nullable field = - descriptor->FindFieldByName(name); + const google::protobuf::FieldDescriptor* field = descriptor->FindFieldByName(name); if (field != nullptr) { return StructTypeField(MessageTypeField(field)); } diff --git a/common/legacy_value.cc b/common/legacy_value.cc index 7fbf16732..50de9a0f8 100644 --- a/common/legacy_value.cc +++ b/common/legacy_value.cc @@ -71,14 +71,15 @@ namespace cel { namespace { -using google::api::expr::runtime::CelList; -using google::api::expr::runtime::CelMap; -using google::api::expr::runtime::CelValue; -using google::api::expr::runtime::FieldBackedListImpl; -using google::api::expr::runtime::FieldBackedMapImpl; -using google::api::expr::runtime::GetGenericProtoTypeInfoInstance; -using google::api::expr::runtime::LegacyTypeInfoApis; -using google::api::expr::runtime::MessageWrapper; +using ::google::api::expr::runtime::CelList; +using ::google::api::expr::runtime::CelMap; +using ::google::api::expr::runtime::CelValue; +using ::google::api::expr::runtime::CreateCelValueFromField; +using ::google::api::expr::runtime::FieldBackedListImpl; +using ::google::api::expr::runtime::FieldBackedMapImpl; +using ::google::api::expr::runtime::GetGenericProtoTypeInfoInstance; +using ::google::api::expr::runtime::LegacyTypeInfoApis; +using ::google::api::expr::runtime::MessageWrapper; using ::google::api::expr::runtime::internal::MaybeWrapValueToMessage; absl::Status InvalidMapKeyTypeError(ValueKind kind) { @@ -1288,6 +1289,36 @@ TypeValue CreateTypeValueFromView(google::protobuf::Arena* arena, return TypeValue(common_internal::LegacyRuntimeType(input)); } +const google::protobuf::Message* absl_nullable GetLegacyMessage(const Value& value) { + if (!common_internal::IsLegacyStructValue(value)) { + return nullptr; + } + + auto legacy = common_internal::GetLegacyStructValue(value); + const auto* legacy_type_info = legacy.legacy_type_info(); + if (legacy_type_info == nullptr) { + return nullptr; + } + if (legacy_type_info != &GetGenericProtoTypeInfoInstance()) { + return nullptr; + } + if (IsWellKnownMessageType(legacy.message_ptr()->GetDescriptor())) { + return nullptr; + } + return legacy.message_ptr(); +} + +absl::Status WrapLegacyMessageField( + const google::protobuf::Message* absl_nonnull message, + const google::protobuf::FieldDescriptor* absl_nonnull field_descriptor, + ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena, + Value* absl_nonnull out) { + CEL_ASSIGN_OR_RETURN(CelValue result, + CreateCelValueFromField(message, field_descriptor, + unboxing_option, arena)); + return ModernValue(arena, result, *out); +} + } // namespace interop_internal } // namespace cel diff --git a/common/legacy_value.h b/common/legacy_value.h index 7e703cea1..8d0392f7a 100644 --- a/common/legacy_value.h +++ b/common/legacy_value.h @@ -27,7 +27,9 @@ #include "common/value.h" #include "eval/public/cel_value.h" #include "internal/status_macros.h" +#include "runtime/runtime_options.h" #include "google/protobuf/arena.h" +#include "google/protobuf/descriptor.h" namespace cel { @@ -59,6 +61,19 @@ google::api::expr::runtime::CelValue UnsafeLegacyValue( namespace cel::interop_internal { +// Returns the underlying `google::protobuf::Message` of a `cel::Value` if it is a legacy +// message with the default type info, or `nullptr` otherwise. +const google::protobuf::Message* absl_nullable GetLegacyMessage(const Value& value); + +// Access a field on a legacy message value, writing the result to `out`. +// Prefers wrapping legacy values instead of using the modern value +// representation. +absl::Status WrapLegacyMessageField( + const google::protobuf::Message* absl_nonnull message, + const google::protobuf::FieldDescriptor* absl_nonnull field_descriptor, + ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena, + Value* absl_nonnull out); + absl::StatusOr FromLegacyValue( google::protobuf::Arena* arena, const google::api::expr::runtime::CelValue& legacy_value, diff --git a/common/type.cc b/common/type.cc index 9ea85954c..2d3469540 100644 --- a/common/type.cc +++ b/common/type.cc @@ -606,6 +606,11 @@ absl::optional StructTypeField::AsMessage() const { return std::nullopt; } +MessageTypeField StructTypeField::GetMessage() const { + ABSL_DCHECK(IsMessage()); + return absl::get(variant_); +} + StructTypeField::operator MessageTypeField() const { ABSL_DCHECK(IsMessage()); return absl::get(variant_); diff --git a/common/type.h b/common/type.h index c8851dd4e..e60452bfd 100644 --- a/common/type.h +++ b/common/type.h @@ -1140,6 +1140,7 @@ class StructTypeField final { } absl::optional AsMessage() const; + MessageTypeField GetMessage() const; explicit operator MessageTypeField() const; diff --git a/common/types/message_type.h b/common/types/message_type.h index 782af87aa..eea811971 100644 --- a/common/types/message_type.h +++ b/common/types/message_type.h @@ -87,6 +87,11 @@ class MessageType final { return descriptor_; } + const google::protobuf::Descriptor* absl_nonnull descriptor() const { + ABSL_DCHECK(*this); + return descriptor_; + } + explicit operator bool() const { return descriptor_ != nullptr; } private: @@ -166,8 +171,12 @@ class MessageTypeField final { return *descriptor_; } - const google::protobuf::FieldDescriptor* absl_nonnull operator->() const - ABSL_ATTRIBUTE_LIFETIME_BOUND { + const google::protobuf::FieldDescriptor* absl_nonnull operator->() const { + ABSL_DCHECK(*this); + return descriptor_; + } + + const google::protobuf::FieldDescriptor* absl_nonnull descriptor() const { ABSL_DCHECK(*this); return descriptor_; } diff --git a/common/value.cc b/common/value.cc index f13ee958e..6284626da 100644 --- a/common/value.cc +++ b/common/value.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include diff --git a/common/values/parsed_message_value.h b/common/values/parsed_message_value.h index f3d1f7b40..2e356d3e8 100644 --- a/common/values/parsed_message_value.h +++ b/common/values/parsed_message_value.h @@ -99,10 +99,8 @@ class ParsedMessageValue final return *value_; } - const google::protobuf::Message* absl_nonnull operator->() const - ABSL_ATTRIBUTE_LIFETIME_BOUND { - return value_; - } + const google::protobuf::Message* absl_nonnull operator->() const { return value_; } + const google::protobuf::Message* absl_nonnull message() const { return value_; } bool IsZeroValue() const; @@ -175,6 +173,15 @@ class ParsedMessageValue final swap(lhs.arena_, rhs.arena_); } + absl::Status GetField( + const google::protobuf::FieldDescriptor* absl_nonnull field, + ProtoWrapperTypeOptions unboxing_options, + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, + google::protobuf::MessageFactory* absl_nonnull message_factory, + google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const; + + bool HasField(const google::protobuf::FieldDescriptor* absl_nonnull field) const; + private: friend std::pointer_traits; friend class StructValue; @@ -203,15 +210,6 @@ class ParsedMessageValue final return absl::OkStatus(); } - absl::Status GetField( - const google::protobuf::FieldDescriptor* absl_nonnull field, - ProtoWrapperTypeOptions unboxing_options, - const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, - google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const; - - bool HasField(const google::protobuf::FieldDescriptor* absl_nonnull field) const; - const google::protobuf::Message* absl_nonnull value_; // Arena that is attributed as owning the value. May be null to indicate that // the value is managed externally. diff --git a/conformance/BUILD b/conformance/BUILD index 35d554c7b..d72237841 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -64,6 +64,7 @@ cc_library( "//runtime:constant_folding", "//runtime:optional_types", "//runtime:reference_resolver", + "//runtime:regex_precompilation", "//runtime:runtime_options", "//runtime:standard_runtime_builder_factory", "//testutil:test_macros", diff --git a/conformance/service.cc b/conformance/service.cc index d81200cad..4d3815d37 100644 --- a/conformance/service.cc +++ b/conformance/service.cc @@ -76,6 +76,7 @@ #include "runtime/constant_folding.h" #include "runtime/optional_types.h" #include "runtime/reference_resolver.h" +#include "runtime/regex_precompilation.h" #include "runtime/runtime.h" #include "runtime/runtime_options.h" #include "runtime/standard_runtime_builder_factory.h" @@ -283,6 +284,7 @@ class LegacyConformanceServiceImpl : public ConformanceServiceInterface { std::cerr << "Enabling optimizations" << std::endl; options.constant_folding = true; options.constant_arena = constant_arena; + options.enable_typed_field_access = true; } if (select_optimization) { @@ -486,6 +488,9 @@ class ModernConformanceServiceImpl : public ConformanceServiceInterface { absl::string_view container) { RuntimeOptions options(options_); options.container = std::string(container); + if (enable_optimizations_) { + options.enable_typed_field_access = true; + } CEL_ASSIGN_OR_RETURN( auto builder, CreateStandardRuntimeBuilder( google::protobuf::DescriptorPool::generated_pool(), options)); @@ -493,6 +498,7 @@ class ModernConformanceServiceImpl : public ConformanceServiceInterface { if (enable_optimizations_) { CEL_RETURN_IF_ERROR(cel::extensions::EnableConstantFolding( builder, google::protobuf::MessageFactory::generated_factory())); + CEL_RETURN_IF_ERROR(cel::extensions::EnableRegexPrecompilation(builder)); } CEL_RETURN_IF_ERROR(cel::EnableReferenceResolver( builder, cel::ReferenceResolverEnabled::kAlways)); diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index ea4dd46b3..2012abeda 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -109,6 +109,7 @@ cc_library( "//common:expr", "//common:kind", "//common:type", + "//common:type_spec_resolver", "//common:value", "//eval/eval:comprehension_step", "//eval/eval:const_value_step", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index d1435b4a3..a650657de 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -30,8 +31,6 @@ #include #include "absl/algorithm/container.h" -#include "absl/base/attributes.h" -#include "absl/base/optimization.h" #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/container/node_hash_map.h" @@ -45,7 +44,6 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/strings/strip.h" -#include "absl/types/optional.h" #include "absl/types/span.h" #include "absl/types/variant.h" #include "base/ast.h" @@ -59,6 +57,7 @@ #include "common/expr.h" #include "common/kind.h" #include "common/type.h" +#include "common/type_spec_resolver.h" #include "common/value.h" #include "eval/compiler/check_ast_extensions.h" #include "eval/compiler/flat_expr_builder_extensions.h" @@ -528,7 +527,7 @@ class FlatExprVisitor : public cel::AstVisitor { FlatExprVisitor( const Resolver& resolver, const cel::RuntimeOptions& options, std::vector> program_optimizers, - const absl::flat_hash_map& reference_map, + const absl::flat_hash_map& type_map, const cel::TypeProvider& type_provider, IssueCollector& issue_collector, ProgramBuilder& program_builder, PlannerContext& extension_context, bool enable_optional_types) @@ -538,6 +537,7 @@ class FlatExprVisitor : public cel::AstVisitor { resolved_select_expr_(nullptr), options_(options), program_optimizers_(std::move(program_optimizers)), + type_map_(type_map), issue_collector_(issue_collector), program_builder_(program_builder), extension_context_(extension_context), @@ -606,6 +606,21 @@ class FlatExprVisitor : public cel::AstVisitor { bool PlanRecursiveProgram() const { return max_recursion_depth_ > 0; } + void SetResolvedType(const cel::Expr& expr, cel::Type type) { + resolved_types_[&expr] = std::move(type); + } + + std::optional GetResolvedType(const cel::Expr* expr) const { + if (expr == nullptr) { + return std::nullopt; + } + auto it = resolved_types_.find(expr); + if (it != resolved_types_.end()) { + return it->second; + } + return std::nullopt; + } + void PreVisitExpr(const cel::Expr& expr) override { ValidateOrError(!absl::holds_alternative(expr.kind()), "Invalid empty expression"); @@ -617,6 +632,10 @@ class FlatExprVisitor : public cel::AstVisitor { resume_from_suppressed_branch_ = &expr; } + if (options_.enable_typed_field_access) { + MaybeResolveType(expr); + } + if (block_.has_value()) { BlockInfo& block = *block_; if (block.in && block.bindings_set.contains(&expr)) { @@ -977,6 +996,24 @@ class FlatExprVisitor : public cel::AstVisitor { } StringValue field = cel::StringValue(select_expr.field()); + std::optional struct_type; + std::optional field_type; + if (options_.enable_typed_field_access) { + std::optional operand_type = + GetResolvedType(&select_expr.operand()); + if (operand_type.has_value() && operand_type->IsStruct()) { + struct_type = operand_type->GetStruct(); + if (struct_type.has_value()) { + auto field_lookup = + extension_context_.type_reflector().FindStructTypeFieldByName( + *struct_type, select_expr.field()); + // Swallow error to fallback to duck typing behavior. + if (field_lookup.ok() && field_lookup->has_value()) { + field_type = *std::move(field_lookup); + } + } + } + } if (auto depth = RecursionEligible(); depth.has_value()) { auto deps = ExtractRecursiveDependencies(); if (deps.size() != 1) { @@ -994,6 +1031,13 @@ class FlatExprVisitor : public cel::AstVisitor { return; } + if (field_type.has_value()) { + AddStep(CreateTypedSelectStep( + std::move(field), *struct_type, *std::move(field_type), + select_expr.test_only(), expr.id(), + options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); + return; + } AddStep(CreateSelectStep( std::move(field), select_expr.test_only(), expr.id(), options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); @@ -1921,6 +1965,8 @@ class FlatExprVisitor : public cel::AstVisitor { CallHandlerResult HandleHeterogeneousEqualityIn(const cel::Expr& expr, const cel::CallExpr& call); + void MaybeResolveType(const cel::Expr& expr); + const Resolver& resolver_; const cel::TypeProvider& type_provider_; absl::Status progress_status_; @@ -1942,6 +1988,8 @@ class FlatExprVisitor : public cel::AstVisitor { absl::flat_hash_set suppressed_branches_; const cel::Expr* resume_from_suppressed_branch_ = nullptr; std::vector> program_optimizers_; + const absl::flat_hash_map& type_map_; + absl::flat_hash_map resolved_types_; IssueCollector& issue_collector_; ProgramBuilder& program_builder_; @@ -2161,6 +2209,23 @@ FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr, return CallHandlerResult::kIntercepted; } +void FlatExprVisitor::MaybeResolveType(const cel::Expr& expr) { + // Try to resolve the type from the type map, but don't fail if it's not + // there. This permits cases where the runtime type is compatible but not + // the same as the type checked type. + auto it = type_map_.find(expr.id()); + if (it == type_map_.end()) { + return; + } + absl::StatusOr type = cel::ConvertTypeSpecToType( + it->second, extension_context_.type_reflector(), + extension_context_.MutableArena()); + if (!type.ok()) { + return; + } + SetResolvedType(expr, *type); +} + void LogicalCondVisitor::PreVisit(const cel::Expr* expr) { visitor_->ValidateOrError( !expr->call_expr().has_target() && expr->call_expr().args().size() >= 2, @@ -2561,8 +2626,8 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( // These objects are expected to remain scoped to one build call -- references // to them shouldn't be persisted in any part of the result expression. FlatExprVisitor visitor(resolver, options_, std::move(optimizers), - ast->reference_map(), GetTypeProvider(), - issue_collector, program_builder, extension_context, + ast->type_map(), GetTypeProvider(), issue_collector, + program_builder, extension_context, enable_optional_types_); if (options_.max_recursion_depth == -1 || options_.max_recursion_depth > 0) { diff --git a/eval/eval/BUILD b/eval/eval/BUILD index c0f544405..2597563a4 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -314,13 +314,11 @@ cc_library( ":direct_expression_step", ":evaluator_core", ":expression_step_base", - "//common:expr", + "//common:type", "//common:value", "//common:value_kind", - "//eval/internal:errors", "//internal:status_macros", "//runtime:runtime_options", - "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/status", @@ -791,7 +789,9 @@ cc_test( deps = [ ":attribute_trail", ":cel_expression_flat_impl", + ":compiler_constant_step", ":const_value_step", + ":create_map_step", ":evaluator_core", ":ident_step", ":select_step", @@ -800,11 +800,14 @@ cc_test( "//common:casting", "//common:expr", "//common:legacy_value", + "//common:type", "//common:value", "//common:value_testing", "//eval/public:activation", "//eval/public:cel_attribute", "//eval/public:cel_value", + "//eval/public:unknown_attribute_set", + "//eval/public:unknown_set", "//eval/public/containers:container_backed_map_impl", "//eval/public/structs:cel_proto_wrapper", "//eval/public/structs:legacy_type_adapter", @@ -831,6 +834,7 @@ cc_test( "@com_google_absl//absl/strings", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_cel_spec//proto/cel/expr/conformance/proto3:test_all_types_cc_proto", + "@com_google_protobuf//:protobuf", "@com_google_protobuf//:wrappers_cc_proto", ], ) diff --git a/eval/eval/attribute_trail.h b/eval/eval/attribute_trail.h index 576d0be34..838cc8875 100644 --- a/eval/eval/attribute_trail.h +++ b/eval/eval/attribute_trail.h @@ -47,6 +47,7 @@ class AttributeTrail { // Creates AttributeTrail with attribute path incremented by "qualifier". AttributeTrail Step(const std::string* qualifier) const { + if (empty()) return AttributeTrail(); return Step(cel::AttributeQualifier::OfString(*qualifier)); } diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 806e20b31..6c0ad2af7 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -11,6 +11,8 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "common/legacy_value.h" +#include "common/type.h" #include "common/value.h" #include "common/value_kind.h" #include "eval/eval/attribute_trail.h" @@ -182,7 +184,7 @@ class SelectStep : public ExpressionStepBase { absl::Status Evaluate(ExecutionFrame* frame) const override; - private: + protected: cel::StringValue field_value_; std::string field_; bool test_field_presence_; @@ -207,7 +209,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { AttributeTrail result_trail; // Handle unknown resolution. - if (frame->enable_unknowns() || frame->enable_missing_attribute_errors()) { + if (frame->attribute_tracking_enabled()) { result_trail = trail.Step(&field_); } @@ -382,6 +384,93 @@ class DirectSelectStep : public DirectExpressionStep { bool enable_optional_types_; }; +class ProtoSelectStep : public SelectStep { + public: + ProtoSelectStep(StringValue value, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, + bool enable_optional_types, + const google::protobuf::Descriptor* descriptor, + const google::protobuf::FieldDescriptor* field_descriptor) + : SelectStep(std::move(value), /*test_field_presence=*/false, expr_id, + enable_wrapper_type_null_unboxing, enable_optional_types), + descriptor_(descriptor), + field_descriptor_(field_descriptor) { + ABSL_DCHECK(descriptor_ != nullptr); + ABSL_DCHECK(field_descriptor_ != nullptr); + } + + absl::Status Evaluate(ExecutionFrame* frame) const override { + if (!frame->value_stack().HasEnough(1)) { + return absl::InternalError( + "No arguments supplied for Select-type expression"); + } + + const Value& arg = frame->value_stack().Peek(); + if (auto unwrapped = arg.AsParsedMessage(); + unwrapped.has_value() && unwrapped->GetDescriptor() == descriptor_) { + return ModernGet(frame, *unwrapped); + } else if (const google::protobuf::Message* legacy_message = + cel::interop_internal::GetLegacyMessage(arg); + legacy_message != nullptr && + legacy_message->GetDescriptor() == descriptor_) { + // A little unfortunate, but need to special case for legacy values so we + // can minimize back and forth interop conversions. + return LegacyGet(frame, legacy_message); + } + // If we get an unexpected value type, fall back to the generic + // implementation. + return SelectStep::Evaluate(frame); + } + + private: + absl::Status ModernGet(ExecutionFrame* frame, + const cel::ParsedMessageValue& parsed_message) const; + absl::Status LegacyGet(ExecutionFrame* frame, + const google::protobuf::Message* legacy_message) const; + bool CheckAttributeTrail(ExecutionFrame* frame) const; + + const google::protobuf::Descriptor* descriptor_; + const google::protobuf::FieldDescriptor* field_descriptor_; +}; + +bool ProtoSelectStep::CheckAttributeTrail(ExecutionFrame* frame) const { + if (!frame->attribute_tracking_enabled()) { + return false; + } + AttributeTrail& attr = frame->value_stack().PeekAttribute(); + attr = attr.Step(&field_); + + absl::optional marked_attribute_check = + CheckForMarkedAttributes(attr, *frame); + if (marked_attribute_check.has_value()) { + frame->value_stack().Peek() = std::move(marked_attribute_check).value(); + return true; + } + + return false; +} + +absl::Status ProtoSelectStep::ModernGet( + ExecutionFrame* frame, + const cel::ParsedMessageValue& parsed_message) const { + if (CheckAttributeTrail(frame)) { + return absl::OkStatus(); + } + return parsed_message.GetField( + field_descriptor_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), &frame->value_stack().Peek()); +} + +absl::Status ProtoSelectStep::LegacyGet( + ExecutionFrame* frame, const google::protobuf::Message* legacy_message) const { + if (CheckAttributeTrail(frame)) { + return absl::OkStatus(); + } + return cel::interop_internal::WrapLegacyMessageField( + legacy_message, field_descriptor_, unboxing_option_, frame->arena(), + &frame->value_stack().Peek()); +} + } // namespace std::unique_ptr CreateDirectSelectStep( @@ -402,4 +491,29 @@ absl::StatusOr> CreateSelectStep( enable_optional_types); } +// Factory method for Select - based Execution step +absl::StatusOr> CreateTypedSelectStep( + cel::StringValue field, cel::StructType resolved_operand_type, + cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, bool enable_optional_types) { + if (!resolved_operand_type.IsMessage() || test_only) { + // The specialization only supports messages. Fallback to the generic + // implementation for other types. + // TODO(uncreated-issue/89): support has() for messages. + return CreateSelectStep(std::move(field), test_only, expr_id, + enable_wrapper_type_null_unboxing, + enable_optional_types); + } + const google::protobuf::Descriptor* descriptor = + resolved_operand_type.GetMessage().descriptor(); + + ABSL_DCHECK(resolved_field.IsMessage()); + const google::protobuf::FieldDescriptor* field_descriptor = + resolved_field.GetMessage().descriptor(); + + return std::make_unique( + std::move(field), expr_id, enable_wrapper_type_null_unboxing, + enable_optional_types, descriptor, field_descriptor); +} + } // namespace google::api::expr::runtime diff --git a/eval/eval/select_step.h b/eval/eval/select_step.h index 2fe35ecb0..c3f965a94 100644 --- a/eval/eval/select_step.h +++ b/eval/eval/select_step.h @@ -5,6 +5,7 @@ #include #include "absl/status/statusor.h" +#include "common/type.h" #include "common/value.h" #include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_core.h" @@ -17,10 +18,15 @@ std::unique_ptr CreateDirectSelectStep( bool test_only, int64_t expr_id, bool enable_wrapper_type_null_unboxing, bool enable_optional_types = false); -// Factory method for Select - based Execution step +// Factory method for Select stack machine based Execution step absl::StatusOr> CreateSelectStep( cel::StringValue field, bool test_only, int64_t expr_id, - bool enable_wrapper_type_null_unboxing, bool enable_optional_types = false); + bool enable_wrapper_type_null_unboxing, bool enable_optional_ytpes = false); + +absl::StatusOr> CreateTypedSelectStep( + cel::StringValue field, cel::StructType resolved_operand_type, + cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, bool enable_optional_types); } // namespace google::api::expr::runtime diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index d5ca2aecd..1580472ba 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -19,11 +19,14 @@ #include "common/casting.h" #include "common/expr.h" #include "common/legacy_value.h" +#include "common/type.h" #include "common/value.h" #include "common/value_testing.h" #include "eval/eval/attribute_trail.h" #include "eval/eval/cel_expression_flat_impl.h" +#include "eval/eval/compiler_constant_step.h" #include "eval/eval/const_value_step.h" +#include "eval/eval/create_map_step.h" #include "eval/eval/evaluator_core.h" #include "eval/eval/ident_step.h" #include "eval/public/activation.h" @@ -34,6 +37,8 @@ #include "eval/public/structs/legacy_type_adapter.h" #include "eval/public/structs/trivial_legacy_type_info.h" #include "eval/public/testing/matchers.h" +#include "eval/public/unknown_attribute_set.h" +#include "eval/public/unknown_set.h" #include "eval/testutil/test_extensions.pb.h" #include "eval/testutil/test_message.pb.h" #include "extensions/protobuf/value.h" @@ -48,6 +53,7 @@ #include "runtime/internal/runtime_type_provider.h" #include "runtime/runtime_options.h" #include "cel/expr/conformance/proto3/test_all_types.pb.h" +#include "google/protobuf/descriptor.h" namespace google::api::expr::runtime { @@ -1062,6 +1068,129 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { } } +TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + + cel::StructType resolved_operand_type( + (cel::MessageType(TestAllTypes::descriptor()))); + const google::protobuf::FieldDescriptor* field_desc = + TestAllTypes::descriptor()->FindFieldByName("single_int64"); + ASSERT_NE(field_desc, nullptr); + cel::StructTypeField resolved_field((cel::MessageTypeField(field_desc))); + + ASSERT_OK_AND_ASSIGN( + auto step1, + CreateTypedSelectStep(cel::StringValue("single_int64"), + resolved_operand_type, resolved_field, + /*test_only=*/false, -1, + /*enable_wrapper_type_null_unboxing=*/false, + /*enable_optional_types=*/false)); + + ExecutionPath path; + path.push_back(std::move(step0)); + path.push_back(std::move(step1)); + cel::RuntimeOptions options; + if (GetParam()) { + options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; + } + CelExpressionFlatImpl cel_expr( + env_, + FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), options)); + + TestAllTypes message; + message.set_single_int64(42); + Activation activation; + activation.InsertValue("message", + CelProtoWrapper::CreateMessage(&message, &arena_)); + + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); + ASSERT_TRUE(result.IsInt64()); + EXPECT_EQ(result.Int64OrDie(), 42); +} + +TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + + cel::StructType resolved_operand_type( + (cel::MessageType(TestAllTypes::descriptor()))); + const google::protobuf::FieldDescriptor* field_desc = + TestAllTypes::descriptor()->FindFieldByName("single_int64"); + ASSERT_NE(field_desc, nullptr); + cel::StructTypeField resolved_field((cel::MessageTypeField(field_desc))); + + ASSERT_OK_AND_ASSIGN( + auto step1, + CreateTypedSelectStep(cel::StringValue("single_int64"), + resolved_operand_type, resolved_field, + /*test_only=*/false, -1, + /*enable_wrapper_type_null_unboxing=*/false, + /*enable_optional_types=*/false)); + + ExecutionPath path; + path.push_back(std::move(step0)); + path.push_back(std::move(step1)); + cel::RuntimeOptions options; + if (GetParam()) { + options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; + } + CelExpressionFlatImpl cel_expr( + env_, + FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), options)); + + UnknownSet unknown_set(UnknownAttributeSet({CelAttribute("message", {})})); + Activation activation; + activation.InsertValue("message", CelValue::CreateUnknownSet(&unknown_set)); + + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); + ASSERT_TRUE(result.IsUnknownSet()); +} + +TEST_F(SelectStepTest, TypedSelectStepUnknownPatternResolvesToUnknown) { + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + + cel::StructType resolved_operand_type( + (cel::MessageType(TestAllTypes::descriptor()))); + const google::protobuf::FieldDescriptor* field_desc = + TestAllTypes::descriptor()->FindFieldByName("single_int64"); + ASSERT_NE(field_desc, nullptr); + cel::StructTypeField resolved_field((cel::MessageTypeField(field_desc))); + + ASSERT_OK_AND_ASSIGN( + auto step1, + CreateTypedSelectStep(cel::StringValue("single_int64"), + resolved_operand_type, resolved_field, + /*test_only=*/false, -1, + /*enable_wrapper_type_null_unboxing=*/false, + /*enable_optional_types=*/false)); + + ExecutionPath path; + path.push_back(std::move(step0)); + path.push_back(std::move(step1)); + cel::RuntimeOptions options; + options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; + CelExpressionFlatImpl cel_expr( + env_, + FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), options)); + + TestAllTypes message; + message.set_single_int64(42); + + std::vector unknown_patterns; + unknown_patterns.push_back(CelAttributePattern( + "message", {CelAttributeQualifierPattern::OfString("single_int64")})); + + Activation activation; + activation.InsertValue("message", + CelProtoWrapper::CreateMessage(&message, &arena_)); + activation.set_unknown_attribute_patterns(unknown_patterns); + + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); + ASSERT_TRUE(result.IsUnknownSet()); +} + INSTANTIATE_TEST_SUITE_P(UnknownsEnabled, SelectStepConformanceTest, testing::Bool()); diff --git a/eval/public/cel_options.cc b/eval/public/cel_options.cc index 938b5e96f..93b67ad35 100644 --- a/eval/public/cel_options.cc +++ b/eval/public/cel_options.cc @@ -19,29 +19,33 @@ namespace google::api::expr::runtime { cel::RuntimeOptions ConvertToRuntimeOptions(const InterpreterOptions& options) { - return cel::RuntimeOptions{/*.container=*/"", - options.unknown_processing, - options.enable_missing_attribute_errors, - options.enable_timestamp_duration_overflow_errors, - options.short_circuiting, - options.enable_comprehension, - options.comprehension_max_iterations, - options.enable_comprehension_list_append, - options.enable_comprehension_mutable_map, - options.enable_regex, - options.regex_max_program_size, - options.enable_string_conversion, - options.enable_string_concat, - options.enable_list_concat, - options.enable_list_contains, - options.fail_on_warnings, - options.enable_qualified_type_identifiers, - options.enable_heterogeneous_equality, - options.enable_empty_wrapper_null_unboxing, - options.enable_lazy_bind_initialization, - options.max_recursion_depth, - options.enable_recursive_tracing, - options.enable_fast_builtins}; + return cel::RuntimeOptions{ + /*.container=*/"", + options.unknown_processing, + options.enable_missing_attribute_errors, + options.enable_timestamp_duration_overflow_errors, + options.short_circuiting, + options.enable_comprehension, + options.comprehension_max_iterations, + options.enable_comprehension_list_append, + options.enable_comprehension_mutable_map, + options.enable_regex, + options.regex_max_program_size, + options.enable_string_conversion, + options.enable_string_concat, + options.enable_list_concat, + options.enable_list_contains, + options.fail_on_warnings, + options.enable_qualified_type_identifiers, + options.enable_heterogeneous_equality, + options.enable_empty_wrapper_null_unboxing, + options.enable_lazy_bind_initialization, + options.max_recursion_depth, + options.enable_recursive_tracing, + options.enable_fast_builtins, + options.enable_precision_preserving_double_format, + options.enable_typed_field_access, + }; } } // namespace google::api::expr::runtime diff --git a/eval/public/cel_options.h b/eval/public/cel_options.h index 4d81eb8a7..001990431 100644 --- a/eval/public/cel_options.h +++ b/eval/public/cel_options.h @@ -218,6 +218,11 @@ struct InterpreterOptions { // Otherwise, will fall back to formatting with the worst-case required // precision. bool enable_precision_preserving_double_format = true; + + // When enabled, the planner will attempt to use a more performant execution + // path for field access when the type is known at plan time, instead of using + // the generic field access implementation. + bool enable_typed_field_access = false; }; // LINT.ThenChange(//depot/google3/runtime/runtime_options.h) diff --git a/eval/public/structs/BUILD b/eval/public/structs/BUILD index d722559e3..2c7d7ed16 100644 --- a/eval/public/structs/BUILD +++ b/eval/public/structs/BUILD @@ -325,6 +325,7 @@ cc_test( ":proto_message_type_adapter", "//base:attributes", "//common:value", + "//common:value_testing", "//eval/public:cel_value", "//eval/public:message_wrapper", "//eval/public/containers:container_backed_list_impl", @@ -365,10 +366,13 @@ cc_test( deps = [ ":legacy_type_info_apis", ":protobuf_descriptor_type_provider", + "//common:type", "//eval/public:cel_value", "//eval/public/testing:matchers", "//extensions/protobuf:memory_manager", "//internal:testing", + "@com_google_absl//absl/status:status_matchers", + "@com_google_cel_spec//proto/cel/expr/conformance/proto3:test_all_types_cc_proto", "@com_google_protobuf//:protobuf", "@com_google_protobuf//:wrappers_cc_proto", ], @@ -414,7 +418,9 @@ cc_test( deps = [ ":legacy_type_info_apis", ":legacy_type_provider", + "//common:type", "//internal:testing", + "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/strings:string_view", ], ) diff --git a/eval/public/structs/legacy_type_info_apis.h b/eval/public/structs/legacy_type_info_apis.h index 4f07470a1..e470ac566 100644 --- a/eval/public/structs/legacy_type_info_apis.h +++ b/eval/public/structs/legacy_type_info_apis.h @@ -61,6 +61,11 @@ class LegacyTypeInfoApis { virtual absl::string_view GetTypename( const MessageWrapper& wrapped_message) const = 0; + // Return a pointer to the descriptor for the wrapped message's type. + // + // Should only be defined for messages with standard behavior (i.e. normal + // duck-typed behavior of resolving fields by associated descriptor is + // correct). virtual const google::protobuf::Descriptor* absl_nullable GetDescriptor( const MessageWrapper& wrapped_message [[maybe_unused]]) const { return nullptr; diff --git a/eval/public/structs/legacy_type_provider.cc b/eval/public/structs/legacy_type_provider.cc index f8db92298..cd7079451 100644 --- a/eval/public/structs/legacy_type_provider.cc +++ b/eval/public/structs/legacy_type_provider.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include "absl/base/nullability.h" @@ -35,6 +36,7 @@ #include "extensions/protobuf/memory_manager.h" #include "internal/status_macros.h" #include "google/protobuf/arena.h" +#include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" namespace google::api::expr::runtime { @@ -197,22 +199,34 @@ LegacyTypeProvider::FindStructTypeFieldByNameImpl( result.has_value()) { return result; } - if (auto type_info = ProvideLegacyTypeInfo(type); type_info.has_value()) { - if (auto field_desc = (*type_info)->FindFieldByName(name); - field_desc.has_value()) { - return cel::common_internal::BasicStructTypeField( - field_desc->name, field_desc->number, cel::DynType{}); - } else { - const auto* mutation_apis = - (*type_info)->GetMutationApis(MessageWrapper()); - if (mutation_apis == nullptr || !mutation_apis->DefinesField(name)) { - return std::nullopt; - } - return cel::common_internal::BasicStructTypeField(name, 0, - cel::DynType{}); + absl::optional type_info = + ProvideLegacyTypeInfo(type); + if (!type_info.has_value()) { + return std::nullopt; + } + if (const auto* descriptor = (*type_info)->GetDescriptor(MessageWrapper()); + descriptor != nullptr) { + // If it's a normal proto, just use the descriptor to find the field. + // Allows us to get the same optimizations as the modern value in most + // cases. + const google::protobuf::FieldDescriptor* field = descriptor->FindFieldByName(name); + if (field != nullptr) { + return cel::StructTypeField(cel::MessageTypeField(field)); } } - return std::nullopt; + + if (auto field_desc = (*type_info)->FindFieldByName(name); + field_desc.has_value()) { + return cel::common_internal::BasicStructTypeField( + field_desc->name, field_desc->number, cel::DynType{}); + } + + const auto* mutation_apis = (*type_info)->GetMutationApis(MessageWrapper()); + if (mutation_apis == nullptr || !mutation_apis->DefinesField(name)) { + return std::nullopt; + } + + return cel::common_internal::BasicStructTypeField(name, 0, cel::DynType{}); } } // namespace google::api::expr::runtime diff --git a/eval/public/structs/legacy_type_provider_test.cc b/eval/public/structs/legacy_type_provider_test.cc index 8de2aba01..2da45e69d 100644 --- a/eval/public/structs/legacy_type_provider_test.cc +++ b/eval/public/structs/legacy_type_provider_test.cc @@ -17,13 +17,17 @@ #include #include +#include "absl/status/status_matchers.h" #include "absl/strings/string_view.h" +#include "common/type.h" #include "eval/public/structs/legacy_type_info_apis.h" #include "internal/testing.h" namespace google::api::expr::runtime { namespace { +using ::absl_testing::IsOk; + class LegacyTypeProviderTestEmpty : public LegacyTypeProvider { public: absl::optional ProvideLegacyType( @@ -46,6 +50,13 @@ class LegacyTypeInfoApisEmpty : public LegacyTypeInfoApis { const MessageWrapper& wrapped_message) const override { return nullptr; } + absl::optional FindFieldByName( + absl::string_view name) const override { + if (name == "field1") { + return FieldDescription{1, "field1"}; + } + return absl::nullopt; + } private: const std::string test_string_ = "test"; @@ -89,5 +100,22 @@ TEST(LegacyTypeProviderTest, NonEmptyTypeProviderProvidesSomeTypes) { EXPECT_EQ(provider.ProvideLegacyTypeInfo("other"), std::nullopt); } +TEST(LegacyTypeProviderTest, FindStructTypeFieldByName) { + LegacyTypeInfoApisEmpty test_type_info; + LegacyTypeProviderTestImpl provider(&test_type_info); + + ASSERT_OK_AND_ASSIGN(absl::optional field, + provider.FindStructTypeFieldByName("test", "field1")); + ASSERT_TRUE(field.has_value()); + EXPECT_EQ(field->name(), "field1"); + EXPECT_EQ(field->number(), 1); + EXPECT_EQ(field->GetType(), cel::DynType()); + + ASSERT_OK_AND_ASSIGN( + absl::optional not_found_field, + provider.FindStructTypeFieldByName("test", "unknown_field")); + EXPECT_FALSE(not_found_field.has_value()); +} + } // namespace } // namespace google::api::expr::runtime diff --git a/eval/public/structs/proto_message_type_adapter.cc b/eval/public/structs/proto_message_type_adapter.cc index 8c140c0c7..a8bb852fc 100644 --- a/eval/public/structs/proto_message_type_adapter.cc +++ b/eval/public/structs/proto_message_type_adapter.cc @@ -142,28 +142,6 @@ absl::StatusOr HasFieldImpl(const google::protobuf::Message* message, return CelFieldIsPresent(message, field_desc, reflection); } -absl::StatusOr CreateCelValueFromField( - const google::protobuf::Message* message, const google::protobuf::FieldDescriptor* field_desc, - ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena) { - if (field_desc->is_map()) { - auto* map = google::protobuf::Arena::Create( - arena, message, field_desc, &MessageCelValueFactory, arena); - - return CelValue::CreateMap(map); - } - if (field_desc->is_repeated()) { - auto* list = google::protobuf::Arena::Create( - arena, message, field_desc, &MessageCelValueFactory, arena); - return CelValue::CreateList(list); - } - - CEL_ASSIGN_OR_RETURN( - CelValue result, - internal::CreateValueFromSingleField(message, field_desc, unboxing_option, - &MessageCelValueFactory, arena)); - return result; -} - // Shared implementation for GetField. // Handles list or map specific behavior before calling reflection helpers. absl::StatusOr GetFieldImpl(const google::protobuf::Message* message, @@ -441,6 +419,28 @@ CelValue MessageCelValueFactory(const google::protobuf::Message* message) { } // namespace +absl::StatusOr CreateCelValueFromField( + const google::protobuf::Message* message, const google::protobuf::FieldDescriptor* field_desc, + ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena) { + if (field_desc->is_map()) { + auto* map = google::protobuf::Arena::Create( + arena, message, field_desc, &MessageCelValueFactory, arena); + + return CelValue::CreateMap(map); + } + if (field_desc->is_repeated()) { + auto* list = google::protobuf::Arena::Create( + arena, message, field_desc, &MessageCelValueFactory, arena); + return CelValue::CreateList(list); + } + + CEL_ASSIGN_OR_RETURN( + CelValue result, + internal::CreateValueFromSingleField(message, field_desc, unboxing_option, + &MessageCelValueFactory, arena)); + return result; +} + std::string ProtoMessageTypeAdapter::DebugString( const MessageWrapper& wrapped_message) const { if (!wrapped_message.HasFullProto() || diff --git a/eval/public/structs/proto_message_type_adapter.h b/eval/public/structs/proto_message_type_adapter.h index f2fc43a8a..eb89a467c 100644 --- a/eval/public/structs/proto_message_type_adapter.h +++ b/eval/public/structs/proto_message_type_adapter.h @@ -26,6 +26,7 @@ #include "eval/public/cel_value.h" #include "eval/public/structs/legacy_type_adapter.h" #include "eval/public/structs/legacy_type_info_apis.h" +#include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" namespace google::api::expr::runtime { @@ -119,6 +120,13 @@ class ProtoMessageTypeAdapter : public LegacyTypeInfoApis, const google::protobuf::Descriptor* descriptor_; }; +// Creates a CelValue from the given field on the proto message. This is the +// shared implementation for ProtoMessageTypeAdapter and +// DucktypedMessageAdapter. +absl::StatusOr CreateCelValueFromField( + const google::protobuf::Message* message, const google::protobuf::FieldDescriptor* field_desc, + ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena); + // Returns a TypeInfo provider representing an arbitrary message. // This allows for the legacy duck-typed behavior of messages on field access // instead of expecting a particular message type given a TypeInfo. diff --git a/eval/public/structs/proto_message_type_adapter_test.cc b/eval/public/structs/proto_message_type_adapter_test.cc index e28d76102..c0e60c632 100644 --- a/eval/public/structs/proto_message_type_adapter_test.cc +++ b/eval/public/structs/proto_message_type_adapter_test.cc @@ -20,7 +20,9 @@ #include "google/protobuf/descriptor.pb.h" #include "absl/status/status.h" #include "base/attribute.h" +#include "common/legacy_value.h" #include "common/value.h" +#include "common/value_testing.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" @@ -41,6 +43,7 @@ namespace google::api::expr::runtime { namespace { +using ::absl_testing::IsOk; using ::absl_testing::IsOkAndHolds; using ::absl_testing::StatusIs; using ::cel::ProtoWrapperTypeOptions; @@ -1407,5 +1410,57 @@ TEST(ProtoMesssageTypeAdapter, QualifyMapIndexLeafWrongType) { HasSubstr("Invalid map key type")))))); } +TEST(ProtoMesssageTypeAdapter, InteropUnwrappingNotGeneric) { + google::protobuf::Arena arena; + ProtoMessageTypeAdapter adapter( + google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( + "google.api.expr.runtime.TestMessage"), + google::protobuf::MessageFactory::generated_factory()); + + TestMessage message; + message.set_string_value("hello"); + auto legacy_value = CelValue::CreateMessageWrapper( + CelValue::MessageWrapper(&message, &adapter)); + cel::Value modern_value; + ASSERT_THAT(cel::ModernValue(&arena, legacy_value, modern_value), IsOk()); + auto unwrapped = cel::interop_internal::GetLegacyMessage(modern_value); + + // Can't unwrap a non-generic MessageWrapper -- we test by identity to + // be sure we're not dropping a custom adapter. + ASSERT_EQ(unwrapped, nullptr); +} + +TEST(ProtoMesssageTypeAdapter, InteropUnwrappingGeneric) { + google::protobuf::Arena arena; + + TestMessage message; + message.set_string_value("hello"); + auto legacy_value = CelValue::CreateMessageWrapper( + CelValue::MessageWrapper(&message, &GetGenericProtoTypeInfoInstance())); + cel::Value modern_value; + ASSERT_THAT(cel::ModernValue(&arena, legacy_value, modern_value), IsOk()); + auto unwrapped = cel::interop_internal::GetLegacyMessage(modern_value); + + ASSERT_EQ(unwrapped, &message); +} + +TEST(ProtoMesssageTypeAdapter, InteropFieldAccess) { + google::protobuf::Arena arena; + + TestMessage message; + message.set_string_value("hello"); + + const google::protobuf::FieldDescriptor* field = + message.GetDescriptor()->FindFieldByName("string_value"); + ASSERT_NE(field, nullptr); + cel::Value field_value; + ASSERT_THAT(cel::interop_internal::WrapLegacyMessageField( + &message, field, ProtoWrapperTypeOptions::kUnsetNull, &arena, + &field_value), + IsOk()); + + EXPECT_THAT(field_value, cel::test::StringValueIs("hello")); +} + } // namespace } // namespace google::api::expr::runtime diff --git a/eval/public/structs/protobuf_descriptor_type_provider_test.cc b/eval/public/structs/protobuf_descriptor_type_provider_test.cc index 3a8fae26b..aabdc38c4 100644 --- a/eval/public/structs/protobuf_descriptor_type_provider_test.cc +++ b/eval/public/structs/protobuf_descriptor_type_provider_test.cc @@ -17,16 +17,23 @@ #include #include "google/protobuf/wrappers.pb.h" +#include "absl/status/status_matchers.h" +#include "common/type.h" #include "eval/public/cel_value.h" #include "eval/public/structs/legacy_type_info_apis.h" #include "eval/public/testing/matchers.h" #include "extensions/protobuf/memory_manager.h" #include "internal/testing.h" +#include "cel/expr/conformance/proto3/test_all_types.pb.h" #include "google/protobuf/arena.h" +#include "google/protobuf/descriptor.h" +#include "google/protobuf/message.h" namespace google::api::expr::runtime { namespace { +using ::absl_testing::IsOk; +using ::cel::expr::conformance::proto3::TestAllTypes; using ::cel::extensions::ProtoMemoryManager; TEST(ProtobufDescriptorProvider, Basic) { @@ -36,7 +43,7 @@ TEST(ProtobufDescriptorProvider, Basic) { google::protobuf::Arena arena; auto manager = ProtoMemoryManager(&arena); auto type_adapter = provider.ProvideLegacyType("google.protobuf.Int64Value"); - absl::optional type_info = + std::optional type_info = provider.ProvideLegacyTypeInfo("google.protobuf.Int64Value"); ASSERT_TRUE(type_adapter.has_value()); @@ -53,8 +60,9 @@ TEST(ProtobufDescriptorProvider, Basic) { ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder value, type_adapter->mutation_apis()->NewInstance(manager)); - ASSERT_OK(type_adapter->mutation_apis()->SetField( - "value", CelValue::CreateInt64(10), manager, value)); + ASSERT_THAT(type_adapter->mutation_apis()->SetField( + "value", CelValue::CreateInt64(10), manager, value), + IsOk()); ASSERT_OK_AND_ASSIGN( CelValue adapted, @@ -91,5 +99,60 @@ TEST(ProtobufDescriptorProvider, NotFound) { ASSERT_FALSE(type_info.has_value()); } +TEST(ProtobufDescriptorProvider, FindType) { + ProtobufDescriptorProvider provider( + google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory()); + ASSERT_OK_AND_ASSIGN(std::optional wrapper_type, + provider.FindType("google.protobuf.Int64Value")); + ASSERT_TRUE(wrapper_type.has_value()); + EXPECT_TRUE(wrapper_type->Is()); + EXPECT_EQ(wrapper_type->name(), "google.protobuf.Int64Value"); + + ASSERT_OK_AND_ASSIGN( + std::optional msg_type, + provider.FindType("cel.expr.conformance.proto3.TestAllTypes")); + ASSERT_TRUE(msg_type.has_value()); + EXPECT_TRUE(msg_type->Is()); + EXPECT_EQ(msg_type->name(), "cel.expr.conformance.proto3.TestAllTypes"); +} + +TEST(ProtobufDescriptorProvider, FindStructTypeFieldByName) { + ProtobufDescriptorProvider provider( + google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory()); + ASSERT_OK_AND_ASSIGN(std::optional field, + provider.FindStructTypeFieldByName( + "google.protobuf.Int64Value", "value")); + ASSERT_TRUE(field.has_value()); + EXPECT_EQ(field->name(), "value"); + EXPECT_EQ(field->number(), 1); + EXPECT_EQ(field->GetType(), cel::IntType()); +} + +TEST(ProtobufDescriptorProvider, FindTypeNotFound) { + ProtobufDescriptorProvider provider( + google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory()); + ASSERT_OK_AND_ASSIGN(std::optional type, + provider.FindType("UnknownType")); + EXPECT_FALSE(type.has_value()); +} + +TEST(ProtobufDescriptorProvider, FindStructTypeFieldByNameNotFound) { + ProtobufDescriptorProvider provider( + google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory()); + ASSERT_OK_AND_ASSIGN(std::optional field, + provider.FindStructTypeFieldByName( + "google.protobuf.Int64Value", "unknown_field")); + EXPECT_FALSE(field.has_value()); + + ASSERT_OK_AND_ASSIGN( + std::optional field2, + provider.FindStructTypeFieldByName("UnknownType", "value")); + EXPECT_FALSE(field2.has_value()); +} + } // namespace } // namespace google::api::expr::runtime diff --git a/eval/tests/BUILD b/eval/tests/BUILD index 9163548d1..c5a7a7062 100644 --- a/eval/tests/BUILD +++ b/eval/tests/BUILD @@ -24,6 +24,12 @@ cc_test( ], deps = [ ":request_context_cc_proto", + "//common:ast_proto", + "//common:decl", + "//common:type", + "//compiler", + "//compiler:compiler_factory", + "//compiler:standard_library", "//eval/public:activation", "//eval/public:builtin_func_registrar", "//eval/public:cel_expr_builder_factory", @@ -42,6 +48,7 @@ cc_test( "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/container:node_hash_set", "@com_google_absl//absl/flags:flag", + "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/strings", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_googleapis//google/rpc/context:attribute_context_cc_proto", @@ -61,22 +68,25 @@ cc_test( ], deps = [ ":request_context_cc_proto", + "//checker:validation_result", "//common:allocator", "//common:casting", + "//common:decl", "//common:legacy_value", - "//common:memory", "//common:native_type", + "//common:type", "//common:value", + "//compiler", + "//compiler:compiler_factory", + "//compiler:standard_library", "//extensions:comprehensions_v2_functions", "//extensions:comprehensions_v2_macros", "//extensions/protobuf:runtime_adapter", - "//extensions/protobuf:value", "//internal:benchmark", "//internal:testing", "//internal:testing_descriptor_pool", "//internal:testing_message_factory", "//parser", - "//parser:macro", "//parser:macro_registry", "//runtime", "//runtime:activation", @@ -94,7 +104,6 @@ cc_test( "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", - "@com_google_absl//absl/types:optional", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_googleapis//google/rpc/context:attribute_context_cc_proto", "@com_google_protobuf//:protobuf", diff --git a/eval/tests/benchmark_test.cc b/eval/tests/benchmark_test.cc index f188dc0b7..abeac608a 100644 --- a/eval/tests/benchmark_test.cc +++ b/eval/tests/benchmark_test.cc @@ -12,7 +12,14 @@ #include "absl/container/flat_hash_set.h" #include "absl/container/node_hash_set.h" #include "absl/flags/flag.h" +#include "absl/status/status_matchers.h" #include "absl/strings/match.h" +#include "common/ast_proto.h" +#include "common/decl.h" +#include "common/type.h" +#include "compiler/compiler.h" +#include "compiler/compiler_factory.h" +#include "compiler/standard_library.h" #include "eval/public/activation.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_expr_builder_factory.h" @@ -31,6 +38,7 @@ ABSL_FLAG(bool, enable_optimizations, false, "enable const folding opt"); ABSL_FLAG(bool, enable_recursive_planning, false, "enable recursive planning"); +ABSL_FLAG(bool, enable_typed_field_access, false, "enable typed field access"); namespace google { namespace api { @@ -39,6 +47,7 @@ namespace runtime { namespace { +using ::absl_testing::IsOk; using ::cel::expr::Expr; using ::cel::expr::ParsedExpr; using ::cel::expr::SourceInfo; @@ -56,6 +65,10 @@ InterpreterOptions GetOptions(google::protobuf::Arena& arena) { options.max_recursion_depth = -1; } + if (absl::GetFlag(FLAGS_enable_typed_field_access)) { + options.enable_typed_field_access = true; + } + return options; } @@ -67,7 +80,8 @@ static void BM_Eval(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -113,7 +127,8 @@ static void BM_Eval_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -155,7 +170,8 @@ static void BM_EvalString(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -198,7 +214,8 @@ static void BM_EvalString_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -295,7 +312,8 @@ void BM_PolicySymbolic(benchmark::State& state) { options.constant_arena = &arena; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); SourceInfo source_info; ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression( @@ -351,7 +369,8 @@ void BM_PolicySymbolicMap(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); SourceInfo source_info; ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression( @@ -373,22 +392,37 @@ BENCHMARK(BM_PolicySymbolicMap); // Uses a protobuf container for "ip", "path", and "token". void BM_PolicySymbolicProto(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto compiler_builder, + cel::NewCompilerBuilder(google::protobuf::DescriptorPool::generated_pool())); + ASSERT_THAT(compiler_builder->AddLibrary(cel::StandardCompilerLibrary()), + IsOk()); + ASSERT_THAT( + compiler_builder->GetCheckerBuilder().AddVariable(cel::MakeVariableDecl( + "request", cel::MessageType(RequestContext::descriptor()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, compiler_builder->Build()); + + ASSERT_OK_AND_ASSIGN(auto validation_result, compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); + + cel::expr::CheckedExpr checked_expr; + ASSERT_THAT(cel::AstToCheckedExpr(*ast, &checked_expr), IsOk()); InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression( - &parsed_expr.expr(), &source_info)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&checked_expr)); Activation activation; RequestContext request; @@ -475,7 +509,8 @@ void BM_Comprehension(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); options.comprehension_max_iterations = 10000000; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -509,7 +544,8 @@ void BM_Comprehension_Trace(benchmark::State& state) { options.comprehension_max_iterations = 10000000; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -531,7 +567,8 @@ void BM_HasMap(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&parsed_expr.expr(), nullptr)); @@ -700,7 +737,8 @@ void BM_ProtoStructAccess(benchmark::State& state) { )cel")); InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&parsed_expr.expr(), nullptr)); @@ -730,7 +768,8 @@ void BM_ProtoListAccess(benchmark::State& state) { )cel")); InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&parsed_expr.expr(), nullptr)); @@ -867,7 +906,8 @@ void BM_NestedComprehension(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); options.comprehension_max_iterations = 10000000; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -903,7 +943,8 @@ void BM_NestedComprehension_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -936,7 +977,8 @@ void BM_ListComprehension(benchmark::State& state) { options.comprehension_max_iterations = 10000000; options.enable_comprehension_list_append = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN( auto cel_expr, builder->CreateExpression(&(parsed_expr.expr()), nullptr)); @@ -971,7 +1013,8 @@ void BM_ListComprehension_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN( auto cel_expr, builder->CreateExpression(&(parsed_expr.expr()), nullptr)); @@ -1006,7 +1049,7 @@ void BM_ListComprehension_Opt(benchmark::State& state) { options.comprehension_max_iterations = 10000000; options.enable_comprehension_list_append = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry()), IsOk()); ASSERT_OK_AND_ASSIGN( auto cel_expr, builder->CreateExpression(&(parsed_expr.expr()), nullptr)); diff --git a/eval/tests/modern_benchmark_test.cc b/eval/tests/modern_benchmark_test.cc index 005f93aa5..28daed981 100644 --- a/eval/tests/modern_benchmark_test.cc +++ b/eval/tests/modern_benchmark_test.cc @@ -14,7 +14,6 @@ // // General benchmarks for CEL evaluator. -#include #include #include #include @@ -36,19 +35,24 @@ #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" #include "absl/strings/match.h" +#include "absl/strings/string_view.h" +#include "checker/validation_result.h" #include "common/allocator.h" #include "common/casting.h" +#include "common/decl.h" #include "common/native_type.h" +#include "common/type.h" #include "common/value.h" +#include "compiler/compiler.h" +#include "compiler/compiler_factory.h" +#include "compiler/standard_library.h" #include "eval/tests/request_context.pb.h" #include "extensions/comprehensions_v2_functions.h" #include "extensions/comprehensions_v2_macros.h" #include "extensions/protobuf/runtime_adapter.h" -#include "extensions/protobuf/value.h" #include "internal/benchmark.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" -#include "internal/testing_message_factory.h" #include "parser/macro_registry.h" #include "parser/parser.h" #include "runtime/activation.h" @@ -62,6 +66,7 @@ #include "google/protobuf/text_format.h" ABSL_FLAG(bool, enable_recursive_planning, false, "enable recursive planning"); +ABSL_FLAG(bool, enable_typed_field_access, false, "enable typed field access"); namespace cel { @@ -85,16 +90,21 @@ RuntimeOptions GetOptions() { options.max_recursion_depth = -1; } + if (absl::GetFlag(FLAGS_enable_typed_field_access)) { + options.enable_typed_field_access = true; + } + return options; } enum class ConstFoldingEnabled { kNo, kYes }; std::unique_ptr StandardRuntimeOrDie( - const cel::RuntimeOptions& options, google::protobuf::Arena* arena = nullptr, + const cel::RuntimeOptions& options, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::Arena* arena = nullptr, ConstFoldingEnabled const_folding = ConstFoldingEnabled::kNo) { - auto builder = CreateStandardRuntimeBuilder( - internal::GetTestingDescriptorPool(), options); + auto builder = CreateStandardRuntimeBuilder(descriptor_pool, options); ABSL_CHECK_OK(builder.status()); switch (const_folding) { @@ -111,13 +121,17 @@ std::unique_ptr StandardRuntimeOrDie( return std::move(runtime).value(); } +std::unique_ptr StandardRuntimeOrDie( + const cel::RuntimeOptions& options, google::protobuf::Arena* arena = nullptr, + ConstFoldingEnabled const_folding = ConstFoldingEnabled::kNo) { + return StandardRuntimeOrDie(options, internal::GetTestingDescriptorPool(), + arena, const_folding); +} + template Value WrapMessageOrDie(const T& message, google::protobuf::Arena* absl_nonnull arena) { - auto value = extensions::ProtoMessageToValue( - message, internal::GetTestingDescriptorPool(), - internal::GetTestingMessageFactory(), arena); - ABSL_CHECK_OK(value.status()); - return std::move(value).value(); + return Value::FromMessage(message, google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory(), arena); } // Benchmark test @@ -335,7 +349,23 @@ BENCHMARK(BM_PolicyNative); void BM_PolicySymbolic(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("ip", StringType())), + IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("path", StringType())), + IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("token", StringType())), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((path.startsWith("v1") && token in ["v1", "v2", "admin"]) || (path.startsWith("v2") && token in ["v2", "admin"]) || @@ -343,13 +373,14 @@ void BM_PolicySymbolic(benchmark::State& state) { "10.0.1.1", "10.0.1.2", "10.0.1.3" ]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); auto runtime = StandardRuntimeOrDie(options, &arena, ConstFoldingEnabled::kYes); - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; activation.InsertOrAssignValue("ip", StringValue(&arena, kIP)); @@ -437,21 +468,31 @@ class RequestMapImpl : public CustomMapValueInterface { // Uses a lazily constructed map container for "ip", "path", and "token". void BM_PolicySymbolicMap(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable(MakeVariableDecl( + "request", + cel::MapType(&arena, cel::StringType(), cel::StringType()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); - auto runtime = StandardRuntimeOrDie(options); - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; CustomMapValue map_value(google::protobuf::Arena::Create(&arena), @@ -472,21 +513,31 @@ BENCHMARK(BM_PolicySymbolicMap); // Uses a protobuf container for "ip", "path", and "token". void BM_PolicySymbolicProto(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(google::protobuf::DescriptorPool::generated_pool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable(MakeVariableDecl( + "request", cel::MessageType(RequestContext::descriptor()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); + auto runtime = + StandardRuntimeOrDie(options, google::protobuf::DescriptorPool::generated_pool()); - auto runtime = StandardRuntimeOrDie(options); - - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; RequestContext request; @@ -497,8 +548,7 @@ void BM_PolicySymbolicProto(benchmark::State& state) { for (auto _ : state) { ASSERT_OK_AND_ASSIGN(cel::Value result, cel_expr->Evaluate(&arena, activation)); - ASSERT_TRUE(InstanceOf(result) && - Cast(result).NativeValue()); + ASSERT_TRUE(result.IsBool() && result.GetBool().NativeValue()); } } diff --git a/runtime/internal/BUILD b/runtime/internal/BUILD index 1223ff6d1..6c96de1cf 100644 --- a/runtime/internal/BUILD +++ b/runtime/internal/BUILD @@ -174,6 +174,7 @@ cc_library( "//internal:testing_message_factory", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", + "@com_google_absl//absl/log:die_if_null", "@com_google_protobuf//:protobuf", ], ) @@ -193,6 +194,7 @@ cc_library( srcs = ["runtime_type_provider.cc"], hdrs = ["runtime_type_provider.h"], deps = [ + "//common:descriptor_pool_type_introspector", "//common:type", "//common:value", "//internal:status_macros", diff --git a/runtime/internal/runtime_env_testing.h b/runtime/internal/runtime_env_testing.h index 71b2096cd..8c391784d 100644 --- a/runtime/internal/runtime_env_testing.h +++ b/runtime/internal/runtime_env_testing.h @@ -18,12 +18,43 @@ #include #include "absl/base/nullability.h" +#include "absl/log/absl_check.h" +#include "absl/log/die_if_null.h" +#include "internal/testing_descriptor_pool.h" +#include "internal/testing_message_factory.h" #include "runtime/internal/runtime_env.h" +#include "google/protobuf/arena.h" +#include "google/protobuf/descriptor.h" +#include "google/protobuf/message.h" namespace cel::runtime_internal { absl_nonnull std::shared_ptr NewTestingRuntimeEnv(); +template +const google::protobuf::Descriptor* absl_nonnull GetTestingEnvDescriptor() { + const google::protobuf::Descriptor* descriptor = + internal::GetTestingDescriptorPool()->FindMessageTypeByName( + T::descriptor()->full_name()); + ABSL_CHECK(descriptor != nullptr) + << "Could not find CEL test env descriptor for type " + << T::descriptor()->full_name(); + return descriptor; +} + +template +google::protobuf::Message* absl_nonnull MakeTestingEnvDynamicProto( + const T& in, google::protobuf::Arena* absl_nonnull arena) { + const google::protobuf::Descriptor* descriptor = GetTestingEnvDescriptor(); + google::protobuf::Message* out = + ABSL_DIE_IF_NULL( + internal::GetTestingMessageFactory()->GetPrototype(descriptor)) + ->New(arena); + + ABSL_CHECK(out->MergeFromString(in.SerializeAsString())); + return out; +} + } // namespace cel::runtime_internal #endif // THIRD_PARTY_CEL_CPP_RUNTIME_INTERNAL_RUNTIME_ENV_TESTING_H_ diff --git a/runtime/internal/runtime_type_provider.cc b/runtime/internal/runtime_type_provider.cc index e6fd55d2c..5314918bf 100644 --- a/runtime/internal/runtime_type_provider.cc +++ b/runtime/internal/runtime_type_provider.cc @@ -14,6 +14,7 @@ #include "runtime/internal/runtime_type_provider.h" +#include #include #include "absl/base/nullability.h" @@ -28,7 +29,6 @@ #include "common/value.h" #include "common/values/value_builder.h" #include "google/protobuf/arena.h" -#include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" namespace cel::runtime_internal { @@ -48,9 +48,14 @@ absl::StatusOr> RuntimeTypeProvider::FindTypeImpl( if (type.has_value()) { return type; } - const auto* desc = descriptor_pool_->FindMessageTypeByName(name); - if (desc != nullptr) { - return MessageType(desc); + + auto result = descriptor_pool_provider_.FindType(name); + if (!result.ok()) { + return result; + } + + if (result->has_value() && !result->value().IsEnum()) { + return result; } if (const auto it = types_.find(name); it != types_.end()) { @@ -66,24 +71,7 @@ RuntimeTypeProvider::FindEnumConstantImpl(absl::string_view type, if (enum_constant.has_value()) { return enum_constant; } - const google::protobuf::EnumDescriptor* enum_desc = - descriptor_pool_->FindEnumTypeByName(type); - if (enum_desc == nullptr) { - return std::nullopt; - } - - // Note: we don't support strong enum typing at this time so only the fully - // qualified enum values are meaningful, so we don't provide any signal if the - // enum type is found but can't match the value name. - const google::protobuf::EnumValueDescriptor* value_desc = - enum_desc->FindValueByName(value); - if (value_desc == nullptr) { - return std::nullopt; - } - - return TypeIntrospector::EnumConstant{ - EnumType(enum_desc), enum_desc->full_name(), value_desc->name(), - value_desc->number()}; + return descriptor_pool_provider_.FindEnumConstant(type, value); } absl::StatusOr> @@ -93,18 +81,7 @@ RuntimeTypeProvider::FindStructTypeFieldByNameImpl( if (field.has_value()) { return field; } - const auto* desc = descriptor_pool_->FindMessageTypeByName(type); - if (desc == nullptr) { - return std::nullopt; - } - const auto* field_desc = desc->FindFieldByName(name); - if (field_desc == nullptr) { - field_desc = descriptor_pool_->FindExtensionByPrintableName(desc, name); - if (field_desc == nullptr) { - return std::nullopt; - } - } - return MessageTypeField(field_desc); + return descriptor_pool_provider_.FindStructTypeFieldByName(type, name); } absl::StatusOr diff --git a/runtime/internal/runtime_type_provider.h b/runtime/internal/runtime_type_provider.h index 3f418af4d..fc7163f6c 100644 --- a/runtime/internal/runtime_type_provider.h +++ b/runtime/internal/runtime_type_provider.h @@ -21,6 +21,7 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "common/descriptor_pool_type_introspector.h" #include "common/type.h" #include "common/type_reflector.h" #include "common/value.h" @@ -34,7 +35,8 @@ class RuntimeTypeProvider final : public TypeReflector { public: explicit RuntimeTypeProvider( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool) - : descriptor_pool_(descriptor_pool) {} + : descriptor_pool_(descriptor_pool), + descriptor_pool_provider_(descriptor_pool) {} absl::Status RegisterType(const OpaqueType& type); @@ -55,6 +57,7 @@ class RuntimeTypeProvider final : public TypeReflector { private: const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; + DescriptorPoolTypeIntrospector descriptor_pool_provider_; absl::flat_hash_map types_; }; diff --git a/runtime/runtime_options.h b/runtime/runtime_options.h index 7a61208a0..072daf26c 100644 --- a/runtime/runtime_options.h +++ b/runtime/runtime_options.h @@ -188,6 +188,19 @@ struct RuntimeOptions { // // If disabled, will use the legacy behavior of rounding to 6 decimal places. bool enable_precision_preserving_double_format = true; + + // When enabled, the planner will attempt to use a more performant execution + // path for field access when the type is known at plan time, instead of using + // the generic field access implementation. + // + // The runtime will try to verify that the field access is compatible with the + // actual type at evaluation time, and will fall back to the generic + // implementation if the value is not what was expected. + // + // This is not recommended if the values bound to the activation are typically + // not what the planner expected (e.g. a map that was declared as a proto or + // a different message with matching field names). + bool enable_typed_field_access = false; }; // LINT.ThenChange(//depot/google3/eval/public/cel_options.h)