gogo / protobuf

[Deprecated] Protocol Buffers for Go with Gadgets
Other
5.66k stars 808 forks source link

jsonpb: unmarshalling of custom []byte type #220

Closed FZambia closed 7 years ago

FZambia commented 7 years ago

Hi, I just came across an issue with custom type. I saw current custom type issues but I have not understood is my case different or not.

I have custom Raw type that behaves the same way as json.RawMessage, so it's a new type derived from []byte.

type Raw []byte

My struct that uses this type looks like:

type ClientInfo struct {
    DefaultInfo *github_com_centrifugal_centrifugo_libcentrifugo_raw.Raw `protobuf:"bytes,3,opt,name=DefaultInfo,proto3,customtype=github.com/centrifugal/centrifugo/libcentrifugo/raw.Raw" json:"default_info,omitempty"`
}

I just updated gogoprotobuf library (previously I used proto2, now moving to proto3) and my autogenerated tests start failing.

func TestClientInfoJSON(t *testing.T) {
    seed := time.Now().UnixNano()
    popr := math_rand.New(math_rand.NewSource(seed))
    p := NewPopulatedClientInfo(popr, true)
    marshaler := github_com_gogo_protobuf_jsonpb.Marshaler{}
    jsondata, err := marshaler.MarshalToString(p)
    println(string(jsondata))
    if err != nil {
        t.Fatalf("seed = %d, err = %v", seed, err)
    }
    msg := &ClientInfo{}
    err = github_com_gogo_protobuf_jsonpb.UnmarshalString(jsondata, msg)
    if err != nil {
        t.Fatalf("seed = %d, err = %v", seed, err)
    }
    if !p.Equal(msg) {
        t.Fatalf("seed = %d, %#v !Json Equal %#v", seed, msg, p)
    }
}

Raw message encoded as base64 so jsondata looks like:

{"DefaultInfo":"xLs8dWviSSvoKOKq5o08GBYSZOCHpg7xLUe3tzCtdYdZ8krjOLvP345vsQ=="}

But after unmarshalling from string equality test fails because DefaultInfo not unmarshalled from that base64 string to []byte.

As far as I understand my tests were passing previously because jsonpb did not even try to encode bytes before cd72cb90120d2d7157d866d5b27cfc122efb4d40, right?

Is this issue related to #201 ?

awalterschulze commented 7 years ago

Do you have an UnmarshalJSON method for your customtype Raw?

awalterschulze commented 7 years ago

I think later this code was added

            if _, ok := outVal.(interface {
                UnmarshalJSON([]byte) error
            }); ok {
                if err := json.Unmarshal(inputValue, outVal); err != nil {
                    return err
                }
                target.Set(outRef.Elem())
                return nil
            }

To check for a customtype with an underlying []byte type

FZambia commented 7 years ago

Do you have an UnmarshalJSON method for your customtype Raw

Here is this Raw type - so yes, it has UnmarshalJSON method and its called.

Btw looks like MarshalJSON never called as default info encoded as base64 after MarshalToString

FZambia commented 7 years ago

This line is called when marshalling ClientInfo to string but it does not call my MarshalJSON method. I am not too familiar with reflection, so can't figure out why this happens:)

awalterschulze commented 7 years ago

Have you tried removing the pointer from Raw for the MarshalJSON method? As the test Uuid type does here https://github.com/gogo/protobuf/blob/master/test/uuid.go#L86

awalterschulze commented 7 years ago

So basically you will have

// MarshalJSON returns *r as the JSON encoding of r.
func (r Raw) MarshalJSON() ([]byte, error) {
    return r, nil
}
FZambia commented 7 years ago

Yep, I tried this before - and tests were failing.. But what I tried now is removing pointer as you suggested and also changed marshaling to:

    // Default handling defers to the encoding/json library.
    var b []byte
    var err error

    if m, ok := v.Interface().(json.Marshaler); ok {
        b, err = m.MarshalJSON()
    } else {
        b, err = json.Marshal(v.Interface())
    }
    if err != nil {
        return err
    }

    /*
        b, err := json.Marshal(v.Interface())
        if err != nil {
            return err
        }
    */

And now tests pass...

FZambia commented 7 years ago

update: sorry, looks like I did a mistake, removing pointer is enough, I will check again)) Give me some time

awalterschulze commented 7 years ago

Ok great. So is removing the pointer an option for you, i.o.w. can we close the issue?

FZambia commented 7 years ago

Yes, sorry for disturbing... Removing pointer is enough, looks like when I tried to do this before I also broke something in another place attempting to find solution.

Actually when I did this type I saw that uuid example had no pointer in MarshalJSON but RawMessage in std lib has. So I decided to write it with pointer.

Thanks!

awalterschulze commented 7 years ago

I have to say that this solution is not ideal and causes confusion, but it is what we have. Have you tried using casttype instead of customtype?

It might be possible for you to delete quite a bit of code.

FZambia commented 7 years ago

No, even have not known about it before this moment, thanks - will try to play with it.

awalterschulze commented 7 years ago

I think it works better than customtype.

FZambia commented 7 years ago

Hi, just tried to use casttype for one of the fields.

string Channel = 3 [(gogoproto.casttype) = "Channel", (gogoproto.jsontag) = "channel"];

where Channel is:

type Channel string

There is a generated function randStringMessage - it returns string so code does not compile because of wrong type used in NewPopulatedX func:

func randStringMessage(r randyMessage) string {
    v3 := r.Intn(100)
    tmps := make([]rune, v3)
    for i := 0; i < v3; i++ {
        tmps[i] = randUTF8RuneMessage(r)
    }
    return string(tmps)
}

func NewPopulatedMessage(r randyMessage, easy bool) *Message {
    this := &Message{}
    ...
    this.Channel = randStringMessage(r)
    ...
    return this
}

Looks like some addition type casting needed here

this.Channel = Channel(randStringMessage(r))
awalterschulze commented 7 years ago

This should fix the issue https://github.com/gogo/protobuf/commit/a9cd0c35b97daf74d0ebf3514c5254814b2703b4