From 0dd0b0bee30775429723ffc34713fe02486b413d Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 5 Aug 2026 16:44:30 -0700 Subject: [PATCH] Update the LegacyTypeProvider to proxy to the normal implementation for field lookups. PiperOrigin-RevId: 959940867 --- eval/compiler/BUILD | 2 - eval/compiler/flat_expr_builder.cc | 2 +- eval/eval/create_struct_step_test.cc | 3 +- eval/public/BUILD | 1 + eval/public/cel_type_registry.h | 21 +- eval/public/cel_type_registry_test.cc | 2 +- eval/public/structs/BUILD | 4 - eval/public/structs/legacy_type_adapter.h | 68 +---- .../structs/legacy_type_adapter_test.cc | 3 +- eval/public/structs/legacy_type_info_apis.h | 16 +- .../structs/legacy_type_provider_test.cc | 2 +- .../structs/proto_message_type_adapter.cc | 178 ------------ .../structs/proto_message_type_adapter.h | 42 +-- .../proto_message_type_adapter_test.cc | 265 +----------------- .../protobuf_descriptor_type_provider.cc | 3 +- .../protobuf_descriptor_type_provider_test.cc | 28 +- .../structs/trivial_legacy_type_info_test.cc | 7 - runtime/internal/BUILD | 14 +- .../internal/legacy_runtime_type_provider.cc | 149 ++-------- .../internal/legacy_runtime_type_provider.h | 28 +- .../legacy_runtime_type_provider_test.cc | 57 +++- runtime/type_registry.cc | 4 +- runtime/type_registry.h | 18 +- 23 files changed, 139 insertions(+), 778 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index 2012abeda..dbd8d9c8b 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -139,7 +139,6 @@ cc_library( "//runtime/internal:issue_collector", "//runtime/internal:runtime_env", "@com_google_absl//absl/algorithm:container", - "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/container:flat_hash_set", @@ -150,7 +149,6 @@ cc_library( "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", - "@com_google_absl//absl/types:optional", "@com_google_absl//absl/types:span", "@com_google_absl//absl/types:variant", "@com_google_protobuf//:protobuf", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index a650657de..53f7cf0c4 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -2660,7 +2660,7 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( const cel::TypeProvider& FlatExprBuilder::GetTypeProvider() const { return use_legacy_type_provider_ ? static_cast( - *GetLegacyRuntimeTypeProvider(type_registry_)) + GetLegacyRuntimeTypeProvider(type_registry_)) : GetRuntimeTypeProvider(type_registry_); } diff --git a/eval/eval/create_struct_step_test.cc b/eval/eval/create_struct_step_test.cc index cd9db9bd9..666dddcdb 100644 --- a/eval/eval/create_struct_step_test.cc +++ b/eval/eval/create_struct_step_test.cc @@ -58,7 +58,6 @@ namespace { using ::absl_testing::IsOk; using ::absl_testing::StatusIs; -using ::cel::Expr; using ::cel::TypeProvider; using ::cel::internal::test::EqualsProto; using ::cel::runtime_internal::NewTestingRuntimeEnv; @@ -200,7 +199,7 @@ TEST_P(CreateCreateStructStepTest, TestEmptyMessageCreation) { auto adapter = env_->legacy_type_registry.FindTypeAdapter( "google.api.expr.runtime.TestMessage"); - ASSERT_TRUE(adapter.has_value() && adapter->mutation_apis() != nullptr); + ASSERT_TRUE(adapter.has_value() && adapter->access_apis() != nullptr); ASSERT_OK_AND_ASSIGN(auto maybe_type, env_->type_registry.GetComposedTypeProvider().FindType( diff --git a/eval/public/BUILD b/eval/public/BUILD index 31ad2d480..7e172cd94 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -813,6 +813,7 @@ cc_library( "//eval/public/structs:legacy_type_provider", "//eval/public/structs:protobuf_descriptor_type_provider", "//runtime:type_registry", + "@com_google_absl//absl/base", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/container:flat_hash_set", diff --git a/eval/public/cel_type_registry.h b/eval/public/cel_type_registry.h index 3fb80bcea..b54b5bb99 100644 --- a/eval/public/cel_type_registry.h +++ b/eval/public/cel_type_registry.h @@ -20,6 +20,7 @@ #include #include +#include "absl/base/call_once.h" #include "absl/base/nullability.h" #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" @@ -28,6 +29,7 @@ #include "base/type_provider.h" #include "eval/public/structs/legacy_type_adapter.h" #include "eval/public/structs/legacy_type_provider.h" +#include "eval/public/structs/protobuf_descriptor_type_provider.h" #include "runtime/type_registry.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" @@ -60,7 +62,9 @@ class CelTypeRegistry { CelTypeRegistry(const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nullable message_factory) - : modern_type_registry_(descriptor_pool, message_factory) {} + : descriptor_pool_(descriptor_pool), + message_factory_(message_factory), + modern_type_registry_(descriptor_pool, message_factory) {} ~CelTypeRegistry() = default; @@ -77,8 +81,12 @@ class CelTypeRegistry { // Get the first registered type provider. std::shared_ptr GetFirstTypeProvider() const { - return cel::runtime_internal::GetLegacyRuntimeTypeProvider( - modern_type_registry_); + absl::call_once(legacy_type_provider_once_, [&]() { + this->legacy_type_provider_ = std::make_shared< + google::api::expr::runtime::ProtobufDescriptorProvider>( + descriptor_pool_, message_factory_); + }); + return legacy_type_provider_; } // Returns the effective type provider that has been configured with the @@ -136,6 +144,13 @@ class CelTypeRegistry { } private: + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; + google::protobuf::MessageFactory* absl_nullable message_factory_; + + // Legacy type provider. This is now disconnected from the actual type + // resolution, but preserved for legacy clients that used it directly. + mutable absl::once_flag legacy_type_provider_once_; + mutable std::shared_ptr legacy_type_provider_; // Internal modern registry. cel::TypeRegistry modern_type_registry_; }; diff --git a/eval/public/cel_type_registry_test.cc b/eval/public/cel_type_registry_test.cc index 881700d55..9dc8d946e 100644 --- a/eval/public/cel_type_registry_test.cc +++ b/eval/public/cel_type_registry_test.cc @@ -36,7 +36,7 @@ class TestTypeProvider : public LegacyTypeProvider { absl::string_view name) const override { for (const auto& type : types_) { if (name == type) { - return LegacyTypeAdapter(/*access=*/nullptr, /*mutation=*/nullptr); + return LegacyTypeAdapter(/*access=*/nullptr); } } return std::nullopt; diff --git a/eval/public/structs/BUILD b/eval/public/structs/BUILD index 75eed88f3..14bd607a3 100644 --- a/eval/public/structs/BUILD +++ b/eval/public/structs/BUILD @@ -273,7 +273,6 @@ cc_library( srcs = ["proto_message_type_adapter.cc"], hdrs = ["proto_message_type_adapter.h"], deps = [ - ":cel_proto_wrap_util", ":field_access_impl", ":legacy_type_adapter", ":legacy_type_info_apis", @@ -311,12 +310,9 @@ cc_test( "//common:value_testing", "//eval/public:cel_value", "//eval/public:message_wrapper", - "//eval/public/containers:container_backed_list_impl", - "//eval/public/containers:container_backed_map_impl", "//eval/public/testing:matchers", "//eval/testutil:test_message_cc_proto", "//extensions/protobuf:memory_manager", - "//internal:proto_matchers", "//internal:testing", "//runtime:runtime_options", "@com_google_absl//absl/status", diff --git a/eval/public/structs/legacy_type_adapter.h b/eval/public/structs/legacy_type_adapter.h index 05fbc9a92..f526460d7 100644 --- a/eval/public/structs/legacy_type_adapter.h +++ b/eval/public/structs/legacy_type_adapter.h @@ -18,7 +18,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_LEGACY_TYPE_ADPATER_H_ #define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_LEGACY_TYPE_ADPATER_H_ -#include +#include #include #include "absl/status/status.h" @@ -36,54 +36,6 @@ namespace google::api::expr::runtime { class DucktypedMessageAdapter; class ProtoMessageTypeAdapter; -// Interface for mutation apis. -// Note: in the new type system, a type provider represents this by returning -// a cel::Type and cel::ValueManager for the type. -class LegacyTypeMutationApis { - public: - virtual ~LegacyTypeMutationApis() = default; - - // Return whether the type defines the given field. - // TODO(uncreated-issue/3): This is only used to eagerly fail during the planning - // phase. Check if it's safe to remove this behavior and fail at runtime. - virtual bool DefinesField(absl::string_view field_name) const = 0; - - // Create a new empty instance of the type. - // May return a status if the type is not possible to create. - virtual absl::StatusOr NewInstance( - cel::MemoryManagerRef memory_manager) const = 0; - - // Normalize special types to a native CEL value after building. - // The interpreter guarantees that instance is uniquely owned by the - // interpreter, and can be safely mutated. - virtual absl::StatusOr AdaptFromWellKnownType( - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder instance) const = 0; - - // Set field on instance to value. - // The interpreter guarantees that instance is uniquely owned by the - // interpreter, and can be safely mutated. - virtual absl::Status SetField( - absl::string_view field_name, const CelValue& value, - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder& instance) const = 0; - - virtual absl::Status SetFieldByNumber( - int64_t field_number [[maybe_unused]], - const CelValue& value [[maybe_unused]], - cel::MemoryManagerRef memory_manager [[maybe_unused]], - CelValue::MessageWrapper::Builder& instance [[maybe_unused]]) const { - return absl::UnimplementedError("SetFieldByNumber is not yet implemented"); - } - - private: - // This class should only be implemented by CEL. Custom structs are only - // supported using the cel::Value APIs. - friend class ProtoMessageTypeAdapter; - - LegacyTypeMutationApis() = default; -}; - // Interface for access apis. // Note: in new type system this is integrated into the StructValue (via // dynamic dispatch to concrete implementations). @@ -162,9 +114,6 @@ class LegacyTypeAccessApis { // Type information about a legacy Struct type. // Provides methods to the interpreter for interacting with a custom type. // -// mutation_apis() provide equivalent behavior to a cel::Type and -// cel::ValueManager (resolved from a type name). -// // access_apis() provide equivalent behavior to cel::StructValue accessors // (virtual dispatch to a concrete implementation for accessing underlying // values). @@ -174,21 +123,18 @@ class LegacyTypeAccessApis { // the type provider that returned this object. class LegacyTypeAdapter { public: - LegacyTypeAdapter(const LegacyTypeAccessApis* access, - const LegacyTypeMutationApis* mutation) - : access_apis_(access), mutation_apis_(mutation) {} + explicit LegacyTypeAdapter(const LegacyTypeAccessApis* access) + : access_apis_(access) {} + // Temporary constructor to support fakes in client tests. + LegacyTypeAdapter(const LegacyTypeAccessApis* access, std::nullptr_t) + : access_apis_(access) {} // Apis for access for the represented type. // If null, access is not supported (this is an opaque type). - const LegacyTypeAccessApis* access_apis() { return access_apis_; } - - // Apis for mutation for the represented type. - // If null, mutation is not supported (this type cannot be created). - const LegacyTypeMutationApis* mutation_apis() { return mutation_apis_; } + const LegacyTypeAccessApis* access_apis() const { return access_apis_; } private: const LegacyTypeAccessApis* access_apis_; - const LegacyTypeMutationApis* mutation_apis_; }; } // namespace google::api::expr::runtime diff --git a/eval/public/structs/legacy_type_adapter_test.cc b/eval/public/structs/legacy_type_adapter_test.cc index 1dabac7d7..603087b6b 100644 --- a/eval/public/structs/legacy_type_adapter_test.cc +++ b/eval/public/structs/legacy_type_adapter_test.cc @@ -23,10 +23,9 @@ namespace { TEST(LegacyTypeAdapter, Basic) { ProtoMessageTypeAdapter adapter(TestMessage::descriptor(), nullptr); - LegacyTypeAdapter type_adapter(&adapter, &adapter); + LegacyTypeAdapter type_adapter(&adapter); EXPECT_EQ(type_adapter.access_apis(), &adapter); - EXPECT_EQ(type_adapter.mutation_apis(), &adapter); } } // namespace diff --git a/eval/public/structs/legacy_type_info_apis.h b/eval/public/structs/legacy_type_info_apis.h index 90e2f3710..d7f5db3fa 100644 --- a/eval/public/structs/legacy_type_info_apis.h +++ b/eval/public/structs/legacy_type_info_apis.h @@ -27,7 +27,6 @@ namespace google::api::expr::runtime { // Forward declared to resolve cyclic dependency. class LegacyTypeAccessApis; -class LegacyTypeMutationApis; // Forward declare permitted subclasses. class DucktypedMessageAdapter; @@ -40,8 +39,8 @@ class TrivialTypeInfo; // Provides ability to obtain field access apis, type info, and debug // representation of a message. // -// The message parameter may wrap a nullptr to request generic accessors / -// mutators for the TypeInfo instance if it is available. +// The message parameter may wrap a nullptr to request generic accessors for +// the TypeInfo instance if it is available. // // This is implemented as a separate class from LegacyTypeAccessApis to resolve // cyclic dependency between CelValue (which needs to access these apis to @@ -87,17 +86,6 @@ class LegacyTypeInfoApis { virtual const LegacyTypeAccessApis* GetAccessApis( const MessageWrapper& wrapped_message) const = 0; - // Return a pointer to the wrapped message's mutation api implementation. - // - // The CEL interpreter assumes that the returned pointer is owned externally - // and will outlive any CelValues created by the interpreter. - // - // Nullptr signals that the value does not provide mutation apis. - virtual const LegacyTypeMutationApis* GetMutationApis( - const MessageWrapper& wrapped_message [[maybe_unused]]) const { - return nullptr; - } - // Return a description of the underlying field if defined. // // The underlying string is expected to remain valid as long as the diff --git a/eval/public/structs/legacy_type_provider_test.cc b/eval/public/structs/legacy_type_provider_test.cc index c8059da7c..59246fed5 100644 --- a/eval/public/structs/legacy_type_provider_test.cc +++ b/eval/public/structs/legacy_type_provider_test.cc @@ -41,7 +41,7 @@ class LegacyTypeProviderTestImpl : public LegacyTypeProvider { absl::optional ProvideLegacyType( absl::string_view name) const override { if (name == "test") { - return LegacyTypeAdapter(nullptr, nullptr); + return LegacyTypeAdapter(nullptr); } return std::nullopt; } diff --git a/eval/public/structs/proto_message_type_adapter.cc b/eval/public/structs/proto_message_type_adapter.cc index db5f3c6e2..2f72ec68a 100644 --- a/eval/public/structs/proto_message_type_adapter.cc +++ b/eval/public/structs/proto_message_type_adapter.cc @@ -26,7 +26,6 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" -#include "absl/strings/substitute.h" #include "absl/types/span.h" #include "base/attribute.h" #include "common/memory.h" @@ -35,7 +34,6 @@ #include "eval/public/containers/internal_field_backed_list_impl.h" #include "eval/public/containers/internal_field_backed_map_impl.h" #include "eval/public/message_wrapper.h" -#include "eval/public/structs/cel_proto_wrap_util.h" #include "eval/public/structs/field_access_impl.h" #include "eval/public/structs/legacy_type_adapter.h" #include "eval/public/structs/legacy_type_info_apis.h" @@ -360,11 +358,6 @@ class DucktypedMessageAdapter : public LegacyTypeAccessApis, return this; } - const LegacyTypeMutationApis* GetMutationApis( - const MessageWrapper& wrapped_message) const override { - return nullptr; - } - static const DucktypedMessageAdapter& GetSingleton() { static absl::NoDestructor instance; return *instance; @@ -418,12 +411,6 @@ absl::string_view ProtoMessageTypeAdapter::GetTypename( return descriptor_->full_name(); } -const LegacyTypeMutationApis* ProtoMessageTypeAdapter::GetMutationApis( - const MessageWrapper& wrapped_message) const { - // Defer checks for misuse on wrong message kind in the accessor calls. - return this; -} - const LegacyTypeAccessApis* ProtoMessageTypeAdapter::GetAccessApis( const MessageWrapper& wrapped_message) const { // Defer checks for misuse on wrong message kind in the builder calls. @@ -447,41 +434,6 @@ ProtoMessageTypeAdapter::FindFieldByName(absl::string_view field_name) const { field_descriptor->name()}; } -absl::Status ProtoMessageTypeAdapter::ValidateSetFieldOp( - bool assertion, absl::string_view field, absl::string_view detail) const { - if (!assertion) { - return absl::InvalidArgumentError( - absl::Substitute("SetField failed on message $0, field '$1': $2", - descriptor_->full_name(), field, detail)); - } - return absl::OkStatus(); -} - -absl::StatusOr -ProtoMessageTypeAdapter::NewInstance( - cel::MemoryManagerRef memory_manager) const { - if (message_factory_ == nullptr) { - return absl::UnimplementedError( - absl::StrCat("Cannot create message ", descriptor_->name())); - } - - // This implementation requires arena-backed memory manager. - google::protobuf::Arena* arena = ProtoMemoryManagerArena(memory_manager); - const Message* prototype = message_factory_->GetPrototype(descriptor_); - - Message* msg = (prototype != nullptr) ? prototype->New(arena) : nullptr; - - if (msg == nullptr) { - return absl::InvalidArgumentError( - absl::StrCat("Failed to create message ", descriptor_->name())); - } - return MessageWrapper::Builder(msg); -} - -bool ProtoMessageTypeAdapter::DefinesField(absl::string_view field_name) const { - return descriptor_->FindFieldByName(field_name) != nullptr; -} - absl::StatusOr ProtoMessageTypeAdapter::HasField( absl::string_view field_name, const CelValue::MessageWrapper& value) const { CEL_ASSIGN_OR_RETURN(const google::protobuf::Message* message, @@ -512,136 +464,6 @@ ProtoMessageTypeAdapter::Qualify( memory_manager); } -absl::Status ProtoMessageTypeAdapter::SetField( - const google::protobuf::FieldDescriptor* field, const CelValue& value, - google::protobuf::Arena* arena, google::protobuf::Message* message) const { - if (field->is_map()) { - constexpr int kKeyField = 1; - constexpr int kValueField = 2; - - const CelMap* cel_map; - CEL_RETURN_IF_ERROR(ValidateSetFieldOp( - value.GetValue(&cel_map) && cel_map != nullptr, - field->name(), - absl::StrCat("value is not CelMap - value is ", - CelValue::TypeName(value.type())))); - - auto entry_descriptor = field->message_type(); - - CEL_RETURN_IF_ERROR( - ValidateSetFieldOp(entry_descriptor != nullptr, field->name(), - "failed to find map entry descriptor")); - auto key_field_descriptor = entry_descriptor->FindFieldByNumber(kKeyField); - auto value_field_descriptor = - entry_descriptor->FindFieldByNumber(kValueField); - - CEL_RETURN_IF_ERROR( - ValidateSetFieldOp(key_field_descriptor != nullptr, field->name(), - "failed to find key field descriptor")); - - CEL_RETURN_IF_ERROR( - ValidateSetFieldOp(value_field_descriptor != nullptr, field->name(), - "failed to find value field descriptor")); - - bool prune_when_null = false; - if (value_field_descriptor->cpp_type() == - google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) { - auto well_known_type = - value_field_descriptor->message_type()->well_known_type(); - if (well_known_type != google::protobuf::Descriptor::WELLKNOWNTYPE_ANY && - well_known_type != google::protobuf::Descriptor::WELLKNOWNTYPE_VALUE && - well_known_type != google::protobuf::Descriptor::WELLKNOWNTYPE_LISTVALUE && - well_known_type != google::protobuf::Descriptor::WELLKNOWNTYPE_STRUCT) { - prune_when_null = true; - } - } - - CEL_ASSIGN_OR_RETURN(const CelList* key_list, cel_map->ListKeys(arena)); - for (int i = 0; i < key_list->size(); i++) { - CelValue key = (*key_list).Get(arena, i); - - auto value = (*cel_map).Get(arena, key); - CEL_RETURN_IF_ERROR(ValidateSetFieldOp(value.has_value(), field->name(), - "error serializing CelMap")); - if (prune_when_null && value->IsNull()) { - continue; - } - Message* entry_msg = message->GetReflection()->AddMessage(message, field); - CEL_RETURN_IF_ERROR(internal::SetValueToSingleField( - key, key_field_descriptor, entry_msg, arena)); - CEL_RETURN_IF_ERROR(internal::SetValueToSingleField( - value.value(), value_field_descriptor, entry_msg, arena)); - } - - } else if (field->is_repeated()) { - const CelList* cel_list; - CEL_RETURN_IF_ERROR(ValidateSetFieldOp( - value.GetValue(&cel_list) && cel_list != nullptr, - field->name(), - absl::StrCat("expected CelList value - value is", - CelValue::TypeName(value.type())))); - - for (int i = 0; i < cel_list->size(); i++) { - CEL_RETURN_IF_ERROR(internal::AddValueToRepeatedField( - (*cel_list).Get(arena, i), field, message, arena)); - } - } else { - CEL_RETURN_IF_ERROR( - internal::SetValueToSingleField(value, field, message, arena)); - } - return absl::OkStatus(); -} - -absl::Status ProtoMessageTypeAdapter::SetField( - absl::string_view field_name, const CelValue& value, - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder& instance) const { - // Assume proto arena implementation if this provider is used. - google::protobuf::Arena* arena = - cel::extensions::ProtoMemoryManagerArena(memory_manager); - - CEL_ASSIGN_OR_RETURN(google::protobuf::Message * mutable_message, - UnwrapMessage(instance, "SetField")); - - const google::protobuf::FieldDescriptor* field_descriptor = - descriptor_->FindFieldByName(field_name); - CEL_RETURN_IF_ERROR( - ValidateSetFieldOp(field_descriptor != nullptr, field_name, "not found")); - - return SetField(field_descriptor, value, arena, mutable_message); -} - -absl::Status ProtoMessageTypeAdapter::SetFieldByNumber( - int64_t field_number, const CelValue& value, - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder& instance) const { - // Assume proto arena implementation if this provider is used. - google::protobuf::Arena* arena = - cel::extensions::ProtoMemoryManagerArena(memory_manager); - - CEL_ASSIGN_OR_RETURN(google::protobuf::Message * mutable_message, - UnwrapMessage(instance, "SetField")); - - const google::protobuf::FieldDescriptor* field_descriptor = - descriptor_->FindFieldByNumber(field_number); - CEL_RETURN_IF_ERROR(ValidateSetFieldOp( - field_descriptor != nullptr, absl::StrCat(field_number), "not found")); - - return SetField(field_descriptor, value, arena, mutable_message); -} - -absl::StatusOr ProtoMessageTypeAdapter::AdaptFromWellKnownType( - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder instance) const { - // Assume proto arena implementation if this provider is used. - google::protobuf::Arena* arena = - cel::extensions::ProtoMemoryManagerArena(memory_manager); - CEL_ASSIGN_OR_RETURN(google::protobuf::Message * message, - UnwrapMessage(instance, "AdaptFromWellKnownType")); - return internal::UnwrapMessageToValue(message, &MessageCelValueFactory, - arena); -} - bool ProtoMessageTypeAdapter::IsEqualTo( const CelValue::MessageWrapper& instance, const CelValue::MessageWrapper& other_instance) const { diff --git a/eval/public/structs/proto_message_type_adapter.h b/eval/public/structs/proto_message_type_adapter.h index f4e6b4c8b..867ba39cf 100644 --- a/eval/public/structs/proto_message_type_adapter.h +++ b/eval/public/structs/proto_message_type_adapter.h @@ -38,12 +38,12 @@ namespace google::api::expr::runtime { // generally the duck-typed instance to support the default behavior of // deferring to the protobuf reflection apis on the message instance. class ProtoMessageTypeAdapter : public LegacyTypeInfoApis, - public LegacyTypeAccessApis, - public LegacyTypeMutationApis { + public LegacyTypeAccessApis { public: - ProtoMessageTypeAdapter(const google::protobuf::Descriptor* absl_nonnull descriptor, - google::protobuf::MessageFactory* message_factory) - : message_factory_(message_factory), descriptor_(descriptor) {} + explicit ProtoMessageTypeAdapter( + const google::protobuf::Descriptor* absl_nonnull descriptor, + google::protobuf::MessageFactory* message_factory = nullptr) + : descriptor_(descriptor) {} ~ProtoMessageTypeAdapter() override = default; @@ -61,32 +61,9 @@ class ProtoMessageTypeAdapter : public LegacyTypeInfoApis, const LegacyTypeAccessApis* GetAccessApis( const MessageWrapper& wrapped_message) const override; - const LegacyTypeMutationApis* GetMutationApis( - const MessageWrapper& wrapped_message) const override; - absl::optional FindFieldByName( absl::string_view field_name) const override; - // Implement LegacyTypeMutation APIs. - absl::StatusOr NewInstance( - cel::MemoryManagerRef memory_manager) const override; - - bool DefinesField(absl::string_view field_name) const override; - - absl::Status SetField( - absl::string_view field_name, const CelValue& value, - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder& instance) const override; - - absl::Status SetFieldByNumber( - int64_t field_number, const CelValue& value, - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder& instance) const override; - - absl::StatusOr AdaptFromWellKnownType( - cel::MemoryManagerRef memory_manager, - CelValue::MessageWrapper::Builder instance) const override; - // Implement LegacyTypeAccessAPIs. absl::StatusOr GetField( absl::string_view field_name, const CelValue::MessageWrapper& instance, @@ -113,15 +90,6 @@ class ProtoMessageTypeAdapter : public LegacyTypeInfoApis, } private: - // Helper for standardizing error messages for SetField operation. - absl::Status ValidateSetFieldOp(bool assertion, absl::string_view field, - absl::string_view detail) const; - - absl::Status SetField(const google::protobuf::FieldDescriptor* field, - const CelValue& value, google::protobuf::Arena* arena, - google::protobuf::Message* message) const; - - google::protobuf::MessageFactory* message_factory_; const google::protobuf::Descriptor* absl_nonnull descriptor_; }; diff --git a/eval/public/structs/proto_message_type_adapter_test.cc b/eval/public/structs/proto_message_type_adapter_test.cc index c0e60c632..a74a32cb4 100644 --- a/eval/public/structs/proto_message_type_adapter_test.cc +++ b/eval/public/structs/proto_message_type_adapter_test.cc @@ -14,6 +14,7 @@ #include "eval/public/structs/proto_message_type_adapter.h" +#include #include #include "google/protobuf/wrappers.pb.h" @@ -24,15 +25,12 @@ #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" #include "eval/public/message_wrapper.h" #include "eval/public/structs/legacy_type_adapter.h" #include "eval/public/structs/legacy_type_info_apis.h" #include "eval/public/testing/matchers.h" #include "eval/testutil/test_message.pb.h" #include "extensions/protobuf/memory_manager.h" -#include "internal/proto_matchers.h" #include "internal/testing.h" #include "runtime/runtime_options.h" #include "google/protobuf/arena.h" @@ -48,12 +46,9 @@ using ::absl_testing::IsOkAndHolds; using ::absl_testing::StatusIs; using ::cel::ProtoWrapperTypeOptions; using ::cel::extensions::ProtoMemoryManagerRef; -using ::cel::internal::test::EqualsProto; using ::google::protobuf::Int64Value; using ::testing::_; -using ::testing::AllOf; using ::testing::ElementsAre; -using ::testing::Eq; using ::testing::Field; using ::testing::HasSubstr; using ::testing::Optional; @@ -427,247 +422,6 @@ TEST(GetGenericProtoTypeInfoInstance, FallbackForNonMessage) { EXPECT_EQ(info_api.DebugString(null_message), ""); } -TEST(ProtoMessageTypeAdapter, NewInstance) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder result, - adapter.NewInstance(manager)); - EXPECT_EQ(result.message_ptr()->SerializeAsString(), ""); -} - -TEST(ProtoMessageTypeAdapter, NewInstanceUnsupportedDescriptor) { - google::protobuf::Arena arena; - - google::protobuf::DescriptorPool pool; - google::protobuf::FileDescriptorProto faked_file; - faked_file.set_name("faked.proto"); - faked_file.set_syntax("proto3"); - faked_file.set_package("google.api.expr.runtime"); - auto msg_descriptor = faked_file.add_message_type(); - msg_descriptor->set_name("FakeMessage"); - pool.BuildFile(faked_file); - - ProtoMessageTypeAdapter adapter( - pool.FindMessageTypeByName("google.api.expr.runtime.FakeMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - // Message factory doesn't know how to create our custom message, even though - // we provided a descriptor for it. - EXPECT_THAT( - adapter.NewInstance(manager), - StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("FakeMessage"))); -} - -TEST(ProtoMessageTypeAdapter, DefinesField) { - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - - EXPECT_TRUE(adapter.DefinesField("int64_value")); - EXPECT_FALSE(adapter.DefinesField("not_a_field")); -} - -TEST(ProtoMessageTypeAdapter, SetFieldSingular) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder value, - adapter.NewInstance(manager)); - - ASSERT_OK(adapter.SetField("int64_value", CelValue::CreateInt64(10), manager, - value)); - - TestMessage message; - message.set_int64_value(10); - EXPECT_EQ(value.message_ptr()->SerializeAsString(), - message.SerializeAsString()); - - ASSERT_THAT(adapter.SetField("not_a_field", CelValue::CreateInt64(10), - manager, value), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("field 'not_a_field': not found"))); -} - -TEST(ProtoMessageTypeAdapter, SetFieldRepeated) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - ContainerBackedListImpl list( - {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}); - CelValue value_to_set = CelValue::CreateList(&list); - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder instance, - adapter.NewInstance(manager)); - - ASSERT_OK(adapter.SetField("int64_list", value_to_set, manager, instance)); - - TestMessage message; - message.add_int64_list(1); - message.add_int64_list(2); - - EXPECT_EQ(instance.message_ptr()->SerializeAsString(), - message.SerializeAsString()); -} - -TEST(ProtoMessageTypeAdapter, SetFieldNotAField) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder instance, - adapter.NewInstance(manager)); - - ASSERT_THAT(adapter.SetField("not_a_field", CelValue::CreateInt64(10), - manager, instance), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("field 'not_a_field': not found"))); -} - -TEST(ProtoMesssageTypeAdapter, SetFieldWrongType) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - ContainerBackedListImpl list( - {CelValue::CreateInt64(1), CelValue::CreateInt64(2)}); - CelValue list_value = CelValue::CreateList(&list); - - CelMapBuilder builder; - ASSERT_OK(builder.Add(CelValue::CreateInt64(1), CelValue::CreateInt64(2))); - ASSERT_OK(builder.Add(CelValue::CreateInt64(2), CelValue::CreateInt64(4))); - - CelValue map_value = CelValue::CreateMap(&builder); - - CelValue int_value = CelValue::CreateInt64(42); - - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder instance, - adapter.NewInstance(manager)); - - EXPECT_THAT(adapter.SetField("int64_value", map_value, manager, instance), - StatusIs(absl::StatusCode::kInvalidArgument)); - EXPECT_THAT(adapter.SetField("int64_value", list_value, manager, instance), - StatusIs(absl::StatusCode::kInvalidArgument)); - - EXPECT_THAT( - adapter.SetField("int64_int32_map", list_value, manager, instance), - StatusIs(absl::StatusCode::kInvalidArgument)); - EXPECT_THAT(adapter.SetField("int64_int32_map", int_value, manager, instance), - StatusIs(absl::StatusCode::kInvalidArgument)); - - EXPECT_THAT(adapter.SetField("int64_list", int_value, manager, instance), - StatusIs(absl::StatusCode::kInvalidArgument)); - EXPECT_THAT(adapter.SetField("int64_list", map_value, manager, instance), - StatusIs(absl::StatusCode::kInvalidArgument)); -} - -TEST(ProtoMesssageTypeAdapter, SetFieldNotAMessage) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - CelValue int_value = CelValue::CreateInt64(42); - CelValue::MessageWrapper::Builder instance( - static_cast(nullptr)); - - EXPECT_THAT(adapter.SetField("int64_value", int_value, manager, instance), - StatusIs(absl::StatusCode::kInternal)); -} - -TEST(ProtoMesssageTypeAdapter, SetFieldNullMessage) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - CelValue int_value = CelValue::CreateInt64(42); - CelValue::MessageWrapper::Builder instance( - static_cast(nullptr)); - - EXPECT_THAT(adapter.SetField("int64_value", int_value, manager, instance), - StatusIs(absl::StatusCode::kInternal)); -} - -TEST(ProtoMessageTypeAdapter, AdaptFromWellKnownType) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.protobuf.Int64Value"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder instance, - adapter.NewInstance(manager)); - ASSERT_OK( - adapter.SetField("value", CelValue::CreateInt64(42), manager, instance)); - - ASSERT_OK_AND_ASSIGN(CelValue value, - adapter.AdaptFromWellKnownType(manager, instance)); - - EXPECT_THAT(value, test::IsCelInt64(42)); -} - -TEST(ProtoMessageTypeAdapter, AdaptFromWellKnownTypeUnspecial) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder instance, - adapter.NewInstance(manager)); - - ASSERT_OK(adapter.SetField("int64_value", CelValue::CreateInt64(42), manager, - instance)); - ASSERT_OK_AND_ASSIGN(CelValue value, - adapter.AdaptFromWellKnownType(manager, instance)); - - // TestMessage should not be converted to a CEL primitive type. - EXPECT_THAT(value, test::IsCelMessage(EqualsProto("int64_value: 42"))); -} - -TEST(ProtoMessageTypeAdapter, AdaptFromWellKnownTypeNotAMessageError) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - CelValue::MessageWrapper::Builder instance( - static_cast(nullptr)); - - // Interpreter guaranteed to call this with a message type, otherwise, - // something has broken. - EXPECT_THAT(adapter.AdaptFromWellKnownType(manager, instance), - StatusIs(absl::StatusCode::kInternal)); -} - TEST(ProtoMesssageTypeAdapter, TypeInfoDebug) { ProtoMessageTypeAdapter adapter( google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( @@ -716,23 +470,6 @@ TEST(ProtoMesssageTypeAdapter, FindFieldNotFound) { EXPECT_EQ(adapter.FindFieldByName("foo_not_a_field"), std::nullopt); } -TEST(ProtoMesssageTypeAdapter, TypeInfoMutator) { - google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); - auto manager = ProtoMemoryManagerRef(&arena); - - const LegacyTypeMutationApis* api = adapter.GetMutationApis(MessageWrapper()); - ASSERT_NE(api, nullptr); - - ASSERT_OK_AND_ASSIGN(MessageWrapper::Builder builder, - api->NewInstance(manager)); - EXPECT_NE(google::protobuf::DynamicCastMessage(builder.message_ptr()), - nullptr); -} - TEST(ProtoMesssageTypeAdapter, TypeInfoAccesor) { google::protobuf::Arena arena; ProtoMessageTypeAdapter adapter( diff --git a/eval/public/structs/protobuf_descriptor_type_provider.cc b/eval/public/structs/protobuf_descriptor_type_provider.cc index b5746523e..37fc8ce77 100644 --- a/eval/public/structs/protobuf_descriptor_type_provider.cc +++ b/eval/public/structs/protobuf_descriptor_type_provider.cc @@ -29,8 +29,7 @@ absl::optional ProtobufDescriptorProvider::ProvideLegacyType( if (result == nullptr) { return std::nullopt; } - // ProtoMessageTypeAdapter provides apis for both access and mutation. - return LegacyTypeAdapter(result, result); + return LegacyTypeAdapter(result); } absl::optional diff --git a/eval/public/structs/protobuf_descriptor_type_provider_test.cc b/eval/public/structs/protobuf_descriptor_type_provider_test.cc index f2b7900ef..ed1ddb811 100644 --- a/eval/public/structs/protobuf_descriptor_type_provider_test.cc +++ b/eval/public/structs/protobuf_descriptor_type_provider_test.cc @@ -17,34 +17,25 @@ #include #include "google/protobuf/wrappers.pb.h" -#include "absl/status/status_matchers.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 "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::extensions::ProtoMemoryManager; - TEST(ProtobufDescriptorProvider, Basic) { ProtobufDescriptorProvider provider( google::protobuf::DescriptorPool::generated_pool(), google::protobuf::MessageFactory::generated_factory()); - google::protobuf::Arena arena; - auto manager = ProtoMemoryManager(&arena); auto type_adapter = provider.ProvideLegacyType("google.protobuf.Int64Value"); std::optional type_info = provider.ProvideLegacyTypeInfo("google.protobuf.Int64Value"); ASSERT_TRUE(type_adapter.has_value()); - ASSERT_TRUE(type_adapter->mutation_apis() != nullptr); + ASSERT_TRUE(type_adapter->access_apis() != nullptr); ASSERT_TRUE(type_info.has_value()); ASSERT_TRUE(type_info != nullptr); @@ -52,20 +43,6 @@ TEST(ProtobufDescriptorProvider, Basic) { CelValue::MessageWrapper int64_cel_value(&int64_value, *type_info); EXPECT_EQ((*type_info)->GetTypename(int64_cel_value), "google.protobuf.Int64Value"); - - ASSERT_TRUE(type_adapter->mutation_apis()->DefinesField("value")); - ASSERT_OK_AND_ASSIGN(CelValue::MessageWrapper::Builder value, - type_adapter->mutation_apis()->NewInstance(manager)); - - ASSERT_THAT(type_adapter->mutation_apis()->SetField( - "value", CelValue::CreateInt64(10), manager, value), - IsOk()); - - ASSERT_OK_AND_ASSIGN( - CelValue adapted, - type_adapter->mutation_apis()->AdaptFromWellKnownType(manager, value)); - - EXPECT_THAT(adapted, test::IsCelInt64(10)); } // This is an implementation detail, but testing for coverage. @@ -76,12 +53,11 @@ TEST(ProtobufDescriptorProvider, MemoizesAdapters) { auto type_adapter = provider.ProvideLegacyType("google.protobuf.Int64Value"); ASSERT_TRUE(type_adapter.has_value()); - ASSERT_TRUE(type_adapter->mutation_apis() != nullptr); + ASSERT_TRUE(type_adapter->access_apis() != nullptr); auto type_adapter2 = provider.ProvideLegacyType("google.protobuf.Int64Value"); ASSERT_TRUE(type_adapter2.has_value()); - EXPECT_EQ(type_adapter->mutation_apis(), type_adapter2->mutation_apis()); EXPECT_EQ(type_adapter->access_apis(), type_adapter2->access_apis()); } diff --git a/eval/public/structs/trivial_legacy_type_info_test.cc b/eval/public/structs/trivial_legacy_type_info_test.cc index 9cc6e4916..ea7b3977e 100644 --- a/eval/public/structs/trivial_legacy_type_info_test.cc +++ b/eval/public/structs/trivial_legacy_type_info_test.cc @@ -44,13 +44,6 @@ TEST(TrivialTypeInfo, GetAccessApis) { EXPECT_EQ(TrivialTypeInfo::GetInstance()->GetAccessApis(wrapper), nullptr); } -TEST(TrivialTypeInfo, GetMutationApis) { - TrivialTypeInfo info; - MessageWrapper wrapper; - - EXPECT_EQ(info.GetMutationApis(wrapper), nullptr); - EXPECT_EQ(TrivialTypeInfo::GetInstance()->GetMutationApis(wrapper), nullptr); -} TEST(TrivialTypeInfo, FindFieldByName) { TrivialTypeInfo info; diff --git a/runtime/internal/BUILD b/runtime/internal/BUILD index 44671102a..d8a442f34 100644 --- a/runtime/internal/BUILD +++ b/runtime/internal/BUILD @@ -184,23 +184,14 @@ cc_library( srcs = ["legacy_runtime_type_provider.cc"], hdrs = ["legacy_runtime_type_provider.h"], deps = [ + ":runtime_type_provider", "//common:legacy_value", - "//common:memory", "//common:type", "//common:value", - "//eval/public:message_wrapper", - "//eval/public/structs:legacy_type_adapter", - "//eval/public/structs:legacy_type_info_apis", - "//eval/public/structs:proto_message_type_adapter", - "//eval/public/structs:protobuf_descriptor_type_provider", - "//extensions/protobuf:memory_manager", "//internal:status_macros", "@com_google_absl//absl/base:nullability", - "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", - "@com_google_absl//absl/types:optional", "@com_google_protobuf//:protobuf", ], ) @@ -210,13 +201,12 @@ cc_test( srcs = ["legacy_runtime_type_provider_test.cc"], deps = [ ":legacy_runtime_type_provider", + ":runtime_type_provider", "//common:type", "//common:value", "//internal:testing", "//internal:testing_descriptor_pool", "//internal:testing_message_factory", - "@com_google_absl//absl/status:status_matchers", - "@com_google_absl//absl/types:optional", "@com_google_cel_spec//proto/cel/expr/conformance/proto3:test_all_types_cc_proto", "@com_google_protobuf//:protobuf", ], diff --git a/runtime/internal/legacy_runtime_type_provider.cc b/runtime/internal/legacy_runtime_type_provider.cc index 5db0b448e..9eca07652 100644 --- a/runtime/internal/legacy_runtime_type_provider.cc +++ b/runtime/internal/legacy_runtime_type_provider.cc @@ -20,85 +20,53 @@ #include #include "absl/base/nullability.h" -#include "absl/status/status.h" #include "absl/status/statusor.h" -#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" -#include "absl/types/optional.h" #include "common/legacy_value.h" -#include "common/memory.h" -#include "common/type.h" -#include "common/type_introspector.h" #include "common/value.h" -#include "eval/public/message_wrapper.h" -#include "eval/public/structs/legacy_type_adapter.h" -#include "eval/public/structs/legacy_type_info_apis.h" -#include "eval/public/structs/proto_message_type_adapter.h" -#include "extensions/protobuf/memory_manager.h" +#include "common/values/value_builder.h" #include "internal/status_macros.h" #include "google/protobuf/arena.h" -#include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" namespace cel::runtime_internal { namespace { -using google::api::expr::runtime::LegacyTypeAdapter; -using google::api::expr::runtime::LegacyTypeInfoApis; -using google::api::expr::runtime::MessageWrapper; - class LegacyValueBuilder final : public cel::ValueBuilder { public: - LegacyValueBuilder(cel::MemoryManagerRef memory_manager, - LegacyTypeAdapter adapter, MessageWrapper::Builder builder) - : memory_manager_(memory_manager), - adapter_(adapter), - builder_(std::move(builder)) {} + LegacyValueBuilder(google::protobuf::Arena* absl_nonnull arena, + cel::ValueBuilderPtr builder) + : arena_(arena), builder_(std::move(builder)) {} - absl::StatusOr> SetFieldByName( + absl::StatusOr> SetFieldByName( absl::string_view name, cel::Value value) override { - CEL_ASSIGN_OR_RETURN( - auto legacy_value, - LegacyValue(cel::extensions::ProtoMemoryManagerArena(memory_manager_), - value), - _.With(cel::ErrorValueReturn())); - CEL_RETURN_IF_ERROR(adapter_.mutation_apis()->SetField( - name, legacy_value, memory_manager_, builder_)) - .With(cel::ErrorValueReturn()); - return std::nullopt; + return builder_->SetFieldByName(name, std::move(value)); } - absl::StatusOr> SetFieldByNumber( + absl::StatusOr> SetFieldByNumber( int64_t number, cel::Value value) override { - CEL_ASSIGN_OR_RETURN( - auto legacy_value, - LegacyValue(cel::extensions::ProtoMemoryManagerArena(memory_manager_), - value), - _.With(cel::ErrorValueReturn())); - CEL_RETURN_IF_ERROR(adapter_.mutation_apis()->SetFieldByNumber( - number, legacy_value, memory_manager_, builder_)) - .With(cel::ErrorValueReturn()); - return std::nullopt; + return builder_->SetFieldByNumber(number, std::move(value)); } absl::StatusOr Build() && override { - CEL_ASSIGN_OR_RETURN(auto value, - adapter_.mutation_apis()->AdaptFromWellKnownType( - memory_manager_, std::move(builder_)), + CEL_ASSIGN_OR_RETURN(auto value, std::move(*builder_).Build(), _.With(cel::ErrorValueReturn())); - CEL_ASSIGN_OR_RETURN( - auto result, - cel::ModernValue( - cel::extensions::ProtoMemoryManagerArena(memory_manager_), value), - _.With(cel::ErrorValueReturn())); - return result; + if (value.Is()) { + // Make the value behave like a legacy message. Minimizes further + // legacy/modern conversions (e.g. on return and when accessing fields). + CEL_ASSIGN_OR_RETURN(auto legacy_value, LegacyValue(arena_, value), + _.With(cel::ErrorValueReturn())); + CEL_ASSIGN_OR_RETURN(auto result, ModernValue(arena_, legacy_value), + _.With(cel::ErrorValueReturn())); + return result; + } + return value; } private: - cel::MemoryManagerRef memory_manager_; - LegacyTypeAdapter adapter_; - MessageWrapper::Builder builder_; + google::protobuf::Arena* const arena_; + cel::ValueBuilderPtr builder_; }; } // namespace @@ -108,79 +76,12 @@ LegacyRuntimeTypeProvider::NewValueBuilder( absl::string_view name, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) const { - auto type_adapter = ProvideLegacyType(name); - - if (!type_adapter.has_value()) { + auto builder = common_internal::NewValueBuilder(arena, descriptor_pool_, + message_factory, name); + if (builder == nullptr) { return nullptr; } - - // We know the implementation should not do this, but can't prove it to type - // system. - // Defensive checks but impractical to exercise. - const auto* mutation_apis = type_adapter->mutation_apis(); - if (mutation_apis == nullptr) { - return absl::FailedPreconditionError( - absl::StrCat("LegacyTypeMutationApis missing for type: ", name)); - } - - CEL_ASSIGN_OR_RETURN( - auto builder, - mutation_apis->NewInstance(cel::MemoryManagerRef::Pooling(arena))); - return std::make_unique( - cel::MemoryManagerRef::Pooling(arena), *type_adapter, std::move(builder)); -} - -absl::StatusOr> LegacyRuntimeTypeProvider::FindTypeImpl( - absl::string_view name) const { - if (auto type = cel::FindWellKnownType(name); type.has_value()) { - return type; - } - if (auto type_info = ProvideLegacyTypeInfo(name); type_info.has_value()) { - const auto* descriptor = (*type_info)->GetDescriptor(MessageWrapper()); - if (descriptor != nullptr) { - return cel::MessageType(descriptor); - } - return cel::common_internal::MakeBasicStructType( - (*type_info)->GetTypename(MessageWrapper())); - } - return std::nullopt; -} - -absl::StatusOr> -LegacyRuntimeTypeProvider::FindStructTypeFieldByNameImpl( - absl::string_view type, absl::string_view name) const { - if (auto result = cel::FindWellKnownTypeFieldByName(type, name); - result.has_value()) { - return result; - } - std::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)); - } - } - - 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{}); + return std::make_unique(arena, std::move(builder)); } } // namespace cel::runtime_internal diff --git a/runtime/internal/legacy_runtime_type_provider.h b/runtime/internal/legacy_runtime_type_provider.h index 45aef5d75..2d289f3ed 100644 --- a/runtime/internal/legacy_runtime_type_provider.h +++ b/runtime/internal/legacy_runtime_type_provider.h @@ -23,22 +23,24 @@ #include "common/type.h" #include "common/type_reflector.h" #include "common/value.h" -#include "eval/public/structs/protobuf_descriptor_type_provider.h" +#include "runtime/internal/runtime_type_provider.h" #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" namespace cel::runtime_internal { -class LegacyRuntimeTypeProvider final - : public google::api::expr::runtime::ProtobufDescriptorProvider, - public TypeReflector { +// LegacyRuntimeTypeProvider is a TypeReflector that uses a RuntimeTypeProvider +// internally to provide types with the google::api::expr::runtime::CelValue +// APIs. It prefers to create wrapped legacy values but otherwise proxies to +// the standard RuntimeTypeProvider. +class LegacyRuntimeTypeProvider final : public TypeReflector { public: LegacyRuntimeTypeProvider( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, - google::protobuf::MessageFactory* absl_nullable message_factory) - : google::api::expr::runtime::ProtobufDescriptorProvider( - descriptor_pool, message_factory) {} + const RuntimeTypeProvider* absl_nonnull runtime_type_provider) + : descriptor_pool_(descriptor_pool), + runtime_type_provider_(runtime_type_provider) {} absl::StatusOr NewValueBuilder( absl::string_view name, @@ -47,10 +49,18 @@ class LegacyRuntimeTypeProvider final protected: absl::StatusOr> FindTypeImpl( - absl::string_view name) const override; + absl::string_view name) const override { + return runtime_type_provider_->FindType(name); + } absl::StatusOr> FindStructTypeFieldByNameImpl( - absl::string_view type, absl::string_view name) const override; + absl::string_view type, absl::string_view name) const override { + return runtime_type_provider_->FindStructTypeFieldByName(type, name); + } + + private: + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; + const RuntimeTypeProvider* absl_nonnull runtime_type_provider_; }; } // namespace cel::runtime_internal diff --git a/runtime/internal/legacy_runtime_type_provider_test.cc b/runtime/internal/legacy_runtime_type_provider_test.cc index eceac65a9..bd3caddc4 100644 --- a/runtime/internal/legacy_runtime_type_provider_test.cc +++ b/runtime/internal/legacy_runtime_type_provider_test.cc @@ -22,6 +22,7 @@ #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" #include "internal/testing_message_factory.h" +#include "runtime/internal/runtime_type_provider.h" #include "cel/expr/conformance/proto3/test_all_types.pb.h" #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" @@ -33,8 +34,9 @@ namespace { using ::cel::expr::conformance::proto3::TestAllTypes; TEST(LegacyRuntimeTypeProviderTest, FindType) { + RuntimeTypeProvider type_provider(cel::internal::GetTestingDescriptorPool()); LegacyRuntimeTypeProvider provider(cel::internal::GetTestingDescriptorPool(), - cel::internal::GetTestingMessageFactory()); + &type_provider); ASSERT_OK_AND_ASSIGN(std::optional wrapper_type, provider.FindType("google.protobuf.Int64Value")); ASSERT_TRUE(wrapper_type.has_value()); @@ -50,18 +52,18 @@ TEST(LegacyRuntimeTypeProviderTest, FindType) { } TEST(LegacyRuntimeTypeProviderTest, FindTypeNotFound) { - LegacyRuntimeTypeProvider provider( - google::protobuf::DescriptorPool::generated_pool(), - google::protobuf::MessageFactory::generated_factory()); + RuntimeTypeProvider type_provider(google::protobuf::DescriptorPool::generated_pool()); + LegacyRuntimeTypeProvider provider(google::protobuf::DescriptorPool::generated_pool(), + &type_provider); ASSERT_OK_AND_ASSIGN(std::optional type, provider.FindType("UnknownType")); EXPECT_FALSE(type.has_value()); } TEST(LegacyRuntimeTypeProviderTest, FindStructTypeFieldByName) { - LegacyRuntimeTypeProvider provider( - google::protobuf::DescriptorPool::generated_pool(), - google::protobuf::MessageFactory::generated_factory()); + RuntimeTypeProvider type_provider(google::protobuf::DescriptorPool::generated_pool()); + LegacyRuntimeTypeProvider provider(google::protobuf::DescriptorPool::generated_pool(), + &type_provider); ASSERT_OK_AND_ASSIGN(std::optional field, provider.FindStructTypeFieldByName( "google.protobuf.Int64Value", "value")); @@ -72,9 +74,9 @@ TEST(LegacyRuntimeTypeProviderTest, FindStructTypeFieldByName) { } TEST(LegacyRuntimeTypeProviderTest, FindStructTypeFieldByNameNotFound) { - LegacyRuntimeTypeProvider provider( - google::protobuf::DescriptorPool::generated_pool(), - google::protobuf::MessageFactory::generated_factory()); + RuntimeTypeProvider type_provider(google::protobuf::DescriptorPool::generated_pool()); + LegacyRuntimeTypeProvider provider(google::protobuf::DescriptorPool::generated_pool(), + &type_provider); ASSERT_OK_AND_ASSIGN(std::optional field, provider.FindStructTypeFieldByName( "google.protobuf.Int64Value", "unknown_field")); @@ -86,9 +88,10 @@ TEST(LegacyRuntimeTypeProviderTest, FindStructTypeFieldByNameNotFound) { EXPECT_FALSE(field2.has_value()); } -TEST(LegacyRuntimeTypeProviderTest, NewValueBuilder) { +TEST(LegacyRuntimeTypeProviderTest, NewValueBuilderMessage) { + RuntimeTypeProvider type_provider(cel::internal::GetTestingDescriptorPool()); LegacyRuntimeTypeProvider provider(cel::internal::GetTestingDescriptorPool(), - cel::internal::GetTestingMessageFactory()); + &type_provider); google::protobuf::Arena arena; ASSERT_OK_AND_ASSIGN(auto builder, provider.NewValueBuilder( @@ -100,14 +103,38 @@ TEST(LegacyRuntimeTypeProviderTest, NewValueBuilder) { builder->SetFieldByName("single_int64", IntValue(42))); EXPECT_FALSE(field_result.has_value()); + ASSERT_OK_AND_ASSIGN(auto field_result2, + builder->SetFieldByNumber(1, IntValue(100))); + EXPECT_FALSE(field_result2.has_value()); + ASSERT_OK_AND_ASSIGN(auto value, std::move(*builder).Build()); EXPECT_TRUE(value.Is()); } +TEST(LegacyRuntimeTypeProviderTest, NewValueBuilderWellKnownType) { + RuntimeTypeProvider type_provider(cel::internal::GetTestingDescriptorPool()); + LegacyRuntimeTypeProvider provider(cel::internal::GetTestingDescriptorPool(), + &type_provider); + google::protobuf::Arena arena; + ASSERT_OK_AND_ASSIGN(auto builder, + provider.NewValueBuilder( + "google.protobuf.Int64Value", + cel::internal::GetTestingMessageFactory(), &arena)); + ASSERT_NE(builder, nullptr); + + ASSERT_OK_AND_ASSIGN(auto field_result, + builder->SetFieldByName("value", IntValue(42))); + EXPECT_FALSE(field_result.has_value()); + + ASSERT_OK_AND_ASSIGN(auto value, std::move(*builder).Build()); + ASSERT_TRUE(value.Is()); + EXPECT_EQ(value.As()->NativeValue(), 42); +} + TEST(LegacyRuntimeTypeProviderTest, NewValueBuilderNotFound) { - LegacyRuntimeTypeProvider provider( - google::protobuf::DescriptorPool::generated_pool(), - google::protobuf::MessageFactory::generated_factory()); + RuntimeTypeProvider type_provider(google::protobuf::DescriptorPool::generated_pool()); + LegacyRuntimeTypeProvider provider(google::protobuf::DescriptorPool::generated_pool(), + &type_provider); google::protobuf::LinkMessageReflection(); google::protobuf::Arena arena; ASSERT_OK_AND_ASSIGN( diff --git a/runtime/type_registry.cc b/runtime/type_registry.cc index a1e8b0328..58bce8afd 100644 --- a/runtime/type_registry.cc +++ b/runtime/type_registry.cc @@ -35,9 +35,7 @@ TypeRegistry::TypeRegistry( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nullable message_factory) : type_provider_(descriptor_pool), - legacy_type_provider_( - std::make_shared( - descriptor_pool, message_factory)) { + legacy_type_provider_(descriptor_pool, &type_provider_) { RegisterEnum("google.protobuf.NullValue", {{"NULL_VALUE", 0}}); } diff --git a/runtime/type_registry.h b/runtime/type_registry.h index eadd1f1ea..22f923bd6 100644 --- a/runtime/type_registry.h +++ b/runtime/type_registry.h @@ -41,8 +41,8 @@ class TypeRegistry; namespace runtime_internal { const RuntimeTypeProvider& GetRuntimeTypeProvider( const TypeRegistry& type_registry); -const absl_nonnull std::shared_ptr& -GetLegacyRuntimeTypeProvider(const TypeRegistry& type_registry); +const LegacyRuntimeTypeProvider& GetLegacyRuntimeTypeProvider( + const TypeRegistry& type_registry); // Returns a memoized table of fully qualified enum values. // @@ -104,10 +104,9 @@ class TypeRegistry { private: friend const runtime_internal::RuntimeTypeProvider& runtime_internal::GetRuntimeTypeProvider(const TypeRegistry& type_registry); - friend const - absl_nonnull std::shared_ptr& - runtime_internal::GetLegacyRuntimeTypeProvider( - const TypeRegistry& type_registry); + friend const runtime_internal::LegacyRuntimeTypeProvider& + runtime_internal::GetLegacyRuntimeTypeProvider( + const TypeRegistry& type_registry); friend std::shared_ptr> runtime_internal::GetEnumValueTable(const TypeRegistry& type_registry); @@ -116,8 +115,7 @@ class TypeRegistry { GetEnumValueTable() const; runtime_internal::RuntimeTypeProvider type_provider_; - absl_nonnull std::shared_ptr - legacy_type_provider_; + runtime_internal::LegacyRuntimeTypeProvider legacy_type_provider_; absl::flat_hash_map enum_types_; // memoized fully qualified enumerator names. @@ -139,8 +137,8 @@ inline const RuntimeTypeProvider& GetRuntimeTypeProvider( const TypeRegistry& type_registry) { return type_registry.type_provider_; } -inline const absl_nonnull std::shared_ptr& -GetLegacyRuntimeTypeProvider(const TypeRegistry& type_registry) { +inline const LegacyRuntimeTypeProvider& GetLegacyRuntimeTypeProvider( + const TypeRegistry& type_registry) { return type_registry.legacy_type_provider_; } inline std::shared_ptr>