Skip to content
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

Jump to bottom

refactor grpc: fix clang-tidy and improve field mask wildcard handling #743

Closed
wants to merge 6 commits into from
Closed

refactor grpc: fix clang-tidy and improve field mask wildcard handling #743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12326be
fix clang-tidy
TTPO100AJIEX Nov 1, 2024
63de67b
improve masks
TTPO100AJIEX Nov 1, 2024
20238e0
Update field_mask_test.cpp
TTPO100AJIEX Nov 1, 2024
87c479a
Update field_mask_test.cpp
TTPO100AJIEX Nov 1, 2024
89b03c5
Update field_mask.cpp
TTPO100AJIEX Nov 1, 2024
a24d9c6
Update field_mask.cpp
TTPO100AJIEX Nov 1, 2024
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
35 changes: 13 additions & 22 deletions grpc/src/ugrpc/field_mask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ google::protobuf::FieldMask RawFromString(std::string_view string, FieldMask::En
}

std::string RawToString(const google::protobuf::FieldMask& field_mask, FieldMask::Encoding encoding) {
const auto comma_separated = google::protobuf::util::FieldMaskUtil::ToString(field_mask);
auto comma_separated = google::protobuf::util::FieldMaskUtil::ToString(field_mask);
if (encoding == FieldMask::Encoding::kCommaSeparated) return comma_separated;
UINVARIANT(encoding == FieldMask::Encoding::kWebSafeBase64, "Unknown encoding");

Expand Down Expand Up @@ -250,17 +250,15 @@ void FieldMask::CheckValidity(const google::protobuf::Descriptor* descriptor) co
bool FieldMask::IsPathFullyIn(std::string_view path) const {
if (path.empty() || IsLeaf()) return IsLeaf();
const Part root = GetRoot(path);
const auto it = utils::impl::FindTransparent(*children_, root.part);
if (it == children_->end()) return false;
return it->second.IsPathFullyIn(path.substr(root.used_symbols));
const utils::OptionalRef<const ugrpc::FieldMask> child = GetMaskForField(root.part);
return child.has_value() ? child->IsPathFullyIn(path.substr(root.used_symbols)) : false;
}

bool FieldMask::IsPathPartiallyIn(std::string_view path) const {
if (path.empty() || IsLeaf()) return true;
const Part root = GetRoot(path);
const auto it = utils::impl::FindTransparent(*children_, root.part);
if (it == children_->end()) return false;
return it->second.IsPathPartiallyIn(path.substr(root.used_symbols));
const utils::OptionalRef<const ugrpc::FieldMask> child = GetMaskForField(root.part);
return child.has_value() ? child->IsPathPartiallyIn(path.substr(root.used_symbols)) : false;
}

void FieldMask::Trim(google::protobuf::Message& message) const {
Expand All @@ -278,13 +276,13 @@ void FieldMask::TrimNoValidate(google::protobuf::Message& message) const {
UINVARIANT(reflection, "reflection is nullptr");

VisitFields(message, [&](google::protobuf::Message&, const google::protobuf::FieldDescriptor& field) {
const auto it = utils::impl::FindTransparent(*children_, field.name());
if (it == children_->end()) {
const utils::OptionalRef<const ugrpc::FieldMask> nested_mask_ref = GetMaskForField(field.name());
if (!nested_mask_ref.has_value()) {
// The field is not in the field mask. Remove it.
return reflection->ClearField(&message, &field);
}

const FieldMask& nested_mask = it->second;
const FieldMask& nested_mask = nested_mask_ref.value();
if (nested_mask.IsLeaf()) {
// The field must not be masked
return;
Expand All @@ -293,18 +291,14 @@ void FieldMask::TrimNoValidate(google::protobuf::Message& message) const {
if (field.is_map()) {
const google::protobuf::MutableRepeatedFieldRef<google::protobuf::Message> map =
reflection->GetMutableRepeatedFieldRef<google::protobuf::Message>(&message, &field);

const auto star_mask_it = nested_mask.children_->find("*");
for (int i = 0; i < map.size(); ++i) {
google::protobuf::Message* entry = reflection->MutableRepeatedMessage(&message, &field, i);
UINVARIANT(entry, "entry is nullptr");

const std::string key = GetMapKeyAsString(*entry);
const auto value_mask_it = utils::impl::FindTransparent(*nested_mask.children_, key);
const utils::OptionalRef<const ugrpc::FieldMask> value_mask_ref = nested_mask.GetMaskForField(key);

const auto& actual_mask_it =
value_mask_it != nested_mask.children_->end() ? value_mask_it : star_mask_it;
if (actual_mask_it == nested_mask.children_->end()) {
if (!value_mask_ref.has_value()) {
// The map key is not in the field mask.
// Remove the record by putting it to the back of the array.
//
Expand All @@ -315,8 +309,7 @@ void FieldMask::TrimNoValidate(google::protobuf::Message& message) const {
continue;
}

const FieldMask& actual_mask = actual_mask_it->second;
if (actual_mask.IsLeaf()) continue;
if (value_mask_ref->IsLeaf()) continue;

// The map key has a mask for the value
const google::protobuf::Descriptor* entry_desc = field.message_type();
Expand All @@ -331,7 +324,7 @@ void FieldMask::TrimNoValidate(google::protobuf::Message& message) const {

google::protobuf::Message* value_msg = entry_reflection->MutableMessage(entry, map_value);
UINVARIANT(value_msg, "value_msg is nullptr");
actual_mask.TrimNoValidate(*value_msg);
value_mask_ref->TrimNoValidate(*value_msg);
}
} else if (field.is_repeated()) {
constexpr std::string_view kBadRepeatedMask = "A non-leaf field mask for an array can contain only *";
Expand Down Expand Up @@ -365,9 +358,7 @@ std::vector<std::string_view> FieldMask::GetFieldNamesList() const {
return std::vector<std::string_view>(view.cbegin(), view.cend());
}

bool FieldMask::HasFieldName(std::string_view field) const {
return IsLeaf() || utils::impl::FindTransparent(*children_, field) != children_->end();
}
bool FieldMask::HasFieldName(std::string_view field) const { return GetMaskForField(field).has_value(); }

utils::OptionalRef<const FieldMask> FieldMask::GetMaskForField(std::string_view field) const {
if (IsLeaf()) {
Expand Down
75 changes: 36 additions & 39 deletions grpc/tests/field_mask_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,6 @@ google::protobuf::FieldMask MakeGoogleFieldMask(const std::vector<std::string>&
return field_mask;
}

sample::ugrpc::MessageWithDifferentTypes::NestedMessage* ConstructNestedMessagePtr() {
auto* message = new sample::ugrpc::MessageWithDifferentTypes::NestedMessage();
message->set_required_string("string1");
message->set_optional_string("string2");
message->set_required_int(123321);
message->set_optional_int(456654);
return message;
}

sample::ugrpc::MessageWithDifferentTypes::NestedMessage ConstructNestedMessage() {
sample::ugrpc::MessageWithDifferentTypes::NestedMessage message;
message.set_required_string("string1");
Expand All @@ -143,47 +134,47 @@ sample::ugrpc::MessageWithDifferentTypes::NestedMessage ConstructNestedMessage()
return message;
}

sample::ugrpc::MessageWithDifferentTypes* ConstructMessage(bool with_recursive = true) {
auto* message = new sample::ugrpc::MessageWithDifferentTypes();
sample::ugrpc::MessageWithDifferentTypes ConstructMessage(bool with_recursive = true) {
sample::ugrpc::MessageWithDifferentTypes message;

// leave required_string empty
message->set_optional_string("string2");
message.set_optional_string("string2");

message->set_required_int(123321);
message->set_optional_int(456654);
message.set_required_int(123321);
message.set_optional_int(456654);

message->set_allocated_required_nested(ConstructNestedMessagePtr());
message->set_allocated_optional_nested(ConstructNestedMessagePtr());
*message.mutable_required_nested() = ConstructNestedMessage();
*message.mutable_optional_nested() = ConstructNestedMessage();

if (with_recursive) {
message->set_allocated_required_recursive(ConstructMessage(false));
message->set_allocated_optional_recursive(ConstructMessage(false));
*message.mutable_required_recursive() = ConstructMessage(false);
*message.mutable_optional_recursive() = ConstructMessage(false);
}

message->add_repeated_primitive("string1");
message->add_repeated_primitive("string2");
message->add_repeated_primitive("string3");
message.add_repeated_primitive("string1");
message.add_repeated_primitive("string2");
message.add_repeated_primitive("string3");

message->mutable_repeated_message()->Add(ConstructNestedMessage());
message->mutable_repeated_message()->Add(ConstructNestedMessage());
message->mutable_repeated_message()->Add(ConstructNestedMessage());
message.mutable_repeated_message()->Add(ConstructNestedMessage());
message.mutable_repeated_message()->Add(ConstructNestedMessage());
message.mutable_repeated_message()->Add(ConstructNestedMessage());

// No key1
message->mutable_primitives_map()->insert({"key2", "value2"});
message->mutable_primitives_map()->insert({"key3", "value3"});
message.mutable_primitives_map()->insert({"key2", "value2"});
message.mutable_primitives_map()->insert({"key3", "value3"});

message->mutable_nested_map()->insert({"key1", ConstructNestedMessage()});
message->mutable_nested_map()->insert({"key2", ConstructNestedMessage()});
message->mutable_nested_map()->insert({"key3", ConstructNestedMessage()});
message->mutable_nested_map()->insert({"key1 value1", ConstructNestedMessage()});
message.mutable_nested_map()->insert({"key1", ConstructNestedMessage()});
message.mutable_nested_map()->insert({"key2", ConstructNestedMessage()});
message.mutable_nested_map()->insert({"key3", ConstructNestedMessage()});
message.mutable_nested_map()->insert({"key1 value1", ConstructNestedMessage()});
// No key2.value2
message->mutable_nested_map()->insert({"key3.value3 field3", ConstructNestedMessage()});
message.mutable_nested_map()->insert({"key3.value3 field3", ConstructNestedMessage()});

// leave oneof_string empty
message->set_oneof_int(789987);
message.set_oneof_int(789987);
// leave oneof_nested empty

message->mutable_google_value()->set_string_value("string");
message.mutable_google_value()->set_string_value("string");

return message;
}
Expand Down Expand Up @@ -751,6 +742,9 @@ TEST(FieldMaskIsPathFullyIn, MockFieldMask) {
EXPECT_TRUE(field_mask.IsPathFullyIn("root9.*.field.subfield2"));
EXPECT_FALSE(field_mask.IsPathFullyIn("root9.*.field2.subfield1"));
EXPECT_FALSE(field_mask.IsPathFullyIn("root9.*.field2.subfield2"));
EXPECT_FALSE(field_mask.IsPathFullyIn("root9.some_key"));
EXPECT_TRUE(field_mask.IsPathFullyIn("root9.some_key.field"));
EXPECT_TRUE(field_mask.IsPathFullyIn("root9.some_key.field.subfield1"));
}

TEST(FieldMaskIsPathFullyIn, EmptyMask) {
Expand Down Expand Up @@ -865,6 +859,9 @@ TEST(FieldMaskIsPathPartiallyIn, MockFieldMask) {
EXPECT_TRUE(field_mask.IsPathPartiallyIn("root9.*.field.subfield2"));
EXPECT_FALSE(field_mask.IsPathPartiallyIn("root9.*.field2.subfield1"));
EXPECT_FALSE(field_mask.IsPathPartiallyIn("root9.*.field2.subfield2"));
EXPECT_TRUE(field_mask.IsPathPartiallyIn("root9.some_key"));
EXPECT_TRUE(field_mask.IsPathPartiallyIn("root9.some_key.field"));
EXPECT_TRUE(field_mask.IsPathPartiallyIn("root9.some_key.field.subfield1"));
}

TEST(FieldMaskIsPathPartiallyIn, EmptyMask) {
Expand Down Expand Up @@ -990,16 +987,14 @@ TEST(FieldMaskTrim, TrivialMessage) {

DISABLE_IF_OLD_PROTOBUF_TEST(FieldMaskTrim, SimpleFieldMask) {
auto message = ConstructMessage();
ugrpc::FieldMask(MakeGoogleFieldMask(kSimpleFieldMask)).Trim(*message);
Compare(*message, ConstructTrimmedSimple());
delete message;
ugrpc::FieldMask(MakeGoogleFieldMask(kSimpleFieldMask)).Trim(message);
Compare(message, ConstructTrimmedSimple());
}

DISABLE_IF_OLD_PROTOBUF_TEST(FieldMaskTrim, HardFieldMask) {
auto message = ConstructMessage();
ugrpc::FieldMask(MakeGoogleFieldMask(kHardFieldMask)).Trim(*message);
Compare(*message, ConstructTrimmedHard());
delete message;
ugrpc::FieldMask(MakeGoogleFieldMask(kHardFieldMask)).Trim(message);
Compare(message, ConstructTrimmedHard());
}

TEST(FieldMaskGetMaskForField, NonExistingChild) {
Expand All @@ -1024,6 +1019,8 @@ TEST(FieldMaskHasFieldName, MockFieldMask) {
ugrpc::FieldMask mask(MakeGoogleFieldMask(kMockFieldMask));
EXPECT_FALSE(mask.HasFieldName("something-weird"));
EXPECT_TRUE(mask.HasFieldName("root1"));
EXPECT_TRUE(mask.GetMaskForField("root9")->HasFieldName("*"));
EXPECT_TRUE(mask.GetMaskForField("root9")->HasFieldName("some_key"));
}

TEST(FieldMaskHasFieldName, NonExistingChildOnLeaf) {
Expand Down
Loading