Proposal: built in support for protobuf wrapper types

[zaquestion]

Hiya, firstly, thank you for this project, of the bunch out there offering similar functionality this package is the only one I found that really does the job right and offered enough configuration to handle edge cases or quirks of my particular needs.

One such need involved merging protobuf's wrapper types
Ref: https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto

These types are really handy for go projects as the allow differentiating between "not set" and zero values in api parameters.

Representation in Go for BoolValue

type BoolValue struct {

	// The [bool](https://pkg.go.dev/builtin#bool) value.
	Value bool `protobuf:"varint,1,opt,name=value,proto3" json:"value,omitempty"`
	// contains filtered or unexported fields
}

I was running into an issue with mergo.Merge when merging a BoolValue field. Both the source and dest fields had the field set (non nil). However the underlying Value in the destination was false whereas the source was true. Mergo understandably thought the right action was to overwrite Value with true being a non-zero value (as it went to merge at the field level of the struct), however the presence of a non-nil wrapper type in the destination means it was explicitly set and shouldn't be overwritten.

mergo Transformers to the rescue! Thankfully y'all have a great mechanism for overriding this behavior and just as I thought I was screwed (proto.Merge is garbage btw), I was able to write a pretty simply transformer that resolved the issue. This ticket is mostly to say thanks for the great project and to see if adding something like the below transformer to the project would be useful.

type wrapperspbTransformer struct{}

func (t wrapperspbTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Value) error {
	switch typ {
	case reflect.TypeOf(&wrapperspb.BoolValue{}),
		reflect.TypeOf(&wrapperspb.StringValue{}),
		reflect.TypeOf(&wrapperspb.BytesValue{}),
		reflect.TypeOf(&wrapperspb.DoubleValue{}),
		reflect.TypeOf(&wrapperspb.FloatValue{}),
		reflect.TypeOf(&wrapperspb.Int64Value{}),
		reflect.TypeOf(&wrapperspb.UInt64Value{}),
		reflect.TypeOf(&wrapperspb.Int32Value{}),
		reflect.TypeOf(&wrapperspb.UInt32Value{}):
		return func(dst, src reflect.Value) error {
			// only overwrite a wrapper type if the destination
			// nil. This prevents mergo from recursing into the
			// wrapper type itself and attempting to merge against
			// the primative `value` field, which means that zero
			// values would get overwritten despite being
			// explicitly set through the destination wrapper.
			//
			// Basically this transformer forces wrapperpb types to merge correctly
			if dst.CanSet() && dst.IsNil() {
				dst.Set(src)
			}
			return nil
		}
	default:
		return nil
	}
}

[xscode-auto-reply]

Thanks for opening a new issue. The team has been notified and will review it as soon as possible.
For urgent issues and priority support, visit https://xscode.com/imdario/mergo

[vtolstov]

I'm prefer to avoid adding deps for protobuf. This types easy can be set via reflection (switch on Type().Kind.String and fill Value struct field)

[vtolstov]

this is example code https://github.com/unistack-org/micro/blob/v3/util/reflect/struct.go#L390

[zaquestion]

@vtolstov seems reasonable to leave the dependency out of this library, I'll close this out.

To your comment about this being doable with reflection and the example you linked. Are you suggesting that's a better alternative to the codesnippet I posted above? Is there some drawback to approach that I'm missing here?

[vtolstov]

In you case you depend on protobuf library and code base, with my -!only reflection used.

[zaquestion]

I see, so you wouldn't mind adding in a transformer that checked the types by string and set the values? You just don't want to directly depend on the protobuf library. In that case I'll reopen this.

[zaquestion]

Should it still be implemented as a transformer, or should this logic be built into the default merging logic? Assuming we'd want the former, but I could see arguments for the later (such as that the merging behavior for these types would be more correct by default given their purpose)

[darccio]

It was already implemented in #211 :)