Skip to content

Commit

Permalink
refactor grpc: fix clang-tidy and improve field mask wildcard handling
Browse files Browse the repository at this point in the history 
Tests: протестировано CI
Pull Request resolved: #743

Co-authored-by: TTPO100AJIEX <69868712+TTPO100AJIEX@users.noreply.github.com>
commit_hash:a5e7a491e84a64212104618bc194fb0afe015d22
  • Loading branch information
@TTPO100AJIEX @fdr400
TTPO100AJIEX authored and fdr400 committed Nov 2, 2024
1 parent 5bcf855 commit ebd6e7d
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 61 deletions.
35 changes: 13 additions & 22 deletions grpc/src/ugrpc/field_mask.cpp
View file
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
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

0 comments on commit ebd6e7d

Please sign in to comment.