Proposal: Add `Override` mode to `proto.Merge`

[marnixbouhuis]

I would like to introduce a change to the proto.Merge method that would allow callers to specify if they want to merge lists / maps by appending them to each other or by overwriting.

Why

In some cases it is nice to be able to supply default values for a protobuf message. Imagine a case where an application has their configuration defined using protobuf. The user of the application could then overwrite parts of this default configuration using a config file. The modified configuration would then be overlayed over the default configuration file. Overwriting lists and maps currently is not possible using proto.Merge, causing this function not to be usable for this use case.

Current behaviour

Merging two protobuf messages will causes values from lists and maps in the src to be appended to the dst.

Current behaviour is:

dst := &Example{
    Field: "example",
    Map: map[string]string{
        "key1": "value1",
    },
    Slice: []string{"value1"},
    Nested: &Example{
        Field: "example",
        Slice: []string{"value1"},
        Map: map[string]string{
            "some-key": "some-value",
        },
    },
}

src := &Example{
    Field: "example2",
    Map: map[string]string{
        "key2": "value2",
    },
    Slice: []string{"value2"},
    Nested: &Example{
        Field: "example2",
        Slice: []string{},
    },
}

proto.Merge(dst, src)

// Resulting dst would be:
/*
    &Example{
        Field: "example2",
        Map: map[string]string{
            "key1": "value1", // It's not possible to clear "key1", you can only overwrite its value.
            "key2": "value2",
        },
        Slice: []string{"value1", "value2"}, // It's not possible to clear "value1"
        Nested: &Example{
            Field: "example2",
            Slice: []string{"value1"}, // It's not possible to clear "value1"
            Map: map[string]string{
                "some-key": "some-value",
            },
        },
    }
*/  

New behaviour with this option enabled.

I want to propose a option for proto.Merge that would change this behaviour for maps / lists. Instead of appending the dst field would be overwritten if the src message has this field. This would result in the following behaviour:

// Resulting dst would be:
/*
    &Example{
        Field: "example2",
        Map: map[string]string{  // Field is overwritten by the list from src.
            "key2": "value2",
        },
        Slice: []string{"value2"}, // Field is overwritten by the list from src.
        Nested: &Example{
            Field: "example2",
            Slice: []string{}, // Replaced with empty slice since, the src message has this field populated.
            Map: map[string]string{ // Unchanged since the src does not have this field populated.
                "some-key": "some-value",
            },
        },
    }
*/  

How

In proto/merge.go, mergeOptions used to be exported for an option to shallow clone messages. This option was later removed in https://go-review.googlesource.com/c/protobuf/+/219145. I suggest the mergeOptions struct is re-exported and a Override option is added:

// In proto/merge.go
// https://github.com/protocolbuffers/protobuf-go/blob/09393c19510d9e0ee2040ff18726c006026a089e/proto/merge.go#L64
type MergeOptions struct{
    pragma.NoUnkeyedLiterals

    Override bool
}

This can then be used in func (o mergeOptions) mergeMessage(dst, src protoreflect.Message) to determine if the list / map should be cleared before merging in any values from the src message:

// Snippet from: proto/merge.go
// https://github.com/protocolbuffers/protobuf-go/blob/09393c19510d9e0ee2040ff18726c006026a089e/proto/merge.go#L83-L97
...
    src.Range(func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool {
        switch {
        case fd.IsList():
            // Here we would clear the dst list if required, or we could use a dst.Set() instead
            o.mergeList(dst.Mutable(fd).List(), v.List(), fd)
        case fd.IsMap():
            // Here we would clear the dst map if required, or we could use a dst.Set() instead
            o.mergeMap(dst.Mutable(fd).Map(), v.Map(), fd.MapValue())
        case fd.Message() != nil:
            o.mergeMessage(dst.Mutable(fd).Message(), v.Message())
        case fd.Kind() == protoreflect.BytesKind:
            dst.Set(fd, o.cloneBytes(v))
        default:
            dst.Set(fd, v)
        }
        return true
    })
...

---

For me this seems like a fairly trivial change without breaking the public API. Any feedback is much appreciated!

[puellanivis]

https://github.com/golang/protobuf/issues/1488 This has been discussed before

[marnixbouhuis]

Thanks for the reference, closing this one.