[protobuf] proto: add MergeWithOptions to enable repeated/map replacement instead of appendage
155 views
Skip to first unread message
Gopher Robot (Gerrit)
Sep 27, 2022, 9:03:50 PM9/27/22
to Kareem Halabi, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
To view, visit change 435635. To unsubscribe, or for help writing mail filters, visit settings.
Kareem Halabi (Gerrit)
Sep 28, 2022, 2:19:26 PM9/28/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Kareem Halabi has uploaded this change for review.
proto: add MergeWithOptions to enable repeated/map replacement instead of appendage
Accomplished by exporting the existing options struct, adding in two bool flags for both list and map field replacement.
Deferring the implementation of fast-path merge when options are present since that whole execution path is more complex and may take longer to update correctly.
Updates golang/protobuf#1488
Change-Id: I41f3e4cfc7ed0e7cf124a6d2b70be26c952f4cbd
---
M proto/merge.go
M proto/merge_test.go
2 files changed, 250 insertions(+), 13 deletions(-)
diff --git a/proto/merge.go b/proto/merge.go
index d761ab3..85125fc 100644
--- a/proto/merge.go
+++ b/proto/merge.go
@@ -15,7 +15,7 @@
//
// Populated scalar fields in src are copied to dst, while populated
// singular messages in src are merged into dst by recursively calling Merge.
-// The elements of every list field in src is appended to the corresponded
+// The elements of every list field in src is appended to the corresponding
// list fields in dst. The entries of every map field in src is copied into
// the corresponding map field in dst, possibly replacing existing entries.
// The unknown fields of src are appended to the unknown fields of dst.
@@ -23,6 +23,12 @@
// It is semantically equivalent to unmarshaling the encoded form of src
// into dst with the UnmarshalOptions.Merge option specified.
func Merge(dst, src Message) {
+ MergeWithOptions(dst, src, MergeOptions{})
+}
+
+// MergeWithOptions is like Merge, but provides fine-grained control on the
+// behaviour of merging repeated and map fields.
+func MergeWithOptions(dst, src Message, opts MergeOptions) {
// TODO: Should nil src be treated as semantically equivalent to a
// untyped, read-only, empty message? What about a nil dst?
@@ -33,7 +39,7 @@
}
panic("descriptor mismatch")
}
- mergeOptions{}.mergeMessage(dstMsg, srcMsg)
+ opts.mergeMessage(dstMsg, srcMsg)
}
// Clone returns a deep copy of m.
@@ -55,17 +61,28 @@
return src.Type().Zero().Interface()
}
dst := src.New()
- mergeOptions{}.mergeMessage(dst, src)
+ MergeOptions{}.mergeMessage(dst, src)
return dst.Interface()
}
-// mergeOptions provides a namespace for merge functions, and can be
-// exported in the future if we add user-visible merge options.
-type mergeOptions struct{}
+// MergeOptions provides user-visible merge options.
+type MergeOptions struct {
+ // If set to true, every populated list field in src replaces the
+ // corresponding list field in dst. Otherwise, the elements of every list
+ // field in src is appended to the corresponding list fields in dst.
+ ReplaceRepeatedFields bool
-func (o mergeOptions) mergeMessage(dst, src protoreflect.Message) {
+ // If set to true, every populated map field in src replaces the corresponding
+ // map field in dst. Otherwise, the entries of every map field in src is
+ // copied into the corresponding map field in dst, possibly replacing existing
+ // entries.
+ ReplaceMapFields bool
+}
+
+func (o MergeOptions) mergeMessage(dst, src protoreflect.Message) {
methods := protoMethods(dst)
- if methods != nil && methods.Merge != nil {
+ // TODO: Implement fast-path merge when options are present.
+ if methods != nil && methods.Merge != nil && o == (MergeOptions{}) {
in := protoiface.MergeInput{
Destination: dst,
Source: src,
@@ -101,8 +118,11 @@
}
}
-func (o mergeOptions) mergeList(dst, src protoreflect.List, fd protoreflect.FieldDescriptor) {
- // Merge semantics appends to the end of the existing list.
+func (o MergeOptions) mergeList(dst, src protoreflect.List, fd protoreflect.FieldDescriptor) {
+ if o.ReplaceRepeatedFields {
+ dst.Truncate(0)
+ }
+ // Default merge semantics appends to the end of the existing list.
for i, n := 0, src.Len(); i < n; i++ {
switch v := src.Get(i); {
case fd.Message() != nil:
@@ -117,8 +137,14 @@
}
}
-func (o mergeOptions) mergeMap(dst, src protoreflect.Map, fd protoreflect.FieldDescriptor) {
- // Merge semantics replaces, rather than merges into existing entries.
+func (o MergeOptions) mergeMap(dst, src protoreflect.Map, fd protoreflect.FieldDescriptor) {
+ if o.ReplaceMapFields {
+ dst.Range(func(k protoreflect.MapKey, v protoreflect.Value) bool {
+ dst.Clear(k)
+ return true
+ })
+ }
+ // Default merge semantics replaces, rather than merges into existing entries.
src.Range(func(k protoreflect.MapKey, v protoreflect.Value) bool {
switch {
case fd.Message() != nil:
@@ -134,6 +160,6 @@
})
}
-func (o mergeOptions) cloneBytes(v protoreflect.Value) protoreflect.Value {
+func (o MergeOptions) cloneBytes(v protoreflect.Value) protoreflect.Value {
return protoreflect.ValueOfBytes(append([]byte{}, v.Bytes()...))
}
diff --git a/proto/merge_test.go b/proto/merge_test.go
index 05978cb..2a9de90 100644
--- a/proto/merge_test.go
+++ b/proto/merge_test.go
@@ -31,6 +31,7 @@
src protobuild.Message
want protobuild.Message // if dst and want are nil, want = src
types []proto.Message
+ opts proto.MergeOptions
}
var testMerges = []testMerge{{
@@ -651,6 +652,202 @@
}
}
+func TestMergeWithOptions(t *testing.T) {
+ tests := []testMerge{
+ {
+ desc: "merge list fields with replacement",
+ dst: protobuild.Message{
+ "repeated_int32": []int32{1, 2, 3},
+ "repeated_int64": []int64{1, 2, 3},
+ "repeated_uint32": []uint32{1, 2, 3},
+ "repeated_uint64": []uint64{1, 2, 3},
+ "repeated_sint32": []int32{1, 2, 3},
+ "repeated_sint64": []int64{1, 2, 3},
+ "repeated_fixed32": []uint32{1, 2, 3},
+ "repeated_fixed64": []uint64{1, 2, 3},
+ "repeated_sfixed32": []int32{1, 2, 3},
+ "repeated_sfixed64": []int64{1, 2, 3},
+ "repeated_float": []float32{1, 2, 3},
+ "repeated_double": []float64{1, 2, 3},
+ "repeated_bool": []bool{true},
+ "repeated_string": []string{"a", "b", "c"},
+ "repeated_bytes": []string{"a", "b", "c"},
+ "repeated_nested_enum": []int{1, 2, 3},
+ "repeated_nested_message": []protobuild.Message{
+ {"a": 100},
+ {"a": 200},
+ },
+ },
+ src: protobuild.Message{
+ "repeated_int32": []int32{4, 5, 6},
+ "repeated_int64": []int64{4, 5, 6},
+ "repeated_uint32": []uint32{4, 5, 6},
+ "repeated_uint64": []uint64{4, 5, 6},
+ "repeated_sint32": []int32{4, 5, 6},
+ "repeated_sint64": []int64{4, 5, 6},
+ "repeated_fixed32": []uint32{4, 5, 6},
+ "repeated_fixed64": []uint64{4, 5, 6},
+ "repeated_sfixed32": []int32{4, 5, 6},
+ "repeated_sfixed64": []int64{4, 5, 6},
+ "repeated_float": []float32{4, 5, 6},
+ "repeated_double": []float64{4, 5, 6},
+ "repeated_bool": []bool{false},
+ "repeated_string": []string{"d", "e", "f"},
+ "repeated_bytes": []string{"d", "e", "f"},
+ "repeated_nested_enum": []int{4, 5, 6},
+ "repeated_nested_message": []protobuild.Message{
+ {"a": 300},
+ {"a": 400},
+ },
+ },
+ want: protobuild.Message{
+ "repeated_int32": []int32{4, 5, 6},
+ "repeated_int64": []int64{4, 5, 6},
+ "repeated_uint32": []uint32{4, 5, 6},
+ "repeated_uint64": []uint64{4, 5, 6},
+ "repeated_sint32": []int32{4, 5, 6},
+ "repeated_sint64": []int64{4, 5, 6},
+ "repeated_fixed32": []uint32{4, 5, 6},
+ "repeated_fixed64": []uint64{4, 5, 6},
+ "repeated_sfixed32": []int32{4, 5, 6},
+ "repeated_sfixed64": []int64{4, 5, 6},
+ "repeated_float": []float32{4, 5, 6},
+ "repeated_double": []float64{4, 5, 6},
+ "repeated_bool": []bool{false},
+ "repeated_string": []string{"d", "e", "f"},
+ "repeated_bytes": []string{"d", "e", "f"},
+ "repeated_nested_enum": []int{4, 5, 6},
+ "repeated_nested_message": []protobuild.Message{
+ {"a": 300},
+ {"a": 400},
+ },
+ },
+ opts: proto.MergeOptions{ReplaceRepeatedFields: true},
+ }, {
+ desc: "merge map fields with replacement",
+ dst: protobuild.Message{
+ "map_int32_int32": map[int]int{1: 1, 3: 1},
+ "map_int64_int64": map[int]int{1: 1, 3: 1},
+ "map_uint32_uint32": map[int]int{1: 1, 3: 1},
+ "map_uint64_uint64": map[int]int{1: 1, 3: 1},
+ "map_sint32_sint32": map[int]int{1: 1, 3: 1},
+ "map_sint64_sint64": map[int]int{1: 1, 3: 1},
+ "map_fixed32_fixed32": map[int]int{1: 1, 3: 1},
+ "map_fixed64_fixed64": map[int]int{1: 1, 3: 1},
+ "map_sfixed32_sfixed32": map[int]int{1: 1, 3: 1},
+ "map_sfixed64_sfixed64": map[int]int{1: 1, 3: 1},
+ "map_int32_float": map[int]int{1: 1, 3: 1},
+ "map_int32_double": map[int]int{1: 1, 3: 1},
+ "map_bool_bool": map[bool]bool{true: true},
+ "map_string_string": map[string]string{"a": "1", "ab": "1"},
+ "map_string_bytes": map[string]string{"a": "1", "ab": "1"},
+ "map_string_nested_message": map[string]protobuild.Message{
+ "a": {"a": 1},
+ "ab": {
+ "a": 1,
+ "corecursive": protobuild.Message{
+ "map_int32_int32": map[int]int{1: 1, 3: 1},
+ },
+ },
+ },
+ "map_string_nested_enum": map[string]int{"a": 1, "ab": 1},
+ },
+ src: protobuild.Message{
+ "map_int32_int32": map[int]int{2: 2, 3: 2},
+ "map_int64_int64": map[int]int{2: 2, 3: 2},
+ "map_uint32_uint32": map[int]int{2: 2, 3: 2},
+ "map_uint64_uint64": map[int]int{2: 2, 3: 2},
+ "map_sint32_sint32": map[int]int{2: 2, 3: 2},
+ "map_sint64_sint64": map[int]int{2: 2, 3: 2},
+ "map_fixed32_fixed32": map[int]int{2: 2, 3: 2},
+ "map_fixed64_fixed64": map[int]int{2: 2, 3: 2},
+ "map_sfixed32_sfixed32": map[int]int{2: 2, 3: 2},
+ "map_sfixed64_sfixed64": map[int]int{2: 2, 3: 2},
+ "map_int32_float": map[int]int{2: 2, 3: 2},
+ "map_int32_double": map[int]int{2: 2, 3: 2},
+ "map_bool_bool": map[bool]bool{false: false},
+ "map_string_string": map[string]string{"b": "2", "ab": "2"},
+ "map_string_bytes": map[string]string{"b": "2", "ab": "2"},
+ "map_string_nested_message": map[string]protobuild.Message{
+ "b": {"a": 2},
+ "ab": {
+ "a": 2,
+ "corecursive": protobuild.Message{
+ "map_int32_int32": map[int]int{2: 2, 3: 2},
+ },
+ },
+ },
+ "map_string_nested_enum": map[string]int{"b": 2, "ab": 2},
+ },
+ want: protobuild.Message{
+ "map_int32_int32": map[int]int{2: 2, 3: 2},
+ "map_int64_int64": map[int]int{2: 2, 3: 2},
+ "map_uint32_uint32": map[int]int{2: 2, 3: 2},
+ "map_uint64_uint64": map[int]int{2: 2, 3: 2},
+ "map_sint32_sint32": map[int]int{2: 2, 3: 2},
+ "map_sint64_sint64": map[int]int{2: 2, 3: 2},
+ "map_fixed32_fixed32": map[int]int{2: 2, 3: 2},
+ "map_fixed64_fixed64": map[int]int{2: 2, 3: 2},
+ "map_sfixed32_sfixed32": map[int]int{2: 2, 3: 2},
+ "map_sfixed64_sfixed64": map[int]int{2: 2, 3: 2},
+ "map_int32_float": map[int]int{2: 2, 3: 2},
+ "map_int32_double": map[int]int{2: 2, 3: 2},
+ "map_bool_bool": map[bool]bool{false: false},
+ "map_string_string": map[string]string{"b": "2", "ab": "2"},
+ "map_string_bytes": map[string]string{"b": "2", "ab": "2"},
+ "map_string_nested_message": map[string]protobuild.Message{
+ "b": {"a": 2},
+ "ab": {
+ "a": 2,
+ "corecursive": protobuild.Message{
+ "map_int32_int32": map[int]int{2: 2, 3: 2},
+ },
+ },
+ },
+ "map_string_nested_enum": map[string]int{"b": 2, "ab": 2},
+ },
+ types: []proto.Message{&testpb.TestAllTypes{}, &test3pb.TestAllTypes{}},
+ opts: proto.MergeOptions{ReplaceMapFields: true},
+ },
+ }
+ for _, tt := range tests {
+ for _, mt := range templateMessages(tt.types...) {
+ t.Run(fmt.Sprintf("%s (%v)", tt.desc, mt.Descriptor().FullName()), func(t *testing.T) {
+ dst := mt.New().Interface()
+ tt.dst.Build(dst.ProtoReflect())
+
+ src := mt.New().Interface()
+ tt.src.Build(src.ProtoReflect())
+
+ want := mt.New().Interface()
+ if tt.dst == nil && tt.want == nil {
+ tt.src.Build(want.ProtoReflect())
+ } else {
+ tt.want.Build(want.ProtoReflect())
+ }
+
+ // Test heterogeneous MessageTypes by merging into a
+ // dynamic message.
+ ddst := dynamicpb.NewMessage(mt.Descriptor())
+ tt.dst.Build(ddst.ProtoReflect())
+ proto.MergeWithOptions(ddst, src, tt.opts)
+ if !proto.Equal(ddst, want) {
+ t.Fatalf("MergeWithOptions() into dynamic message mismatch:\n got %v\nwant %v\ndiff (-want,+got):\n%v", ddst, want, cmp.Diff(want, ddst, protocmp.Transform()))
+ }
+
+ proto.MergeWithOptions(dst, src, tt.opts)
+ if !proto.Equal(dst, want) {
+ t.Fatalf("MergeWithOptions() mismatch:\n got %v\nwant %v\ndiff (-want,+got):\n%v", dst, want, cmp.Diff(want, dst, protocmp.Transform()))
+ }
+ mutateValue(protoreflect.ValueOfMessage(src.ProtoReflect()))
+ if !proto.Equal(dst, want) {
+ t.Fatalf("mutation observed after modifying source:\n got %v\nwant %v\ndiff (-want,+got):\n%v", dst, want, cmp.Diff(want, dst, protocmp.Transform()))
+ }
+ })
+ }
+ }
+}
+
func TestMergeFromNil(t *testing.T) {
dst := &testpb.TestAllTypes{}
proto.Merge(dst, (*testpb.TestAllTypes)(nil))
To view, visit change 435635. To unsubscribe, or for help writing mail filters, visit settings.
Kareem Halabi (Gerrit)
Sep 29, 2022, 1:37:46 PM9/29/22
to goph...@pubsubhelper.golang.org, Gopher Robot, golang-co...@googlegroups.com
Kareem Halabi abandoned this change.
To view, visit change 435635. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages