Proposal: built in support for protobuf wrapper types
zaquestion opened this issue · 8 comments
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
}
}
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
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)
this is example code https://github.com/unistack-org/micro/blob/v3/util/reflect/struct.go#L390
@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?
In you case you depend on protobuf library and code base, with my -!only reflection used.
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.
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)