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(),