From ad9ebbe3c14602c79debda5b1906562c51fd8b27 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 12 Aug 2026 16:44:07 -0700 Subject: [PATCH] Migrate low-touch cel::LegacyStructValue member functions to use the modern equivalent implementations. These members should be safer than the getters (either non-allocating or not a hot path). PiperOrigin-RevId: 963730307 --- common/legacy_value.cc | 103 ++++++++---------- eval/public/structs/BUILD | 1 + .../proto_message_type_adapter_test.cc | 9 +- 3 files changed, 47 insertions(+), 66 deletions(-) diff --git a/common/legacy_value.cc b/common/legacy_value.cc index 3f3de6d85..184a09c0b 100644 --- a/common/legacy_value.cc +++ b/common/legacy_value.cc @@ -850,34 +850,30 @@ absl::Status LegacyStructValue::ConvertToJson( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Message* absl_nonnull json) const { + ABSL_DCHECK(message_ptr_ != nullptr); ABSL_DCHECK(descriptor_pool != nullptr); ABSL_DCHECK(message_factory != nullptr); ABSL_DCHECK(json != nullptr); ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(), google::protobuf::Descriptor::WELLKNOWNTYPE_VALUE); - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - - return internal::MessageToJson( - *google::protobuf::DownCastMessage(message_wrapper.message_ptr()), - descriptor_pool, message_factory, json); + return internal::MessageToJson(*message_ptr_, descriptor_pool, + message_factory, json); } absl::Status LegacyStructValue::ConvertToJsonObject( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Message* absl_nonnull json) const { + ABSL_DCHECK(message_ptr_ != nullptr); ABSL_DCHECK(descriptor_pool != nullptr); ABSL_DCHECK(message_factory != nullptr); ABSL_DCHECK(json != nullptr); ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(), google::protobuf::Descriptor::WELLKNOWNTYPE_STRUCT); - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - - return internal::MessageToJson( - *google::protobuf::DownCastMessage(message_wrapper.message_ptr()), - descriptor_pool, message_factory, json); + return internal::MessageToJson(*message_ptr_, descriptor_pool, + message_factory, json); } absl::Status LegacyStructValue::Equal( @@ -885,38 +881,36 @@ absl::Status LegacyStructValue::Equal( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const { - if (auto legacy_struct_value = common_internal::AsLegacyStructValue(other); - legacy_struct_value.has_value()) { - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - if (ABSL_PREDICT_FALSE(legacy_type_info_ == - TrivialTypeInfo::GetInstance())) { - return absl::UnimplementedError( - absl::StrCat("legacy access APIs missing for ", GetTypeName())); - } - auto other_message_wrapper = - AsMessageWrapper(legacy_struct_value->message_ptr(), - legacy_struct_value->legacy_type_info()); - *result = BoolValue{GetGenericProtoAccessApisInstance().IsEqualTo( - message_wrapper, other_message_wrapper)}; - return absl::OkStatus(); + ABSL_DCHECK(message_ptr_ != nullptr); + if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { + return absl::UnimplementedError( + absl::StrCat("legacy access APIs missing for ", GetTypeName())); } - if (auto struct_value = other.AsStruct(); struct_value.has_value()) { - return common_internal::StructValueEqual( - common_internal::LegacyStructValue(message_ptr_, legacy_type_info_), - *struct_value, descriptor_pool, message_factory, arena, result); + auto modern_value = UnsafeParsedMessageValue(message_ptr_); + + // Unwrap the rhs if it's a legacy struct so the normal implementation + // doesn't hit the fallback abstract struct comparison. + if (auto other_legacy = common_internal::AsLegacyStructValue(other); + other_legacy) { + if (other_legacy->legacy_type_info_ == TrivialTypeInfo::GetInstance()) { + return absl::UnimplementedError(absl::StrCat( + "legacy access APIs missing for ", other_legacy->GetTypeName())); + } + auto other_message = UnsafeParsedMessageValue(other_legacy->message_ptr_); + return modern_value.Equal(other_message, descriptor_pool, message_factory, + arena, result); } - *result = FalseValue(); - return absl::OkStatus(); + + return modern_value.Equal(other, descriptor_pool, message_factory, arena, + result); } bool LegacyStructValue::IsZeroValue() const { - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); + ABSL_DCHECK(message_ptr_ != nullptr); if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { return false; } - return GetGenericProtoAccessApisInstance() - .ListFields(message_wrapper) - .empty(); + return UnsafeParsedMessageValue(message_ptr_).IsZeroValue(); } absl::Status LegacyStructValue::GetFieldByName( @@ -948,16 +942,19 @@ absl::Status LegacyStructValue::GetFieldByNumber( absl::StatusOr LegacyStructValue::HasFieldByName( absl::string_view name) const { - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); + ABSL_DCHECK(message_ptr_ != nullptr); if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { - return NoSuchFieldError(name).NativeValue(); + return NoSuchFieldError(name).ToStatus(); } - return GetGenericProtoAccessApisInstance().HasField(name, message_wrapper); + return UnsafeParsedMessageValue(message_ptr_).HasFieldByName(name); } absl::StatusOr LegacyStructValue::HasFieldByNumber(int64_t number) const { - return absl::UnimplementedError( - "access to fields by numbers is not available for legacy structs"); + ABSL_DCHECK(message_ptr_ != nullptr); + if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { + return NoSuchFieldError(absl::StrCat(number)).ToStatus(); + } + return UnsafeParsedMessageValue(message_ptr_).HasFieldByNumber(number); } absl::Status LegacyStructValue::ForEachField( @@ -965,27 +962,13 @@ absl::Status LegacyStructValue::ForEachField( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) const { - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); + ABSL_DCHECK(message_ptr_ != nullptr); if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { return absl::UnimplementedError( absl::StrCat("legacy access APIs missing for ", GetTypeName())); } - const auto& access_apis = GetGenericProtoAccessApisInstance(); - auto field_names = access_apis.ListFields(message_wrapper); - Value value; - for (const auto& field_name : field_names) { - CEL_ASSIGN_OR_RETURN( - auto cel_value, - access_apis.GetField(field_name, message_wrapper, - ProtoWrapperTypeOptions::kUnsetNull, - MemoryManagerRef::Pooling(arena))); - CEL_RETURN_IF_ERROR(ModernValue(arena, cel_value, value)); - CEL_ASSIGN_OR_RETURN(auto ok, callback(field_name, value)); - if (!ok) { - break; - } - } - return absl::OkStatus(); + return UnsafeParsedMessageValue(message_ptr_) + .ForEachField(callback, descriptor_pool, message_factory, arena); } absl::Status LegacyStructValue::Qualify( @@ -1291,12 +1274,12 @@ const google::protobuf::Message* absl_nullable GetLegacyMessage(const Value& val auto legacy = common_internal::GetLegacyStructValue(value); const auto* legacy_type_info = legacy.legacy_type_info(); - if (legacy_type_info == nullptr) { - return nullptr; - } - if (legacy_type_info != &GetGenericProtoTypeInfoInstance()) { + if (legacy_type_info == nullptr || + legacy_type_info == TrivialTypeInfo::GetInstance()) { return nullptr; } + // This should not be possible using the normal public APIs, but possible + // if someone used the MessageWrapper class directly. if (IsWellKnownMessageType(legacy.message_ptr()->GetDescriptor())) { return nullptr; } diff --git a/eval/public/structs/BUILD b/eval/public/structs/BUILD index 992651b8b..468867294 100644 --- a/eval/public/structs/BUILD +++ b/eval/public/structs/BUILD @@ -305,6 +305,7 @@ cc_test( ":legacy_type_adapter", ":legacy_type_info_apis", ":proto_message_type_adapter", + ":trivial_legacy_type_info", "//base:attributes", "//common:value", "//common:value_testing", diff --git a/eval/public/structs/proto_message_type_adapter_test.cc b/eval/public/structs/proto_message_type_adapter_test.cc index a74a32cb4..529052025 100644 --- a/eval/public/structs/proto_message_type_adapter_test.cc +++ b/eval/public/structs/proto_message_type_adapter_test.cc @@ -28,6 +28,7 @@ #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/trivial_legacy_type_info.h" #include "eval/public/testing/matchers.h" #include "eval/testutil/test_message.pb.h" #include "extensions/protobuf/memory_manager.h" @@ -1147,17 +1148,13 @@ TEST(ProtoMesssageTypeAdapter, QualifyMapIndexLeafWrongType) { HasSubstr("Invalid map key type")))))); } -TEST(ProtoMesssageTypeAdapter, InteropUnwrappingNotGeneric) { +TEST(ProtoMesssageTypeAdapter, InteropUnwrappingNotTestInstance) { google::protobuf::Arena arena; - ProtoMessageTypeAdapter adapter( - google::protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( - "google.api.expr.runtime.TestMessage"), - google::protobuf::MessageFactory::generated_factory()); TestMessage message; message.set_string_value("hello"); auto legacy_value = CelValue::CreateMessageWrapper( - CelValue::MessageWrapper(&message, &adapter)); + CelValue::MessageWrapper(&message, TrivialTypeInfo::GetInstance())); cel::Value modern_value; ASSERT_THAT(cel::ModernValue(&arena, legacy_value, modern_value), IsOk()); auto unwrapped = cel::interop_internal::GetLegacyMessage(modern_value);