Skip to content

Commit 0f109cb

Browse files
protobuf-github-botcopybara-github
protobuf-github-bot
authored and
copybara-github
committed
FieldMaskUtil::TrimMessage: Handle repeated messages.
Currently this fails with the following error: Protocol Buffer reflection usage error: Method : proto2::Reflection::MutableMessage Message type: proto2_unittest.TestAllTypes Field : proto2_unittest.TestAllTypes.repeated_nested_message Problem : Field is repeated; the method requires a singular field. This now handles repeated messages in a separate branch. PiperOrigin-RevId: 847099614
1 parent 0a5c2f6 commit 0f109cb

File tree

4 files changed

+139
-26
lines changed

4 files changed

+139
-26
lines changed

‎src/google/protobuf/util/BUILD.bazel‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,21 +135,43 @@ cc_library(
135135
cc_test(
136136
name = "field_mask_util_test",
137137
srcs = ["field_mask_util_test.cc"],
138-
copts = COPTS,
138+
copts = COPTS + select({
139+
"//build_defs:config_msvc": [],
140+
"//conditions:default": [
141+
"-Wno-deprecated-declarations",
142+
],
143+
}),
139144
deps = [
140145
":field_mask_util",
146+
":field_mask_util_test_cc_proto",
141147
"//src/google/protobuf",
142148
"//src/google/protobuf:cc_test_protos",
143149
"//src/google/protobuf:field_mask_cc_proto",
150+
"//src/google/protobuf:test_textproto",
144151
"//src/google/protobuf:test_util",
145152
"//src/google/protobuf/stubs",
146153
"//src/google/protobuf/testing",
147154
"//src/google/protobuf/testing:file",
155+
"@abseil-cpp//absl/base:log_severity",
156+
"@abseil-cpp//absl/strings:string_view",
148157
"@googletest//:gtest",
149158
"@googletest//:gtest_main",
150159
],
151160
)
152161

162+
proto_library(
163+
name = "field_mask_util_test_proto",
164+
testonly = 1,
165+
srcs = ["field_mask_util_test.proto"],
166+
strip_import_prefix = "/src",
167+
)
168+
169+
cc_proto_library(
170+
name = "field_mask_util_test_cc_proto",
171+
testonly = 1,
172+
deps = [":field_mask_util_test_proto"],
173+
)
174+
153175
cc_library(
154176
name = "internal_timeval",
155177
hdrs = ["internal_timeval.h"],
@@ -266,6 +288,7 @@ filegroup(
266288
name = "test_proto_srcs",
267289
testonly = 1,
268290
srcs = [
291+
"field_mask_util_test.proto",
269292
"json_format.proto",
270293
"json_format_proto3.proto",
271294
"message_differencer_unittest.proto",

‎src/google/protobuf/util/field_mask_util.cc‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,12 @@
1717
#include "absl/log/absl_log.h"
1818
#include "absl/log/die_if_null.h"
1919
#include "absl/memory/memory.h"
20+
#include "absl/strings/str_cat.h"
2021
#include "absl/strings/str_join.h"
2122
#include "absl/strings/str_split.h"
2223
#include "absl/strings/string_view.h"
2324
#include "absl/strings/strip.h"
25+
#include "google/protobuf/descriptor.h"
2426
#include "google/protobuf/message.h"
2527

2628
// Must be included last.
@@ -584,9 +586,17 @@ bool FieldMaskTree::TrimMessage(const Node* node, Message* message) {
584586
if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
585587
Node* child = it->second.get();
586588
if (!child->children.empty() && reflection->HasField(*message, field)) {
587-
bool nestedMessageChanged =
588-
TrimMessage(child, reflection->MutableMessage(message, field));
589-
modified = nestedMessageChanged || modified;
589+
if (field->is_repeated()) {
590+
for (int i = 0; i < reflection->FieldSize(*message, field); ++i) {
591+
bool nestedMessageChanged = TrimMessage(
592+
child, reflection->MutableRepeatedMessage(message, field, i));
593+
modified = nestedMessageChanged || modified;
594+
}
595+
} else {
596+
bool nestedMessageChanged =
597+
TrimMessage(child, reflection->MutableMessage(message, field));
598+
modified = nestedMessageChanged || modified;
599+
}
590600
}
591601
}
592602
}

‎src/google/protobuf/util/field_mask_util_test.cc‎

Lines changed: 87 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,19 @@
99

1010
#include <algorithm>
1111
#include <cstdint>
12+
#include <string>
1213
#include <vector>
1314

1415
#include "google/protobuf/field_mask.pb.h"
16+
#include <gmock/gmock.h>
1517
#include <gtest/gtest.h>
18+
#include "absl/base/log_severity.h"
19+
#include "absl/strings/string_view.h"
20+
#include "google/protobuf/test_textproto.h"
1621
#include "google/protobuf/test_util.h"
1722
#include "google/protobuf/unittest.pb.h"
23+
#include "google/protobuf/util/field_mask_util.h"
24+
#include "google/protobuf/util/field_mask_util_test.pb.h"
1825

1926
namespace google {
2027
namespace protobuf {
@@ -95,6 +102,7 @@ using proto2_unittest::NestedTestAllTypes;
95102
using proto2_unittest::TestAllTypes;
96103
using proto2_unittest::TestRequired;
97104
using proto2_unittest::TestRequiredMessage;
105+
using third_party_protobuf_util::TestTrimMessageRepeatedField;
98106

99107
TEST(FieldMaskUtilTest, StringFormat) {
100108
FieldMask mask;
@@ -581,16 +589,16 @@ TEST(FieldMaskUtilTest, TrimMessage) {
581589
TEST_TRIM_ONE_PRIMITIVE_FIELD(optional_import_enum)
582590
#undef TEST_TRIM_ONE_PRIMITIVE_FIELD
583591

584-
#define TEST_TRIM_ONE_FIELD(field_name) \
585-
{ \
586-
TestAllTypes msg; \
587-
TestUtil::SetAllFields(&msg); \
588-
TestAllTypes tmp; \
589-
*tmp.mutable_##field_name() = msg.field_name(); \
590-
FieldMask mask; \
591-
mask.add_paths(#field_name); \
592-
FieldMaskUtil::TrimMessage(mask, &msg); \
593-
EXPECT_EQ(tmp.DebugString(), msg.DebugString()); \
592+
#define TEST_TRIM_ONE_FIELD(field_name) \
593+
{ \
594+
TestAllTypes msg; \
595+
TestUtil::SetAllFields(&msg); \
596+
TestAllTypes tmp; \
597+
*tmp.mutable_##field_name() = msg.field_name(); \
598+
FieldMask mask; \
599+
mask.add_paths(#field_name); \
600+
EXPECT_TRUE(FieldMaskUtil::TrimMessage(mask, &msg)); \
601+
EXPECT_EQ(tmp.DebugString(), msg.DebugString()); \
594602
}
595603
TEST_TRIM_ONE_FIELD(optional_nested_message)
596604
TEST_TRIM_ONE_FIELD(optional_foreign_message)
@@ -629,25 +637,25 @@ TEST(FieldMaskUtilTest, TrimMessage) {
629637
NestedTestAllTypes trimmed_msg(nested_msg);
630638
FieldMask mask;
631639
FieldMaskUtil::FromString("child.payload", &mask);
632-
FieldMaskUtil::TrimMessage(mask, &trimmed_msg);
640+
EXPECT_TRUE(FieldMaskUtil::TrimMessage(mask, &trimmed_msg));
633641
EXPECT_EQ(1234, trimmed_msg.child().payload().optional_int32());
634642
EXPECT_EQ(0, trimmed_msg.child().child().payload().optional_int32());
635643

636644
trimmed_msg = nested_msg;
637645
FieldMaskUtil::FromString("child.child.payload", &mask);
638-
FieldMaskUtil::TrimMessage(mask, &trimmed_msg);
646+
EXPECT_TRUE(FieldMaskUtil::TrimMessage(mask, &trimmed_msg));
639647
EXPECT_EQ(0, trimmed_msg.child().payload().optional_int32());
640648
EXPECT_EQ(5678, trimmed_msg.child().child().payload().optional_int32());
641649

642650
trimmed_msg = nested_msg;
643651
FieldMaskUtil::FromString("child", &mask);
644-
FieldMaskUtil::TrimMessage(mask, &trimmed_msg);
652+
EXPECT_FALSE(FieldMaskUtil::TrimMessage(mask, &trimmed_msg));
645653
EXPECT_EQ(1234, trimmed_msg.child().payload().optional_int32());
646654
EXPECT_EQ(5678, trimmed_msg.child().child().payload().optional_int32());
647655

648656
trimmed_msg = nested_msg;
649657
FieldMaskUtil::FromString("child.child", &mask);
650-
FieldMaskUtil::TrimMessage(mask, &trimmed_msg);
658+
EXPECT_TRUE(FieldMaskUtil::TrimMessage(mask, &trimmed_msg));
651659
EXPECT_EQ(0, trimmed_msg.child().payload().optional_int32());
652660
EXPECT_EQ(5678, trimmed_msg.child().child().payload().optional_int32());
653661

@@ -656,7 +664,7 @@ TEST(FieldMaskUtilTest, TrimMessage) {
656664
TestUtil::SetAllFields(&all_types_msg);
657665
TestAllTypes trimmed_all_types(all_types_msg);
658666
FieldMask empty_mask;
659-
FieldMaskUtil::TrimMessage(empty_mask, &trimmed_all_types);
667+
EXPECT_FALSE(FieldMaskUtil::TrimMessage(empty_mask, &trimmed_all_types));
660668
EXPECT_EQ(trimmed_all_types.DebugString(), all_types_msg.DebugString());
661669

662670
// Test trim required fields with keep_required_fields is set true.
@@ -668,15 +676,17 @@ TEST(FieldMaskUtilTest, TrimMessage) {
668676
TestRequired trimmed_required_msg_1(required_msg_1);
669677
FieldMaskUtil::FromString("dummy2", &mask);
670678
options.set_keep_required_fields(true);
671-
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_1, options);
679+
EXPECT_FALSE(
680+
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_1, options));
672681
EXPECT_EQ(trimmed_required_msg_1.DebugString(), required_msg_1.DebugString());
673682

674683
// Test trim required fields with keep_required_fields is set false.
675684
required_msg_1.clear_a();
676685
required_msg_1.clear_b();
677686
required_msg_1.clear_c();
678687
options.set_keep_required_fields(false);
679-
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_1, options);
688+
EXPECT_TRUE(
689+
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_1, options));
680690
EXPECT_EQ(trimmed_required_msg_1.DebugString(), required_msg_1.DebugString());
681691

682692
// Test trim required message with keep_required_fields is set true.
@@ -697,14 +707,16 @@ TEST(FieldMaskUtilTest, TrimMessage) {
697707
options.set_keep_required_fields(true);
698708
required_msg_2.clear_repeated_message();
699709
required_msg_2.mutable_required_message()->clear_dummy2();
700-
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_2, options);
710+
EXPECT_TRUE(
711+
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_2, options));
701712
EXPECT_EQ(trimmed_required_msg_2.DebugString(), required_msg_2.DebugString());
702713

703714
FieldMaskUtil::FromString("required_message", &mask);
704715
required_msg_2.mutable_required_message()->set_dummy2(7890);
705716
trimmed_required_msg_2.mutable_required_message()->set_dummy2(7890);
706717
required_msg_2.clear_optional_message();
707-
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_2, options);
718+
EXPECT_TRUE(
719+
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_2, options));
708720
EXPECT_EQ(trimmed_required_msg_2.DebugString(), required_msg_2.DebugString());
709721

710722
// Test trim required message with keep_required_fields is set false.
@@ -713,15 +725,16 @@ TEST(FieldMaskUtilTest, TrimMessage) {
713725
required_msg_2.mutable_required_message()->clear_b();
714726
required_msg_2.mutable_required_message()->clear_c();
715727
options.set_keep_required_fields(false);
716-
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_2, options);
728+
EXPECT_TRUE(
729+
FieldMaskUtil::TrimMessage(mask, &trimmed_required_msg_2, options));
717730
EXPECT_EQ(trimmed_required_msg_2.DebugString(), required_msg_2.DebugString());
718731

719732
// Verify that trimming an empty message has no effect. In particular, fields
720733
// mentioned in the field mask should not be created or changed.
721734
TestAllTypes empty_msg;
722735
FieldMaskUtil::FromString(
723736
"optional_int32,optional_bytes,optional_nested_message.bb", &mask);
724-
FieldMaskUtil::TrimMessage(mask, &empty_msg);
737+
EXPECT_FALSE(FieldMaskUtil::TrimMessage(mask, &empty_msg));
725738
EXPECT_FALSE(empty_msg.has_optional_int32());
726739
EXPECT_FALSE(empty_msg.has_optional_bytes());
727740
EXPECT_FALSE(empty_msg.has_optional_nested_message());
@@ -731,10 +744,62 @@ TEST(FieldMaskUtilTest, TrimMessage) {
731744
TestAllTypes oneof_msg;
732745
oneof_msg.set_oneof_uint32(11);
733746
FieldMaskUtil::FromString("oneof_uint32,oneof_nested_message.bb", &mask);
734-
FieldMaskUtil::TrimMessage(mask, &oneof_msg);
747+
EXPECT_FALSE(FieldMaskUtil::TrimMessage(mask, &oneof_msg));
735748
EXPECT_EQ(11, oneof_msg.oneof_uint32());
736749
}
737750

751+
TEST(FieldMaskUtilTest, TrimMessageRepeatedField) {
752+
FieldMask f1_mask;
753+
FieldMaskUtil::FromString("repeated_nested_message.f1", &f1_mask);
754+
TestTrimMessageRepeatedField msg = ParseTextOrDie(R"pb(
755+
repeated_nested_message { f1: 1234 }
756+
repeated_nested_message { f1: 5678 f2: 9012 }
757+
repeated_nested_message { f2: 9012 }
758+
)pb");
759+
EXPECT_TRUE(FieldMaskUtil::TrimMessage(f1_mask, &msg));
760+
EXPECT_THAT(msg, EqualsProto(R"pb(
761+
repeated_nested_message { f1: 1234 }
762+
repeated_nested_message { f1: 5678 }
763+
repeated_nested_message {}
764+
)pb"));
765+
EXPECT_FALSE(FieldMaskUtil::TrimMessage(f1_mask, &msg));
766+
}
767+
768+
TEST(FieldMaskUtilTest, TrimMessageRepeatedFieldLastFieldUnchanged) {
769+
FieldMask f1_mask;
770+
FieldMaskUtil::FromString("repeated_nested_message.f1", &f1_mask);
771+
// The last field will not be modified but TrimMessage should still return
772+
// true because the message was modified.
773+
TestTrimMessageRepeatedField msg = ParseTextOrDie(R"pb(
774+
repeated_nested_message { f1: 5678 f2: 9012 }
775+
repeated_nested_message { f1: 1234 }
776+
)pb");
777+
EXPECT_TRUE(FieldMaskUtil::TrimMessage(f1_mask, &msg));
778+
EXPECT_THAT(msg, EqualsProto(R"pb(
779+
repeated_nested_message { f1: 5678 }
780+
repeated_nested_message { f1: 1234 }
781+
)pb"));
782+
EXPECT_FALSE(FieldMaskUtil::TrimMessage(f1_mask, &msg));
783+
}
784+
785+
TEST(FieldMaskUtilTest, LastFieldUnchangedStillReturnsTrue) {
786+
FieldMask f1_mask;
787+
FieldMaskUtil::FromString("nested_message.f1,nested_message2.f1", &f1_mask);
788+
// The first nested message is modified, but the second is not. The function
789+
// should return true and not get over written because nested_message2 is not
790+
// modified.
791+
TestTrimMessageRepeatedField msg = ParseTextOrDie(R"pb(
792+
nested_message { f2: 1234 }
793+
nested_message2 { f1: 5678 }
794+
)pb");
795+
EXPECT_TRUE(FieldMaskUtil::TrimMessage(f1_mask, &msg));
796+
EXPECT_THAT(msg, EqualsProto(R"pb(
797+
nested_message {}
798+
nested_message2 { f1: 5678 }
799+
)pb"));
800+
EXPECT_FALSE(FieldMaskUtil::TrimMessage(f1_mask, &msg));
801+
}
802+
738803
TEST(FieldMaskUtilTest, TrimMessageReturnValue) {
739804
FieldMask mask;
740805
TestAllTypes trimmed_msg;

‎src/google/protobuf/util/field_mask_util_test.proto‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
syntax = "proto2";
2+
3+
package third_party_protobuf_util;
4+
5+
option optimize_for = SPEED;
6+
7+
message TestTrimMessageRepeatedField {
8+
message NestedMessage {
9+
optional int32 f1 = 1;
10+
optional int32 f2 = 2;
11+
}
12+
optional NestedMessage nested_message = 1;
13+
repeated NestedMessage repeated_nested_message = 2;
14+
optional NestedMessage nested_message2 = 3;
15+
}

0 commit comments

Comments
 (0)