Known Issue: proto.MarshalTextString cannot handle custom types with Bytes method

[ericlagergren]

If a custom type implements proto.raw (i.e., has a Bytes() []byte method) proto.TextMarshaler.writeStruct will call it, causing invalid bytes to be written.

For example, I have a UUID type with a Bytes() []byte method which returns the UUID's text format as a slice of bytes (it reduces allocations for other methods like MarshalText).

I use this as a custom type and when I do I get the errors seen here.

I'm not sure what your take is on this. Either there should be some docs (I know you said before custom type has issues, so if I could help let me know) or instead of Bytes() []byte the interface should be type raw interface { bytes() []byte } so no external types can implement it.

[ericlagergren]

Oh. And this is tangentially related if a custom type implements encoding.TextMarshaler: since proto.TextMarshaler.Text does not check the error return of the call to Marshal, if the custom type's MarshalText method returns an error the test will fail.

session: <
  id: "Kp3ro_dqJvf4FHTcqTHbsjLMCl8NYkZdWpfu8jqKLTWt2Z2CBqwoD3poXhYYaXK-CgI9tSUbGndFpFS6S0bRlg=="
  token_hash: "e\260\226N)\343\242\322\262Q\263\002\227\005\245\244\264l\035\025\014c\314!\"\010?7\265\253\201\372b\327\274>>\235m\303\rA\203\302\236\006\245"
  principal: <
    id: "\366\211I\201\327x\221 ._w\273l\276\177\020"
    name: "jQsrF340cm7mPOcG1uRvsSeP1SU4aLCvoE5XcIdjgFGi50gUaUMS0uiHqR3ak2OU"
    type: sermoadmin
    domain: "hI4QYd5i1FMJlbFl"
    template: "B0pgKupYaI98wljnt9DoDkgHTlJR6FZnL3ypNEgpKcd2Z4Frp0uointY9hX17i"
  >
  email: "ZsvtMVQ"
  user_id: "\231\350l$0:U\315Y\022\025\r5\276pr"
  first_name: "60pN7eVYbkykbKISujGrCqrOE224vlyFX1EhQU2rA7WTUq96OsQolqR69KdYvSbWCMmL5oKNwT9"
  start: <
    seconds: 5253274347
    nanos: 988209534
  >
  last: <
    seconds: 6963835369
    nanos: 876487934
  >
  end: <
    seconds: 7489709870
    nanos: 752888569
  >
  remember: true
  addr: <

--- FAIL: TestSignInReplyProtoText (0.01s)
    authpbpb_test.go:1192: seed = 1474061698397926986, err = line 27: unknown field name "" in ip.IP

[ericlagergren]

Actually, even if MarshalText does not return an error it breaks the test because:

 addr: <
    239.66.234.218>
  tok: "9\"\013\010\351\317\032\317\244\351\362\344\246\230\3174*\253\275\\\016\262\005\177\242p|\361.\016\343\227\301\215p~\333w\327\224r\3209\375\3267h\004\303\t\327\024\260w\020\350\357/9\300\"\221\251\030\247\324u(\201ER\332\3423GM*\321\271D\034H)^\030\nErc\261\240\027\035yv!"
>
cookie: <
  id: "Sq-TTKnR6hvT8185eIXzoGYtkAkb701gYq-AsY_sGj8yXDndvhpOdZtzDt2bXQ0X0TqUHMqV4xfDrEQ36phhaw=="
  auth_token: "ZwGidys3beXhnN9P2N0-iOR6X-wVoWk-4tljGUGnhF6oyK7WI27ZQkI-FnB-HOvbGSn6-IcCAboelCVBSvJYuA=="
>

--- FAIL: TestSignInReplyProtoText (0.00s)
    authpbpb_test.go:1192: seed = 1474062550691587189, err = line 28: unknown field name "239.66.234.218" in ip.IP

[awalterschulze]

customtype has its issues, but they are unfixable without breaking backwards compatibility in my opinion.

The raw interface comes all the way from golang/protobuf
https://github.com/golang/protobuf/blob/master/proto/text.go#L174
And I think changing this will break backwards compatibility.
Someone might be using this.

[awalterschulze]

proto.TextMarshaler.Text not checking the error also comes from golang/protobuf
https://github.com/golang/protobuf/blob/master/proto/text.go#L830
And the current function signature does not allow us to return an error.

[ericlagergren]

Yeah, I printf debugged my way to those conclusions. I can add some docs for customtype to let people know of this behavior if you make some (short + brief) bullet points on what to add. 😄

[awalterschulze]

I thought I would just leave this issue open. But if I document the issue I can close it. Great idea :)

[ericlagergren]

Also, tangentially related: I just ran into this today: entitiespbpb_test.go:1632: seed = 1474307054202007660, err = database.Allowed declared custom type sermocrm.com/pkg/database.Allowed but it is not convertible to <nil>

where database.Allowed is a struct and gogoproto.nullable = false

Any ideas before I dig through the code?

[awalterschulze]

Did you import github.com/gogo/protobuf/proto or github.com/golang/protobuf/proto?
In other words how are generated the protobufs?
And I guess you are using github.com/gogo/protobuf/jsonpb.

[ericlagergren]

In my .proto file I import github.com/gogo/protobuf/proto.proto

[awalterschulze]

It seems proto.MessageType returns nil in jsonpb/jsonpb.go

            t := proto.MessageType(prop.CustomType)
            if t == nil || !i.Type().ConvertibleTo(t) {
                return fmt.Errorf("%v declared custom type %s but it is not convertible to %v", v.Type(), prop.CustomType, t)
            }
            pm = i.Convert(t).Interface().(proto.Message)

[ericlagergren]

Yep, I actually am on that line right now. :)

[awalterschulze]

Why it returns nil and other don't is a mystery to me at this point in time.

[awalterschulze]

Can you maybe tell me the underlying type of sermocrm.com/pkg/database.Allowed ?

[ericlagergren]

It doesn't implement proto.Message, so perhaps that's why?

type Allowed struct {
    input string
    tables []string
    names []string
}

[awalterschulze]

My customtypes don't either.

[ericlagergren]

Try making a struct with nullable = false.

[awalterschulze]

Done
https://github.com/gogo/protobuf/blob/master/test/thetest.proto#L239
https://github.com/gogo/protobuf/blob/master/test/thetestpb_test.go#L4971

[ericlagergren]

I meant something like

type Foo struct {}

message Bar {
    bytes foo = 1 [(gogoproto.customtype = "Foo"), (gogoproto.nullable = false)];
}

which is essentially what I'm trying to do.

[awalterschulze]

Fair so a struct. Yes that is also the only way we will get into that if.
Hmmm.
I wonder why the code is like this is the first place.

[ericlagergren]

Do you mean why my code is like that?

[awalterschulze]

No, my code.

[ericlagergren]

Ah, gotcha.

[awalterschulze]

I think the only thing to really to do is call MarshalJSON

pm, ok := iface.(proto.Message)
        if !ok {
            if prop.CustomType == "" {
                return fmt.Errorf("%v does not implement proto.Message", v.Type())
            }
                        jsonface, ok := iface.(interface { MarshalJSON() ([]byte, error) })
            if !ok {
                return fmt.Errorf("%v declared custom type %s but does not implement MarshalJSON", v.Type(), prop.CustomType)
            }
                        data, err := jsonface.MarshalJSON()
                        if err != nil { return err }
                        return out.write(data)
        }
...

[awalterschulze]

Could you maybe try if that works for you?

[ericlagergren]

Why not just call json.Marshal on iface?

[awalterschulze]

fair enough :) Does it the modified code work for you?
Maybe you would like to submit a pull request with a test?

[ericlagergren]

haha, I suppose after making a handful of issues the last couple days submitting a pr is the least I could do ;)

[awalterschulze]

Well then you can at least get some credit for all the work you have done. So you can also add yourself and/or your company to the AUTHORS and/or CONTRIBUTORS as appropriate.

[awalterschulze]

This issue is related #147

[awalterschulze]

Docs made a31fa02

[ericlagergren]

Slight issue: if the struct doesn't return {"this": "notation"} when json.Marshal is called then ../jsonpb/jsonpb.go breaks because it'll try to marshal, e.g., a string into map[string]json.RawMessage.

This happens b/c I have a type T struct{} which has a MarshalJSON message that returns a base64-encoded string instead of the "typical" JSON representation of a Go struct.

[awalterschulze]

Could you make a new issue for the jsonpb case please?

[ericlagergren]

yep