Feature Request: proto.Merge with option repeated/map field replacement instead of appendage

[kareemhalabi]

Is your feature request related to a problem? Please describe. Not really but this would be a nice to have.

Describe the solution you'd like Currently the documentation for proto.Merge states the following:

...The elements of every list field in src is appended to the corresponded 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...

It would be nice if there was an option to replace the populated list and/or map fields from src in dst rather than appending to them.

I noticed there is an unexported struct mergeOptions that is seems to preemptively support user-visible merge options. Could look something like:

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

  // 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
}

For backwards compatibility a new exported function could be added

func MergeWithOptions(src, dst Message, opts MergeOptions)

Describe alternatives you've considered Currently what I do to achieve this is to recursively compare all repeated and map fields in src and dst, clearing out any list/map fields in dst that are populated in src. Then run proto.Merge

[puellanivis]

🤷‍♀️ Yeah, I guess this sounds entirely reasonable and possible. I’m not saying I approve adding it, but I don’t see anything obvious as to why not.

[stapelberg]

Yep, we have seen similar requests before, so there seems to be an occasional need for this.

So let’s say we tentatively agree to this, provided the implementation is straight-forward and not too complex to maintain.

Can you send a Gerrit change for this, @kareemhalabi?

[kareemhalabi]

Yeah I can give it a go (no pun intended)

[stapelberg]

Actually, change of plans (sorry!).

I spoke to other Protobuf folks and they advised against adding this change. The semantics of proto.Merge currently match the semantics of concatenating two byte streams. With the suggested change, that invariant would no longer be true.

That said, I’m pretty confident you can make your behavior work in a separate package by using the https://pkg.go.dev/google.golang.org/protobuf/reflect/protoreflect package.

[kareemhalabi]

FWIW here's the Gerrit change that I made: https://go-review.googlesource.com/c/protobuf/+/435635?usp=email

proto.Merge is entirely unaffected, I'm exporting a separate function (proto.MergeWithOptions) to support the additional options. Wouldn't it then be ok in that case to have different semantics?

[neild]

As a general rule, we don't want to add new general-purpose features to only one protobuf implementation. If we add merge-but-replace-repeated-fields to the Go API, then we should also add it to C++, C#, Java, Objective C, PHP, Python, Ruby, and whatever other supported implementations I'm forgetting about.

This means that the bar for adding new features is quite high, and that the decision on whether to add them is not made by the maintainers of any one implementation.

The good news is that this is entirely possible to do in a separate package; the implementation of Merge is small (merge.go is just 139 lines counting comments) and uses only public features of the protoreflect package.

[puellanivis]

In light of the information above, I’d probably suggest someone implementing this might want to go with OverwriteInto rather than Merge.

A suggestion of that function to the protobuf team might even see traction towards adoption.

[kareemhalabi]

Ah okay that makes sense, thanks for clarifying @neild. I'll abandon my Gerrit change then.