Skip to content

updating wrapper.StringValue with proto.Merge; nil pointer vs zero-value #1050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
donotnoot opened this issue Mar 4, 2020 · 3 comments
Closed

Comments

@donotnoot
Copy link

donotnoot commented Mar 4, 2020

Hello all,

Given this message:

message A {
	google.protobuf.StringValue S = 1;
}

this code:

initialMessage := &pb.A{S: &wrappers.StringValue{Value: "Some string"}}

updateWith := &pb.A{S: &wrappers.StringValue{Value: ""}}

proto.Merge(initialMessage, updateWith)

fmt.Printf("the new value is %q\n", initialMessage)

will output "Some string".

Is there any way at all to make it output ""? In other words, is there any way to update the S wrapper field it with its corresponding zero value if this has to be merged into an existing struct? I thought that was the whole point of the wrapper types.

For context, I'm pulling a record off storage, updating only some fields, and storing it back. The storage that I'm using only supports PUT operations so I can't just send some values, I have to send the whole message, hence the need to merge.

Regards

@neild
Copy link
Contributor

neild commented Mar 4, 2020

I'm afraid Merge doesn't do what you want (as you've observed). It's one of the places where the proto3 value wrappers are an imperfect replacement for proto2 optional fields.

Java's FieldMaskUtil provides a helper function which can merge a specified set of fields, which partially addresses this use case (although you then have to manage the list of fields to merge yourself somehow). We don't currently have an officially-supported Go equivalent (#398 tracks the desire for one), unfortunately, but building one using protoreflect would be fairly straightforward.

@donotnoot
Copy link
Author

@neild That's what I suspected, then! I'm guessing there are no unofficial equivalents either?

Either way, I'm not particularly proud of this piece of code, but just in case anyone stumbles upon this, you can always do something like this:

package merge

import (
	"bytes"
	"encoding/json"

	"github.com/golang/protobuf/jsonpb"
	"github.com/golang/protobuf/proto"
	"github.com/imdario/mergo"
)

// Merge is the slowest, unsafest way to merge two proto messages preserving
// wrapper types' zero values.
func Merge(dst, src proto.Message) {
	marshaler := jsonpb.Marshaler{}

	sourceBuf := &bytes.Buffer{}
	err := marshaler.Marshal(sourceBuf, src)
	if err != nil {
		panic(err)
	}

	destinationBuf := &bytes.Buffer{}
	err = marshaler.Marshal(destinationBuf, dst)
	if err != nil {
		panic(err)
	}

	sourceMap := make(map[string]interface{})
	destinationMap := make(map[string]interface{})

	err = json.NewDecoder(sourceBuf).Decode(&sourceMap)
	if err != nil {
		panic(err)
	}
	err = json.NewDecoder(destinationBuf).Decode(&destinationMap)
	if err != nil {
		panic(err)
	}

	if err := mergo.Merge(&destinationMap, sourceMap, mergo.WithOverride); err != nil {
		panic(err)
	}

	finalDestinationBuffer := &bytes.Buffer{}
	err = json.NewEncoder(finalDestinationBuffer).Encode(destinationMap)
	if err != nil {
		panic(err)
	}

	dst.Reset()
	err = jsonpb.Unmarshal(finalDestinationBuffer, dst)
	if err != nil {
		panic(err)
	}
}

func deleteNils(in map[string]interface{}) {
	for k, v := range in {
		if m, ok := v.(map[string]interface{}); ok {
			deleteNils(m)
		}
		if v == nil {
			delete(in, k)
		}
	}
}

As it says in the comment, it truly is absolutely horrible and I'll probably never get hired again if anyone ever sees this but it definitely works!

@dsnet
Copy link
Member

dsnet commented Mar 18, 2020

Closing this as working as intended since the proto.Merge is following the expected behavior according to the protobuf data model.

@neild's suggestion of using FieldMasks to select what to merge (or not to merge) can be considered when we address #398.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants