Skip to content

Update field accesses to use the modern impl for scalars. #2216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
copybara-service wants to merge 1 commit into from
Closed

Update field accesses to use the modern impl for scalars. #2216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 64 additions & 9 deletions common/legacy_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<google::protobuf::Message>(
message_wrapper.message_ptr()),
message_wrapper.legacy_type_info());
Expand Down Expand Up @@ -1153,7 +1208,7 @@ absl::StatusOr<Value> 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<google::protobuf::Message>(
message_wrapper.message_ptr()),
message_wrapper.legacy_type_info());
Expand Down
14 changes: 14 additions & 0 deletions common/value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion common/values/parsed_message_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 8 additions & 3 deletions common/values/parsed_message_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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_;
};

Expand Down
64 changes: 33 additions & 31 deletions eval/eval/select_step_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ class SelectStepTest : public testing::Test {
// Helper method. Creates simple pipeline containing Select step and runs it.
absl::StatusOr<CelValue> RunExpression(const CelValue target,
absl::string_view field, bool test,
absl::string_view unknown_path,
RunExpressionOptions options) {
ExecutionPath path;

Expand All @@ -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<void>(env_->MutableMessageFactory());
CelExpressionFlatImpl cel_expr(
env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0,
env_->type_registry.GetComposedTypeProvider(),
Expand All @@ -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<CelValue> 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<CelValue> RunExpression(const TestMessage* message,
absl::string_view field, bool test,
RunExpressionOptions options) {
return RunExpression(message, field, test, "", options);
field, test, options);
}

absl::StatusOr<CelValue> 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<CelValue> 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:
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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));
Expand Down
Loading