Vote For New Feature: Support BinaryMarshaler and TextMarshaler for custom types about protobuf HOT 19 CLOSED

From [email protected] on September 30, 2014 02:22:48

I was able to at least prove my point altering decode_gogo.go like so:

func (o *Buffer) dec_custom_ref_bytes(p *Properties, base structPointer) error {
b, err := o.DecodeRawBytes(true)
if err != nil {
return err
}
i := reflect.New(p.ctype).Interface()
switch (i).(type) {
case Unmarshaler:
if err := (i).(Unmarshaler).Unmarshal(b); err != nil {
return err
}
case encoding.BinaryUnmarshaler:
if err := (i).(encoding.BinaryUnmarshaler).UnmarshalBinary(b); err != nil {
return err
}
}
if i != nil {
setCustomType(base, p.field, i)
}
return nil
}

and encode_gogo.go like so:

func (o *Buffer) enc_custom_ref_bytes(p *Properties, base structPointer) error {
custom := structPointer_InterfaceAt(base, p.field, p.ctype)
var (
data []byte
err error
)
switch custom.(type) {
case Marshaler:
data, err = custom.(Marshaler).Marshal()
case encoding.BinaryMarshaler:
data, err = custom.(encoding.BinaryMarshaler).MarshalBinary()
}
if err != nil {
return err
}
if data == nil {
return ErrNil
}
o.buf = append(o.buf, p.tagcode...)
o.EncodeRawBytes(data)
return nil
}

This way we would not break the current API, allowing people to use custom proto.Marshal and proto.Unmarshal (check for these first) but we could then fall back to the stdlib interfaces.

from protobuf.

From awalterschulze on September 30, 2014 02:56:13

What do you propose for the generated code.

Currently a Size() int and a MarshalTo(buf []byte) int, error methods are expected to be implemented.

I don't know how I feel about doing a type check in generated code.

Why not just wrap the type in an alias type?

What about the Equal method and the NewPopulatedXXX function for tests.
I think the generated tests are especially important when using custom types.

I am not saying no, yet, I just think there are a few design questions left.

Labels: -Type-Defect Type-Enhancement

from protobuf.

From [email protected] on September 30, 2014 06:57:28

I would rather not wrap it since I'm using the "real" type all over my application and I don't want to repeatedly converting to and from a wrapper type, my use case for custom gogoproto type is to be able to simply work with my existing type with no hassle.

My mistake! I have simply used gogoprotobuf without those plugins, I see your point. No, we probably don't want to have a type switch in the generated code.

It could be solved anyhow, if we want to. We could just put some logic in the code generation. Something like (pseudo):
if not implements proto.marshaler
check if implements encoding.BinaryUnmarshaler
generate .Size() func that unmarshals into a []byte and return its len
generate .MarshalTo() func using encoding.BinaryUnmarshaler

It should work, I can try and see if I can make a rough draft.

from protobuf.

From awalterschulze on September 30, 2014 08:17:32

A type assert with an ok, is basically a type switch with one element.
Checking the type in the generated code is a little weird.

Checking the type at generation time will cause a weird coupling.

I am a little skeptical about finding a good solution here, but I am thinking.

from protobuf.

From awalterschulze on October 12, 2014 23:07:35

We could maybe define another extension.

custombinary

And allow you to instead of writing customtype=myType write custombinary=myType
Equal might work by simply Marshaling both sides and checking whether the marshaled bytes are equal.

But I don't know how NewPopulatedMyType would work for a package you don't have access to?

from protobuf.

From awalterschulze on November 25, 2014 10:19:05

Maybe the NewPopulatedMyType function could assume that the Populate function will be written in the package the protocol buffer is generated in.
But then you would have to rewrite the function for every package where you reuse the type, ug :(

from protobuf.

From awalterschulze on November 25, 2014 10:22:23

I am currently at a loss at how to do this right.

from protobuf.

From [email protected] on November 27, 2014 23:52:58

Yeah I know, after digging into your actual implementation I also find this very tricky to achive. I still think it would be nice and idiomatic but I honestly don't know if it would fit into the rest of your code...

from protobuf.

From awalterschulze on November 28, 2014 00:01:16

I agree.

I am going to close this, but if you or anyone figures out a way to do this please don't refrain from reopening this case.

I am sorry I couldn't figure this out.

Status: WontFix

from protobuf.

I think I found a way to support NewPopulated or at least the intention behind it better.
http://golang.org/pkg/testing/quick/#Generator

from protobuf.

But I think I should first check to see if it is really used in the golang standard library

from protobuf.

Personally I've never used Equal or Populate, but encounter the need for custom marshaling every now and then. For me, personally, it's not the end of the world if types using MarshalBinary won't work with Equal or Populate.

from protobuf.

If customtype does not work with Equal and Populate is also does not work for any test generation.
I think this is a little scary, since your custom type is not tested anywhere else.

I think there are a couple of ideas about Equal, Marshaling and then checking, reflect.DeepEqual after finding that the type does not implement the equaler interface.

I hope testing/quick.Generator is used a lot in the standard library, since then this is a perfect replacement for Populate.

I just haven't had the time to check.

from protobuf.

Ok quick.Generator does not look very popular in the standard library :(

grep -R "Generate(rand" . ./crypto/tls/handshake_messages_test.go:func (_clientHelloMsg) Generate(rand *rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_serverHelloMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_certificateMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_certificateRequestMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_certificateVerifyMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_certificateStatusMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_clientKeyExchangeMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_finishedMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_nextProtoMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_newSessionTicketMsg) Generate(rand _rand.Rand, size int) reflect.Value {
./crypto/tls/handshake_messages_test.go:func (_sessionState) Generate(rand *rand.Rand, size int) reflect.Value {
./testing/quick/quick.go: Generate(rand *rand.Rand, size int) reflect.Value
./testing/quick/quick.go: return m.Generate(rand, complexSize), true

from protobuf.

Ok I propose a new extension custombinary and customtext.
This is if someone is interested in implementing this.

from protobuf.

Sorry for the super long delay.

from protobuf.

This also assumes that NewPopulated will not be implemented and in the case that a random value needs to be generated an empty one will have to do.
Equal with call MarshalBinary or MarshalText and simply compare the result.

from protobuf.

If anyone still needs this, please drop a comment.

from protobuf.

no comments for more than a year, closing.

from protobuf.