From 6ea636095895c3eb938a3a12da72ec3831a213d8 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 5 Aug 2026 11:16:06 -0700 Subject: [PATCH] Remove LegacyMutationApis This is now unused internally and no known references in other projects. PiperOrigin-RevId: 959765405 --- eval/eval/create_struct_step_test.cc | 3 +- 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 | 6 - .../internal/legacy_runtime_type_provider.cc | 95 ++----- .../internal/legacy_runtime_type_provider.h | 8 +- .../legacy_runtime_type_provider_test.cc | 25 +- 17 files changed, 78 insertions(+), 677 deletions(-) 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/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..7247630bf 100644 --- a/runtime/internal/BUILD +++ b/runtime/internal/BUILD @@ -185,20 +185,14 @@ cc_library( hdrs = ["legacy_runtime_type_provider.h"], deps = [ "//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", diff --git a/runtime/internal/legacy_runtime_type_provider.cc b/runtime/internal/legacy_runtime_type_provider.cc index 5db0b448e..f6f3c26ca 100644 --- a/runtime/internal/legacy_runtime_type_provider.cc +++ b/runtime/internal/legacy_runtime_type_provider.cc @@ -20,21 +20,15 @@ #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 "common/values/value_builder.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 "internal/status_macros.h" #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" @@ -44,61 +38,43 @@ 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,26 +84,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)); + return std::make_unique(arena, std::move(builder)); } absl::StatusOr> LegacyRuntimeTypeProvider::FindTypeImpl( @@ -175,12 +137,7 @@ LegacyRuntimeTypeProvider::FindStructTypeFieldByNameImpl( 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::nullopt; } } // namespace cel::runtime_internal diff --git a/runtime/internal/legacy_runtime_type_provider.h b/runtime/internal/legacy_runtime_type_provider.h index 45aef5d75..ca277f3ec 100644 --- a/runtime/internal/legacy_runtime_type_provider.h +++ b/runtime/internal/legacy_runtime_type_provider.h @@ -37,8 +37,9 @@ class LegacyRuntimeTypeProvider final 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) {} + : google::api::expr::runtime::ProtobufDescriptorProvider(descriptor_pool, + message_factory), + descriptor_pool_(descriptor_pool) {} absl::StatusOr NewValueBuilder( absl::string_view name, @@ -51,6 +52,9 @@ class LegacyRuntimeTypeProvider final absl::StatusOr> FindStructTypeFieldByNameImpl( absl::string_view type, absl::string_view name) const override; + + private: + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; }; } // 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..a83f11390 100644 --- a/runtime/internal/legacy_runtime_type_provider_test.cc +++ b/runtime/internal/legacy_runtime_type_provider_test.cc @@ -86,7 +86,7 @@ TEST(LegacyRuntimeTypeProviderTest, FindStructTypeFieldByNameNotFound) { EXPECT_FALSE(field2.has_value()); } -TEST(LegacyRuntimeTypeProviderTest, NewValueBuilder) { +TEST(LegacyRuntimeTypeProviderTest, NewValueBuilderMessage) { LegacyRuntimeTypeProvider provider(cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory()); google::protobuf::Arena arena; @@ -100,10 +100,33 @@ 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) { + LegacyRuntimeTypeProvider provider(cel::internal::GetTestingDescriptorPool(), + cel::internal::GetTestingMessageFactory()); + 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(),