diff --git a/common/legacy_value.cc b/common/legacy_value.cc index 184a09c0b..d84127d2d 100644 --- a/common/legacy_value.cc +++ b/common/legacy_value.cc @@ -352,6 +352,44 @@ CelValue LegacyTrivialMapValue(google::protobuf::Arena* absl_nonnull arena, value.GetRuntimeType().DebugString())))); } +bool FieldIsScalar(const google::protobuf::FieldDescriptor* absl_nonnull field) { + ABSL_DCHECK(field != nullptr); + if (field->is_repeated()) { + return false; + } + if (field->cpp_type() == google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) { + switch (field->message_type()->well_known_type()) { + case google::protobuf::Descriptor::WELLKNOWNTYPE_ANY: + case google::protobuf::Descriptor::WELLKNOWNTYPE_VALUE: + case google::protobuf::Descriptor::WELLKNOWNTYPE_LISTVALUE: + case google::protobuf::Descriptor::WELLKNOWNTYPE_STRUCT: + return false; + default: + return true; + } + } + return true; +} + +LegacyStructValue ParsedMessageToLegacyStructValue( + const ParsedMessageValue& parsed_message) { + return LegacyStructValue(cel::to_address(parsed_message), + &GetGenericProtoTypeInfoInstance()); +} + +LegacyStructValue MakeLegacyStructValue( + const google::protobuf::Message* absl_nonnull message, + const LegacyTypeInfoApis* legacy_type_info) { + // Guard against edge cases where a custom implementation of Message + // misbehaves. + // Modern value handles this with DCHECKs on value creation, legacy value + // would allow it and just report an ErrorValue on accesses. + if (message->GetReflection() == nullptr || legacy_type_info == nullptr) { + legacy_type_info = TrivialTypeInfo::GetInstance(); + } + return LegacyStructValue(message, legacy_type_info); +} + } // namespace google::api::expr::runtime::CelValue UnsafeLegacyValue( @@ -394,10 +432,6 @@ google::api::expr::runtime::CelValue UnsafeLegacyValue( } } -} // namespace common_internal - -namespace common_internal { - std::string LegacyListValue::DebugString() const { return CelValue::CreateList(impl_).DebugString(); } @@ -837,10 +871,8 @@ absl::Status LegacyStructValue::SerializeTo( ABSL_DCHECK(message_factory != nullptr); ABSL_DCHECK(output != nullptr); - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); if (ABSL_PREDICT_TRUE( - message_wrapper.message_ptr()->SerializePartialToZeroCopyStream( - output))) { + message_ptr_->SerializePartialToZeroCopyStream(output))) { return absl::OkStatus(); } return absl::UnknownError("failed to serialize protocol buffer message"); @@ -923,6 +955,29 @@ absl::Status LegacyStructValue::GetFieldByName( *result = NoSuchFieldError(name); return absl::OkStatus(); } + + ParsedMessageValue parsed_message = UnsafeParsedMessageValue(message_ptr_); + const auto* descriptor = parsed_message.GetDescriptor(); + const auto* field = descriptor->FindFieldByName(name); + if (field == nullptr) { + field = descriptor->file()->pool()->FindExtensionByPrintableName(descriptor, + name); + if (field == nullptr) { + *result = NoSuchFieldError(name); + return absl::OkStatus(); + } + } + + if (FieldIsScalar(field)) { + CEL_RETURN_IF_ERROR( + parsed_message.GetField(field, unboxing_options, descriptor_pool, + message_factory, arena, result)); + if (result->IsParsedMessage()) { + *result = ParsedMessageToLegacyStructValue(result->GetParsedMessage()); + } + return absl::OkStatus(); + } + CEL_ASSIGN_OR_RETURN(auto cel_value, GetGenericProtoAccessApisInstance().GetField( name, message_wrapper, unboxing_options, @@ -1035,7 +1090,7 @@ absl::Status ModernValue(google::protobuf::Arena* arena, return absl::OkStatus(); case CelValue::Type::kMessage: { auto message_wrapper = legacy_value.MessageWrapperOrDie(); - result = common_internal::LegacyStructValue( + result = common_internal::MakeLegacyStructValue( google::protobuf::DownCastMessage( message_wrapper.message_ptr()), message_wrapper.legacy_type_info()); @@ -1153,7 +1208,7 @@ absl::StatusOr FromLegacyValue(google::protobuf::Arena* arena, legacy_value.BytesOrDie().value()); case CelValue::Type::kMessage: { auto message_wrapper = legacy_value.MessageWrapperOrDie(); - return common_internal::LegacyStructValue( + return common_internal::MakeLegacyStructValue( google::protobuf::DownCastMessage( message_wrapper.message_ptr()), message_wrapper.legacy_type_info()); diff --git a/common/value.cc b/common/value.cc index fdb18d388..9ea8ec891 100644 --- a/common/value.cc +++ b/common/value.cc @@ -1512,6 +1512,13 @@ Value WrapFieldImpl( ABSL_DCHECK(!IsWellKnownMessageType(message->GetDescriptor())); const auto* reflection = message->GetReflection(); + if (ABSL_PREDICT_FALSE(reflection == nullptr)) { + // This only happens for special implementations of Message that + // should not normally be used with CEL. + return ErrorValue(absl::InvalidArgumentError( + absl::StrCat("failed to get reflection for message type: ", + message->GetDescriptor()->full_name()))); + } if (field->is_map()) { if (reflection->FieldSize(*message, field) == 0) { return MapValue(); @@ -1653,6 +1660,13 @@ Value WrapRepeatedFieldImpl( ABSL_DCHECK(arena != nullptr); const auto* reflection = message->GetReflection(); + if (ABSL_PREDICT_FALSE(reflection == nullptr)) { + // This only happens for special implementations of Message that + // should not normally be used with CEL. + return ErrorValue(absl::InvalidArgumentError( + absl::StrCat("failed to get reflection for message type: ", + message->GetDescriptor()->full_name()))); + } const int size = reflection->FieldSize(*message, field); if (ABSL_PREDICT_FALSE(index < 0 || index >= size)) { return ErrorValue(absl::InvalidArgumentError( diff --git a/common/values/parsed_message_value.cc b/common/values/parsed_message_value.cc index b0e881998..03d2d461d 100644 --- a/common/values/parsed_message_value.cc +++ b/common/values/parsed_message_value.cc @@ -382,7 +382,7 @@ absl::Status ParsedMessageValue::GetField( ABSL_DCHECK(arena != nullptr); ABSL_DCHECK(result != nullptr); - if (arena_ == nullptr) { + if (is_unsafe()) { *result = Value::WrapFieldUnsafe(unboxing_options, value_, field, descriptor_pool, message_factory, arena); } else { diff --git a/common/values/parsed_message_value.h b/common/values/parsed_message_value.h index 2e356d3e8..3cad912e7 100644 --- a/common/values/parsed_message_value.h +++ b/common/values/parsed_message_value.h @@ -192,7 +192,7 @@ class ParsedMessageValue final explicit ParsedMessageValue( const google::protobuf::Message* absl_nonnull value ABSL_ATTRIBUTE_LIFETIME_BOUND) - : value_(value), arena_(value->GetArena()) { + : value_(value), arena_(nullptr) { ABSL_DCHECK(value != nullptr); ABSL_DCHECK(!value_ || !IsWellKnownMessageType(value_->GetDescriptor())) << value_->GetTypeName() << " is a well known type"; @@ -210,9 +210,14 @@ class ParsedMessageValue final return absl::OkStatus(); } + bool is_unsafe() const { return arena_ == nullptr; } + const google::protobuf::Message* absl_nonnull value_; - // Arena that is attributed as owning the value. May be null to indicate that - // the value is managed externally. + + // The arena attributed as Owning this value. Null if the value is created by + // UnsafeParsedMessageValue() or derived from such a value. This is used to + // identify externally managed messages and propagating the unsafe field + // access behavior. google::protobuf::Arena* absl_nullable arena_; }; diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index 494def40f..5a0d1dfba 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -92,7 +92,6 @@ class SelectStepTest : public testing::Test { // Helper method. Creates simple pipeline containing Select step and runs it. absl::StatusOr RunExpression(const CelValue target, absl::string_view field, bool test, - absl::string_view unknown_path, RunExpressionOptions options) { ExecutionPath path; @@ -118,6 +117,8 @@ class SelectStepTest : public testing::Test { runtime_options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } + // Force the creation of a message factory at the env level. + static_cast(env_->MutableMessageFactory()); CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env_->type_registry.GetComposedTypeProvider(), @@ -132,35 +133,20 @@ class SelectStepTest : public testing::Test { absl::string_view field, bool test, RunExpressionOptions options) { return RunExpression(CelProtoWrapper::CreateMessage(message, &arena_), - field, test, "", options); + field, test, options); } absl::StatusOr RunExpression(const TestMessage* message, absl::string_view field, bool test, - absl::string_view unknown_path, RunExpressionOptions options) { return RunExpression(CelProtoWrapper::CreateMessage(message, &arena_), - field, test, unknown_path, options); - } - - absl::StatusOr RunExpression(const TestMessage* message, - absl::string_view field, bool test, - RunExpressionOptions options) { - return RunExpression(message, field, test, "", options); + field, test, options); } absl::StatusOr RunExpression(const CelMap* map_value, absl::string_view field, bool test, - absl::string_view unknown_path, RunExpressionOptions options) { - return RunExpression(CelValue::CreateMap(map_value), field, test, - unknown_path, options); - } - - absl::StatusOr RunExpression(const CelMap* map_value, - absl::string_view field, bool test, - RunExpressionOptions options) { - return RunExpression(map_value, field, test, "", options); + return RunExpression(CelValue::CreateMap(map_value), field, test, options); } protected: @@ -189,8 +175,7 @@ TEST_P(SelectStepConformanceTest, SelectTargetNotStructOrMap) { ASSERT_OK_AND_ASSIGN( CelValue result, RunExpression(CelValue::CreateStringView("some_value"), "some_field", - /*test=*/false, - /*unknown_path=*/"", options)); + /*test=*/false, options)); ASSERT_TRUE(result.IsError()); EXPECT_THAT(*result.ErrorOrDie(), @@ -547,17 +532,37 @@ TEST_P(SelectStepConformanceTest, GlobalExtensionsMessageTest) { } TEST_P(SelectStepConformanceTest, GlobalExtensionsMessageUnsetTest) { - TestExtensions exts; + // Implementation details: + // The test environment is a dynamic descriptor pool with the same definition + // as the linked proto. + // Use a dynamic message with the expected factory and pool. Otherwise, + // we can end up in a state where we're comparing messages with different + // prototypes. + const google::protobuf::Descriptor* descriptor = + env_->descriptor_pool->FindMessageTypeByName( + TestExtensions::descriptor()->full_name()); + ASSERT_NE(descriptor, nullptr); + const google::protobuf::FieldDescriptor* field = + env_->descriptor_pool->FindExtensionByPrintableName( + descriptor, "google.api.expr.runtime.nested_ext"); + ASSERT_NE(field, nullptr); + ASSERT_TRUE(field->containing_type() == descriptor); + const auto* prototype = + env_->MutableMessageFactory()->GetPrototype(descriptor); + ASSERT_NE(prototype, nullptr); + const auto* msg_default = &prototype->GetReflection()->GetMessage( + *prototype, field, env_->MutableMessageFactory()); + RunExpressionOptions options; options.enable_unknowns = GetParam(); ASSERT_OK_AND_ASSIGN( CelValue result, - RunExpression(&exts, "google.api.expr.runtime.nested_ext", false, - options)); + RunExpression(CelProtoWrapper::CreateMessage(prototype, &arena_), + "google.api.expr.runtime.nested_ext", false, options)); ASSERT_TRUE(result.IsMessage()); - EXPECT_THAT(result.MessageOrDie(), Eq(&TestExtensions::default_instance())); + EXPECT_THAT(result.MessageOrDie(), Eq(msg_default)); } TEST_P(SelectStepConformanceTest, GlobalExtensionsWrapperTest) { @@ -653,18 +658,15 @@ TEST_P(SelectStepConformanceTest, NullMessageAccessor) { CelValue value = CelValue::CreateMessageWrapper( CelValue::MessageWrapper(&message, TrivialTypeInfo::GetInstance())); - ASSERT_OK_AND_ASSIGN(CelValue result, - RunExpression(value, "message_value", - /*test=*/false, - /*unknown_path=*/"", options)); + ASSERT_OK_AND_ASSIGN(CelValue result, RunExpression(value, "message_value", + /*test=*/false, options)); ASSERT_TRUE(result.IsError()); EXPECT_THAT(*result.ErrorOrDie(), StatusIs(absl::StatusCode::kNotFound)); // same for has ASSERT_OK_AND_ASSIGN(result, RunExpression(value, "message_value", - /*test=*/true, - /*unknown_path=*/"", options)); + /*test=*/true, options)); ASSERT_TRUE(result.IsError()); EXPECT_THAT(*result.ErrorOrDie(), StatusIs(absl::StatusCode::kNotFound));